> > Subject: [PATCH v4 02/10] drm/i915/dp: Limit M/N ratio for DP SST
> >
> > The hardware cannot support DisplayPort configurations where the
> > ceiling of the Link M/Link N ratio exceeds the limit the transcoder's
> > timing generator DDA can track. This limitation has always existed,
> > but it typically wasn't encountered without the use of joiners and DSC.
> >
> > With higher resolutions and combinations involving joiners and DSC,
> > this constraint can now be hit in certain scenarios.
> >
> > Restrict the link configurations to those satisfying the constraint
> > while computing the link config limits, so that the SST and the MST
> > paths both get it, and so that a mode which no link rate can support
> > fails early rather than deep inside link config selection.
>
> Nit: This check is inside intel_dp_compute_config_limits, which
> mst_stream_compute_config_limits also calls, so MST compute is already
> constrained. The subject ("for DP SST") and the "applies only for SST" note
> contradict that and the "SST and MST paths both get it" sentence. Please say
> that MST compute is covered and only MST mode_valid is left for the follow-
> up.
Sure will update the commit message
>
> >
> > Also prune such modes in mode_valid. Note the ratio is the pixel clock
> > over the link symbol clock, and the symbol clock is not monotonic in
> > the link rate: it is the rate divided by 32 for 128b/132b but only by 10 for
> 8b/10b, so e.g.
> > HBR3 has a higher symbol clock than UHBR20. There is therefore no
> > single best link rate to evaluate against, and mode_valid walks the
> > link configurations instead, only rejecting the mode if none of them
> > can satisfy the ratio.
> >
> > Note: This change applies the check only for SST. Support for MST will
> > be added in a subsequent commit.
> >
> > v2:
> > -Move the M/N ratio check to the link rate configuration phase instead
> > of during M/N computation. (Ville) -Prune modes that cannot be
> > supported even with highest link rate due to M/N ratio restriction.
> > v3:
> > -Rebase on the link caps iterator based link config selection.
> > -Set the constraint up in intel_dp_compute_config_limits() by
> > narrowing the link config filter, and fail there if no config
> > qualifies, instead of skipping configs while iterating them. (Imre) -Use a
> limit of 4 on DG2.
> > (Wa_14014191401) -Walk the link configurations in mode_valid rather
> > than evaluating the ratio at the highest link rate, which is not the best
> > case
> for it.
> > v4:
> > -Add iterator for all dp configs in intel_dp_mode_valid_m_n() so that
> > we do not prune any mode that could have been supported (Sashiko)
> >
> > Signed-off-by: Ankit Nautiyal <[email protected]>
> > Signed-off-by: Suraj Kandpal <[email protected]>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 4 +-
> > drivers/gpu/drm/i915/display/intel_display.h | 2 +
> > drivers/gpu/drm/i915/display/intel_dp.c | 101 +++++++++++++++++++
> > drivers/gpu/drm/i915/display/intel_dp.h | 2 +
> > 4 files changed, 107 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index f04a6f97e05a..c53163d7c082 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -2641,8 +2641,8 @@ static void compute_m_n(u32 *ret_m, u32
> *ret_n,
> > intel_reduce_m_n_ratio(ret_m, ret_n); }
> >
> > -static void intel_display_compute_link_m_n(u32 *link_m, u32 *link_n,
> > - int pixel_clock, int
> > link_symbol_clock)
> > +void intel_display_compute_link_m_n(u32 *link_m, u32 *link_n,
> > + int pixel_clock, int link_symbol_clock)
> > {
> > compute_m_n(link_m, link_n,
> > pixel_clock, link_symbol_clock, diff --git
> > a/drivers/gpu/drm/i915/display/intel_display.h
> > b/drivers/gpu/drm/i915/display/intel_display.h
> > index eecafff167d3..404168f4213b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > @@ -523,5 +523,7 @@ bool assert_port_valid(struct intel_display
> > *display, enum port port);
> >
> > bool intel_scanout_needs_vtd_wa(struct intel_display *display); int
> > intel_crtc_num_joined_pipes(const struct intel_crtc_state
> > *crtc_state);
> > +void intel_display_compute_link_m_n(u32 *link_m, u32 *link_n,
> > + int pixel_clock, int link_symbol_clock);
> >
> > #endif
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 0cd5e6b5034c..f6a2152c4432 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -61,6 +61,7 @@
> > #include "intel_cx0_phy.h"
> > #include "intel_ddi.h"
> > #include "intel_de.h"
> > +#include "intel_display.h"
> > #include "intel_display_driver.h"
> > #include "intel_display_jiffies.h"
> > #include "intel_display_utils.h"
> > @@ -1265,6 +1266,34 @@ bool intel_dp_has_dsc(const struct
> > intel_connector *connector)
> > return true;
> > }
> >
> > +static int
> > +intel_dp_get_max_m_n_ratio(struct intel_display *display) {
> > + if (display->platform.dg2)
> > + return 4;
>
> Please route the DG2 limit through intel_display_wa()
> (INTEL_DISPLAY_WA_14014191401) instead of a bare platform check. Right
> now the Wa ID only appears in the changelog.
>
Ill add a seprarte patch series moving this into its own display wa framework
rather than doing it here
Since this series is already adding a WA
> > +
> > + return 10;
>
> Also, what's the BSpec reference for the generic limit of 10 ?
Will add it
> > +}
> > +
> > +static bool
> > +intel_dp_can_support_m_n(struct intel_display *display,
> > + int pixel_clock, int link_rate)
> > +{
> > + int max_m_n_ratio = intel_dp_get_max_m_n_ratio(display);
> > + u32 link_m, link_n;
> > + int m_n_ratio;
> > +
> > + intel_display_compute_link_m_n(&link_m, &link_n, pixel_clock,
> > + intel_dp_link_symbol_clock(link_rate));
> > +
> > + if (!link_n)
> > + return true;
>
> link_n can't be 0 here: compute_m_n() fixes N at 0x80000 and only halves it
> while M/N exceed 24 bits. Please drop the check.
Will drop it
>
> > +
> > + m_n_ratio = DIV_ROUND_UP(link_m, link_n);
> > +
> > + return m_n_ratio <= max_m_n_ratio;
> > +}
> > +
> > static
> > bool intel_dp_can_join(struct intel_dp *intel_dp,
> > int num_joined_pipes)
> > @@ -1300,6 +1329,31 @@ bool intel_dp_dotclk_valid(struct intel_display
> > *display,
> > return target_clock <= effective_dotclk_limit; }
> >
> > +bool intel_dp_mode_valid_m_n(struct intel_dp *intel_dp, int
> > +pixel_clock) {
> > + struct intel_display *display = to_intel_display(intel_dp);
> > + struct intel_dp_link_caps_order order = {
> > + .key = INTEL_DP_LINK_CAPS_ORDER_KEY_RATE_LANE,
> > + .dir = INTEL_DP_LINK_CAPS_ORDER_DIR_DESC,
> > + };
> > + struct intel_dp_link_config link_config;
> > + struct intel_dp_link_caps_iter iter;
> > + bool found = false;
> > +
> > + intel_dp_link_caps_iter_start(&iter, intel_dp->link.caps, order,
> > + INTEL_DP_LINK_CAPS_FILTER_ALL);
> > + for_each_dp_link_config(&iter, &link_config) {
> > + if (intel_dp_can_support_m_n(display, pixel_clock,
> > link_config.rate)) {
> > + found = true;
> > +
> > + break;
> > + }
> > + }
> > + intel_dp_link_caps_iter_end(&iter);
> > +
> > + return found;
> > +}
> > +
> > static enum drm_mode_status
> > intel_dp_mode_valid_format(struct intel_connector *connector,
> > const struct drm_display_mode *mode, @@ -
> > 1454,6 +1508,9 @@ intel_dp_mode_valid(struct drm_connector
> > *_connector,
> > return status;
> > }
> >
> > + if (!intel_dp_mode_valid_m_n(intel_dp, target_clock))
> > + return MODE_CLOCK_HIGH;
> > +
> > /*
> > * TODO: Even when using a 4:2:0 sink_format
> > intel_dp_output_format()
> > * will always choose a 4:4:4 output_format if the DFP can do the
> @@
> > -2654,6 +2711,47 @@ intel_dp_dsc_compute_pipe_bpp_limits(struct
> > intel_connector *connector,
> > return true;
> > }
> >
> > +static bool
> > +intel_dp_compute_m_n_ratio_limits(struct intel_dp *intel_dp,
> > + struct drm_connector_state *conn_state,
> > + const struct intel_crtc_state *crtc_state,
> > + struct link_config_limits *limits) {
> > + struct intel_display *display = to_intel_display(intel_dp);
> > + struct intel_connector *connector = to_intel_connector(conn_state-
> > >connector);
> > + struct intel_dp_link_caps *link_caps = intel_dp->link.caps;
> > + struct intel_dp_link_caps_order order =
> > + intel_dp_link_caps_connector_compute_order(connector);
> > + struct intel_dp_link_caps_filter new_filter =
> > INTEL_DP_LINK_CAPS_FILTER_NONE;
> > + int pixel_clock = intel_dp_mode_clock(crtc_state, conn_state);
> > + struct intel_dp_link_config link_config;
> > + struct intel_dp_link_caps_iter iter;
> > + bool found = false;
> > +
> > + intel_dp_link_caps_iter_start(&iter, link_caps, order, limits-
> > >link_config_filter);
> > + for_each_dp_link_config(&iter, &link_config) {
> > + if (!intel_dp_can_support_m_n(display, pixel_clock,
> > link_config.rate))
> > + continue;
> > +
> > + intel_dp_link_caps_filter_add(link_caps, &new_filter,
> > &link_config);
> > + found = true;
> > + }
> > + intel_dp_link_caps_iter_end(&iter);
> > +
> > + if (!found) {
> > + drm_dbg_kms(display->drm,
> > + "[CONNECTOR:%d:%s] No link config with a Link
> > M/N ratio <= %d\n",
> > + connector->base.base.id, connector->base.name,
> > + intel_dp_get_max_m_n_ratio(display));
> > +
> > + return false;
> > + }
> > +
> > + limits->link_config_filter = new_filter;
> > +
> > + return true;
> > +}
> > +
> > bool
> > intel_dp_compute_config_limits(struct intel_dp *intel_dp,
> > struct drm_connector_state *conn_state, @@ -
> > 2770,6 +2868,9 @@ intel_dp_compute_config_limits(struct intel_dp
> > *intel_dp,
> > limits->link_config_filter = new_filter;
> > }
> >
> > + if (!intel_dp_compute_m_n_ratio_limits(intel_dp, conn_state,
> > crtc_state, limits))
> > + return false;
>
> For MSO (eDP splitter) this uses the unsplit clock, while the programmed link
> M/N uses crtc_clock / mso_link_count (computed later in
> intel_dp_compute_config()). Does the DDA limit apply to the per-segment
> clock? If so, the check is too strict by a factor of mso_link_count.
We use unsplit clock here and switch to using mso clock later in patch 4 so its
not a problem.
>
> > +
> > if (!intel_dp_test_compute_config(connector, crtc_state, limits))
> > return false;
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.h
> > b/drivers/gpu/drm/i915/display/intel_dp.h
> > index 04fae203bb69..aa58833a405b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> > @@ -239,6 +239,8 @@ bool intel_dp_joiner_candidate_valid(struct
> > intel_connector *connector,
> >
> > u8 intel_dp_as_sdp_transmission_time(void);
> >
> > +bool intel_dp_mode_valid_m_n(struct intel_dp *intel_dp, int
> > +pixel_clock);
>
> nit: This is only used in intel_dp.c. Make it static unless the MST follow-up
> needs it, and if it does, say so.
This is used in a later commit . will mention in commit message
Regards,
Suraj Kandpal
>
> > +
> > int intel_dp_link_init(struct intel_dp *intel_dp); void
> > intel_dp_link_cleanup(struct intel_dp *intel_dp);
> >
> > --
> > 2.34.1
