[Intel-gfx] [PATCH] drm/atomic-helper: check that drivers call drm_crtc_vblank_off

[Daniel Vetter]

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 at gmail.com>
> > Tested-by: Rob Clark <robdclark at gmail.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala at 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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch