| Message ID | 20171017152714.6849-1-daniel.vetter@ffwll.ch |
|---|---|
| State | Accepted |
| Commit | 84014b0a39eef6df4a26f8afabf2b50bd376d515 |
| Headers | show |
| Series |
"drm/atomic-helper: check that drivers call drm_crtc_vblank_off"
( rev:
2
)
in
Intel GFX |
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ae56d91433ff..029c8c1d97f6 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -860,6 +860,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) { const struct drm_crtc_helper_funcs *funcs; + int ret; /* Shut down everything that needs a full modeset. */ if (!drm_atomic_crtc_needs_modeset(new_crtc_state)) @@ -883,6 +884,14 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) funcs->disable(crtc); else funcs->dpms(crtc, DRM_MODE_DPMS_OFF); + + if (!(dev->irq_enabled && dev->num_crtcs)) + continue; + + ret = drm_crtc_vblank_get(crtc); + WARN_ONCE(ret != -EINVAL, "driver forgot to call drm_crtc_vblank_off()\n"); + if (ret == 0) + drm_crtc_vblank_put(crtc); } }
On Tue, Oct 17, 2017 at 05:27:14PM +0200, Daniel Vetter wrote: > At least when they have vblank support they need to call this, or the > vblank core will happily call into their crtc->enable_vblank callback > even when the crtc is off. Which leads to a boom when the clocks are > off on most hardware (besides the inevitable confusion in the > book-keeping). > > The consistency checks in drm_vblank.c will then make sure that > vblank_off/on calls are balanced, and if drivers forget to re-enable > it all the commits will stall, so I think we're covered. > > It'd be nice to be able to place this check outside of commit helpers, > but tha's not really possible (due to nonblocking commits and all > that). Placing it into atomic helpers should at least cover most > drivers. > > Also note that vblank support is still optional (for virtual drivers, > which tend to not have this), check for that. > > v2: Fixup the handling for vblank_put (Rob). > > Cc: Rob Clark <robdclark@gmail.com> > Tested-by: Rob Clark <robdclark@gmail.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/drm_atomic_helper.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index ae56d91433ff..029c8c1d97f6 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -860,6 +860,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) > > for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) { > const struct drm_crtc_helper_funcs *funcs; > + int ret; > > /* Shut down everything that needs a full modeset. */ > if (!drm_atomic_crtc_needs_modeset(new_crtc_state)) > @@ -883,6 +884,14 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) > funcs->disable(crtc); > else > funcs->dpms(crtc, DRM_MODE_DPMS_OFF); > + > + if (!(dev->irq_enabled && dev->num_crtcs)) > + continue; > + > + ret = drm_crtc_vblank_get(crtc); > + WARN_ONCE(ret != -EINVAL, "driver forgot to call drm_crtc_vblank_off()\n"); > + if (ret == 0) > + drm_crtc_vblank_put(crtc); > } > } > > -- > 2.14.1
On Tue, Oct 17, 2017 at 06:32:52PM +0300, Ville Syrjälä wrote: > On Tue, Oct 17, 2017 at 05:27:14PM +0200, Daniel Vetter wrote: > > At least when they have vblank support they need to call this, or the > > vblank core will happily call into their crtc->enable_vblank callback > > even when the crtc is off. Which leads to a boom when the clocks are > > off on most hardware (besides the inevitable confusion in the > > book-keeping). > > > > The consistency checks in drm_vblank.c will then make sure that > > vblank_off/on calls are balanced, and if drivers forget to re-enable > > it all the commits will stall, so I think we're covered. > > > > It'd be nice to be able to place this check outside of commit helpers, > > but tha's not really possible (due to nonblocking commits and all > > that). Placing it into atomic helpers should at least cover most > > drivers. > > > > Also note that vblank support is still optional (for virtual drivers, > > which tend to not have this), check for that. > > > > v2: Fixup the handling for vblank_put (Rob). > > > > Cc: Rob Clark <robdclark@gmail.com> > > Tested-by: Rob Clark <robdclark@gmail.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Applied, thanks for your review. -Daniel > > > --- > > drivers/gpu/drm/drm_atomic_helper.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > index ae56d91433ff..029c8c1d97f6 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -860,6 +860,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) > > > > for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) { > > const struct drm_crtc_helper_funcs *funcs; > > + int ret; > > > > /* Shut down everything that needs a full modeset. */ > > if (!drm_atomic_crtc_needs_modeset(new_crtc_state)) > > @@ -883,6 +884,14 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) > > funcs->disable(crtc); > > else > > funcs->dpms(crtc, DRM_MODE_DPMS_OFF); > > + > > + if (!(dev->irq_enabled && dev->num_crtcs)) > > + continue; > > + > > + ret = drm_crtc_vblank_get(crtc); > > + WARN_ONCE(ret != -EINVAL, "driver forgot to call drm_crtc_vblank_off()\n"); > > + if (ret == 0) > > + drm_crtc_vblank_put(crtc); > > } > > } > > > > -- > > 2.14.1 > > -- > Ville Syrjälä > Intel OTC