Patchwork drm/atomic-helper: check that drivers call drm_crtc_vblank_off

login
register
mail settings
Submitter Daniel Vetter
Date Oct. 17, 2017, 2:59 p.m.
Message ID <20171017145939.438-1-daniel.vetter@ffwll.ch>
Download mbox | patch
Permalink /patch/10012245/
State New
Headers show

Comments

Daniel Vetter - Oct. 17, 2017, 2:59 p.m.
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.

Cc: Rob Clark <robdclark@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 9 +++++++++
 1 file changed, 9 insertions(+)
Ville Syrjälä - Oct. 17, 2017, 3:11 p.m.
On Tue, Oct 17, 2017 at 04:59:39PM +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.
> 
> Cc: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@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..cc9c0173e075 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))

num_crtcs confused me. Maybe we should rename it to num_vblank_crtcs or
something...

> +			continue;
> +
> +		ret = drm_crtc_vblank_get(crtc);
> +		WARN_ONCE(ret != -EINVAL, "driver forgot to call drm_crtc_vblank_off()\n");
> +		if (ret)

ret==0 presumably

> +			drm_crtc_vblank_put(crtc);
>  	}
>  }
>  
> -- 
> 2.14.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Sean Paul - Oct. 17, 2017, 5:47 p.m.
On Tue, Oct 17, 2017 at 04:59:39PM +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.
> 
> Cc: Rob Clark <robdclark@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@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..cc9c0173e075 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)

Shouldn't this be !ret

Sean

> +			drm_crtc_vblank_put(crtc);
>  	}
>  }
>  
> -- 
> 2.14.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Sean Paul - Oct. 17, 2017, 5:48 p.m.
On Tue, Oct 17, 2017 at 01:47:36PM -0400, Sean Paul wrote:
> On Tue, Oct 17, 2017 at 04:59:39PM +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.
> > 
> > Cc: Rob Clark <robdclark@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@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..cc9c0173e075 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)
> 
> Shouldn't this be !ret

Annnd I should check my email before sending. Sorry, v2 is 

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> 
> Sean
> 
> > +			drm_crtc_vblank_put(crtc);
> >  	}
> >  }
> >  
> > -- 
> > 2.14.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

Patch

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index ae56d91433ff..cc9c0173e075 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)
+			drm_crtc_vblank_put(crtc);
 	}
 }