[PATCH] drm/msm/dsi: setup RC model properly

[PATCH] drm/msm/dsi: setup RC model properly

[Pengyu Luo]

Using incorrect parameters does not seem to affect the display, but we
should use the correct in accordance with the DSC 1.1 or 1.2.

Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 31 +++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index b60b26ddb0..276c63d2ac 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -1843,10 +1843,35 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc
 	drm_dsc_set_const_params(dsc);
 	drm_dsc_set_rc_buf_thresh(dsc);
 
-	/* DPU supports only pre-SCR panels */
-	ret = drm_dsc_setup_rc_params(dsc, DRM_DSC_1_1_PRE_SCR);
+	if (dsc->dsc_version_major != 1) {
+		DRM_DEV_ERROR(&msm_host->pdev->dev, "Unsupported DSC version: %x.%x\n",
+			      dsc->dsc_version_major, dsc->dsc_version_minor);
+		return -EOPNOTSUPP;
+	}
+
+	switch (dsc->dsc_version_minor) {
+	case 1:
+		/*
+		 * For DSC1.1. the upstream lacks SCR parameters, the downstream
+		 * parameters are unverified here, we support pre-SCR only.
+		 */
+		ret = drm_dsc_setup_rc_params(dsc, DRM_DSC_1_1_PRE_SCR);
+		break;
+	case 2:
+		if (dsc->native_422)
+			ret = drm_dsc_setup_rc_params(dsc, DRM_DSC_1_2_422);
+		else if (dsc->native_420)
+			ret = drm_dsc_setup_rc_params(dsc, DRM_DSC_1_2_420);
+		else
+			ret = drm_dsc_setup_rc_params(dsc, DRM_DSC_1_2_444);
+		break;
+	default:
+		ret = -EOPNOTSUPP;
+	}
+
 	if (ret) {
-		DRM_DEV_ERROR(&msm_host->pdev->dev, "could not find DSC RC parameters\n");
+		DRM_DEV_ERROR(&msm_host->pdev->dev, "could not find DSC RC parameters for DSC version: %x.%x\n",
+			      dsc->dsc_version_major, dsc->dsc_version_minor);
 		return ret;
 	}
 
-- 
2.53.0

Re: [PATCH] drm/msm/dsi: setup RC model properly

[Dmitry Baryshkov]

On Fri, Mar 06, 2026 at 05:25:53PM +0800, Pengyu Luo wrote:
> Using incorrect parameters does not seem to affect the display, but we
> should use the correct in accordance with the DSC 1.1 or 1.2.
> 
> Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
> ---
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 31 +++++++++++++++++++++++++++---
>  1 file changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index b60b26ddb0..276c63d2ac 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -1843,10 +1843,35 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc
>  	drm_dsc_set_const_params(dsc);
>  	drm_dsc_set_rc_buf_thresh(dsc);
>  
> -	/* DPU supports only pre-SCR panels */
> -	ret = drm_dsc_setup_rc_params(dsc, DRM_DSC_1_1_PRE_SCR);
> +	if (dsc->dsc_version_major != 1) {
> +		DRM_DEV_ERROR(&msm_host->pdev->dev, "Unsupported DSC version: %x.%x\n",
> +			      dsc->dsc_version_major, dsc->dsc_version_minor);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	switch (dsc->dsc_version_minor) {
> +	case 1:
> +		/*
> +		 * For DSC1.1. the upstream lacks SCR parameters, the downstream
> +		 * parameters are unverified here, we support pre-SCR only.

It doesn't. It's the same as DRM_DSC_1_2_444. Please send a patch adding
the comment.

> +		 */
> +		ret = drm_dsc_setup_rc_params(dsc, DRM_DSC_1_1_PRE_SCR);
> +		break;
> +	case 2:
> +		if (dsc->native_422)
> +			ret = drm_dsc_setup_rc_params(dsc, DRM_DSC_1_2_422);
> +		else if (dsc->native_420)
> +			ret = drm_dsc_setup_rc_params(dsc, DRM_DSC_1_2_420);
> +		else

It's not that we support 422/420 output... But yes, it's easier to fix
it now.

> +			ret = drm_dsc_setup_rc_params(dsc, DRM_DSC_1_2_444);
> +		break;
> +	default:
> +		ret = -EOPNOTSUPP;
> +	}
> +
>  	if (ret) {
> -		DRM_DEV_ERROR(&msm_host->pdev->dev, "could not find DSC RC parameters\n");
> +		DRM_DEV_ERROR(&msm_host->pdev->dev, "could not find DSC RC parameters for DSC version: %x.%x\n",
> +			      dsc->dsc_version_major, dsc->dsc_version_minor);
>  		return ret;
>  	}
>  
> -- 
> 2.53.0
> 

-- 
With best wishes
Dmitry

Re: [PATCH] drm/msm/dsi: setup RC model properly

[Pengyu Luo]

On Sat, Mar 7, 2026 at 2:44 AM Dmitry Baryshkov
<dmitry.baryshkov@oss.qualcomm.com> wrote:
>
> On Fri, Mar 06, 2026 at 05:25:53PM +0800, Pengyu Luo wrote:
> > Using incorrect parameters does not seem to affect the display, but we
> > should use the correct in accordance with the DSC 1.1 or 1.2.
> >
> > Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
> > ---
> >  drivers/gpu/drm/msm/dsi/dsi_host.c | 31 +++++++++++++++++++++++++++---
> >  1 file changed, 28 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > index b60b26ddb0..276c63d2ac 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > @@ -1843,10 +1843,35 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc
> >       drm_dsc_set_const_params(dsc);
> >       drm_dsc_set_rc_buf_thresh(dsc);
> >
> > -     /* DPU supports only pre-SCR panels */
> > -     ret = drm_dsc_setup_rc_params(dsc, DRM_DSC_1_1_PRE_SCR);
> > +     if (dsc->dsc_version_major != 1) {
> > +             DRM_DEV_ERROR(&msm_host->pdev->dev, "Unsupported DSC version: %x.%x\n",
> > +                           dsc->dsc_version_major, dsc->dsc_version_minor);
> > +             return -EOPNOTSUPP;
> > +     }
> > +
> > +     switch (dsc->dsc_version_minor) {
> > +     case 1:
> > +             /*
> > +              * For DSC1.1. the upstream lacks SCR parameters, the downstream
> > +              * parameters are unverified here, we support pre-SCR only.
>
> It doesn't. It's the same as DRM_DSC_1_2_444. Please send a patch adding
> the comment.
>

Indeed, I didn't remember this correctly, and I recheck it now

> > +              */
> > +             ret = drm_dsc_setup_rc_params(dsc, DRM_DSC_1_1_PRE_SCR);
> > +             break;
> > +     case 2:
> > +             if (dsc->native_422)
> > +                     ret = drm_dsc_setup_rc_params(dsc, DRM_DSC_1_2_422);
> > +             else if (dsc->native_420)
> > +                     ret = drm_dsc_setup_rc_params(dsc, DRM_DSC_1_2_420);
> > +             else
>
> It's not that we support 422/420 output... But yes, it's easier to fix
> it now.
>

Yes, this will fallback to 444 unconditionally now , since I noticed
dsc programming takes 422/420 into account, I added it here too.
But calculations don't take them into account, like we use scr_bpc * 3
in dsi_adjust_pclk_for_compression(). Should I remove them and add a
comment?

Best wishes,
Pengyu

> > +                     ret = drm_dsc_setup_rc_params(dsc, DRM_DSC_1_2_444);
> > +             break;
> > +     default:
> > +             ret = -EOPNOTSUPP;
> > +     }
> > +
> >       if (ret) {
> > -             DRM_DEV_ERROR(&msm_host->pdev->dev, "could not find DSC RC parameters\n");
> > +             DRM_DEV_ERROR(&msm_host->pdev->dev, "could not find DSC RC parameters for DSC version: %x.%x\n",
> > +                           dsc->dsc_version_major, dsc->dsc_version_minor);
> >               return ret;
> >       }
> >
> > --
> > 2.53.0
> >
>
> --
> With best wishes
> Dmitry

Claude review: drm/msm/dsi: setup RC model properly

[Claude Code Review Bot]

Overall Series Review

Subject: drm/msm/dsi: setup RC model properly
Author: Pengyu Luo <mitltlatltl@gmail.com>
Patches: 3
Reviewed: 2026-03-09T09:04:14.370369

---

This is a single patch that improves the DSC (Display Stream Compression) RC model parameter selection in the MSM DSI host driver. The current code unconditionally uses `DRM_DSC_1_1_PRE_SCR` parameters regardless of the DSC version, while this patch adds proper version-based selection to also support DSC 1.2 with 4:2:2, 4:2:0, and 4:4:4 subsampling modes.

The change is reasonable and the logic is correct. The commit message honestly notes that incorrect parameters don't visibly affect display, which is good transparency. However, there are a few issues worth raising.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/msm/dsi: setup RC model properly

[Claude Code Review Bot]

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 appropriate 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 are stored as hex-encoded BCD, `%x` is fine but the commit message should clarify.

```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 for 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 switch, `ret` is set to `-EOPNOTSUPP`, which is fine. However, the subsequent `if (ret)` block will print "could not find DSC RC parameters" which is misleading for an unsupported version — the error from the earlier `dsc_version_major != 1` check ("Unsupported DSC version") would be more appropriate here too. Consider printing a distinct message in the default case, or folding the default into the major version check.

3. **Comment accuracy**: The comment says "the upstream lacks SCR parameters" but looking at the `drm_dsc_helper.h` enum, there is no `DRM_DSC_1_1_SCR` variant — only `DRM_DSC_1_1_PRE_SCR` exists. The comment is useful context but "upstream lacks SCR parameters" is somewhat vague. It would be clearer to say something like "DSC 1.1 SCR RC parameters are not yet defined 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."` — 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 correct [parameters]" which implies the current code is buggy (albeit benignly). If this is intended as a correctness fix rather than a new feature, a `Fixes:` tag pointing to the commit that introduced the current `DRM_DSC_1_1_PRE_SCR` usage would be helpful for backporting decisions — even if the visual impact is nil.

6. **No handling for DSC 1.2 with `bits_per_component` validation**: The Intel driver (`intel_vdsc.c:345-352`) checks `compressed_bpp` and `bits_per_component` before choosing between `DRM_DSC_1_1_PRE_SCR` and `DRM_DSC_1_2_444`. This patch doesn't validate those constraints for DSC 1.2. It's worth considering whether certain `bits_per_component` values might not have RC parameter tables defined, which would cause `drm_dsc_setup_rc_params()` to fail 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 are the double-period typo and considering whether the `%x` format specifier is intentional.

---
Generated by Claude Code Patch Reviewer