Re: [PATCH 3/4] drm/msm/dpu: Add has_data_compress to dpu_caps

[Jessica Zhang]

On 5/3/2023 4:03 PM, Marijn Suijten wrote:
> Hi Jessica,
>
> On 2023-05-03 12:03:40, Jessica Zhang wrote:
> > On 5/3/2023 12:07 AM, Marijn Suijten wrote:
> > > On 2023-05-02 18:19:14, Jessica Zhang wrote:
> > > > Add data_compress feature to DPU HW catalog.
> > > >
> > > > In DPU 7.x and later, there is a DATA_COMPRESS register that must be set
> > > > within the DPU INTF block for DSC to work.
> > > >
> > > > As core_rev (and related macros) was removed from the dpu_kms struct, the
> > > > most straightforward way to indicate the presence of this register would be
> > > > to have a flag in dpu_caps.
> > >
> > > This is a very generic name to have in the global dpu_caps for a very
> > > specific register on the INTF block since DPU >= 7.0.0, and I doubt any
> > > new catalog contributor will know how to fill this field.  After all,
> > > DPU < 7.0.0 also has DCE but it is controlled via the PINGPONG block.
> > >
> > > Instead, how about having it as a DPU_INTF_DATA_COMPRESS (or similar)
> > > feature flag on the INTF block?  We do the same for other (register
> > > related) features on the INTF block, and you did the same to disable DSC
> > > callbacks on PP in [1].
>
> (Note: I said "you" but meant Kuogee)
>
> > Hi Marijn,
> >
> > Sounds good.
> >
> > > In fact it seems that the DSC/DCE (enablement) registers have been moved
> > > from PINGPONG to INTF in DPU 7.0.0.  Can you clarify in the patch
> > > message for v2 that this is the case, and do the same in the linked
> > > PINGPONG patch?  Perhaps these patches should be part of the same series
> > > as they do not seem DSI-specific.
> >
> > Will make a note of the PP to INTF change in the commit message.
>
> Thanks.
>
> > I would prefer to keep this patch in this series is because it is needed
> > for DSI over command mode to work and the subsequent patch is
> > specifically for command mode.
>
> That is fine, but do mention this in the commit message if it is
> relevant here.  Otherwise only mention it as part of patch 4/4.

Acked.

Thanks,

Jessica Zhang

> - Marijn