> -----Original Message-----
> From: Deak, Imre <imre.deak@xxxxxxxxx>
> Sent: Tuesday, November 14, 2023 1:13 PM
> To: Murthy, Arun R <arun.r.murthy@xxxxxxxxx>
> Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH 2/4] drm/i915/dp: Fix UHBR link M/N values
>
> On Tue, Nov 14, 2023 at 05:29:35AM +0200, Murthy, Arun R wrote:
> >
> > > -----Original Message-----
> > > From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf
> > > Of Imre Deak
> > > Sent: Tuesday, November 14, 2023 1:41 AM
> > > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > Subject: [PATCH 2/4] drm/i915/dp: Fix UHBR link M/N
> > > values
> > >
> > > The link M/N ratio is the data rate / link symbol clock rate, fix
> > > things up accordingly. On DP 1.4 this ratio was correct as the link
> > > symbol clock rate in that case matched the link data rate (in
> > > bytes/sec units, the symbol size being 8 bits), however it wasn't correct for
> UHBR rates where the symbol size is 32 bits.
> > >
> > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_display.c | 16 ++++++++-----
> > > drivers/gpu/drm/i915/display/intel_dp.c | 24 ++++++++++++++++++++
> > > drivers/gpu/drm/i915/display/intel_dp.h | 2 ++
> > > 3 files changed, 36 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 24aebdb715e7d..c059eb0170a5b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -2411,6 +2411,7 @@ intel_link_compute_m_n(u16 bits_per_pixel, int
> > > nlanes,
> > > struct intel_link_m_n *m_n) {
> > > u32 data_clock = bits_per_pixel * pixel_clock;
> > > + u32 link_symbol_clock =
> > > + intel_dp_link_symbol_clock(link_clock);
> > > u32 data_m;
> > > u32 data_n;
> > >
> > > @@ -2431,7 +2432,7 @@ intel_link_compute_m_n(u16 bits_per_pixel, int
> > > nlanes,
> > > 0x8000000);
> > >
> > > compute_m_n(&m_n->link_m, &m_n->link_n,
> > > - pixel_clock, link_clock,
> > > + pixel_clock, link_symbol_clock,
> > > 0x80000);
> > > }
> > Better if this can be moved to intel_dp.c
>
> The function is also used by non-DP outputs, so not sure. In any case it would
> need to be a separate change.
>
> > Also per the spec the constant N values is 0x800000
>
> For the link M/N values I can't see this in the spec. It's mentioned in the context
> of calculating data M/N. Changing that - if it makes sense - should be in a
> separate patch.
Agree!
Reviewed-by: Arun R Murthy <arun.r.murthy@xxxxxxxxx>
Thanks and Regards,
Arun R Murthy
-------------------
>
> > The calculation of data M has dependency with DP symbol
> > >
> > > @@ -3943,20 +3944,23 @@ int intel_dotclock_calculate(int link_freq,
> > > const struct intel_link_m_n *m_n) {
> > > /*
> > > - * The calculation for the data clock is:
> > > + * The calculation for the data clock -> pixel clock is:
> > > * pixel_clock = ((m/n)*(link_clock * nr_lanes))/bpp
> > > * But we want to avoid losing precison if possible, so:
> > > * pixel_clock = ((m * link_clock * nr_lanes)/(n*bpp))
> > > *
> > > - * and the link clock is simpler:
> > > - * link_clock = (m * link_clock) / n
> > > + * and for link freq (10kbs units) -> pixel clock it is:
> > > + * link_symbol_clock = link_freq * 10 / link_symbol_size
> > > + * pixel_clock = (m * link_symbol_clock) / n
> > > + * or for more precision:
> > > + * pixel_clock = (m * link_freq * 10) / (n * link_symbol_size)
> > > */
> > >
> > > if (!m_n->link_n)
> > > return 0;
> > >
> > > - return DIV_ROUND_UP_ULL(mul_u32_u32(m_n->link_m, link_freq),
> > > - m_n->link_n);
> > > + return DIV_ROUND_UP_ULL(mul_u32_u32(m_n->link_m, link_freq *
> > > 10),
> > > + m_n->link_n *
> > > intel_dp_link_symbol_size(link_freq));
> > > }
> > >
> > > int intel_crtc_dotclock(const struct intel_crtc_state *pipe_config)
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index f662d1ce5f72c..80e1e887432fa 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -132,6 +132,30 @@ bool intel_dp_is_uhbr(const struct
> > > intel_crtc_state
> > > *crtc_state)
> > > return intel_dp_is_uhbr_rate(crtc_state->port_clock);
> > > }
> > >
> > > +/**
> > > + * intel_dp_link_symbol_size - get the link symbol size for a given
> > > +link rate
> > > + * @rate: link rate in 10kbit/s units
> > > + *
> > > + * Returns the link symbol size in bits/symbol units depending on
> > > +the link
> > > + * rate -> channel coding.
> > > + */
> > > +int intel_dp_link_symbol_size(int rate) {
> > > + return intel_dp_is_uhbr_rate(rate) ? 32 : 10; }
> > As per the spec this DP symbol is 32 for DP2.0 and 8 for DP1.4
>
> On DP1.4 before the 8b/10b conversion the symbol size is 8 bits, after the
> conversion (which is what @rate describes and for which the symbol size is
> returned for) it's 10 bits.
>
> >
> > Thanks and Regards,
> > Arun R Murthy
> > --------------------
> > > +
> > > +/**
> > > + * intel_dp_link_symbol_clock - convert link rate to link symbol
> > > +clock
> > > + * @rate: link rate in 10kbit/s units
> > > + *
> > > + * Returns the link symbol clock frequency in kHz units depending
> > > +on the
> > > + * link rate and channel coding.
> > > + */
> > > +int intel_dp_link_symbol_clock(int rate) {
> > > + return DIV_ROUND_CLOSEST(rate * 10,
> > > intel_dp_link_symbol_size(rate));
> > > +}
> > > +
> > > static void intel_dp_set_default_sink_rates(struct intel_dp *intel_dp) {
> > > intel_dp->sink_rates[0] = 162000; diff --git
> > > a/drivers/gpu/drm/i915/display/intel_dp.h
> > > b/drivers/gpu/drm/i915/display/intel_dp.h
> > > index e80da67554196..64dbf8f192708 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> > > @@ -82,6 +82,8 @@ bool intel_dp_has_hdmi_sink(struct intel_dp
> > > *intel_dp); bool intel_dp_is_edp(struct intel_dp *intel_dp); bool
> > > intel_dp_is_uhbr_rate(int rate); bool intel_dp_is_uhbr(const struct
> > > intel_crtc_state *crtc_state);
> > > +int intel_dp_link_symbol_size(int rate); int
> > > +intel_dp_link_symbol_clock(int rate);
> > > bool intel_dp_is_port_edp(struct drm_i915_private *dev_priv, enum
> > > port port); enum irqreturn intel_dp_hpd_pulse(struct intel_digital_port
> *dig_port,
> > > bool long_hpd);
> > > --
> > > 2.39.2
> >
