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.
-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
