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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID:
 <TYUPR06MB60994CC9F0097F95C6E51B2EDDB2A@TYUPR06MB6099.apcprd06.prod.outlook.com>
Date: Wed, 24 Dec 2025 13:30:35 +0000
From: Teguh Sobirin <teguh@...ir.in>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
CC: Marijn Suijten <marijn.suijten@...ainline.org>, Rob Clark
	<robin.clark@....qualcomm.com>, Dmitry Baryshkov <lumag@...nel.org>, Abhinav
 Kumar <abhinav.kumar@...ux.dev>, Jessica Zhang <jesszhan0024@...il.com>, Sean
 Paul <sean@...rly.run>, David Airlie <airlied@...il.com>, Simona Vetter
	<simona@...ll.ch>, "linux-arm-msm@...r.kernel.org"
	<linux-arm-msm@...r.kernel.org>, "dri-devel@...ts.freedesktop.org"
	<dri-devel@...ts.freedesktop.org>, "freedreno@...ts.freedesktop.org"
	<freedreno@...ts.freedesktop.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] drm/msm/dpu: Set vsync source irrespective of mdp top
 support


>
> On 24 Dec 2025, at 21.03, Dmitry Baryshkov <dmitry.baryshkov@....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@...ir.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.