From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/i915/dp: Decode DSC max delta bpp from sink DPCD
Date: Thu, 23 Apr 2026 10:06:55 +1000
Message-ID: <review-patch2-20260420112611.1481530-3-nemesa.garg@intel.com>
In-Reply-To: <20260420112611.1481530-3-nemesa.garg@intel.com>
References: <20260420112611.1481530-1-nemesa.garg@intel.com> <20260420112611.1481530-3-nemesa.garg@intel.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

This adds `intel_dp_dsc_max_delta_bppx16()` and inserts it as a new fallbac=
k level in `intel_dp_dsc_max_sink_compressed_bppx16()` between the eDP abso=
lute DPCD value and the spec-table defaults.

**Bug: Reserved 444 delta values produce wrong output.** The 444 mask (`0x1=
f`) yields a 5-bit field with range 0=E2=80=9331. The valid range check onl=
y covers 1=E2=80=9321:

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

If a sink reports a reserved value (22=E2=80=9331), the raw encoded value p=
asses through without decoding, and `max_bpp_delta << 4` returns a nonsensi=
cal bppx16 value (e.g., value 25 would give 400, meaning 25 bpp =E2=80=94 w=
hich is a wrong decode). Reserved values should return 0 (unsupported) to f=
all through to the bpc-based defaults:

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

The 420 case doesn't have this issue since the 3-bit field (0xe0 >> 5) maxe=
s at 7, and the check `>=3D 1 && <=3D 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 form=
at), 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 pr=
ecise 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 ne=
ither the RGB/444 nor 420 cases match and `output_format` is some unexpecte=
d 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) =E2=80=94 new
3. Spec table defaults based on bpc =E2=80=94 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