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@xxxxxxxxxxxxxx>
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@xxxxxxxxxxx>
