| Message ID | 20250321112650.3594298-2-ankit.k.nautiyal@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Implement Wa_14021768792 to bypass m_n ratio limit | expand |
On Fri, Mar 21, 2025 at 04:56:47PM +0530, Ankit Nautiyal wrote: > LINK_N register has bits 31:24 for extended link N value used for > HDMI2.1 and for an alternate mode of operation of DP TG DDA > (Bspec:50488). > > Add support for these extra bits. > > For displays with version 14 or higher, the `PIPE_LINK_N1_EXTENDED_MASK` > (bits 31:24) is used to handle the extended link N bits. > For older platforms, the `DATA_LINK_M_N_MASK` (bits 23:0) is used to > handle the standard link N bits. This distinction ensures clarity and > maintains the semantics for platforms that support the extended bits. > In subsequent changes the logic is updated to conditionally apply the > extended link N bits. > > v2: Drop extra link_n_ext member. (Jani) > v3: Avoid link_n_ext in set_m_n helper. (Jani) > v4: Rebase, and update commit message. > > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 18 ++++++++++++++++-- > drivers/gpu/drm/i915/i915_reg.h | 2 ++ > 2 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 3afb85fe8536..8fb0df388571 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -2583,14 +2583,22 @@ void intel_set_m_n(struct intel_display *display, > i915_reg_t data_m_reg, i915_reg_t data_n_reg, > i915_reg_t link_m_reg, i915_reg_t link_n_reg) > { > + u32 link_n = m_n->link_n; > + > intel_de_write(display, data_m_reg, TU_SIZE(m_n->tu) | m_n->data_m); > intel_de_write(display, data_n_reg, m_n->data_n); > intel_de_write(display, link_m_reg, m_n->link_m); > + > + if (DISPLAY_VER(display) >= 14) > + link_n &= ~PIPE_LINK_N1_EXTENDED_MASK; > + else > + link_n &= DATA_LINK_M_N_MASK; There should never be anything in those bits or we've screwed up somewhere. The actual w/a for the M/N > 10.0 looks like a pile of annoying hacks in the hardware. I wonder if we could just live without it to avoid complicating the code? As for the 10.0 limit, I suspect there's nothing platform specific about it and it's always been there. It's just not possible to hit it with 8b/10b encoding. The correct place to handle this would seem to be the link rate calculation, ie. just bump up the link rate until the limit is no longer an issue (or error out if we can't increase the link rate sufficiently). > + > /* > * On BDW+ writing LINK_N arms the double buffered update > * of all the M/N registers, so it must be written last. > */ > - intel_de_write(display, link_n_reg, m_n->link_n); > + intel_de_write(display, link_n_reg, link_n); > } > > bool intel_cpu_transcoder_has_m2_n2(struct intel_display *display, > @@ -3279,7 +3287,13 @@ void intel_get_m_n(struct intel_display *display, > i915_reg_t link_m_reg, i915_reg_t link_n_reg) > { > m_n->link_m = intel_de_read(display, link_m_reg) & DATA_LINK_M_N_MASK; > - m_n->link_n = intel_de_read(display, link_n_reg) & DATA_LINK_M_N_MASK; > + m_n->link_n = intel_de_read(display, link_n_reg); > + > + if (DISPLAY_VER(display) >= 14) > + m_n->link_n &= ~PIPE_LINK_N1_EXTENDED_MASK; > + else > + m_n->link_n &= DATA_LINK_M_N_MASK; > + > m_n->data_m = intel_de_read(display, data_m_reg) & DATA_LINK_M_N_MASK; > m_n->data_n = intel_de_read(display, data_n_reg) & DATA_LINK_M_N_MASK; > m_n->tu = REG_FIELD_GET(TU_SIZE_MASK, intel_de_read(display, data_m_reg)) + 1; > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index c5064eebe063..a2054aced4f8 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -1869,6 +1869,8 @@ > > #define _PIPEA_LINK_N1 0x60044 > #define _PIPEB_LINK_N1 0x61044 > +#define PIPE_LINK_N1_EXTENDED_MASK REG_GENMASK(31, 24) > +#define PIPE_LINK_N1_EXTENDED(val) REG_FIELD_PREP(PIPE_LINK_N1_EXTENDED_MASK, (val)) > #define PIPE_LINK_N1(dev_priv, tran) _MMIO_TRANS2(dev_priv, tran, _PIPEA_LINK_N1) > > #define _PIPEA_LINK_M2 0x60048 > -- > 2.45.2
On Fri, Mar 21, 2025 at 08:31:22PM +0200, Ville Syrjälä wrote: > On Fri, Mar 21, 2025 at 04:56:47PM +0530, Ankit Nautiyal wrote: > > LINK_N register has bits 31:24 for extended link N value used for > > HDMI2.1 and for an alternate mode of operation of DP TG DDA > > (Bspec:50488). > > > > Add support for these extra bits. > > > > For displays with version 14 or higher, the `PIPE_LINK_N1_EXTENDED_MASK` > > (bits 31:24) is used to handle the extended link N bits. > > For older platforms, the `DATA_LINK_M_N_MASK` (bits 23:0) is used to > > handle the standard link N bits. This distinction ensures clarity and > > maintains the semantics for platforms that support the extended bits. > > In subsequent changes the logic is updated to conditionally apply the > > extended link N bits. > > > > v2: Drop extra link_n_ext member. (Jani) > > v3: Avoid link_n_ext in set_m_n helper. (Jani) > > v4: Rebase, and update commit message. > > > > Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 18 ++++++++++++++++-- > > drivers/gpu/drm/i915/i915_reg.h | 2 ++ > > 2 files changed, 18 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > index 3afb85fe8536..8fb0df388571 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -2583,14 +2583,22 @@ void intel_set_m_n(struct intel_display *display, > > i915_reg_t data_m_reg, i915_reg_t data_n_reg, > > i915_reg_t link_m_reg, i915_reg_t link_n_reg) > > { > > + u32 link_n = m_n->link_n; > > + > > intel_de_write(display, data_m_reg, TU_SIZE(m_n->tu) | m_n->data_m); > > intel_de_write(display, data_n_reg, m_n->data_n); > > intel_de_write(display, link_m_reg, m_n->link_m); > > + > > + if (DISPLAY_VER(display) >= 14) > > + link_n &= ~PIPE_LINK_N1_EXTENDED_MASK; > > + else > > + link_n &= DATA_LINK_M_N_MASK; > > There should never be anything in those bits or we've screwed up > somewhere. > > The actual w/a for the M/N > 10.0 looks like a pile of annoying > hacks in the hardware. I wonder if we could just live without it > to avoid complicating the code? > > As for the 10.0 limit, I suspect there's nothing platform specific > about it and it's always been there. It's just not possible to hit > it with 8b/10b encoding. Hmm. Maybe it would be possible to hit it with DSC+bigjoiner on ICL+. Without some kind of joiner it's definitely not possible due to cdclk limiting the max dotclock well below 1.62 GHz. > The correct place to handle this would seem > to be the link rate calculation, ie. just bump up the link rate until > the limit is no longer an issue (or error out if we can't increase the > link rate sufficiently). > > > + > > /* > > * On BDW+ writing LINK_N arms the double buffered update > > * of all the M/N registers, so it must be written last. > > */ > > - intel_de_write(display, link_n_reg, m_n->link_n); > > + intel_de_write(display, link_n_reg, link_n); > > } > > > > bool intel_cpu_transcoder_has_m2_n2(struct intel_display *display, > > @@ -3279,7 +3287,13 @@ void intel_get_m_n(struct intel_display *display, > > i915_reg_t link_m_reg, i915_reg_t link_n_reg) > > { > > m_n->link_m = intel_de_read(display, link_m_reg) & DATA_LINK_M_N_MASK; > > - m_n->link_n = intel_de_read(display, link_n_reg) & DATA_LINK_M_N_MASK; > > + m_n->link_n = intel_de_read(display, link_n_reg); > > + > > + if (DISPLAY_VER(display) >= 14) > > + m_n->link_n &= ~PIPE_LINK_N1_EXTENDED_MASK; > > + else > > + m_n->link_n &= DATA_LINK_M_N_MASK; > > + > > m_n->data_m = intel_de_read(display, data_m_reg) & DATA_LINK_M_N_MASK; > > m_n->data_n = intel_de_read(display, data_n_reg) & DATA_LINK_M_N_MASK; > > m_n->tu = REG_FIELD_GET(TU_SIZE_MASK, intel_de_read(display, data_m_reg)) + 1; > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index c5064eebe063..a2054aced4f8 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -1869,6 +1869,8 @@ > > > > #define _PIPEA_LINK_N1 0x60044 > > #define _PIPEB_LINK_N1 0x61044 > > +#define PIPE_LINK_N1_EXTENDED_MASK REG_GENMASK(31, 24) > > +#define PIPE_LINK_N1_EXTENDED(val) REG_FIELD_PREP(PIPE_LINK_N1_EXTENDED_MASK, (val)) > > #define PIPE_LINK_N1(dev_priv, tran) _MMIO_TRANS2(dev_priv, tran, _PIPEA_LINK_N1) > > > > #define _PIPEA_LINK_M2 0x60048 > > -- > > 2.45.2 > > -- > Ville Syrjälä > Intel
On 3/22/2025 12:01 AM, Ville Syrjälä wrote: > On Fri, Mar 21, 2025 at 04:56:47PM +0530, Ankit Nautiyal wrote: >> LINK_N register has bits 31:24 for extended link N value used for >> HDMI2.1 and for an alternate mode of operation of DP TG DDA >> (Bspec:50488). >> >> Add support for these extra bits. >> >> For displays with version 14 or higher, the `PIPE_LINK_N1_EXTENDED_MASK` >> (bits 31:24) is used to handle the extended link N bits. >> For older platforms, the `DATA_LINK_M_N_MASK` (bits 23:0) is used to >> handle the standard link N bits. This distinction ensures clarity and >> maintains the semantics for platforms that support the extended bits. >> In subsequent changes the logic is updated to conditionally apply the >> extended link N bits. >> >> v2: Drop extra link_n_ext member. (Jani) >> v3: Avoid link_n_ext in set_m_n helper. (Jani) >> v4: Rebase, and update commit message. >> >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_display.c | 18 ++++++++++++++++-- >> drivers/gpu/drm/i915/i915_reg.h | 2 ++ >> 2 files changed, 18 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c >> index 3afb85fe8536..8fb0df388571 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display.c >> +++ b/drivers/gpu/drm/i915/display/intel_display.c >> @@ -2583,14 +2583,22 @@ void intel_set_m_n(struct intel_display *display, >> i915_reg_t data_m_reg, i915_reg_t data_n_reg, >> i915_reg_t link_m_reg, i915_reg_t link_n_reg) >> { >> + u32 link_n = m_n->link_n; >> + >> intel_de_write(display, data_m_reg, TU_SIZE(m_n->tu) | m_n->data_m); >> intel_de_write(display, data_n_reg, m_n->data_n); >> intel_de_write(display, link_m_reg, m_n->link_m); >> + >> + if (DISPLAY_VER(display) >= 14) >> + link_n &= ~PIPE_LINK_N1_EXTENDED_MASK; >> + else >> + link_n &= DATA_LINK_M_N_MASK; > There should never be anything in those bits or we've screwed up > somewhere. > > The actual w/a for the M/N > 10.0 looks like a pile of annoying > hacks in the hardware. I wonder if we could just live without it > to avoid complicating the code? Hi Ville, Apologies for replying late on this. Yes for general case we should not need it, but for very high refresh modes. For BMG since we can support higher resolution with ultrajoiner, this WA seems to be required for such cases to support till M/N ratio 15. I am yet to come across such HW though. > > As for the 10.0 limit, I suspect there's nothing platform specific > about it and it's always been there. Yes that's right, but for newer platforms this seems to be getting changed. > It's just not possible to hit > it with 8b/10b encoding. The correct place to handle this would seem > to be the link rate calculation, ie. just bump up the link rate until > the limit is no longer an issue (or error out if we can't increase the > link rate sufficiently). Alright, will try this out. Do you see a need to check the M/N ratio in modevalid to prune the modes for which, even with highest rate, the ratio is more than the limit? Currently I have not added this change. Regards, Ankit > >> + >> /* >> * On BDW+ writing LINK_N arms the double buffered update >> * of all the M/N registers, so it must be written last. >> */ >> - intel_de_write(display, link_n_reg, m_n->link_n); >> + intel_de_write(display, link_n_reg, link_n); >> } >> >> bool intel_cpu_transcoder_has_m2_n2(struct intel_display *display, >> @@ -3279,7 +3287,13 @@ void intel_get_m_n(struct intel_display *display, >> i915_reg_t link_m_reg, i915_reg_t link_n_reg) >> { >> m_n->link_m = intel_de_read(display, link_m_reg) & DATA_LINK_M_N_MASK; >> - m_n->link_n = intel_de_read(display, link_n_reg) & DATA_LINK_M_N_MASK; >> + m_n->link_n = intel_de_read(display, link_n_reg); >> + >> + if (DISPLAY_VER(display) >= 14) >> + m_n->link_n &= ~PIPE_LINK_N1_EXTENDED_MASK; >> + else >> + m_n->link_n &= DATA_LINK_M_N_MASK; >> + >> m_n->data_m = intel_de_read(display, data_m_reg) & DATA_LINK_M_N_MASK; >> m_n->data_n = intel_de_read(display, data_n_reg) & DATA_LINK_M_N_MASK; >> m_n->tu = REG_FIELD_GET(TU_SIZE_MASK, intel_de_read(display, data_m_reg)) + 1; >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index c5064eebe063..a2054aced4f8 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -1869,6 +1869,8 @@ >> >> #define _PIPEA_LINK_N1 0x60044 >> #define _PIPEB_LINK_N1 0x61044 >> +#define PIPE_LINK_N1_EXTENDED_MASK REG_GENMASK(31, 24) >> +#define PIPE_LINK_N1_EXTENDED(val) REG_FIELD_PREP(PIPE_LINK_N1_EXTENDED_MASK, (val)) >> #define PIPE_LINK_N1(dev_priv, tran) _MMIO_TRANS2(dev_priv, tran, _PIPEA_LINK_N1) >> >> #define _PIPEA_LINK_M2 0x60048 >> -- >> 2.45.2
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 3afb85fe8536..8fb0df388571 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -2583,14 +2583,22 @@ void intel_set_m_n(struct intel_display *display, i915_reg_t data_m_reg, i915_reg_t data_n_reg, i915_reg_t link_m_reg, i915_reg_t link_n_reg) { + u32 link_n = m_n->link_n; + intel_de_write(display, data_m_reg, TU_SIZE(m_n->tu) | m_n->data_m); intel_de_write(display, data_n_reg, m_n->data_n); intel_de_write(display, link_m_reg, m_n->link_m); + + if (DISPLAY_VER(display) >= 14) + link_n &= ~PIPE_LINK_N1_EXTENDED_MASK; + else + link_n &= DATA_LINK_M_N_MASK; + /* * On BDW+ writing LINK_N arms the double buffered update * of all the M/N registers, so it must be written last. */ - intel_de_write(display, link_n_reg, m_n->link_n); + intel_de_write(display, link_n_reg, link_n); } bool intel_cpu_transcoder_has_m2_n2(struct intel_display *display, @@ -3279,7 +3287,13 @@ void intel_get_m_n(struct intel_display *display, i915_reg_t link_m_reg, i915_reg_t link_n_reg) { m_n->link_m = intel_de_read(display, link_m_reg) & DATA_LINK_M_N_MASK; - m_n->link_n = intel_de_read(display, link_n_reg) & DATA_LINK_M_N_MASK; + m_n->link_n = intel_de_read(display, link_n_reg); + + if (DISPLAY_VER(display) >= 14) + m_n->link_n &= ~PIPE_LINK_N1_EXTENDED_MASK; + else + m_n->link_n &= DATA_LINK_M_N_MASK; + m_n->data_m = intel_de_read(display, data_m_reg) & DATA_LINK_M_N_MASK; m_n->data_n = intel_de_read(display, data_n_reg) & DATA_LINK_M_N_MASK; m_n->tu = REG_FIELD_GET(TU_SIZE_MASK, intel_de_read(display, data_m_reg)) + 1; diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index c5064eebe063..a2054aced4f8 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -1869,6 +1869,8 @@ #define _PIPEA_LINK_N1 0x60044 #define _PIPEB_LINK_N1 0x61044 +#define PIPE_LINK_N1_EXTENDED_MASK REG_GENMASK(31, 24) +#define PIPE_LINK_N1_EXTENDED(val) REG_FIELD_PREP(PIPE_LINK_N1_EXTENDED_MASK, (val)) #define PIPE_LINK_N1(dev_priv, tran) _MMIO_TRANS2(dev_priv, tran, _PIPEA_LINK_N1) #define _PIPEA_LINK_M2 0x60048
LINK_N register has bits 31:24 for extended link N value used for HDMI2.1 and for an alternate mode of operation of DP TG DDA (Bspec:50488). Add support for these extra bits. For displays with version 14 or higher, the `PIPE_LINK_N1_EXTENDED_MASK` (bits 31:24) is used to handle the extended link N bits. For older platforms, the `DATA_LINK_M_N_MASK` (bits 23:0) is used to handle the standard link N bits. This distinction ensures clarity and maintains the semantics for platforms that support the extended bits. In subsequent changes the logic is updated to conditionally apply the extended link N bits. v2: Drop extra link_n_ext member. (Jani) v3: Avoid link_n_ext in set_m_n helper. (Jani) v4: Rebase, and update commit message. Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 18 ++++++++++++++++-- drivers/gpu/drm/i915/i915_reg.h | 2 ++ 2 files changed, 18 insertions(+), 2 deletions(-)