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

From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
To: Abhinav Kumar <quic_abhinavk@quicinc.com>
Cc: freedreno@lists.freedesktop.org, Sean Paul <sean@poorly.run>,
	Bjorn Andersson <andersson@kernel.org>,
	dri-devel@lists.freedesktop.org,
	Stephen Boyd <swboyd@chromium.org>,
	linux-arm-msm@vger.kernel.org
Subject: Re: [RFC PATCH v2 10/13] drm/msm/dpu: add list of supported formats to the DPU caps
Date: Wed, 7 Jun 2023 01:50:51 +0300	[thread overview]
Message-ID: <12aacfc4-8ed5-1c85-8c1c-abd0aed7f8a9@linaro.org> (raw)
In-Reply-To: <e43c9e17-1c0b-f3cd-036f-1a4968a0150d@quicinc.com>

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@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.

> 
>>>
>>>> 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