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@quicinc.com>
Signed-off-by: Jessica Zhang <quic_jesszhan@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(-)

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@quicinc.com>

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@quicinc.com>
> Signed-off-by: Jessica Zhang <quic_jesszhan@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(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

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@quicinc.com>
> Signed-off-by: Jessica Zhang <quic_jesszhan@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]

[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@quicinc.com>
> 

-- 
With best wishes
Dmitry

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@quicinc.com>
>> Signed-off-by: Jessica Zhang <quic_jesszhan@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 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@quicinc.com>
>>
>

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@quicinc.com>
> > > Signed-off-by: Jessica Zhang <quic_jesszhan@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.

> 
> 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@quicinc.com>
> > > 
> > 

-- 
With best wishes
Dmitry

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@quicinc.com>
>>>> Signed-off-by: Jessica Zhang <quic_jesszhan@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@quicinc.com>
>>>>
>>>
> 
> -- 
> With best wishes
> Dmitry

On Mon, Oct 14, 2024 at 01:23:24PM -0700, Jessica Zhang wrote:
> 
> 
> 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@quicinc.com>
> > > > > Signed-off-by: Jessica Zhang <quic_jesszhan@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.

I had a different understanding of them, but after going through the
patches second time, you are right.

-- 
With best wishes
Dmitry