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.
> 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
>
