public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] drm/msm/dsi: remove bpc > 8 entries from allow list
@ 2026-03-06  9:25 Pengyu Luo
  2026-03-06 18:47 ` Dmitry Baryshkov
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Pengyu Luo @ 2026-03-06  9:25 UTC (permalink / raw)
  To: Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
	Sean Paul, Marijn Suijten, David Airlie, Simona Vetter,
	Eugene Lepshy, Danila Tikhonov
  Cc: Pengyu Luo, linux-arm-msm, dri-devel, freedreno, linux-kernel

In upstream the msm, for bpc greater than 8 are not supported yet,
although the hardware block supports this. Remove them until we
support them.

Fixes: b0e71c2637d1 ("drm/msm/dsi: Allow values of 10 and 12 for bits per component")
Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
---
 drivers/gpu/drm/msm/dsi/dsi_host.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
index e8e83ee61e..b60b26ddb0 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -1824,12 +1824,9 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc
 
 	switch (dsc->bits_per_component) {
 	case 8:
-	case 10:
-	case 12:
 		/*
-		 * Only 8, 10, and 12 bpc are supported for DSC 1.1 block.
-		 * If additional bpc values need to be supported, update
-		 * this quard with the appropriate DSC version verification.
+		 * In the upstream msm, only 8 bpc is supported for DSC 1.1/1.2
+		 * block.
 		 */
 		break;
 	default:
-- 
2.53.0


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

* Re: [PATCH] drm/msm/dsi: remove bpc > 8 entries from allow list
  2026-03-06  9:25 [PATCH] drm/msm/dsi: remove bpc > 8 entries from allow list Pengyu Luo
@ 2026-03-06 18:47 ` Dmitry Baryshkov
  2026-03-06 19:56   ` Eugene Lepshy
  2026-03-06 20:19   ` Евгений Лепший
  2026-03-08 23:06 ` Claude review: " Claude Code Review Bot
  2026-03-08 23:06 ` Claude Code Review Bot
  2 siblings, 2 replies; 7+ messages in thread
From: Dmitry Baryshkov @ 2026-03-06 18:47 UTC (permalink / raw)
  To: Pengyu Luo, Eugene Lepshy, Danila Tikhonov
  Cc: Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
	Sean Paul, Marijn Suijten, David Airlie, Simona Vetter,
	Eugene Lepshy, Danila Tikhonov, linux-arm-msm, dri-devel,
	freedreno, linux-kernel

On Fri, Mar 06, 2026 at 05:25:00PM +0800, Pengyu Luo wrote:
> In upstream the msm, for bpc greater than 8 are not supported yet,
> although the hardware block supports this. Remove them until we
> support them.
> 
> Fixes: b0e71c2637d1 ("drm/msm/dsi: Allow values of 10 and 12 for bits per component")
> Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>

I hope Marijn, Danila or Eugeny can comment. The patch series with this
patchset added 10 bpc panel and used it for one of the phones.

> ---
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index e8e83ee61e..b60b26ddb0 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -1824,12 +1824,9 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc
>  
>  	switch (dsc->bits_per_component) {
>  	case 8:
> -	case 10:
> -	case 12:
>  		/*
> -		 * Only 8, 10, and 12 bpc are supported for DSC 1.1 block.
> -		 * If additional bpc values need to be supported, update
> -		 * this quard with the appropriate DSC version verification.
> +		 * In the upstream msm, only 8 bpc is supported for DSC 1.1/1.2
> +		 * block.
>  		 */
>  		break;
>  	default:
> -- 
> 2.53.0
> 

-- 
With best wishes
Dmitry

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

* Re: [PATCH] drm/msm/dsi: remove bpc > 8 entries from allow list
  2026-03-06 18:47 ` Dmitry Baryshkov
@ 2026-03-06 19:56   ` Eugene Lepshy
  2026-03-06 20:19   ` Евгений Лепший
  1 sibling, 0 replies; 7+ messages in thread
From: Eugene Lepshy @ 2026-03-06 19:56 UTC (permalink / raw)
  To: Dmitry Baryshkov, Pengyu Luo, Danila Tikhonov
  Cc: Rob Clark, Dmitry Baryshkov, Abhinav Kumar, Jessica Zhang,
	Sean Paul, Marijn Suijten, David Airlie, Simona Vetter,
	linux-arm-msm, dri-devel, freedreno, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 2286 bytes --]

Hi,

I have a concern regarding this patch. The Nothing Phone (1) (sm7325-nothing-spacewar), which is already supported in mainline, utilizes a panel with bpc=10 and bpp=8 (DSC) [1].

Currently, this configuration works properly. While I have encountered minor graphical artifacts during brightness changes, the display output is otherwise reliable across all supported refresh rates (60/90/120 Hz).

Since this panel is already upstreamed, this patch might cause regressions.

[1] https://github.com/NothingOSS/android_kernel_devicetree_nothing_sm7325/blob/6f027f0440e3dce8a674d9cbd2f6ad944120e209/msm-extra/display-devicetree/display/dsi-panel-rm692e5-visionox-fhd-plus-120hz-cmd.dtsi#L483-L484

Best regards,
Eugene Lepshy

On March 6, 2026 9:47:23 PM GMT+03:00, Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> wrote:
>On Fri, Mar 06, 2026 at 05:25:00PM +0800, Pengyu Luo wrote:
>> In upstream the msm, for bpc greater than 8 are not supported yet,
>> although the hardware block supports this. Remove them until we
>> support them.
>> 
>> Fixes: b0e71c2637d1 ("drm/msm/dsi: Allow values of 10 and 12 for bits per component")
>> Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
>
>I hope Marijn, Danila or Eugeny can comment. The patch series with this
>patchset added 10 bpc panel and used it for one of the phones.
>
>> ---
>>  drivers/gpu/drm/msm/dsi/dsi_host.c | 7 ++-----
>>  1 file changed, 2 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> index e8e83ee61e..b60b26ddb0 100644
>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>> @@ -1824,12 +1824,9 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc
>>  
>>  	switch (dsc->bits_per_component) {
>>  	case 8:
>> -	case 10:
>> -	case 12:
>>  		/*
>> -		 * Only 8, 10, and 12 bpc are supported for DSC 1.1 block.
>> -		 * If additional bpc values need to be supported, update
>> -		 * this quard with the appropriate DSC version verification.
>> +		 * In the upstream msm, only 8 bpc is supported for DSC 1.1/1.2
>> +		 * block.
>>  		 */
>>  		break;
>>  	default:
>> -- 
>> 2.53.0
>> 
>
>-- 
>With best wishes
>Dmitry

[-- Attachment #2: Type: text/html, Size: 3210 bytes --]

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

* Re: [PATCH] drm/msm/dsi: remove bpc > 8 entries from allow list
  2026-03-06 18:47 ` Dmitry Baryshkov
  2026-03-06 19:56   ` Eugene Lepshy
@ 2026-03-06 20:19   ` Евгений Лепший
  2026-03-07  6:37     ` Pengyu Luo
  1 sibling, 1 reply; 7+ messages in thread
From: Евгений Лепший @ 2026-03-06 20:19 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Pengyu Luo, Danila Tikhonov, Rob Clark, Dmitry Baryshkov,
	Abhinav Kumar, Jessica Zhang, Sean Paul, Marijn Suijten,
	David Airlie, Simona Vetter, linux-arm-msm, dri-devel, freedreno,
	linux-kernel

Hi,

I have a concern regarding this patch. The Nothing Phone (1)
(sm7325-nothing-spacewar), which is already supported in mainline,
utilizes a panel with bpc=10 and bpp=8 (DSC) [1].

Currently, this configuration works properly. While I have encountered
minor graphical artifacts during brightness changes, the display
output is otherwise reliable across all supported refresh rates
(60/90/120 Hz).

Since this panel is already upstreamed, this patch might cause regressions.

[1] https://github.com/NothingOSS/android_kernel_devicetree_nothing_sm7325/blob/6f027f0440e3dce8a674d9cbd2f6ad944120e209/msm-extra/display-devicetree/display/dsi-panel-rm692e5-visionox-fhd-plus-120hz-cmd.dtsi#L483-L484

Best regards,
Eugene Lepshy

пт, 6 мар. 2026 г. в 21:47, Dmitry Baryshkov
<dmitry.baryshkov@oss.qualcomm.com>:
>
> On Fri, Mar 06, 2026 at 05:25:00PM +0800, Pengyu Luo wrote:
> > In upstream the msm, for bpc greater than 8 are not supported yet,
> > although the hardware block supports this. Remove them until we
> > support them.
> >
> > Fixes: b0e71c2637d1 ("drm/msm/dsi: Allow values of 10 and 12 for bits per component")
> > Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
>
> I hope Marijn, Danila or Eugeny can comment. The patch series with this
> patchset added 10 bpc panel and used it for one of the phones.
>
> > ---
> >  drivers/gpu/drm/msm/dsi/dsi_host.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > index e8e83ee61e..b60b26ddb0 100644
> > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > @@ -1824,12 +1824,9 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc
> >
> >       switch (dsc->bits_per_component) {
> >       case 8:
> > -     case 10:
> > -     case 12:
> >               /*
> > -              * Only 8, 10, and 12 bpc are supported for DSC 1.1 block.
> > -              * If additional bpc values need to be supported, update
> > -              * this quard with the appropriate DSC version verification.
> > +              * In the upstream msm, only 8 bpc is supported for DSC 1.1/1.2
> > +              * block.
> >                */
> >               break;
> >       default:
> > --
> > 2.53.0
> >
>
> --
> With best wishes
> Dmitry

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

* Re: [PATCH] drm/msm/dsi: remove bpc > 8 entries from allow list
  2026-03-06 20:19   ` Евгений Лепший
@ 2026-03-07  6:37     ` Pengyu Luo
  0 siblings, 0 replies; 7+ messages in thread
From: Pengyu Luo @ 2026-03-07  6:37 UTC (permalink / raw)
  To: Евгений Лепший
  Cc: Dmitry Baryshkov, Danila Tikhonov, Rob Clark, Dmitry Baryshkov,
	Abhinav Kumar, Jessica Zhang, Sean Paul, Marijn Suijten,
	David Airlie, Simona Vetter, linux-arm-msm, dri-devel, freedreno,
	linux-kernel

On Sat, Mar 7, 2026 at 4:20 AM Евгений Лепший <fekz115@gmail.com> wrote:
>
> Hi,
>
> I have a concern regarding this patch. The Nothing Phone (1)
> (sm7325-nothing-spacewar), which is already supported in mainline,
> utilizes a panel with bpc=10 and bpp=8 (DSC) [1].
>
> Currently, this configuration works properly. While I have encountered
> minor graphical artifacts during brightness changes, the display
> output is otherwise reliable across all supported refresh rates
> (60/90/120 Hz).
>
> Since this panel is already upstreamed, this patch might cause regressions.
>
> [1] https://github.com/NothingOSS/android_kernel_devicetree_nothing_sm7325/blob/6f027f0440e3dce8a674d9cbd2f6ad944120e209/msm-extra/display-devicetree/display/dsi-panel-rm692e5-visionox-fhd-plus-120hz-cmd.dtsi#L483-L484
>


Oh, I see. I messed up bpc, it is fine now. Recently, encoders have
supported src bpc > 8, dsi don't support dst bpc > 8. But the nothing
phone panel does not require dsi to support it.

Since the panel is a 8-bit panel, and src bpc != dst bpc. Although they
are equal on some devices(they are, on my 8-bit or 10-bit devices).

i.e. qcom,mdss-dsc-bit-per-component != qcom,mdss-dsi-bpp / 3

Please ignore this patch.

Best wishes,
Pengyu

> Best regards,
> Eugene Lepshy
>
> пт, 6 мар. 2026 г. в 21:47, Dmitry Baryshkov
> <dmitry.baryshkov@oss.qualcomm.com>:
> >
> > On Fri, Mar 06, 2026 at 05:25:00PM +0800, Pengyu Luo wrote:
> > > In upstream the msm, for bpc greater than 8 are not supported yet,
> > > although the hardware block supports this. Remove them until we
> > > support them.
> > >
> > > Fixes: b0e71c2637d1 ("drm/msm/dsi: Allow values of 10 and 12 for bits per component")
> > > Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
> >
> > I hope Marijn, Danila or Eugeny can comment. The patch series with this
> > patchset added 10 bpc panel and used it for one of the phones.
> >
> > > ---
> > >  drivers/gpu/drm/msm/dsi/dsi_host.c | 7 ++-----
> > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > > index e8e83ee61e..b60b26ddb0 100644
> > > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> > > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> > > @@ -1824,12 +1824,9 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc
> > >
> > >       switch (dsc->bits_per_component) {
> > >       case 8:
> > > -     case 10:
> > > -     case 12:
> > >               /*
> > > -              * Only 8, 10, and 12 bpc are supported for DSC 1.1 block.
> > > -              * If additional bpc values need to be supported, update
> > > -              * this quard with the appropriate DSC version verification.
> > > +              * In the upstream msm, only 8 bpc is supported for DSC 1.1/1.2
> > > +              * block.
> > >                */
> > >               break;
> > >       default:
> > > --
> > > 2.53.0
> > >
> >
> > --
> > With best wishes
> > Dmitry

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

* Claude review: drm/msm/dsi: remove bpc > 8 entries from allow list
  2026-03-06  9:25 [PATCH] drm/msm/dsi: remove bpc > 8 entries from allow list Pengyu Luo
  2026-03-06 18:47 ` Dmitry Baryshkov
  2026-03-08 23:06 ` Claude review: " Claude Code Review Bot
@ 2026-03-08 23:06 ` Claude Code Review Bot
  2 siblings, 0 replies; 7+ messages in thread
From: Claude Code Review Bot @ 2026-03-08 23:06 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: drm/msm/dsi: remove bpc > 8 entries from allow list
Author: Pengyu Luo <mitltlatltl@gmail.com>
Patches: 5
Reviewed: 2026-03-09T09:06:15.193451

---

This is a single patch that removes `case 10:` and `case 12:` from the `bits_per_component` allow list in `dsi_populate_dsc_params()`, restricting DSC to 8 bpc only. The stated rationale is that bpc > 8 is "not supported yet" in upstream MSM.

The patch is **questionable in its motivation and correctness**. The code path for 10/12 bpc appears to be fully functional — the RC parameters tables (`rc_parameters_pre_scr`) in `drm_dsc_helper.c` have entries for bpc=10, and the rest of `dsi_populate_dsc_params()` is generic with respect to bpc. The original commit `b0e71c2637d1` deliberately added 10/12 bpc support. Removing it without a clear bug report or test failure constitutes a functionality regression.

---
Generated by Claude Code Patch Reviewer

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

* Claude review: drm/msm/dsi: remove bpc > 8 entries from allow list
  2026-03-06  9:25 [PATCH] drm/msm/dsi: remove bpc > 8 entries from allow list Pengyu Luo
  2026-03-06 18:47 ` Dmitry Baryshkov
@ 2026-03-08 23:06 ` Claude Code Review Bot
  2026-03-08 23:06 ` Claude Code Review Bot
  2 siblings, 0 replies; 7+ messages in thread
From: Claude Code Review Bot @ 2026-03-08 23:06 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Commit message issues:**

1. The commit message says "In upstream the msm, for bpc greater than 8 are not supported yet" — this is grammatically awkward and, more importantly, **factually unclear**. The code *does* support 10/12 bpc at the DSC parameter computation level. If there is a specific hardware or integration issue that makes 10/12 bpc broken, the commit message should explain what that issue is (e.g., incorrect packing, register programming, or observed display corruption).

2. The updated comment claims "DSC 1.1/1.2 block" but the code at line 1848 uses `DRM_DSC_1_1_PRE_SCR` exclusively. Mentioning DSC 1.2 is misleading since this code path only handles pre-SCR DSC 1.1 parameters.

```c
+		 * In the upstream msm, only 8 bpc is supported for DSC 1.1/1.2
+		 * block.
```

This should say "DSC 1.1" only, consistent with:
```c
	/* DPU supports only pre-SCR panels */
	ret = drm_dsc_setup_rc_params(dsc, DRM_DSC_1_1_PRE_SCR);
```

**Functional concerns:**

3. The `Fixes:` tag implies this is a bug fix suitable for stable backport, but no actual bug is described. Removing supported functionality is not a "fix" — it's a feature removal. If 10/12 bpc was never validated on hardware but is otherwise correct code, a `Fixes:` tag is inappropriate. A more accurate approach would be to document the limitation without the `Fixes:` tag, or to explain the specific failure mode.

4. The `rc_parameters_pre_scr` table in `drm_dsc_helper.c` has valid entries for bpc=10. The downstream code in `dsi_populate_dsc_params()` — including `drm_dsc_set_const_params()`, `drm_dsc_set_rc_buf_thresh()`, `drm_dsc_setup_rc_params()`, and the `line_buf_depth` calculation — all work correctly with bpc values of 10 and 12. Removing them without evidence of breakage will regress any panels that actually use 10/12 bpc DSC.

**Recommendation:** This patch should **not be applied** in its current form. The author should either:
- Provide evidence of an actual bug caused by 10/12 bpc (display corruption, register misconfiguration, etc.) and describe it in the commit message, or
- Drop the `Fixes:` tag and reframe this as a deliberate scope reduction with justification for why untested-but-correct code should be removed rather than left in place.

---
Generated by Claude Code Patch Reviewer

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

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

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-06  9:25 [PATCH] drm/msm/dsi: remove bpc > 8 entries from allow list Pengyu Luo
2026-03-06 18:47 ` Dmitry Baryshkov
2026-03-06 19:56   ` Eugene Lepshy
2026-03-06 20:19   ` Евгений Лепший
2026-03-07  6:37     ` Pengyu Luo
2026-03-08 23:06 ` Claude review: " Claude Code Review Bot
2026-03-08 23:06 ` 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