Re: [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@xxxxxxxxxxx>
> > > > Signed-off-by: Jessica Zhang <quic_jesszhan@xxxxxxxxxxx>
> > > > ---
> > > > Changes in v2:
> > > > - Added more detailed commit message
> > > > - Link to v1: https://lore.kernel.org/r/20241009-merge3d-fix-v1-1-0d0b6f5c244e@xxxxxxxxxxx
> > > > ---
> > > >    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@xxxxxxxxxxx/
> > >
> > > > 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@xxxxxxxxxxx>
>
> --
> With best wishes
> Dmitry