[Freedreno] [PATCH v3 18/27] drm/msm/dpu: populate SmartDMA features in hw catalog

[Abhinav Kumar]

On 2/4/2023 1:08 PM, Dmitry Baryshkov wrote:
> On 04/02/2023 20:35, Abhinav Kumar wrote:
>>
>>
>> On 2/4/2023 2:43 AM, Dmitry Baryshkov wrote:
>>> On 04/02/2023 07:10, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 2/3/2023 8:10 PM, Dmitry Baryshkov wrote:
>>>>> On 04/02/2023 04:43, Abhinav Kumar wrote:
>>>>>>
>>>>>>
>>>>>> On 2/3/2023 6:29 PM, Dmitry Baryshkov wrote:
>>>>>>> On 04/02/2023 01:35, Abhinav Kumar wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2/3/2023 10:21 AM, Dmitry Baryshkov wrote:
>>>>>>>>> Downstream driver uses dpu->caps->smart_dma_rev to update
>>>>>>>>> sspp->cap->features with the bit corresponding to the supported 
>>>>>>>>> SmartDMA
>>>>>>>>> version. Upstream driver does not do this, resulting in SSPP 
>>>>>>>>> subdriver
>>>>>>>>> not enbaling setup_multirect callback. Add corresponding 
>>>>>>>>> SmartDMA SSPP
>>>>>>>>> feature bits to dpu hw catalog.
>>>>>>>>>
>>>>>>>>
>>>>>>>> While reviewing this patch, I had a first hand experience of how 
>>>>>>>> we are reusing SSPP bitmasks for so many chipsets but I think 
>>>>>>>> overall you got them right here :)
>>>>>>>>
>>>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>>>>>>>> ---
>>>>>>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 10 +++++++---
>>>>>>>>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c 
>>>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>>>>>>> index cf053e8f081e..fc818b0273e7 100644
>>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>>>>>>> @@ -21,13 +21,16 @@
>>>>>>>>>       (VIG_MASK | BIT(DPU_SSPP_SCALER_QSEED3))
>>>>>>>>>   #define VIG_SDM845_MASK \
>>>>>>>>> -    (VIG_MASK | BIT(DPU_SSPP_QOS_8LVL) | 
>>>>>>>>> BIT(DPU_SSPP_SCALER_QSEED3))
>>>>>>>>> +    (VIG_MASK | BIT(DPU_SSPP_QOS_8LVL) | 
>>>>>>>>> BIT(DPU_SSPP_SCALER_QSEED3) |\
>>>>>>>>> +    BIT(DPU_SSPP_SMART_DMA_V2))
>>>>>>>>>   #define VIG_SC7180_MASK \
>>>>>>>>> -    (VIG_MASK | BIT(DPU_SSPP_QOS_8LVL) | 
>>>>>>>>> BIT(DPU_SSPP_SCALER_QSEED4))
>>>>>>>>> +    (VIG_MASK | BIT(DPU_SSPP_QOS_8LVL) | 
>>>>>>>>> BIT(DPU_SSPP_SCALER_QSEED4) |\
>>>>>>>>> +    BIT(DPU_SSPP_SMART_DMA_V2))
>>>>>>>>>   #define VIG_SM8250_MASK \
>>>>>>>>> -    (VIG_MASK | BIT(DPU_SSPP_QOS_8LVL) | 
>>>>>>>>> BIT(DPU_SSPP_SCALER_QSEED3LITE))
>>>>>>>>> +    (VIG_MASK | BIT(DPU_SSPP_QOS_8LVL) | 
>>>>>>>>> BIT(DPU_SSPP_SCALER_QSEED3LITE) |\
>>>>>>>>> +    BIT(DPU_SSPP_SMART_DMA_V2))
>>>>>>>>>   #define VIG_QCM2290_MASK (VIG_MASK | BIT(DPU_SSPP_QOS_8LVL))
>>>>>>>>> @@ -42,6 +45,7 @@
>>>>>>>>>   #define DMA_SDM845_MASK \
>>>>>>>>>       (BIT(DPU_SSPP_SRC) | BIT(DPU_SSPP_QOS) | 
>>>>>>>>> BIT(DPU_SSPP_QOS_8LVL) |\
>>>>>>>>>       BIT(DPU_SSPP_TS_PREFILL) | BIT(DPU_SSPP_TS_PREFILL_REC1) |\
>>>>>>>>> +    BIT(DPU_SSPP_SMART_DMA_V2) |\
>>>>>>>>>       BIT(DPU_SSPP_CDP) | BIT(DPU_SSPP_EXCL_RECT))
>>>>>>>>>   #define DMA_CURSOR_SDM845_MASK \
>>>>>>>>
>>>>>>>> VIG_SDM845_MASK and DMA_SDM845_MASK are used for many other 
>>>>>>>> chipsets like 8250, 8450, 8550.
>>>>>>>>
>>>>>>>> At the moment, for visual validation of this series, I only have 
>>>>>>>> sc7180/sc7280. We are leaving the rest for CI.
>>>>>>>>
>>>>>>>> Was that an intentional approach?
>>>>>>>>
>>>>>>>> If so, we will need tested-by tags from folks having 
>>>>>>>> 8350/8450/8550/sc8280x,qcm2290?
>>>>>>>>
>>>>>>>> I am only owning the visual validation on sc7280 atm.
>>>>>>>
>>>>>>> I'm not quite sure what is your intent here. Are there any SoCs 
>>>>>>> after 845 that do not have SmartDMA 2.5? Or do you propose to 
>>>>>>> enable SmartDMA only for the chipsets that we can visually test? 
>>>>>>> That sounds strange.
>>>>>>>
>>>>>>
>>>>>> Yes I was thinking to enable smartDMA at the moment on chipsets 
>>>>>> which we can validate visually that display comes up. But I am not 
>>>>>> sure if thats entirely practical.
>>>>>>
>>>>>> But the intent was I just want to make sure basic display does 
>>>>>> come up with smartDMA enabled if we are enabling it for all chipsets.
>>>>>
>>>>> I don't think it is practical or logical. We don't require 
>>>>> validating other changes on all possible chipsets, so what is so 
>>>>> different with this one?
>>>>>
>>>>
>>>> Thats because with smartDMA if the programming of stages goes wrong 
>>>> we could potentially just see a blank screen. Its not about other 
>>>> changes, this change in particular controls enabling a feature.
>>>>
>>>> But thats just my thought. I am not going to request to ensure this 
>>>> or block this for this.
>>>>
>>>> You can still have my
>>>>
>>>> Reviewed-by: Abhinav Kumar <quic_abhinavk at quicinc.com>
>>>>
>>>> But think of the validations that have to be done before we merge it.
>>>
>>> The usual way: verify as much as feasible and let anybody else 
>>> complain during the development cycle.
>>>
>>
>> Well, our perspective is to enable the feature on devices on which you 
>> are able to test and not enable then wait for others to complain.
> 
> This would not be really practical. There are plenty of people who can 
> test things on obscure platforms, but unfortunately far less amount of 
> people who tightly follow the development and can track which new 
> feature applies to a particular platform. I hope to be able to fix that 
> slightly with the hw catalog rework. However enabling features on other 
> platforms definitely requires more knowledge than simply testing the 
> kernel.
> 
>>
>> I did not say test all devices. My point was to enable smartDMA on 
>> devices which we are able to test.
>>
>> There are other examples of this, like inline rotation, writeback etc. 
>> which are at the moment enabled only on devices which QC or others 
>> have tested on.
> 
> But at the time it was added, inline rotation 2.0 could only be 
> supported on sc7280. Probably we should expand it not to sc8280xp and 
> sm8[345]50.
> 
> For WB I don't remember which platforms were supported at the moment it 
> was added. But it's also worth expanding support to new platforms.
> 
> And, as we speak about testing, is there an easy way to setup the plane 
> with UBWC format modifier? Also, did the WB support patches land into 
> libdrm?
> 

I will check the compositor code and update you on the UWBC format 
modifier as I am not too familiar with it.

libdrm always supported virtual encoder 
https://github.com/grate-driver/libdrm/blob/master/include/drm/drm_mode.h#L352

What other support patches are needed? Right now we only use IGT to 
validate writeback.

>> So when i said my suggestion was not practical, yes because if you 
>> want to go ahead with this change in the current form, you would have 
>> to validate all the chipsets as you are enabling smartDMA on all of them.
>>
>> If you enable smartDMA only on the chipsets you OR others can validate 
>> and give Tested-by for like I was planning to do for sc7280, then I am 
>> not sure why it doesnt sound logical.
>>
>> But like I said, thats my perspective. I will let you decide as you 
>> would know how confident you are with this getting enabled for all 
>> chipsets upstream.
> 
> I'd say, that once tested on some of the platforms and granted that even 
> smalled (qcm2290, sm6115) platforms support smartdma, it will be safe to 
> enable smart DMA globablly for every SoC >= sdm845. If I remember 
> correctly, msm8998 (and sdm660/630) support smartdma/rect only on DMA 
> planes. Is it correct?
> 
> 
Yes thats right msm8998 supports smartdma only on DMA sspps.