> > On 24 Dec 2025, at 21.03, Dmitry Baryshkov > <[email protected]> wrote: > > 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 Yes please, thank you.
Regards, Teguh S.
