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

[Dmitry Baryshkov]

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.

-- 
With best wishes
Dmitry