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

[Sean Paul <sean@poorly.run>]

On Thu, Oct 10, 2019 at 12:20:56AM +0200, Daniel Vetter wrote:
> 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).

Indeed. I'm not sure what my thinking was when submitting this, I think some of
the callsites may have changed since this was posted (with the enable/probe
refactors from a few months ago). At any rate, doesn't matter now, I'll post the
revert :-)

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

Ok, I'll just assume that you read seanpaul's 11-month old reviews before bed to
relax ;-)

Sean


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

-- 
Sean Paul, Software Engineer, Google / Chromium OS