diff mbox series

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

Message ID 20230203182132.1307834-19-dmitry.baryshkov@linaro.org
State New
Headers show
Series [v3,01/27] drm/msm/dpu: rename struct dpu_hw_pipe(_cfg) to dpu_hw_sspp(_cfg) | expand

Commit Message

Dmitry Baryshkov Feb. 3, 2023, 6:21 p.m. UTC
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.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Abhinav Kumar Feb. 3, 2023, 11:35 p.m. UTC | #1
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@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.
Dmitry Baryshkov Feb. 4, 2023, 2:29 a.m. UTC | #2
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@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.
Abhinav Kumar Feb. 4, 2023, 2:43 a.m. UTC | #3
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@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.
Dmitry Baryshkov Feb. 4, 2023, 4:10 a.m. UTC | #4
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@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?
Abhinav Kumar Feb. 4, 2023, 5:10 a.m. UTC | #5
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@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@quicinc.com>

But think of the validations that have to be done before we merge it.
Dmitry Baryshkov Feb. 4, 2023, 10:43 a.m. UTC | #6
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@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@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.
Abhinav Kumar Feb. 4, 2023, 6:35 p.m. UTC | #7
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@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@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.

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.

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.
Dmitry Baryshkov Feb. 4, 2023, 9:08 p.m. UTC | #8
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@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@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?

> 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?
Abhinav Kumar Feb. 4, 2023, 11:20 p.m. UTC | #9
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@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@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.
Dmitry Baryshkov Feb. 5, 2023, 12:29 a.m. UTC | #10
On Sun, 5 Feb 2023 at 01:20, Abhinav Kumar <quic_abhinavk@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@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@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?

>
> >> 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
Dmitry Baryshkov Feb. 8, 2023, 11:53 p.m. UTC | #11
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@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@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@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
>>
diff mbox series

Patch

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 \