Re: [Intel-gfx] [PATCH V2] drm/i915/jsl: Disable cursor clock gating in HDR mode

From: "Souza, Jose" <jose.souza@intel.com>
To: "Surendrakumar Upadhyay,
	TejaskumarX" <tejaskumarx.surendrakumar.upadhyay@intel.com>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>
Cc: "Pandey, Hariom" <hariom.pandey@intel.com>
Subject: Re: [Intel-gfx] [PATCH V2] drm/i915/jsl: Disable cursor clock gating in HDR mode
Date: Tue, 3 Nov 2020 00:01:36 +0000	[thread overview]
Message-ID: <025cddace9c4164803fb71f770adcf260b7052e4.camel@intel.com> (raw)
In-Reply-To: <20201102073946.158633-1-tejaskumarx.surendrakumar.upadhyay@intel.com>

On Mon, 2020-11-02 at 13:09 +0530, Tejas Upadhyay wrote:
> Display underrun in HDR mode when cursor is enabled.
> RTL fix will be implemented CLKGATE_DIS_PSL_A bit 28-46520h.
> As per W/A 1604331009, Disable cursor clock gating in HDR mode.
> 
> Bspec : 33451
> 
> Changes since V1:
> 	- Modified way CLKGATE_DIS_PSL bit 28 was modified
> 
> Cc: Souza Jose <jose.souza@intel.com>
> Signed-off-by: Tejas Upadhyay <tejaskumarx.surendrakumar.upadhyay@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 28 ++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h              |  5 ++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index cddbda5303ff..b132585d9e78 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -541,6 +541,15 @@ icl_wa_scalerclkgating(struct drm_i915_private *dev_priv, enum pipe pipe,
>  		               intel_de_read(dev_priv, CLKGATE_DIS_PSL(pipe)) & ~DPFR_GATING_DIS);
>  }
>  
> 
> 
> 
> 
> 
> 
> 
> +/* Wa_1604331009:jsl */
> +static void
> +jsl_wa_cursorclkgating(struct drm_i915_private *dev_priv, enum pipe pipe,
> +		       bool enable)

if this is a gen11 WA why naming as jsl? also include in the comment icl and ehl.

> +{
> +	intel_de_rmw(dev_priv, CLKGATE_DIS_PSL(pipe),
> +		     CURSOR_GATING_DIS, enable ? CURSOR_GATING_DIS : 0);
> +}
> +
>  static bool
>  needs_modeset(const struct intel_crtc_state *state)
>  {
> @@ -6637,6 +6646,16 @@ static bool needs_scalerclk_wa(const struct intel_crtc_state *crtc_state)
>  	return false;
>  }
>  
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> +static bool needs_cursorclk_wa(const struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);

line break here

> +	/* Wa_1604331009:jsl */
> +	if (crtc_state->active_planes & icl_hdr_plane_mask() &&
> +	    IS_GEN(dev_priv, 11))
> +		return true;

line break here

> +	return false;
> +}
> +
>  static bool planes_enabling(const struct intel_crtc_state *old_crtc_state,
>  			    const struct intel_crtc_state *new_crtc_state)
>  {
> @@ -6678,6 +6697,10 @@ static void intel_post_plane_update(struct intel_atomic_state *state,
>  	if (needs_scalerclk_wa(old_crtc_state) &&
>  	    !needs_scalerclk_wa(new_crtc_state))
>  		icl_wa_scalerclkgating(dev_priv, pipe, false);
> +
> +	if (needs_cursorclk_wa(old_crtc_state) &&
> +	    !needs_cursorclk_wa(new_crtc_state))
> +		jsl_wa_cursorclkgating(dev_priv, pipe, false);
>  }
>  
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
>  static void skl_disable_async_flip_wa(struct intel_atomic_state *state,
> @@ -6743,6 +6766,11 @@ static void intel_pre_plane_update(struct intel_atomic_state *state,
>  	    needs_scalerclk_wa(new_crtc_state))
>  		icl_wa_scalerclkgating(dev_priv, pipe, true);
>  
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> +	/* Wa_1604331009:jsl */
> +	if (!needs_cursorclk_wa(old_crtc_state) &&
> +	    needs_cursorclk_wa(new_crtc_state))
> +		jsl_wa_cursorclkgating(dev_priv, pipe, true);

Like the idea of only enable the WA when a HDR plane is enabled but there is some problems:
- never disable the wa
- not checking if a cursor plane is also active
- calling it in the post and pre plane update, I think only the pre is needed
- checking the old state, no need to do optimizations like that for just one MMIO write

other thing, would be better have the wa function being called and inside of that function it will check if the WA is needed and write to the
register, no need of a function to check if needs and another to apply the WA.

ICL WA description says that it can only be applied if "CUR_CTL[18], CUR_CTL[16] or CUR_COLOR_CTL[15]" is not set, did you checked if when a HDR plane
is enabled it causes a complete modeset(disable pipe, set wa, enable pipe) in the pipe? if that happens it is complying if not we have a problem here.

> +
>  	/*
>  	 * Vblank time updates from the shadow to live plane control register
>  	 * are blocked if the memory self-refresh mode is active at that
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index bb0656875697..f81a503c5d4b 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4194,6 +4194,11 @@ enum {
>  #define INF_UNIT_LEVEL_CLKGATE		_MMIO(0x9560)
>  #define   CGPSF_CLKGATE_DIS		(1 << 3)
>  
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> +/*
> + * GEN11 clock gating regs
> + */
> +#define   CURSOR_GATING_DIS		BIT(28)

should be defined between other CLKGATE_DIS_PSL bits.

> +
>  /*
>   * Display engine regs
>   */

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