Re: [PATCH 1/7] drm/msm/dpu: enable PINGPONG TE operations only when supported by HW

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 29/07/2023 21:31, Marijn Suijten wrote:
> On 2023-07-29 02:59:32, Dmitry Baryshkov wrote:
>> On 27/07/2023 23:03, Marijn Suijten wrote:
>>> On 2023-07-27 19:20:58, Dmitry Baryshkov wrote:
>>>> The DPU_PINGPONG_TE bit is set for all PINGPONG blocks on DPU < 5.0.
>>>> Rather than checking for the flag, check for the presense of the
>>>> corresponding interrupt line.
>>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>
>>> That's a smart use of the interrupt field.  I both like it, and I do
>>> not.  While we didn't do any validation for consistency previously, this
>>> means we now have multiple ways of controlling available "features":
>>>
>>> - Feature flags on hardware blocks;
>>> - Presence of certain IRQs;
>>> - DPU core revision.
>>
>> I hesitated here too. For INTF it is clear that there is no other good
>> way to check for the TE feature. For PP we can just check for the DPU
>> revision.
> 
> For both we could additionally check the DPU revision, and for INTF we
> could check for TYPE_DSI.  Both would aid in extra validation, if we
> require the IRQ to be present or absent under these conditions.

Yep, maybe that's better.

> 
> It might also help to document this, either here and on the respective
> struct fields so that catalog implementers know when they should supply
> or leave out an IRQ?

Probably a WARN_ON would be enough.

> 
> - Marijn
> 
>>> Maybe that is more confusing to follow?  Regardless of that I'm
>>> convinced that this patch does what it's supposed to and gets rid of
>>> some ambiguity.  Maybe a comment above the IF explaining the "PP TE"
>>> feature could alleviate the above concerns thoo.  Hence:
>>>
>>> Reviewed-by: Marijn Suijten <marijn.suijten@somainline.org>
>>>
>>>> ---
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
>>>> index 9298c166b213..912a3bdf8ad4 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
>>>> @@ -296,7 +296,7 @@ struct dpu_hw_pingpong *dpu_hw_pingpong_init(const struct dpu_pingpong_cfg *cfg,
>>>>    	c->idx = cfg->id;
>>>>    	c->caps = cfg;
>>>>    
>>>> -	if (test_bit(DPU_PINGPONG_TE, &cfg->features)) {
>>>> +	if (cfg->intr_rdptr) {
>>>>    		c->ops.enable_tearcheck = dpu_hw_pp_enable_te;
>>>>    		c->ops.disable_tearcheck = dpu_hw_pp_disable_te;
>>>>    		c->ops.connect_external_te = dpu_hw_pp_connect_external_te;
>>>> -- 
>>>> 2.39.2
>>>>
>>
>> -- 
>> With best wishes
>> Dmitry
>>

-- 
With best wishes
Dmitry