On Tue, Feb 03, 2015 at 01:31:34PM +0200, Laurent Pinchart wrote: > Hi Daniel, > > Thank you for the patch. > > On Tuesday 03 February 2015 11:30:11 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. > > (Thinking out loud) > > In principle this looks good, but I find the naming pretty confusing. The > drm_crtc_vblank_* functions now have get, put, on, off and reset variants. > The > fact that on is supposed to be called both at runtime and an init time while > off is replaced by reset at init time breaks the symmetry between on and off. > What would you think of a drm_crtc_vblank_init() function instead, used at > init time only, and that would take an enabled boolean argument ? > > On the other hand, calling drm_crtc_vblank_on() at init time also makes > sense, > as even if the CRTC is active the vblank interrupt should be off then, and an > explicit call to the function means "turn the vblank interrupt on". I'm thus > not totally opposed to keeping that as-is. Wouldn't it then be better to > modify the core to default to off, and let drivers call drm_crtc_vblank_on() > explicitly if the default isn't correct ? I think I like this solution > better, > and it could be conditioned by a driver flag if we don't want to audit all > drivers for possible breakages.
Yeah, that's been my thinking with sticking with on and only replacing the off: For _on the environment exactly matches what we do when enabling the crtc in a modeset: - the pipe is actually running - we want vblanks to start working Totally different for _off, which assumes: - pipe is still on - vblanks should stop running and state should be saved - any pending Wheras _reset just disallows vblanks. Long-term I wonder whether a drm_crtc_vblank_init would be useful, which: - uses dev->mode_config.num_crtcs to set up the right amount of vblank pipes. - calls _reset directly - in the future might even store the vblank state in the drm_crtc. But before we can do that we need to split the vblank code into ums legacy paths using int pipe and kms paths dealing in struct drm_crtc *. We're not there yet. > > > Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com> > > Signed-off-by: Daniel Vetter <daniel.vet...@intel.com> > > --- > > drivers/gpu/drm/drm_irq.c | 32 ++++++++++++++++++++++++++++++++ > > drivers/gpu/drm/i915/intel_display.c | 4 ++-- > > include/drm/drmP.h | 1 + > > 3 files changed, 35 insertions(+), 2 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 > > Typo: difference compared to > > > + * drm_crtc_vblank_off() is that this function doesn't save the vblank > > counter > > + * and hence doesn't need to call any driver hooks. > > + */ > > +void drm_crtc_vblank_reset(struct drm_crtc *drm_crtc) > > +{ > > + struct drm_device *dev = drm_crtc->dev; > > + unsigned long irqflags; > > + int crtc = drm_crtc_index(drm_crtc); > > + struct drm_vblank_crtc *vblank = &dev->vblank[crtc]; > > + > > + spin_lock_irqsave(&dev->vbl_lock, irqflags); > > + /* > > + * Prevent subsequent drm_vblank_get() from enabling the vblank > > + * interrupt by bumping the refcount. > > + */ > > + if (!vblank->inmodeset) { > > + atomic_inc(&vblank->refcount); > > + vblank->inmodeset = 1; > > + } > > + spin_unlock_irqrestore(&dev->vbl_lock, irqflags); > > + > > + WARN_ON(!list_empty(&dev->vblank_event_list)); > > +} > > +EXPORT_SYMBOL(drm_crtc_vblank_reset); > > + > > +/** > > * drm_vblank_on - enable vblank events on a CRTC > > * @dev: DRM device > > * @crtc: CRTC in question > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c index 423ef959264d..f8871a184747 > > 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -13296,9 +13296,9 @@ static void intel_sanitize_crtc(struct intel_crtc > > *crtc) /* restore vblank interrupts to correct state */ > > if (crtc->active) { > > update_scanline_offset(crtc); > > - drm_vblank_on(dev, crtc->pipe); > > + drm_crtc_vblank_on(&crtc->base); > > } else > > - drm_vblank_off(dev, crtc->pipe); > > + drm_crtc_vblank_reset(&crtc->base); > > > > /* We need to sanitize the plane -> pipe mapping first because this will > > * disable the crtc (and hence change the state) if it is wrong. Note > > diff --git a/include/drm/drmP.h b/include/drm/drmP.h > > index e928625a9da0..54c6ea1e5866 100644 > > --- a/include/drm/drmP.h > > +++ b/include/drm/drmP.h > > @@ -922,6 +922,7 @@ extern void drm_crtc_wait_one_vblank(struct drm_crtc > > *crtc); extern void drm_vblank_off(struct drm_device *dev, int crtc); > > extern void drm_vblank_on(struct drm_device *dev, int crtc); > > extern void drm_crtc_vblank_off(struct drm_crtc *crtc); > > +extern void drm_crtc_vblank_reset(struct drm_crtc *crtc); > > extern void drm_crtc_vblank_on(struct drm_crtc *crtc); > > extern void drm_vblank_cleanup(struct drm_device *dev); > > -- > Regards, > > Laurent Pinchart > -- 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