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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

> +
>  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



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux