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 <robdcl...@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vet...@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-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Reply via email to