Re: [Intel-gfx] [PATCH 4/5] drm/i915: Introduce a non-blocking power domain for vblank interrupts

[Pandiyan, Dhinakaran]

On Tue, 2018-01-09 at 10:38 +0100, Maarten Lankhorst wrote:
> Op 08-01-18 om 20:59 schreef Pandiyan, Dhinakaran:
> > On Thu, 2018-01-04 at 18:08 -0800, Rodrigo Vivi wrote:

> >> I will probably have more comments later, but just doing a brain dump now
> >> since I end up forgetting to write yesterday...
> >>
> >> The approach here in general is good and much better than that pre,post 
> >> hooks.
> >> But I just believe we can do this here in a more generic approach than 
> >> deviating
> >> the initial power well and domains.
> > I would have liked a generic approach (for display_power_{get,put}), but
> > I think this case is special enough that making it stand out is better.
> Agreed, I can think of no other way myself without making the generic case 
> too complicated. The whole runtime power management became way too complex 
> for a special case.
> 
I found out that DMC keeps the hardware out of DC5/6 when vblank
interrupts are enabled. This simplified the solution a lot
(https://patchwork.freedesktop.org/series/36435/) Thanks for your review
on this series, would appreciate any feedback on the new one too :)

-DK



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

Re: [Intel-gfx] [PATCH 4/5] drm/i915: Introduce a non-blocking power domain for vblank interrupts

[Maarten Lankhorst]

Op 08-01-18 om 20:59 schreef Pandiyan, Dhinakaran:
> On Thu, 2018-01-04 at 18:08 -0800, Rodrigo Vivi wrote:
>> On Wed, Jan 03, 2018 at 08:40:00PM +, Dhinakaran Pandiyan wrote:
>>> When DC states are enabled and PSR is active, the hardware enters DC5/DC6
>>> states resulting in the frame counter resetting. The frame counter reset
>>> mess up the vblank counting logic as the diff between the new frame
>>> counter value and the previous is negative, and this negative diff gets
>>> applied as an unsigned value to the vblank count. We cannot reject negative
>>> diffs altogether because they can arise from legitimate frame counter
>>> overflows when there is a long period with vblank disabled. So, this
>>> approach allows for the driver to notice a DC state toggle between a vblank
>>> disable and enable and fill in the missed vblanks.
>>>
>>> But, in order to disable DC states when vblank interrupts are required,
>>> the DC_OFF power well has to be disabled in an atomic context. So,
>>> introduce a new VBLANK power domain that can be acquired and released in
>>> atomic contexts with these changes -
>>> 1)  _vblank_get() and _vblank_put() methods skip the power_domain mutex
>>> and use a spin lock for the DC power well.
>>> 2) power_domains->domain_use_count is converted to an atomic_t array so
>>> that it can be updated outside of the power domain mutex.
>>>
>>> v3: Squash domain_use_count atomic_t conversion (Maarten)
>>> v2: Fix deadlock by switching irqsave spinlock.
>>> Implement atomic version of get_if_enabled.
>>> Modify power_domain_verify_state to check power well use count and
>>> enabled status atomically.
>>> Rewrite of intel_power_well_{get,put}
>>>
>>> Cc: Maarten Lankhorst 
>>> Cc: Daniel Vetter 
>>> Cc: Ville Syrjälä 
>>> Cc: Rodrigo Vivi 
>>> Signed-off-by: Dhinakaran Pandiyan 
>>> ---
>>>  drivers/gpu/drm/i915/i915_debugfs.c |   2 +-
>>>  drivers/gpu/drm/i915/i915_drv.h |  19 +++-
>>>  drivers/gpu/drm/i915/intel_display.h|   1 +
>>>  drivers/gpu/drm/i915/intel_drv.h|   3 +
>>>  drivers/gpu/drm/i915/intel_runtime_pm.c | 195 
>>> 
>>>  5 files changed, 199 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
>>> b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index d81cb2513069..5a7ce734de02 100644
>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> @@ -2746,7 +2746,7 @@ static int i915_power_domain_info(struct seq_file *m, 
>>> void *unused)
>>> for_each_power_domain(power_domain, power_well->domains)
>>> seq_printf(m, "  %-23s %d\n",
>>>  intel_display_power_domain_str(power_domain),
>>> -power_domains->domain_use_count[power_domain]);
>>> +
>>> atomic_read(_domains->domain_use_count[power_domain]));
>>> }
>>>  
>>> mutex_unlock(_domains->lock);
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>>> b/drivers/gpu/drm/i915/i915_drv.h
>>> index caebd5825279..61a635f03af7 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -1032,6 +1032,23 @@ struct i915_power_well {
>>> bool has_fuses:1;
>>> } hsw;
>>> };
>>> +
>>> +   /* Lock to serialize access to count, hw_enabled and ops, used for
>>> +* power wells that have supports_atomix_ctx set to True.
>>> +*/
>>> +   spinlock_t lock;
>>> +
>>> +   /* Indicates that the get/put methods for this power well can be called
>>> +* in atomic contexts, requires .ops to not sleep. This is valid
>>> +* only for the DC_OFF power well currently.
>>> +*/
>>> +   bool supports_atomic_ctx;
>>> +
>>> +   /* DC_OFF power well was disabled since the last time vblanks were
>>> +* disabled.
>>> +*/
>>> +   bool dc_off_disabled;
>>> +
>>> const struct i915_power_well_ops *ops;
>>>  };
>>>  
>>> @@ -1045,7 +1062,7 @@ struct i915_power_domains {
>>> int power_well_count;
>>>  
>>> struct mutex lock;
>>> -   int domain_use_count[POWER_DOMAIN_NUM];
>>> +   atomic_t domain_use_count[POWER_DOMAIN_NUM];
>>> struct i915_power_well *power_wells;
>>>  };
>>>  
>>> diff --git a/drivers/gpu/drm/i915/intel_display.h 
>>> b/drivers/gpu/drm/i915/intel_display.h
>>> index a0d2b6169361..3e9671ff6f79 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.h
>>> +++ b/drivers/gpu/drm/i915/intel_display.h
>>> @@ -172,6 +172,7 @@ enum intel_display_power_domain {
>>> POWER_DOMAIN_AUX_C,
>>> POWER_DOMAIN_AUX_D,
>>> POWER_DOMAIN_GMBUS,
>>> +   POWER_DOMAIN_VBLANK,
>> Maybe we could start a new category of atomic domains and on interations 
>> that makes sense go over both
>> or making the domains in an array with is_atomic bool in case we can 
>> transform 

Re: [Intel-gfx] [PATCH 4/5] drm/i915: Introduce a non-blocking power domain for vblank interrupts

[Pandiyan, Dhinakaran]

On Thu, 2018-01-04 at 18:08 -0800, Rodrigo Vivi wrote:
> On Wed, Jan 03, 2018 at 08:40:00PM +, Dhinakaran Pandiyan wrote:
> > When DC states are enabled and PSR is active, the hardware enters DC5/DC6
> > states resulting in the frame counter resetting. The frame counter reset
> > mess up the vblank counting logic as the diff between the new frame
> > counter value and the previous is negative, and this negative diff gets
> > applied as an unsigned value to the vblank count. We cannot reject negative
> > diffs altogether because they can arise from legitimate frame counter
> > overflows when there is a long period with vblank disabled. So, this
> > approach allows for the driver to notice a DC state toggle between a vblank
> > disable and enable and fill in the missed vblanks.
> > 
> > But, in order to disable DC states when vblank interrupts are required,
> > the DC_OFF power well has to be disabled in an atomic context. So,
> > introduce a new VBLANK power domain that can be acquired and released in
> > atomic contexts with these changes -
> > 1)  _vblank_get() and _vblank_put() methods skip the power_domain mutex
> > and use a spin lock for the DC power well.
> > 2) power_domains->domain_use_count is converted to an atomic_t array so
> > that it can be updated outside of the power domain mutex.
> > 
> > v3: Squash domain_use_count atomic_t conversion (Maarten)
> > v2: Fix deadlock by switching irqsave spinlock.
> > Implement atomic version of get_if_enabled.
> > Modify power_domain_verify_state to check power well use count and
> > enabled status atomically.
> > Rewrite of intel_power_well_{get,put}
> > 
> > Cc: Maarten Lankhorst 
> > Cc: Daniel Vetter 
> > Cc: Ville Syrjälä 
> > Cc: Rodrigo Vivi 
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c |   2 +-
> >  drivers/gpu/drm/i915/i915_drv.h |  19 +++-
> >  drivers/gpu/drm/i915/intel_display.h|   1 +
> >  drivers/gpu/drm/i915/intel_drv.h|   3 +
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 195 
> > 
> >  5 files changed, 199 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index d81cb2513069..5a7ce734de02 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2746,7 +2746,7 @@ static int i915_power_domain_info(struct seq_file *m, 
> > void *unused)
> > for_each_power_domain(power_domain, power_well->domains)
> > seq_printf(m, "  %-23s %d\n",
> >  intel_display_power_domain_str(power_domain),
> > -power_domains->domain_use_count[power_domain]);
> > +
> > atomic_read(_domains->domain_use_count[power_domain]));
> > }
> >  
> > mutex_unlock(_domains->lock);
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index caebd5825279..61a635f03af7 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1032,6 +1032,23 @@ struct i915_power_well {
> > bool has_fuses:1;
> > } hsw;
> > };
> > +
> > +   /* Lock to serialize access to count, hw_enabled and ops, used for
> > +* power wells that have supports_atomix_ctx set to True.
> > +*/
> > +   spinlock_t lock;
> > +
> > +   /* Indicates that the get/put methods for this power well can be called
> > +* in atomic contexts, requires .ops to not sleep. This is valid
> > +* only for the DC_OFF power well currently.
> > +*/
> > +   bool supports_atomic_ctx;
> > +
> > +   /* DC_OFF power well was disabled since the last time vblanks were
> > +* disabled.
> > +*/
> > +   bool dc_off_disabled;
> > +
> > const struct i915_power_well_ops *ops;
> >  };
> >  
> > @@ -1045,7 +1062,7 @@ struct i915_power_domains {
> > int power_well_count;
> >  
> > struct mutex lock;
> > -   int domain_use_count[POWER_DOMAIN_NUM];
> > +   atomic_t domain_use_count[POWER_DOMAIN_NUM];
> > struct i915_power_well *power_wells;
> >  };
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_display.h 
> > b/drivers/gpu/drm/i915/intel_display.h
> > index a0d2b6169361..3e9671ff6f79 100644
> > --- a/drivers/gpu/drm/i915/intel_display.h
> > +++ b/drivers/gpu/drm/i915/intel_display.h
> > @@ -172,6 +172,7 @@ enum intel_display_power_domain {
> > POWER_DOMAIN_AUX_C,
> > POWER_DOMAIN_AUX_D,
> > POWER_DOMAIN_GMBUS,
> > +   POWER_DOMAIN_VBLANK,
> 
> Maybe we could start a new category of atomic domains and on interations that 
> makes sense go over both
> or making the domains in an array with is_atomic bool in case we can 
> transform the atomic get,put to
> be really generic or into 

Re: [Intel-gfx] [PATCH 4/5] drm/i915: Introduce a non-blocking power domain for vblank interrupts

[Rodrigo Vivi]

On Wed, Jan 03, 2018 at 08:40:00PM +, Dhinakaran Pandiyan wrote:
> When DC states are enabled and PSR is active, the hardware enters DC5/DC6
> states resulting in the frame counter resetting. The frame counter reset
> mess up the vblank counting logic as the diff between the new frame
> counter value and the previous is negative, and this negative diff gets
> applied as an unsigned value to the vblank count. We cannot reject negative
> diffs altogether because they can arise from legitimate frame counter
> overflows when there is a long period with vblank disabled. So, this
> approach allows for the driver to notice a DC state toggle between a vblank
> disable and enable and fill in the missed vblanks.
> 
> But, in order to disable DC states when vblank interrupts are required,
> the DC_OFF power well has to be disabled in an atomic context. So,
> introduce a new VBLANK power domain that can be acquired and released in
> atomic contexts with these changes -
> 1)  _vblank_get() and _vblank_put() methods skip the power_domain mutex
> and use a spin lock for the DC power well.
> 2) power_domains->domain_use_count is converted to an atomic_t array so
> that it can be updated outside of the power domain mutex.
> 
> v3: Squash domain_use_count atomic_t conversion (Maarten)
> v2: Fix deadlock by switching irqsave spinlock.
> Implement atomic version of get_if_enabled.
> Modify power_domain_verify_state to check power well use count and
> enabled status atomically.
> Rewrite of intel_power_well_{get,put}
> 
> Cc: Maarten Lankhorst 
> Cc: Daniel Vetter 
> Cc: Ville Syrjälä 
> Cc: Rodrigo Vivi 
> Signed-off-by: Dhinakaran Pandiyan 
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |   2 +-
>  drivers/gpu/drm/i915/i915_drv.h |  19 +++-
>  drivers/gpu/drm/i915/intel_display.h|   1 +
>  drivers/gpu/drm/i915/intel_drv.h|   3 +
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 195 
> 
>  5 files changed, 199 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index d81cb2513069..5a7ce734de02 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2746,7 +2746,7 @@ static int i915_power_domain_info(struct seq_file *m, 
> void *unused)
>   for_each_power_domain(power_domain, power_well->domains)
>   seq_printf(m, "  %-23s %d\n",
>intel_display_power_domain_str(power_domain),
> -  power_domains->domain_use_count[power_domain]);
> +  
> atomic_read(_domains->domain_use_count[power_domain]));
>   }
>  
>   mutex_unlock(_domains->lock);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index caebd5825279..61a635f03af7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1032,6 +1032,23 @@ struct i915_power_well {
>   bool has_fuses:1;
>   } hsw;
>   };
> +
> + /* Lock to serialize access to count, hw_enabled and ops, used for
> +  * power wells that have supports_atomix_ctx set to True.
> +  */
> + spinlock_t lock;
> +
> + /* Indicates that the get/put methods for this power well can be called
> +  * in atomic contexts, requires .ops to not sleep. This is valid
> +  * only for the DC_OFF power well currently.
> +  */
> + bool supports_atomic_ctx;
> +
> + /* DC_OFF power well was disabled since the last time vblanks were
> +  * disabled.
> +  */
> + bool dc_off_disabled;
> +
>   const struct i915_power_well_ops *ops;
>  };
>  
> @@ -1045,7 +1062,7 @@ struct i915_power_domains {
>   int power_well_count;
>  
>   struct mutex lock;
> - int domain_use_count[POWER_DOMAIN_NUM];
> + atomic_t domain_use_count[POWER_DOMAIN_NUM];
>   struct i915_power_well *power_wells;
>  };
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.h 
> b/drivers/gpu/drm/i915/intel_display.h
> index a0d2b6169361..3e9671ff6f79 100644
> --- a/drivers/gpu/drm/i915/intel_display.h
> +++ b/drivers/gpu/drm/i915/intel_display.h
> @@ -172,6 +172,7 @@ enum intel_display_power_domain {
>   POWER_DOMAIN_AUX_C,
>   POWER_DOMAIN_AUX_D,
>   POWER_DOMAIN_GMBUS,
> + POWER_DOMAIN_VBLANK,

Maybe we could start a new category of atomic domains and on interations that 
makes sense go over both
or making the domains in an array with is_atomic bool in case we can transform 
the atomic get,put to
be really generic or into .enable .disable function pointers.

>   POWER_DOMAIN_MODESET,
>   POWER_DOMAIN_GT_IRQ,
>   POWER_DOMAIN_INIT,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> b/drivers/gpu/drm/i915/intel_drv.h
> 

[Intel-gfx] [PATCH 4/5] drm/i915: Introduce a non-blocking power domain for vblank interrupts

[Dhinakaran Pandiyan]

When DC states are enabled and PSR is active, the hardware enters DC5/DC6
states resulting in the frame counter resetting. The frame counter reset
mess up the vblank counting logic as the diff between the new frame
counter value and the previous is negative, and this negative diff gets
applied as an unsigned value to the vblank count. We cannot reject negative
diffs altogether because they can arise from legitimate frame counter
overflows when there is a long period with vblank disabled. So, this
approach allows for the driver to notice a DC state toggle between a vblank
disable and enable and fill in the missed vblanks.

But, in order to disable DC states when vblank interrupts are required,
the DC_OFF power well has to be disabled in an atomic context. So,
introduce a new VBLANK power domain that can be acquired and released in
atomic contexts with these changes -
1)  _vblank_get() and _vblank_put() methods skip the power_domain mutex
and use a spin lock for the DC power well.
2) power_domains->domain_use_count is converted to an atomic_t array so
that it can be updated outside of the power domain mutex.

v3: Squash domain_use_count atomic_t conversion (Maarten)
v2: Fix deadlock by switching irqsave spinlock.
Implement atomic version of get_if_enabled.
Modify power_domain_verify_state to check power well use count and
enabled status atomically.
Rewrite of intel_power_well_{get,put}

Cc: Maarten Lankhorst 
Cc: Daniel Vetter 
Cc: Ville Syrjälä 
Cc: Rodrigo Vivi 
Signed-off-by: Dhinakaran Pandiyan 
---
 drivers/gpu/drm/i915/i915_debugfs.c |   2 +-
 drivers/gpu/drm/i915/i915_drv.h |  19 +++-
 drivers/gpu/drm/i915/intel_display.h|   1 +
 drivers/gpu/drm/i915/intel_drv.h|   3 +
 drivers/gpu/drm/i915/intel_runtime_pm.c | 195 
 5 files changed, 199 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index d81cb2513069..5a7ce734de02 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2746,7 +2746,7 @@ static int i915_power_domain_info(struct seq_file *m, 
void *unused)
for_each_power_domain(power_domain, power_well->domains)
seq_printf(m, "  %-23s %d\n",
 intel_display_power_domain_str(power_domain),
-power_domains->domain_use_count[power_domain]);
+
atomic_read(_domains->domain_use_count[power_domain]));
}
 
mutex_unlock(_domains->lock);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index caebd5825279..61a635f03af7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1032,6 +1032,23 @@ struct i915_power_well {
bool has_fuses:1;
} hsw;
};
+
+   /* Lock to serialize access to count, hw_enabled and ops, used for
+* power wells that have supports_atomix_ctx set to True.
+*/
+   spinlock_t lock;
+
+   /* Indicates that the get/put methods for this power well can be called
+* in atomic contexts, requires .ops to not sleep. This is valid
+* only for the DC_OFF power well currently.
+*/
+   bool supports_atomic_ctx;
+
+   /* DC_OFF power well was disabled since the last time vblanks were
+* disabled.
+*/
+   bool dc_off_disabled;
+
const struct i915_power_well_ops *ops;
 };
 
@@ -1045,7 +1062,7 @@ struct i915_power_domains {
int power_well_count;
 
struct mutex lock;
-   int domain_use_count[POWER_DOMAIN_NUM];
+   atomic_t domain_use_count[POWER_DOMAIN_NUM];
struct i915_power_well *power_wells;
 };
 
diff --git a/drivers/gpu/drm/i915/intel_display.h 
b/drivers/gpu/drm/i915/intel_display.h
index a0d2b6169361..3e9671ff6f79 100644
--- a/drivers/gpu/drm/i915/intel_display.h
+++ b/drivers/gpu/drm/i915/intel_display.h
@@ -172,6 +172,7 @@ enum intel_display_power_domain {
POWER_DOMAIN_AUX_C,
POWER_DOMAIN_AUX_D,
POWER_DOMAIN_GMBUS,
+   POWER_DOMAIN_VBLANK,
POWER_DOMAIN_MODESET,
POWER_DOMAIN_GT_IRQ,
POWER_DOMAIN_INIT,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 30f791f89d64..164e62cb047b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1797,6 +1797,9 @@ bool intel_display_power_get_if_enabled(struct 
drm_i915_private *dev_priv,
enum intel_display_power_domain domain);
 void intel_display_power_put(struct drm_i915_private *dev_priv,
 enum intel_display_power_domain domain);
+void intel_display_power_vblank_get(struct 

Re: [Intel-gfx] [PATCH 4/5] drm/i915: Introduce a non-blocking power domain for vblank interrupts

[Pandiyan, Dhinakaran]

On Wed, 2017-12-06 at 15:54 -0800, Rodrigo Vivi wrote:
> On Wed, Dec 06, 2017 at 10:47:40PM +, Dhinakaran Pandiyan wrote:
> > When DC states are enabled and PSR is active, the hardware enters DC5/DC6
> > states resulting in frame counter resets. The frame counter resets mess
> > up the vblank counting logic. So in order to disable DC states when
> > vblank interrupts are required and to disallow DC states when vblanks
> > interrupts are already enabled, introduce a new power domain. Since this
> > power domain reference needs to be acquired and released in atomic context,
> > the corresponding _get() and _put() methods skip the power_domain mutex.
> 
> \o/
> 
> > 
> > Signed-off-by: Dhinakaran Pandiyan 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |   5 ++
> >  drivers/gpu/drm/i915/intel_drv.h|   3 +-
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 125 
> > +++-
> >  3 files changed, 128 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 18d42885205b..ba9107ec1ed1 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -397,6 +397,7 @@ enum intel_display_power_domain {
> > POWER_DOMAIN_AUX_C,
> > POWER_DOMAIN_AUX_D,
> > POWER_DOMAIN_GMBUS,
> > +   POWER_DOMAIN_VBLANK,
> > POWER_DOMAIN_MODESET,
> > POWER_DOMAIN_INIT,
> >  
> > @@ -1475,6 +1476,10 @@ struct i915_power_well {
> > bool has_vga:1;
> > bool has_fuses:1;
> > } hsw;
> > +   struct {
> > +   spinlock_t lock;
> > +   bool was_disabled;
> > +   } dc_off;
> 
> what about a more generic name here?
> something like
> 
> + struct {
> + spinlock_t lock;
> + bool was_disabled;
> + } atomic;
> + is_atomic; //or needs_atomic
> 
> so...
> 
> > };
> > const struct i915_power_well_ops *ops;
> >  };
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 30f791f89d64..93ca503f18bb 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1865,7 +1865,8 @@ void chv_phy_powergate_lanes(struct intel_encoder 
> > *encoder,
> >  bool override, unsigned int mask);
> >  bool chv_phy_powergate_ch(struct drm_i915_private *dev_priv, enum dpio_phy 
> > phy,
> >   enum dpio_channel ch, bool override);
> > -
> > +bool intel_display_power_vblank_get(struct drm_i915_private *dev_priv);
> > +void intel_display_power_vblank_put(struct drm_i915_private *dev_priv);
> >  
> >  /* intel_pm.c */
> >  void intel_init_clock_gating(struct drm_i915_private *dev_priv);
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index f88f2c070c5f..f1807bd74242 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -126,6 +126,8 @@ intel_display_power_domain_str(enum 
> > intel_display_power_domain domain)
> > return "AUX_D";
> > case POWER_DOMAIN_GMBUS:
> > return "GMBUS";
> > +   case POWER_DOMAIN_VBLANK:
> > +   return "VBLANK";
> > case POWER_DOMAIN_INIT:
> > return "INIT";
> > case POWER_DOMAIN_MODESET:
> > @@ -196,10 +198,17 @@ bool __intel_display_power_is_enabled(struct 
> > drm_i915_private *dev_priv,
> > if (power_well->always_on)
> > continue;
> >  
> > -   if (!power_well->hw_enabled) {
> > +   if (power_well->id == SKL_DISP_PW_DC_OFF)
> > +   spin_lock(_well->dc_off.lock);
> 
> ... instead of a specif pw check here you would have
> 
> + if (power_well->is_atomic)
> + spin_lock(_well->atomic.lock)
> 
> > +
> > +   if (!power_well->hw_enabled)
> > is_enabled = false;
> > +
> > +   if (power_well->id == SKL_DISP_PW_DC_OFF)
> > +   spin_unlock(_well->dc_off.lock);
> > +
> > +   if (!is_enabled)
> > break;
> > -   }
> > }
> >  
> > return is_enabled;
> > @@ -724,6 +733,7 @@ static void gen9_dc_off_power_well_disable(struct 
> > drm_i915_private *dev_priv,
> > skl_enable_dc6(dev_priv);
> > else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
> > gen9_enable_dc5(dev_priv);
> > +   power_well->dc_off.was_disabled = true;
> >  }
> >  
> >  static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv,
> > @@ -1441,6 +1451,77 @@ static void chv_pipe_power_well_disable(struct 
> > drm_i915_private *dev_priv,
> > chv_set_pipe_power_well(dev_priv, power_well, false);
> >  }
> >  
> > +/**
> > + * intel_display_power_vblank_get - acquire a VBLANK power domain 
> > reference 

Re: [Intel-gfx] [PATCH 4/5] drm/i915: Introduce a non-blocking power domain for vblank interrupts

[Rodrigo Vivi]

On Wed, Dec 06, 2017 at 10:47:40PM +, Dhinakaran Pandiyan wrote:
> When DC states are enabled and PSR is active, the hardware enters DC5/DC6
> states resulting in frame counter resets. The frame counter resets mess
> up the vblank counting logic. So in order to disable DC states when
> vblank interrupts are required and to disallow DC states when vblanks
> interrupts are already enabled, introduce a new power domain. Since this
> power domain reference needs to be acquired and released in atomic context,
> the corresponding _get() and _put() methods skip the power_domain mutex.

\o/

> 
> Signed-off-by: Dhinakaran Pandiyan 
> ---
>  drivers/gpu/drm/i915/i915_drv.h |   5 ++
>  drivers/gpu/drm/i915/intel_drv.h|   3 +-
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 125 
> +++-
>  3 files changed, 128 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 18d42885205b..ba9107ec1ed1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -397,6 +397,7 @@ enum intel_display_power_domain {
>   POWER_DOMAIN_AUX_C,
>   POWER_DOMAIN_AUX_D,
>   POWER_DOMAIN_GMBUS,
> + POWER_DOMAIN_VBLANK,
>   POWER_DOMAIN_MODESET,
>   POWER_DOMAIN_INIT,
>  
> @@ -1475,6 +1476,10 @@ struct i915_power_well {
>   bool has_vga:1;
>   bool has_fuses:1;
>   } hsw;
> + struct {
> + spinlock_t lock;
> + bool was_disabled;
> + } dc_off;

what about a more generic name here?
something like

+   struct {
+   spinlock_t lock;
+   bool was_disabled;
+   } atomic;
+   is_atomic; //or needs_atomic

so...

>   };
>   const struct i915_power_well_ops *ops;
>  };
> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> b/drivers/gpu/drm/i915/intel_drv.h
> index 30f791f89d64..93ca503f18bb 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1865,7 +1865,8 @@ void chv_phy_powergate_lanes(struct intel_encoder 
> *encoder,
>bool override, unsigned int mask);
>  bool chv_phy_powergate_ch(struct drm_i915_private *dev_priv, enum dpio_phy 
> phy,
> enum dpio_channel ch, bool override);
> -
> +bool intel_display_power_vblank_get(struct drm_i915_private *dev_priv);
> +void intel_display_power_vblank_put(struct drm_i915_private *dev_priv);
>  
>  /* intel_pm.c */
>  void intel_init_clock_gating(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index f88f2c070c5f..f1807bd74242 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -126,6 +126,8 @@ intel_display_power_domain_str(enum 
> intel_display_power_domain domain)
>   return "AUX_D";
>   case POWER_DOMAIN_GMBUS:
>   return "GMBUS";
> + case POWER_DOMAIN_VBLANK:
> + return "VBLANK";
>   case POWER_DOMAIN_INIT:
>   return "INIT";
>   case POWER_DOMAIN_MODESET:
> @@ -196,10 +198,17 @@ bool __intel_display_power_is_enabled(struct 
> drm_i915_private *dev_priv,
>   if (power_well->always_on)
>   continue;
>  
> - if (!power_well->hw_enabled) {
> + if (power_well->id == SKL_DISP_PW_DC_OFF)
> + spin_lock(_well->dc_off.lock);

... instead of a specif pw check here you would have

+   if (power_well->is_atomic)
+   spin_lock(_well->atomic.lock)

> +
> + if (!power_well->hw_enabled)
>   is_enabled = false;
> +
> + if (power_well->id == SKL_DISP_PW_DC_OFF)
> + spin_unlock(_well->dc_off.lock);
> +
> + if (!is_enabled)
>   break;
> - }
>   }
>  
>   return is_enabled;
> @@ -724,6 +733,7 @@ static void gen9_dc_off_power_well_disable(struct 
> drm_i915_private *dev_priv,
>   skl_enable_dc6(dev_priv);
>   else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
>   gen9_enable_dc5(dev_priv);
> + power_well->dc_off.was_disabled = true;
>  }
>  
>  static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv,
> @@ -1441,6 +1451,77 @@ static void chv_pipe_power_well_disable(struct 
> drm_i915_private *dev_priv,
>   chv_set_pipe_power_well(dev_priv, power_well, false);
>  }
>  
> +/**
> + * intel_display_power_vblank_get - acquire a VBLANK power domain reference 
> atomically
> + * @dev_priv: i915 device instance
> + *
> + * This function gets a POWER_DOMAIN_VBLANK reference without blocking and
> + * returns true if the DC_OFF power well was disabled since this function was
> + * 

[Intel-gfx] [PATCH 4/5] drm/i915: Introduce a non-blocking power domain for vblank interrupts

[Dhinakaran Pandiyan]

When DC states are enabled and PSR is active, the hardware enters DC5/DC6
states resulting in frame counter resets. The frame counter resets mess
up the vblank counting logic. So in order to disable DC states when
vblank interrupts are required and to disallow DC states when vblanks
interrupts are already enabled, introduce a new power domain. Since this
power domain reference needs to be acquired and released in atomic context,
the corresponding _get() and _put() methods skip the power_domain mutex.

Signed-off-by: Dhinakaran Pandiyan 
---
 drivers/gpu/drm/i915/i915_drv.h |   5 ++
 drivers/gpu/drm/i915/intel_drv.h|   3 +-
 drivers/gpu/drm/i915/intel_runtime_pm.c | 125 +++-
 3 files changed, 128 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 18d42885205b..ba9107ec1ed1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -397,6 +397,7 @@ enum intel_display_power_domain {
POWER_DOMAIN_AUX_C,
POWER_DOMAIN_AUX_D,
POWER_DOMAIN_GMBUS,
+   POWER_DOMAIN_VBLANK,
POWER_DOMAIN_MODESET,
POWER_DOMAIN_INIT,
 
@@ -1475,6 +1476,10 @@ struct i915_power_well {
bool has_vga:1;
bool has_fuses:1;
} hsw;
+   struct {
+   spinlock_t lock;
+   bool was_disabled;
+   } dc_off;
};
const struct i915_power_well_ops *ops;
 };
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 30f791f89d64..93ca503f18bb 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1865,7 +1865,8 @@ void chv_phy_powergate_lanes(struct intel_encoder 
*encoder,
 bool override, unsigned int mask);
 bool chv_phy_powergate_ch(struct drm_i915_private *dev_priv, enum dpio_phy phy,
  enum dpio_channel ch, bool override);
-
+bool intel_display_power_vblank_get(struct drm_i915_private *dev_priv);
+void intel_display_power_vblank_put(struct drm_i915_private *dev_priv);
 
 /* intel_pm.c */
 void intel_init_clock_gating(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index f88f2c070c5f..f1807bd74242 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -126,6 +126,8 @@ intel_display_power_domain_str(enum 
intel_display_power_domain domain)
return "AUX_D";
case POWER_DOMAIN_GMBUS:
return "GMBUS";
+   case POWER_DOMAIN_VBLANK:
+   return "VBLANK";
case POWER_DOMAIN_INIT:
return "INIT";
case POWER_DOMAIN_MODESET:
@@ -196,10 +198,17 @@ bool __intel_display_power_is_enabled(struct 
drm_i915_private *dev_priv,
if (power_well->always_on)
continue;
 
-   if (!power_well->hw_enabled) {
+   if (power_well->id == SKL_DISP_PW_DC_OFF)
+   spin_lock(_well->dc_off.lock);
+
+   if (!power_well->hw_enabled)
is_enabled = false;
+
+   if (power_well->id == SKL_DISP_PW_DC_OFF)
+   spin_unlock(_well->dc_off.lock);
+
+   if (!is_enabled)
break;
-   }
}
 
return is_enabled;
@@ -724,6 +733,7 @@ static void gen9_dc_off_power_well_disable(struct 
drm_i915_private *dev_priv,
skl_enable_dc6(dev_priv);
else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
gen9_enable_dc5(dev_priv);
+   power_well->dc_off.was_disabled = true;
 }
 
 static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv,
@@ -1441,6 +1451,77 @@ static void chv_pipe_power_well_disable(struct 
drm_i915_private *dev_priv,
chv_set_pipe_power_well(dev_priv, power_well, false);
 }
 
+/**
+ * intel_display_power_vblank_get - acquire a VBLANK power domain reference 
atomically
+ * @dev_priv: i915 device instance
+ *
+ * This function gets a POWER_DOMAIN_VBLANK reference without blocking and
+ * returns true if the DC_OFF power well was disabled since this function was
+ * called the last time.
+ */
+bool intel_display_power_vblank_get(struct drm_i915_private *dev_priv)
+{
+   struct i915_power_domains *power_domains = _priv->power_domains;
+   struct i915_power_well *power_well;
+   bool needs_restore = false;
+
+   if (!HAS_CSR(dev_priv) || !dev_priv->psr.source_ok)
+   return false;
+
+   /* The corresponding CRTC should be active by the time driver turns on
+* vblank interrupts, which in turn means the enabled pipe power domain
+* would have acquired the device runtime pm reference.
+*/
+