[Intel-gfx] [PATCH V2] drm/i915/jsl: Add W/A 1409054076 for JSL
Surendrakumar Upadhyay, TejaskumarX
tejaskumarx.surendrakumar.upadhyay at intel.com
Tue May 18 09:08:43 UTC 2021
> -----Original Message-----
> From: Jani Nikula <jani.nikula at linux.intel.com>
> Sent: 18 May 2021 14:21
> To: Surendrakumar Upadhyay, TejaskumarX
> <tejaskumarx.surendrakumar.upadhyay at intel.com>; intel-
> gfx at lists.freedesktop.org; Pandey, Hariom <hariom.pandey at 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 at intel.com> wrote:
> > Thanks for review. Responses inline.
> >
> >> -----Original Message-----
> >> From: Jani Nikula <jani.nikula at linux.intel.com>
> >> Sent: 17 May 2021 19:43
> >> To: Surendrakumar Upadhyay, TejaskumarX
> >> <tejaskumarx.surendrakumar.upadhyay at intel.com>; intel-
> >> gfx at lists.freedesktop.org; Pandey, Hariom <hariom.pandey at 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 at 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 at 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
More information about the Intel-gfx
mailing list