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
