Re: [v1] drm/msm: Fix race condition in msm driver with async layer updates

[mkrishn@xxxxxxxxxxxxxx]

On 2020-10-14 20:55, Rob Clark wrote:
> On Wed, Oct 14, 2020 at 5:58 AM Krishna Manikandan
> <mkrishn@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > ---
> >  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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel