| Message ID | 20241111080808.3182188-1-nemesa.garg@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/display: Add WA_14018221282 | expand |
On Mon, 11 Nov 2024, Nemesa Garg <nemesa.garg@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@intel.com> > Signed-off-by: Kulkarni, Vandita <vandita.kulkarni@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. > +{ > + 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);
> -----Original Message----- > From: Jani Nikula <jani.nikula@linux.intel.com> > Sent: Monday, November 11, 2024 2:03 PM > To: Garg, Nemesa <nemesa.garg@intel.com>; intel-gfx@lists.freedesktop.org > Cc: Garg, Nemesa <nemesa.garg@intel.com>; Kulkarni@freedesktop.org; > Kulkarni, Vandita <vandita.kulkarni@intel.com> > Subject: Re: [PATCH] drm/i915/display: Add WA_14018221282 > > On Mon, 11 Nov 2024, Nemesa Garg <nemesa.garg@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@intel.com> > > Signed-off-by: Kulkarni, Vandita <vandita.kulkarni@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
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) +{ + 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);