Thanks for review. Responses inline. > -----Original Message----- > From: Jani Nikula <jani.nik...@linux.intel.com> > Sent: 17 May 2021 19:43 > To: Surendrakumar Upadhyay, TejaskumarX > <tejaskumarx.surendrakumar.upadh...@intel.com>; intel- > g...@lists.freedesktop.org; Pandey, Hariom <hariom.pan...@intel.com> > Subject: Re: [Intel-gfx] [PATCH V2] drm/i915/jsl: Add W/A 1409054076 for JSL > > On Thu, 13 May 2021, Tejas Upadhyay > <tejaskumarx.surendrakumar.upadh...@intel.com> wrote: > > When pipe A is disabled and MIPI DSI is enabled on pipe B, the AMT > > KVMR feature will incorrectly see pipe A as enabled. > > Set 0x42080 bit 23=1 before enabling DSI on pipe B and leave it set > > while DSI is enabled on pipe B. No impact to setting it all the time. > > > > Changes since V1: > > - ./dim checkpatch errors addressed > > > > Signed-off-by: Tejas Upadhyay > > <tejaskumarx.surendrakumar.upadh...@intel.com> > > --- > > drivers/gpu/drm/i915/display/icl_dsi.c | 38 > ++++++++++++++++++++++++++ > > drivers/gpu/drm/i915/i915_reg.h | 1 + > > 2 files changed, 39 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c > > b/drivers/gpu/drm/i915/display/icl_dsi.c > > index ce544e20f35c..e5a6660861e8 100644 > > --- a/drivers/gpu/drm/i915/display/icl_dsi.c > > +++ b/drivers/gpu/drm/i915/display/icl_dsi.c > > @@ -40,6 +40,8 @@ > > #include "skl_scaler.h" > > #include "skl_universal_plane.h" > > > > +static bool gen11_dsi_get_hw_state(struct intel_encoder *encoder, > > + enum pipe *pipe); > > static int header_credits_available(struct drm_i915_private *dev_priv, > > enum transcoder dsi_trans) > > { > > @@ -1036,9 +1038,26 @@ static void gen11_dsi_enable_transcoder(struct > intel_encoder *encoder) > > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > > struct intel_dsi *intel_dsi = enc_to_intel_dsi(encoder); > > enum port port; > > + enum pipe pipe; > > enum transcoder dsi_trans; > > u32 tmp; > > > > + /* > > + * WA 1409054076:JSL > > + * When pipe A is disabled and MIPI DSI is enabled on pipe B, > > + * the AMT KVMR feature will incorrectly see pipe A as enabled. > > + * Set 0x42080 bit 23=1 before enabling DSI on pipe B and leave > > + * it set while DSI is enabled on pipe B > > + */ > > + gen11_dsi_get_hw_state(encoder, &pipe); > > That function is only for reading the state for taking over hardware state at > probe and hardware/software state verification after modeset. > > It reads the state that is being set later in this function, so it's never > going to > be correct here! Also, we try not to do stuff based on the hardware state, but > rather the software state.
Okay I will correct that. > > > + if (IS_PLATFORM(dev_priv, INTEL_JASPERLAKE) && > > + pipe == PIPE_B && > > + dev_priv->active_pipes != BIT(PIPE_A) && > > + !(intel_de_read(dev_priv, CHICKEN_PAR1_1) & > > + IGNORE_KVMR_PIPE_A)) { > > + intel_de_write(dev_priv, CHICKEN_PAR1_1, > > + intel_de_read(dev_priv, CHICKEN_PAR1_1) | > IGNORE_KVMR_PIPE_A); > > + } > > As far as I understand the explanation, we can set this regardless of whether > pipe A is disabled or not, and we can just set it based on where DSI is > enabled. > > It should probably also be IS_JSL_EHL(). Will it not affect if pipe A is enabled and we set intel_de_rmw(dev_priv, CHICKEN_PAR1_1, 0, IGNORE_KVMR_PIPE_A);. What I could understand is we only set this bit when pipe A is disable and we have MIPI DSI enable on pipe B. Correct me again If I am getting it wrong. Also Bspec says clearly workaround is for JSL only. Should I consider EHL also in this? > > With pipe from new_crtc_state: > > if (IS_JSL_EHL(dev_priv) && pipe == PIPE_B) > intel_de_rmw(dev_priv, CHICKEN_PAR1_1, 0, > IGNORE_KVMR_PIPE_A); > > To disable, with pipe from old_crtc_state: > > if (IS_JSL_EHL(dev_priv) && pipe == PIPE_B) > intel_de_rmw(dev_priv, CHICKEN_PAR1_1, IGNORE_KVMR_PIPE_A, > 0); > > At the right locations. Ok I will take this into consideration. > > > for_each_dsi_port(port, intel_dsi->ports) { > > dsi_trans = dsi_port_to_transcoder(port); > > tmp = intel_de_read(dev_priv, PIPECONF(dsi_trans)); @@ - > 1245,6 > > +1264,7 @@ static void gen11_dsi_enable(struct intel_atomic_state > > *state, > > > > drm_WARN_ON(state->base.dev, crtc_state->has_pch_encoder); > > > > + > > /* step6d: enable dsi transcoder */ > > gen11_dsi_enable_transcoder(encoder); > > > > @@ -1260,9 +1280,27 @@ static void gen11_dsi_disable_transcoder(struct > intel_encoder *encoder) > > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > > struct intel_dsi *intel_dsi = enc_to_intel_dsi(encoder); > > enum port port; > > + enum pipe pipe; > > enum transcoder dsi_trans; > > u32 tmp; > > > > + /* > > + * WA 1409054076:JSL > > + * When pipe A is disabled and MIPI DSI is enabled on pipe B, > > + * the AMT KVMR feature will incorrectly see pipe A as enabled. > > + * Set 0x42080 bit 23=1 before enabling DSI on pipe B and leave > > + * it set while DSI is enabled on pipe B > > + */ > > + gen11_dsi_get_hw_state(encoder, &pipe); > > + if (IS_PLATFORM(dev_priv, INTEL_JASPERLAKE) && > > + pipe == PIPE_B && > > + dev_priv->active_pipes != BIT(PIPE_A) && > > + (intel_de_read(dev_priv, CHICKEN_PAR1_1) & > > + IGNORE_KVMR_PIPE_A)) { > > + intel_de_write(dev_priv, CHICKEN_PAR1_1, > > + intel_de_read(dev_priv, CHICKEN_PAR1_1) & > > + !IGNORE_KVMR_PIPE_A); > > + } > > for_each_dsi_port(port, intel_dsi->ports) { > > dsi_trans = dsi_port_to_transcoder(port); > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > b/drivers/gpu/drm/i915/i915_reg.h index 871d839dfcb8..8b67cd14ff7e > > 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -8039,6 +8039,7 @@ enum { > > # define CHICKEN3_DGMG_DONE_FIX_DISABLE (1 << 2) > > > > #define CHICKEN_PAR1_1 _MMIO(0x42080) > > +#define IGNORE_KVMR_PIPE_A BIT(23) > > REG_BIT(), not BIT(). Please read the big comment near the top of the file. > Please observe the REG_BIT() on the very next line. Sorry to say but there is no uniformity in term of which macro to use. Some places I have got review earlier to add BIT() and I can see at some places not BIT() used nor REG_BIT(). I will correct for this matter to use REG_BIT(). Thanks, Tejas > > > #define KBL_ARB_FILL_SPARE_22 REG_BIT(22) > > #define DIS_RAM_BYPASS_PSR2_MAN_TRACK (1 << 16) > > #define SKL_DE_COMPRESSED_HASH_MODE (1 << 15) > > -- > Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx