| Message ID | 20210513122352.176643-1-tejaskumarx.surendrakumar.upadhyay@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2] drm/i915/jsl: Add W/A 1409054076 for JSL | expand |
On Thu, 13 May 2021, Tejas Upadhyay <tejaskumarx.surendrakumar.upadhyay@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.upadhyay@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. > + 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(). 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. > 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. > #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)
Thanks for review. Responses inline. > -----Original Message----- > From: Jani Nikula <jani.nikula@linux.intel.com> > Sent: 17 May 2021 19:43 > To: Surendrakumar Upadhyay, TejaskumarX > <tejaskumarx.surendrakumar.upadhyay@intel.com>; intel- > gfx@lists.freedesktop.org; Pandey, Hariom <hariom.pandey@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.upadhyay@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.upadhyay@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
On Tue, 18 May 2021, "Surendrakumar Upadhyay, TejaskumarX" <tejaskumarx.surendrakumar.upadhyay@intel.com> wrote: > Thanks for review. Responses inline. > >> -----Original Message----- >> From: Jani Nikula <jani.nikula@linux.intel.com> >> Sent: 17 May 2021 19:43 >> To: Surendrakumar Upadhyay, TejaskumarX >> <tejaskumarx.surendrakumar.upadhyay@intel.com>; intel- >> gfx@lists.freedesktop.org; Pandey, Hariom <hariom.pandey@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.upadhyay@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.upadhyay@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. The spec description is lacking, really. But, how are we supposed to interpret "No impact to setting it all the time."? Only setting the bit when pipe A is disabled is going to be harder. That's another thing that was wrong with using gen11_dsi_get_hw_state(); it'll only take DSI into account, not *other* things that might be using pipe A. Do you actually have a real world bug where you can see this? > Also Bspec says clearly workaround is for JSL only. Should I consider > EHL also in this? Yes, they're practically the same, and we don't even have IS_JASPERLAKE() or IS_ELKHARTLAKE() for that precise reason. There are a couple of rare cases where we need to make the distinction. > >> >> 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(). In i915_reg.h *always* use REG_BIT(). I just sent a patch to fix accidental uses of BIT() that have crept in. BR, Jani. > > 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
> -----Original Message----- > From: Jani Nikula <jani.nikula@linux.intel.com> > Sent: 18 May 2021 14:21 > To: Surendrakumar Upadhyay, TejaskumarX > <tejaskumarx.surendrakumar.upadhyay@intel.com>; intel- > gfx@lists.freedesktop.org; Pandey, Hariom <hariom.pandey@intel.com> > Subject: RE: [Intel-gfx] [PATCH V2] drm/i915/jsl: Add W/A 1409054076 for JSL > > On Tue, 18 May 2021, "Surendrakumar Upadhyay, TejaskumarX" > <tejaskumarx.surendrakumar.upadhyay@intel.com> wrote: > > Thanks for review. Responses inline. > > > >> -----Original Message----- > >> From: Jani Nikula <jani.nikula@linux.intel.com> > >> Sent: 17 May 2021 19:43 > >> To: Surendrakumar Upadhyay, TejaskumarX > >> <tejaskumarx.surendrakumar.upadhyay@intel.com>; intel- > >> gfx@lists.freedesktop.org; Pandey, Hariom <hariom.pandey@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.upadhyay@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.upadhyay@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. > > The spec description is lacking, really. But, how are we supposed to interpret > "No impact to setting it all the time."? > Ok I think we can ignore pipe A status with statement " Set 0x42080 bit 23=1 before enabling DSI on pipe B and leave it set while DSI is enabled on pipe B"(Here There is no mention of pipe A for leaving the bit set). I will modify accordingly. > Only setting the bit when pipe A is disabled is going to be harder. That's > another thing that was wrong with using gen11_dsi_get_hw_state(); it'll only > take DSI into account, not *other* things that might be using pipe A. > > Do you actually have a real world bug where you can see this? No I don't have real world bug. > > > Also Bspec says clearly workaround is for JSL only. Should I consider > > EHL also in this? > > Yes, they're practically the same, and we don't even have > IS_JASPERLAKE() or IS_ELKHARTLAKE() for that precise reason. There are a > couple of rare cases where we need to make the distinction. > > > > >> > >> 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(). > > In i915_reg.h *always* use REG_BIT(). I just sent a patch to fix accidental > uses of BIT() that have crept in. > > > BR, > Jani. > > > > > 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 > > -- > Jani Nikula, Intel Open Source Graphics Center
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); + 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); 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) #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)
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.upadhyay@intel.com> --- drivers/gpu/drm/i915/display/icl_dsi.c | 38 ++++++++++++++++++++++++++ drivers/gpu/drm/i915/i915_reg.h | 1 + 2 files changed, 39 insertions(+)