| Message ID | 1407345419-30007-1-git-send-email-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > > 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
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.. 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. 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
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. -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
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
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. > > Signed-off-by: Rob Clark <robdclark@gmail.com> Queued for -next, thanks for the patch. -Daniel > --- > 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
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.
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. 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(+)