diff mbox series

[4/5] drm/msm/dpu1: Account for DSC's bits_per_pixel having 4 fractional bits

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

Commit Message

Marijn Suijten Oct. 1, 2022, 7:08 p.m. UTC
According to the comment this DPU register contains the bits per pixel
as a 6.4 fractional value, conveniently matching the contents of
bits_per_pixel in struct drm_dsc_config which also uses 4 fractional
bits.  However, the downstream source this implementation was
copy-pasted from has its bpp field stored _without_ fractional part.

This makes the entire convoluted math obsolete as it is impossible to
pull those 4 fractional bits out of thin air, by somehow trying to reuse
the lowest 2 bits of a non-fractional bpp (lsb = bpp % 4??).

The rest of the code merely attempts to keep the integer part a multiple
of 4, which is rendered useless thanks to data |= dsc->bits_per_pixel <<
12; already filling up those bits anyway (but not on downstream).

Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC")
Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

Comments

Dmitry Baryshkov Oct. 4, 2022, 2:35 p.m. UTC | #1
On Sat, 1 Oct 2022 at 22:08, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> According to the comment this DPU register contains the bits per pixel
> as a 6.4 fractional value, conveniently matching the contents of
> bits_per_pixel in struct drm_dsc_config which also uses 4 fractional
> bits.  However, the downstream source this implementation was
> copy-pasted from has its bpp field stored _without_ fractional part.
>
> This makes the entire convoluted math obsolete as it is impossible to
> pull those 4 fractional bits out of thin air, by somehow trying to reuse
> the lowest 2 bits of a non-fractional bpp (lsb = bpp % 4??).
>
> The rest of the code merely attempts to keep the integer part a multiple
> of 4, which is rendered useless thanks to data |= dsc->bits_per_pixel <<
> 12; already filling up those bits anyway (but not on downstream).
>
> Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC")
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
Abhinav Kumar Oct. 4, 2022, 5:03 p.m. UTC | #2
On 10/1/2022 12:08 PM, Marijn Suijten wrote:
> According to the comment this DPU register contains the bits per pixel
> as a 6.4 fractional value, conveniently matching the contents of
> bits_per_pixel in struct drm_dsc_config which also uses 4 fractional
> bits.  However, the downstream source this implementation was
> copy-pasted from has its bpp field stored _without_ fractional part.
> 
> This makes the entire convoluted math obsolete as it is impossible to
> pull those 4 fractional bits out of thin air, by somehow trying to reuse
> the lowest 2 bits of a non-fractional bpp (lsb = bpp % 4??).
> 
> The rest of the code merely attempts to keep the integer part a multiple
> of 4, which is rendered useless thanks to data |= dsc->bits_per_pixel <<
> 12; already filling up those bits anyway (but not on downstream).
> 
> Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC")
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>

Many of this bugs are because the downstream code from which this 
implementation was derived wasnt the latest perhaps?

Earlier, downstream had its own DSC struct maybe leading to this 
redundant math but now we have migrated over to use the upstream struct 
drm_dsc_config.

That being said, this patch LGTM
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c | 11 ++---------
>   1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> index f2ddcfb6f7ee..3662df698dae 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
> @@ -42,7 +42,7 @@ static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc,
>   			      u32 initial_lines)
>   {
>   	struct dpu_hw_blk_reg_map *c = &hw_dsc->hw;
> -	u32 data, lsb, bpp;
> +	u32 data;
>   	u32 slice_last_group_size;
>   	u32 det_thresh_flatness;
>   	bool is_cmd_mode = !(mode & DSC_MODE_VIDEO);
> @@ -56,14 +56,7 @@ static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc,
>   	data = (initial_lines << 20);
>   	data |= ((slice_last_group_size - 1) << 18);
>   	/* bpp is 6.4 format, 4 LSBs bits are for fractional part */
> -	data |= dsc->bits_per_pixel << 12;
> -	lsb = dsc->bits_per_pixel % 4;
> -	bpp = dsc->bits_per_pixel / 4;
> -	bpp *= 4;
> -	bpp <<= 4;
> -	bpp |= lsb;
> -
> -	data |= bpp << 8;
> +	data |= (dsc->bits_per_pixel << 8);
>   	data |= (dsc->block_pred_enable << 7);
>   	data |= (dsc->line_buf_depth << 3);
>   	data |= (dsc->simple_422 << 2);
Marijn Suijten Oct. 4, 2022, 10:11 p.m. UTC | #3
On 2022-10-04 10:03:07, Abhinav Kumar wrote:
> 
> 
> On 10/1/2022 12:08 PM, Marijn Suijten wrote:
> > According to the comment this DPU register contains the bits per pixel
> > as a 6.4 fractional value, conveniently matching the contents of
> > bits_per_pixel in struct drm_dsc_config which also uses 4 fractional
> > bits.  However, the downstream source this implementation was
> > copy-pasted from has its bpp field stored _without_ fractional part.
> > 
> > This makes the entire convoluted math obsolete as it is impossible to
> > pull those 4 fractional bits out of thin air, by somehow trying to reuse
> > the lowest 2 bits of a non-fractional bpp (lsb = bpp % 4??).
> > 
> > The rest of the code merely attempts to keep the integer part a multiple
> > of 4, which is rendered useless thanks to data |= dsc->bits_per_pixel <<
> > 12; already filling up those bits anyway (but not on downstream).
> > 
> > Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC")
> > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> 
> Many of this bugs are because the downstream code from which this 
> implementation was derived wasnt the latest perhaps?

Perhaps, this code is "identical" to what I'm looking at in some
downstream 4.14 / 4.19, where the upstream struct for DSC either wasn't
there or wasn't used.  We have to find and address these bugs one by one
to make our panels work, and this series gets one platform (sdm845) down
but has more work pending for others (sm8250 has my current focus).

Or are you suggesting to "redo" the DSC integration work based on a
(much) newer display techpack (SDE driver)?

> Earlier, downstream had its own DSC struct maybe leading to this 
> redundant math but now we have migrated over to use the upstream struct 
> drm_dsc_config.

Found the 3-year-old `disp: msm: use upstream dsc config data` commit
that makes this change.  It carries a similar comment:

    /* integer bpp support only */

The superfluous math was howerver removed earlier, in:

    disp: msm: fix dsc parameters related to 10 bpc 10 bpp

- Marijn

> That being said, this patch LGTM
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Abhinav Kumar Oct. 5, 2022, 2:19 p.m. UTC | #4
On 10/4/2022 3:11 PM, Marijn Suijten wrote:
> On 2022-10-04 10:03:07, Abhinav Kumar wrote:
>>
>>
>> On 10/1/2022 12:08 PM, Marijn Suijten wrote:
>>> According to the comment this DPU register contains the bits per pixel
>>> as a 6.4 fractional value, conveniently matching the contents of
>>> bits_per_pixel in struct drm_dsc_config which also uses 4 fractional
>>> bits.  However, the downstream source this implementation was
>>> copy-pasted from has its bpp field stored _without_ fractional part.
>>>
>>> This makes the entire convoluted math obsolete as it is impossible to
>>> pull those 4 fractional bits out of thin air, by somehow trying to reuse
>>> the lowest 2 bits of a non-fractional bpp (lsb = bpp % 4??).
>>>
>>> The rest of the code merely attempts to keep the integer part a multiple
>>> of 4, which is rendered useless thanks to data |= dsc->bits_per_pixel <<
>>> 12; already filling up those bits anyway (but not on downstream).
>>>
>>> Fixes: c110cfd1753e ("drm/msm/disp/dpu1: Add support for DSC")
>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
>>
>> Many of this bugs are because the downstream code from which this
>> implementation was derived wasnt the latest perhaps?
> 
> Perhaps, this code is "identical" to what I'm looking at in some
> downstream 4.14 / 4.19, where the upstream struct for DSC either wasn't
> there or wasn't used.  We have to find and address these bugs one by one
> to make our panels work, and this series gets one platform (sdm845) down
> but has more work pending for others (sm8250 has my current focus).
> 
> Or are you suggesting to "redo" the DSC integration work based on a
> (much) newer display techpack (SDE driver)?

There is no need to redo the DSC integration now.

The code I am referring to is here :

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

So with respect to the redundant math in patches 1/3/4/5 of this series, 
I dont see all the redundant math anymore in this calculation.

This is what i meant by my comment.

When DSC changes were pushed, they were indeed validated on sdm845 
devices by Vinod so there was a certain level of confidence on those 
changes.

At this point, we should just consider these as bug-fixes for upstream 
and keep going. A full redo is not required.

At some point in the next couple of months, we plan to add DSC 1.2 
support to MSM.

We will check for any missing changes (if any after this series of 
yours) and push those as part of that.

> 
>> Earlier, downstream had its own DSC struct maybe leading to this
>> redundant math but now we have migrated over to use the upstream struct
>> drm_dsc_config.
> 
> Found the 3-year-old `disp: msm: use upstream dsc config data` commit
> that makes this change.  It carries a similar comment:
> 
>      /* integer bpp support only */
> 
> The superfluous math was howerver removed earlier, in:
> 
>      disp: msm: fix dsc parameters related to 10 bpc 10 bpp
> 
> - Marijn
> 
>> That being said, this patch LGTM
>> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Marijn Suijten Oct. 5, 2022, 6:45 p.m. UTC | #5
On 2022-10-05 07:19:11, Abhinav Kumar wrote:
> > [..]
> > 
> > Or are you suggesting to "redo" the DSC integration work based on a
> > (much) newer display techpack (SDE driver)?
> 
> There is no need to redo the DSC integration now.
> 
> The code I am referring to is here :
> 
> https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/DISPLAY.LA.2.0.r1-08000-WAIPIO.0/msm/sde_dsc_helper.c#L240
> 
> So with respect to the redundant math in patches 1/3/4/5 of this series, 
> I dont see all the redundant math anymore in this calculation.
> 
> This is what i meant by my comment.

It all seems to have had a nice clean-up.  What I meant is that it might
have been more efficient to copy-paste the cleaned-up, improved
downstream implementation instead of individually trying to find and
address all issues; either by running into these bugs on upstream (the
way this patch series came to be), or by comparing the new/improved
downstream with upstream.

> When DSC changes were pushed, they were indeed validated on sdm845 
> devices by Vinod so there was a certain level of confidence on those 
> changes.

Some branches seemed to have a display driver without the DCS PPS
command, or with the command commented out (relying on the panel being
configured for DSC by the bootloader).  The "4 fractional bits" issue
might have gone unnoticed since the panel driver was writing, and both
the DSI and DPU1 drivers were reading this field without those
fractional bits.

It's only a small bug (but with disastrous results on panel drivers with
proper DCS PPS command), the rest is cruft that was copied from
downstream but not filtered out in development nor review.

> At this point, we should just consider these as bug-fixes for upstream 
> and keep going. A full redo is not required.

Ack, at least that doesn't make this series/work obsolete :)

> At some point in the next couple of months, we plan to add DSC 1.2 
> support to MSM.

That's appreciated as all devices I have here (on newer SoCs with DSC
1.2) also have high-resolution, high-fps panels that need DSC to
function correctly.
We'll see who gets to it first though :)

> We will check for any missing changes (if any after this series of 
> yours) and push those as part of that.

There are a few, but it's hard to say until the panel is fully working.
Current focus is on sm8250.
We can discuss this at a more informal pace in #linux-msm if you're
interested.

- Marijn
diff mbox series

Patch

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
index f2ddcfb6f7ee..3662df698dae 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_dsc.c
@@ -42,7 +42,7 @@  static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc,
 			      u32 initial_lines)
 {
 	struct dpu_hw_blk_reg_map *c = &hw_dsc->hw;
-	u32 data, lsb, bpp;
+	u32 data;
 	u32 slice_last_group_size;
 	u32 det_thresh_flatness;
 	bool is_cmd_mode = !(mode & DSC_MODE_VIDEO);
@@ -56,14 +56,7 @@  static void dpu_hw_dsc_config(struct dpu_hw_dsc *hw_dsc,
 	data = (initial_lines << 20);
 	data |= ((slice_last_group_size - 1) << 18);
 	/* bpp is 6.4 format, 4 LSBs bits are for fractional part */
-	data |= dsc->bits_per_pixel << 12;
-	lsb = dsc->bits_per_pixel % 4;
-	bpp = dsc->bits_per_pixel / 4;
-	bpp *= 4;
-	bpp <<= 4;
-	bpp |= lsb;
-
-	data |= bpp << 8;
+	data |= (dsc->bits_per_pixel << 8);
 	data |= (dsc->block_pred_enable << 7);
 	data |= (dsc->line_buf_depth << 3);
 	data |= (dsc->simple_422 << 2);