[PATCH v2 11/12] drm/i915: Add support for forcing the link bpp on a connector
Imre Deak
imre.deak at intel.com
Wed May 7 12:03:13 UTC 2025
On Wed, May 07, 2025 at 11:07:57AM +0530, Nautiyal, Ankit K wrote:
>
> On 4/28/2025 7:01 PM, Imre Deak wrote:
> > Add support for forcing the link bpp on a connector via a connector
> > debugfs entry. During reducing link bpps due to a link BW limit, keep
> > bpps close to their forced value.
> >
> > Add the debugfs entry to all relevant connectors: all DP connectors and
> > on an FDI link CRT/SDVO/LVDS/HDMI connectors.
> >
> > v2:
> > - Move adding the debugfs entries to this patch.
> > - Rename i915_force_link_bpp to intel_force_link_bpp. (Jani)
> > - Select the relevant connectors via platform checks. (Jani)
> > - Use for_each_new_intel_connector_in_state(). (Jani)
> > - Fix 64 bit division vs. 32 bit build when converting str to q4. (lkp)
> > - Avoid division and addition overflow when converting str to q4.
> >
> > Cc: Jani Nikula <jani.nikula at intel.com>
> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > ---
> > .../drm/i915/display/intel_display_debugfs.c | 2 +
> > .../drm/i915/display/intel_display_device.h | 1 +
> > .../drm/i915/display/intel_display_types.h | 4 +
> > drivers/gpu/drm/i915/display/intel_link_bw.c | 238 +++++++++++++++++-
> > drivers/gpu/drm/i915/display/intel_link_bw.h | 2 +
> > 5 files changed, 240 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_debugfs.c b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > index 8d0a1779dd193..a9b1ec4cf0f75 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_debugfs.c
> > @@ -39,6 +39,7 @@
> > #include "intel_hdcp.h"
> > #include "intel_hdmi.h"
> > #include "intel_hotplug.h"
> > +#include "intel_link_bw.h"
> > #include "intel_panel.h"
> > #include "intel_pps.h"
> > #include "intel_psr.h"
> > @@ -1325,6 +1326,7 @@ void intel_connector_debugfs_add(struct intel_connector *connector)
> > intel_psr_connector_debugfs_add(connector);
> > intel_alpm_lobf_debugfs_add(connector);
> > intel_dp_link_training_debugfs_add(connector);
> > + intel_link_bw_connector_debugfs_add(connector);
> > if (DISPLAY_VER(display) >= 11 &&
> > ((connector_type == DRM_MODE_CONNECTOR_DisplayPort && !connector->mst.dp) ||
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h b/drivers/gpu/drm/i915/display/intel_display_device.h
> > index 87c666792c0da..fe14a92ae8c65 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_device.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
> > @@ -172,6 +172,7 @@ struct intel_display_platforms {
> > #define HAS_GMBUS_BURST_READ(__display) (DISPLAY_VER(__display) >= 10 || (__display)->platform.kabylake)
> > #define HAS_GMBUS_IRQ(__display) (DISPLAY_VER(__display) >= 4)
> > #define HAS_GMCH(__display) (DISPLAY_INFO(__display)->has_gmch)
> > +#define HAS_FDI(__display) (IS_DISPLAY_VER((__display), 5, 8) && !HAS_GMCH(__display))
> > #define HAS_HOTPLUG(__display) (DISPLAY_INFO(__display)->has_hotplug)
> > #define HAS_HW_SAGV_WM(__display) (DISPLAY_VER(__display) >= 13 && !(__display)->platform.dgfx)
> > #define HAS_IPC(__display) (DISPLAY_INFO(__display)->has_ipc)
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 7415564d058a2..23e3e6f204741 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -550,6 +550,10 @@ struct intel_connector {
> > struct intel_dp *dp;
> > } mst;
> > + struct {
> > + int force_bpp_x16;
> > + } link;
> > +
> > /* Work struct to schedule a uevent on link train failure */
> > struct work_struct modeset_retry_work;
> > diff --git a/drivers/gpu/drm/i915/display/intel_link_bw.c b/drivers/gpu/drm/i915/display/intel_link_bw.c
> > index a10cd39926075..b51b62e04fe59 100644
> > --- a/drivers/gpu/drm/i915/display/intel_link_bw.c
> > +++ b/drivers/gpu/drm/i915/display/intel_link_bw.c
> > @@ -3,6 +3,11 @@
> > * Copyright © 2023 Intel Corporation
> > */
> > +#include <linux/ctype.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/int_log.h>
> > +#include <linux/math.h>
> > +
> > #include <drm/drm_fixed.h>
> > #include <drm/drm_print.h>
> > @@ -10,11 +15,33 @@
> > #include "intel_crtc.h"
> > #include "intel_display_core.h"
> > #include "intel_display_types.h"
> > +#include "intel_dp.h"
> > #include "intel_dp_mst.h"
> > #include "intel_dp_tunnel.h"
> > #include "intel_fdi.h"
> > #include "intel_link_bw.h"
> > +static int get_forced_link_bpp_x16(struct intel_atomic_state *state,
> > + const struct intel_crtc *crtc)
> > +{
> > + struct intel_digital_connector_state *conn_state;
> > + struct intel_connector *connector;
> > + int force_bpp_x16 = INT_MAX;
> > + int i;
> > +
> > + for_each_new_intel_connector_in_state(state, connector, conn_state, i) {
> > + if (conn_state->base.crtc != &crtc->base)
> > + continue;
> > +
> > + if (!connector->link.force_bpp_x16)
> > + continue;
>
> Hmm we already have the connector for the given crtc. If
> link.force_bpp_x16 is not set, should we not break from the loop?
Checking all the connectors handles the case where multiple connectors
are connected to the same crtc (possible on old platforms).
> > +
> > + force_bpp_x16 = min(force_bpp_x16, connector->link.force_bpp_x16);
> > + }
> > +
> > + return force_bpp_x16 < INT_MAX ? force_bpp_x16 : 0;
> > +}
> > +
> > /**
> > * intel_link_bw_init_limits - initialize BW limits
> > * @state: Atomic state
> > @@ -31,9 +58,10 @@ void intel_link_bw_init_limits(struct intel_atomic_state *state,
> > limits->force_fec_pipes = 0;
> > limits->bpp_limit_reached_pipes = 0;
> > for_each_pipe(display, pipe) {
> > + struct intel_crtc *crtc = intel_crtc_for_pipe(display, pipe);
> > const struct intel_crtc_state *crtc_state =
> > - intel_atomic_get_new_crtc_state(state,
> > - intel_crtc_for_pipe(display, pipe));
> > + intel_atomic_get_new_crtc_state(state, crtc);
> > + int forced_bpp_x16 = get_forced_link_bpp_x16(state, crtc);
> > if (state->base.duplicated && crtc_state) {
> > limits->max_bpp_x16[pipe] = crtc_state->max_link_bpp_x16;
> > @@ -42,15 +70,19 @@ void intel_link_bw_init_limits(struct intel_atomic_state *state,
> > } else {
> > limits->max_bpp_x16[pipe] = INT_MAX;
> > }
> > +
> > + if (forced_bpp_x16)
> > + limits->max_bpp_x16[pipe] = min(limits->max_bpp_x16[pipe], forced_bpp_x16);
> > }
> > }
> > /**
> > - * intel_link_bw_reduce_bpp - reduce maximum link bpp for a selected pipe
> > + * __intel_link_bw_reduce_bpp - reduce maximum link bpp for a selected pipe
> > * @state: atomic state
> > * @limits: link BW limits
> > * @pipe_mask: mask of pipes to select from
> > * @reason: explanation of why bpp reduction is needed
> > + * @reduce_forced_bpp: allow reducing bpps below their forced link bpp
> > *
> > * Select the pipe from @pipe_mask with the biggest link bpp value and set the
> > * maximum of link bpp in @limits below this value. Modeset the selected pipe,
> > @@ -64,10 +96,11 @@ void intel_link_bw_init_limits(struct intel_atomic_state *state,
> > * - %-ENOSPC if no pipe can further reduce its link bpp
> > * - Other negative error, if modesetting the selected pipe failed
> > */
> > -int intel_link_bw_reduce_bpp(struct intel_atomic_state *state,
> > - struct intel_link_bw_limits *limits,
> > - u8 pipe_mask,
> > - const char *reason)
> > +static int __intel_link_bw_reduce_bpp(struct intel_atomic_state *state,
> > + struct intel_link_bw_limits *limits,
> > + u8 pipe_mask,
> > + const char *reason,
> > + bool reduce_forced_bpp)
> > {
> > struct intel_display *display = to_intel_display(state);
> > enum pipe max_bpp_pipe = INVALID_PIPE;
> > @@ -97,6 +130,10 @@ int intel_link_bw_reduce_bpp(struct intel_atomic_state *state,
> > */
> > link_bpp_x16 = fxp_q4_from_int(crtc_state->pipe_bpp);
> > + if (!reduce_forced_bpp &&
> > + link_bpp_x16 <= get_forced_link_bpp_x16(state, crtc))
> > + continue;
> > +
> > if (link_bpp_x16 > max_bpp_x16) {
> > max_bpp_x16 = link_bpp_x16;
> > max_bpp_pipe = crtc->pipe;
> > @@ -112,6 +149,21 @@ int intel_link_bw_reduce_bpp(struct intel_atomic_state *state,
> > BIT(max_bpp_pipe));
> > }
> > +int intel_link_bw_reduce_bpp(struct intel_atomic_state *state,
> > + struct intel_link_bw_limits *limits,
> > + u8 pipe_mask,
> > + const char *reason)
> > +{
> > + int ret;
> > +
> > + /* Try to keep any forced link BPP. */
> > + ret = __intel_link_bw_reduce_bpp(state, limits, pipe_mask, reason, false);
> > + if (ret == -ENOSPC)
> > + ret = __intel_link_bw_reduce_bpp(state, limits, pipe_mask, reason, true);
> > +
> > + return ret;
> > +}
> > +
> > /**
> > * intel_link_bw_set_bpp_limit_for_pipe - set link bpp limit for a pipe to its minimum
> > * @state: atomic state
> > @@ -245,3 +297,175 @@ int intel_link_bw_atomic_check(struct intel_atomic_state *state,
> > return -EAGAIN;
> > }
> > +
> > +static int force_link_bpp_show(struct seq_file *m, void *data)
> > +{
> > + struct intel_connector *connector = m->private;
> > +
> > + seq_printf(m, FXP_Q4_FMT "\n", FXP_Q4_ARGS(connector->link.force_bpp_x16));
> > +
> > + return 0;
> > +}
> > +
> > +static int str_to_fxp_q4_nonneg_int(const char *str, int *val_x16)
> > +{
> > + unsigned int val;
> > + int err;
> > +
> > + err = kstrtouint(str, 10, &val);
> > + if (err)
> > + return err;
> > +
> > + if (val > INT_MAX >> 4)
> > + return -ERANGE;
> > +
> > + *val_x16 = fxp_q4_from_int(val);
> > +
> > + return 0;
> > +}
> > +
> > +/* modifies str */
> > +static int str_to_fxp_q4_nonneg(char *str, int *val_x16)
> > +{
> > + const char *int_str;
> > + char *frac_str;
> > + int frac_digits;
> > + int frac_val;
> > + int err;
> > +
> > + int_str = strim(str);
> > + frac_str = strchr(int_str, '.');
> > +
> > + if (frac_str)
> > + *frac_str++ = '\0';
> > +
> > + err = str_to_fxp_q4_nonneg_int(int_str, val_x16);
> > + if (err)
> > + return err;
> > +
> > + if (!frac_str)
> > + return 0;
> > +
> > + /* prevent negative number/leading +- sign mark */
> > + if (!isdigit(*frac_str))
> > + return -EINVAL;
> > +
> > + err = str_to_fxp_q4_nonneg_int(frac_str, &frac_val);
> > + if (err)
> > + return err;
> > +
> > + frac_digits = strlen(frac_str);
> > + if (frac_digits > intlog10(INT_MAX) >> 24 ||
> > + frac_val > INT_MAX - int_pow(10, frac_digits) / 2)
> > + return -ERANGE;
> > +
> > + frac_val = DIV_ROUND_CLOSEST(frac_val, (int)int_pow(10, frac_digits));
> > +
> > + if (*val_x16 > INT_MAX - frac_val)
> > + return -ERANGE;
> > +
> > + *val_x16 += frac_val;
> > +
> > + return 0;
> > +}
> > +
> > +static int user_str_to_fxp_q4_nonneg(const char __user *ubuf, size_t len, int *val_x16)
> > +{
> > + char *kbuf;
> > + int err;
> > +
> > + kbuf = memdup_user_nul(ubuf, len);
> > + if (IS_ERR(kbuf))
> > + return PTR_ERR(kbuf);
> > +
> > + err = str_to_fxp_q4_nonneg(kbuf, val_x16);
> > +
> > + kfree(kbuf);
> > +
> > + return err;
> > +}
> > +
> > +static bool connector_supports_dsc(struct intel_connector *connector)
> > +{
> > + struct intel_display *display = to_intel_display(connector);
> > +
> > + switch (connector->base.connector_type) {
> > + case DRM_MODE_CONNECTOR_eDP:
> > + return intel_dp_has_dsc(connector);
> > + case DRM_MODE_CONNECTOR_DisplayPort:
> > + if (connector->mst.dp)
> > + return HAS_DSC_MST(display);
> > +
> > + return HAS_DSC(display);
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > +static ssize_t
> > +force_link_bpp_write(struct file *file, const char __user *ubuf, size_t len, loff_t *offp)
> > +{
> > + struct seq_file *m = file->private_data;
> > + struct intel_connector *connector = m->private;
> > + struct intel_display *display = to_intel_display(connector);
> > + int min_bpp;
> > + int bpp_x16;
> > + int err;
> > +
> > + err = user_str_to_fxp_q4_nonneg(ubuf, len, &bpp_x16);
> > + if (err)
> > + return err;
> > +
> > + if (connector_supports_dsc(connector))
> > + min_bpp = intel_dp_dsc_min_src_compressed_bpp();
> > + else
> > + min_bpp = intel_display_min_pipe_bpp();
>
> Alright, so for forcing link bpp to say 10 bpp (160 bppx16) for a connector
> supporting DSC will automatically force use of DSC.
Right.
> So if an MST setup has 1 DSC and 1 non DSC panel connected. Setting 10 bpp
> for DSC one will work but setting 10 bpp for non DSC would not work.
Yes, setting a bpp in the DSC range will enable DSC when committing on a
panel supporting DSC and the set bpp is within the range it supports and
will fail the commit for a non-DSC panel.
> Overall I agree with the debugfs and mechanism to force the link bpp.
>
> I am thinking from the validation perspective, how the tests should be
> designed to use this debugfs.
>
> Something like:
>
> ->Check for DSC capability for the panel then force an appropriate
> link bpp (it can try lower value, the write will fail if its not in
> range)
Writing a value to the debugfs entry outside of the range of the sink
still succeeds, but the commit will fail. This allows for setting the
bpp already before connecting the sink and also allows for testing an
out-of-range value.
> ->Force DSC on DSC capable, force link bpp for non DSC.
There are different things that could be still tested. Forcing DSC (via
the current i915_dsc_fec_support) would just test DSC with whatever
compressed bpp the driver selects by default. Forcing a link bpp would
test a particular compressed or non-compressed link bpp, which would be
also good to test separately (but increases test time).
> And then combination with forcing dsc with fractional bpp.
Yes, fractional bpp should be also tested and I think all the ones
supported by the source/sink or at least a bigger subset of the
supported values.
> Does this scheme for tests make sense?
Something like that, yes. This debugfs entry would be also useful during
debugging DSC issues with a particular compressed bpp.
> Do you have any other things in mind?
I think we also have to add a way to test DSC decompression by a hub's
branch device and DSC decompression by the sink connected to the hub,
the branch device just passing through the DSC stream (atm pass-through
being the default choice by the driver if both the hub and the sink
supports this).
> Regards,
> Ankit
>
> > +
> > + if (bpp_x16 &&
> > + (bpp_x16 < fxp_q4_from_int(min_bpp) ||
> > + bpp_x16 > fxp_q4_from_int(intel_display_max_pipe_bpp(display))))
> > + return -EINVAL;
> > +
> > + err = drm_modeset_lock_single_interruptible(&display->drm->mode_config.connection_mutex);
> > + if (err)
> > + return err;
> > +
> > + connector->link.force_bpp_x16 = bpp_x16;
> > +
> > + drm_modeset_unlock(&display->drm->mode_config.connection_mutex);
> > +
> > + *offp += len;
> > +
> > + return len;
> > +}
> > +DEFINE_SHOW_STORE_ATTRIBUTE(force_link_bpp);
> > +
> > +void intel_link_bw_connector_debugfs_add(struct intel_connector *connector)
> > +{
> > + struct intel_display *display = to_intel_display(connector);
> > + struct dentry *root = connector->base.debugfs_entry;
> > +
> > + switch (connector->base.connector_type) {
> > + case DRM_MODE_CONNECTOR_DisplayPort:
> > + case DRM_MODE_CONNECTOR_eDP:
> > + break;
> > + case DRM_MODE_CONNECTOR_VGA:
> > + case DRM_MODE_CONNECTOR_SVIDEO:
> > + case DRM_MODE_CONNECTOR_LVDS:
> > + case DRM_MODE_CONNECTOR_DVID:
> > + if (HAS_FDI(display))
> > + break;
> > +
> > + return;
> > + case DRM_MODE_CONNECTOR_HDMIA:
> > + if (HAS_FDI(display) && !HAS_DDI(display))
> > + break;
> > +
> > + return;
> > + default:
> > + return;
> > + }
> > +
> > + debugfs_create_file("intel_force_link_bpp", 0644, root,
> > + connector, &force_link_bpp_fops);
> > +}
> > diff --git a/drivers/gpu/drm/i915/display/intel_link_bw.h b/drivers/gpu/drm/i915/display/intel_link_bw.h
> > index e69049cf178f6..b499042e62b13 100644
> > --- a/drivers/gpu/drm/i915/display/intel_link_bw.h
> > +++ b/drivers/gpu/drm/i915/display/intel_link_bw.h
> > @@ -11,6 +11,7 @@
> > #include "intel_display_limits.h"
> > struct intel_atomic_state;
> > +struct intel_connector;
> > struct intel_crtc_state;
> > struct intel_link_bw_limits {
> > @@ -32,5 +33,6 @@ bool intel_link_bw_set_bpp_limit_for_pipe(struct intel_atomic_state *state,
> > enum pipe pipe);
> > int intel_link_bw_atomic_check(struct intel_atomic_state *state,
> > struct intel_link_bw_limits *new_limits);
> > +void intel_link_bw_connector_debugfs_add(struct intel_connector *connector);
> > #endif
More information about the Intel-xe
mailing list