From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/msm/dsi: setup RC model properly Date: Mon, 09 Mar 2026 09:04:14 +1000 Message-ID: In-Reply-To: <20260306092553.37973-1-mitltlatltl@gmail.com> References: <20260306092553.37973-1-mitltlatltl@gmail.com> <20260306092553.37973-1-mitltlatltl@gmail.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 **Positive aspects:** - The version-based dispatch logic is sound and mirrors how other drivers (= e.g., `intel_vdsc.c`) differentiate between DSC parameter sets. - Good use of the existing `drm_dsc_setup_rc_params()` API with the appropr= iate enum values. - The error messages are improved with version information for debugging. **Issues:** 1. **Commit message format**: The version format specifier `%x.%x` implies = hex formatting, but the subject says "DSC 1.1 or 1.2" using decimal. If the= version fields are actually decimal values (1, 1) and (1, 2), then `%d.%d`= or `%u.%u` would be more appropriate and less confusing. If they really ar= e stored as hex-encoded BCD, `%x` is fine but the commit message should cla= rify. ```c + DRM_DEV_ERROR(&msm_host->pdev->dev, "Unsupported DSC version: %x.%x\n", + dsc->dsc_version_major, dsc->dsc_version_minor); ``` and: ```c + DRM_DEV_ERROR(&msm_host->pdev->dev, "could not find DSC RC parameters fo= r DSC version: %x.%x\n", + dsc->dsc_version_major, dsc->dsc_version_minor); ``` 2. **Uninitialized `ret` on default path**: In the `default` case of the sw= itch, `ret` is set to `-EOPNOTSUPP`, which is fine. However, the subsequent= `if (ret)` block will print "could not find DSC RC parameters" which is mi= sleading for an unsupported version =E2=80=94 the error from the earlier `d= sc_version_major !=3D 1` check ("Unsupported DSC version") would be more ap= propriate here too. Consider printing a distinct message in the default cas= e, or folding the default into the major version check. 3. **Comment accuracy**: The comment says "the upstream lacks SCR parameter= s" but looking at the `drm_dsc_helper.h` enum, there is no `DRM_DSC_1_1_SCR= ` variant =E2=80=94 only `DRM_DSC_1_1_PRE_SCR` exists. The comment is usefu= l context but "upstream lacks SCR parameters" is somewhat vague. It would b= e clearer to say something like "DSC 1.1 SCR RC parameters are not yet defi= ned in drm_dsc_helper". ```c + /* + * For DSC1.1. the upstream lacks SCR parameters, the downstream + * parameters are unverified here, we support pre-SCR only. + */ ``` 4. **Minor**: There's a double period in the comment: `"For DSC1.1."` =E2= =80=94 should be `"For DSC 1.1,"` (also add a space between "DSC" and "1.1"= ). 5. **Missing Fixes tag?**: The commit message says "we should use the corre= ct [parameters]" which implies the current code is buggy (albeit benignly).= If this is intended as a correctness fix rather than a new feature, a `Fix= es:` tag pointing to the commit that introduced the current `DRM_DSC_1_1_PR= E_SCR` usage would be helpful for backporting decisions =E2=80=94 even if t= he visual impact is nil. 6. **No handling for DSC 1.2 with `bits_per_component` validation**: The In= tel driver (`intel_vdsc.c:345-352`) checks `compressed_bpp` and `bits_per_c= omponent` before choosing between `DRM_DSC_1_1_PRE_SCR` and `DRM_DSC_1_2_44= 4`. This patch doesn't validate those constraints for DSC 1.2. It's worth c= onsidering whether certain `bits_per_component` values might not have RC pa= rameter tables defined, which would cause `drm_dsc_setup_rc_params()` to fa= il silently or return an error. The existing error path would catch it, but= it might be worth a comment noting that the helper validates internally. Overall this is a clean, low-risk improvement. The main actionable items ar= e the double-period typo and considering whether the `%x` format specifier = is intentional. --- Generated by Claude Code Patch Reviewer