Re: [PATCH v2 16/24] drm/msm: dpu: Add modeset lock checks where applicable

From: Daniel Vetter <daniel@ffwll.ch>
To: Sean Paul <sean@poorly.run>
Cc: dri-devel <dri-devel@lists.freedesktop.org>,
	freedreno <freedreno@lists.freedesktop.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	Sean Paul <seanpaul@chromium.org>
Subject: Re: [PATCH v2 16/24] drm/msm: dpu: Add modeset lock checks where applicable
Date: Thu, 10 Oct 2019 00:20:56 +0200	[thread overview]
Message-ID: <CAKMK7uEx2fL69jPGXVpQhhcj8Q8zEzb3ogCv-uVyGrX3QYFeWQ@mail.gmail.com> (raw)
In-Reply-To: <20181116184238.170034-17-sean@poorly.run>

On Fri, Nov 16, 2018 at 7:44 PM Sean Paul <sean@poorly.run> wrote:
>
> From: Sean Paul <seanpaul@chromium.org>
>
> Add modeset lock checks to functions that could be called outside the
> core atomic stack.
>
> Changes in v2:
> - None
>
> Signed-off-by: Sean Paul <seanpaul@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 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index a008a87a8113..cd0a0bea4335 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -284,6 +284,8 @@ 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));
> +
>         /* 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 64134d619748..5104fc01147e 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -358,6 +358,7 @@ 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));
>         drm_for_each_crtc(crtc, dev) {
>                 if (!(crtc->state->encoder_mask & drm_encoder_mask(encoder)))
>                         continue;

I'm fairly sure this is called in the atomic_commit path, and in there
you might not actually hold these locks (if you do a nonblocking
modeset).

The locking rules for ->state are pretty fun: Either hold the lock, or
be in atomic commit. In the later case atomic helpers' commit ordering
guarantees that you can safely access ->state (but read-only only)
without hodling any locks. You might want to revert.

Don't ask why I stumbled over this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch