[PATCH] drm: idiot-proof vblank
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
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(>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(>vbl_lock, irqflags);
> /* Going from 0->1 means we have to enable interrupts again */
> if (atomic_add_return(1, >vblank[crtc].refcount) == 1) {
> @@ -975,6 +983,9 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
> {
> BUG_ON(atomic_read(>vblank[crtc].refcount) == 0);
>
> + if (WARN_ON(crtc >= dev->num_crtcs))
> + return;
> +
> /* Last user schedules interrupt disable */
> if (atomic_dec_and_test(>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(>vbl_lock, irqflags);
> vblank_disable_and_save(dev, crtc);
> wake_up(>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(>vbl_lock, irqflags);
> /* re-enable interrupts if there's are users left */
> if (atomic_read(>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 at 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
[PATCH] drm: idiot-proof vblank
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?
> 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
> >> ---
> >> 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(>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(>vbl_lock, irqflags);
> >> /* Going from 0->1 means we have to enable interrupts again */
> >> if (atomic_add_return(1, >vblank[crtc].refcount) == 1) {
> >> @@ -975,6 +983,9 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
> >> {
> >> BUG_ON(atomic_read(>vblank[crtc].refcount) == 0);
> >>
> >> + if (WARN_ON(crtc >= dev->num_crtcs))
> >> + return;
> >> +
> >> /* Last user schedules interrupt disable */
> >> if (atomic_dec_and_test(>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(>vbl_lock, irqflags);
> >> vblank_disable_and_save(dev, crtc);
> >> wake_up(>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(>vbl_lock, irqflags);
> >> /* re-enable interrupts if there's are users left */
> >> if (atomic_read(>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
[PATCH] drm: idiot-proof vblank
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
> ---
> 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(>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(>vbl_lock, irqflags);
> /* Going from 0->1 means we have to enable interrupts again */
> if (atomic_add_return(1, >vblank[crtc].refcount) == 1) {
> @@ -975,6 +983,9 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
> {
> BUG_ON(atomic_read(>vblank[crtc].refcount) == 0);
>
> + if (WARN_ON(crtc >= dev->num_crtcs))
> + return;
> +
> /* Last user schedules interrupt disable */
> if (atomic_dec_and_test(>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(>vbl_lock, irqflags);
> vblank_disable_and_save(dev, crtc);
> wake_up(>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(>vbl_lock, irqflags);
> /* re-enable interrupts if there's are users left */
> if (atomic_read(>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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrj?l?
Intel OTC
[PATCH] drm: idiot-proof vblank
On Wed, Aug 6, 2014 at 4:28 PM, Daniel Vetter 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?
>> 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
>> >> ---
>> >> 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(>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(>vbl_lock, irqflags);
>> >> /* Going from 0->1 means we have to enable interrupts again */
>> >> if (atomic_add_return(1, >vblank[crtc].refcount) == 1) {
>> >> @@ -975,6 +983,9 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
>> >> {
>> >> BUG_ON(atomic_read(>vblank[crtc].refcount) == 0);
>> >>
>> >> + if (WARN_ON(crtc >= dev->num_crtcs))
>> >> + return;
>> >> +
>> >> /* Last user schedules interrupt disable */
>> >> if (atomic_dec_and_test(>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(>vbl_lock, irqflags);
>> >> vblank_disable_and_save(dev, crtc);
>> >> wake_up(>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;
>> >> +
>> >>
[PATCH] drm: idiot-proof vblank
On Wed, Aug 6, 2014 at 2:12 PM, Ville Syrj?l?
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
>> ---
>> 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(>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(>vbl_lock, irqflags);
>> /* Going from 0->1 means we have to enable interrupts again */
>> if (atomic_add_return(1, >vblank[crtc].refcount) == 1) {
>> @@ -975,6 +983,9 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
>> {
>> BUG_ON(atomic_read(>vblank[crtc].refcount) == 0);
>>
>> + if (WARN_ON(crtc >= dev->num_crtcs))
>> + return;
>> +
>> /* Last user schedules interrupt disable */
>> if (atomic_dec_and_test(>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(>vbl_lock, irqflags);
>> vblank_disable_and_save(dev, crtc);
>> wake_up(>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(>vbl_lock, irqflags);
>> /* re-enable interrupts if there's are users left */
>> if (atomic_read(>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 at lists.freedesktop.org
>>
[PATCH] drm: idiot-proof vblank
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
---
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(>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(>vbl_lock, irqflags);
/* Going from 0->1 means we have to enable interrupts again */
if (atomic_add_return(1, >vblank[crtc].refcount) == 1) {
@@ -975,6 +983,9 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
{
BUG_ON(atomic_read(>vblank[crtc].refcount) == 0);
+ if (WARN_ON(crtc >= dev->num_crtcs))
+ return;
+
/* Last user schedules interrupt disable */
if (atomic_dec_and_test(>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(>vbl_lock, irqflags);
vblank_disable_and_save(dev, crtc);
wake_up(>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(>vbl_lock, irqflags);
/* re-enable interrupts if there's are users left */
if (atomic_read(>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
