[PATCH v2] drm/msm/dpu: Set vsync source irrespective of mdp top support

[PATCH v2] drm/msm/dpu: Set vsync source irrespective of mdp top support

[Teguh Sobirin]

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
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 <teguh@sobir.in>
---
Changes in v2:
- Corrected commit message suggested by Dmitry Baryshkov.
- Link to v1: https://lore.kernel.org/linux-arm-msm/TYUPR06MB6099CBBE5090DB12A2C187E3DDFDA@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 */
+	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;
-		vsync_cfg.frame_rate = drm_mode_vrefresh(&dpu_enc->base.crtc->state->adjusted_mode);
-
-		vsync_cfg.vsync_source = disp_info->vsync_source;
-
-		hw_mdptop->ops.setup_vsync_source(hw_mdptop, &vsync_cfg);
-
-		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

Re: [PATCH v2] drm/msm/dpu: Set vsync source irrespective of mdp top support

[Dmitry Baryshkov]

On Fri, Nov 21, 2025 at 02:02:08PM +0800, 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
> 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 <teguh@sobir.in>
> ---
> Changes in v2:
> - Corrected commit message suggested by Dmitry Baryshkov.
> - Link to v1: https://lore.kernel.org/linux-arm-msm/TYUPR06MB6099CBBE5090DB12A2C187E3DDFDA@TYUPR06MB6099.apcprd06.prod.outlook.com/
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 22 +++++++++------------
>  1 file changed, 9 insertions(+), 13 deletions(-)

Fixes: e955a3f0d86e ("drm/msm/dpu: Implement tearcheck support on INTF block")


Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>


-- 
With best wishes
Dmitry

Re: [PATCH v2] drm/msm/dpu: Set vsync source irrespective of mdp top support

[Dmitry Baryshkov]

On Fri, 21 Nov 2025 14:02:08 +0800, 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
> 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.
> 
> [...]

Applied to msm-fixes, thanks!

[1/1] drm/msm/dpu: Set vsync source irrespective of mdp top support
      https://gitlab.freedesktop.org/lumag/msm/-/commit/0a7836e1db9c

Best regards,
-- 
With best wishes
Dmitry

Re: [PATCH v2] drm/msm/dpu: Set vsync source irrespective of mdp top support

[Marijn Suijten]

On 2025-11-21 14:16:55, Dmitry Baryshkov wrote:
> On Fri, Nov 21, 2025 at 02:02:08PM +0800, 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
> > 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 <teguh@sobir.in>
> > ---
> > Changes in v2:
> > - Corrected commit message suggested by Dmitry Baryshkov.
> > - Link to v1: https://lore.kernel.org/linux-arm-msm/TYUPR06MB6099CBBE5090DB12A2C187E3DDFDA@TYUPR06MB6099.apcprd06.prod.outlook.com/
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 22 +++++++++------------
> >  1 file changed, 9 insertions(+), 13 deletions(-)
> 
> Fixes: e955a3f0d86e ("drm/msm/dpu: Implement tearcheck support on INTF block")

Back at that commit, setup_vsync_source was still always assigned:

https://github.com/torvalds/linux/blob/e955a3f0d86e4a789364af62495ac37546eb71c8/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c#L260-L263

While that was technically a wrong assumption by me - I should've put this loop
outside the hw_mdptop check in case that ever happened, it got unassigned in
commit 2f69e5458447 ("drm/msm/dpu: skip watchdog timer programming through TOP
on >= SM8450").

Note that this patch is a severe regression on DPU <5 and doesn't set up the WD
timer on DPU [5, 8), but I'll comment in-line on the diff.

- Marijn

Re: [PATCH v2] drm/msm/dpu: Set vsync source irrespective of mdp top support

[Marijn Suijten]

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 <teguh@sobir.in>
> ---
> Changes in v2:
> - Corrected commit message suggested by Dmitry Baryshkov.
> - Link to v1: https://lore.kernel.org/linux-arm-msm/TYUPR06MB6099CBBE5090DB12A2C187E3DDFDA@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.

> +	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.

- 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
> 

Re: [PATCH v2] drm/msm/dpu: Set vsync source irrespective of mdp top support

[Dmitry Baryshkov]

On Wed, Dec 24, 2025 at 11:00:23AM +0100, Marijn Suijten wrote:
> On 2025-11-21 14:16:55, Dmitry Baryshkov wrote:
> > On Fri, Nov 21, 2025 at 02:02:08PM +0800, 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
> > > 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 <teguh@sobir.in>
> > > ---
> > > Changes in v2:
> > > - Corrected commit message suggested by Dmitry Baryshkov.
> > > - Link to v1: https://lore.kernel.org/linux-arm-msm/TYUPR06MB6099CBBE5090DB12A2C187E3DDFDA@TYUPR06MB6099.apcprd06.prod.outlook.com/
> > > ---
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 22 +++++++++------------
> > >  1 file changed, 9 insertions(+), 13 deletions(-)
> > 
> > Fixes: e955a3f0d86e ("drm/msm/dpu: Implement tearcheck support on INTF block")
> 
> Back at that commit, setup_vsync_source was still always assigned:
> 
> https://github.com/torvalds/linux/blob/e955a3f0d86e4a789364af62495ac37546eb71c8/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_top.c#L260-L263
> 
> While that was technically a wrong assumption by me - I should've put this loop
> outside the hw_mdptop check in case that ever happened, it got unassigned in
> commit 2f69e5458447 ("drm/msm/dpu: skip watchdog timer programming through TOP
> on >= SM8450").

Fair enough,

Fixes: 2f69e5458447 ("drm/msm/dpu: skip watchdog timer programming through TOP on >= SM8450")

> 
> Note that this patch is a severe regression on DPU <5 and doesn't set up the WD
> timer on DPU [5, 8), but I'll comment in-line on the diff.
> 
> - Marijn

-- 
With best wishes
Dmitry

Re: [PATCH v2] drm/msm/dpu: Set vsync source irrespective of mdp top support

[Dmitry Baryshkov]

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 <teguh@sobir.in>
> > ---
> > Changes in v2:
> > - Corrected commit message suggested by Dmitry Baryshkov.
> > - Link to v1: https://lore.kernel.org/linux-arm-msm/TYUPR06MB6099CBBE5090DB12A2C187E3DDFDA@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

Re: [PATCH v2] drm/msm/dpu: Set vsync source irrespective of mdp top support

[Teguh Sobirin]

>
> On 24 Dec 2025, at 21.03, Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com> 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 <teguh@sobir.in>
>>> ---
>>> Changes in v2:
>>> - Corrected commit message suggested by Dmitry Baryshkov.
>>> - Link to v1: https://lore.kernel.org/linux-arm-msm/TYUPR06MB6099CBBE5090DB12A2C187E3DDFDA@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.