From: Andrzej Hajda <andrzej.hajda@intel.com>
To: Dave Stevenson <dave.stevenson@raspberrypi.com>,
Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Marek Vasut <marex@denx.de>,
Jernej Skrabec <jernej.skrabec@gmail.com>,
Thomas Zimmermann <tzimmermann@suse.de>,
Neil Armstrong <narmstrong@baylibre.com>,
David Airlie <airlied@linux.ie>,
Robert Foss <robert.foss@linaro.org>,
Jonas Karlman <jonas@kwiboo.se>,
Douglas Anderson <dianders@chromium.org>,
DRI Development <dri-devel@lists.freedesktop.org>,
Andrzej Hajda <andrzej.hajda@gmail.com>,
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
Jagan Teki <jagan@amarulasolutions.com>
Subject: Re: [PATCH 1/2] drm: Introduce DRM_BRIDGE_OP_UPSTREAM_FIRST to alter bridge init order
Date: Mon, 28 Feb 2022 16:35:59 +0100 [thread overview]
Message-ID: <35234e49-8e1c-b88c-2764-cd79e50dd0ad@intel.com> (raw)
In-Reply-To: <CAPY8ntA=1ZD2kgFy=deuV4FWetT7wq3s_=eKH6kJwOx6CBLTkw@mail.gmail.com>
On 22.02.2022 09:43, Dave Stevenson wrote:
> Hi Laurent.
>
> Thanks for the review.
>
> On Tue, 22 Feb 2022 at 06:34, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>> Hi Dave,
>>
>> Thank you for the patch.
>>
>> On Wed, Feb 16, 2022 at 04:59:43PM +0000, Dave Stevenson wrote:
>>> DSI sink devices typically want the DSI host powered up and configured
>>> before they are powered up. pre_enable is the place this would normally
>>> happen, but they are called in reverse order from panel/connector towards
>>> the encoder, which is the "wrong" order.
>>>
>>> Add a new flag DRM_BRIDGE_OP_UPSTREAM_FIRST that any bridge can set
>>> to swap the order of pre_enable (and post_disable) so that any upstream
>>> bridges are called first to create the desired state.
>>>
>>> eg:
>>> - Panel
>>> - Bridge 1
>>> - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST
>>> - Bridge 3
>>> - Encoder
>>> Would result in pre_enable's being called as Panel, Bridge 1, Bridge 3,
>>> Bridge 2.
>> If there was a Bridge 4 between Bridge 3 and Encoder, would it be
>>
>> Panel, Bridge 1, Bridge 3, Bridge 4, Bridge 2
>>
>> ? I'd capture that here, to be explicit.
> No.
> - Panel
> - Bridge 1
> - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST
> - Bridge 3
> - Bridge 4
> - Encoder
> Would result in pre_enable's being called as Panel, Bridge 1, Bridge
> 3, Bridge 2, Bridge 4, Encoder.
> ie it only swaps the order of bridges 2 & 3.
>
> - Panel
> - Bridge 1
> - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST
> - Bridge 3 DRM_BRIDGE_OP_UPSTREAM_FIRST
> - Bridge 4
> - Encoder
> Would result in pre_enable's being called as Panel, Bridge 1, Bridge
> 4, Bridge 3, Bridge 2, Encoder.
> (Bridge 2&3 have asked for upstream to be enabled first, which means
> bridge 4. Bridge 2 wants upstream enabled first, which means bridge
> 3).
>
> - Panel
> - Bridge 1
> - Bridge 2 DRM_BRIDGE_OP_UPSTREAM_FIRST
> - Bridge 3
> - Bridge 4 DRM_BRIDGE_OP_UPSTREAM_FIRST
> - Bridge 5
> - Encoder
> Would result in Panel, Bridge 1, Bridge 3, Bridge 2, Bridge 5, Bridge
> 4, Encoder.
>
> So we only reverse the order whilst the bridges request that they want
> upstream enabled first, but we can do that multiple times within the
> chain. I hope that makes sense.
>
>>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
>>> ---
>>> drivers/gpu/drm/drm_bridge.c | 197 +++++++++++++++++++++++++++++++++++++------
>>> include/drm/drm_bridge.h | 8 ++
>>> 2 files changed, 180 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>>> index c96847fc0ebc..7c24e8340efa 100644
>>> --- a/drivers/gpu/drm/drm_bridge.c
>>> +++ b/drivers/gpu/drm/drm_bridge.c
>>> @@ -522,21 +522,58 @@ EXPORT_SYMBOL(drm_bridge_chain_disable);
>>> * Calls &drm_bridge_funcs.post_disable op for all the bridges in the
>>> * encoder chain, starting from the first bridge to the last. These are called
>>> * after completing the encoder's prepare op.
>> Missing blank line, as well as in three locations below.
>>
>>> + * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the post_disable for
>>> + * that bridge will be called before the previous one to reverse the pre_enable
>>> + * calling direction.
>>> *
>>> * Note: the bridge passed should be the one closest to the encoder
>>> */
>>> void drm_bridge_chain_post_disable(struct drm_bridge *bridge)
>>> {
>>> struct drm_encoder *encoder;
>>> + struct drm_bridge *next, *limit;
>>>
>>> if (!bridge)
>>> return;
>>>
>>> encoder = bridge->encoder;
>>> list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
>>> + limit = NULL;
>>> +
>>> + if (!list_is_last(&bridge->chain_node, &encoder->bridge_chain)) {
>>> + next = list_next_entry(bridge, chain_node);
>>> +
>>> + if (next->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
>>> + limit = next;
>>> +
>>> + list_for_each_entry_from(next, &encoder->bridge_chain,
>>> + chain_node) {
>>> + if (!(next->ops &
>>> + DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
>>> + next = list_prev_entry(next, chain_node);
>>> + limit = next;
>>> + break;
>>> + }
>>> + }
>>> +
>>> + list_for_each_entry_from_reverse(next, &encoder->bridge_chain,
>>> + chain_node) {
>>> + if (next == bridge)
>>> + break;
>>> +
>>> + if (next->funcs->post_disable)
>>> + next->funcs->post_disable(next);
>>> + }
>>> + }
>>> + }
>>> +
>>> if (bridge->funcs->post_disable)
>>> bridge->funcs->post_disable(bridge);
>>> +
>>> + if (limit)
>>> + bridge = limit;
>>> }
>>> +
>>> }
>>> EXPORT_SYMBOL(drm_bridge_chain_post_disable);
>>>
>>> @@ -577,22 +614,53 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);
>>> * Calls &drm_bridge_funcs.pre_enable op for all the bridges in the encoder
>>> * chain, starting from the last bridge to the first. These are called
>>> * before calling the encoder's commit op.
>>> + * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the pre_enable for
>>> + * the previous bridge will be called before pre_enable of this bridge.
>>> *
>>> * Note: the bridge passed should be the one closest to the encoder
>>> */
>>> void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
>>> {
>>> struct drm_encoder *encoder;
>>> - struct drm_bridge *iter;
>>> + struct drm_bridge *iter, *next, *limit;
>>>
>>> if (!bridge)
>>> return;
>>>
>>> encoder = bridge->encoder;
>>> +
>>> list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
>>> + if (iter->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
>>> + next = iter;
>>> + limit = bridge;
>>> + list_for_each_entry_from_reverse(next,
>>> + &encoder->bridge_chain,
>>> + chain_node) {
>>> + if (next == bridge)
>>> + break;
>>> +
>>> + if (!(next->ops &
>>> + DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
>>> + limit = list_prev_entry(next, chain_node);
>>> + break;
>>> + }
>>> + }
>>> +
>>> + list_for_each_entry_from(next, &encoder->bridge_chain, chain_node) {
>>> + if (next == iter)
>>> + break;
>>> +
>>> + if (next->funcs->pre_enable)
>>> + next->funcs->pre_enable(next);
>>> + }
>>> + }
>>> +
>>> if (iter->funcs->pre_enable)
>>> iter->funcs->pre_enable(iter);
>>>
>>> + if (iter->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST)
>>> + iter = limit;
>>> +
>>> if (iter == bridge)
>>> break;
>>> }
>>> @@ -667,6 +735,25 @@ void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,
>>> }
>>> EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);
>>>
>>> +static void drm_atomic_bridge_call_post_disable(struct drm_bridge *bridge,
>>> + struct drm_atomic_state *old_state)
>>> +{
>>> + if (bridge->funcs->atomic_post_disable) {
>>> + struct drm_bridge_state *old_bridge_state;
>>> +
>>> + old_bridge_state =
>>> + drm_atomic_get_old_bridge_state(old_state,
>>> + bridge);
>>> + if (WARN_ON(!old_bridge_state))
>>> + return;
>>> +
>>> + bridge->funcs->atomic_post_disable(bridge,
>>> + old_bridge_state);
>>> + } else if (bridge->funcs->post_disable) {
>>> + bridge->funcs->post_disable(bridge);
>>> + }
>>> +}
>>> +
>>> /**
>>> * drm_atomic_bridge_chain_post_disable - cleans up after disabling all bridges
>>> * in the encoder chain
>>> @@ -677,6 +764,9 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);
>>> * &drm_bridge_funcs.post_disable) op for all the bridges in the encoder chain,
>>> * starting from the first bridge to the last. These are called after completing
>>> * &drm_encoder_helper_funcs.atomic_disable
>>> + * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the post_disable for
>>> + * that bridge will be called before the previous one to reverse the pre_enable
>>> + * calling direction.
>>> *
>>> * Note: the bridge passed should be the one closest to the encoder
>>> */
>>> @@ -684,30 +774,69 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
>>> struct drm_atomic_state *old_state)
>>> {
>>> struct drm_encoder *encoder;
>>> + struct drm_bridge *next, *limit;
>>>
>>> if (!bridge)
>>> return;
>>>
>>> encoder = bridge->encoder;
>>> +
>>> list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
>>> - if (bridge->funcs->atomic_post_disable) {
>>> - struct drm_bridge_state *old_bridge_state;
>>> + limit = NULL;
>>> +
>>> + if (!list_is_last(&bridge->chain_node, &encoder->bridge_chain)) {
>>> + next = list_next_entry(bridge, chain_node);
>>> +
>>> + if (next->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
>>> + limit = next;
>>> +
>>> + list_for_each_entry_from(next, &encoder->bridge_chain,
>>> + chain_node) {
>>> + if (!(next->ops &
>>> + DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
>>> + next = list_prev_entry(next, chain_node);
>>> + limit = next;
>>> + break;
>>> + }
>>> + }
>>> +
>>> + list_for_each_entry_from_reverse(next, &encoder->bridge_chain,
>>> + chain_node) {
>>> + if (next == bridge)
>>> + break;
>>> +
>>> + drm_atomic_bridge_call_post_disable(next,
>>> + old_state);
>>> + }
>>> + }
>>> + }
>>>
>>> - old_bridge_state =
>>> - drm_atomic_get_old_bridge_state(old_state,
>>> - bridge);
>>> - if (WARN_ON(!old_bridge_state))
>>> - return;
>>> + drm_atomic_bridge_call_post_disable(bridge, old_state);
>>>
>>> - bridge->funcs->atomic_post_disable(bridge,
>>> - old_bridge_state);
>>> - } else if (bridge->funcs->post_disable) {
>>> - bridge->funcs->post_disable(bridge);
>>> - }
>>> + if (limit)
>>> + bridge = limit;
>>> }
>>> }
>>> EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
>>>
>>> +static void drm_atomic_bridge_call_pre_enable(struct drm_bridge *bridge,
>>> + struct drm_atomic_state *old_state)
>>> +{
>>> + if (bridge->funcs->atomic_pre_enable) {
>>> + struct drm_bridge_state *old_bridge_state;
>>> +
>>> + old_bridge_state =
>>> + drm_atomic_get_old_bridge_state(old_state,
>>> + bridge);
>>> + if (WARN_ON(!old_bridge_state))
>>> + return;
>>> +
>>> + bridge->funcs->atomic_pre_enable(bridge, old_bridge_state);
>>> + } else if (bridge->funcs->pre_enable) {
>>> + bridge->funcs->pre_enable(bridge);
>>> + }
>>> +}
>>> +
>>> /**
>>> * drm_atomic_bridge_chain_pre_enable - prepares for enabling all bridges in
>>> * the encoder chain
>>> @@ -718,6 +847,8 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
>>> * &drm_bridge_funcs.pre_enable) op for all the bridges in the encoder chain,
>>> * starting from the last bridge to the first. These are called before calling
>>> * &drm_encoder_helper_funcs.atomic_enable
>>> + * If a bridge sets the DRM_BRIDGE_OP_UPSTREAM_FIRST, then the pre_enable for
>>> + * the previous bridge will be called before pre_enable of this bridge.
>>> *
>>> * Note: the bridge passed should be the one closest to the encoder
>>> */
>>> @@ -725,26 +856,42 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
>>> struct drm_atomic_state *old_state)
>>> {
>>> struct drm_encoder *encoder;
>>> - struct drm_bridge *iter;
>>> + struct drm_bridge *iter, *next, *limit;
>>>
>>> if (!bridge)
>>> return;
>>>
>>> encoder = bridge->encoder;
>>> +
>>> list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
>>> - if (iter->funcs->atomic_pre_enable) {
>>> - struct drm_bridge_state *old_bridge_state;
>>> + if (iter->ops & DRM_BRIDGE_OP_UPSTREAM_FIRST) {
>>> + next = iter;
>>> + limit = bridge;
>>> + list_for_each_entry_from_reverse(next,
>>> + &encoder->bridge_chain,
>>> + chain_node) {
>>> + if (next == bridge)
>>> + break;
>>> +
>>> + if (!(next->ops &
>>> + DRM_BRIDGE_OP_UPSTREAM_FIRST)) {
>>> + limit = list_prev_entry(next, chain_node);
>>> + break;
>>> + }
>>> + }
>>> +
>>> + list_for_each_entry_from(next, &encoder->bridge_chain, chain_node) {
>>> + if (next == iter)
>>> + break;
>>> +
>>> + drm_atomic_bridge_call_pre_enable(next, old_state);
>>> + }
>>> + }
>> This is hard to understand, I have trouble figuring out if it does the
>> right thing when multiple bridges set the DRM_BRIDGE_OP_UPSTREAM_FIRST
>> flag (or actually even when a single bridge does so). Comments would
>> help, but I wonder if it wouldn't be simpler to switch to a recursive
>> implementation.
> Recursive - such joys!
> With the explanation above, I'm not sure that recursive helps, but
> certainly can add comments.
It could be recursive, or just proper iteration, for example pseudocode:
list_for_each_entry_reverse(...) {
if (iter->upstream_first && iter->prev)
continue;
tmp = iter;
do {
call_op
tmp = tmp->next;
} while (tmp && tmp->upstream_first);
}
Regards
Andrzej
next prev parent reply other threads:[~2022-02-28 15:36 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-02-16 16:59 [PATCH 0/2] DSI host and peripheral initialisation ordering Dave Stevenson
2022-02-16 16:59 ` [PATCH 1/2] drm: Introduce DRM_BRIDGE_OP_UPSTREAM_FIRST to alter bridge init order Dave Stevenson
2022-02-22 6:33 ` Laurent Pinchart
2022-02-22 8:43 ` Dave Stevenson
2022-02-28 15:35 ` Andrzej Hajda [this message]
2022-03-02 14:01 ` Dave Stevenson
2022-02-16 16:59 ` [PATCH 2/2] drm/bridge: Document the expected behaviour of DSI host controllers Dave Stevenson
2022-02-22 6:22 ` Laurent Pinchart
2022-02-22 8:15 ` Dave Stevenson
2022-02-17 16:05 ` [PATCH 0/2] DSI host and peripheral initialisation ordering Maxime Ripard
2022-02-17 17:46 ` Dmitry Baryshkov
2022-02-18 13:20 ` Andrzej Hajda
2022-02-22 6:43 ` Laurent Pinchart
2022-02-22 8:51 ` Dave Stevenson
2022-03-02 0:13 ` Doug Anderson
2022-03-02 17:20 ` Dave Stevenson
2022-03-02 18:47 ` Doug Anderson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=35234e49-8e1c-b88c-2764-cd79e50dd0ad@intel.com \
--to=andrzej.hajda@intel.com \
--cc=airlied@linux.ie \
--cc=andrzej.hajda@gmail.com \
--cc=dave.stevenson@raspberrypi.com \
--cc=dianders@chromium.org \
--cc=dmitry.baryshkov@linaro.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=jagan@amarulasolutions.com \
--cc=jernej.skrabec@gmail.com \
--cc=jonas@kwiboo.se \
--cc=laurent.pinchart@ideasonboard.com \
--cc=marex@denx.de \
--cc=narmstrong@baylibre.com \
--cc=robert.foss@linaro.org \
--cc=tzimmermann@suse.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox