On Wed, Dec 24, 2025 at 11:10:35AM +0100, Marijn Suijten wrote:
> On 2025-11-21 14:02:08, Teguh Sobirin wrote:
> > Since DPU 5.x the vsync source TE setup is split between MDP TOP and
> > INTF blocks. Currently all code to setup vsync_source is only exectued
>
> typo: executed.
>
> > if MDP TOP implements the setup_vsync_source() callback. However on
> > DPU >= 8.x this callback is not implemented, making DPU driver skip all
> > vsync setup. Move the INTF part out of this condition, letting DPU
> > driver to setup TE vsync selection on all new DPU devices.
> >
> > Signed-off-by: Teguh Sobirin <[email protected]>
> > ---
> > Changes in v2:
> > - Corrected commit message suggested by Dmitry Baryshkov.
> > - Link to v1:
> > https://lore.kernel.org/linux-arm-msm/tyupr06mb6099cbbe5090db12a2c187e3dd...@tyupr06mb6099.apcprd06.prod.outlook.com/
> > ---
> > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 22 +++++++++------------
> > 1 file changed, 9 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index d1cfe81a3373..f468d054f5bd 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -774,24 +774,20 @@ static void _dpu_encoder_update_vsync_source(struct
> > dpu_encoder_virt *dpu_enc,
> > return;
> > }
> >
> > + /* Set vsync source irrespective of mdp top support */
>
> Unnecessary comment, it's clear from the code flow that vsync_cfg.vsync_source
> is passed down into both functions.
>
> Perhaps we should pass disp_info->vsync_source directly into
> hw_intf->ops.vsync_sel() and only initialize vsync_cfg when hw_mdptop has the
> function, to make this clear.
No, because on DPU 8.0+ WD is also setup per intf.
>
> > + vsync_cfg.vsync_source = disp_info->vsync_source;
> > +
> > if (hw_mdptop->ops.setup_vsync_source) {
> > for (i = 0; i < dpu_enc->num_phys_encs; i++)
> > vsync_cfg.ppnumber[i] = dpu_enc->hw_pp[i]->idx;
> > + }
> >
> > - vsync_cfg.pp_count = dpu_enc->num_phys_encs;
>
> This change is not mentioned in the commit description. While true that
> pp_count is only used by dpu_hw_setup_vsync_sel(), that is still a valid
> function to be called on DPU < 5; it denotes the amount of ppnumber[i] array
> entries are used above.
>
> > - vsync_cfg.frame_rate =
> > drm_mode_vrefresh(&dpu_enc->base.crtc->state->adjusted_mode);
>
> Same, also not mentioned in the commit description. frame_rate
> is used by dpu_hw_setup_wd_timer() on DPU < 8.
>
> > -
> > - vsync_cfg.vsync_source = disp_info->vsync_source;
> > -
> > - hw_mdptop->ops.setup_vsync_source(hw_mdptop, &vsync_cfg);
>
> But all of the above comments don't matter if the call to setup_vsync_source()
> is removed entirely - it didn't move anywhere else. This is not described in
> the commit message.
Fun, I missed it earlier. Thanks for pointing it out.
Teguh, since we need to fix watchdog on MDP 8.0+, I'll send v3 on my
own, if you don't mind.
>
> - Marijn
>
> > -
> > - for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> > - phys_enc = dpu_enc->phys_encs[i];
> > + for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> > + phys_enc = dpu_enc->phys_encs[i];
> >
> > - if (phys_enc->has_intf_te &&
> > phys_enc->hw_intf->ops.vsync_sel)
> > -
> > phys_enc->hw_intf->ops.vsync_sel(phys_enc->hw_intf,
> > - vsync_cfg.vsync_source);
> > - }
> > + if (phys_enc->has_intf_te && phys_enc->hw_intf->ops.vsync_sel)
> > + phys_enc->hw_intf->ops.vsync_sel(phys_enc->hw_intf,
> > + vsync_cfg.vsync_source);
> > }
> > }
> >
> > --
> > 2.34.1
> >
--
With best wishes
Dmitry
