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

Dmitry Baryshkov dmitry.baryshkov at linaro.org
Wed Feb 8 23:53:28 UTC 2023 

On 05/02/2023 02:36, Abhinav Kumar wrote:
> 
> 
> 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.

Oh. Quite a pity :-(
I hope somebody can pick it up on either of our sides.

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

-- 
With best wishes
Dmitry

More information about the dri-devel mailing list