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.
Sam
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_bridge_new_crtc_state() 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 | 47 ++++++++++ drivers/gpu/drm/bridge/lontium-lt9611.c | 69 ++++++--------- drivers/gpu/drm/bridge/parade-ps8640.c | 14 +-- drivers/gpu/drm/drm_atomic.c | 34 ++++++++ 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, 119 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 functions - convert these to the atomic variants.
Signed-off-by: Sam Ravnborg sam@ravnborg.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 7bd0affa057a..58f8122ffb03 100644 --- a/drivers/gpu/drm/bridge/parade-ps8640.c +++ b/drivers/gpu/drm/bridge/parade-ps8640.c @@ -189,7 +189,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; @@ -201,7 +202,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);
@@ -287,7 +289,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); @@ -297,7 +299,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; } @@ -305,8 +307,8 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge, static const struct drm_bridge_funcs ps8640_bridge_funcs = { .attach = ps8640_bridge_attach, .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)
On Thu, Jul 22, 2021 at 08:22:40AM +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 functions - convert these to the atomic variants.
Signed-off-by: Sam Ravnborg sam@ravnborg.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
Reviewed-by: Maxime Ripard maxime@cerno.tech
Maxime
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 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 044acd07c153..f34a3382a738 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -415,61 +415,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 @@ -499,61 +444,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 46bdfa48c413..7bf3d44ecf46 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, @@ -860,13 +836,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,
On Thu, Jul 22, 2021 at 08:22:41AM +0200, Sam Ravnborg wrote:
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 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
Reviewed-by: Maxime Ripard maxime@cerno.tech
Maxime
drm_bridge_new_crtc_state() 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 atomic operations of struct drm_bridge_funcs.
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 | 34 ++++++++++++++++++++++++++++++++++ include/drm/drm_atomic.h | 3 +++ 2 files changed, 37 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index a8bbb021684b..93d513078e9a 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1133,6 +1133,40 @@ drm_atomic_get_new_bridge_state(struct drm_atomic_state *state, } EXPORT_SYMBOL(drm_atomic_get_new_bridge_state);
+/** + * drm_bridge_new_crtc_state - get crtc_state for the bridge + * @bridge: bridge object + * @old_bridge_state: state of the bridge + * + * This function returns the &struct drm_crtc_state for the given bridge/state, + * or NULL if no crtc_state could be looked up. In case no crtc_state then this is + * logged using WARN as the crtc_state is always expected to be present. + * This function is often used in the &struct drm_bridge_funcs operations. + */ +const struct drm_crtc_state * +drm_bridge_new_crtc_state(struct drm_bridge *bridge, + struct drm_bridge_state *old_bridge_state) +{ + struct drm_atomic_state *state = old_bridge_state->base.state; + const struct drm_connector_state *conn_state; + const struct drm_crtc_state *crtc_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; + + crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc); + if (WARN_ON(!crtc_state)) + return NULL; + + return crtc_state; +} + /** * 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..a3001eef98bf 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_bridge_new_crtc_state(struct drm_bridge *bridge, + struct drm_bridge_state *old_bridge_state);
#endif /* DRM_ATOMIC_H_ */
Hi,
On Thu, Jul 22, 2021 at 08:22:42AM +0200, Sam Ravnborg wrote:
drm_bridge_new_crtc_state() 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 atomic operations of struct drm_bridge_funcs.
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 | 34 ++++++++++++++++++++++++++++++++++ include/drm/drm_atomic.h | 3 +++ 2 files changed, 37 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index a8bbb021684b..93d513078e9a 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1133,6 +1133,40 @@ drm_atomic_get_new_bridge_state(struct drm_atomic_state *state, } EXPORT_SYMBOL(drm_atomic_get_new_bridge_state);
+/**
- drm_bridge_new_crtc_state - get crtc_state for the bridge
- @bridge: bridge object
- @old_bridge_state: state of the bridge
- This function returns the &struct drm_crtc_state for the given bridge/state,
- or NULL if no crtc_state could be looked up. In case no crtc_state then this is
- logged using WARN as the crtc_state is always expected to be present.
- This function is often used in the &struct drm_bridge_funcs operations.
- */
+const struct drm_crtc_state * +drm_bridge_new_crtc_state(struct drm_bridge *bridge,
Shouldn't we call this drm_atomic_bridge_get_new_crtc_state for consistency?
struct drm_bridge_state *old_bridge_state)
It seems odd to me to name it old_bridge_state, I guess it would make more sense to pass in the new bridge state?
+{
- struct drm_atomic_state *state = old_bridge_state->base.state;
- const struct drm_connector_state *conn_state;
- const struct drm_crtc_state *crtc_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;- crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
- if (WARN_ON(!crtc_state))
return NULL;- return crtc_state;
You don't even seem to use the bridge state itself, so maybe we just need to pass the drm_atomic_state? And thus we end up with something like drm_atomic_get_new_connector_for_encoder, so maybe we should just call it drm_atomic_get_new_crtc_for_bridge?
Also, can we end up with a commit that affects the bridge state but not the crtc state (like a connector property change)? In such a case drm_atomic_get_new_crtc_state would return NULL.
I'm not sure if it's a big deal or not, but we should make it clear in the documentation and remove the WARN_ON.
Maxime
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()
Signed-off-by: Sam Ravnborg sam@ravnborg.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/lontium-lt9611.c | 69 ++++++++++--------------- 1 file changed, 27 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/bridge/lontium-lt9611.c b/drivers/gpu/drm/bridge/lontium-lt9611.c index 29b1ce2140ab..dfa7baefe2ab 100644 --- a/drivers/gpu/drm/bridge/lontium-lt9611.c +++ b/drivers/gpu/drm/bridge/lontium-lt9611.c @@ -700,9 +700,17 @@ 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_bridge_new_crtc_state(bridge, old_bridge_state); + mode = &crtc_state->mode;
if (lt9611_power_on(lt9611)) { dev_err(lt9611->dev, "power on failed\n"); @@ -719,9 +727,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 +897,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 +940,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,
Hi,
On Thu, Jul 22, 2021 at 08:22:43AM +0200, Sam Ravnborg wrote:
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()
Signed-off-by: Sam Ravnborg sam@ravnborg.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/lontium-lt9611.c | 69 ++++++++++--------------- 1 file changed, 27 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/bridge/lontium-lt9611.c b/drivers/gpu/drm/bridge/lontium-lt9611.c index 29b1ce2140ab..dfa7baefe2ab 100644 --- a/drivers/gpu/drm/bridge/lontium-lt9611.c +++ b/drivers/gpu/drm/bridge/lontium-lt9611.c @@ -700,9 +700,17 @@ 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_bridge_new_crtc_state(bridge, old_bridge_state);
- mode = &crtc_state->mode;
So, yeah, it looks like you can't make the assumption that crtc_state is going to be valid here.
I'm not entirely clear on how bridge states are allocated, but it looks to me that they are through drm_atomic_add_encoder_bridges, which is called for all the affected connectors in a commit here:
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_atomic_he...
Then, the atomic_enable call is made through drm_atomic_bridge_chain_enable(), which is called in drm_atomic_helper_commit_modeset_enables only if the CRTC is active and needs a modeset.
I guess this means that we won't have a null pointer for crtc_state there, but wouldn't that cause some issues? I can imagine a property like the bpc count or output format where it wouldn't imply a modeset but would definitely affect the bridges in the chain?
Maxime
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 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 dea46d66e712..c45e4dbd9a2f 100644 --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c @@ -1206,22 +1206,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 (mode->clock < 27000) return MODE_CLOCK_LOW; if (mode->clock > 297000)
On Thu, Jul 22, 2021 at 08:22:44AM +0200, Sam Ravnborg wrote:
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 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
Reviewed-by: Maxime Ripard maxime@cerno.tech
Maxime
There are no users left and we do not want to have this function available.
drm_atomic_bridge_check() is used to call the mode_fixup() operation for the chained bridges and there is no need for drm_atomic_bridge_check(). Drop it.
Signed-off-by: Sam Ravnborg sam@ravnborg.org 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 f34a3382a738..fff5abb2a733 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -336,43 +336,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 7bf3d44ecf46..a39531406bfe 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -829,9 +829,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,
On Thu, Jul 22, 2021 at 08:22:45AM +0200, Sam Ravnborg wrote:
There are no users left and we do not want to have this function available.
drm_atomic_bridge_check() is used to call the mode_fixup() operation for the chained bridges and there is no need for drm_atomic_bridge_check(). Drop it.
Signed-off-by: Sam Ravnborg sam@ravnborg.org 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
Reviewed-by: Maxime Ripard maxime@cerno.tech
Maxime
- deprecated callbacks in drm_bridge_funcs - move connector creation to display drivers
Signed-off-by: Sam Ravnborg sam@ravnborg.org 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 | 47 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)
diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst index 12e61869939e..0ed1f49df73e 100644 --- a/Documentation/gpu/todo.rst +++ b/Documentation/gpu/todo.rst @@ -477,6 +477,53 @@ Contact: Thomas Zimmermann tzimmermann@suse.de, Christian König, Daniel Vette
Level: Intermediate
+Drop use of deprecated callbacks in bridge drivers +-------------------------------------------------- + +&struct drm_bridge_funcs contains a number of deprecated operations +which use can be replaced by the atomic variants. + +* pre_enable => atomic_pre_enable +* enable => atomic_enable +* disable => atomic_disable +* post_disable => atomic_post_disable + +The above are in most cases a simple adjustment of the arguments and names. + +* 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: 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. + +This task requires all bridge drivers to support optional or no connector creation and all +display drivers using bridges to support creating the connector. + +Contact: Sam Ravnborg sam@ravnborg.org, bridge maintainers, driver maintainer + +Level: Intermediate or advanced (depending on the driver)
Core refactorings =================
On Thu, Jul 22, 2021 at 08:22:46AM +0200, Sam Ravnborg wrote:
- deprecated callbacks in drm_bridge_funcs
- move connector creation to display drivers
Signed-off-by: Sam Ravnborg sam@ravnborg.org 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
Acked-by: Maxime Ripard maxime@cerno.tech
Maxime
dri-devel@lists.freedesktop.org
-
Maxime Ripard -
Sam Ravnborg