Re: [Intel-gfx] [PATCH 1/5] drm/i915: Use separate "DC off" power well for ADL-P and DG2

Imre Deak
2023-03-17T17:42:52Z

On Thu, Mar 16, 2023 at 01:25:45PM -0700, Radhakrishna Sripada wrote:
> From: Matt Roper <matthew.d.roper@intel.com>
>
> Although ADL-P and DG2 both use the same general power well setup, the
> DC5/DC6 requirements are slightly different which means each platform
> should have its own "DC off" power well.
>
> DG2 (i.e., Xe_HPD IP) requires that DC5 be disabled whenever PG2 is
> active. However ADL-P (i.e., Xe_LPD IP) only requires DC5/DC6 to be
> disabled when the PGC or PGD subwells are active; we should be able to
> remain in these DC states when PGB and general PG2 functionality is in
> use.
>
> Bspec: 49193
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> ---
> .../i915/display/intel_display_power_map.c | 41 +++++++++++++++++--
> 1 file changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_map.c b/drivers/gpu/drm/i915/display/intel_display_power_map.c
> index 6645eb1911d8..d9e02cc9cf3c 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power_map.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power_map.c
> @@ -1301,7 +1301,8 @@ I915_DECL_PW_DOMAINS(xelpd_pwdoms_pw_2,
> */
>
> I915_DECL_PW_DOMAINS(xelpd_pwdoms_dc_off,
> - XELPD_PW_2_POWER_DOMAINS,
> + XELPD_PW_C_POWER_DOMAINS,
> + XELPD_PW_D_POWER_DOMAINS,
> POWER_DOMAIN_PORT_DSI,
> POWER_DOMAIN_AUDIO_MMIO,
> POWER_DOMAIN_AUX_A,
> @@ -1310,14 +1311,38 @@ I915_DECL_PW_DOMAINS(xelpd_pwdoms_dc_off,
> POWER_DOMAIN_DC_OFF,
> POWER_DOMAIN_INIT);
>
> -static const struct i915_power_well_desc xelpd_power_wells_main[] = {
> +static const struct i915_power_well_desc xelpd_power_wells_dcoff[] = {

Nit: Here and in the xehpd defintion, s/dcoff/dc_off/ to match other platforms.

> {
> .instances = &I915_PW_INSTANCES(
> I915_PW("DC_off", &xelpd_pwdoms_dc_off,
> .id = SKL_DISP_DC_OFF),
> ),
> .ops = &gen9_dc_off_power_well_ops,
> - }, {
> + }
> +};
> +
> +I915_DECL_PW_DOMAINS(xehpd_pwdoms_dc_off,
> + XELPD_PW_2_POWER_DOMAINS,
> + POWER_DOMAIN_PORT_DSI,
> + POWER_DOMAIN_AUDIO_MMIO,
> + POWER_DOMAIN_AUX_A,
> + POWER_DOMAIN_AUX_B,
> + POWER_DOMAIN_MODESET,
> + POWER_DOMAIN_DC_OFF,
> + POWER_DOMAIN_INIT);
> +
> +static const struct i915_power_well_desc xehpd_power_wells_dcoff[] = {
> + {
> + .instances = &I915_PW_INSTANCES(
> + I915_PW("DC_off", &xehpd_pwdoms_dc_off,
> + .id = SKL_DISP_DC_OFF),
> + ),
> + .ops = &gen9_dc_off_power_well_ops,
> + }
> +};

Could you move the xehpd definitions to precede xehpd_power_wells[]?

Patches 1, 2 look ok to me:
Reviewed-by: Imre Deak <imre.deak@intel.com>

> +
> +static const struct i915_power_well_desc xelpd_power_wells_main[] = {
> + {
> .instances = &I915_PW_INSTANCES(
> I915_PW("PW_2", &xelpd_pwdoms_pw_2,
> .hsw.idx = ICL_PW_CTL_IDX_PW_2,
> @@ -1400,6 +1425,14 @@ static const struct i915_power_well_desc xelpd_power_wells_main[] = {
> static const struct i915_power_well_desc_list xelpd_power_wells[] = {
> I915_PW_DESCRIPTORS(i9xx_power_wells_always_on),
> I915_PW_DESCRIPTORS(icl_power_wells_pw_1),
> + I915_PW_DESCRIPTORS(xelpd_power_wells_dcoff),
> + I915_PW_DESCRIPTORS(xelpd_power_wells_main),
> +};
> +
> +static const struct i915_power_well_desc_list xehpd_power_wells[] = {
> + I915_PW_DESCRIPTORS(i9xx_power_wells_always_on),
> + I915_PW_DESCRIPTORS(icl_power_wells_pw_1),
> + I915_PW_DESCRIPTORS(xehpd_power_wells_dcoff),
> I915_PW_DESCRIPTORS(xelpd_power_wells_main),
> };
>
> @@ -1624,6 +1657,8 @@ int intel_display_power_map_init(struct i915_power_domains *power_domains)
>
> if (DISPLAY_VER(i915) >= 14)
> return set_power_wells(power_domains, xelpdp_power_wells);
> + else if (IS_DG2(i915))
> + return set_power_wells(power_domains, xehpd_power_wells);
> else if (DISPLAY_VER(i915) >= 13)
> return set_power_wells(power_domains, xelpd_power_wells);
> else if (IS_DG1(i915))
> --
> 2.34.1
>