[Freedreno] [RFC PATCH v2 10/13] drm/msm/dpu: add list of supported formats to the DPU caps

[Dmitry Baryshkov]

On 07/06/2023 02:14, Abhinav Kumar wrote:
> 
> 
> On 6/6/2023 3:59 PM, Dmitry Baryshkov wrote:
>> On 07/06/2023 01:57, Abhinav Kumar wrote:
>>>
>>>
>>> On 6/6/2023 3:50 PM, Dmitry Baryshkov wrote:
>>>> On 07/06/2023 01:47, Abhinav Kumar wrote:
>>>>>
>>>>>
>>>>> On 6/6/2023 2:52 PM, Dmitry Baryshkov wrote:
>>>>>> On 07/06/2023 00:47, Abhinav Kumar wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 6/6/2023 2:29 PM, Dmitry Baryshkov wrote:
>>>>>>>> On 07/06/2023 00:14, Abhinav Kumar wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 5/24/2023 6:47 PM, Dmitry Baryshkov wrote:
>>>>>>>>>> On Thu, 25 May 2023 at 02:16, Abhinav Kumar 
>>>>>>>>>> <quic_abhinavk at quicinc.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 3/20/2023 6:18 PM, Dmitry Baryshkov wrote:
>>>>>>>>>>>> As we are going to add virtual planes, add the list of 
>>>>>>>>>>>> supported formats
>>>>>>>>>>>> to the hw catalog entry. It will be used to setup universal 
>>>>>>>>>>>> planes, with
>>>>>>>>>>>> later selecting a pipe depending on whether the YUV format 
>>>>>>>>>>>> is used for
>>>>>>>>>>>> the framebuffer.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> If your usage of format_list is going to be internal to 
>>>>>>>>>>> dpu_plane.c, I
>>>>>>>>>>> can think of another idea for this change.
>>>>>>>>>>>
>>>>>>>>>>> This essentially translates to if (num_vig >= 1)
>>>>>>>>>>>
>>>>>>>>>>> If we can just have a small helper to detect that from the 
>>>>>>>>>>> catalog can
>>>>>>>>>>> we use that instead of adding formats to the dpu caps?
>>>>>>>>>>
>>>>>>>>>> I'd prefer to be explicit here. The list of supported formats 
>>>>>>>>>> might
>>>>>>>>>> vary between generations, might it not? Also we don't have a 
>>>>>>>>>> case of
>>>>>>>>>> the devices not having VIG planes. Even the qcm2290 (which 
>>>>>>>>>> doesn't
>>>>>>>>>> have CSC) lists YUV as supported.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> the list of formats is tied to the sspps the dpu generation has 
>>>>>>>>> and the capabilities of those sspps.
>>>>>>>>>
>>>>>>>>> qcm2290 is really an interesting case. It has one vig sspp but 
>>>>>>>>> no csc block for that vig sspp, hence it cannot support non-RGB 
>>>>>>>>> formats.
>>>>>>>>>
>>>>>>>>> I have confirmed that downstream is incorrect to populate yuv 
>>>>>>>>> formats for qcm2290.
>>>>>>>>>
>>>>>>>>> I still think that having atleast one vig sspp with csc sub-blk 
>>>>>>>>> is the right condition to check if we want to decide if the dpu 
>>>>>>>>> for that chipset supports yuv formats. Extra csc-blk check to 
>>>>>>>>> handle qcm2290.
>>>>>>>>>
>>>>>>>>> Having a small helper in dpu_plane.c is good enough for that 
>>>>>>>>> because with virtual planes, you only need to know "if such a 
>>>>>>>>> plane exists and not which plane" and a full catalog change 
>>>>>>>>> isnt needed IMO
>>>>>>>>
>>>>>>>> This goes down to the question: is the list of YUV and non-YUV 
>>>>>>>> formats static or not? Do all DPU devices support the same set 
>>>>>>>> of YUV and non-YUV formats? If it is static, we might as well 
>>>>>>>> drop dpu_sspp_sub_blks::format_list.
>>>>>>>>
>>>>>>>
>>>>>>> I would say yes based on the below reference:
>>>>>>>
>>>>>>> https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/blob/clo/main/msm/sde/sde_hw_catalog.c#L3858
>>>>>>>
>>>>>>> We always add the same set of YUV formats for all Vig SSPPs 
>>>>>>> irrespective of the chipsets.
>>>>>>
>>>>>> Well, as your example pointed out, few lines below it starts 
>>>>>> adding formats to the list, so the format list is not static and 
>>>>>> depends on the generation.
>>>>>>
>>>>>
>>>>> No, the DPU revision checks are there to add P010 UBWC formats on 
>>>>> top of the Vig formats.
>>>>>
>>>>> At the moment, the latest downstream code (code which is not on CLO 
>>>>> hence I cannot share) has dropped all that and just checks if P010 
>>>>> UBWC is supported for the Vig SSPP and adds all those.
>>>>>
>>>>> So its still tied to the feature bit whether P010 UBWC is supported 
>>>>> in the Vig SSPP and not at the chipset level.
>>>>
>>>> So, what is the difference? This means that depending on some 
>>>> conditions either we can support P010 UBWC or we can not. So the 
>>>> list of all suppored formats is not static.
>>>>
>>>
>>> The difference is SSPP level vs chipset level. This patch was made 
>>> with chipset level in mind and had to expand the format list for each 
>>> chipset.
>>>
>>> What I am suggesting is its a SSPP level decision. Please note its 
>>> not just P010 UBWC but P010 UBWC FOR Vig SSPP.
>>>
>>> So expanding each chipset's format list is not needed when the 
>>> decision can be made just on the basis of the SSPP's feature flag OR 
>>> the presence of the csc blk.
>>
>> Maybe I misunderstand something. Is P010 UBWC format supported on VIG 
>> SSPPs on all generations? The code that you have pointed me suggests 
>> that is is not.
>>
> 
> No, your understanding is correct that P010 UBWC format is supported 
> only on Vig SSPPs of certain generations.
> 
> But my point is that, its still a SSPP level decision.
> 
> So even if we add P010 UBWC formats later to the list of supported 
> formats, what we will do is add that feature bit alone in the Vig SSPP's 
> feature mask of that chipset and based on that all the P010 UBWC formats 
> for the virtual planes.
> 
> But if we follow your approach, we will add another array called 
> plane_formats_p010_ubwc and add that to each chipset which will support it.

sspp->sblk->formats_list is constant, so we will have to add another 
formats array anyway.

> The only difference in idea is that, based on the SSPP's feature set of 
> that chipset we can still determine that Vs adding it to each chipset.
> 
>>>
>>>>>
>>>>>>>
>>>>>>>> Note to myself: consider 
>>>>>>>> dpu_mdss_cfg::{dma_formats,cursor_formats,vig_formats}. Either 
>>>>>>>> migrate dpu_sspp_sub_blks::format_list users to these fields or 
>>>>>>>> drop them.
>>>>>>>>
>>>>>>>
>>>>>>> Yes, I agree. There is no need to have format list in the sub_blk 
>>>>>>> as for one type of SSPP, it supports the same format across DPU 
>>>>>>> generations.
>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> Note: I think at some point we should have a closer look at 
>>>>>>>>>> the list
>>>>>>>>>> of supported formats and crosscheck that we do not have either
>>>>>>>>>> unsupported formats being listed, or missing formats which are 
>>>>>>>>>> not
>>>>>>>>>> listed as supported.
>>>>>>>>>>
>>>>
>>

-- 
With best wishes
Dmitry