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

[Daniel Vetter]

On Wed, Oct 12, 2016 at 07:21:44AM +0200, Maarten Lankhorst wrote:
> Op 11-10-16 om 10:32 schreef Ville Syrjälä:
> > On Tue, Oct 11, 2016 at 09:45:45AM +0200, Maarten Lankhorst wrote:
> >> Op 11-10-16 om 08:55 schreef Ville Syrjälä:
> >>> On Tue, Oct 11, 2016 at 08:17:22AM +0200, Maarten Lankhorst wrote:
> >>>> Op 10-10-16 om 13:56 schreef Ville Syrjälä:
> >>>>> 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 at chris-wilson.co.uk>
> >>>>>>>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >>>>>>>> Cc: Maarten Lankhorst <maarten.lankhorst at 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.
> >>>>>
> >>>> I'm trying to kill crtc->active.
> >>> Because it's evil? I still don't see much problem in having a thing to
> >>> track the state of each pipe fairly accurately.
> >>>
> >>>> Maybe you'd want to use dev_priv->active_crtcs, but that won't save you if you enable interrupts in between atomic commit and .crtc_disable
> >>> Nothing atomic based will work well because the state is not flipped at
> >>> the same time as the actual hardware state changes.
> >>>
> >>>> Safest bet is to look at the power state I think.
> >>> I don't know which power state you mean, but I already shot down the
> >>> power domain thing.
> >>>
> >>>
> >> I would say use assert_pipe_enabled then.
> > Nope. That one frobs with power domains too these days.
> >
> I don't see a nice way to do this, it probably means we shouldn't do this at all..
> Maybe have a function look at dev_priv->power_domains.domain_use_count[[POWER_DOMAIN_PIPE(pipe)] >= 1?
> It's potentially racy but I don't see a nice way to check, apart from hoping we handle the drm vblank on/off
> calls correctly.
> 
> The only other way I see is demidlayering vblank.

Or switch over power domains over to the core power domain stuff. There's
a reason those are protected with spinlocks, so that you can do these
kinds of checks ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch