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) + drm_crtc_vblank_put(crtc); } }
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
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 --- 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); } }
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
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
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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Daniel Vetter -
Sean Paul -
Ville Syrjälä