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

From: "Surendrakumar Upadhyay, TejaskumarX" <tejaskumarx.surendrakumar.upadhyay@intel.com>
To: "Surendrakumar Upadhyay,
	TejaskumarX" <tejaskumarx.surendrakumar.upadhyay@intel.com>,
	"Souza, Jose" <jose.souza@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: Thu, 4 Feb 2021 16:14:37 +0000	[thread overview]
Message-ID: <SN6PR11MB3421A8B6D025F8D96A75DD5DDFB39@SN6PR11MB3421.namprd11.prod.outlook.com> (raw)
In-Reply-To: <SN6PR11MB3421C859BD47570139D7A694DFF50@SN6PR11MB3421.namprd11.prod.outlook.com>

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of
> Surendrakumar Upadhyay, TejaskumarX
> Sent: 30 November 2020 21:21
> To: Souza, Jose <jose.souza@intel.com>; 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
> 
> 
> 
> > -----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.

V3 patch is ready but Jose as I understand, here you referring to check when system normally boots up with 
planes are initialized and it finds HDR plane is enabled it should basically restart pipe and during restart it 
should enable W/a as well. Is there any other workaround which is doing same which I can refer ? Or if I check 
myself it will be putting logs which I can show you as proof, right!. Any other way you can suggest to prove it!

Regards,
Tejas

> 
> >
> > > +
> > >  /*
> > >   * 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
> > >   */
> >
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx