Re: [Intel-gfx] [PATCH V2] drm/i915/jsl: Add W/A 1409054076 for JSL

["Surendrakumar Upadhyay, TejaskumarX" <tejaskumarx.surendrakumar.upadhyay@intel.com>]

> -----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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx