On Tue, 18 May 2021, "Surendrakumar Upadhyay, TejaskumarX" <tejaskumarx.surendrakumar.upadhyay@xxxxxxxxx> wrote:
> Thanks for review. Responses inline.
>
>> -----Original Message-----
>> From: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
>> Sent: 17 May 2021 19:43
>> To: Surendrakumar Upadhyay, TejaskumarX
>> <tejaskumarx.surendrakumar.upadhyay@xxxxxxxxx>; intel-
>> gfx@xxxxxxxxxxxxxxxxxxxxx; Pandey, Hariom <hariom.pandey@xxxxxxxxx>
>> Subject: Re: [PATCH V2] drm/i915/jsl: Add W/A 1409054076 for JSL
>>
>> On Thu, 13 May 2021, Tejas Upadhyay
>> <tejaskumarx.surendrakumar.upadhyay@xxxxxxxxx> 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@xxxxxxxxx>
>> > ---
>> > 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
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
