[PATCH] drm/i915/display: Replace bw_constrained with max_link_bpp_x16

[PATCH] drm/i915/display: Replace bw_constrained with max_link_bpp_x16

[George Karagounis]

In intel_crt.c (VGA displays) and intel_lvds.c (old laptop displays),

both files were using a legacy boolean flag crtc_state->bw_constrained

to guess if the display link was bandwidth limited.

To fix this, replaced the bw_constrained boolean checks with direct

comparisons against the max_link_bpp_x16 fixed-point API.

Signed-off-by: George Karagounis <mail@taterr.org>
---
 drivers/gpu/drm/i915/display/intel_crt.c  | 5 ++---
 drivers/gpu/drm/i915/display/intel_lvds.c | 6 +++---
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_crt.c b/drivers/gpu/drm/i915/display/intel_crt.c
index 5b8968197fbc..3da9547f2c6e 100644
--- a/drivers/gpu/drm/i915/display/intel_crt.c
+++ b/drivers/gpu/drm/i915/display/intel_crt.c
@@ -459,10 +459,9 @@ static int hsw_crt_compute_config(struct intel_atomic_state *state,
 
 	/* LPT FDI RX only supports 8bpc. */
 	if (HAS_PCH_LPT(display)) {
-		/* TODO: Check crtc_state->max_link_bpp_x16 instead of bw_constrained */
-		if (crtc_state->bw_constrained && crtc_state->pipe_bpp < 24) {
+		if (crtc_state->max_link_bpp_x16 < fxp_q4_from_int(24)) {
 			drm_dbg_kms(display->drm,
-				    "LPT only supports 24bpp\n");
+				   "LPT only supports 24bpp\n");
 			return -EINVAL;
 		}
 
diff --git a/drivers/gpu/drm/i915/display/intel_lvds.c b/drivers/gpu/drm/i915/display/intel_lvds.c
index 872753478cf2..6bbd13ab5da7 100644
--- a/drivers/gpu/drm/i915/display/intel_lvds.c
+++ b/drivers/gpu/drm/i915/display/intel_lvds.c
@@ -443,10 +443,10 @@ static int intel_lvds_compute_config(struct intel_atomic_state *state,
 	else
 		lvds_bpp = 6*3;
 
-	/* TODO: Check crtc_state->max_link_bpp_x16 instead of bw_constrained */
-	if (lvds_bpp != crtc_state->pipe_bpp && !crtc_state->bw_constrained) {
+	if (lvds_bpp != crtc_state->pipe_bpp &&
+	    crtc_state->max_link_bpp_x16 >= fxp_q4_from_int(lvds_bpp)) {
 		drm_dbg_kms(display->drm,
-			    "forcing display bpp (was %d) to LVDS (%d)\n",
+			   "forcing display bpp (was %d) to LVDS (%d)\n",
 			    crtc_state->pipe_bpp, lvds_bpp);
 		crtc_state->pipe_bpp = lvds_bpp;
 	}
-- 
2.55.0

Re: [PATCH] drm/i915/display: Replace bw_constrained with max_link_bpp_x16

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] drm/i915/display: incomplete cleanup of bw_constrained flag
--

commit f4c7087cb31d33cb453437886f7788a5ffe134a2
Author: George Karagounis <taterraster@gmail.com>

drm/i915/display: Replace bw_constrained with max_link_bpp_x16

This commit replaces the legacy bw_constrained boolean flag checks with
direct comparisons against the max_link_bpp_x16 fixed-point API in both
intel_crt.c and intel_lvds.c.

> diff --git a/drivers/gpu/drm/i915/display/intel_lvds.c b/drivers/gpu/drm/i915/display/intel_lvds.c
> index 872753478cf21..6bbd13ab5da7b 100644
> --- a/drivers/gpu/drm/i915/display/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/display/intel_lvds.c
[ ... ]
> @@ -443,10 +443,10 @@ static int intel_lvds_compute_config(struct intel_atomic_state *state,
>  	else
>  		lvds_bpp = 6*3;
>  
> -	/* TODO: Check crtc_state->max_link_bpp_x16 instead of bw_constrained */
> -	if (lvds_bpp != crtc_state->pipe_bpp && !crtc_state->bw_constrained) {
> +	if (lvds_bpp != crtc_state->pipe_bpp &&
> +	    crtc_state->max_link_bpp_x16 >= fxp_q4_from_int(lvds_bpp)) {

[Severity: Low]
Does this commit intentionally leave the bw_constrained flag definition and
other writes intact?

While this patch removes the readers of bw_constrained, the flag remains
defined in struct intel_crtc_state:

drivers/gpu/drm/i915/display/intel_display_types.h:struct intel_crtc_state {
...
	bool bw_constrained;
...
}

It also still appears to be stored whenever crtc_state->pipe_bpp > 
fxp_q4_to_int(crtc_state->max_link_bpp_x16) during mode setting:

drivers/gpu/drm/i915/display/intel_display.c:intel_modeset_pipe_config() {
...
	crtc_state->bw_constrained = true;
...
}

Because the readers were removed, this causes unnecessary code complexity and
leaves behind a dead store. Is there a plan to remove the flag entirely, or
was this an oversight?

>  		drm_dbg_kms(display->drm,
> -			    "forcing display bpp (was %d) to LVDS (%d)\n",
> +			   "forcing display bpp (was %d) to LVDS (%d)\n",
>  			    crtc_state->pipe_bpp, lvds_bpp);
>  		crtc_state->pipe_bpp = lvds_bpp;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260919161927.98829-1-mail@taterr.org?part=1

✗ LGCI.VerificationFailed: failure for drm/i915/display: Replace bw_constrained with max_link_bpp_x16

[Patchwork]

== Series Details ==

Series: drm/i915/display: Replace bw_constrained with max_link_bpp_x16
URL   : https://patchwork.freedesktop.org/series/174626/
State : failure

== Summary ==

Series author address 'taterraster@gmail.com' is not on the allowlist, which prevents CI from being automatically triggered.
If you want CI to run for this series, ask Patchwork project owners to click 'retest' on the series in Patchwork.
Exception occurred during validation, bailing out!
Build URL: http://gfx-ci.igk.intel.com:8080/job/CI_PW_kernel/188273/ (on built-in)

[PATCH v2] drm/i915/display: Replace bw_constrained with max_link_bpp_x16

[George Karagounis]

In intel_crt.c (VGA displays) and intel_lvds.c (old laptop displays),

both files were using a legacy boolean flag crtc_state->bw_constrained

to guess if the display link was bandwidth limited.

To fix this, replaced the bw_constrained boolean checks with direct

comparisons against the max_link_bpp_x16 fixed-point API.

Signed-off-by: George Karagounis <mail@taterr.org>
---
v2: Remove the dead bw_constrained flag definition and writer entirely,
    as pointed out by the sashiko bot reviewer
 drivers/gpu/drm/i915/display/intel_crt.c           | 5 ++---
 drivers/gpu/drm/i915/display/intel_display.c       | 1 -
 drivers/gpu/drm/i915/display/intel_display_types.h | 1 -
 drivers/gpu/drm/i915/display/intel_lvds.c          | 6 +++---
 4 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_crt.c b/drivers/gpu/drm/i915/display/intel_crt.c
index 5b8968197fbc..3da9547f2c6e 100644
--- a/drivers/gpu/drm/i915/display/intel_crt.c
+++ b/drivers/gpu/drm/i915/display/intel_crt.c
@@ -459,10 +459,9 @@ static int hsw_crt_compute_config(struct intel_atomic_state *state,
 
 	/* LPT FDI RX only supports 8bpc. */
 	if (HAS_PCH_LPT(display)) {
-		/* TODO: Check crtc_state->max_link_bpp_x16 instead of bw_constrained */
-		if (crtc_state->bw_constrained && crtc_state->pipe_bpp < 24) {
+		if (crtc_state->max_link_bpp_x16 < fxp_q4_from_int(24)) {
 			drm_dbg_kms(display->drm,
-				    "LPT only supports 24bpp\n");
+				   "LPT only supports 24bpp\n");
 			return -EINVAL;
 		}
 
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 9dfd095f325f..fa6b11c4df47 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -4767,7 +4767,6 @@ intel_modeset_pipe_config(struct intel_atomic_state *state,
 			    "[CRTC:%d:%s] Link bpp limited to " FXP_Q4_FMT "\n",
 			    crtc->base.base.id, crtc->base.name,
 			    FXP_Q4_ARGS(crtc_state->max_link_bpp_x16));
-		crtc_state->bw_constrained = true;
 	}
 
 	base_bpp = crtc_state->pipe_bpp;
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index 43d53a98dae7..2166c84596c6 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -1146,7 +1146,6 @@ struct intel_crtc_state {
 	 * required. This is set in the 2nd loop of calling encoder's
 	 * ->compute_config if the first pick doesn't work out.
 	 */
-	bool bw_constrained;
 
 	/* Settings for the intel dpll used on pretty much everything but
 	 * haswell. */
diff --git a/drivers/gpu/drm/i915/display/intel_lvds.c b/drivers/gpu/drm/i915/display/intel_lvds.c
index 872753478cf2..6bbd13ab5da7 100644
--- a/drivers/gpu/drm/i915/display/intel_lvds.c
+++ b/drivers/gpu/drm/i915/display/intel_lvds.c
@@ -443,10 +443,10 @@ static int intel_lvds_compute_config(struct intel_atomic_state *state,
 	else
 		lvds_bpp = 6*3;
 
-	/* TODO: Check crtc_state->max_link_bpp_x16 instead of bw_constrained */
-	if (lvds_bpp != crtc_state->pipe_bpp && !crtc_state->bw_constrained) {
+	if (lvds_bpp != crtc_state->pipe_bpp &&
+	    crtc_state->max_link_bpp_x16 >= fxp_q4_from_int(lvds_bpp)) {
 		drm_dbg_kms(display->drm,
-			    "forcing display bpp (was %d) to LVDS (%d)\n",
+			   "forcing display bpp (was %d) to LVDS (%d)\n",
 			    crtc_state->pipe_bpp, lvds_bpp);
 		crtc_state->pipe_bpp = lvds_bpp;
 	}
-- 
2.55.0

✗ LGCI.VerificationFailed: failure for drm/i915/display: Replace bw_constrained with max_link_bpp_x16 (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/display: Replace bw_constrained with max_link_bpp_x16 (rev2)
URL   : https://patchwork.freedesktop.org/series/174626/
State : failure

== Summary ==

Series author address 'taterraster@gmail.com' is not on the allowlist, which prevents CI from being automatically triggered.
If you want CI to run for this series, ask Patchwork project owners to click 'retest' on the series in Patchwork.
Exception occurred during validation, bailing out!
Build URL: http://gfx-ci.igk.intel.com:8080/job/CI_PW_kernel/188505/ (on built-in)