diff mbox series

[5/5] drm/dsc: Prevent negative BPG offsets from shadowing adjacent bitfields

Message ID 20221001190807.358691-6-marijn.suijten@somainline.org (mailing list archive)
State Not Applicable
Headers show
Series drm: Fix math issues in MSM DSC implementation | expand

Commit Message

Marijn Suijten Oct. 1, 2022, 7:08 p.m. UTC
msm's dsi_host specifies negative BPG offsets which fill the full 8 bits
of a char thanks to two's complement: this however results in those bits
bleeding into the next parameter when the field is only expected to
contain 6-bit wide values.
As a consequence random slices appear corrupted on-screen (tested on a
Sony Tama Akatsuki device with sdm845).

Use AND operators to limit all values that constitute the RC Range
parameter fields to their expected size.

Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 drivers/gpu/drm/display/drm_dsc_helper.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Marijn Suijten Oct. 1, 2022, 8:23 p.m. UTC | #1
On 2022-10-01 21:08:07, Marijn Suijten wrote:
> msm's dsi_host specifies negative BPG offsets which fill the full 8 bits
> of a char thanks to two's complement: this however results in those bits
> bleeding into the next parameter when the field is only expected to
> contain 6-bit wide values.
> As a consequence random slices appear corrupted on-screen (tested on a
> Sony Tama Akatsuki device with sdm845).
> 
> Use AND operators to limit all values that constitute the RC Range
> parameter fields to their expected size.
> 
> Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data")
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
>  drivers/gpu/drm/display/drm_dsc_helper.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c b/drivers/gpu/drm/display/drm_dsc_helper.c
> index c869c6e51e2b..2e7ef242685d 100644
> --- a/drivers/gpu/drm/display/drm_dsc_helper.c
> +++ b/drivers/gpu/drm/display/drm_dsc_helper.c
> @@ -243,11 +243,11 @@ void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_payload,
>  	 */
>  	for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
>  		pps_payload->rc_range_parameters[i] =
> -			cpu_to_be16((dsc_cfg->rc_range_params[i].range_min_qp <<
> +			cpu_to_be16(((dsc_cfg->rc_range_params[i].range_min_qp & 0x1f) <<
>  				     DSC_PPS_RC_RANGE_MINQP_SHIFT) |
> -				    (dsc_cfg->rc_range_params[i].range_max_qp <<
> +				    ((dsc_cfg->rc_range_params[i].range_max_qp & 0x1f) <<
>  				     DSC_PPS_RC_RANGE_MAXQP_SHIFT) |
> -				    (dsc_cfg->rc_range_params[i].range_bpg_offset));
> +				    (dsc_cfg->rc_range_params[i].range_bpg_offset & 0x3f));

Pre-empting the reviews: I was contemplating whether to use FIELD_PREP
here, given that it's not yet used anywhere else in this file.  For that
I'd remove the existing _SHIFT definitions and replace them with:

	#define DSC_PPS_RC_RANGE_MINQP_MASK		GENMASK(15, 11)
	#define DSC_PPS_RC_RANGE_MAXQP_MASK		GENMASK(10, 6)
	#define DSC_PPS_RC_RANGE_BPG_OFFSET_MASK	GENMASK(5, 0)

And turn this section of code into:

	cpu_to_be16(FIELD_PREP(DSC_PPS_RC_RANGE_MINQP_MASK,
			       dsc_cfg->rc_range_params[i].range_min_qp) |
		    FIELD_PREP(DSC_PPS_RC_RANGE_MAXQP_MASK,
			       dsc_cfg->rc_range_params[i].range_max_qp) |
		    FIELD_PREP(DSC_PPS_RC_RANGE_BPG_OFFSET_MASK,
			       dsc_cfg->rc_range_params[i].range_bpg_offset));

Is that okay/recommended?

- Marijn

>  	}
>  
>  	/* PPS 88 */
> -- 
> 2.37.3
>
Dmitry Baryshkov Oct. 4, 2022, 2:41 p.m. UTC | #2
On Sat, 1 Oct 2022 at 23:23, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> On 2022-10-01 21:08:07, Marijn Suijten wrote:
> > msm's dsi_host specifies negative BPG offsets which fill the full 8 bits
> > of a char thanks to two's complement: this however results in those bits
> > bleeding into the next parameter when the field is only expected to
> > contain 6-bit wide values.
> > As a consequence random slices appear corrupted on-screen (tested on a
> > Sony Tama Akatsuki device with sdm845).
> >
> > Use AND operators to limit all values that constitute the RC Range
> > parameter fields to their expected size.
> >
> > Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data")
> > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > ---
> >  drivers/gpu/drm/display/drm_dsc_helper.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c b/drivers/gpu/drm/display/drm_dsc_helper.c
> > index c869c6e51e2b..2e7ef242685d 100644
> > --- a/drivers/gpu/drm/display/drm_dsc_helper.c
> > +++ b/drivers/gpu/drm/display/drm_dsc_helper.c
> > @@ -243,11 +243,11 @@ void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_payload,
> >        */
> >       for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
> >               pps_payload->rc_range_parameters[i] =
> > -                     cpu_to_be16((dsc_cfg->rc_range_params[i].range_min_qp <<
> > +                     cpu_to_be16(((dsc_cfg->rc_range_params[i].range_min_qp & 0x1f) <<
> >                                    DSC_PPS_RC_RANGE_MINQP_SHIFT) |
> > -                                 (dsc_cfg->rc_range_params[i].range_max_qp <<
> > +                                 ((dsc_cfg->rc_range_params[i].range_max_qp & 0x1f) <<
> >                                    DSC_PPS_RC_RANGE_MAXQP_SHIFT) |
> > -                                 (dsc_cfg->rc_range_params[i].range_bpg_offset));
> > +                                 (dsc_cfg->rc_range_params[i].range_bpg_offset & 0x3f));
>
> Pre-empting the reviews: I was contemplating whether to use FIELD_PREP
> here, given that it's not yet used anywhere else in this file.  For that
> I'd remove the existing _SHIFT definitions and replace them with:
>
>         #define DSC_PPS_RC_RANGE_MINQP_MASK             GENMASK(15, 11)
>         #define DSC_PPS_RC_RANGE_MAXQP_MASK             GENMASK(10, 6)
>         #define DSC_PPS_RC_RANGE_BPG_OFFSET_MASK        GENMASK(5, 0)
>
> And turn this section of code into:
>
>         cpu_to_be16(FIELD_PREP(DSC_PPS_RC_RANGE_MINQP_MASK,
>                                dsc_cfg->rc_range_params[i].range_min_qp) |
>                     FIELD_PREP(DSC_PPS_RC_RANGE_MAXQP_MASK,
>                                dsc_cfg->rc_range_params[i].range_max_qp) |
>                     FIELD_PREP(DSC_PPS_RC_RANGE_BPG_OFFSET_MASK,
>                                dsc_cfg->rc_range_params[i].range_bpg_offset));
>
> Is that okay/recommended?

This is definitely easier to review. However if you do not want to use
FIELD_PREP, it would be better to split this into a series of `data |=
something` assignments terminated with the rc_range_parameters[i]
assignment.

>
> - Marijn
>
> >       }
> >
> >       /* PPS 88 */
> > --
> > 2.37.3
> >
Abhinav Kumar Oct. 4, 2022, 8:22 p.m. UTC | #3
On 10/1/2022 12:08 PM, Marijn Suijten wrote:
> msm's dsi_host specifies negative BPG offsets which fill the full 8 bits
> of a char thanks to two's complement: this however results in those bits
> bleeding into the next parameter when the field is only expected to
> contain 6-bit wide values.
> As a consequence random slices appear corrupted on-screen (tested on a
> Sony Tama Akatsuki device with sdm845).
> 
> Use AND operators to limit all values that constitute the RC Range
> parameter fields to their expected size.
> 
> Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data")
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
>   drivers/gpu/drm/display/drm_dsc_helper.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c b/drivers/gpu/drm/display/drm_dsc_helper.c
> index c869c6e51e2b..2e7ef242685d 100644
> --- a/drivers/gpu/drm/display/drm_dsc_helper.c
> +++ b/drivers/gpu/drm/display/drm_dsc_helper.c
> @@ -243,11 +243,11 @@ void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_payload,
>   	 */
>   	for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
>   		pps_payload->rc_range_parameters[i] =
> -			cpu_to_be16((dsc_cfg->rc_range_params[i].range_min_qp <<
> +			cpu_to_be16(((dsc_cfg->rc_range_params[i].range_min_qp & 0x1f) <<
>   				     DSC_PPS_RC_RANGE_MINQP_SHIFT) |
> -				    (dsc_cfg->rc_range_params[i].range_max_qp <<
> +				    ((dsc_cfg->rc_range_params[i].range_max_qp & 0x1f) <<
>   				     DSC_PPS_RC_RANGE_MAXQP_SHIFT) |
> -				    (dsc_cfg->rc_range_params[i].range_bpg_offset));
> +				    (dsc_cfg->rc_range_params[i].range_bpg_offset & 0x3f));
>   	}
>   

Looking at some examples of this for other vendors, they have managed to 
limit the value to 6 bits in their drivers:

https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L532

https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dsc/rc_calc_dpi.c#L87

Perhaps, msm should do the same thing instead of the helper change.

If you want to move to helper, other drivers need to be changed too to 
remove duplicate & 0x3f.

FWIW, this too has already been fixed in the latest downstream driver too.


Thanks

Abhinav

>   	/* PPS 88 */
Marijn Suijten Oct. 4, 2022, 9:48 p.m. UTC | #4
On 2022-10-04 17:41:07, Dmitry Baryshkov wrote:
> On Sat, 1 Oct 2022 at 23:23, Marijn Suijten
> <marijn.suijten@somainline.org> wrote:
> [..]
> > Pre-empting the reviews: I was contemplating whether to use FIELD_PREP
> > here, given that it's not yet used anywhere else in this file.  For that
> > I'd remove the existing _SHIFT definitions and replace them with:
> >
> >         #define DSC_PPS_RC_RANGE_MINQP_MASK             GENMASK(15, 11)
> >         #define DSC_PPS_RC_RANGE_MAXQP_MASK             GENMASK(10, 6)
> >         #define DSC_PPS_RC_RANGE_BPG_OFFSET_MASK        GENMASK(5, 0)
> >
> > And turn this section of code into:
> >
> >         cpu_to_be16(FIELD_PREP(DSC_PPS_RC_RANGE_MINQP_MASK,
> >                                dsc_cfg->rc_range_params[i].range_min_qp) |
> >                     FIELD_PREP(DSC_PPS_RC_RANGE_MAXQP_MASK,
> >                                dsc_cfg->rc_range_params[i].range_max_qp) |
> >                     FIELD_PREP(DSC_PPS_RC_RANGE_BPG_OFFSET_MASK,
> >                                dsc_cfg->rc_range_params[i].range_bpg_offset));
> >
> > Is that okay/recommended?
> 
> This is definitely easier to review. However if you do not want to use
> FIELD_PREP, it would be better to split this into a series of `data |=
> something` assignments terminated with the rc_range_parameters[i]
> assignment.

Anything is fine by me, I have no strong opinion on this and rather
leave it up to the maintainers.  However, FIELD_PREP gives us concise
`#define`s through a single `GENMASK()` carrying both the shift and
mask/field-width.
At the same time these genmask definitions map more clearly to the
layout comment right above:

	/*
	 * For DSC sink programming the RC Range parameter fields
	 * are as follows: Min_qp[15:11], max_qp[10:6], offset[5:0]
	 */

If switching to `data |=` however, I've been recommended to not use
FIELD_PREP but fulyl write out `data |= (value & MASK) << SHIFT`
instead.

Perhaps a more important question is how to apply this consistently
throughout the function?

- Marijn
Marijn Suijten Oct. 4, 2022, 9:57 p.m. UTC | #5
On 2022-10-04 13:22:25, Abhinav Kumar wrote:
> 
> On 10/1/2022 12:08 PM, Marijn Suijten wrote:
> > msm's dsi_host specifies negative BPG offsets which fill the full 8 bits
> > of a char thanks to two's complement: this however results in those bits
> > bleeding into the next parameter when the field is only expected to
> > contain 6-bit wide values.
> > As a consequence random slices appear corrupted on-screen (tested on a
> > Sony Tama Akatsuki device with sdm845).
> > 
> > Use AND operators to limit all values that constitute the RC Range
> > parameter fields to their expected size.
> > 
> > Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data")
> > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > ---
> >   drivers/gpu/drm/display/drm_dsc_helper.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c b/drivers/gpu/drm/display/drm_dsc_helper.c
> > index c869c6e51e2b..2e7ef242685d 100644
> > --- a/drivers/gpu/drm/display/drm_dsc_helper.c
> > +++ b/drivers/gpu/drm/display/drm_dsc_helper.c
> > @@ -243,11 +243,11 @@ void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_payload,
> >   	 */
> >   	for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
> >   		pps_payload->rc_range_parameters[i] =
> > -			cpu_to_be16((dsc_cfg->rc_range_params[i].range_min_qp <<
> > +			cpu_to_be16(((dsc_cfg->rc_range_params[i].range_min_qp & 0x1f) <<
> >   				     DSC_PPS_RC_RANGE_MINQP_SHIFT) |
> > -				    (dsc_cfg->rc_range_params[i].range_max_qp <<
> > +				    ((dsc_cfg->rc_range_params[i].range_max_qp & 0x1f) <<
> >   				     DSC_PPS_RC_RANGE_MAXQP_SHIFT) |
> > -				    (dsc_cfg->rc_range_params[i].range_bpg_offset));
> > +				    (dsc_cfg->rc_range_params[i].range_bpg_offset & 0x3f));
> >   	}
> >   
> 
> Looking at some examples of this for other vendors, they have managed to 
> limit the value to 6 bits in their drivers:
> 
> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L532
> 
> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dsc/rc_calc_dpi.c#L87
> 
> Perhaps, msm should do the same thing instead of the helper change.

Thanks, I should have done my due-diligence and look up how other
drivers dealt with this, but wasn't immediately expecting negative
values elsewhere.

Alas, as explained in the cover letter I opted to perform the masking in
the PPS packing code as the DSC block code also reads these values, and
would suddenly write 6-bit intead of 8-bit values to the
DSC_RANGE_BPG_OFFSET registers.  Quick testing on the mentioned sdm845
platform shows no regressions, but I'm not sure if that's safe to rely
on?

> If you want to move to helper, other drivers need to be changed too to 
> remove duplicate & 0x3f.

Sure, we only have to confirm whether those drivers also read back the
value(s) in rc_range_params, and expect / allow this to be 8 instead of
6 bits.

> FWIW, this too has already been fixed in the latest downstream driver too.

What is this supposed to mean?  Is there a downstream DPU project that
has pending patches needing to be upstreamed?  Or is the downstream SDE,
techpack/display, or whatever it is called nowadays, slowly using more
DRM structs like drm_dsc_config and this drm_dsc_pps_payload_pack()
helper function as pointed out in an earlier mail?

Offtopic: are SDE and DPU growing closer together, hopefully achieving
feature parity allowing the SDE project to be dropped in favour of a
fully upstreamed DPU driver for day-one out-of-the-box mainline support
for new SoCs (as long as work is published and on its way upstream)?

- Marijn
Abhinav Kumar Oct. 4, 2022, 10:31 p.m. UTC | #6
On 10/4/2022 2:57 PM, Marijn Suijten wrote:
> On 2022-10-04 13:22:25, Abhinav Kumar wrote:
>>
>> On 10/1/2022 12:08 PM, Marijn Suijten wrote:
>>> msm's dsi_host specifies negative BPG offsets which fill the full 8 bits
>>> of a char thanks to two's complement: this however results in those bits
>>> bleeding into the next parameter when the field is only expected to
>>> contain 6-bit wide values.
>>> As a consequence random slices appear corrupted on-screen (tested on a
>>> Sony Tama Akatsuki device with sdm845).
>>>
>>> Use AND operators to limit all values that constitute the RC Range
>>> parameter fields to their expected size.
>>>
>>> Fixes: b9080324d6ca ("drm/msm/dsi: add support for dsc data")
>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
>>> ---
>>>    drivers/gpu/drm/display/drm_dsc_helper.c | 6 +++---
>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c b/drivers/gpu/drm/display/drm_dsc_helper.c
>>> index c869c6e51e2b..2e7ef242685d 100644
>>> --- a/drivers/gpu/drm/display/drm_dsc_helper.c
>>> +++ b/drivers/gpu/drm/display/drm_dsc_helper.c
>>> @@ -243,11 +243,11 @@ void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_payload,
>>>    	 */
>>>    	for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
>>>    		pps_payload->rc_range_parameters[i] =
>>> -			cpu_to_be16((dsc_cfg->rc_range_params[i].range_min_qp <<
>>> +			cpu_to_be16(((dsc_cfg->rc_range_params[i].range_min_qp & 0x1f) <<
>>>    				     DSC_PPS_RC_RANGE_MINQP_SHIFT) |
>>> -				    (dsc_cfg->rc_range_params[i].range_max_qp <<
>>> +				    ((dsc_cfg->rc_range_params[i].range_max_qp & 0x1f) <<
>>>    				     DSC_PPS_RC_RANGE_MAXQP_SHIFT) |
>>> -				    (dsc_cfg->rc_range_params[i].range_bpg_offset));
>>> +				    (dsc_cfg->rc_range_params[i].range_bpg_offset & 0x3f));
>>>    	}
>>>    
>>
>> Looking at some examples of this for other vendors, they have managed to
>> limit the value to 6 bits in their drivers:
>>
>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/i915/display/intel_vdsc.c#L532
>>
>> https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/amd/display/dc/dsc/rc_calc_dpi.c#L87
>>
>> Perhaps, msm should do the same thing instead of the helper change.
> 
> Thanks, I should have done my due-diligence and look up how other
> drivers dealt with this, but wasn't immediately expecting negative
> values elsewhere.
> 
> Alas, as explained in the cover letter I opted to perform the masking in
> the PPS packing code as the DSC block code also reads these values, and
> would suddenly write 6-bit intead of 8-bit values to the
> DSC_RANGE_BPG_OFFSET registers.  Quick testing on the mentioned sdm845
> platform shows no regressions, but I'm not sure if that's safe to rely
> on?

I looked up the MDP_DSC_0_RANGE_BPG_OFFSET_* registers.
They take only a 6-bit value according to the SW documentation ( bits 5:0 )

It was always expecting only a 6-bit value and not 8.

So this change is safe.

> 
>> If you want to move to helper, other drivers need to be changed too to
>> remove duplicate & 0x3f.
> 
> Sure, we only have to confirm whether those drivers also read back the
> value(s) in rc_range_params, and expect / allow this to be 8 instead of
> 6 bits.
> 
>> FWIW, this too has already been fixed in the latest downstream driver too.
> 
> What is this supposed to mean?  Is there a downstream DPU project that
> has pending patches needing to be upstreamed?  Or is the downstream SDE,
> techpack/display, or whatever it is called nowadays, slowly using more
> DRM structs like drm_dsc_config and this drm_dsc_pps_payload_pack()
> helper function as pointed out in an earlier mail?
> 

No, what I meant was, the version of downstream driver based on which 
the upstream DSC was made seems to be an older version. Downstream 
drivers keep getting updated and we always keep trying to align with 
upstream structs.

This is true not just for DSC but even other blocks.

So as part of that effort, we started using struct drm_dsc_config . That 
change was made on newer chipsets. But the downstream SW on sdm845 based 
on which the DSC was upstreamed seems like didnt have that. Hence all 
this redundant math happened.

So this comment was more of a explanation about why this issue happened 
even though latest downstream didnt have this issue.

> Offtopic: are SDE and DPU growing closer together, hopefully achieving
> feature parity allowing the SDE project to be dropped in favour of a
> fully upstreamed DPU driver for day-one out-of-the-box mainline support
> for new SoCs (as long as work is published and on its way upstream)?
> 

There is still a lot of gap between SDE and DPU drivers at this point. 
We keep trying to upstream as many features as possible to minimize the 
gap but there is still a lot of work to do.


> - Marijn
Marijn Suijten Oct. 4, 2022, 10:39 p.m. UTC | #7
On 2022-10-04 15:31:10, Abhinav Kumar wrote:
> 
> 
> On 10/4/2022 2:57 PM, Marijn Suijten wrote:
> > [..]
> > Alas, as explained in the cover letter I opted to perform the masking in
> > the PPS packing code as the DSC block code also reads these values, and
> > would suddenly write 6-bit intead of 8-bit values to the
> > DSC_RANGE_BPG_OFFSET registers.  Quick testing on the mentioned sdm845
> > platform shows no regressions, but I'm not sure if that's safe to rely
> > on?
> 
> I looked up the MDP_DSC_0_RANGE_BPG_OFFSET_* registers.
> They take only a 6-bit value according to the SW documentation ( bits 5:0 )
> 
> It was always expecting only a 6-bit value and not 8.
> 
> So this change is safe.

Ack, I think that implies I should make this change and move the masks
to the DSI driver?

> >> If you want to move to helper, other drivers need to be changed too to
> >> remove duplicate & 0x3f.
> > 
> > Sure, we only have to confirm whether those drivers also read back the
> > value(s) in rc_range_params, and expect / allow this to be 8 instead of
> > 6 bits.
> > 
> >> FWIW, this too has already been fixed in the latest downstream driver too.
> > 
> > What is this supposed to mean?  Is there a downstream DPU project that
> > has pending patches needing to be upstreamed?  Or is the downstream SDE,
> > techpack/display, or whatever it is called nowadays, slowly using more
> > DRM structs like drm_dsc_config and this drm_dsc_pps_payload_pack()
> > helper function as pointed out in an earlier mail?
> > 
> 
> No, what I meant was, the version of downstream driver based on which 
> the upstream DSC was made seems to be an older version. Downstream 
> drivers keep getting updated and we always keep trying to align with 
> upstream structs.
> 
> This is true not just for DSC but even other blocks.
> 
> So as part of that effort, we started using struct drm_dsc_config . That 
> change was made on newer chipsets. But the downstream SW on sdm845 based 
> on which the DSC was upstreamed seems like didnt have that. Hence all 
> this redundant math happened.
> 
> So this comment was more of a explanation about why this issue happened 
> even though latest downstream didnt have this issue.

Thanks, I understood most of that but wasn't aware these exact "issues"
were also addressed downstream (by i.e. also using the upstream
structs).

> > Offtopic: are SDE and DPU growing closer together, hopefully achieving
> > feature parity allowing the SDE project to be dropped in favour of a
> > fully upstreamed DPU driver for day-one out-of-the-box mainline support
> > for new SoCs (as long as work is published and on its way upstream)?
> > 
> 
> There is still a lot of gap between SDE and DPU drivers at this point. 
> We keep trying to upstream as many features as possible to minimize the 
> gap but there is still a lot of work to do.

Glad to hear, but that sounds like a very hard to close gap unless
downstream "just works on DPU" instead of having parallel development on
two "competing" drivers for the exact same hardware.

- Marijn
Abhinav Kumar Oct. 5, 2022, 3:33 p.m. UTC | #8
On 10/4/2022 3:39 PM, Marijn Suijten wrote:
> On 2022-10-04 15:31:10, Abhinav Kumar wrote:
>>
>>
>> On 10/4/2022 2:57 PM, Marijn Suijten wrote:
>>> [..]
>>> Alas, as explained in the cover letter I opted to perform the masking in
>>> the PPS packing code as the DSC block code also reads these values, and
>>> would suddenly write 6-bit intead of 8-bit values to the
>>> DSC_RANGE_BPG_OFFSET registers.  Quick testing on the mentioned sdm845
>>> platform shows no regressions, but I'm not sure if that's safe to rely
>>> on?
>>
>> I looked up the MDP_DSC_0_RANGE_BPG_OFFSET_* registers.
>> They take only a 6-bit value according to the SW documentation ( bits 5:0 )
>>
>> It was always expecting only a 6-bit value and not 8.
>>
>> So this change is safe.
> 
> Ack, I think that implies I should make this change and move the masks
> to the DSI driver?

hmm .... downstream seems to have the & just before the reg write

https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/DISPLAY.LA.2.0.r1-08000-WAIPIO.0/msm/sde/sde_hw_dsc_1_2.c#L310

But yes, we can move this to the dsi calculation to match what others 
are doing. I am fine with that.

> 
>>>> If you want to move to helper, other drivers need to be changed too to
>>>> remove duplicate & 0x3f.
>>>
>>> Sure, we only have to confirm whether those drivers also read back the
>>> value(s) in rc_range_params, and expect / allow this to be 8 instead of
>>> 6 bits.
>>>
>>>> FWIW, this too has already been fixed in the latest downstream driver too.
>>>
>>> What is this supposed to mean?  Is there a downstream DPU project that
>>> has pending patches needing to be upstreamed?  Or is the downstream SDE,
>>> techpack/display, or whatever it is called nowadays, slowly using more
>>> DRM structs like drm_dsc_config and this drm_dsc_pps_payload_pack()
>>> helper function as pointed out in an earlier mail?
>>>
>>
>> No, what I meant was, the version of downstream driver based on which
>> the upstream DSC was made seems to be an older version. Downstream
>> drivers keep getting updated and we always keep trying to align with
>> upstream structs.
>>
>> This is true not just for DSC but even other blocks.
>>
>> So as part of that effort, we started using struct drm_dsc_config . That
>> change was made on newer chipsets. But the downstream SW on sdm845 based
>> on which the DSC was upstreamed seems like didnt have that. Hence all
>> this redundant math happened.
>>
>> So this comment was more of a explanation about why this issue happened
>> even though latest downstream didnt have this issue.
> 
> Thanks, I understood most of that but wasn't aware these exact "issues"
> were also addressed downstream (by i.e. also using the upstream
> structs).
> 

Even I wasnt. When I was reviewing this series, it seemed like a valid 
change so I checked the latest downstream code like i always do to see 
whether and how this is handled and found that these issues were 
addressed there so thought i would update that here.

>>> Offtopic: are SDE and DPU growing closer together, hopefully achieving
>>> feature parity allowing the SDE project to be dropped in favour of a
>>> fully upstreamed DPU driver for day-one out-of-the-box mainline support
>>> for new SoCs (as long as work is published and on its way upstream)?
>>>
>>
>> There is still a lot of gap between SDE and DPU drivers at this point.
>> We keep trying to upstream as many features as possible to minimize the
>> gap but there is still a lot of work to do.
> 
> Glad to hear, but that sounds like a very hard to close gap unless
> downstream "just works on DPU" instead of having parallel development on
> two "competing" drivers for the exact same hardware.
> 
Its not really parallel development OR competing drivers.
The design of these features downstream (not just DSC but many others) 
is quite different because some of the downstream designs wont get 
accepted upstream as its tightly coupled with our 
compositor/device-tree. Thats where we are slowly trying to implement 
these in an upstream compatible way.

BTW, that being said. Its not always the case that every issue seen 
upstream has already been fixed downstream. In fact, on some occasions 
we found something fixed in upstream in a better way and ported them 
downstream too.

Thanks

Abhinav
> - Marijn
Marijn Suijten Oct. 5, 2022, 6:29 p.m. UTC | #9
On 2022-10-05 08:33:23, Abhinav Kumar wrote:
[..]
> hmm .... downstream seems to have the & just before the reg write
> 
> https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/DISPLAY.LA.2.0.r1-08000-WAIPIO.0/msm/sde/sde_hw_dsc_1_2.c#L310

https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/DISPLAY.LA.2.0.r1-08000-WAIPIO.0/msm/sde/sde_hw_dsc.c#L156

The reference code for NON-v1.2 doesn't do this here, but you already
confirmed by the docs - and I confirmed by testing a set of size #1 -
that the register is fine with having only 6 bits.

> But yes, we can move this to the dsi calculation to match what others 
> are doing. I am fine with that.

Thanks, done that in v2.

> > [..]
> 
> Even I wasnt. When I was reviewing this series, it seemed like a valid 
> change so I checked the latest downstream code like i always do to see 
> whether and how this is handled and found that these issues were 
> addressed there so thought i would update that here.

Thanks for confirming that it was done in the correct/same way :)

> > [..]
> Its not really parallel development OR competing drivers.
> The design of these features downstream (not just DSC but many others) 
> is quite different because some of the downstream designs wont get 
> accepted upstream as its tightly coupled with our 
> compositor/device-tree. Thats where we are slowly trying to implement 
> these in an upstream compatible way.

But this is what it feels like.

To me this sounds like downstream is more of a staging / prototyping
area that is actively used in production, while the driver
implementation is fleshed out and slowly brought to mainline in a
careful and controlled manner.

That's not a bad thing, but it does mean that mainline always lags
behind in terms of features and hardware support.  At least I'm glad
to hear that downstream is slowly using more DRM primitives, and the
driver is becoming more digestible as well.

> BTW, that being said. Its not always the case that every issue seen 
> upstream has already been fixed downstream. In fact, on some occasions 
> we found something fixed in upstream in a better way and ported them 
> downstream too.

I wasn't expecting anything else, as different drivers have inevitably
different details and different bugs.  The issues this series fixes
weren't applicable to the downstream kernel because it (at the time)
wasn't even using this drm_dsc_config struct with different semantics.
Regardless, it's good to hear that fixes are transplanted in both ways
even if it does mean extra overhead maintaining and keeping tabs on two
drivers at once.

- Marijn
diff mbox series

Patch

diff --git a/drivers/gpu/drm/display/drm_dsc_helper.c b/drivers/gpu/drm/display/drm_dsc_helper.c
index c869c6e51e2b..2e7ef242685d 100644
--- a/drivers/gpu/drm/display/drm_dsc_helper.c
+++ b/drivers/gpu/drm/display/drm_dsc_helper.c
@@ -243,11 +243,11 @@  void drm_dsc_pps_payload_pack(struct drm_dsc_picture_parameter_set *pps_payload,
 	 */
 	for (i = 0; i < DSC_NUM_BUF_RANGES; i++) {
 		pps_payload->rc_range_parameters[i] =
-			cpu_to_be16((dsc_cfg->rc_range_params[i].range_min_qp <<
+			cpu_to_be16(((dsc_cfg->rc_range_params[i].range_min_qp & 0x1f) <<
 				     DSC_PPS_RC_RANGE_MINQP_SHIFT) |
-				    (dsc_cfg->rc_range_params[i].range_max_qp <<
+				    ((dsc_cfg->rc_range_params[i].range_max_qp & 0x1f) <<
 				     DSC_PPS_RC_RANGE_MAXQP_SHIFT) |
-				    (dsc_cfg->rc_range_params[i].range_bpg_offset));
+				    (dsc_cfg->rc_range_params[i].range_bpg_offset & 0x3f));
 	}
 
 	/* PPS 88 */