public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH 0/2]  DSC max delta bpp support
@ 2026-04-20 11:26 Nemesa Garg
  2026-04-20 11:26 ` [PATCH 1/2] drm/dp: Define DSC bpp delta DPCD fields Nemesa Garg
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Nemesa Garg @ 2026-04-20 11:26 UTC (permalink / raw)
  To: intel-gfx, intel-xe, dri-devel; +Cc: Nemesa Garg

    Some sinks exposes DSC max bpp through delta based
DPCD fields. To support those sinks, add DP DPCD field
field and logic to decode the delta value in bppx16
format.


Nemesa Garg (2):
  drm/dp: Define DSC bpp delta DPCD fields
  drm/i915/dp: Decode DSC max delta bpp from sink DPCD

 drivers/gpu/drm/i915/display/intel_dp.c | 40 +++++++++++++++++++++++++
 include/drm/display/drm_dp.h            |  8 ++++-
 2 files changed, 47 insertions(+), 1 deletion(-)

-- 
2.25.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] drm/dp: Define DSC bpp delta DPCD fields
  2026-04-20 11:26 [PATCH 0/2] DSC max delta bpp support Nemesa Garg
@ 2026-04-20 11:26 ` Nemesa Garg
  2026-04-21  6:34   ` Nautiyal, Ankit K
  2026-04-23  0:06   ` Claude review: " Claude Code Review Bot
  2026-04-20 11:26 ` [PATCH 2/2] drm/i915/dp: Decode DSC max delta bpp from sink DPCD Nemesa Garg
  2026-04-23  0:06 ` Claude review: DSC max delta bpp support Claude Code Review Bot
  2 siblings, 2 replies; 8+ messages in thread
From: Nemesa Garg @ 2026-04-20 11:26 UTC (permalink / raw)
  To: intel-gfx, intel-xe, dri-devel; +Cc: Nemesa Garg

Add DP_DSC_MAX_BPP_DELTA and related field and mask helper
macros for RGB/YCbCr444 delta and YCbCr420 delta encoding.

Signed-off-by: Nemesa Garg <nemesa.garg@intel.com>
---
 include/drm/display/drm_dp.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/drm/display/drm_dp.h b/include/drm/display/drm_dp.h
index 8b15d3eeb716..2afd33d90a0e 100644
--- a/include/drm/display/drm_dp.h
+++ b/include/drm/display/drm_dp.h
@@ -354,9 +354,15 @@
 # define DP_DSC_20_PER_DP_DSC_SINK          (1 << 1)
 # define DP_DSC_24_PER_DP_DSC_SINK          (1 << 2)
 
-#define DP_DSC_BITS_PER_PIXEL_INC           0x06F
+#define MIN_DSC_BPP_DELTA_444 16
+#define MIN_DSC_BPP_DELTA_420 12
+#define BPP_DELTA_SHIFT_420 5
+
+#define DP_DSC_MAX_BPP_DELTA			0x06E
 # define DP_DSC_RGB_YCbCr444_MAX_BPP_DELTA_MASK 0x1f
 # define DP_DSC_RGB_YCbCr420_MAX_BPP_DELTA_MASK 0xe0
+
+#define DP_DSC_BITS_PER_PIXEL_INC           0x06F
 # define DP_DSC_BITS_PER_PIXEL_1_16         0x0
 # define DP_DSC_BITS_PER_PIXEL_1_8          0x1
 # define DP_DSC_BITS_PER_PIXEL_1_4          0x2
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/2] drm/i915/dp: Decode DSC max delta bpp from sink DPCD
  2026-04-20 11:26 [PATCH 0/2] DSC max delta bpp support Nemesa Garg
  2026-04-20 11:26 ` [PATCH 1/2] drm/dp: Define DSC bpp delta DPCD fields Nemesa Garg
@ 2026-04-20 11:26 ` Nemesa Garg
  2026-04-21 10:50   ` Nautiyal, Ankit K
  2026-04-23  0:06   ` Claude review: " Claude Code Review Bot
  2026-04-23  0:06 ` Claude review: DSC max delta bpp support Claude Code Review Bot
  2 siblings, 2 replies; 8+ messages in thread
From: Nemesa Garg @ 2026-04-20 11:26 UTC (permalink / raw)
  To: intel-gfx, intel-xe, dri-devel; +Cc: Nemesa Garg

Add intel_dp_dsc_max_delta_bppx16() to parse sink DSC max
delta bpp from DPCD when DP_DSC_MAX_BPP_DELTA_AVAILABILITY
is set. The helper decodes RGB/YCbCr444 delta range and
YCbCr420 delta range from DP_DSC_MAX_BPP_DELTA.

Signed-off-by: Nemesa Garg <nemesa.garg@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 40 +++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 35b8fb5740aa..7cc760aedd59 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -2169,6 +2169,41 @@ static int dsc_compute_link_config(struct intel_dp *intel_dp,
 	return -EINVAL;
 }
 
+static u16 intel_dp_dsc_max_delta_bppx16(const struct intel_connector *connector,
+					 enum intel_output_format output_format)
+{
+	const u8 *dsc_dpcd = connector->dp.dsc_dpcd;
+
+	if (dsc_dpcd[DP_DSC_MAX_BITS_PER_PIXEL_HI - DP_DSC_SUPPORT] &
+	    DP_DSC_MAX_BPP_DELTA_AVAILABILITY) {
+		int max_bpp_delta = 0;
+
+		switch (output_format) {
+		case INTEL_OUTPUT_FORMAT_RGB:
+		case INTEL_OUTPUT_FORMAT_YCBCR444:
+			max_bpp_delta = dsc_dpcd[DP_DSC_MAX_BPP_DELTA - DP_DSC_SUPPORT] &
+				DP_DSC_RGB_YCbCr444_MAX_BPP_DELTA_MASK;
+			if (max_bpp_delta >= 1 && max_bpp_delta <= 21)
+				max_bpp_delta =  max_bpp_delta + MIN_DSC_BPP_DELTA_444 - 1;
+			break;
+		case INTEL_OUTPUT_FORMAT_YCBCR420:
+			max_bpp_delta = (dsc_dpcd[DP_DSC_MAX_BPP_DELTA - DP_DSC_SUPPORT] &
+					DP_DSC_RGB_YCbCr420_MAX_BPP_DELTA_MASK) >>
+					BPP_DELTA_SHIFT_420;
+			if (max_bpp_delta >= 1 && max_bpp_delta <= 7)
+				max_bpp_delta = max_bpp_delta + MIN_DSC_BPP_DELTA_420 - 1;
+			break;
+		default:
+			MISSING_CASE(output_format);
+			return 0;
+		}
+
+		return max_bpp_delta << 4;
+	}
+
+	return 0;
+}
+
 static
 u16 intel_dp_dsc_max_sink_compressed_bppx16(const struct intel_connector *connector,
 					    enum intel_output_format output_format,
@@ -2176,6 +2211,11 @@ u16 intel_dp_dsc_max_sink_compressed_bppx16(const struct intel_connector *connec
 {
 	u16 max_bppx16 = drm_edp_dsc_sink_output_bpp(connector->dp.dsc_dpcd);
 
+	if (max_bppx16)
+		return max_bppx16;
+
+	max_bppx16 = intel_dp_dsc_max_delta_bppx16(connector, output_format);
+
 	if (max_bppx16)
 		return max_bppx16;
 	/*
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] drm/dp: Define DSC bpp delta DPCD fields
  2026-04-20 11:26 ` [PATCH 1/2] drm/dp: Define DSC bpp delta DPCD fields Nemesa Garg
@ 2026-04-21  6:34   ` Nautiyal, Ankit K
  2026-04-23  0:06   ` Claude review: " Claude Code Review Bot
  1 sibling, 0 replies; 8+ messages in thread
From: Nautiyal, Ankit K @ 2026-04-21  6:34 UTC (permalink / raw)
  To: Nemesa Garg, intel-gfx, intel-xe, dri-devel


On 4/20/2026 4:56 PM, Nemesa Garg wrote:
> Add DP_DSC_MAX_BPP_DELTA and related field and mask helper
> macros for RGB/YCbCr444 delta and YCbCr420 delta encoding.
>
> Signed-off-by: Nemesa Garg <nemesa.garg@intel.com>
> ---
>   include/drm/display/drm_dp.h | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/include/drm/display/drm_dp.h b/include/drm/display/drm_dp.h
> index 8b15d3eeb716..2afd33d90a0e 100644
> --- a/include/drm/display/drm_dp.h
> +++ b/include/drm/display/drm_dp.h
> @@ -354,9 +354,15 @@
>   # define DP_DSC_20_PER_DP_DSC_SINK          (1 << 1)
>   # define DP_DSC_24_PER_DP_DSC_SINK          (1 << 2)
>   
> -#define DP_DSC_BITS_PER_PIXEL_INC           0x06F
> +#define MIN_DSC_BPP_DELTA_444 16
> +#define MIN_DSC_BPP_DELTA_420 12

I understand, you are not using 422 format, but for the sake of 
completeness, lets add the values and mask for 422 formats also.


> +#define BPP_DELTA_SHIFT_420 5


These belong to the 0x06E group.


> +
> +#define DP_DSC_MAX_BPP_DELTA			0x06E


To match with the DP spec name add VERSION_1 suffix.

There might be support for Version 2 in future, (atleast the DPCD 0x68 
MAX_BPP_DELTA_VERSION points to that).


>   # define DP_DSC_RGB_YCbCr444_MAX_BPP_DELTA_MASK 0x1f
>   # define DP_DSC_RGB_YCbCr420_MAX_BPP_DELTA_MASK 0xe0


These bits were wrongly grouped with DPCD 0x06F. This correction should 
be a separate patch.


Regards,

Ankit

> +
> +#define DP_DSC_BITS_PER_PIXEL_INC           0x06F
>   # define DP_DSC_BITS_PER_PIXEL_1_16         0x0
>   # define DP_DSC_BITS_PER_PIXEL_1_8          0x1
>   # define DP_DSC_BITS_PER_PIXEL_1_4          0x2

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] drm/i915/dp: Decode DSC max delta bpp from sink DPCD
  2026-04-20 11:26 ` [PATCH 2/2] drm/i915/dp: Decode DSC max delta bpp from sink DPCD Nemesa Garg
@ 2026-04-21 10:50   ` Nautiyal, Ankit K
  2026-04-23  0:06   ` Claude review: " Claude Code Review Bot
  1 sibling, 0 replies; 8+ messages in thread
From: Nautiyal, Ankit K @ 2026-04-21 10:50 UTC (permalink / raw)
  To: Nemesa Garg, intel-gfx, intel-xe, dri-devel


On 4/20/2026 4:56 PM, Nemesa Garg wrote:
> Add intel_dp_dsc_max_delta_bppx16() to parse sink DSC max
> delta bpp from DPCD when DP_DSC_MAX_BPP_DELTA_AVAILABILITY
> is set. The helper decodes RGB/YCbCr444 delta range and
> YCbCr420 delta range from DP_DSC_MAX_BPP_DELTA.


Looking from the spec, i think we need to check this support first, so 
overall to get the maximum compressed bpp supported for an output format:

Step 1: check if format specific range is given, if given use that for 
getting the max compressed bpp based on the output format. (DPCD 0x6E-6F)

Step 2: if above is not present, check for the maximum compressed bpp 
supported by sink and use that. (DPCD 0x67-68)

Step 3: if both of the above are not there, then go with the mandatory 
supported range given in the Table 2-157.


>
> Signed-off-by: Nemesa Garg <nemesa.garg@intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_dp.c | 40 +++++++++++++++++++++++++
>   1 file changed, 40 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 35b8fb5740aa..7cc760aedd59 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -2169,6 +2169,41 @@ static int dsc_compute_link_config(struct intel_dp *intel_dp,
>   	return -EINVAL;
>   }
>   
> +static u16 intel_dp_dsc_max_delta_bppx16(const struct intel_connector *connector,
> +					 enum intel_output_format output_format)
> +{
> +	const u8 *dsc_dpcd = connector->dp.dsc_dpcd;
> +
> +	if (dsc_dpcd[DP_DSC_MAX_BITS_PER_PIXEL_HI - DP_DSC_SUPPORT] &
> +	    DP_DSC_MAX_BPP_DELTA_AVAILABILITY) {


I think we can return early from here.


> +		int max_bpp_delta = 0;

Perhaps just use max_bpp.


> +
> +		switch (output_format) {
> +		case INTEL_OUTPUT_FORMAT_RGB:
> +		case INTEL_OUTPUT_FORMAT_YCBCR444:
> +			max_bpp_delta = dsc_dpcd[DP_DSC_MAX_BPP_DELTA - DP_DSC_SUPPORT] &

This can be kept in a variable to avoid computing samething again:

u8 max_bpp_delta_v1 = dsc_dpcd[DP_DSC_MAX_BPP_DELTA  - DP_DSC_SUPPORT];


> +				DP_DSC_RGB_YCbCr444_MAX_BPP_DELTA_MASK;
> +			if (max_bpp_delta >= 1 && max_bpp_delta <= 21)
> +				max_bpp_delta =  max_bpp_delta + MIN_DSC_BPP_DELTA_444 - 1;
> +			break;
> +		case INTEL_OUTPUT_FORMAT_YCBCR420:
> +			max_bpp_delta = (dsc_dpcd[DP_DSC_MAX_BPP_DELTA - DP_DSC_SUPPORT] &
> +					DP_DSC_RGB_YCbCr420_MAX_BPP_DELTA_MASK) >>
> +					BPP_DELTA_SHIFT_420;
> +			if (max_bpp_delta >= 1 && max_bpp_delta <= 7)
> +				max_bpp_delta = max_bpp_delta + MIN_DSC_BPP_DELTA_420 - 1;
> +			break;
> +		default:
> +			MISSING_CASE(output_format);
> +			return 0;
> +		}
> +
> +		return max_bpp_delta << 4;
> +	}
> +
> +	return 0;
> +}
> +
>   static
>   u16 intel_dp_dsc_max_sink_compressed_bppx16(const struct intel_connector *connector,
>   					    enum intel_output_format output_format,
> @@ -2176,6 +2211,11 @@ u16 intel_dp_dsc_max_sink_compressed_bppx16(const struct intel_connector *connec
>   {
>   	u16 max_bppx16 = drm_edp_dsc_sink_output_bpp(connector->dp.dsc_dpcd);
>   
> +	if (max_bppx16)
> +		return max_bppx16;
> +
> +	max_bppx16 = intel_dp_dsc_max_delta_bppx16(connector, output_format);


As mentioned, read this first, then fallback to the 
drm_edp_dsc_sink_output_bpp() and at last fall back to the table.

Also need to update the comment below.


Regards,

Ankit

> +
>   	if (max_bppx16)
>   		return max_bppx16;
>   	/*

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Claude review: DSC max delta bpp support
  2026-04-20 11:26 [PATCH 0/2] DSC max delta bpp support Nemesa Garg
  2026-04-20 11:26 ` [PATCH 1/2] drm/dp: Define DSC bpp delta DPCD fields Nemesa Garg
  2026-04-20 11:26 ` [PATCH 2/2] drm/i915/dp: Decode DSC max delta bpp from sink DPCD Nemesa Garg
@ 2026-04-23  0:06 ` Claude Code Review Bot
  2 siblings, 0 replies; 8+ messages in thread
From: Claude Code Review Bot @ 2026-04-23  0:06 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: DSC max delta bpp support
Author: Nemesa Garg <nemesa.garg@intel.com>
Patches: 5
Reviewed: 2026-04-23T10:06:54.839347

---

This 2-patch series adds support for decoding DSC maximum bpp from the delta-based DPCD fields (register 0x06E) defined in eDP 1.5 / DP 2.0. The goal is reasonable — some sinks only expose max bpp via the delta encoding rather than the absolute DPCD 0x067/0x068 fields.

However, the series has a **correctness bug** (reserved 444 delta values produce garbage output), **naming convention violations** in the shared DRM header, and questionable placement of driver-specific decode constants in the common `drm_dp.h`. The series needs a respin.

**Key issues:**
- Reserved delta values (22–31) for RGB/YCbCr444 are silently passed through without decoding, producing wrong max bpp values.
- New macro names in `drm_dp.h` break the `DP_DSC_` naming convention used throughout the file.
- Driver-specific decode constants don't belong in the shared DPCD header.

---

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Claude review: drm/dp: Define DSC bpp delta DPCD fields
  2026-04-20 11:26 ` [PATCH 1/2] drm/dp: Define DSC bpp delta DPCD fields Nemesa Garg
  2026-04-21  6:34   ` Nautiyal, Ankit K
@ 2026-04-23  0:06   ` Claude Code Review Bot
  1 sibling, 0 replies; 8+ messages in thread
From: Claude Code Review Bot @ 2026-04-23  0:06 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

This patch moves the existing `DP_DSC_RGB_YCbCr444_MAX_BPP_DELTA_MASK` and `DP_DSC_RGB_YCbCr420_MAX_BPP_DELTA_MASK` definitions from under `DP_DSC_BITS_PER_PIXEL_INC (0x06F)` to the correct register `DP_DSC_MAX_BPP_DELTA (0x06E)`, and adds three new constants. The register reassociation is correct per the DP spec, but the new constants have problems.

**Naming convention violation.** All DPCD field definitions in this header use the `DP_DSC_` prefix. The three new defines break this convention:

```c
+#define MIN_DSC_BPP_DELTA_444 16
+#define MIN_DSC_BPP_DELTA_420 12
+#define BPP_DELTA_SHIFT_420 5
```

These should be `DP_DSC_MIN_BPP_DELTA_444`, `DP_DSC_MIN_BPP_DELTA_420`, and `DP_DSC_BPP_DELTA_SHIFT_420` at minimum. The unprefixed names risk collisions in the global namespace.

**Placement concern.** `MIN_DSC_BPP_DELTA_444`, `MIN_DSC_BPP_DELTA_420`, and `BPP_DELTA_SHIFT_420` are not DPCD register or field definitions — they're decode helper constants used only by the i915 driver in patch 2. They don't belong in `include/drm/display/drm_dp.h`, which is a shared header for DPCD register definitions. Either move them to the i915 driver code, or if they are truly spec-mandated values, add a comment referencing the relevant spec section/table.

**`BPP_DELTA_SHIFT_420` is redundant.** The shift value 5 is trivially derivable from `DP_DSC_RGB_YCbCr420_MAX_BPP_DELTA_MASK (0xe0)` using `__ffs()` or by simply writing `>> 5` at the call site. If you do keep it, the name should reference it's a shift for the mask, e.g., `DP_DSC_RGB_YCbCr420_MAX_BPP_DELTA_SHIFT`.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Claude review: drm/i915/dp: Decode DSC max delta bpp from sink DPCD
  2026-04-20 11:26 ` [PATCH 2/2] drm/i915/dp: Decode DSC max delta bpp from sink DPCD Nemesa Garg
  2026-04-21 10:50   ` Nautiyal, Ankit K
@ 2026-04-23  0:06   ` Claude Code Review Bot
  1 sibling, 0 replies; 8+ messages in thread
From: Claude Code Review Bot @ 2026-04-23  0:06 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

This adds `intel_dp_dsc_max_delta_bppx16()` and inserts it as a new fallback level in `intel_dp_dsc_max_sink_compressed_bppx16()` between the eDP absolute DPCD value and the spec-table defaults.

**Bug: Reserved 444 delta values produce wrong output.** The 444 mask (`0x1f`) yields a 5-bit field with range 0–31. The valid range check only covers 1–21:

```c
max_bpp_delta = dsc_dpcd[DP_DSC_MAX_BPP_DELTA - DP_DSC_SUPPORT] &
    DP_DSC_RGB_YCbCr444_MAX_BPP_DELTA_MASK;
if (max_bpp_delta >= 1 && max_bpp_delta <= 21)
    max_bpp_delta =  max_bpp_delta + MIN_DSC_BPP_DELTA_444 - 1;
```

If a sink reports a reserved value (22–31), the raw encoded value passes through without decoding, and `max_bpp_delta << 4` returns a nonsensical bppx16 value (e.g., value 25 would give 400, meaning 25 bpp — which is a wrong decode). Reserved values should return 0 (unsupported) to fall through to the bpc-based defaults:

```c
if (max_bpp_delta >= 1 && max_bpp_delta <= 21)
    max_bpp_delta = max_bpp_delta + MIN_DSC_BPP_DELTA_444 - 1;
else
    max_bpp_delta = 0;
```

The 420 case doesn't have this issue since the 3-bit field (0xe0 >> 5) maxes at 7, and the check `>= 1 && <= 7` covers all non-zero values.

**Misleading function name.** `intel_dp_dsc_max_delta_bppx16` says "delta" but the function returns the *absolute* maximum compressed bpp (in x16 format), not a delta. The delta is an encoding detail internal to the function. Consider `intel_dp_dsc_sink_max_bppx16_from_delta` or similar.

**Variable type.** `max_bpp_delta` is declared as `int` but only holds non-negative values derived from masked byte reads. Using `u8` would be more precise and self-documenting.

**Missing `else` for default case value.** When the `default:` case in the switch is hit, the function does `MISSING_CASE()` and `return 0`, but if neither the RGB/444 nor 420 cases match and `output_format` is some unexpected value, the flow is correct. This is fine.

**Integration logic is correct.** The insertion into `intel_dp_dsc_max_sink_compressed_bppx16` creates a clean three-tier fallback:
1. eDP absolute DPCD (0x067/0x068)
2. Delta-based DPCD (0x06E) — new
3. Spec table defaults based on bpc — existing

This ordering is logical. The duplicated `if (max_bppx16) return max_bppx16;` pattern is slightly repetitive but functionally correct.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2026-04-23  0:06 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-20 11:26 [PATCH 0/2] DSC max delta bpp support Nemesa Garg
2026-04-20 11:26 ` [PATCH 1/2] drm/dp: Define DSC bpp delta DPCD fields Nemesa Garg
2026-04-21  6:34   ` Nautiyal, Ankit K
2026-04-23  0:06   ` Claude review: " Claude Code Review Bot
2026-04-20 11:26 ` [PATCH 2/2] drm/i915/dp: Decode DSC max delta bpp from sink DPCD Nemesa Garg
2026-04-21 10:50   ` Nautiyal, Ankit K
2026-04-23  0:06   ` Claude review: " Claude Code Review Bot
2026-04-23  0:06 ` Claude review: DSC max delta bpp support Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox