[PATCH] drm/i915/display: Add WA_14018221282

[Garg, Nemesa]

> -----Original Message-----
> From: Jani Nikula <jani.nikula at linux.intel.com>
> Sent: Monday, November 11, 2024 2:03 PM
> To: Garg, Nemesa <nemesa.garg at intel.com>; intel-gfx at lists.freedesktop.org
> Cc: Garg, Nemesa <nemesa.garg at intel.com>; Kulkarni at freedesktop.org;
> Kulkarni, Vandita <vandita.kulkarni at intel.com>
> Subject: Re: [PATCH] drm/i915/display: Add WA_14018221282
> 
> On Mon, 11 Nov 2024, Nemesa Garg <nemesa.garg at intel.com> wrote:
> > It was observed that the first write to DKL PHY DP Mode register was
> > not taking effect, hence rewrite this register.
> >
> > v2: Rename function [Mitul]
> >
> > Signed-off-by: Nemesa Garg <nemesa.garg at intel.com>
> > Signed-off-by: Kulkarni, Vandita <vandita.kulkarni at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index 769bd1f26db2..f955d89951b8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -2104,10 +2104,21 @@ void
> intel_ddi_sanitize_encoder_pll_mapping(struct intel_encoder *encoder)
> >  	encoder->disable_clock(encoder);
> >  }
> >
> > +static void
> > +tgl_is_dp_mode_enabled(struct drm_i915_private *dev_priv,
> > +		       enum tc_port tc_port, u32 ln0, u32 ln1)
> 
> I went ahead and asked ChatGPT what it thinks of the function name:
> 
> If a function's name is tgl_is_dp_mode_enabled(), do you expect it to return a
> value, and what would you expect it to return?
> 
> 
> It replied:
> 
> Yes, based on the function's name, I would expect `tgl_is_dp_mode_enabled()` to
> return a value. The naming convention suggests that it checks whether "DP
> mode" (perhaps "DisplayPort mode") is enabled and returns a Boolean value
> (`true` or `false`).
> 
> In this context:
> 
> - `tgl_` might be a prefix denoting a specific category or module (e.g.,
>   "toggle" or "Tiger Lake" if it's hardware-specific).
> 
> - `is_` implies a question, typical of Boolean-returning functions.
> 
> - `dp_mode_enabled` likely refers to a state or condition ("DisplayPort
>   mode enabled" in this case).
> 
> So, I would expect `tgl_is_dp_mode_enabled()` to return `true` if DP mode is
> currently enabled, and `false` otherwise.
> 
> I will try to come up with some better name. Here if first write fails only then need to re-write the 
register again so don't need to return any value.
Something like icl_dkl_phy_read will be fine?

Thanks and Regard,
Nemesa
> > 
> > +	if (ln0 != intel_dkl_phy_read(dev_priv, DKL_DP_MODE(tc_port, 0)))
> > +		intel_dkl_phy_write(dev_priv, DKL_DP_MODE(tc_port, 0), ln0);
> > +	if (ln1 != intel_dkl_phy_read(dev_priv, DKL_DP_MODE(tc_port, 1)))
> > +		intel_dkl_phy_write(dev_priv, DKL_DP_MODE(tc_port, 1), ln1); }
> > +
> >  static void
> >  icl_program_mg_dp_mode(struct intel_digital_port *dig_port,
> >  		       const struct intel_crtc_state *crtc_state)  {
> > +	struct intel_display *display = to_intel_display(crtc_state);
> >  	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> >  	enum tc_port tc_port = intel_encoder_to_tc(&dig_port->base);
> >  	u32 ln0, ln1, pin_assignment;
> > @@ -2185,6 +2196,10 @@ icl_program_mg_dp_mode(struct intel_digital_port
> *dig_port,
> >  	if (DISPLAY_VER(dev_priv) >= 12) {
> >  		intel_dkl_phy_write(dev_priv, DKL_DP_MODE(tc_port, 0), ln0);
> >  		intel_dkl_phy_write(dev_priv, DKL_DP_MODE(tc_port, 1), ln1);
> > +		 /* WA_14018221282 */
> > +		if (DISPLAY_VER(display) == 12)
> > +			tgl_is_dp_mode_enabled(dev_priv, tc_port, ln0, ln1);
> > +
> >  	} else {
> >  		intel_de_write(dev_priv, MG_DP_MODE(0, tc_port), ln0);
> >  		intel_de_write(dev_priv, MG_DP_MODE(1, tc_port), ln1);
> 
> --
> Jani Nikula, Intel