| Message ID | 20181116184238.170034-17-sean@poorly.run (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/msm: Various dpu locking and legacy cleanups | expand |
On 2018-11-16 10:42, Sean Paul 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> > --- Reviewed-by: Jeykumar Sankaran <jsanka@codeaurora.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;
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
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
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;