On Wed, Aug 6, 2014 at 4:28 PM, Daniel Vetter <daniel@xxxxxxxx> 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@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..
>
> 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@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
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
