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

[Abhinav Kumar]

On 2/4/2023 4:29 PM, Dmitry Baryshkov wrote:
> On Sun, 5 Feb 2023 at 01:20, Abhinav Kumar <quic_abhinavk at quicinc.com> wrote:
>>
>>
>>
>> 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.
> 
> Ideally it would be nice to support ubwc planes in some simple tool,
> e.g. modetest.
> 
>>
>> 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.
> 
> I remember there was a patchset to make modeset to support using
> writeback. What was its fate?
> 

Once our intern finished his internship, noone could take up the pending 
review comments after that so its yet to be merged.

https://patchwork.kernel.org/project/dri-devel/list/?series=667290&archive=both

Once more item to the to-do list.

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