| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <dda1ec958f188216b067fd40a36eb41d@codeaurora.org>
Date: Thu, 15 Oct 2020 14:48:00 +0530
From: mkrishn@...eaurora.org
To: Rob Clark <robdclark@...il.com>
Cc: dri-devel <dri-devel@...ts.freedesktop.org>,
linux-arm-msm <linux-arm-msm@...r.kernel.org>,
freedreno <freedreno@...ts.freedesktop.org>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Sean Paul <seanpaul@...omium.org>,
"Kristian H. Kristensen" <hoegsberg@...omium.org>,
Kalyan Thota <kalyan_t@...eaurora.org>,
Douglas Anderson <dianders@...omium.org>
Subject: Re: [v1] drm/msm: Fix race condition in msm driver with async layer
updates
On 2020-10-14 20:55, Rob Clark wrote:
> On Wed, Oct 14, 2020 at 5:58 AM Krishna Manikandan
> <mkrishn@...eaurora.org> wrote:
>>
>> When there are back to back commits with async cursor update,
>> there is a case where second commit can program the DPU hw
>> blocks while first didn't complete flushing config to HW.
>>
>> Synchronize the compositions such that second commit waits
>> until first commit flushes the composition.
>>
>> This change also introduces per crtc commit lock, such that
>> commits on different crtcs are not blocked by each other.
>>
>> Signed-off-by: Krishna Manikandan <mkrishn@...eaurora.org>
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 1 +
>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h | 1 +
>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 26
>> ++++++++++++++++++++++++
>> drivers/gpu/drm/msm/msm_atomic.c | 35
>> ++++++++++++++++++++++----------
>> drivers/gpu/drm/msm/msm_kms.h | 5 +++++
>> 5 files changed, 57 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> index c2729f7..9024719 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> @@ -1383,6 +1383,7 @@ struct drm_crtc *dpu_crtc_init(struct drm_device
>> *dev, struct drm_plane *plane,
>>
>> /* initialize event handling */
>> spin_lock_init(&dpu_crtc->event_lock);
>> + mutex_init(&dpu_crtc->commit_lock);
>>
>> DPU_DEBUG("%s: successfully initialized crtc\n",
>> dpu_crtc->name);
>> return crtc;
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> index cec3474..1eeb73d 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.h
>> @@ -169,6 +169,7 @@ struct dpu_crtc {
>>
>> /* for handling internal event thread */
>> spinlock_t event_lock;
>> + struct mutex commit_lock;
>>
>> struct dpu_core_perf_params cur_perf;
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> index c0a4d4e..f99ae7a 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -445,6 +445,30 @@ static void dpu_kms_wait_flush(struct msm_kms
>> *kms, unsigned crtc_mask)
>> dpu_kms_wait_for_commit_done(kms, crtc);
>> }
>>
>> +static void dpu_kms_commit_lock(struct msm_kms *kms, unsigned int
>> crtc_mask)
>> +{
>> + struct dpu_kms *dpu_kms = to_dpu_kms(kms);
>> + struct drm_crtc *crtc;
>> + struct dpu_crtc *dpu_crtc;
>> +
>> + for_each_crtc_mask(dpu_kms->dev, crtc, crtc_mask) {
>> + dpu_crtc = to_dpu_crtc(crtc);
>> + mutex_lock(&dpu_crtc->commit_lock);
>> + }
>> +}
>> +
>> +static void dpu_kms_commit_unlock(struct msm_kms *kms, unsigned int
>> crtc_mask)
>> +{
>> + struct dpu_kms *dpu_kms = to_dpu_kms(kms);
>> + struct drm_crtc *crtc;
>> + struct dpu_crtc *dpu_crtc;
>> +
>> + for_each_crtc_mask(dpu_kms->dev, crtc, crtc_mask) {
>> + dpu_crtc = to_dpu_crtc(crtc);
>> + mutex_unlock(&dpu_crtc->commit_lock);
>> + }
>> +}
>> +
>> static int _dpu_kms_initialize_dsi(struct drm_device *dev,
>> struct msm_drm_private *priv,
>> struct dpu_kms *dpu_kms)
>> @@ -738,6 +762,8 @@ static const struct msm_kms_funcs kms_funcs = {
>> #ifdef CONFIG_DEBUG_FS
>> .debugfs_init = dpu_kms_debugfs_init,
>> #endif
>> + .commit_lock = dpu_kms_commit_lock,
>> + .commit_unlock = dpu_kms_commit_unlock,
>> };
>>
>> static void _dpu_kms_mmu_destroy(struct dpu_kms *dpu_kms)
>> diff --git a/drivers/gpu/drm/msm/msm_atomic.c
>> b/drivers/gpu/drm/msm/msm_atomic.c
>> index 561bfa4..d33253f 100644
>> --- a/drivers/gpu/drm/msm/msm_atomic.c
>> +++ b/drivers/gpu/drm/msm/msm_atomic.c
>> @@ -55,16 +55,32 @@ static void vblank_put(struct msm_kms *kms,
>> unsigned crtc_mask)
>> }
>> }
>>
>> +static void msm_commit_lock(struct msm_kms *kms, unsigned int
>> crtc_mask)
>> +{
>> + if (kms->funcs->commit_lock)
>> + kms->funcs->commit_lock(kms, crtc_mask);
>> + else
>> + mutex_lock(&kms->commit_lock);
>> +}
>> +
>> +static void msm_commit_unlock(struct msm_kms *kms, unsigned int
>> crtc_mask)
>> +{
>> + if (kms->funcs->commit_unlock)
>> + kms->funcs->commit_unlock(kms, crtc_mask);
>> + else
>> + mutex_unlock(&kms->commit_lock);
>> +}
>
> Hi, I think the per-crtc commit-lock, and the updated
> locking/unlocking points are the right thing to do, but I don't think
> we need to touch dpu for this. Just change kms->commit_lock to an
> array of mutexes, and drop the vfunc indirection. All the same
> locking logic applies to mdp4/mdp5 as well (ie. don't touch the hw
> until it has flushed)
>
> BR,
> -R
>
Thanks for the comments Rob. I have addressed them in v2 patch.
Regards,
Krishna
>> +
>> static void msm_atomic_async_commit(struct msm_kms *kms, int
>> crtc_idx)
>> {
>> unsigned crtc_mask = BIT(crtc_idx);
>>
>> trace_msm_atomic_async_commit_start(crtc_mask);
>>
>> - mutex_lock(&kms->commit_lock);
>> + msm_commit_lock(kms, crtc_mask);
>>
>> if (!(kms->pending_crtc_mask & crtc_mask)) {
>> - mutex_unlock(&kms->commit_lock);
>> + msm_commit_unlock(kms, crtc_mask);
>> goto out;
>> }
>>
>> @@ -79,7 +95,6 @@ static void msm_atomic_async_commit(struct msm_kms
>> *kms, int crtc_idx)
>> */
>> trace_msm_atomic_flush_commit(crtc_mask);
>> kms->funcs->flush_commit(kms, crtc_mask);
>> - mutex_unlock(&kms->commit_lock);
>>
>> /*
>> * Wait for flush to complete:
>> @@ -90,9 +105,8 @@ static void msm_atomic_async_commit(struct msm_kms
>> *kms, int crtc_idx)
>>
>> vblank_put(kms, crtc_mask);
>>
>> - mutex_lock(&kms->commit_lock);
>> kms->funcs->complete_commit(kms, crtc_mask);
>> - mutex_unlock(&kms->commit_lock);
>> + msm_commit_unlock(kms, crtc_mask);
>> kms->funcs->disable_commit(kms);
>>
>> out:
>> @@ -189,12 +203,11 @@ void msm_atomic_commit_tail(struct
>> drm_atomic_state *state)
>> * Ensure any previous (potentially async) commit has
>> * completed:
>> */
>> + msm_commit_lock(kms, crtc_mask);
>> trace_msm_atomic_wait_flush_start(crtc_mask);
>> kms->funcs->wait_flush(kms, crtc_mask);
>> trace_msm_atomic_wait_flush_finish(crtc_mask);
>>
>> - mutex_lock(&kms->commit_lock);
>> -
>> /*
>> * Now that there is no in-progress flush, prepare the
>> * current update:
>> @@ -232,7 +245,7 @@ void msm_atomic_commit_tail(struct
>> drm_atomic_state *state)
>> }
>>
>> kms->funcs->disable_commit(kms);
>> - mutex_unlock(&kms->commit_lock);
>> + msm_commit_unlock(kms, crtc_mask);
>>
>> /*
>> * At this point, from drm core's perspective, we
>> @@ -260,7 +273,7 @@ void msm_atomic_commit_tail(struct
>> drm_atomic_state *state)
>> */
>> trace_msm_atomic_flush_commit(crtc_mask);
>> kms->funcs->flush_commit(kms, crtc_mask);
>> - mutex_unlock(&kms->commit_lock);
>> + msm_commit_unlock(kms, crtc_mask);
>>
>> /*
>> * Wait for flush to complete:
>> @@ -271,9 +284,9 @@ void msm_atomic_commit_tail(struct
>> drm_atomic_state *state)
>>
>> vblank_put(kms, crtc_mask);
>>
>> - mutex_lock(&kms->commit_lock);
>> + msm_commit_lock(kms, crtc_mask);
>> kms->funcs->complete_commit(kms, crtc_mask);
>> - mutex_unlock(&kms->commit_lock);
>> + msm_commit_unlock(kms, crtc_mask);
>> kms->funcs->disable_commit(kms);
>>
>> drm_atomic_helper_commit_hw_done(state);
>> diff --git a/drivers/gpu/drm/msm/msm_kms.h
>> b/drivers/gpu/drm/msm/msm_kms.h
>> index 1cbef6b..f02e73e 100644
>> --- a/drivers/gpu/drm/msm/msm_kms.h
>> +++ b/drivers/gpu/drm/msm/msm_kms.h
>> @@ -126,6 +126,11 @@ struct msm_kms_funcs {
>> /* debugfs: */
>> int (*debugfs_init)(struct msm_kms *kms, struct drm_minor
>> *minor);
>> #endif
>> + /* commit lock for crtc */
>> + void (*commit_lock)(struct msm_kms *kms, unsigned int
>> crtc_mask);
>> +
>> + /* commit unlock for crtc */
>> + void (*commit_unlock)(struct msm_kms *kms, unsigned int
>> crtc_mask);
>> };
>>
>> struct msm_kms;
>> --
>> 2.7.4
>>
Powered by blists - more mailing lists