linux-kernel - Re: [PATCH v9 01/14] drm/atomic-helper: Add crtc check before checking plane

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <6cfc706f-4909-4121-9849-a37e4769ab2f@oss.qualcomm.com>
Date: Fri, 9 May 2025 18:18:40 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
To: Jun Nie <jun.nie@...aro.org>
Cc: Rob Clark <robdclark@...il.com>,
        Abhinav Kumar
 <quic_abhinavk@...cinc.com>,
        Sean Paul <sean@...rly.run>,
        Marijn Suijten <marijn.suijten@...ainline.org>,
        David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
        Jessica Zhang <quic_jesszhan@...cinc.com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>, linux-arm-msm@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v9 01/14] drm/atomic-helper: Add crtc check before
 checking plane

On 09/05/2025 06:08, Jun Nie wrote:
> Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com> 于2025年5月8日周四 18:47写道：
>>
>> On Tue, May 06, 2025 at 11:47:31PM +0800, Jun Nie wrote:
>>> Some display controller support flexible CRTC and DMA, such as the display
>>> controllers in snapdragon SoCs. CRTC can be implemented with several mixers
>>> in parallel, and plane fetching can be implemented with several DMA under
>>> umberala of a virtual drm plane.
>>>
>>> The mixer number is decided per panel resolution and clock rate constrain
>>> first, which happens in CRTC side. Then plane is split per mixer number
>>> and configure DMA accordingly.
>>
>> Here you are describing a behaviour of one particular driver as a reason
>> to change the framework.
> 
> Yeah, the specific driver requires a change in framework. Maybe the
> comment is not
> proper?

Yes. Explain how does that benefit the framework / other drivers. 
Otherwise the answer would be as simple as 'replace 
drm_atomic_helper_check_planes() in your driver'.

>>
>>>
>>> To support such forthcoming usage case, CRTC checking shall happen before
>>> checking plane. Add the checking in the drm_atomic_helper_check_modeset().
>>
>> So, now drivers will get two calls to atomic_check(), one coming in
>> circumstances which were not expected by the drivers before. Are you
>> sure that this won't break anything?
> 
> Yes, it is a concern. Is there any way to limit the change in
> framework to specific
> driver with a flag, such as DRM_FLAG_CHECK_CRTC_BEFORE_PLANE?

Definitely not with a flag. You can try adding a new helper callback, 
but I don't know how DRM core maintainers would react to it.

>>
>>>
>>> Signed-off-by: Jun Nie <jun.nie@...aro.org>
>>> ---
>>>   drivers/gpu/drm/drm_atomic_helper.c | 19 +++++++++++++++++++
>>>   1 file changed, 19 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>> index 5302ab3248985d3e0a47e40fd3deb7ad0d9f775b..5bca4c9683838c38574c8cb7c0bc9d57960314fe 100644
>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>> @@ -816,6 +816,25 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>>>                        return ret;
>>>        }
>>>
>>> +     for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
>>> +             const struct drm_crtc_helper_funcs *funcs;
>>> +
>>> +             funcs = crtc->helper_private;
>>> +
>>> +             if (!funcs || !funcs->atomic_check)
>>> +                     continue;
>>> +
>>> +             ret = funcs->atomic_check(crtc, state);
>>> +             if (ret) {
>>> +                     drm_dbg_atomic(crtc->dev,
>>> +                                    "[CRTC:%d:%s] atomic driver check failed\n",
>>> +                                    crtc->base.id, crtc->name);
>>> +                     return ret;
>>> +             }
>>> +     }
>>> +
>>> +
>>> +
>>
>> Too many empty lines. But the main quesiton is: why are you calling it
>> before mode_valid()? According to your description a better place would
>> be in drm_atomic_helper_check_planes().
>>
> Agree, that's the proper function. Will remove the empty line in next version.
> 
>>>        ret = mode_valid(state);
>>>        if (ret)
>>>                return ret;
>>>
>>> --
>>> 2.34.1
>>>
>>
>> --
>> With best wishes
>> Dmitry
>>


-- 
With best wishes
Dmitry