* [v1] drm/msm: Fix race condition in msm driver with async layer updates
@ 2020-10-14 12:58 Krishna Manikandan
2020-10-14 15:25 ` Rob Clark
0 siblings, 1 reply; 3+ messages in thread
From: Krishna Manikandan @ 2020-10-14 12:58 UTC (permalink / raw)
To: dri-devel, linux-arm-msm, freedreno, devicetree
Cc: Krishna Manikandan, linux-kernel, robdclark, seanpaul, hoegsberg,
kalyan_t, dianders
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@codeaurora.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);
+}
+
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [v1] drm/msm: Fix race condition in msm driver with async layer updates
2020-10-14 12:58 [v1] drm/msm: Fix race condition in msm driver with async layer updates Krishna Manikandan
@ 2020-10-14 15:25 ` Rob Clark
2020-10-15 9:18 ` mkrishn
0 siblings, 1 reply; 3+ messages in thread
From: Rob Clark @ 2020-10-14 15:25 UTC (permalink / raw)
To: Krishna Manikandan
Cc: dri-devel, linux-arm-msm, freedreno,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux Kernel Mailing List, Sean Paul, Kristian H. Kristensen,
Kalyan Thota, Douglas Anderson
On Wed, Oct 14, 2020 at 5:58 AM Krishna Manikandan
<mkrishn@codeaurora.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@codeaurora.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
> +
> 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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [v1] drm/msm: Fix race condition in msm driver with async layer updates
2020-10-14 15:25 ` Rob Clark
@ 2020-10-15 9:18 ` mkrishn
0 siblings, 0 replies; 3+ messages in thread
From: mkrishn @ 2020-10-15 9:18 UTC (permalink / raw)
To: Rob Clark
Cc: dri-devel, linux-arm-msm, freedreno,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux Kernel Mailing List, Sean Paul, Kristian H. Kristensen,
Kalyan Thota, Douglas Anderson
On 2020-10-14 20:55, Rob Clark wrote:
> On Wed, Oct 14, 2020 at 5:58 AM Krishna Manikandan
> <mkrishn@codeaurora.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@codeaurora.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
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-10-15 9:18 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-14 12:58 [v1] drm/msm: Fix race condition in msm driver with async layer updates Krishna Manikandan
2020-10-14 15:25 ` Rob Clark
2020-10-15 9:18 ` mkrishn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).