On Wed, Aug 6, 2014 at 4:28 PM, Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Aug 06, 2014 at 03:06:40PM -0400, Rob Clark wrote:
On Wed, Aug 6, 2014 at 2:12 PM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Wed, Aug 06, 2014 at 01:16:59PM -0400, Rob Clark wrote:
After spending slightly more time than I'd care to admit debugging the various and presumably spectacular way things fail when you pass too low a value to drm_vblank_init() (thanks console-lock for not letting me see the carnage!), I decided it might be a good idea to add some sanity checking.
Hmm. Could we instead do some kind of cross checking in drm_vblank_init() and drm_crtc_init() to avoid having to sprinkle this stuff all over the place? I guess the checks would need to happen both ways since the driver might call those in either order.
it would be nice if we could just drop the arg to drm_vblank_init() to have one less place to screw things up.. I suspect that is some UMS relic..
It is.
We could possibly co-check in vblank_init() each crtc_init().. I guess it would be marginally fewer lines of diffstat, and in theory the driver could still manage to screw up by just passing bogus pipe #'s. The vblank code already has these sort of checks for things that are potentially userspace facing, so adding the same check (plus WARN_ON()) to the internal fxns seemed like the logical and straightforward solution.
The problem is that currently don't have a point where we know that all crtc are set up, so we can't just chuck the right drm_vblank_init call into a modeset setup function.
But we could add a drm_mode_set_vblank_init which looks at dev->mode_config.num_crtcs and for paranoia sets a flag which makes all subsequent crtc_init fail loud.
In a perfect world we'd just move the vblank data into drm_crtc, but we're not yet there. Ville' has already neatly split things up (mostly), and I've started to consistently add and use vblank functions which take a drm_crtc * instead of a pipe integer. But really not there yet by far.
yeah, in the mean time we should add my simple patch to make drm developers not hate console_lock quite so much :-)
BR, -R
-Daniel
BR, -R
Signed-off-by: Rob Clark robdclark@gmail.com
btw, I wonder if we could add a module param hack to toss initial modeset off to a workqueue to sneak it out from the tyranny of console_lock?
drivers/gpu/drm/drm_irq.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 0de123a..6f16a104 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -730,6 +730,8 @@ EXPORT_SYMBOL(drm_get_last_vbltimestamp); */ u32 drm_vblank_count(struct drm_device *dev, int crtc) {
if (WARN_ON(crtc >= dev->num_crtcs))return 0; return atomic_read(&dev->vblank[crtc].count);} EXPORT_SYMBOL(drm_vblank_count); @@ -752,6 +754,9 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, int crtc, { u32 cur_vblank;
if (WARN_ON(crtc >= dev->num_crtcs))return 0;/* Read timestamp from slot of _vblank_time ringbuffer * that corresponds to current vblank count. Retry if * count has incremented during readout. This works like@@ -927,6 +932,9 @@ int drm_vblank_get(struct drm_device *dev, int crtc) unsigned long irqflags; int ret = 0;
if (WARN_ON(crtc >= dev->num_crtcs))return -EINVAL;spin_lock_irqsave(&dev->vbl_lock, irqflags); /* Going from 0->1 means we have to enable interrupts again */ if (atomic_add_return(1, &dev->vblank[crtc].refcount) == 1) {@@ -975,6 +983,9 @@ void drm_vblank_put(struct drm_device *dev, int crtc) { BUG_ON(atomic_read(&dev->vblank[crtc].refcount) == 0);
if (WARN_ON(crtc >= dev->num_crtcs))return;/* Last user schedules interrupt disable */ if (atomic_dec_and_test(&dev->vblank[crtc].refcount) && (drm_vblank_offdelay > 0))@@ -1019,6 +1030,9 @@ void drm_vblank_off(struct drm_device *dev, int crtc) unsigned long irqflags; unsigned int seq;
if (WARN_ON(crtc >= dev->num_crtcs))return;spin_lock_irqsave(&dev->vbl_lock, irqflags); vblank_disable_and_save(dev, crtc); wake_up(&dev->vblank[crtc].queue);@@ -1078,6 +1092,9 @@ void drm_vblank_on(struct drm_device *dev, int crtc) { unsigned long irqflags;
if (WARN_ON(crtc >= dev->num_crtcs))return;spin_lock_irqsave(&dev->vbl_lock, irqflags); /* re-enable interrupts if there's are users left */ if (atomic_read(&dev->vblank[crtc].refcount) != 0)@@ -1131,6 +1148,10 @@ void drm_vblank_pre_modeset(struct drm_device *dev, int crtc) /* vblank is not initialized (IRQ not installed ?), or has been freed */ if (!dev->num_crtcs) return;
if (WARN_ON(crtc >= dev->num_crtcs))return;/* * To avoid all the problems that might happen if interrupts * were enabled/disabled around or between these calls, we just@@ -1439,6 +1460,9 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc) if (!dev->num_crtcs) return false;
if (WARN_ON(crtc >= dev->num_crtcs))return false;/* Need timestamp lock to prevent concurrent execution with * vblank enable/disable, as this would cause inconsistent * or corrupted timestamps and vblank counts.-- 1.9.3
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Ville Syrjälä Intel OTC
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch