diff mbox series

[V2] drm/i915/jsl: Disable cursor clock gating in HDR mode

Message ID 20201102073946.158633-1-tejaskumarx.surendrakumar.upadhyay@intel.com (mailing list archive)
State New, archived
Headers show
Series [V2] drm/i915/jsl: Disable cursor clock gating in HDR mode | expand

Commit Message

Tejas Upadhyay Nov. 2, 2020, 7:39 a.m. UTC
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(+)

Comments

Souza, Jose Nov. 3, 2020, 12:01 a.m. UTC | #1
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
>   */
Tejas Upadhyay Nov. 30, 2020, 3:50 p.m. UTC | #2
> -----Original Message-----
> From: Souza, Jose <jose.souza@intel.com>
> Sent: 03 November 2020 05:32
> To: Surendrakumar Upadhyay, TejaskumarX
> <tejaskumarx.surendrakumar.upadhyay@intel.com>; intel-
> gfx@lists.freedesktop.org
> Cc: Pandey, Hariom <hariom.pandey@intel.com>
> Subject: Re: [PATCH V2] drm/i915/jsl: Disable cursor clock gating in HDR
> mode
> 
> 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.

Tejas : I have addressed all above review comments in next patchset.

> 
> 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.

Tejas : Would you elaborate more this scenario? As far as I understand once planes are attached to CRTC/pipe they can be updated runtime. Are you referring planes attached to CRTC/pipe will change while CRTC is enable? I would like to understand if I am missing something.  

> 
> > +
> >  /*
> >   * 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_DISBIT(28)
> 
> should be defined between other CLKGATE_DIS_PSL bits.
> 
> > +
> >  /*
> >   * Display engine regs
> >   */
>
diff mbox series

Patch

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)
+{
+	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);
+	/* Wa_1604331009:jsl */
+	if (crtc_state->active_planes & icl_hdr_plane_mask() &&
+	    IS_GEN(dev_priv, 11))
+		return true;
+	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);
+
 	/*
 	 * 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)
+
 /*
  * Display engine regs
  */