[PATCH v2 11/12] drm/i915: Add support for forcing the link bpp on a connector
Nautiyal, Ankit K
ankit.k.nautiyal at intel.com
Wed May 7 12:41:25 UTC 2025
On 5/7/2025 5:33 PM, Imre Deak wrote:
> 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).
Ah ok.
>
>>> +
>>> + 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).
Thanks for the pointers. Lets have this support in, and then we can
start working on IGT subtests.
Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
>
>> 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-gfx
mailing list