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

[Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>]

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