Re: [PATCH 1/5] drm/irq: Add drm_crtc_vblank_reset

[Daniel Vetter <daniel@ffwll.ch>]

On Sun, Feb 15, 2015 at 04:08:31PM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> Thank you for the patch.
> 
> On Friday 13 February 2015 21:03:42 Daniel Vetter wrote:
> > At driver load we need to tell the vblank code about the state of the
> > pipes, so that the logic around reject vblank_get when the pipe is off
> > works correctly.
> > 
> > Thus far i915 used drm_vblank_off, but one of the side-effects of it
> > is that it also saves the vblank counter. And for that it calls down
> > into the ->get_vblank_counter hook. Which isn't really a good idea
> > when the pipe is off for a few reasons:
> > - With runtime pm the register might not respond.
> > - If the pipe is off some datastructures might not be around or
> >   unitialized.
> > 
> > The later is what blew up on gen3: We look at intel_crtc->config to
> > compute the vblank counter, and for a disabled pipe at boot-up that's
> > just not there. Thus far this was papered over by a check for
> > intel_crtc->active, but I want to get rid of that (since it's fairly
> > race, vblank hooks are called from all kinds of places).
> > 
> > So prep for that by adding a _reset functions which only does what we
> > really need to be done at driver load: Mark the vblank pipe as off,
> > but don't do any vblank counter saving or event flushing - neither of
> > that is required.
> > 
> > v2: Clarify the code flow slightly as suggested by Ville.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Imre Deak <imre.deak@intel.com>
> > Reviewed-by: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/drm_irq.c            | 32 ++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_display.c |  6 +++---
> >  include/drm/drmP.h                   |  1 +
> >  3 files changed, 36 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index 75647e7f012b..1e5fb1b994d7 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -1226,6 +1226,38 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
> >  EXPORT_SYMBOL(drm_crtc_vblank_off);
> > 
> >  /**
> > + * drm_crtc_vblank_reset - reset vblank state to off on a CRTC
> > + * @crtc: CRTC in question
> > + *
> > + * Drivers can use this function to reset the vblank state to off at load
> > time.
> > + * Drivers should use this together with the drm_crtc_vblank_off() and
> > + * drm_crtc_vblank_on() functions. The diffrence comparet to
> 
> s/diffrence comparet/difference compared/
> 
> With that fixed,
> 
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Fixed while applying, thanks for the feedback.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx