linux-kernel - Re: [PATCH v4 10/11] drm/atomic-helper: Re-order bridge chain pre-enable and post-disable

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: <c5ad23fb-b517-435a-8002-bcbff9e312c0@ti.com>
Date: Thu, 26 Dec 2024 19:44:46 +0530
From: Devarsh Thakkar <devarsht@...com>
To: Aradhya Bhatia <a-bhatia1@...com>, Maxime Ripard <mripard@...nel.org>,
        Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
CC: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Andrzej Hajda
	<andrzej.hajda@...el.com>,
        Neil Armstrong <neil.armstrong@...aro.org>,
        "Robert Foss" <rfoss@...nel.org>,
        Laurent Pinchart
	<Laurent.pinchart@...asonboard.com>,
        Jonas Karlman <jonas@...boo.se>,
        "Jernej
 Skrabec" <jernej.skrabec@...il.com>,
        Maarten Lankhorst
	<maarten.lankhorst@...ux.intel.com>,
        Jyri Sarha <jyri.sarha@....fi>,
        "Thomas
 Zimmermann" <tzimmermann@...e.de>,
        David Airlie <airlied@...il.com>, "Daniel
 Vetter" <daniel@...ll.ch>,
        DRI Development List
	<dri-devel@...ts.freedesktop.org>,
        Linux Kernel List
	<linux-kernel@...r.kernel.org>,
        Dominik Haller <d.haller@...tec.de>,
        "Sam
 Ravnborg" <sam@...nborg.org>,
        Thierry Reding <treding@...dia.com>,
        "Kieran
 Bingham" <kieran.bingham+renesas@...asonboard.com>,
        Nishanth Menon
	<nm@...com>, Vignesh Raghavendra <vigneshr@...com>,
        Praneeth Bajjuri
	<praneeth@...com>, Udit Kumar <u-kumar1@...com>,
        Jayesh Choudhary
	<j-choudhary@...com>, Jai Luthra <j-luthra@...com>
Subject: Re: [PATCH v4 10/11] drm/atomic-helper: Re-order bridge chain
 pre-enable and post-disable

Hi Aradhya, Tomi,

Thanks for the patch Aradhya.

On 11/07/24 13:02, Aradhya Bhatia wrote:
> On 26/06/24 18:37, Maxime Ripard wrote:
>> On Wed, Jun 26, 2024 at 02:28:57PM GMT, Tomi Valkeinen wrote:
>>> On 22/06/2024 14:09, Aradhya Bhatia wrote:
>>>> Move the bridge pre_enable call before crtc enable, and the bridge
>>>> post_disable call after the crtc disable.
>>>>
>>>> The sequence of enable after this patch will look like:
>>>>
>>>> 	bridge[n]_pre_enable
>>>> 	...
>>>> 	bridge[1]_pre_enable
>>>>
>>>> 	crtc_enable
>>>> 	encoder_enable
>>>>
>>>> 	bridge[1]_enable
>>>> 	...
>>>> 	bridge[n]__enable
>>>>

>>> and vice-versa for the bridge chain disable sequence.

Could you please add the disable sequence too (including the
post_disable hook in the sequence)?

>>>>
>>>> The definition of bridge pre_enable hook says that,
>>>> "The display pipe (i.e. clocks and timing signals) feeding this bridge
>>>> will not yet be running when this callback is called".
>>>>
>>>> Since CRTC is also a source feeding the bridge, it should not be enabled
>>>> before the bridges in the pipeline are pre_enabled. Fix that by
>>>> re-ordering the sequence of bridge pre_enable and bridge post_disable.
>>>>
>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@...com>
>>>> ---
>>>>   drivers/gpu/drm/drm_atomic_helper.c | 165 ++++++++++++++++++----------
>>>>   include/drm/drm_atomic_helper.h     |   7 ++
>>>>   2 files changed, 114 insertions(+), 58 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>>> index fb97b51b38f1..e8ad08634f58 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c

<snip>
>>> I don't like the enum... Having "enum bridge_chain_operation_type" as a
>>> parameter to a function looks like one can pass any of the enum's values,
>>> which is not the case.
>>>
>>> How about an enum with just two values:
>>>
>>> DRM_BRIDGE_PRE_ENABLE_POST_DISABLE

This sounds like it is being used to do both pre-enable and post-disable
at once, but in reality it is either doing pre-enable (when called
during enable sequence) or post-disable (when called during disable
sequence).

Hence, I would suggest to make it DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE

>>> DRM_BRIDGE_ENABLE_DISABLE

Similarly for this, it should be DRM_BRIDGE_ENABLE_OR_DISABLE

Regards
Devarsh