> -----Original Message----- > From: Souza, Jose <jose.souza@xxxxxxxxx> > Sent: 03 November 2020 05:32 > To: Surendrakumar Upadhyay, TejaskumarX > <tejaskumarx.surendrakumar.upadhyay@xxxxxxxxx>; intel- > gfx@xxxxxxxxxxxxxxxxxxxxx > Cc: Pandey, Hariom <hariom.pandey@xxxxxxxxx> > 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@xxxxxxxxx> > > Signed-off-by: Tejas Upadhyay > > <tejaskumarx.surendrakumar.upadhyay@xxxxxxxxx> > > --- > > 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 > > */ > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx