Re: [Freedreno] [PATCH v2 5/7] drm/msm/dpu: add DPU_PINGPONG_DSC feature PP_BLK and PP_BLK_TE

From: Dmitry Baryshkov
Date: Sat Apr 29 2023 - 17:12:05 EST

On 29/04/2023 23:23, Abhinav Kumar wrote:


On 4/29/2023 12:45 PM, Dmitry Baryshkov wrote:
On Sat, 29 Apr 2023 at 11:43, Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> wrote:



On 4/28/2023 9:35 PM, Dmitry Baryshkov wrote:
On 29/04/2023 07:04, Abhinav Kumar wrote:


On 4/28/2023 8:21 PM, Dmitry Baryshkov wrote:
On Sat, 29 Apr 2023 at 05:50, Abhinav Kumar
<quic_abhinavk@xxxxxxxxxxx> wrote:



On 4/28/2023 6:41 PM, Dmitry Baryshkov wrote:
On 29/04/2023 04:08, Abhinav Kumar wrote:


On 4/28/2023 5:45 PM, Dmitry Baryshkov wrote:
On 29/04/2023 02:45, Kuogee Hsieh wrote:
Legacy DPU requires PP hardware block involved into setting up DSC

Nit: to be envolved

data path. This patch add DDPU_PINGPONG_DSC feature bit to both

adds

PP_BLK and PP_BLK_TE so that both dpu_hw_pp_setup_dsc() and
dpu_hw_pp_dsc_enable() will be executed during DSC path setup.

Would it be easier to add PP_BLK_NO_DSC instead and make DSC enabled
by default for PP_BLK / PP_BLK_TE?


No because for some chipsets like qcm2290, it has a ping pong block
but no DSC.

And so it will use PP_BLK_NO_DSC (like msm8998 for PP_2/_3). But this

Ok so you meant adding another PP_BLK_* macro. I mistook it for a
feature flag.

This is getting a bit confusing with many PP_BLK_* variations.

Yes. As I wrote it might be not the optimal solution.


might be not the optimal solution. Let's check all possible cases:

- PP (or PP_TE?) with no DSC support,

Yes correct for chipsets like qcm2290.

- PP/PP_TE tightly bound to the particular DSC instance, should be
allocated together,

I need to check which exact chipset does this (I recall one does) but
perhaps msm8998 fits here.

All targets earlier than sm8150. SDE driver sets SDE_DSC_OUTPUT_CTRL
if for v1 CTL blocks (sm8150+).


- PP/PP_TE which can use any DSC block.

This is what DPU_PINGPONG_DSC feature flag should mean today and should
cover most of the DSC 1.1 chipsets present in upstream today.

Then it should probably be renamed to DPU_PINGPONG_ANY_DSC


Actually that "any" is incorrect. As part of the DSC pairing logic
change it will become clear but not all ping-pongs can pair with all
DSCs. So any is not right.

This is why I think we should go with this feature flag first. Lets
identify a pattern and then work on unifying or cleaning up the
PP_BLK_* masks.

Then I'd like to see the series sent together so that we don't have to
be wondering in the myst. At least the RM changes might better go into
this series (together with the topology changes. I think RM will depend
on 1 DSC topology support).

Topology changes depend on which way the virtual planes series is going
to do and since we are going with fixed topology you preferred it to be
dropped right?

It was not about the topology changes (e.g. supporting 1:1:1), but
about storying the topology in the encoder.


So you are suggesting drop patch 6 of this series and keep 7 OR drop 6 and 7 both?

It looks like if 6 is dropped, then 7 has to be as well because we need to store topology somewhere cannot keep passing locally.

Yes, 6/7 should be split from this series. For #7 we have all the information necessary (num_dsc and num_intf), so it is not strictly dependent on #6, but is anyway a separate change, not related to DSC 1.2.

Are there any actual changes in #7 for 2:1:1 case? If not, it should go together with 1:1:1 topology support.


Thats why I was okay to drop 6 and 7 together.


Now that you have seen the DSC to PP mapping code downstream you know
that every DSC cannot goto every ping-pong. So which way do you want to
go with this patch. If we are going to post a next revision, need some
conclusion here and I do not think PP_BLK_* was agreed as a clean
solution anyway.

No, it was not agreed. That's why I tried listing all possible cases
(in an attempt to understand the limitations and/or possible
usecases).


Thats why I suggested:

1) Lets go with feature flag
2) When DP lands, thats the one which uses 1-1-1 topology so we can hit
some corner cases of lets say DP uses one DSC and then DSI uses merge we
should go with the right DSC pair. With only DSI, this will not arise.
3) With (1) and (2) done, we can take up PP_BLK_* cleanup if we feel its
necessary.

I dont see any conclusion or plan suggested by you so far on which way
you want to go.

I'd propose the following plan:

Thanks for summarizing what you had in mind, that makes it clear.

- Drop all the topology and PP changes from this series. Make it add
just DSC 1.2 support.

Then for the DSI DSC 1.2 we should be able to continue using existing
RM/PP code with known limitations.



So just keep patches 1-3 right? The issue with that is _setup_pingpong_ops will still get called even for DSC 1.2 with those changes. This was actually noted by Marijn during the catalog cleanup we recently merged to -fixes. Thats why we included that in this so that we dont call DSC 1.1 related functions for 1.2

I thought about 1-4 initially. I didn't notice that this flag was not set for newer PP blocks.

Please split patch #5 into two: generic code + definition and catalog changes actually enabling this feature.

Also please note that 'PP block binding to DSC' isn't probably a good enough description of this feature bit. It doesn't control PP/DSC binding, it shows if DCE is controlled from PP block or not.


Any specific reason why you dont want to go with patches 1-5 now and drop 6,7 as that depends on storing topology.

Next:
- Split DPU_PINGPONG_DSC feature flag to a separate patch series. It
looks like it should have some Fixes tag, as it prevents one from
calling *dsc functions for PPs which do not support them

We can still add the fixes tag and keep it here and move it to the front?

Yes! Even w/o DSC 1.2 we should not be calling dsc functions for PP which have no DCE support.

- Extend the series with the RM fix which skips non-DSC-enabled PPs
when DSC is required.
- As this series is touching RM, it is a good place to handle DSC
allocation issues, which includes:
   * tight coupling of PP/DSC blocks on sdm845 and msm8998
   * limitations for 1:1:1 or co-allocation of DSC blocks for both DSI
and DP DSC (might be split to a separate patchset, up to you)

I am a bit confused about this. This patch is not touching RM at all from what I can see. So the issue of DSC pair allocation is there even today with or without DSC 1.2 about which DSCs pair with which ones and which DSCs go with which PP.

I was also confused here, as you see. What I meant here was to add a new patch touching RM code to the new, separate series.

I feel that once DSC 1.2, that is just patches 1-5 land, I agree this can be and should be a series on its own to prepare for DP DSC.

Yes, please.

What use-case gets broken for DSI if we land 1-5?

None. As I wrote, "Then for the DSI DSC 1.2 we should be able to continue using existing RM/PP code with known limitations."


We are trying to expand this series for a condition which wont hit till DP DSC lands due to the fixed topology and only one client for DSC which is DSI.

Please don't. Let it be just DSC 1.2 (e.g. as required for DSI DSC 1.2). The 1:1:1 and multi-client can be a separate series fixing RM/dpu_encoder.



DP DSC depends either on this series or on a separate patchset



I went on and checked the SDE. It has condition that pp % 2 == dsc % 2.
Does this condition apply only to "newest" DSC/PP combo (DITHER + 1.2)
or to the older ones too? Are there any other conditionals?


It was there even for older DSCs and looks like wasnt implemented in the
current upstream code.

Thanks for the info.


--
With best wishes
Dmitry