| Message ID | 20191010151351.126735-1-sean@poorly.run (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Revert "drm/msm: dpu: Add modeset lock checks where applicable" | expand |
On Thu, Oct 10, 2019 at 5:13 PM Sean Paul <sean@poorly.run> wrote: > > From: Sean Paul <seanpaul@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@codeaurora.org> > Cc: Rob Clark <robdclark@chromium.org> > Suggested-by: Daniel Vetter <daniel@ffwll.ch> > 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 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@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 >
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)); - /* 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)); drm_for_each_crtc(crtc, dev) { if (!(crtc->state->encoder_mask & drm_encoder_mask(encoder))) continue;