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

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>,
	"Chris Wilson" <chris@chris-wilson.co.uk>,
	intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Assert we hold the CRTC powerwell for generating vblank interrupts
Date: Tue, 11 Oct 2016 08:17:22 +0200	[thread overview]
Message-ID: <6925210f-e485-64de-e0be-17fe03887924@linux.intel.com> (raw)
In-Reply-To: <20161010115640.GC4329@intel.com>

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@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.
>
I'm trying to kill crtc->active. 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

Safest bet is to look at the power state I think.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx