Re: [PATCH v3 10/13] drm/i915/dmc: Add interface to control start of PKG C-state exit

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Apr 14, 2025 at 01:05:05PM +0300, Jouni Högander wrote:
> Add interface to control if package C exit starts at the start of the
> undelayed vblank. This is needed to implement workaround for underrun on
> idle PSR HW issue (Wa_16025596647).
> 
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dmc.c | 31 ++++++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dmc.h |  2 ++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.c b/drivers/gpu/drm/i915/display/intel_dmc.c
> index c65544e48c42..bd50c98c0ee4 100644
> --- a/drivers/gpu/drm/i915/display/intel_dmc.c
> +++ b/drivers/gpu/drm/i915/display/intel_dmc.c
> @@ -534,6 +534,37 @@ void intel_dmc_block_pkgc(struct intel_display *display, enum pipe pipe,
>  		     PIPEDMC_BLOCK_PKGC_SW_BLOCK_PKGC_ALWAYS : 0);
>  }
>  
> +/**
> + * intel_dmc_start_pkgc_exit_at_start_of_undelayed_vblank() - start of PKG
> + * C-state exit
> + * @display: display instance
> + * @pipe: pipe which register use to block
> + * @enable: enable/disable
> + *
> + * This interface is target for Wa_16025596647 usage. I.e. start the package C
> + * exit at the start of the undelayed vblank
> + */
> +void intel_dmc_start_pkgc_exit_at_start_of_undelayed_vblank(struct intel_display *display,
> +							    enum pipe pipe, bool enable)
> +{
> +	u32 val;
> +
> +	if (enable)
> +		val = DMC_EVT_CTL_ENABLE | DMC_EVT_CTL_RECURRING |
> +			REG_FIELD_PREP(DMC_EVT_CTL_TYPE_MASK,
> +				       DMC_EVT_CTL_TYPE_EDGE_0_1) |
> +			REG_FIELD_PREP(DMC_EVT_CTL_EVENT_ID_MASK,
> +				       DMC_EVT_CTL_EVENT_ID_VBLANK_A);
> +	else
> +		val = REG_FIELD_PREP(DMC_EVT_CTL_EVENT_ID_MASK,
> +				     DMC_EVT_CTL_EVENT_ID_FALSE) |
> +			REG_FIELD_PREP(DMC_EVT_CTL_TYPE_MASK,
> +				       DMC_EVT_CTL_TYPE_EDGE_0_1);
> +
> +	intel_de_write(display, MTL_PIPEDMC_EVT_CTL_4(pipe),
> +		       val);
> +}

This thing doesn't seem to be fully thought through:

- Enabling PIPEDMC events exposes us to the DSB vs. DMC register
  corruption issues once again. Someone would need to come up with some
  kind of scheme to avoid DMC and DSB executing in parallel. Otherwise
  I don't think we can safely enable this
- The w/a seems to be only for cases where the pkgc latency exceeds the
  delayed vblank length, but that is never allowed by the
  skl_is_vblank_too_short() stuff, so looks like this should never be
  needed currently

> +
>  static bool is_dmc_evt_ctl_reg(struct intel_display *display,
>  			       enum intel_dmc_id dmc_id, i915_reg_t reg)
>  {
> diff --git a/drivers/gpu/drm/i915/display/intel_dmc.h b/drivers/gpu/drm/i915/display/intel_dmc.h
> index 236312fb791c..bd1c459b0075 100644
> --- a/drivers/gpu/drm/i915/display/intel_dmc.h
> +++ b/drivers/gpu/drm/i915/display/intel_dmc.h
> @@ -20,6 +20,8 @@ void intel_dmc_enable_pipe(struct intel_display *display, enum pipe pipe);
>  void intel_dmc_disable_pipe(struct intel_display *display, enum pipe pipe);
>  void intel_dmc_block_pkgc(struct intel_display *display, enum pipe pipe,
>  			  bool block);
> +void intel_dmc_start_pkgc_exit_at_start_of_undelayed_vblank(struct intel_display *display,
> +							    enum pipe pipe, bool enable);
>  void intel_dmc_fini(struct intel_display *display);
>  void intel_dmc_suspend(struct intel_display *display);
>  void intel_dmc_resume(struct intel_display *display);
> -- 
> 2.43.0

-- 
Ville Syrjälä
Intel