[PATCH] Revert "drm/msm: dpu: Add modeset lock checks where applicable"

[Daniel Vetter]

On Thu, Oct 10, 2019 at 5:13 PM Sean Paul <sean at poorly.run> wrote:
>
> From: Sean Paul <seanpaul at chromium.org>
>
> This reverts commit 1dfdb0e107dbe6ebff3f6bbbe4aad0b5aa87bba4.
>
> As Daniel mentions in his email [1], non-blocking commits don't hold the
> modeset locks, so we can safely access state as long as these functions
> are in the commit path. I'm not entirely sure if these have always been
> isolated to the commit path, but they seem to be now.
>
> [1]- https://lists.freedesktop.org/archives/dri-devel/2019-October/239441.html
>
> Fixes: 1dfdb0e107db ("drm/msm: dpu: Add modeset lock checks where applicable")
> Cc: Jeykumar Sankaran <jsanka at codeaurora.org>
> Cc: Rob Clark <robdclark at chromium.org>
> Suggested-by: Daniel Vetter <daniel at ffwll.ch>
> Signed-off-by: Sean Paul <seanpaul at chromium.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 2 --
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  | 1 -
>  2 files changed, 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index db6c9ccf3be26..c645dd201368b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -282,8 +282,6 @@ enum dpu_intf_mode dpu_crtc_get_intf_mode(struct drm_crtc *crtc)
>                 return INTF_MODE_NONE;
>         }
>
> -       WARN_ON(!drm_modeset_is_locked(&crtc->mutex));

This one is worse ... it's used in two places:
- debugfs, where you actually want to make sure you're holding this lock
- atomic_check, where this is broken since you're supposed to look at
the free-standing states only, except if you really know what you're
doing. Given that there's no comment here, I suspect that's not the
case. Note that for atomic_check you're guaranteed to hold the modeset
lock.

I'd put a FIXME here, but leave the WARN_ON until this is fixed properly.
> -
>         /* TODO: Returns the first INTF_MODE, could there be multiple values? */
>         drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
>                 return dpu_encoder_get_intf_mode(encoder);
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index e393a423d7d7a..0e68e20d19c87 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -305,7 +305,6 @@ void dpu_kms_encoder_enable(struct drm_encoder *encoder)
>         if (funcs && funcs->commit)
>                 funcs->commit(encoder);
>
> -       WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));

Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>

but only for this hunk here.
-Daniel

>         drm_for_each_crtc(crtc, dev) {
>                 if (!(crtc->state->encoder_mask & drm_encoder_mask(encoder)))
>                         continue;
> --
> Sean Paul, Software Engineer, Google / Chromium OS
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch