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

Sat Apr 29 21:11:56 UTC 2023

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 at quicinc.com> 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 at quicinc.com> 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