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

[Chris Wilson]

On Thu, Oct 13, 2016 at 04:17:07PM +0200, Daniel Vetter wrote:
> On Wed, Oct 12, 2016 at 07:21:44AM +0200, Maarten Lankhorst wrote:
> > 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 ;-)

Why not both! They're on the wishlist somewhere.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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 
>  Cc: Ville Syrjälä 
>  Cc: Maarten Lankhorst 
>  ---
>   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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Maarten Lankhorst]

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 
 Cc: Ville Syrjälä 
 Cc: Maarten Lankhorst 
 ---
  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.

~Maarten

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

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

[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 
> >> Cc: Ville Syrjälä 
> >> Cc: Maarten Lankhorst 
> >> ---
> >>  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.

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

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

[Maarten Lankhorst]

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 
>> Cc: Ville Syrjälä 
>> Cc: Maarten Lankhorst 
>> ---
>>  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.

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

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

[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 
>  Cc: Ville Syrjälä 
>  Cc: Maarten Lankhorst 
>  ---
>   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.


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

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

[Maarten Lankhorst]

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 
 Cc: Ville Syrjälä 
 Cc: Maarten Lankhorst 
 ---
  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

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

[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 
> > > Cc: Ville Syrjälä 
> > > Cc: Maarten Lankhorst 
> > > ---
> > >  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

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

[Chris Wilson]

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 
> > Cc: Ville Syrjälä 
> > Cc: Maarten Lankhorst 
> > ---
> >  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?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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 
> Cc: Ville Syrjälä 
> Cc: Maarten Lankhorst 
> ---
>  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.

> +}
> +
>  /* Called from drm generic code, passed 'crtc' which
>   * we use as a pipe index
>   */
> @@ -2723,6 +2731,9 @@ static int i8xx_enable_vblank(struct drm_device *dev, 
> unsigned int pipe)
>   struct drm_i915_private *dev_priv = to_i915(dev);
>   unsigned long irqflags;
>  
> + /* vblank IRQ requires the powerwell, held awake by the CRTC */
> + assert_pipe_is_awake(dev_priv, pipe);
> +
>   spin_lock_irqsave(_priv->irq_lock, irqflags);
>   i915_enable_pipestat(dev_priv, pipe, PIPE_VBLANK_INTERRUPT_STATUS);
>   spin_unlock_irqrestore(_priv->irq_lock, irqflags);
> @@ -2735,6 +2746,9 @@ static int i965_enable_vblank(struct drm_device *dev, 
> unsigned int pipe)
>   struct drm_i915_private *dev_priv = to_i915(dev);
>   unsigned long irqflags;
>  
> + /* vblank IRQ requires the powerwell, held awake by the CRTC */
> + assert_pipe_is_awake(dev_priv, pipe);
> +
>   spin_lock_irqsave(_priv->irq_lock, irqflags);
>   i915_enable_pipestat(dev_priv, pipe,
>PIPE_START_VBLANK_INTERRUPT_STATUS);
> @@ -2750,6 +2764,9 @@ static int ironlake_enable_vblank(struct drm_device 
> *dev, unsigned int pipe)
>   uint32_t bit = INTEL_GEN(dev) >= 7 ?
>   DE_PIPE_VBLANK_IVB(pipe) : DE_PIPE_VBLANK(pipe);
>  
> + /* vblank IRQ requires the powerwell, held awake by the CRTC */
> + assert_pipe_is_awake(dev_priv, pipe);
> +
>   spin_lock_irqsave(_priv->irq_lock, irqflags);
>   ilk_enable_display_irq(dev_priv, bit);
>   spin_unlock_irqrestore(_priv->irq_lock, irqflags);
> @@ -2762,6 +2779,9 @@ static int gen8_enable_vblank(struct drm_device *dev, 
> unsigned int pipe)
>   struct drm_i915_private *dev_priv = to_i915(dev);
>   unsigned long irqflags;
>  
> + /* vblank IRQ requires the powerwell, held awake by the CRTC */
> + assert_pipe_is_awake(dev_priv, pipe);
> +
>   spin_lock_irqsave(_priv->irq_lock, irqflags);
>   bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK);
>   spin_unlock_irqrestore(_priv->irq_lock, irqflags);
> -- 
> 2.9.3

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

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

[Chris Wilson]

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.
v3: Keep the WARN_ON() tidy (avoid macro expansions that get recorded
in their completeness in the user string).

Signed-off-by: Chris Wilson 
Cc: Ville Syrjälä 
Cc: Maarten Lankhorst 
---
 drivers/gpu/drm/i915/i915_irq.c | 24 
 1 file changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1e43fe30da11..8e2eb5adde33 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2715,6 +2715,18 @@ 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)
+{
+   enum intel_display_power_domain pipe_domain;
+
+   if (!IS_ENABLED(CONFIG_DRM_I915_DEBUG))
+   return;
+
+   pipe_domain = POWER_DOMAIN_PIPE(pipe);
+   WARN_ON(!intel_display_power_is_enabled(dev_priv, pipe_domain));
+}
+
 /* Called from drm generic code, passed 'crtc' which
  * we use as a pipe index
  */
@@ -2723,6 +2735,9 @@ static int i8xx_enable_vblank(struct drm_device *dev, 
unsigned int pipe)
struct drm_i915_private *dev_priv = to_i915(dev);
unsigned long irqflags;
 
+   /* vblank IRQ requires the powerwell, held awake by the CRTC */
+   assert_pipe_is_awake(dev_priv, pipe);
+
spin_lock_irqsave(_priv->irq_lock, irqflags);
i915_enable_pipestat(dev_priv, pipe, PIPE_VBLANK_INTERRUPT_STATUS);
spin_unlock_irqrestore(_priv->irq_lock, irqflags);
@@ -2735,6 +2750,9 @@ static int i965_enable_vblank(struct drm_device *dev, 
unsigned int pipe)
struct drm_i915_private *dev_priv = to_i915(dev);
unsigned long irqflags;
 
+   /* vblank IRQ requires the powerwell, held awake by the CRTC */
+   assert_pipe_is_awake(dev_priv, pipe);
+
spin_lock_irqsave(_priv->irq_lock, irqflags);
i915_enable_pipestat(dev_priv, pipe,
 PIPE_START_VBLANK_INTERRUPT_STATUS);
@@ -2750,6 +2768,9 @@ static int ironlake_enable_vblank(struct drm_device *dev, 
unsigned int pipe)
uint32_t bit = INTEL_GEN(dev) >= 7 ?
DE_PIPE_VBLANK_IVB(pipe) : DE_PIPE_VBLANK(pipe);
 
+   /* vblank IRQ requires the powerwell, held awake by the CRTC */
+   assert_pipe_is_awake(dev_priv, pipe);
+
spin_lock_irqsave(_priv->irq_lock, irqflags);
ilk_enable_display_irq(dev_priv, bit);
spin_unlock_irqrestore(_priv->irq_lock, irqflags);
@@ -2762,6 +2783,9 @@ static int gen8_enable_vblank(struct drm_device *dev, 
unsigned int pipe)
struct drm_i915_private *dev_priv = to_i915(dev);
unsigned long irqflags;
 
+   /* vblank IRQ requires the powerwell, held awake by the CRTC */
+   assert_pipe_is_awake(dev_priv, pipe);
+
spin_lock_irqsave(_priv->irq_lock, irqflags);
bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK);
spin_unlock_irqrestore(_priv->irq_lock, irqflags);
-- 
2.9.3

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

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

[Chris Wilson]

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 
Cc: Ville Syrjälä 
Cc: Maarten Lankhorst 
---
 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)));
+}
+
 /* Called from drm generic code, passed 'crtc' which
  * we use as a pipe index
  */
@@ -2723,6 +2731,9 @@ static int i8xx_enable_vblank(struct drm_device *dev, 
unsigned int pipe)
struct drm_i915_private *dev_priv = to_i915(dev);
unsigned long irqflags;
 
+   /* vblank IRQ requires the powerwell, held awake by the CRTC */
+   assert_pipe_is_awake(dev_priv, pipe);
+
spin_lock_irqsave(_priv->irq_lock, irqflags);
i915_enable_pipestat(dev_priv, pipe, PIPE_VBLANK_INTERRUPT_STATUS);
spin_unlock_irqrestore(_priv->irq_lock, irqflags);
@@ -2735,6 +2746,9 @@ static int i965_enable_vblank(struct drm_device *dev, 
unsigned int pipe)
struct drm_i915_private *dev_priv = to_i915(dev);
unsigned long irqflags;
 
+   /* vblank IRQ requires the powerwell, held awake by the CRTC */
+   assert_pipe_is_awake(dev_priv, pipe);
+
spin_lock_irqsave(_priv->irq_lock, irqflags);
i915_enable_pipestat(dev_priv, pipe,
 PIPE_START_VBLANK_INTERRUPT_STATUS);
@@ -2750,6 +2764,9 @@ static int ironlake_enable_vblank(struct drm_device *dev, 
unsigned int pipe)
uint32_t bit = INTEL_GEN(dev) >= 7 ?
DE_PIPE_VBLANK_IVB(pipe) : DE_PIPE_VBLANK(pipe);
 
+   /* vblank IRQ requires the powerwell, held awake by the CRTC */
+   assert_pipe_is_awake(dev_priv, pipe);
+
spin_lock_irqsave(_priv->irq_lock, irqflags);
ilk_enable_display_irq(dev_priv, bit);
spin_unlock_irqrestore(_priv->irq_lock, irqflags);
@@ -2762,6 +2779,9 @@ static int gen8_enable_vblank(struct drm_device *dev, 
unsigned int pipe)
struct drm_i915_private *dev_priv = to_i915(dev);
unsigned long irqflags;
 
+   /* vblank IRQ requires the powerwell, held awake by the CRTC */
+   assert_pipe_is_awake(dev_priv, pipe);
+
spin_lock_irqsave(_priv->irq_lock, irqflags);
bdw_enable_pipe_irq(dev_priv, pipe, GEN8_PIPE_VBLANK);
spin_unlock_irqrestore(_priv->irq_lock, irqflags);
-- 
2.9.3

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