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
