[PATCH v2] drm/msm/dpu: don't always activate merge_3d block

[Jessica Zhang]

On 10/14/2024 12:13 AM, Dmitry Baryshkov wrote:
> On Sun, Oct 13, 2024 at 07:37:20PM -0700, Abhinav Kumar wrote:
>> Hi Dmitry
>>
>> On 10/13/2024 5:20 PM, Dmitry Baryshkov wrote:
>>> On Fri, Oct 11, 2024 at 10:25:13AM -0700, Jessica Zhang wrote:
>>>> Only enable the merge_3d block for the video phys encoder when the 3d
>>>> blend mode is not *_NONE since there is no need to activate the merge_3d
>>>> block for cases where merge_3d is not needed.
>>>>
>>>> Fixes: 3e79527a33a8 ("drm/msm/dpu: enable merge_3d support on sm8150/sm8250")
>>>> Suggested-by: Abhinav Kumar <quic_abhinavk at quicinc.com>
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan at quicinc.com>
>>>> ---
>>>> Changes in v2:
>>>> - Added more detailed commit message
>>>> - Link to v1: https://lore.kernel.org/r/20241009-merge3d-fix-v1-1-0d0b6f5c244e@quicinc.com
>>>> ---
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> LGTM now. Please clarify, is there any dependency between this patch and
>>> [1]
>>>
>>
>> No dependency as such. Both are tackling similar issues though. One for
>> video mode and the other for writeback thats all. Namely:
>>
>> 1) We should not be enabling merge_3d block if two LMs are not being used as
>> that block needs to be enabled only to merge two streams. If its always
>> enabled, its incorrect programming because as per the docs its mentioned "if
>> required". Even if thats not causing issues, I would prefer not to enable it
>> always due to the "if required" clause and also we dont need to enable a
>> hardware sub-block unnecessarily.
>>
>> 2) We should be flushing the merge_3d only if its active like Jessica wrote
>> in the commit message of [1]. Otherwise, the flush bit will never be taken
>> by hardware leading to the false timeout errors.
>>
>> It has been sent as two patches as one is for video mode and the other for
>> writeback and for writeback it includes both (1) and (2) together in the
>> same patch.
> 
> I think it's better to handle (1) in a single patch (both for video and
> WB) and (2) in another patch. This way it becomes more obvious that WB
> had two different independent issues issues.

Hi Dmitry,

Just to clarify, the patches are already being split this way.

Thanks,

Jessica Zhang

> 
>>
>> I thought this separation is fine, if we need to squash it, let me know.
>>
>> Thanks
>>
>> Abhinav
>>
>>> [1] https://lore.kernel.org/dri-devel/20241009-mode3d-fix-v1-1-c0258354fadc@quicinc.com/
>>>
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>>> index ba8878d21cf0e1945a393cca806cb64f03b16640..c5e27eeaff0423a69fad98122ffef7e041fbc68e 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
>>>> @@ -302,7 +302,7 @@ static void dpu_encoder_phys_vid_setup_timing_engine(
>>>>    	intf_cfg.stream_sel = 0; /* Don't care value for video mode */
>>>>    	intf_cfg.mode_3d = dpu_encoder_helper_get_3d_blend_mode(phys_enc);
>>>>    	intf_cfg.dsc = dpu_encoder_helper_get_dsc(phys_enc);
>>>> -	if (phys_enc->hw_pp->merge_3d)
>>>> +	if (intf_cfg.mode_3d && phys_enc->hw_pp->merge_3d)
>>>>    		intf_cfg.merge_3d = phys_enc->hw_pp->merge_3d->idx;
>>>>    	spin_lock_irqsave(phys_enc->enc_spinlock, lock_flags);
>>>>
>>>> ---
>>>> base-commit: a20a91fb1bfac5d05ec5bcf9afe0c9363f6c8c93
>>>> change-id: 20240828-merge3d-fix-1a8d005e3277
>>>>
>>>> Best regards,
>>>> -- 
>>>> Jessica Zhang <quic_jesszhan at quicinc.com>
>>>>
>>>
> 
> -- 
> With best wishes
> Dmitry