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
