From 6a80184cdba319b407fb4215d821f6aeb144ad23 Mon Sep 17 00:00:00 2001 From: Ping Li Date: Mon, 23 Nov 2020 16:10:34 -0800 Subject: [PATCH] disp: msm: sde: fix potential race condition Move the hist irq handling out of callback function, i.e., the hw interrupt irq_lock context, to avoid dead lock between crtc spin_lock and irq_lock. This change also extends crtc spin_lock coverage in _sde_cp_crtc_enable_hist_irq to prevent null pointer dereference on event node, which can be deleted during crtc event de-registration. Change-Id: Iadaed54ab93c4c4abe065a8762d2addccb0c65c6 Signed-off-by: Ping Li --- msm/sde/sde_color_processing.c | 120 ++++++++++++++++--------- msm/sde/sde_crtc.h | 4 +- msm/sde/sde_hw_color_processing_v1_7.c | 12 +-- 3 files changed, 87 insertions(+), 49 deletions(-) diff --git a/msm/sde/sde_color_processing.c b/msm/sde/sde_color_processing.c index 42b319bbf073..f4b3cf7b33e3 100644 --- a/msm/sde/sde_color_processing.c +++ b/msm/sde/sde_color_processing.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-only /* - * Copyright (c) 2016-2020, The Linux Foundation. All rights reserved. + * Copyright (c) 2016-2021, The Linux Foundation. All rights reserved. */ #define pr_fmt(fmt) "%s: " fmt, __func__ @@ -1435,7 +1435,7 @@ static void _sde_cp_crtc_enable_hist_irq(struct sde_crtc *sde_crtc) struct sde_hw_dspp *hw_dspp = NULL; struct sde_crtc_irq_info *node = NULL; int i, irq_idx, ret = 0; - unsigned long flags; + unsigned long flags, state_flags; if (!crtc_drm) { DRM_ERROR("invalid crtc %pK\n", crtc_drm); @@ -1465,12 +1465,13 @@ static void _sde_cp_crtc_enable_hist_irq(struct sde_crtc *sde_crtc) spin_lock_irqsave(&sde_crtc->spin_lock, flags); node = _sde_cp_get_intr_node(DRM_EVENT_HISTOGRAM, sde_crtc); - spin_unlock_irqrestore(&sde_crtc->spin_lock, flags); - if (!node) + if (!node) { + spin_unlock_irqrestore(&sde_crtc->spin_lock, flags); return; + } - spin_lock_irqsave(&node->state_lock, flags); + spin_lock_irqsave(&node->state_lock, state_flags); if (node->state == IRQ_DISABLED) { ret = sde_core_irq_enable(kms, &irq_idx, 1); if (ret) @@ -1478,7 +1479,8 @@ static void _sde_cp_crtc_enable_hist_irq(struct sde_crtc *sde_crtc) else node->state = IRQ_ENABLED; } - spin_unlock_irqrestore(&node->state_lock, flags); + spin_unlock_irqrestore(&node->state_lock, state_flags); + spin_unlock_irqrestore(&sde_crtc->spin_lock, flags); } static int sde_cp_crtc_checkfeature(struct sde_cp_node *prop_node, @@ -2305,6 +2307,7 @@ void sde_cp_crtc_destroy_properties(struct drm_crtc *crtc) } sde_crtc->ltm_buffer_cnt = 0; sde_crtc->ltm_hist_en = false; + sde_crtc->hist_irq_idx = -1; mutex_destroy(&sde_crtc->crtc_cp_lock); INIT_LIST_HEAD(&sde_crtc->active_list); @@ -2399,6 +2402,7 @@ void sde_cp_crtc_clear(struct drm_crtc *crtc) } sde_crtc->ltm_buffer_cnt = 0; sde_crtc->ltm_hist_en = false; + sde_crtc->hist_irq_idx = -1; INIT_LIST_HEAD(&sde_crtc->ltm_buf_free); INIT_LIST_HEAD(&sde_crtc->ltm_buf_busy); } @@ -3229,45 +3233,20 @@ static void sde_cp_hist_interrupt_cb(void *arg, int irq_idx) struct sde_crtc *crtc = arg; struct drm_crtc *crtc_drm = &crtc->base; struct sde_hw_dspp *hw_dspp; - struct sde_kms *kms; - struct sde_crtc_irq_info *node = NULL; + u32 lock_hist = 1; u32 i; - int ret = 0; - unsigned long flags; - - /* disable histogram irq */ - kms = get_kms(crtc_drm); - spin_lock_irqsave(&crtc->spin_lock, flags); - node = _sde_cp_get_intr_node(DRM_EVENT_HISTOGRAM, crtc); - spin_unlock_irqrestore(&crtc->spin_lock, flags); - - if (!node) { - DRM_DEBUG_DRIVER("cannot find histogram event node in crtc\n"); - return; - } - - spin_lock_irqsave(&node->state_lock, flags); - if (node->state == IRQ_ENABLED) { - if (sde_core_irq_disable_nolock(kms, irq_idx)) { - DRM_ERROR("failed to disable irq %d, ret %d\n", - irq_idx, ret); - spin_unlock_irqrestore(&node->state_lock, flags); - return; - } - node->state = IRQ_DISABLED; - } - spin_unlock_irqrestore(&node->state_lock, flags); /* lock histogram buffer */ for (i = 0; i < crtc->num_mixers; i++) { hw_dspp = crtc->mixers[i].hw_dspp; if (hw_dspp && hw_dspp->ops.lock_histogram) - hw_dspp->ops.lock_histogram(hw_dspp, NULL); + hw_dspp->ops.lock_histogram(hw_dspp, &lock_hist); } + crtc->hist_irq_idx = irq_idx; /* notify histogram event */ sde_crtc_event_queue(crtc_drm, sde_cp_notify_hist_event, - NULL, true); + &crtc->hist_irq_idx, true); } static void sde_cp_notify_hist_event(struct drm_crtc *crtc_drm, void *arg) @@ -3277,11 +3256,13 @@ static void sde_cp_notify_hist_event(struct drm_crtc *crtc_drm, void *arg) struct drm_event event; struct drm_msm_hist *hist_data; struct sde_kms *kms; - int ret; - u32 i; + struct sde_crtc_irq_info *node = NULL; + unsigned long flags, state_flags; + int ret, irq_idx; + u32 i, lock_hist = 0; - if (!crtc_drm) { - DRM_ERROR("invalid crtc %pK\n", crtc_drm); + if (!crtc_drm || !arg) { + DRM_ERROR("invalid drm crtc %pK or arg %pK\n", crtc_drm, arg); return; } @@ -3291,15 +3272,70 @@ static void sde_cp_notify_hist_event(struct drm_crtc *crtc_drm, void *arg) return; } - if (!crtc->hist_blob) - return; - kms = get_kms(crtc_drm); if (!kms || !kms->dev) { SDE_ERROR("invalid arg(s)\n"); return; } + /* disable histogram irq */ + spin_lock_irqsave(&crtc->spin_lock, flags); + node = _sde_cp_get_intr_node(DRM_EVENT_HISTOGRAM, crtc); + + if (!node) { + spin_unlock_irqrestore(&crtc->spin_lock, flags); + DRM_DEBUG_DRIVER("cannot find histogram event node in crtc\n"); + /* unlock histogram */ + ret = pm_runtime_get_sync(kms->dev->dev); + if (ret < 0) { + SDE_ERROR("failed to enable power resource %d\n", ret); + SDE_EVT32(ret, SDE_EVTLOG_ERROR); + return; + } + for (i = 0; i < crtc->num_mixers; i++) { + hw_dspp = crtc->mixers[i].hw_dspp; + if (hw_dspp && hw_dspp->ops.lock_histogram) + hw_dspp->ops.lock_histogram(hw_dspp, + &lock_hist); + } + pm_runtime_put_sync(kms->dev->dev); + return; + } + + irq_idx = *(int *)arg; + spin_lock_irqsave(&node->state_lock, state_flags); + if (node->state == IRQ_ENABLED) { + ret = sde_core_irq_disable_nolock(kms, irq_idx); + if (ret) { + DRM_ERROR("failed to disable irq %d, ret %d\n", + irq_idx, ret); + spin_unlock_irqrestore(&node->state_lock, state_flags); + spin_unlock_irqrestore(&crtc->spin_lock, flags); + ret = pm_runtime_get_sync(kms->dev->dev); + if (ret < 0) { + SDE_ERROR("failed to enable power %d\n", ret); + SDE_EVT32(ret, SDE_EVTLOG_ERROR); + return; + } + + /* unlock histogram */ + for (i = 0; i < crtc->num_mixers; i++) { + hw_dspp = crtc->mixers[i].hw_dspp; + if (hw_dspp && hw_dspp->ops.lock_histogram) + hw_dspp->ops.lock_histogram(hw_dspp, + &lock_hist); + } + pm_runtime_put_sync(kms->dev->dev); + return; + } + node->state = IRQ_DISABLED; + } + spin_unlock_irqrestore(&node->state_lock, state_flags); + spin_unlock_irqrestore(&crtc->spin_lock, flags); + + if (!crtc->hist_blob) + return; + ret = pm_runtime_get_sync(kms->dev->dev); if (ret < 0) { SDE_ERROR("failed to enable power resource %d\n", ret); diff --git a/msm/sde/sde_crtc.h b/msm/sde/sde_crtc.h index f4efaac5578d..7892d50f9e5f 100644 --- a/msm/sde/sde_crtc.h +++ b/msm/sde/sde_crtc.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2015-2020 The Linux Foundation. All rights reserved. + * Copyright (c) 2015-2021 The Linux Foundation. All rights reserved. * Copyright (C) 2013 Red Hat * Author: Rob Clark * @@ -286,6 +286,7 @@ struct sde_crtc_misr_info { * @ltm_buffer_lock : muttx to protect ltm_buffers allcation and free * @ltm_lock : Spinlock to protect ltm buffer_cnt, hist_en and ltm lists * @needs_hw_reset : Initiate a hw ctl reset + * @hist_irq_idx : hist interrupt irq idx * @src_bpp : source bpp used to calculate compression ratio * @target_bpp : target bpp used to calculate compression ratio * @static_cache_read_work: delayed worker to transition cache state to read @@ -374,6 +375,7 @@ struct sde_crtc { struct mutex ltm_buffer_lock; spinlock_t ltm_lock; bool needs_hw_reset; + int hist_irq_idx; int src_bpp; int target_bpp; diff --git a/msm/sde/sde_hw_color_processing_v1_7.c b/msm/sde/sde_hw_color_processing_v1_7.c index e7920320fab2..b39250b9c699 100644 --- a/msm/sde/sde_hw_color_processing_v1_7.c +++ b/msm/sde/sde_hw_color_processing_v1_7.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0-only /* - * Copyright (c) 2016-2019, The Linux Foundation. All rights reserved. + * Copyright (c) 2016-2019, 2021, The Linux Foundation. All rights reserved. */ #include @@ -951,17 +951,17 @@ void sde_read_dspp_hist_v1_7(struct sde_hw_dspp *ctx, void *cfg) void sde_lock_dspp_hist_v1_7(struct sde_hw_dspp *ctx, void *cfg) { - u32 offset_ctl; + u32 offset_ctl, val; - if (!ctx) { - DRM_ERROR("invalid parameters ctx %pK", ctx); + if (!ctx || !cfg) { + DRM_ERROR("invalid parameters ctx %pK cfg %pK", ctx, cfg); return; } offset_ctl = ctx->cap->sblk->hist.base + PA_HIST_CTRL_DSPP_OFF; - /* lock hist buffer */ - SDE_REG_WRITE(&ctx->hw, offset_ctl, 1); + val = (*(u32 *)cfg) & 0x1; + SDE_REG_WRITE(&ctx->hw, offset_ctl, val); } void sde_setup_dspp_dither_v1_7(struct sde_hw_dspp *ctx, void *cfg)