Over time we have accumulated some deprecated functions etc. in drm_bridge. This patch-set starts to move over to the atomic variants and deletes what is not used anymore.
There was only one user of the non-atomic drm_bridge_chain functions in parade-ps8640 - migrate it to the atomic variants and delete the non-atomic drm_bridge_chain functions.
There was only one user of drm_bridge_chain_mode_fixup in mediatk. The use in the mediatek driver was wrong and with the single user gone we could also delete this function.
Added a few todo items.
Next step is to migrate the easy bridge drivers to use the atomic variants of drm_bridge_funcs operations. The easy ones are the drivers wihtout mode_set or mode_fixup. I have something typed up already, but wanted feedback on this patchset before sending out additional patches.
v2: v2 have been long in the coming as I wanted to wait until I started to have some spare time for linux stuff again, and thus time to address any comments timely. - Added Maxime's r-b (from old thread, so using old mail address [1]) - Fixed several small issues in patch 3 that introduces a new helper - Added a few more folks on cc in hope to see some testing
Sam
[1] https://lore.kernel.org/dri-devel/20210722062246.2512666-1-sam@ravnborg.org/
Sam Ravnborg (7): drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs drm/bridge: Drop unused drm_bridge_chain functions drm/bridge: Add drm_atomic_get_new_crtc_for_bridge() helper drm/bridge: lontium-lt9611: Use atomic variants of drm_bridge_funcs drm/mediatek: Drop chain_mode_fixup call in mode_valid() drm/bridge: Drop drm_bridge_chain_mode_fixup drm/todo: Add bridge related todo items
Documentation/gpu/todo.rst | 49 +++++++++++ drivers/gpu/drm/bridge/lontium-lt9611.c | 75 +++++++--------- drivers/gpu/drm/bridge/parade-ps8640.c | 14 +-- drivers/gpu/drm/drm_atomic.c | 42 +++++++++ drivers/gpu/drm/drm_bridge.c | 147 -------------------------------- drivers/gpu/drm/mediatek/mtk_hdmi.c | 11 --- include/drm/drm_atomic.h | 3 + include/drm/drm_bridge.h | 31 ------- 8 files changed, 135 insertions(+), 237 deletions(-)
The atomic variants of enable/disable in drm_bridge_funcs are the preferred operations - introduce these.
The ps8640 driver used the non-atomic variants of the drm_bridge_chain_pre_enable/ drm_bridge_chain_post_disable - convert these to the atomic variants.
v2: - Added a few more people to cc: (Jitao, Enric, Philip) to increase possibility to get test feedback
Signed-off-by: Sam Ravnborg sam@ravnborg.org Reviewed-by: Maxime Ripard maxime@cerno.tech Cc: Jitao Shi jitao.shi@mediatek.com Cc: Enric Balletbo i Serra enric.balletbo@collabora.com Cc: Philip Chen philipchen@chromium.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@gmail.com --- drivers/gpu/drm/bridge/parade-ps8640.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c index 3aaa90913bf8..0b620afe99c0 100644 --- a/drivers/gpu/drm/bridge/parade-ps8640.c +++ b/drivers/gpu/drm/bridge/parade-ps8640.c @@ -376,7 +376,8 @@ static void ps8640_bridge_poweroff(struct ps8640 *ps_bridge) ps_bridge->powered = false; }
-static void ps8640_pre_enable(struct drm_bridge *bridge) +static void ps8640_atomic_pre_enable(struct drm_bridge *bridge, + struct drm_bridge_state *old_bridge_state) { struct ps8640 *ps_bridge = bridge_to_ps8640(bridge); int ret; @@ -388,7 +389,8 @@ static void ps8640_pre_enable(struct drm_bridge *bridge) ps8640_bridge_poweroff(ps_bridge); }
-static void ps8640_post_disable(struct drm_bridge *bridge) +static void ps8640_atomic_post_disable(struct drm_bridge *bridge, + struct drm_bridge_state *old_bridge_state) { struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
@@ -489,7 +491,7 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge, * EDID, for this chip, we need to do a full poweron, otherwise it will * fail. */ - drm_bridge_chain_pre_enable(bridge); + drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
edid = drm_get_edid(connector, ps_bridge->page[PAGE0_DP_CNTL]->adapter); @@ -499,7 +501,7 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge, * before, return the chip to its original power state. */ if (poweroff) - drm_bridge_chain_post_disable(bridge); + drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
return edid; } @@ -508,8 +510,8 @@ static const struct drm_bridge_funcs ps8640_bridge_funcs = { .attach = ps8640_bridge_attach, .detach = ps8640_bridge_detach, .get_edid = ps8640_bridge_get_edid, - .post_disable = ps8640_post_disable, - .pre_enable = ps8640_pre_enable, + .atomic_post_disable = ps8640_atomic_post_disable, + .atomic_pre_enable = ps8640_atomic_pre_enable, };
static int ps8640_probe(struct i2c_client *client)
Hi Sam,
Thank you for the patch.
On Wed, Oct 20, 2021 at 08:18:55PM +0200, Sam Ravnborg wrote:
The atomic variants of enable/disable in drm_bridge_funcs are the preferred operations - introduce these.
The ps8640 driver used the non-atomic variants of the drm_bridge_chain_pre_enable/ drm_bridge_chain_post_disable - convert these to the atomic variants.
v2:
- Added a few more people to cc: (Jitao, Enric, Philip) to increase possibility to get test feedback
Signed-off-by: Sam Ravnborg sam@ravnborg.org Reviewed-by: Maxime Ripard maxime@cerno.tech Cc: Jitao Shi jitao.shi@mediatek.com Cc: Enric Balletbo i Serra enric.balletbo@collabora.com Cc: Philip Chen philipchen@chromium.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@gmail.com
drivers/gpu/drm/bridge/parade-ps8640.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c index 3aaa90913bf8..0b620afe99c0 100644 --- a/drivers/gpu/drm/bridge/parade-ps8640.c +++ b/drivers/gpu/drm/bridge/parade-ps8640.c @@ -376,7 +376,8 @@ static void ps8640_bridge_poweroff(struct ps8640 *ps_bridge) ps_bridge->powered = false; }
-static void ps8640_pre_enable(struct drm_bridge *bridge) +static void ps8640_atomic_pre_enable(struct drm_bridge *bridge,
struct drm_bridge_state *old_bridge_state){ struct ps8640 *ps_bridge = bridge_to_ps8640(bridge); int ret; @@ -388,7 +389,8 @@ static void ps8640_pre_enable(struct drm_bridge *bridge) ps8640_bridge_poweroff(ps_bridge); }
-static void ps8640_post_disable(struct drm_bridge *bridge) +static void ps8640_atomic_post_disable(struct drm_bridge *bridge,
struct drm_bridge_state *old_bridge_state){ struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
@@ -489,7 +491,7 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge, * EDID, for this chip, we need to do a full poweron, otherwise it will * fail. */
- drm_bridge_chain_pre_enable(bridge);
drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
edid = drm_get_edid(connector, ps_bridge->page[PAGE0_DP_CNTL]->adapter);
@@ -499,7 +501,7 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge, * before, return the chip to its original power state. */ if (poweroff)
drm_bridge_chain_post_disable(bridge);
drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);return edid;
} @@ -508,8 +510,8 @@ static const struct drm_bridge_funcs ps8640_bridge_funcs = { .attach = ps8640_bridge_attach, .detach = ps8640_bridge_detach, .get_edid = ps8640_bridge_get_edid,
- .post_disable = ps8640_post_disable,
- .pre_enable = ps8640_pre_enable,
- .atomic_post_disable = ps8640_atomic_post_disable,
- .atomic_pre_enable = ps8640_atomic_pre_enable,
Don't you also need to implement .atomic_duplicate_state(), .atomic_destroy_state() and .atomic_reset() to use the atomic API ?
};
static int ps8640_probe(struct i2c_client *client)
Hi Laurent,
@@ -508,8 +510,8 @@ static const struct drm_bridge_funcs ps8640_bridge_funcs = { .attach = ps8640_bridge_attach, .detach = ps8640_bridge_detach, .get_edid = ps8640_bridge_get_edid,
- .post_disable = ps8640_post_disable,
- .pre_enable = ps8640_pre_enable,
- .atomic_post_disable = ps8640_atomic_post_disable,
- .atomic_pre_enable = ps8640_atomic_pre_enable,
Don't you also need to implement .atomic_duplicate_state(), .atomic_destroy_state() and .atomic_reset() to use the atomic API ?
What I think happens is that the atomic drivers uses the bridges and the implementation in drm_bridge has fall-backs to the non-atomic variants.
So for example drm_atomic_bridge_chain_pre_enable() will call atomic_pre_enable() if it exists, and if not it will call pre_enable() if it exists.
With this patch-set I show that the non-atomic versions of several of the drm_bridge helper functions are almost not used and easy to drop. So based on this I concluded that the bridge drivers were used in a way so we only would need to implement the atomic variants of the functions.
That said I did not consider .atomic_duplicate_state(), .atomic_destroy_state(), or .atomic_reset().
From a quick look only cadence/cdns-mhdp8546 subclass
drm_bridge_state and I wonder if the right thing to do would be to implement fallback to the helpers if the bridge driver do not set any of the .atomic_duplicate_state(), .atomic_destroy_state(), or .atomic_reset().
That would drop the following from a few bridges: .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, .atomic_reset = drm_atomic_helper_bridge_reset,
And new bridges (or bridges we convert to use the atomic operations) would not have to specify them.
I would prefer implementing the fallback - rather than adding the above to this driver.
Sam
Hi Laurent,
From a quick look only cadence/cdns-mhdp8546 subclass
drm_bridge_state and I wonder if the right thing to do would be to implement fallback to the helpers if the bridge driver do not set any of the .atomic_duplicate_state(), .atomic_destroy_state(), or .atomic_reset().
That would drop the following from a few bridges: .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, .atomic_reset = drm_atomic_helper_bridge_reset,
To answer myself here. This would create a dependency from the core to the helpers which is not OK so idea dropped again.
Sam
Hi Sam,
On Fri, Oct 22, 2021 at 07:13:58PM +0200, Sam Ravnborg wrote:
Hi Laurent,
From a quick look only cadence/cdns-mhdp8546 subclass drm_bridge_state and I wonder if the right thing to do would be to implement fallback to the helpers if the bridge driver do not set any of the .atomic_duplicate_state(), .atomic_destroy_state(), or .atomic_reset().
That would drop the following from a few bridges: .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, .atomic_reset = drm_atomic_helper_bridge_reset,
To answer myself here. This would create a dependency from the core to the helpers which is not OK so idea dropped again.
I agree it would be nicer, but the dependency is likely a problem. That being said, we have multiple types of helpers. The first set is the modeset helpers, which were designed as one implementation of KMS operations, with an opt-in API for drivers. The core should not depend on those. There are however other helpers that are only default implementations of some operations, without any dependency on other components. The atomic state helpers fall in this category, they implement .atomic_* operations of the drm_*_funcs structures, not drm_*_helper_funcs. It could make sense to move them to the DRM core.
Hi Laurent,
On Fri, Oct 22, 2021 at 09:42:37PM +0300, Laurent Pinchart wrote:
Hi Sam,
On Fri, Oct 22, 2021 at 07:13:58PM +0200, Sam Ravnborg wrote:
Hi Laurent,
From a quick look only cadence/cdns-mhdp8546 subclass drm_bridge_state and I wonder if the right thing to do would be to implement fallback to the helpers if the bridge driver do not set any of the .atomic_duplicate_state(), .atomic_destroy_state(), or .atomic_reset().
That would drop the following from a few bridges: .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, .atomic_reset = drm_atomic_helper_bridge_reset,
To answer myself here. This would create a dependency from the core to the helpers which is not OK so idea dropped again.
I agree it would be nicer, but the dependency is likely a problem. That being said, we have multiple types of helpers. The first set is the modeset helpers, which were designed as one implementation of KMS operations, with an opt-in API for drivers. The core should not depend on those. There are however other helpers that are only default implementations of some operations, without any dependency on other components. The atomic state helpers fall in this category, they implement .atomic_* operations of the drm_*_funcs structures, not drm_*_helper_funcs. It could make sense to move them to the DRM core.
For now I went with a simple macro:
+/** + * DRM_BRIDGE_STATE_OPS - Default drm_bridge state funcs + * + * Bridge driver that do not subclass &drm_bridge_state can use the helpers + * for reset, duplicate, and destroy. This macro provides a shortcut for + * setting the helpers in the &drm_bridge_funcs structure. + */ +#define DRM_BRIDGE_STATE_OPS \ + .atomic_reset = drm_atomic_helper_bridge_reset, \ + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, \ + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state +
Thomas Z. is trying to make the core smaller so pulling in these helpers would be counterproductive to that. So I took the simpler approach here which we have already done in several places. It will be part of v3 when I post it.
Drop a note if you (or any other reader) have better ideas.
Sam
Hi Sam,
On Fri, Oct 22, 2021 at 09:32:08PM +0200, Sam Ravnborg wrote:
On Fri, Oct 22, 2021 at 09:42:37PM +0300, Laurent Pinchart wrote:
On Fri, Oct 22, 2021 at 07:13:58PM +0200, Sam Ravnborg wrote:
Hi Laurent,
From a quick look only cadence/cdns-mhdp8546 subclass drm_bridge_state and I wonder if the right thing to do would be to implement fallback to the helpers if the bridge driver do not set any of the .atomic_duplicate_state(), .atomic_destroy_state(), or .atomic_reset().
That would drop the following from a few bridges: .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, .atomic_reset = drm_atomic_helper_bridge_reset,
To answer myself here. This would create a dependency from the core to the helpers which is not OK so idea dropped again.
I agree it would be nicer, but the dependency is likely a problem. That being said, we have multiple types of helpers. The first set is the modeset helpers, which were designed as one implementation of KMS operations, with an opt-in API for drivers. The core should not depend on those. There are however other helpers that are only default implementations of some operations, without any dependency on other components. The atomic state helpers fall in this category, they implement .atomic_* operations of the drm_*_funcs structures, not drm_*_helper_funcs. It could make sense to move them to the DRM core.
For now I went with a simple macro:
+/**
- DRM_BRIDGE_STATE_OPS - Default drm_bridge state funcs
- Bridge driver that do not subclass &drm_bridge_state can use the helpers
- for reset, duplicate, and destroy. This macro provides a shortcut for
- setting the helpers in the &drm_bridge_funcs structure.
- */
+#define DRM_BRIDGE_STATE_OPS \
.atomic_reset = drm_atomic_helper_bridge_reset, \.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, \.atomic_destroy_state = drm_atomic_helper_bridge_destroy_stateThomas Z. is trying to make the core smaller so pulling in these helpers would be counterproductive to that. So I took the simpler approach here which we have already done in several places.
Those helpers are in the same file as the other state helpers, which are used by all atomic drivers as far as I can tell, so I'm not sure we can really make anything smaller (except if we moved the bridge helpers to a separate file, but I don't think it would be worth it).
It will be part of v3 when I post it.
Drop a note if you (or any other reader) have better ideas.
The drm_bridge_chain_{pre_enable,enable,disable,post_disable} has no users left and we have atomic variants that should be used. Drop them so they do not gain new users.
Adjust a few comments to avoid references to the dropped functions.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Reviewed-by: Maxime Ripard maxime@cerno.tech Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Daniel Vetter daniel@ffwll.ch --- drivers/gpu/drm/drm_bridge.c | 110 ----------------------------------- include/drm/drm_bridge.h | 28 --------- 2 files changed, 138 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index c96847fc0ebc..7a57d6816105 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -485,61 +485,6 @@ drm_bridge_chain_mode_valid(struct drm_bridge *bridge, } EXPORT_SYMBOL(drm_bridge_chain_mode_valid);
-/** - * drm_bridge_chain_disable - disables all bridges in the encoder chain - * @bridge: bridge control structure - * - * Calls &drm_bridge_funcs.disable 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 prepare op. - * - * Note: the bridge passed should be the one closest to the encoder - */ -void drm_bridge_chain_disable(struct drm_bridge *bridge) -{ - struct drm_encoder *encoder; - struct drm_bridge *iter; - - if (!bridge) - return; - - encoder = bridge->encoder; - list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) { - if (iter->funcs->disable) - iter->funcs->disable(iter); - - if (iter == bridge) - break; - } -} -EXPORT_SYMBOL(drm_bridge_chain_disable); - -/** - * drm_bridge_chain_post_disable - cleans up after disabling all bridges in the - * encoder chain - * @bridge: bridge control structure - * - * 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. - * - * 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; - - if (!bridge) - return; - - encoder = bridge->encoder; - list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) { - if (bridge->funcs->post_disable) - bridge->funcs->post_disable(bridge); - } -} -EXPORT_SYMBOL(drm_bridge_chain_post_disable); - /** * drm_bridge_chain_mode_set - set proposed mode for all bridges in the * encoder chain @@ -569,61 +514,6 @@ void drm_bridge_chain_mode_set(struct drm_bridge *bridge, } EXPORT_SYMBOL(drm_bridge_chain_mode_set);
-/** - * drm_bridge_chain_pre_enable - prepares for enabling all bridges in the - * encoder chain - * @bridge: bridge control structure - * - * 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. - * - * 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; - - if (!bridge) - return; - - encoder = bridge->encoder; - list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) { - if (iter->funcs->pre_enable) - iter->funcs->pre_enable(iter); - - if (iter == bridge) - break; - } -} -EXPORT_SYMBOL(drm_bridge_chain_pre_enable); - -/** - * drm_bridge_chain_enable - enables all bridges in the encoder chain - * @bridge: bridge control structure - * - * Calls &drm_bridge_funcs.enable 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 commit op. - * - * Note that the bridge passed should be the one closest to the encoder - */ -void drm_bridge_chain_enable(struct drm_bridge *bridge) -{ - struct drm_encoder *encoder; - - if (!bridge) - return; - - encoder = bridge->encoder; - list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) { - if (bridge->funcs->enable) - bridge->funcs->enable(bridge); - } -} -EXPORT_SYMBOL(drm_bridge_chain_enable); - /** * drm_atomic_bridge_chain_disable - disables all bridges in the encoder chain * @bridge: bridge control structure diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 061d87313fac..f1eb71ff5379 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -297,12 +297,6 @@ struct drm_bridge_funcs { * not enable the display link feeding the next bridge in the chain (if * there is one) when this callback is called. * - * Note that this function will only be invoked in the context of an - * atomic commit. It will not be invoked from - * &drm_bridge_chain_pre_enable. It would be prudent to also provide an - * implementation of @pre_enable if you are expecting driver calls into - * &drm_bridge_chain_pre_enable. - * * The @atomic_pre_enable callback is optional. */ void (*atomic_pre_enable)(struct drm_bridge *bridge, @@ -323,11 +317,6 @@ struct drm_bridge_funcs { * callback must enable the display link feeding the next bridge in the * chain if there is one. * - * Note that this function will only be invoked in the context of an - * atomic commit. It will not be invoked from &drm_bridge_chain_enable. - * It would be prudent to also provide an implementation of @enable if - * you are expecting driver calls into &drm_bridge_chain_enable. - * * The @atomic_enable callback is optional. */ void (*atomic_enable)(struct drm_bridge *bridge, @@ -345,12 +334,6 @@ struct drm_bridge_funcs { * The bridge can assume that the display pipe (i.e. clocks and timing * signals) feeding it is still running when this callback is called. * - * Note that this function will only be invoked in the context of an - * atomic commit. It will not be invoked from - * &drm_bridge_chain_disable. It would be prudent to also provide an - * implementation of @disable if you are expecting driver calls into - * &drm_bridge_chain_disable. - * * The @atomic_disable callback is optional. */ void (*atomic_disable)(struct drm_bridge *bridge, @@ -370,13 +353,6 @@ struct drm_bridge_funcs { * signals) feeding it is no longer running when this callback is * called. * - * Note that this function will only be invoked in the context of an - * atomic commit. It will not be invoked from - * &drm_bridge_chain_post_disable. - * It would be prudent to also provide an implementation of - * @post_disable if you are expecting driver calls into - * &drm_bridge_chain_post_disable. - * * The @atomic_post_disable callback is optional. */ void (*atomic_post_disable)(struct drm_bridge *bridge, @@ -868,13 +844,9 @@ enum drm_mode_status drm_bridge_chain_mode_valid(struct drm_bridge *bridge, const struct drm_display_info *info, const struct drm_display_mode *mode); -void drm_bridge_chain_disable(struct drm_bridge *bridge); -void drm_bridge_chain_post_disable(struct drm_bridge *bridge); void drm_bridge_chain_mode_set(struct drm_bridge *bridge, const struct drm_display_mode *mode, const struct drm_display_mode *adjusted_mode); -void drm_bridge_chain_pre_enable(struct drm_bridge *bridge); -void drm_bridge_chain_enable(struct drm_bridge *bridge);
int drm_atomic_bridge_chain_check(struct drm_bridge *bridge, struct drm_crtc_state *crtc_state,
drm_atomic_get_new_crtc_for_bridge() will be used by bridge drivers to provide easy access to the mode from the drm_bridge_funcs operations.
The helper will be useful in the conversion to the atomic operations of struct drm_bridge_funcs.
v2: - Renamed to drm_atomic_get_new_crtc_for_bridge (Maxime) - Use atomic_state, not bridge_State (Maxime) - Drop WARN on crtc_state as it is a valid case (Maxime) - Added small code snip to help readers - Updated description, fixed kernel-doc and exported the symbol
Signed-off-by: Sam Ravnborg sam@ravnborg.org Suggested-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Daniel Vetter daniel@ffwll.ch --- drivers/gpu/drm/drm_atomic.c | 42 ++++++++++++++++++++++++++++++++++++ include/drm/drm_atomic.h | 3 +++ 2 files changed, 45 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index ff1416cd609a..8b107194b157 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1134,6 +1134,48 @@ drm_atomic_get_new_bridge_state(struct drm_atomic_state *state, } EXPORT_SYMBOL(drm_atomic_get_new_bridge_state);
+/** + * drm_atomic_get_new_crtc_for_bridge - get new crtc_state for the bridge + * @state: state of the bridge + * @bridge: bridge object + * + * This function is often used in the &struct drm_bridge_funcs operations + * to provide easy access to the mode like this: + * + * .. code-block:: c + * + * crtc_state = drm_atomic_get_new_crtc_for_bridge(old_bridge_state->base.state, + * bridge); + * if (crtc_state) { + * mode = &crtc_state->mode; + * ... + * + * If no connector can be looked up or if no connector state is available + * then this will be logged using WARN(). + * + * Returns: + * The &struct drm_crtc_state for the given bridge/state, or NULL + * if no crtc_state could be looked up. + */ +const struct drm_crtc_state * +drm_atomic_get_new_crtc_for_bridge(struct drm_atomic_state *state, + struct drm_bridge *bridge) +{ + const struct drm_connector_state *conn_state; + struct drm_connector *connector; + + connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder); + if (WARN_ON(!connector)) + return NULL; + + conn_state = drm_atomic_get_new_connector_state(state, connector); + if (WARN_ON(!conn_state || !conn_state->crtc)) + return NULL; + + return drm_atomic_get_new_crtc_state(state, conn_state->crtc); +} +EXPORT_SYMBOL(drm_atomic_get_new_crtc_for_bridge); + /** * drm_atomic_add_encoder_bridges - add bridges attached to an encoder * @state: atomic state diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 1701c2128a5c..f861d73296cc 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -1119,5 +1119,8 @@ drm_atomic_get_old_bridge_state(struct drm_atomic_state *state, struct drm_bridge_state * drm_atomic_get_new_bridge_state(struct drm_atomic_state *state, struct drm_bridge *bridge); +const struct drm_crtc_state * +drm_atomic_get_new_crtc_for_bridge(struct drm_atomic_state *state, + struct drm_bridge *bridge);
#endif /* DRM_ATOMIC_H_ */
Hi,
On Wed, Oct 20, 2021 at 08:18:57PM +0200, Sam Ravnborg wrote:
drm_atomic_get_new_crtc_for_bridge() will be used by bridge drivers to provide easy access to the mode from the drm_bridge_funcs operations.
The helper will be useful in the conversion to the atomic operations of struct drm_bridge_funcs.
v2:
- Renamed to drm_atomic_get_new_crtc_for_bridge (Maxime)
- Use atomic_state, not bridge_State (Maxime)
- Drop WARN on crtc_state as it is a valid case (Maxime)
- Added small code snip to help readers
- Updated description, fixed kernel-doc and exported the symbol
Signed-off-by: Sam Ravnborg sam@ravnborg.org Suggested-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Daniel Vetter daniel@ffwll.ch
drivers/gpu/drm/drm_atomic.c | 42 ++++++++++++++++++++++++++++++++++++ include/drm/drm_atomic.h | 3 +++ 2 files changed, 45 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index ff1416cd609a..8b107194b157 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1134,6 +1134,48 @@ drm_atomic_get_new_bridge_state(struct drm_atomic_state *state, } EXPORT_SYMBOL(drm_atomic_get_new_bridge_state);
+/**
- drm_atomic_get_new_crtc_for_bridge - get new crtc_state for the bridge
- @state: state of the bridge
- @bridge: bridge object
I appreciate that the function name is fairly long already, but given its name I'd expect it to return a drm_crtc, not drm_crtc_state.
All the other state related functions are named using the pattern drm_atomic_get_(old|new)_$object_state.
Moreover, we already have a drm_atomic_get_new_connector_for_encoder function that does return a drm_connector, so I think we should make it consistent there and call it drm_atomic_get_new_crtc_state_for_bridge
Maxime
Hi Maxime,
On Fri, Oct 22, 2021 at 01:03:55PM +0200, Maxime Ripard wrote:
Hi,
On Wed, Oct 20, 2021 at 08:18:57PM +0200, Sam Ravnborg wrote:
drm_atomic_get_new_crtc_for_bridge() will be used by bridge drivers to provide easy access to the mode from the drm_bridge_funcs operations.
The helper will be useful in the conversion to the atomic operations of struct drm_bridge_funcs.
v2:
- Renamed to drm_atomic_get_new_crtc_for_bridge (Maxime)
- Use atomic_state, not bridge_State (Maxime)
- Drop WARN on crtc_state as it is a valid case (Maxime)
- Added small code snip to help readers
- Updated description, fixed kernel-doc and exported the symbol
Signed-off-by: Sam Ravnborg sam@ravnborg.org Suggested-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Daniel Vetter daniel@ffwll.ch
drivers/gpu/drm/drm_atomic.c | 42 ++++++++++++++++++++++++++++++++++++ include/drm/drm_atomic.h | 3 +++ 2 files changed, 45 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index ff1416cd609a..8b107194b157 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1134,6 +1134,48 @@ drm_atomic_get_new_bridge_state(struct drm_atomic_state *state, } EXPORT_SYMBOL(drm_atomic_get_new_bridge_state);
+/**
- drm_atomic_get_new_crtc_for_bridge - get new crtc_state for the bridge
- @state: state of the bridge
- @bridge: bridge object
I appreciate that the function name is fairly long already, but given its name I'd expect it to return a drm_crtc, not drm_crtc_state.
All the other state related functions are named using the pattern drm_atomic_get_(old|new)_$object_state.
Moreover, we already have a drm_atomic_get_new_connector_for_encoder function that does return a drm_connector, so I think we should make it consistent there and call it drm_atomic_get_new_crtc_state_for_bridge
That's a better name - thanks! I will fix it in v3.
Sam
The atomic variants of enable/disable in drm_bridge_funcs are the preferred operations - introduce these.
Use of mode_set is deprecated - merge the functionality with atomic_enable()
v2: - Added check if crtc_state is NULL (Maxime) - Updated to use drm_atomic_get_new_crtc_for_bridge()
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Maxime Ripard maxime@cerno.tech Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Robert Foss robert.foss@linaro.org Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@gmail.com --- drivers/gpu/drm/bridge/lontium-lt9611.c | 75 +++++++++++-------------- 1 file changed, 33 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/bridge/lontium-lt9611.c b/drivers/gpu/drm/bridge/lontium-lt9611.c index 29b1ce2140ab..cdc3d5f8dcac 100644 --- a/drivers/gpu/drm/bridge/lontium-lt9611.c +++ b/drivers/gpu/drm/bridge/lontium-lt9611.c @@ -700,9 +700,23 @@ lt9611_connector_mode_valid(struct drm_connector *connector, }
/* bridge funcs */ -static void lt9611_bridge_enable(struct drm_bridge *bridge) +static void lt9611_bridge_atomic_enable(struct drm_bridge *bridge, + struct drm_bridge_state *old_bridge_state) { struct lt9611 *lt9611 = bridge_to_lt9611(bridge); + const struct drm_display_mode *mode; + const struct drm_crtc_state *crtc_state; + struct hdmi_avi_infoframe avi_frame; + int ret; + + crtc_state = drm_atomic_get_new_crtc_for_bridge(old_bridge_state->base.state, + bridge); + if (!crtc_state) { + dev_err(lt9611->dev, "no crtc_state available\n"); + return; + } + + mode = &crtc_state->mode;
if (lt9611_power_on(lt9611)) { dev_err(lt9611->dev, "power on failed\n"); @@ -719,9 +733,21 @@ static void lt9611_bridge_enable(struct drm_bridge *bridge)
/* Enable HDMI output */ regmap_write(lt9611->regmap, 0x8130, 0xea); + + lt9611_mipi_input_digital(lt9611, mode); + lt9611_pll_setup(lt9611, mode); + lt9611_mipi_video_setup(lt9611, mode); + lt9611_pcr_setup(lt9611, mode); + + ret = drm_hdmi_avi_infoframe_from_display_mode(&avi_frame, + <9611->connector, + mode); + if (!ret) + lt9611->vic = avi_frame.video_code; }
-static void lt9611_bridge_disable(struct drm_bridge *bridge) +static void lt9611_bridge_atomic_disable(struct drm_bridge *bridge, + struct drm_bridge_state *old_bridge_state) { struct lt9611 *lt9611 = bridge_to_lt9611(bridge); int ret; @@ -877,48 +903,14 @@ static enum drm_mode_status lt9611_bridge_mode_valid(struct drm_bridge *bridge, return MODE_OK; }
-static void lt9611_bridge_pre_enable(struct drm_bridge *bridge) -{ - struct lt9611 *lt9611 = bridge_to_lt9611(bridge); - - if (!lt9611->sleep) - return; - - lt9611_reset(lt9611); - regmap_write(lt9611->regmap, 0x80ee, 0x01); - - lt9611->sleep = false; -} - -static void lt9611_bridge_post_disable(struct drm_bridge *bridge) +static void lt9611_bridge_atomic_post_disable(struct drm_bridge *bridge, + struct drm_bridge_state *old_bridge_state) { struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
lt9611_sleep_setup(lt9611); }
-static void lt9611_bridge_mode_set(struct drm_bridge *bridge, - const struct drm_display_mode *mode, - const struct drm_display_mode *adj_mode) -{ - struct lt9611 *lt9611 = bridge_to_lt9611(bridge); - struct hdmi_avi_infoframe avi_frame; - int ret; - - lt9611_bridge_pre_enable(bridge); - - lt9611_mipi_input_digital(lt9611, mode); - lt9611_pll_setup(lt9611, mode); - lt9611_mipi_video_setup(lt9611, mode); - lt9611_pcr_setup(lt9611, mode); - - ret = drm_hdmi_avi_infoframe_from_display_mode(&avi_frame, - <9611->connector, - mode); - if (!ret) - lt9611->vic = avi_frame.video_code; -} - static enum drm_connector_status lt9611_bridge_detect(struct drm_bridge *bridge) { struct lt9611 *lt9611 = bridge_to_lt9611(bridge); @@ -954,10 +946,9 @@ static const struct drm_bridge_funcs lt9611_bridge_funcs = { .attach = lt9611_bridge_attach, .detach = lt9611_bridge_detach, .mode_valid = lt9611_bridge_mode_valid, - .enable = lt9611_bridge_enable, - .disable = lt9611_bridge_disable, - .post_disable = lt9611_bridge_post_disable, - .mode_set = lt9611_bridge_mode_set, + .atomic_enable = lt9611_bridge_atomic_enable, + .atomic_disable = lt9611_bridge_atomic_disable, + .atomic_post_disable = lt9611_bridge_atomic_post_disable, .detect = lt9611_bridge_detect, .get_edid = lt9611_bridge_get_edid, .hpd_enable = lt9611_bridge_hpd_enable,
The mode_valid implementation had a call to drm_bridge_chain_mode_fixup() which would be wrong as the mode_valid is not allowed to change anything - only to validate the mode.
As the next bridge is often/always a connector the call had no effect anyway. So drop it.
From the git history I could see this call was included in the original
version of the driver so there was no help there to find out why it was added in the first place. But a lot has changed since the initial driver were added and is seems safe to remove the call now.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Reviewed-by: Maxime Ripard maxime@cerno.tech Cc: Chun-Kuang Hu chunkuang.hu@kernel.org Cc: Philipp Zabel p.zabel@pengutronix.de Cc: Matthias Brugger matthias.bgg@gmail.com Cc: Dafna Hirschfeld dafna.hirschfeld@collabora.com Cc: linux-mediatek@lists.infradead.org Cc: linux-arm-kernel@lists.infradead.org --- drivers/gpu/drm/mediatek/mtk_hdmi.c | 11 ----------- 1 file changed, 11 deletions(-)
diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c index 5838c44cbf6f..bade1cbd782d 100644 --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c @@ -1208,22 +1208,11 @@ static int mtk_hdmi_bridge_mode_valid(struct drm_bridge *bridge, const struct drm_display_mode *mode) { struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge); - struct drm_bridge *next_bridge;
dev_dbg(hdmi->dev, "xres=%d, yres=%d, refresh=%d, intl=%d clock=%d\n", mode->hdisplay, mode->vdisplay, drm_mode_vrefresh(mode), !!(mode->flags & DRM_MODE_FLAG_INTERLACE), mode->clock * 1000);
- next_bridge = drm_bridge_get_next_bridge(&hdmi->bridge); - if (next_bridge) { - struct drm_display_mode adjusted_mode; - - drm_mode_copy(&adjusted_mode, mode); - if (!drm_bridge_chain_mode_fixup(next_bridge, mode, - &adjusted_mode)) - return MODE_BAD; - } - if (hdmi->conf->cea_modes_only && !drm_match_cea_mode(mode)) return MODE_BAD;
There are no users left of drm_bridge_chain_mode_fixup() and we do not want to have this function available, so drop it.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Reviewed-by: Maxime Ripard maxime@cerno.tech Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch --- drivers/gpu/drm/drm_bridge.c | 37 ------------------------------------ include/drm/drm_bridge.h | 3 --- 2 files changed, 40 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 7a57d6816105..57a864d9a87f 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -406,43 +406,6 @@ void drm_bridge_detach(struct drm_bridge *bridge) * needed, in order to gradually transition to the new model. */
-/** - * drm_bridge_chain_mode_fixup - fixup proposed mode for all bridges in the - * encoder chain - * @bridge: bridge control structure - * @mode: desired mode to be set for the bridge - * @adjusted_mode: updated mode that works for this bridge - * - * Calls &drm_bridge_funcs.mode_fixup for all the bridges in the - * encoder chain, starting from the first bridge to the last. - * - * Note: the bridge passed should be the one closest to the encoder - * - * RETURNS: - * true on success, false on failure - */ -bool drm_bridge_chain_mode_fixup(struct drm_bridge *bridge, - const struct drm_display_mode *mode, - struct drm_display_mode *adjusted_mode) -{ - struct drm_encoder *encoder; - - if (!bridge) - return true; - - encoder = bridge->encoder; - list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) { - if (!bridge->funcs->mode_fixup) - continue; - - if (!bridge->funcs->mode_fixup(bridge, mode, adjusted_mode)) - return false; - } - - return true; -} -EXPORT_SYMBOL(drm_bridge_chain_mode_fixup); - /** * drm_bridge_chain_mode_valid - validate the mode against all bridges in the * encoder chain. diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index f1eb71ff5379..c8d07bd27f63 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -837,9 +837,6 @@ drm_bridge_chain_get_first_bridge(struct drm_encoder *encoder) #define drm_for_each_bridge_in_chain(encoder, bridge) \ list_for_each_entry(bridge, &(encoder)->bridge_chain, chain_node)
-bool drm_bridge_chain_mode_fixup(struct drm_bridge *bridge, - const struct drm_display_mode *mode, - struct drm_display_mode *adjusted_mode); enum drm_mode_status drm_bridge_chain_mode_valid(struct drm_bridge *bridge, const struct drm_display_info *info,
- deprecated callbacks in drm_bridge_funcs - move connector creation to display drivers
v2: - Updated descriptions in todo.rst
Signed-off-by: Sam Ravnborg sam@ravnborg.org Acked-by: Maxime Ripard maxime@cerno.tech Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch --- Documentation/gpu/todo.rst | 49 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)
diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst index 60d1d7ee0719..17c03e7c41e5 100644 --- a/Documentation/gpu/todo.rst +++ b/Documentation/gpu/todo.rst @@ -463,6 +463,55 @@ Contact: Thomas Zimmermann tzimmermann@suse.de, Christian König, Daniel Vette
Level: Intermediate
+Drop use of deprecated operations in bridge drivers +-------------------------------------------------- + +&struct drm_bridge_funcs contains a number of deprecated operations +which use can be replaced by the atomic variants. + +The following is more or less 1:1 replacements whit the arguments +and names adjusted: +* pre_enable => atomic_pre_enable +* enable => atomic_enable +* disable => atomic_disable +* post_disable => atomic_post_disable + +* mode_set is no longer required and the implementation shall be merged + with atomic_enable. + +* mode_fixup => atomic_check + mode_fixup() was created a long time ago, when we were supposed to have + a single bridge at the output of the CRTC. The bridge could then instruct + the CRTC to output a different mode than what the display requires. + Now that we have support for multiple bridges, it's not as straightforward, + and we've so far just pretended to ignore the problem. The .mode_fixup() + operation is used and abused, and just telling people to use .atomic_check() + will likely make things worse as that operation has access to the full atomic + commit and can alter the mode of pretty much anything. We need to define clear + semantics for .atomic_check() in bridges. + + +Contact: bridge maintainers, Sam Ravnborg sam@ravnborg.org, + Laurent Pinchart laurent.pinchart@ideasonboard.com + +Level: Beginner or intermediate (depending on the driver) + +Move connector creation to display drivers +------------------------------------------ + +With the introduction of chained bridges the creation of connectors are moved +to the display drivers. The flag DRM_BRIDGE_ATTACH_NO_CONNECTOR is used to +signal to the bridge driver that no connector shall be created and that the +display driver will take care. Display drivers will in most cases be able to +utilise drm_bridge_connector_init() for all the logic. + +Step 1 is to have all bridge drivers supporting DRM_BRIDGE_ATTACH_NO_CONNECTOR. +Step 2 is to move connector creation to all relevant display drivers, utilizing +the drm_bridge_connector where possible. + +Contact: Sam Ravnborg sam@ravnborg.org, bridge and/or driver maintainer(s) + +Level: Intermediate or advanced (depending on the driver)
Core refactorings =================
dri-devel@lists.freedesktop.org
-
Laurent Pinchart -
Maxime Ripard -
Sam Ravnborg