On Wed, Aug 6, 2014 at 2:12 PM, Ville Syrjälä
<ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
