[<prev] [next>] [<thread-prev] [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
Powered by blists - more mailing lists