Re: [Intel-gfx] [PATCH v2 3/3] drm/i915/psr: Implement Wa_1136

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

On Fri, Mar 17, 2023 at 01:04:37PM +0200, Jouni Högander wrote:
> Implement Wa_1136 for SKL/BXT/ICL.
> 
> Bspec: 21664
> 
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_psr.c     | 15 +++++++++++++++
>  drivers/gpu/drm/i915/display/skl_watermark.c |  5 -----
>  2 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index a385cb8dbf13..e6bd46441392 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1049,6 +1049,13 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
>  		return;
>  	}
>  
> +	/* Wa_1136 */

The syntax we've used for the old w/as is different.

> +	if (DISPLAY_VER(dev_priv) < 12 && crtc_state->wm_level_disabled) {

This won't have been calculated yet.

As for the platform check. I think the one hsd we still have left
indicates that icl already got some kind of full fix. So probably
that should at least be safe. And I do think the KBL+ should also
work fine.

But we could do that as followups:
1. do this
2. switch to the chicken bit approach for icl
3. switch to the chicken bit approach for kbl+

Then of any issue later come up that point to a problem
with the chicken bits we could more easily revert to full
psr disable.

> +		drm_dbg_kms(&dev_priv->drm,
> +			    "PSR condition failed: WM level disabled and no HW WA available\n");
> +		return;
> +	}
> +
>  	crtc_state->has_psr = true;
>  	crtc_state->has_psr2 = intel_psr2_config_valid(intel_dp, crtc_state);
>  
> @@ -1260,6 +1267,10 @@ static void intel_psr_enable_locked(struct intel_dp *intel_dp,
>  
>  	drm_WARN_ON(&dev_priv->drm, intel_dp->psr.enabled);
>  
> +	/* Wa_1136 */
> +	if (DISPLAY_VER(dev_priv) < 12 && crtc_state->wm_level_disabled)

It's a bit weird to handle this differently than the active_planes case.
Though the fact that the pre and post updatre hooks also do things
in different ways is also confusing. Seems to me some general cleanup
in this area could be worthwile.

> +		return;
> +
>  	intel_dp->psr.psr2_enabled = crtc_state->has_psr2;
>  	intel_dp->psr.busy_frontbuffer_bits = 0;
>  	intel_dp->psr.pipe = to_intel_crtc(crtc_state->uapi.crtc)->pipe;
> @@ -1940,6 +1951,10 @@ void intel_psr_pre_plane_update(struct intel_atomic_state *state,
>  		needs_to_disable |= !new_crtc_state->active_planes;
>  		needs_to_disable |= new_crtc_state->has_psr2 != psr->psr2_enabled;
>  
> +		/* Wa_1136 */
> +		needs_to_disable |= DISPLAY_VER(i915) < 12 &&
> +			new_crtc_state->wm_level_disabled;
> +
>  		if (psr->enabled && needs_to_disable)
>  			intel_psr_disable_locked(intel_dp);
>  
> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> index afb751c024ba..ced61da8b496 100644
> --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> @@ -2278,11 +2278,6 @@ static int skl_wm_check_vblank(struct intel_crtc_state *crtc_state)
>  	 */
>  	crtc_state->wm_level_disabled = level < i915->display.wm.num_levels - 1;
>  
> -	/*
> -	 * FIXME also related to skl+ w/a 1136 (also unimplemented as of
> -	 * now) perhaps?
> -	 */
> -
>  	for (level++; level < i915->display.wm.num_levels; level++) {
>  		enum plane_id plane_id;
>  
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel