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

[Devarsh Thakkar]

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