linux-kernel - Re: [PATCH 3/4] drm/msm/dsi: Allow all bpc values

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <rnuv4dieiy6s6c5s33hff7ntr6hkneemsq5qzk3u4ug2abwisd@6ahmijlayhhr>
Date: Sat, 8 Feb 2025 23:09:56 +0100
From: Marijn Suijten <marijn.suijten@...ainline.org>
To: Danila Tikhonov <danila@...xyga.com>
Cc: neil.armstrong@...aro.org, quic_jesszhan@...cinc.com, 
	maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de, airlied@...il.com, 
	simona@...ll.ch, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, 
	andersson@...nel.org, konradybcio@...nel.org, robdclark@...il.com, 
	quic_abhinavk@...cinc.com, dmitry.baryshkov@...aro.org, sean@...rly.run, jonathan@...ek.ca, 
	jun.nie@...aro.org, fekz115@...il.com, dri-devel@...ts.freedesktop.org, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org, 
	freedreno@...ts.freedesktop.org, linux@...nlining.org, ~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH 3/4] drm/msm/dsi: Allow all bpc values

On 2025-02-03 21:14:26, Danila Tikhonov wrote:
> From: Eugene Lepshy <fekz115@...il.com>
> 
> DRM DSC helper has parameters for various bpc values other than 8:

Weird zero-width \u200b spaces here between "values" and "other", please delete
those.

> (8/10/12/14/16).
> 
> Remove this guard.
> 
> Signed-off-by: Eugene Lepshy <fekz115@...il.com>
> Signed-off-by: Danila Tikhonov <danila@...xyga.com>

Should this patch elaborate that those "DRM DSC helper" don't have any
additional guarding for the values you mention either, i.e. passing 9 or 11 or
>16 don't seem to be checked anywhere else either?

And your title might have space to spell out "Bits Per Component" entirely.

> ---
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 007311c21fda..d182af7bbb81 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -1767,11 +1767,6 @@ static int dsi_populate_dsc_params(struct msm_dsi_host *msm_host, struct drm_dsc
>  		return -EINVAL;
>  	}
>  
> -	if (dsc->bits_per_component != 8) {
> -		DRM_DEV_ERROR(&msm_host->pdev->dev, "DSI does not support bits_per_component != 8 yet\n");
> -		return -EOPNOTSUPP;
> -	}
> -
>  	dsc->simple_422 = 0;
>  	dsc->convert_rgb = 1;
>  	dsc->vbr_enable = 0;

This seems supicous on the dpu1 side, in the original DSC 1.1 (not 1.2) block in
dpu_hw_dsc_config(), which has:

	data |= (dsc->line_buf_depth << 3);
	data |= (dsc->simple_422 << 2);
	data |= (dsc->convert_rgb << 1);
	data |= dsc->bits_per_component;

The original value of `8` would overlap with the lowest bit of line_buf_depth
(4th bit in `data`).  Now, the 2nd bit which will take the value from
convert_rgb, which is already set to 1 above, will overlap with the 2nd bit in
your new bpc value of 10.

Can you double-check that this code in DPU1 is actually valid?  I assume you
have tested this panel at least and it is working (worthy mention in the cover
letter?), this just seems like yet another mistake in the original bindings
(though the register always had a matching value with downstream on 8 BPC panels
for me).

> @@ -1779,7 +1774,7 @@ 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);
>  
> -	/* handle only bpp = bpc = 8, pre-SCR panels */
> +	/* handle only pre-SCR panels */
>  	ret = drm_dsc_setup_rc_params(dsc, DRM_DSC_1_1_PRE_SCR);

Good catch - this comment sounds like it's documenting a limitation of
this helper function, but the function does not have such limitations...
rc_parameters_pre_scr has values for all these combinations.

- Marijn

>  	if (ret) {
>  		DRM_DEV_ERROR(&msm_host->pdev->dev, "could not find DSC RC parameters\n");
> -- 
> 2.48.1
>