Re: [PATCH] drm/i915: Assert we hold the CRTC powerwell for generating vblank interrupts

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Oct 10, 2016 at 12:46:32PM +0100, Chris Wilson wrote:
> On Mon, Oct 10, 2016 at 02:42:01PM +0300, Ville Syrjälä wrote:
> > On Mon, Oct 10, 2016 at 12:34:54PM +0100, Chris Wilson wrote:
> > > To enable the vblank itself, we need to have an RPM wakeref for the mmio
> > > access, and whilst generating the vblank interrupts we continue to
> > > require the rpm wakeref. The assumption is that the RPM wakeref is held
> > > by the display powerwell held by the active pipe. As this chain was not
> > > obvious to me chasing the drm_wait_vblank ioctl, document it with a WARN
> > > during *_vblank_enable().
> > > 
> > > v2: Check the display power well rather than digging inside the atomic
> > > CRTC state.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_irq.c | 20 ++++++++++++++++++++
> > >  1 file changed, 20 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > > index 1e43fe30da11..f0f17055dbb9 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -2715,6 +2715,14 @@ void i915_handle_error(struct drm_i915_private *dev_priv,
> > >  	i915_reset_and_wakeup(dev_priv);
> > >  }
> > >  
> > > +static void assert_pipe_is_awake(struct drm_i915_private *dev_priv,
> > > +				 enum pipe pipe)
> > > +{
> > > +	WARN_ON(IS_ENABLED(CONFIG_DRM_I915_DEBUG) &&
> > > +		!intel_display_power_is_enabled(dev_priv,
> > > +						POWER_DOMAIN_PIPE(pipe)));
> > 
> > Uses a mutex. And having a power well enabled doesn't mean much. It
> > doesn't guarantee that vblanks work.
> 
> Impasse. :|
> 
> There should be no point in an explicit assert_rpm_wakeref here as the
> register access should catch an error there. Is there no safe way we can
> assert the current state of the CRTC is correct for enabling vblanks?

crtc->active might be the closest thing, if we just ignore any locking.
Though it looks like that has gone a bit mad these days, what with being
set from the .crtc_enable() hooks but getting cleared outside the
.crtc_disable() hooks.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx