[PATCH] drm/i915: Add documentation to gen9_set_dc_state()

[PATCH] drm/i915: Add documentation to gen9_set_dc_state()

[Imre Deak]

Add documentation to gen9_set_dc_state() on what enabling a given DC
state means and at what point HW/DMC actually enters/exits these states.

Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

[ On IRC I stated that PSR entry would be prevented in a given DC state,
  but looking more at it I haven't found any proof for this. So as I
  understand the only connection between PSR and DC states is that if
  DC5/6 is disabled power saving will be blocked, which would otherwise
  be possible when PSR is active and the display pipe is off. ]

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 53ea564f971e..40a7955886d4 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -542,6 +542,29 @@ void gen9_sanitize_dc_state(struct drm_i915_private *dev_priv)
 	dev_priv->csr.dc_state = val;
 }
 
+/**
+ * gen9_set_dc_state - set target display C power state
+ * @dev_priv: i915 device instance
+ * @state: target DC power state
+ * - DC_STATE_DISABLE
+ * - DC_STATE_EN_UPTO_DC5
+ * - DC_STATE_EN_UPTO_DC6
+ * - DC_STATE_EN_DC9
+ *
+ * Signal to DMC firmware/HW the target DC power state passed in @state.
+ * DMC/HW can turn off individual display clocks and power rails when entering
+ * a deeper DC power state (higher in number) and turns these back when exiting
+ * that state to a shallower power state (lower in number). The HW will decide
+ * when to actually enter a given state on an on-demand basis, for instance
+ * depending on the active state of display pipes. The state of display
+ * registers backed by affected power rails are saved/restored as needed.
+ *
+ * Based on the above enabling a deeper DC power state is asynchronous wrt.
+ * enabling it. Disabling a deeper power state is synchronous: for instance
+ * setting %DC_STATE_DISABLE won't complete until all HW resources are turned
+ * back on and register state is restored. This is guaranteed by the MMIO write
+ * to DC_STATE_EN blocking until the state is restored.
+ */
 static void gen9_set_dc_state(struct drm_i915_private *dev_priv, uint32_t state)
 {
 	uint32_t val;
-- 
2.13.2

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

✓ Fi.CI.BAT: success for drm/i915: Add documentation to gen9_set_dc_state()

[Patchwork]

== Series Details ==

Series: drm/i915: Add documentation to gen9_set_dc_state()
URL   : https://patchwork.freedesktop.org/series/41811/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4059 -> Patchwork_8705 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/41811/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_8705 that come from known issues:

  === IGT changes ===

    ==== Possible fixes ====

    igt@gem_exec_suspend@basic-s3:
      fi-ivb-3520m:       DMESG-WARN (fdo#106084) -> PASS +1

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
      fi-elk-e7500:       INCOMPLETE (fdo#103989) -> PASS

    
  fdo#103989 https://bugs.freedesktop.org/show_bug.cgi?id=103989
  fdo#106084 https://bugs.freedesktop.org/show_bug.cgi?id=106084


== Participating hosts (36 -> 32) ==

  Missing    (4): fi-ctg-p8600 fi-ilk-m540 fi-glk-1 fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4059 -> Patchwork_8705

  CI_DRM_4059: c1645edc253f2b52a8c94565a75b479a6782e75f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4435: ddbe5a4d8bb1780ecf07f72e815062d3bce8ff71 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8705: 0aa28a05f4e92e6d5d1dd2824b76b515782989a1 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4435: e60d247eb359f044caf0c09904da14e39d7adca1 @ git://anongit.freedesktop.org/piglit


== Linux commits ==

0aa28a05f4e9 drm/i915: Add documentation to gen9_set_dc_state()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8705/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: Add documentation to gen9_set_dc_state()

[Patchwork]

== Series Details ==

Series: drm/i915: Add documentation to gen9_set_dc_state()
URL   : https://patchwork.freedesktop.org/series/41811/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4059_full -> Patchwork_8705_full =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/41811/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_8705_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_workarounds@suspend-resume-context:
      shard-kbl:          PASS -> INCOMPLETE (fdo#103665)

    igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
      shard-hsw:          PASS -> FAIL (fdo#102887)

    igt@kms_setmode@basic:
      shard-kbl:          PASS -> FAIL (fdo#99912)

    
    ==== Possible fixes ====

    igt@kms_flip@2x-flip-vs-expired-vblank:
      shard-hsw:          FAIL (fdo#102887) -> PASS

    igt@kms_flip@flip-vs-expired-vblank-interruptible:
      shard-apl:          FAIL (fdo#102887, fdo#105363) -> PASS

    igt@kms_flip@modeset-vs-vblank-race:
      shard-hsw:          FAIL (fdo#103060) -> PASS

    
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (6 -> 4) ==

  Missing    (2): shard-glk shard-glkb 


== Build changes ==

    * Linux: CI_DRM_4059 -> Patchwork_8705

  CI_DRM_4059: c1645edc253f2b52a8c94565a75b479a6782e75f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4435: ddbe5a4d8bb1780ecf07f72e815062d3bce8ff71 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_8705: 0aa28a05f4e92e6d5d1dd2824b76b515782989a1 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4435: e60d247eb359f044caf0c09904da14e39d7adca1 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8705/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add documentation to gen9_set_dc_state()

[Daniel Vetter]

On Tue, Apr 17, 2018 at 02:31:47PM +0300, Imre Deak wrote:
> Add documentation to gen9_set_dc_state() on what enabling a given DC
> state means and at what point HW/DMC actually enters/exits these states.
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

lgtm.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> [ On IRC I stated that PSR entry would be prevented in a given DC state,
>   but looking more at it I haven't found any proof for this. So as I
>   understand the only connection between PSR and DC states is that if
>   DC5/6 is disabled power saving will be blocked, which would otherwise
>   be possible when PSR is active and the display pipe is off. ]
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 53ea564f971e..40a7955886d4 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -542,6 +542,29 @@ void gen9_sanitize_dc_state(struct drm_i915_private *dev_priv)
>  	dev_priv->csr.dc_state = val;
>  }
>  
> +/**
> + * gen9_set_dc_state - set target display C power state
> + * @dev_priv: i915 device instance
> + * @state: target DC power state
> + * - DC_STATE_DISABLE
> + * - DC_STATE_EN_UPTO_DC5
> + * - DC_STATE_EN_UPTO_DC6
> + * - DC_STATE_EN_DC9
> + *
> + * Signal to DMC firmware/HW the target DC power state passed in @state.
> + * DMC/HW can turn off individual display clocks and power rails when entering
> + * a deeper DC power state (higher in number) and turns these back when exiting
> + * that state to a shallower power state (lower in number). The HW will decide
> + * when to actually enter a given state on an on-demand basis, for instance
> + * depending on the active state of display pipes. The state of display
> + * registers backed by affected power rails are saved/restored as needed.
> + *
> + * Based on the above enabling a deeper DC power state is asynchronous wrt.
> + * enabling it. Disabling a deeper power state is synchronous: for instance
> + * setting %DC_STATE_DISABLE won't complete until all HW resources are turned
> + * back on and register state is restored. This is guaranteed by the MMIO write
> + * to DC_STATE_EN blocking until the state is restored.
> + */
>  static void gen9_set_dc_state(struct drm_i915_private *dev_priv, uint32_t state)
>  {
>  	uint32_t val;
> -- 
> 2.13.2
> 

-- 
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: [PATCH] drm/i915: Add documentation to gen9_set_dc_state()

[Jani Nikula]

Cc: Rodrigo, DK, Ville

On Tue, 17 Apr 2018, Imre Deak <imre.deak@intel.com> wrote:
> Add documentation to gen9_set_dc_state() on what enabling a given DC
> state means and at what point HW/DMC actually enters/exits these states.
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> [ On IRC I stated that PSR entry would be prevented in a given DC state,
>   but looking more at it I haven't found any proof for this. So as I
>   understand the only connection between PSR and DC states is that if
>   DC5/6 is disabled power saving will be blocked, which would otherwise
>   be possible when PSR is active and the display pipe is off. ]

I think I'm still missing a definitive answer to the question, who
disables PSR before DP AUX transactions?

Does intel_display_power_get(dev_priv, intel_dp->aux_power_domain) in
pps_lock() cause PSR exit, through the DC state change? If yes, this
needs to be properly documented in code. Maybe with a WARN_ON(psr
active) on top.

Quoting bspec 7530, "DDI A AUX channel transactions must not be sent
while SRD is enabled. SRD must be completely disabled before a DDI A AUX
channel transaction can be sent."

I'm also confused how sink CRC would ever work with PSR enabled.

BR,
Jani.


>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 53ea564f971e..40a7955886d4 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -542,6 +542,29 @@ void gen9_sanitize_dc_state(struct drm_i915_private *dev_priv)
>  	dev_priv->csr.dc_state = val;
>  }
>  
> +/**
> + * gen9_set_dc_state - set target display C power state
> + * @dev_priv: i915 device instance
> + * @state: target DC power state
> + * - DC_STATE_DISABLE
> + * - DC_STATE_EN_UPTO_DC5
> + * - DC_STATE_EN_UPTO_DC6
> + * - DC_STATE_EN_DC9
> + *
> + * Signal to DMC firmware/HW the target DC power state passed in @state.
> + * DMC/HW can turn off individual display clocks and power rails when entering
> + * a deeper DC power state (higher in number) and turns these back when exiting
> + * that state to a shallower power state (lower in number). The HW will decide
> + * when to actually enter a given state on an on-demand basis, for instance
> + * depending on the active state of display pipes. The state of display
> + * registers backed by affected power rails are saved/restored as needed.
> + *
> + * Based on the above enabling a deeper DC power state is asynchronous wrt.
> + * enabling it. Disabling a deeper power state is synchronous: for instance
> + * setting %DC_STATE_DISABLE won't complete until all HW resources are turned
> + * back on and register state is restored. This is guaranteed by the MMIO write
> + * to DC_STATE_EN blocking until the state is restored.
> + */
>  static void gen9_set_dc_state(struct drm_i915_private *dev_priv, uint32_t state)
>  {
>  	uint32_t val;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add documentation to gen9_set_dc_state()

[Jani Nikula]

Argh, now with Ville's correct address.

On Wed, 25 Apr 2018, Jani Nikula <jani.nikula@intel.com> wrote:
> Cc: Rodrigo, DK, Ville
>
> On Tue, 17 Apr 2018, Imre Deak <imre.deak@intel.com> wrote:
>> Add documentation to gen9_set_dc_state() on what enabling a given DC
>> state means and at what point HW/DMC actually enters/exits these states.
>>
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_runtime_pm.c | 23 +++++++++++++++++++++++
>>  1 file changed, 23 insertions(+)
>>
>> [ On IRC I stated that PSR entry would be prevented in a given DC state,
>>   but looking more at it I haven't found any proof for this. So as I
>>   understand the only connection between PSR and DC states is that if
>>   DC5/6 is disabled power saving will be blocked, which would otherwise
>>   be possible when PSR is active and the display pipe is off. ]
>
> I think I'm still missing a definitive answer to the question, who
> disables PSR before DP AUX transactions?
>
> Does intel_display_power_get(dev_priv, intel_dp->aux_power_domain) in
> pps_lock() cause PSR exit, through the DC state change? If yes, this
> needs to be properly documented in code. Maybe with a WARN_ON(psr
> active) on top.
>
> Quoting bspec 7530, "DDI A AUX channel transactions must not be sent
> while SRD is enabled. SRD must be completely disabled before a DDI A AUX
> channel transaction can be sent."
>
> I'm also confused how sink CRC would ever work with PSR enabled.
>
> BR,
> Jani.
>
>
>>
>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> index 53ea564f971e..40a7955886d4 100644
>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> @@ -542,6 +542,29 @@ void gen9_sanitize_dc_state(struct drm_i915_private *dev_priv)
>>  	dev_priv->csr.dc_state = val;
>>  }
>>  
>> +/**
>> + * gen9_set_dc_state - set target display C power state
>> + * @dev_priv: i915 device instance
>> + * @state: target DC power state
>> + * - DC_STATE_DISABLE
>> + * - DC_STATE_EN_UPTO_DC5
>> + * - DC_STATE_EN_UPTO_DC6
>> + * - DC_STATE_EN_DC9
>> + *
>> + * Signal to DMC firmware/HW the target DC power state passed in @state.
>> + * DMC/HW can turn off individual display clocks and power rails when entering
>> + * a deeper DC power state (higher in number) and turns these back when exiting
>> + * that state to a shallower power state (lower in number). The HW will decide
>> + * when to actually enter a given state on an on-demand basis, for instance
>> + * depending on the active state of display pipes. The state of display
>> + * registers backed by affected power rails are saved/restored as needed.
>> + *
>> + * Based on the above enabling a deeper DC power state is asynchronous wrt.
>> + * enabling it. Disabling a deeper power state is synchronous: for instance
>> + * setting %DC_STATE_DISABLE won't complete until all HW resources are turned
>> + * back on and register state is restored. This is guaranteed by the MMIO write
>> + * to DC_STATE_EN blocking until the state is restored.
>> + */
>>  static void gen9_set_dc_state(struct drm_i915_private *dev_priv, uint32_t state)
>>  {
>>  	uint32_t val;

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add documentation to gen9_set_dc_state()

[Imre Deak]

On Wed, Apr 25, 2018 at 12:50:06PM +0300, Jani Nikula wrote:
> 
> Argh, now with Ville's correct address.
> 
> On Wed, 25 Apr 2018, Jani Nikula <jani.nikula@intel.com> wrote:
> > Cc: Rodrigo, DK, Ville
> >
> > On Tue, 17 Apr 2018, Imre Deak <imre.deak@intel.com> wrote:
> >> Add documentation to gen9_set_dc_state() on what enabling a given DC
> >> state means and at what point HW/DMC actually enters/exits these states.
> >>
> >> Cc: Jani Nikula <jani.nikula@intel.com>
> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Signed-off-by: Imre Deak <imre.deak@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_runtime_pm.c | 23 +++++++++++++++++++++++
> >>  1 file changed, 23 insertions(+)
> >>
> >> [ On IRC I stated that PSR entry would be prevented in a given DC state,
> >>   but looking more at it I haven't found any proof for this. So as I
> >>   understand the only connection between PSR and DC states is that if
> >>   DC5/6 is disabled power saving will be blocked, which would otherwise
> >>   be possible when PSR is active and the display pipe is off. ]
> >
> > I think I'm still missing a definitive answer to the question, who
> > disables PSR before DP AUX transactions?
> >
> > Does intel_display_power_get(dev_priv, intel_dp->aux_power_domain) in
> > pps_lock() cause PSR exit, through the DC state change? If yes, this
> > needs to be properly documented in code. Maybe with a WARN_ON(psr
> > active) on top.

No, that was only my misunderstanding earlier on IRC. Disabling DC
states doesn't prevent PSR entry. So the PSR active state is independent
of DC states; if PSR is enabled with DC5/6 disabled it just means there
will be less power saving during PSR active periods than it would be
possible with DC5/6 enabled.

Disabling PSR then has to happen by some other means while the driver
performs AUX transfers, either disabling it manually from the driver, or
by using the AUX HW mutex.

> >
> > Quoting bspec 7530, "DDI A AUX channel transactions must not be sent
> > while SRD is enabled. SRD must be completely disabled before a DDI A AUX
> > channel transaction can be sent."
> >
> > I'm also confused how sink CRC would ever work with PSR enabled.
> >
> > BR,
> > Jani.
> >
> >
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >> index 53ea564f971e..40a7955886d4 100644
> >> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> >> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >> @@ -542,6 +542,29 @@ void gen9_sanitize_dc_state(struct drm_i915_private *dev_priv)
> >>  	dev_priv->csr.dc_state = val;
> >>  }
> >>  
> >> +/**
> >> + * gen9_set_dc_state - set target display C power state
> >> + * @dev_priv: i915 device instance
> >> + * @state: target DC power state
> >> + * - DC_STATE_DISABLE
> >> + * - DC_STATE_EN_UPTO_DC5
> >> + * - DC_STATE_EN_UPTO_DC6
> >> + * - DC_STATE_EN_DC9
> >> + *
> >> + * Signal to DMC firmware/HW the target DC power state passed in @state.
> >> + * DMC/HW can turn off individual display clocks and power rails when entering
> >> + * a deeper DC power state (higher in number) and turns these back when exiting
> >> + * that state to a shallower power state (lower in number). The HW will decide
> >> + * when to actually enter a given state on an on-demand basis, for instance
> >> + * depending on the active state of display pipes. The state of display
> >> + * registers backed by affected power rails are saved/restored as needed.
> >> + *
> >> + * Based on the above enabling a deeper DC power state is asynchronous wrt.
> >> + * enabling it. Disabling a deeper power state is synchronous: for instance
> >> + * setting %DC_STATE_DISABLE won't complete until all HW resources are turned
> >> + * back on and register state is restored. This is guaranteed by the MMIO write
> >> + * to DC_STATE_EN blocking until the state is restored.
> >> + */
> >>  static void gen9_set_dc_state(struct drm_i915_private *dev_priv, uint32_t state)
> >>  {
> >>  	uint32_t val;
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add documentation to gen9_set_dc_state()

[Rodrigo Vivi]

On Wed, Apr 25, 2018 at 02:09:14PM +0300, Imre Deak wrote:
> On Wed, Apr 25, 2018 at 12:50:06PM +0300, Jani Nikula wrote:
> > 
> > Argh, now with Ville's correct address.
> > 
> > On Wed, 25 Apr 2018, Jani Nikula <jani.nikula@intel.com> wrote:
> > > Cc: Rodrigo, DK, Ville
> > >
> > > On Tue, 17 Apr 2018, Imre Deak <imre.deak@intel.com> wrote:
> > >> Add documentation to gen9_set_dc_state() on what enabling a given DC
> > >> state means and at what point HW/DMC actually enters/exits these states.
> > >>
> > >> Cc: Jani Nikula <jani.nikula@intel.com>
> > >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > >> Signed-off-by: Imre Deak <imre.deak@intel.com>
> > >> ---
> > >>  drivers/gpu/drm/i915/intel_runtime_pm.c | 23 +++++++++++++++++++++++
> > >>  1 file changed, 23 insertions(+)
> > >>
> > >> [ On IRC I stated that PSR entry would be prevented in a given DC state,
> > >>   but looking more at it I haven't found any proof for this. So as I
> > >>   understand the only connection between PSR and DC states is that if
> > >>   DC5/6 is disabled power saving will be blocked, which would otherwise
> > >>   be possible when PSR is active and the display pipe is off. ]
> > >
> > > I think I'm still missing a definitive answer to the question, who
> > > disables PSR before DP AUX transactions?

no one. :(

This is a gap that we are aware. I believe Jose is working to address that.

cc: Jose.

> > >
> > > Does intel_display_power_get(dev_priv, intel_dp->aux_power_domain) in
> > > pps_lock() cause PSR exit, through the DC state change? If yes, this
> > > needs to be properly documented in code. Maybe with a WARN_ON(psr
> > > active) on top.
> 
> No, that was only my misunderstanding earlier on IRC. Disabling DC
> states doesn't prevent PSR entry. So the PSR active state is independent
> of DC states; if PSR is enabled with DC5/6 disabled it just means there
> will be less power saving during PSR active periods than it would be
> possible with DC5/6 enabled.

Yeap, they are independent.

> 
> Disabling PSR then has to happen by some other means while the driver
> performs AUX transfers, either disabling it manually from the driver, or
> by using the AUX HW mutex.

AUX HW mutex is not an option. It got discarded again. Guidance from HW eng
is to avoid it and disable PSR manually before aux transactions.

> 
> > >
> > > Quoting bspec 7530, "DDI A AUX channel transactions must not be sent
> > > while SRD is enabled. SRD must be completely disabled before a DDI A AUX
> > > channel transaction can be sent."
> > >
> > > I'm also confused how sink CRC would ever work with PSR enabled.

Our driver does aux transactions on modesets and hw does them on psr sync.

Sink CRC is used on the tests after modeset and hopefully when psr is
stable. But this actually could explain most of sink CRC issues we had
so far, indeed. Too much assumption and so less checks and protections :(

my bad...

> > >
> > > BR,
> > > Jani.
> > >
> > >
> > >>
> > >> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > >> index 53ea564f971e..40a7955886d4 100644
> > >> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > >> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > >> @@ -542,6 +542,29 @@ void gen9_sanitize_dc_state(struct drm_i915_private *dev_priv)
> > >>  	dev_priv->csr.dc_state = val;
> > >>  }
> > >>  
> > >> +/**
> > >> + * gen9_set_dc_state - set target display C power state
> > >> + * @dev_priv: i915 device instance
> > >> + * @state: target DC power state
> > >> + * - DC_STATE_DISABLE
> > >> + * - DC_STATE_EN_UPTO_DC5
> > >> + * - DC_STATE_EN_UPTO_DC6
> > >> + * - DC_STATE_EN_DC9
> > >> + *
> > >> + * Signal to DMC firmware/HW the target DC power state passed in @state.
> > >> + * DMC/HW can turn off individual display clocks and power rails when entering
> > >> + * a deeper DC power state (higher in number) and turns these back when exiting
> > >> + * that state to a shallower power state (lower in number). The HW will decide
> > >> + * when to actually enter a given state on an on-demand basis, for instance
> > >> + * depending on the active state of display pipes. The state of display
> > >> + * registers backed by affected power rails are saved/restored as needed.
> > >> + *
> > >> + * Based on the above enabling a deeper DC power state is asynchronous wrt.
> > >> + * enabling it. Disabling a deeper power state is synchronous: for instance
> > >> + * setting %DC_STATE_DISABLE won't complete until all HW resources are turned
> > >> + * back on and register state is restored. This is guaranteed by the MMIO write
> > >> + * to DC_STATE_EN blocking until the state is restored.
> > >> + */
> > >>  static void gen9_set_dc_state(struct drm_i915_private *dev_priv, uint32_t state)
> > >>  {
> > >>  	uint32_t val;
> > 
> > -- 
> > Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add documentation to gen9_set_dc_state()

[Dhinakaran Pandiyan]

On Wed, 2018-04-25 at 10:45 -0700, Rodrigo Vivi wrote:
> On Wed, Apr 25, 2018 at 02:09:14PM +0300, Imre Deak wrote:
> > On Wed, Apr 25, 2018 at 12:50:06PM +0300, Jani Nikula wrote:
> > > 
> > > Argh, now with Ville's correct address.
> > > 
> > > On Wed, 25 Apr 2018, Jani Nikula <jani.nikula@intel.com> wrote:
> > > > Cc: Rodrigo, DK, Ville
> > > >
> > > > On Tue, 17 Apr 2018, Imre Deak <imre.deak@intel.com> wrote:
> > > >> Add documentation to gen9_set_dc_state() on what enabling a given DC
> > > >> state means and at what point HW/DMC actually enters/exits these states.
> > > >>
> > > >> Cc: Jani Nikula <jani.nikula@intel.com>
> > > >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > >> Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > >> ---
> > > >>  drivers/gpu/drm/i915/intel_runtime_pm.c | 23 +++++++++++++++++++++++
> > > >>  1 file changed, 23 insertions(+)
> > > >>
> > > >> [ On IRC I stated that PSR entry would be prevented in a given DC state,
> > > >>   but looking more at it I haven't found any proof for this. So as I
> > > >>   understand the only connection between PSR and DC states is that if
> > > >>   DC5/6 is disabled power saving will be blocked, which would otherwise
> > > >>   be possible when PSR is active and the display pipe is off. ]
> > > >
> > > > I think I'm still missing a definitive answer to the question, who
> > > > disables PSR before DP AUX transactions?
> 
> no one. :(
> 
> This is a gap that we are aware. I believe Jose is working to address that.
> 
> cc: Jose.
> 
> > > >
> > > > Does intel_display_power_get(dev_priv, intel_dp->aux_power_domain) in
> > > > pps_lock() cause PSR exit, through the DC state change? If yes, this
> > > > needs to be properly documented in code. Maybe with a WARN_ON(psr
> > > > active) on top.
> > 
> > No, that was only my misunderstanding earlier on IRC. Disabling DC
> > states doesn't prevent PSR entry. So the PSR active state is independent
> > of DC states; if PSR is enabled with DC5/6 disabled it just means there
> > will be less power saving during PSR active periods than it would be
> > possible with DC5/6 enabled.
> 
> Yeap, they are independent.
> 
> > 
> > Disabling PSR then has to happen by some other means while the driver
> > performs AUX transfers, either disabling it manually from the driver, or
> > by using the AUX HW mutex.
> 
> AUX HW mutex is not an option. It got discarded again. Guidance from HW eng
> is to avoid it and disable PSR manually before aux transactions.
> 
> > 
> > > >
> > > > Quoting bspec 7530, "DDI A AUX channel transactions must not be sent
> > > > while SRD is enabled. SRD must be completely disabled before a DDI A AUX
> > > > channel transaction can be sent."
> > > >
> > > > I'm also confused how sink CRC would ever work with PSR enabled.

The bspec quote is specifically for BDW and HSW. The reason, presumably,
is HW sends aux transactions for PSR exit (this, we know for sure) and
the transactions can interfere with driver initiated aux transactions.

afaict, there is nothing in the DP spec against aux reads (for sink-crc)
when PSR is enabled.

So, we have to be careful about reading sink-crc and not start the reads
when PSR exit events happen. The current sink-crc code does the exact
opposite by reading sink-crc after triggering PSR exit events, namely
vbi enable.

-DK

> 
> Our driver does aux transactions on modesets and hw does them on psr sync.
> 
> Sink CRC is used on the tests after modeset and hopefully when psr is
> stable. But this actually could explain most of sink CRC issues we had
> so far, indeed. Too much assumption and so less checks and protections :(
> 
> my bad...
> 
> > > >
> > > > BR,
> > > > Jani.
> > > >
> > > >
> > > >>
> > > >> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > >> index 53ea564f971e..40a7955886d4 100644
> > > >> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > >> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > >> @@ -542,6 +542,29 @@ void gen9_sanitize_dc_state(struct drm_i915_private *dev_priv)
> > > >>  	dev_priv->csr.dc_state = val;
> > > >>  }
> > > >>  
> > > >> +/**
> > > >> + * gen9_set_dc_state - set target display C power state
> > > >> + * @dev_priv: i915 device instance
> > > >> + * @state: target DC power state
> > > >> + * - DC_STATE_DISABLE
> > > >> + * - DC_STATE_EN_UPTO_DC5
> > > >> + * - DC_STATE_EN_UPTO_DC6
> > > >> + * - DC_STATE_EN_DC9
> > > >> + *
> > > >> + * Signal to DMC firmware/HW the target DC power state passed in @state.
> > > >> + * DMC/HW can turn off individual display clocks and power rails when entering
> > > >> + * a deeper DC power state (higher in number) and turns these back when exiting
> > > >> + * that state to a shallower power state (lower in number). The HW will decide
> > > >> + * when to actually enter a given state on an on-demand basis, for instance
> > > >> + * depending on the active state of display pipes. The state of display
> > > >> + * registers backed by affected power rails are saved/restored as needed.
> > > >> + *
> > > >> + * Based on the above enabling a deeper DC power state is asynchronous wrt.
> > > >> + * enabling it. Disabling a deeper power state is synchronous: for instance
> > > >> + * setting %DC_STATE_DISABLE won't complete until all HW resources are turned
> > > >> + * back on and register state is restored. This is guaranteed by the MMIO write
> > > >> + * to DC_STATE_EN blocking until the state is restored.
> > > >> + */
> > > >>  static void gen9_set_dc_state(struct drm_i915_private *dev_priv, uint32_t state)
> > > >>  {
> > > >>  	uint32_t val;
> > > 
> > > -- 
> > > Jani Nikula, Intel Open Source Technology Center

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

Re: [PATCH] drm/i915: Add documentation to gen9_set_dc_state()

[Dhinakaran Pandiyan]

On Tue, 2018-04-17 at 14:31 +0300, Imre Deak wrote:
> Add documentation to gen9_set_dc_state() on what enabling a given DC
> state means and at what point HW/DMC actually enters/exits these states.
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> [ On IRC I stated that PSR entry would be prevented in a given DC state,
>   but looking more at it I haven't found any proof for this. So as I
>   understand the only connection between PSR and DC states is that if
>   DC5/6 is disabled power saving will be blocked, which would otherwise
>   be possible when PSR is active and the display pipe is off. ]
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 53ea564f971e..40a7955886d4 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -542,6 +542,29 @@ void gen9_sanitize_dc_state(struct drm_i915_private *dev_priv)
>  	dev_priv->csr.dc_state = val;
>  }
>  
> +/**
> + * gen9_set_dc_state - set target display C power state
> + * @dev_priv: i915 device instance
> + * @state: target DC power state
> + * - DC_STATE_DISABLE
> + * - DC_STATE_EN_UPTO_DC5
> + * - DC_STATE_EN_UPTO_DC6
> + * - DC_STATE_EN_DC9
> + *
> + * Signal to DMC firmware/HW the target DC power state passed in @state.
> + * DMC/HW can turn off individual display clocks and power rails when entering

Any chance you could eliminate the firmware/HW ambiguity? Not knowing
whether it is the firmware or the HW leads to potentially incorrect
assumptions.

> + * a deeper DC power state (higher in number) and turns these back when exiting
> + * that state to a shallower power state (lower in number). The HW will decide
> + * when to actually enter a given state on an on-demand basis, for instance
> + * depending on the active state of display pipes. The state of display
> + * registers backed by affected power rails are saved/restored as needed.
> + *

One of things that is completely misleading is the enable_dc6 debug
message we print. It is all over dmesg, but like you already wrote, HW
needs PSR to be enabled and only in a single-pipe active case, the
display controller can go to DC6. So, with PSR disabled upstream, there
is really no chance that the HW can go to DC6 without all displays
disabled.

> + * Based on the above enabling a deeper DC power state is asynchronous wrt.
> + * enabling it. Disabling a deeper power state is synchronous: for instance
> + * setting %DC_STATE_DISABLE won't complete until all HW resources are turned
> + * back on and register state is restored. This is guaranteed by the MMIO write
> + * to DC_STATE_EN blocking until the state is restored.
> + */
>  static void gen9_set_dc_state(struct drm_i915_private *dev_priv, uint32_t state)
>  {
>  	uint32_t val;

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

Re: [PATCH] drm/i915: Add documentation to gen9_set_dc_state()

[Imre Deak]

On Wed, Apr 25, 2018 at 11:47:28AM -0700, Dhinakaran Pandiyan wrote:
> On Tue, 2018-04-17 at 14:31 +0300, Imre Deak wrote:
> > Add documentation to gen9_set_dc_state() on what enabling a given DC
> > state means and at what point HW/DMC actually enters/exits these states.
> > 
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > [ On IRC I stated that PSR entry would be prevented in a given DC state,
> >   but looking more at it I haven't found any proof for this. So as I
> >   understand the only connection between PSR and DC states is that if
> >   DC5/6 is disabled power saving will be blocked, which would otherwise
> >   be possible when PSR is active and the display pipe is off. ]
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 53ea564f971e..40a7955886d4 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -542,6 +542,29 @@ void gen9_sanitize_dc_state(struct drm_i915_private *dev_priv)
> >  	dev_priv->csr.dc_state = val;
> >  }
> >  
> > +/**
> > + * gen9_set_dc_state - set target display C power state
> > + * @dev_priv: i915 device instance
> > + * @state: target DC power state
> > + * - DC_STATE_DISABLE
> > + * - DC_STATE_EN_UPTO_DC5
> > + * - DC_STATE_EN_UPTO_DC6
> > + * - DC_STATE_EN_DC9
> > + *
> > + * Signal to DMC firmware/HW the target DC power state passed in @state.
> > + * DMC/HW can turn off individual display clocks and power rails when entering
> 
> Any chance you could eliminate the firmware/HW ambiguity? Not knowing
> whether it is the firmware or the HW leads to potentially incorrect
> assumptions.

It's not specified. What I understood about current DMC firmware blobs
is that they will prevent entering DC5/6 if there is a pending
GT/display interrupt. So all other gating - for instance based on a
pipe's active state - happens outside of the firmware.

> > + * a deeper DC power state (higher in number) and turns these back when exiting
> > + * that state to a shallower power state (lower in number). The HW will decide
> > + * when to actually enter a given state on an on-demand basis, for instance
> > + * depending on the active state of display pipes. The state of display
> > + * registers backed by affected power rails are saved/restored as needed.
> > + *
> 
> One of things that is completely misleading is the enable_dc6 debug
> message we print. It is all over dmesg, but like you already wrote, HW
> needs PSR to be enabled and only in a single-pipe active case, the
> display controller can go to DC6. So, with PSR disabled upstream, there
> is really no chance that the HW can go to DC6 without all displays
> disabled.

Yes, that is how things work. The driver only enables - or allows if you
like - these states and this enabling is what the debug message is about
The actual toggling of power rails, clocks etc. will happen behind the
scenes. This is similar to how RC6 works for instance, with DC states
you just need more assistance atm from the driver, hence the need for
more cases where we have to disable/re-enable DC states.

> > + * Based on the above enabling a deeper DC power state is asynchronous wrt.
> > + * enabling it. Disabling a deeper power state is synchronous: for instance
> > + * setting %DC_STATE_DISABLE won't complete until all HW resources are turned
> > + * back on and register state is restored. This is guaranteed by the MMIO write
> > + * to DC_STATE_EN blocking until the state is restored.
> > + */
> >  static void gen9_set_dc_state(struct drm_i915_private *dev_priv, uint32_t state)
> >  {
> >  	uint32_t val;
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add documentation to gen9_set_dc_state()

[Imre Deak]

On Tue, Apr 17, 2018 at 02:31:47PM +0300, Imre Deak wrote:
> Add documentation to gen9_set_dc_state() on what enabling a given DC
> state means and at what point HW/DMC actually enters/exits these states.
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Pushed to -dinq, thanks for the review, comments.

> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> [ On IRC I stated that PSR entry would be prevented in a given DC state,
>   but looking more at it I haven't found any proof for this. So as I
>   understand the only connection between PSR and DC states is that if
>   DC5/6 is disabled power saving will be blocked, which would otherwise
>   be possible when PSR is active and the display pipe is off. ]
> 
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 53ea564f971e..40a7955886d4 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -542,6 +542,29 @@ void gen9_sanitize_dc_state(struct drm_i915_private *dev_priv)
>  	dev_priv->csr.dc_state = val;
>  }
>  
> +/**
> + * gen9_set_dc_state - set target display C power state
> + * @dev_priv: i915 device instance
> + * @state: target DC power state
> + * - DC_STATE_DISABLE
> + * - DC_STATE_EN_UPTO_DC5
> + * - DC_STATE_EN_UPTO_DC6
> + * - DC_STATE_EN_DC9
> + *
> + * Signal to DMC firmware/HW the target DC power state passed in @state.
> + * DMC/HW can turn off individual display clocks and power rails when entering
> + * a deeper DC power state (higher in number) and turns these back when exiting
> + * that state to a shallower power state (lower in number). The HW will decide
> + * when to actually enter a given state on an on-demand basis, for instance
> + * depending on the active state of display pipes. The state of display
> + * registers backed by affected power rails are saved/restored as needed.
> + *
> + * Based on the above enabling a deeper DC power state is asynchronous wrt.
> + * enabling it. Disabling a deeper power state is synchronous: for instance
> + * setting %DC_STATE_DISABLE won't complete until all HW resources are turned
> + * back on and register state is restored. This is guaranteed by the MMIO write
> + * to DC_STATE_EN blocking until the state is restored.
> + */
>  static void gen9_set_dc_state(struct drm_i915_private *dev_priv, uint32_t state)
>  {
>  	uint32_t val;
> -- 
> 2.13.2
> 
> _______________________________________________
> 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