[PATCH v5 00/10] Add support for hot-pluggable DRM bridges

[PATCH v5 00/10] Add support for hot-pluggable DRM bridges

[Luca Ceresoli]

Hello,

this series aims at supporting Linux devices with a DRM pipeline whose
final components can be hot-plugged and hot-unplugged, including one or
more bridges.

If you already know the use case and the approach, feel free to skip to
"Roadmap and current status" below for the ongoing changes.

Use case
========

This series targets a professional product (GE SUNH) that is composed of a
"main" part running on battery, with the main SoC and able to work
autonomously with limited features, and an optional "add-on" that enables
more features by adding more hardware peripherals.

The hotplug connector has a MIPI DSI bus. The addon has a DSI-to-LVDS
bridge and an LVDS panel attached to it. Different addon models can have
different components. As a consequence, a DRM bridge must be added and
removed at runtime without tearing down the whole card, which is currently
not possible.

Up to v4 (link at the bottom) there was a single patch series implementing
both the hotplug connector driver (based on device tree overlays) and the
DRM aspects. Starting with v5 we have split the two series to let each part
of the work progress with its own pace.

DRM hotplug bridge driver
=========================

DRM natively supports pipelines whose display can be removed, but all the
components preceding it (all the display controller and any bridges) are
assumed to be fixed and cannot be plugged, removed or modified at runtime.

This series adds support for DRM pipelines having a removable part after
the encoder, thus also allowing bridges to be removed and reconnected at
runtime, possibly with different components.

This picture summarizes the DRM structure implemented by this series:

 .------------------------.
 |   DISPLAY CONTROLLER   |
 | .---------.   .------. |
 | | ENCODER |<--| CRTC | |
 | '---------'   '------' |
 '------|-----------------'
        |
        |DSI            HOTPLUG
        V              CONNECTOR
   .---------.        .--.    .-.        .---------.         .-------.
   | 0 to N  |        | _|   _| |        | 1 to N  |         |       |
   | BRIDGES |--DSI-->||_   |_  |--DSI-->| BRIDGES |--LVDS-->| PANEL |
   |         |        |  |    | |        |         |         |       |
   '---------'        '--'    '-'        '---------'         '-------'

 [--- fixed components --]  [----------- removable add-on -----------]

Fixed components include:

 * all components up to the DRM encoder, usually part of the SoC
 * optionally some bridges, in the SoC and/or as external chips

Components on the removable add-on include:

 * one or more bridges
 * a fixed connector (not one natively supporting hotplug such as HDMI)
 * the panel

The video bus is MIPI DSI in the example and in the implementation provided
by this series, but the implementation is meant to allow generalization to
other video busses without native hotplug support, such as parallel video
and LVDS.

Note that the term "connector" in this context is different from the "DRM
connector" abstraction already present in the DRM subsystem (struct
drm_connector).

More details in the commit message of patch 4.

Roadmap and current status
==========================

Up to v4 the design idea was heavily based on a "hotplug bridge driver" to
decouple the two sides of the DRM pipeline. After a long discussion during
Linux Plumbers Conference 2024, a different strategy was agreed with other
DRM developers, which adds:

 1. add refcounting to DRM bridges (struct drm_bridge)
 2. handle gracefully atomic updates during bridge removal
 3. avoid DSI host drivers to have dangling pointers to DSI devices
 4. finish the hotplug bridge work, moving code to the core and
    potentially removing the hotplug-bridge itself (this needs to be
    clarified as points 1-3 are developed)

This version implements the first item. Items 2-4 have not yet been
developed. This version is sent to allow discussing the bridge refcounting
implementation as soon as possible.

Patch series overview
=====================

 * 2 Preliminary patches (may to be removed as this work progresses):
   - drm/bridge: allow bridges to be informed about added and removed bridges
   - drm/encoder: add drm_encoder_cleanup_from()

 * Implement refcounting in the drm_bridge core:
   - drm/bridge: add support for refcounted DRM bridges
   - drm/tests: bridge: add KUnit tests for DRM bridges (init and destroy)
   - drm/bridge: add documentation of refcounted bridges

 * Adapt some existing bridges to be refcounted and/or to refcount bridges
   they take a pointer to:
   - drm/bridge: ti-sn65dsi83: use dynamic lifetime management
   - drm/bridge: panel: use dynamic lifetime management
   - drm/bridge: samsung-dsim: use supporting variable for out_bridge
   - drm/bridge: samsung-dsim: refcount the out_bridge

 * Add hotplug-bridge (may be removed as this work progresses): 
   - drm/bridge: hotplug-bridge: add driver to support hot-pluggable DSI bridges

That's all
==========

Thanks for you patience in reading this!

Luca

Changes in v5:
- Implemented DRM bridge refcounting
- removed the non-DRM patches, now in a separate series
- Updated To/Cc list
- Link to v4: https://lore.kernel.org/r/20240917-hotplug-drm-bridge-v4-0-bc4dfee61be6@bootlin.com

Changes in v4:
- Replaced DRM bridge notifier with a new callback in struct drm_bridge_funcs
- Added patch for missing devlink (LEDs used by backlight)
- Various cleanups
- Rebased on v6.11
- Link to v3: https://lore.kernel.org/r/20240809-hotplug-drm-bridge-v3-0-b4c178380bc9@bootlin.com

Changes in v3 (too many changes in v3 to mention them all, but here are the
big ones):
- Rewrote the DT format to allow fully decoupled overlays and to avoid
  adding properties (with the NVMEM exception still to be solved)
- Implemented device instantiation based on the new DT format: i2c in
  i2c-core-of.c nobus-devices in the connector driver
- DRM: insert/remove an LVDS DRM connector on hot(un)plug events
- Added patch for a devlink issue on overlay removal (mostly to start
  discussion)
- Link to v2: https://lore.kernel.org/r/20240510-hotplug-drm-bridge-v2-0-ec32f2c66d56@bootlin.com

Changes in v2:
- Added bindings and driver for ge,sunh-addon-connector
- Removed bindings for the hotplug-video-connector, this is now represented
  in DT as part of the ge,sunh-addon-connector
- Various monior improvements to the DRM hotplug-bridge driver
- Link to v1: https://lore.kernel.org/r/20240326-hotplug-drm-bridge-v1-0-4b51b5eb75d5@bootlin.com

Co-developed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
Luca Ceresoli (10):
      drm/bridge: allow bridges to be informed about added and removed bridges
      drm/encoder: add drm_encoder_cleanup_from()
      drm/bridge: add support for refcounted DRM bridges
      drm/bridge: add documentation of refcounted bridges
      drm/tests: bridge: add KUnit tests for DRM bridges (init and destroy)
      drm/bridge: ti-sn65dsi83: use dynamic lifetime management
      drm/bridge: panel: use dynamic lifetime management
      drm/bridge: samsung-dsim: use supporting variable for out_bridge
      drm/bridge: samsung-dsim: refcount the out_bridge
      drm/bridge: hotplug-bridge: add driver to support hot-pluggable DSI bridges

 Documentation/gpu/drm-kms-helpers.rst   |   6 +
 MAINTAINERS                             |   5 +
 drivers/gpu/drm/bridge/Kconfig          |  17 +
 drivers/gpu/drm/bridge/Makefile         |   1 +
 drivers/gpu/drm/bridge/hotplug-bridge.c | 695 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/bridge/panel.c          |  20 +-
 drivers/gpu/drm/bridge/samsung-dsim.c   |  26 +-
 drivers/gpu/drm/bridge/ti-sn65dsi83.c   |  13 +-
 drivers/gpu/drm/drm_bridge.c            | 221 ++++++++++
 drivers/gpu/drm/drm_encoder.c           |  21 +
 drivers/gpu/drm/tests/Makefile          |   1 +
 drivers/gpu/drm/tests/drm_bridge_test.c | 128 ++++++
 include/drm/drm_bridge.h                | 125 ++++++
 include/drm/drm_encoder.h               |   1 +
 14 files changed, 1261 insertions(+), 19 deletions(-)
---
base-commit: be4c56b4b053bc75d98260740df4f9ec261c9699
change-id: 20240319-hotplug-drm-bridge-16b86e67fe92

Best regards,
-- 
Luca Ceresoli <luca.ceresoli@bootlin.com>

[PATCH v5 01/10] drm/bridge: allow bridges to be informed about added and removed bridges

[Luca Ceresoli]

In preparation for allowing bridges to be added to and removed from a DRM
card without destroying the whole card, add a new DRM bridge function
called on addition and removal of bridges.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

Changed in v5:
 - fixed kerneldoc errors

This patch was added in v4.
---
 drivers/gpu/drm/drm_bridge.c | 12 ++++++++++++
 include/drm/drm_bridge.h     | 23 +++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index c6af46dd02bfa9e15b59e4c460debdd7fd84be44..b1f0d25d55e23000521ac2ac37ee410348978ed4 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -205,8 +205,14 @@ static LIST_HEAD(bridge_list);
  */
 void drm_bridge_add(struct drm_bridge *bridge)
 {
+	struct drm_bridge *br, *tmp;
+
 	mutex_init(&bridge->hpd_mutex);
 
+	list_for_each_entry_safe(br, tmp, &bridge_list, list)
+		if (br->funcs->bridge_event_notify)
+			br->funcs->bridge_event_notify(br, DRM_EVENT_BRIDGE_ADD, bridge);
+
 	mutex_lock(&bridge_lock);
 	list_add_tail(&bridge->list, &bridge_list);
 	mutex_unlock(&bridge_lock);
@@ -243,10 +249,16 @@ EXPORT_SYMBOL(devm_drm_bridge_add);
  */
 void drm_bridge_remove(struct drm_bridge *bridge)
 {
+	struct drm_bridge *br, *tmp;
+
 	mutex_lock(&bridge_lock);
 	list_del_init(&bridge->list);
 	mutex_unlock(&bridge_lock);
 
+	list_for_each_entry_safe(br, tmp, &bridge_list, list)
+		if (br->funcs->bridge_event_notify)
+			br->funcs->bridge_event_notify(br, DRM_EVENT_BRIDGE_REMOVE, bridge);
+
 	mutex_destroy(&bridge->hpd_mutex);
 }
 EXPORT_SYMBOL(drm_bridge_remove);
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index e8d735b7f6a480468c88287e2517b387ceec0f22..6976bd842cedf9ce06abfb7306e7a3b4915f0378 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -54,6 +54,11 @@ enum drm_bridge_attach_flags {
 	DRM_BRIDGE_ATTACH_NO_CONNECTOR = BIT(0),
 };
 
+enum drm_bridge_event_type {
+	DRM_EVENT_BRIDGE_ADD,
+	DRM_EVENT_BRIDGE_REMOVE,
+};
+
 /**
  * struct drm_bridge_funcs - drm_bridge control functions
  */
@@ -676,6 +681,24 @@ struct drm_bridge_funcs {
 				    enum hdmi_infoframe_type type,
 				    const u8 *buffer, size_t len);
 
+	/**
+	 * @bridge_event_notify:
+	 *
+	 * Notify that another bridge is being added or removed.
+	 *
+	 * This callback is optional. Bridges implementing it must always
+	 * check whether the event refers to a bridge they actually need to
+	 * interact with.
+	 *
+	 * @bridge: bridge being notified
+	 * @event: event happened (add/remove bridge)
+	 * @event_bridge: the bridge mentioned by the event (i.e. the
+	 * bridge being added or removed)
+	 */
+	void (*bridge_event_notify)(struct drm_bridge *bridge,
+				    enum drm_bridge_event_type event,
+				    struct drm_bridge *event_bridge);
+
 	/**
 	 * @debugfs_init:
 	 *

-- 
2.34.1

[PATCH v5 02/10] drm/encoder: add drm_encoder_cleanup_from()

[Luca Ceresoli]

Supporting hardware whose final part of the DRM pipeline can be physically
removed requires the ability to detach all bridges from a given point to
the end of the pipeline.

Introduce a variant of drm_encoder_cleanup() for this.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

Changes in v5: none
Changes in v4: none
Changes in v3: none

Changed in v2:
 - fix a typo in a comment
---
 drivers/gpu/drm/drm_encoder.c | 21 +++++++++++++++++++++
 include/drm/drm_encoder.h     |  1 +
 2 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index 8f2bc6a28482229fd0b030a1958f87753ad7885f..472dfbefe2960924a4e83bec425af8c7ef5f5265 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -207,6 +207,27 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
 }
 EXPORT_SYMBOL(drm_encoder_cleanup);
 
+/**
+ * drm_encoder_cleanup_from - remove a given bridge and all the following
+ * @encoder: encoder whole list of bridges shall be pruned
+ * @bridge: first bridge to remove
+ *
+ * Removes from an encoder all the bridges starting with a given bridge
+ * and until the end of the chain.
+ *
+ * This should not be used in "normal" DRM pipelines. It is only useful for
+ * devices whose final part of the DRM chain can be physically removed and
+ * later reconnected (possibly with different hardware).
+ */
+void drm_encoder_cleanup_from(struct drm_encoder *encoder, struct drm_bridge *bridge)
+{
+	struct drm_bridge *next;
+
+	list_for_each_entry_safe_from(bridge, next, &encoder->bridge_chain, chain_node)
+		drm_bridge_detach(bridge);
+}
+EXPORT_SYMBOL(drm_encoder_cleanup_from);
+
 static void drmm_encoder_alloc_release(struct drm_device *dev, void *ptr)
 {
 	struct drm_encoder *encoder = ptr;
diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
index 977a9381c8ba943b4d3e021635ea14856df8a17d..bafcabb242674880a97dfb62a50d93cc4d80c1d4 100644
--- a/include/drm/drm_encoder.h
+++ b/include/drm/drm_encoder.h
@@ -320,6 +320,7 @@ static inline struct drm_encoder *drm_encoder_find(struct drm_device *dev,
 }
 
 void drm_encoder_cleanup(struct drm_encoder *encoder);
+void drm_encoder_cleanup_from(struct drm_encoder *encoder, struct drm_bridge *bridge);
 
 /**
  * drm_for_each_encoder_mask - iterate over encoders specified by bitmask

-- 
2.34.1

[PATCH v5 03/10] drm/bridge: add support for refcounted DRM bridges

[Luca Ceresoli]

DRM bridges are currently considered as a fixed element of a DRM card, and
thus their lifetime is assumed to extend for as long as the card
exists. New use cases, such as hot-pluggable hardware with video bridges,
require DRM bridges to be added and removed to a DRM card without tearing
the card down. This is possible for connectors already (used by DP MST), so
add this possibility to DRM bridges as well.

Implementation is based on drm_connector_init() as far as it makes sense,
and differs when it doesn't. A difference is that bridges are not exposed
to userspace,hence struct drm_bridge does not embed a struct
drm_mode_object which would provide the refcount and the free_cb. So here
we add to struct drm_bridge just the refcount and free_cb fields (we don't
need other struct drm_mode_object fields here) and instead of using the
drm_mode_object_*() functions we reimplement from those functions the few
lines that drm_bridge needs for refcounting.

The function to enroll a private bridge driver data structure into
refcounting is based on drm_connector_init() and so called
drm_bridge_init() for symmetry, even though it does not initialize anything
except the refcounting and the funcs pointer which is needed to access
funcs->destroy.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

This patch was added in v5.
---
 drivers/gpu/drm/drm_bridge.c |  87 ++++++++++++++++++++++++++++++++++++
 include/drm/drm_bridge.h     | 102 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 189 insertions(+)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index b1f0d25d55e23000521ac2ac37ee410348978ed4..6255ef59f73d8041a8cb7f2c6e23e5a67d1ae926 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -198,6 +198,85 @@
 static DEFINE_MUTEX(bridge_lock);
 static LIST_HEAD(bridge_list);
 
+static void drm_bridge_put_void(void *data)
+{
+	struct drm_bridge *bridge = (struct drm_bridge *)data;
+
+	drm_bridge_put(bridge);
+}
+
+static void drm_bridge_free(struct kref *kref)
+{
+	struct drm_bridge *bridge = container_of(kref, struct drm_bridge, refcount);
+
+	DRM_DEBUG("bridge=%p\n", bridge);
+
+	WARN_ON(!bridge->funcs->destroy);
+
+	if (bridge->funcs->destroy)
+		bridge->funcs->destroy(bridge);
+}
+
+/**
+ * drm_bridge_init - Initialize bridge and move private driver data
+ *                   lifetime management to the DRM bridge core
+ *
+ * @dev: struct device of the device whose removal shall trigger deallocation
+ * @bridge: the bridge to initialize
+ * @funcs: funcs structure for @bridge, which must have a valid .destroy func
+ *
+ * Takes over lifetime of a private bridge driver struct which embeds a
+ * struct drm_bridge. To be called by bridge drivers just after having
+ * allocated such a private structure. Initializes refcount to 1 and
+ * installs a callback which will call funcs->destroy when refcount drops
+ * to zero.
+ *
+ * After calling this function a bridge becomes a bridge with dynamic
+ * lifetime (aka a refcounted bridge).
+ *
+ * Drivers calling this function:
+ *  - must not allocate the private structure using devm_*() functions
+ *  - must not deallocate the private structure on device removal
+ *  - must deallocate the private structure in funcs->destroy
+ *
+ * Drivers not calling this function:
+ *  - must take care of freeing their private structure either by allocating
+ *    it using devm_*() functions or free it explicitly on device removal
+ *    using kfree()
+ *  - must set funcs->destroy to NULL
+ *
+ * On failure, calls funcs->destroy, thus the caller does not need to free
+ * the driver private struct in case of error.
+ *
+ * Returns:
+ * Zero on success, error code on failure.
+ */
+int drm_bridge_init(struct device *dev,
+		    struct drm_bridge *bridge,
+		    const struct drm_bridge_funcs *funcs)
+{
+	int err;
+
+	DRM_DEBUG("bridge=%p, funcs=%ps\n", bridge, funcs);
+
+	if (!(funcs && funcs->destroy)) {
+		dev_warn(dev, "Missing funcs or destroy func pointer\n");
+		return -EINVAL;
+	}
+
+	bridge->free_cb = drm_bridge_free;
+	kref_init(&bridge->refcount);
+
+	err = devm_add_action_or_reset(dev, drm_bridge_put_void, bridge);
+	if (err)
+		return err;
+
+	bridge->funcs = funcs;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_bridge_init);
+
 /**
  * drm_bridge_add - add the given bridge to the global bridge list
  *
@@ -207,6 +286,10 @@ void drm_bridge_add(struct drm_bridge *bridge)
 {
 	struct drm_bridge *br, *tmp;
 
+	DRM_DEBUG("bridge=%p\n", bridge);
+
+	drm_bridge_get(bridge);
+
 	mutex_init(&bridge->hpd_mutex);
 
 	list_for_each_entry_safe(br, tmp, &bridge_list, list)
@@ -251,6 +334,8 @@ void drm_bridge_remove(struct drm_bridge *bridge)
 {
 	struct drm_bridge *br, *tmp;
 
+	DRM_DEBUG("bridge=%p\n", bridge);
+
 	mutex_lock(&bridge_lock);
 	list_del_init(&bridge->list);
 	mutex_unlock(&bridge_lock);
@@ -260,6 +345,8 @@ void drm_bridge_remove(struct drm_bridge *bridge)
 			br->funcs->bridge_event_notify(br, DRM_EVENT_BRIDGE_REMOVE, bridge);
 
 	mutex_destroy(&bridge->hpd_mutex);
+
+	drm_bridge_put(bridge);
 }
 EXPORT_SYMBOL(drm_bridge_remove);
 
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 6976bd842cedf9ce06abfb7306e7a3b4915f0378..a548a6acb02e3d70c8e34de965f648320420a7d5 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -31,6 +31,7 @@
 #include <drm/drm_encoder.h>
 #include <drm/drm_mode_object.h>
 #include <drm/drm_modes.h>
+#include <drm/drm_print.h>
 
 struct device_node;
 
@@ -89,6 +90,18 @@ struct drm_bridge_funcs {
 	 */
 	void (*detach)(struct drm_bridge *bridge);
 
+	/**
+	 * @destroy:
+	 *
+	 * Clean up bridge resources for bridges with dynamic
+	 * lifetime. This is called when the bridge refcount drops to
+	 * zero. It is never called for bridges without dynamic lifetime.
+	 *
+	 * See drm_bridge_init() for info about bridges with dynamic
+	 * lifetime.
+	 */
+	void (*destroy)(struct drm_bridge *bridge);
+
 	/**
 	 * @mode_valid:
 	 *
@@ -810,6 +823,18 @@ struct drm_bridge {
 	const struct drm_bridge_timings *timings;
 	/** @funcs: control functions */
 	const struct drm_bridge_funcs *funcs;
+
+	/**
+	 * @refcount: reference count for bridges with dynamic lifetime
+	 * (see drm_bridge_init)
+	 */
+	struct kref refcount;
+	/**
+	 * @free_cb: free function callback, only set for bridges with
+	 * dynamic lifetime
+	 */
+	void (*free_cb)(struct kref *kref);
+
 	/** @driver_private: pointer to the bridge driver's internal context */
 	void *driver_private;
 	/** @ops: bitmask of operations supported by the bridge */
@@ -890,6 +915,83 @@ drm_priv_to_bridge(struct drm_private_obj *priv)
 	return container_of(priv, struct drm_bridge, base);
 }
 
+static inline bool drm_bridge_is_refcounted(struct drm_bridge *bridge)
+{
+	return bridge->free_cb;
+}
+
+/**
+ * drm_bridge_get - Acquire a bridge reference
+ * @bridge: DRM bridge
+ *
+ * This function increments the bridge's refcount.
+ *
+ * It does nothing on non-refcounted bridges. See drm_bridge_init().
+ */
+static inline void drm_bridge_get(struct drm_bridge *bridge)
+{
+	if (!drm_bridge_is_refcounted(bridge))
+		return;
+
+	DRM_DEBUG("bridge=%p GET\n", bridge);
+
+	kref_get(&bridge->refcount);
+}
+
+/**
+ * drm_bridge_put - Release a bridge reference
+ * @bridge: DRM bridge
+ *
+ * This function decrements the bridge's reference count and frees the
+ * object if the reference count drops to zero.
+ *
+ * It does nothing on non-refcounted bridges. See drm_bridge_init().
+ *
+ * See also drm_bridge_put_and_clear() which is more handy in many cases.
+ */
+static inline void drm_bridge_put(struct drm_bridge *bridge)
+{
+	if (!drm_bridge_is_refcounted(bridge))
+		return;
+
+	DRM_DEBUG("bridge=%p PUT\n", bridge);
+
+	kref_put(&bridge->refcount, bridge->free_cb);
+}
+
+/**
+ * drm_bridge_put_and_clear - Given a bridge pointer, clear the pointer
+ *                            then put the bridge
+ *
+ * @br_ptr: pointer to a struct drm_bridge (must be != NULL)
+ *
+ * Helper to put a DRM bridge (whose pointer is passed), but only after
+ * setting its pointer to NULL. Useful for drivers having struct drm_bridge
+ * pointers they need to dispose of, without leaving a use-after-free
+ * window where the pointed bridge might have been freed while still
+ * holding a pointer to it.
+ *
+ * For example a driver having this private struct::
+ *
+ *     struct my_bridge {
+ *         struct drm_bridge *remote_bridge;
+ *         ...
+ *     };
+ *
+ * can dispose of remote_bridge using::
+ *
+ *     drm_bridge_put_and_clear(my_bridge->remote_bridge);
+ */
+#define drm_bridge_put_and_clear(br_ptr) do { \
+	struct drm_bridge **brpp = &(br_ptr); \
+	struct drm_bridge *brp = *brpp; \
+	*brpp = NULL; \
+	drm_bridge_put(brp); \
+} while (0)
+
+int drm_bridge_init(struct device *dev,
+		    struct drm_bridge *bridge,
+		    const struct drm_bridge_funcs *funcs);
 void drm_bridge_add(struct drm_bridge *bridge);
 int devm_drm_bridge_add(struct device *dev, struct drm_bridge *bridge);
 void drm_bridge_remove(struct drm_bridge *bridge);

-- 
2.34.1

[PATCH v5 04/10] drm/bridge: add documentation of refcounted bridges

[Luca Ceresoli]

Document in detail the new refcounted bridges as well as the "legacy" way.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

This patch was added in v5.
---
 Documentation/gpu/drm-kms-helpers.rst |   6 ++
 drivers/gpu/drm/drm_bridge.c          | 122 ++++++++++++++++++++++++++++++++++
 2 files changed, 128 insertions(+)

diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
index 8cf2f041af4704875910ce8228ae04615d0f21bd..ca2cfef2101988933e1464fe146997c1a661a117 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -151,6 +151,12 @@ Overview
 .. kernel-doc:: drivers/gpu/drm/drm_bridge.c
    :doc: overview
 
+Bridge lifecycle
+----------------
+
+.. kernel-doc:: drivers/gpu/drm/drm_bridge.c
+   :doc: bridge lifecycle
+
 Display Driver Integration
 --------------------------
 
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 6255ef59f73d8041a8cb7f2c6e23e5a67d1ae926..e9f138aa5b3270b4e3a1a56dc8d4b7e5f993c929 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -60,6 +60,128 @@
  * encoder chain.
  */
 
+/**
+ * DOC: bridge lifecycle
+ *
+ * Allocation, initializion and teardown of a bridge can be implemented in
+ * one of two ways: *refcounted* mode and *legacy* mode.
+ *
+ * In **refcounted** mode:
+ *
+ * - each &struct drm_bridge is reference counted since its instantiation
+ * - any code taking a pointer to a bridge has get and put APIs to refcount
+ *   it and so ensure the bridge won't be deallocated while using it
+ * - deallocation is done when the last put happens and the refcount drops
+ *   to zero
+ * - the driver instantiating the bridge also holds a reference, but the
+ *   allocated struct can survive it
+ *
+ * A bridge using refcounted mode is called a *refcounted bridge*.
+ *
+ * In **legacy** mode the &struct drm_bridge lifetime is tied to the device
+ * instantiating it: it is allocated on probe and freed on removal. Any
+ * other kernel entities holding a pointer to the bridge could incur in
+ * use-after-free in case the bridge is deallocated at runtime.
+ *
+ * Legacy mode used to be the only one until refcounted bridges were
+ * introduced, hance the name. It is still fine in case the bridges are a
+ * fixed part of the pipeline, i.e. if the bridges are removed only when
+ * tearing down the entire card. Refcounted bridges support both that case
+ * and the case of more dynamic hardware with bridges that can be removed
+ * at runtime without tearing down the entire card.
+ *
+ * Usage of refcounted bridges happens in two sides: the driver
+ * implementing the bridge and the code using the bridge.
+ *
+ * For *drivers implemeting the bridge*, in both refcounted and legacy
+ * modes the common and expected pattern is that the driver declares a
+ * driver-specific struct embedding a &struct drm_bridge. E.g.::
+ *
+ *   struct my_bridge {
+ *       ...
+ *       struct drm_bridge bridge;
+ *       ...
+ *   };
+ *
+ * When using refcounted mode, the driver should allocate ``struct
+ * my_bridge`` using regular allocation (as opposed to ``devm_`` or
+ * ``drmm_`` allocation), call drm_bridge_init() immediately afterwards to
+ * transfer lifecycle management to the DRM bridge core, and implement a
+ * ``.destroy`` function to deallocate the ``struct my_bridge``, as in this
+ * example::
+ *
+ *     static void my_bridge_destroy(struct drm_bridge *bridge)
+ *     {
+ *         kfree(container_of(bridge, struct my_bridge, bridge));
+ *     }
+ *
+ *     static const struct drm_bridge_funcs my_bridge_funcs = {
+ *         .destroy = my_bridge_destroy,
+ *         ...
+ *     };
+ *
+ *     static int my_bridge_probe(...)
+ *     {
+ *         struct my_bridge *mybr;
+ *         int err;
+ *
+ *         mybr = kzalloc(sizeof(*mybr), GFP_KERNEL);
+ *         if (!mybr)
+ *             return -ENOMEM;
+ *
+ *         err = drm_bridge_init(dev, &mybr->bridge, &my_bridge_funcs);
+ *         if (err)
+ *             return err;
+ *
+ *         ...
+ *         drm_bridge_add();
+ *         ...
+ *     }
+ *
+ *     static void my_bridge_remove()
+ *     {
+ *         struct my_bridge *mybr = ...;
+ *         drm_bridge_remove(&mybr->bridge);
+ *         // ... NO kfree here!
+ *     }
+ *
+ * In legacy mode, the driver can either use ``devm_`` allocation or
+ * equivalently free ``struct my_bridge`` in their remove function::
+ *
+ *     static int my_bridge_probe(...)
+ *     {
+ *         struct my_bridge *mybr;
+ *
+ *         mybr = devm_kzalloc(dev, sizeof(*mybr), GFP_KERNEL);
+ *         if (!mybr)
+ *             return -ENOMEM;
+ *
+ *         ...
+ *         drm_bridge_add();
+ *         ...
+ *     }
+ *
+ *     static void my_bridge_remove()
+ *     {
+ *         struct my_bridge *mybr = ...;
+ *         drm_bridge_remove(&mybr->bridge);
+ *         // kfree(mybr) if not using devm_*() for allocation
+ *     }
+ *
+ * The *code using the bridge* is all the code taking a &struct drm_bridge
+ * pointer, including other bridges, encoders and the DRM core. As the
+ * bridge could be removed at any time, such code can incur in
+ * use-after-free. To void that, it has to call drm_bridge_get() when
+ * taking a pointer and drm_bridge_put() after it has done using it. This
+ * will extend the allocation lifetime of the bridge struct until the last
+ * reference has been put, potentially after the bridge device has been
+ * removed from the kernel.
+ *
+ * Calling drm_bridge_get() and drm_bridge_put() on a bridge that is not
+ * refcounted does nothing, so code using these two APIs will work both on
+ * refcounted bridges and non-refcounted ones.
+ */
+
 /**
  * DOC:	display driver integration
  *

-- 
2.34.1

[PATCH v5 05/10] drm/tests: bridge: add KUnit tests for DRM bridges (init and destroy)

[Luca Ceresoli]

Add two simple KUnit tests for the newly introduced drm_bridge_init() and
the corresponding .destroy deallocation function.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

This patch was added in v5.
---
 drivers/gpu/drm/tests/Makefile          |   1 +
 drivers/gpu/drm/tests/drm_bridge_test.c | 128 ++++++++++++++++++++++++++++++++
 2 files changed, 129 insertions(+)

diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile
index 56dab563abd7a7ee7c147bd6b4927e2436b82e1d..909f98a132bb1d057b2666e8b891683ffb11cca4 100644
--- a/drivers/gpu/drm/tests/Makefile
+++ b/drivers/gpu/drm/tests/Makefile
@@ -4,6 +4,7 @@ obj-$(CONFIG_DRM_KUNIT_TEST_HELPERS) += \
 	drm_kunit_helpers.o
 
 obj-$(CONFIG_DRM_KUNIT_TEST) += \
+	drm_bridge_test.o \
 	drm_buddy_test.o \
 	drm_cmdline_parser_test.o \
 	drm_connector_test.o \
diff --git a/drivers/gpu/drm/tests/drm_bridge_test.c b/drivers/gpu/drm/tests/drm_bridge_test.c
new file mode 100644
index 0000000000000000000000000000000000000000..b42e6b81a904abccfe1b3e88999b64cc6b2d4946
--- /dev/null
+++ b/drivers/gpu/drm/tests/drm_bridge_test.c
@@ -0,0 +1,128 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Kunit test for DRM bridges
+ */
+
+#include <drm/drm_bridge.h>
+#include <drm/drm_kunit_helpers.h>
+
+#include <kunit/test.h>
+
+struct drm_bridge_init_priv {
+	struct drm_device drm;
+	bool destroyed;
+};
+
+/*
+ * Mimick the typical struct defined by a bridge driver, which embeds a
+ * bridge plus other fields.
+ */
+struct dummy_drm_bridge {
+	struct drm_bridge_init_priv *init_priv;
+	struct drm_bridge bridge;
+};
+
+static struct dummy_drm_bridge *bridge_to_dummy(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct dummy_drm_bridge, bridge);
+}
+
+static void dummy_drm_bridge_destroy(struct drm_bridge *bridge)
+{
+	struct dummy_drm_bridge *dummy = bridge_to_dummy(bridge);
+
+	dummy->init_priv->destroyed = true;
+	kfree(dummy);
+}
+
+static const struct drm_bridge_funcs drm_bridge_dummy_funcs = {
+	.destroy		= dummy_drm_bridge_destroy,
+};
+
+static int drm_test_bridge_init(struct kunit *test)
+{
+	struct drm_bridge_init_priv *priv;
+	struct device *dev;
+
+	dev = drm_kunit_helper_alloc_device(test);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev);
+
+	priv = drm_kunit_helper_alloc_drm_device(test, dev,
+						 struct drm_bridge_init_priv, drm,
+						 DRIVER_MODESET | DRIVER_ATOMIC);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
+
+	test->priv = priv;
+	return 0;
+}
+
+/*
+ * Test that the allocation and initialization of a bridge works as
+ * expected and doesn't report any error.
+ */
+static void drm_test_drm_bridge_init(struct kunit *test)
+{
+	struct drm_bridge_init_priv *priv = test->priv;
+	struct dummy_drm_bridge *dummy;
+	int ret;
+
+	dummy = kzalloc(sizeof(*dummy), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dummy);
+
+	dummy->init_priv = priv;
+
+	ret = drm_bridge_init(priv->drm.dev,
+			      &dummy->bridge,
+			      &drm_bridge_dummy_funcs);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+
+	KUNIT_EXPECT_FALSE(test, priv->destroyed);
+}
+
+/*
+ * Test that the allocation and initialization of a bridge works as expected
+ * and doesn't report any error, and that the destroy func is called at the
+ * last drm_bridge_put().
+ */
+static void drm_test_drm_bridge_put(struct kunit *test)
+{
+	struct drm_bridge_init_priv *priv = test->priv;
+	struct dummy_drm_bridge *dummy;
+	int ret;
+
+	dummy = kzalloc(sizeof(*dummy), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dummy);
+
+	dummy->init_priv = priv;
+
+	ret = drm_bridge_init(priv->drm.dev,
+			      &dummy->bridge,
+			      &drm_bridge_dummy_funcs);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+
+	drm_bridge_get(&dummy->bridge);
+	drm_bridge_put(&dummy->bridge);
+	KUNIT_EXPECT_FALSE(test, priv->destroyed);
+	drm_bridge_put(&dummy->bridge);
+	KUNIT_EXPECT_TRUE(test, priv->destroyed);
+}
+
+static struct kunit_case drm_bridge_init_tests[] = {
+	KUNIT_CASE(drm_test_drm_bridge_init),
+	KUNIT_CASE(drm_test_drm_bridge_put),
+	{ }
+};
+
+static struct kunit_suite drm_bridge_init_test_suite = {
+	.name = "drm_bridge_init",
+	.init = drm_test_bridge_init,
+	.test_cases = drm_bridge_init_tests,
+};
+
+kunit_test_suites(
+	&drm_bridge_init_test_suite,
+);
+
+MODULE_AUTHOR("Luca Ceresoli <luca.ceresoli@bootlin.com>");
+MODULE_DESCRIPTION("Kunit test for drm_bridge functions");
+MODULE_LICENSE("GPL");

-- 
2.34.1

[PATCH v5 06/10] drm/bridge: ti-sn65dsi83: use dynamic lifetime management

[Luca Ceresoli]

With proper use of drm_bridge_get() and _put(), this allows the bridge to
be removable without dangling pointers and use-after-free.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

This patch was added in v5.
---
 drivers/gpu/drm/bridge/ti-sn65dsi83.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
index 246fece303bc0ae9fb2eaa9c6218ce2ecd550f3b..df3ec39817f32879f9aab65812b6e7f4723f72fb 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c
@@ -267,6 +267,11 @@ static void sn65dsi83_detach(struct drm_bridge *bridge)
 	ctx->dsi = NULL;
 }
 
+static void sn65dsi83_destroy(struct drm_bridge *bridge)
+{
+	kfree(bridge_to_sn65dsi83(bridge));
+}
+
 static u8 sn65dsi83_get_lvds_range(struct sn65dsi83 *ctx,
 				   const struct drm_display_mode *mode)
 {
@@ -695,6 +700,7 @@ sn65dsi83_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
 static const struct drm_bridge_funcs sn65dsi83_funcs = {
 	.attach			= sn65dsi83_attach,
 	.detach			= sn65dsi83_detach,
+	.destroy		= sn65dsi83_destroy,
 	.atomic_enable		= sn65dsi83_atomic_enable,
 	.atomic_pre_enable	= sn65dsi83_atomic_pre_enable,
 	.atomic_disable		= sn65dsi83_atomic_disable,
@@ -813,10 +819,14 @@ static int sn65dsi83_probe(struct i2c_client *client)
 	struct sn65dsi83 *ctx;
 	int ret;
 
-	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
 	if (!ctx)
 		return -ENOMEM;
 
+	ret = drm_bridge_init(dev, &ctx->bridge, &sn65dsi83_funcs);
+	if (ret)
+		return ret;
+
 	ctx->dev = dev;
 	INIT_WORK(&ctx->reset_work, sn65dsi83_reset_work);
 	INIT_DELAYED_WORK(&ctx->monitor_work, sn65dsi83_monitor_work);
@@ -855,7 +865,6 @@ static int sn65dsi83_probe(struct i2c_client *client)
 	dev_set_drvdata(dev, ctx);
 	i2c_set_clientdata(client, ctx);
 
-	ctx->bridge.funcs = &sn65dsi83_funcs;
 	ctx->bridge.of_node = dev->of_node;
 	ctx->bridge.pre_enable_prev_first = true;
 	drm_bridge_add(&ctx->bridge);

-- 
2.34.1

[PATCH v5 07/10] drm/bridge: panel: use dynamic lifetime management

[Luca Ceresoli]

Enable lifetime management of panel-bridge, so that other modules taking a
pointer to a panel bridge can refcount it and avoid use-after-free in case
the panel bridge is hot-unplugged.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

This patch was added in v5.
---
 drivers/gpu/drm/bridge/panel.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index 6e88339dec0f5faee690b7c53e8dcd0f1ee2281c..805809778f79f4519d9e31214cc5407357264da3 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -108,6 +108,11 @@ static void panel_bridge_detach(struct drm_bridge *bridge)
 		drm_connector_cleanup(connector);
 }
 
+static void panel_bridge_destroy(struct drm_bridge *bridge)
+{
+	kfree(drm_bridge_to_panel_bridge(bridge));
+}
+
 static void panel_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 				struct drm_bridge_state *old_bridge_state)
 {
@@ -210,6 +215,7 @@ static void panel_bridge_debugfs_init(struct drm_bridge *bridge,
 static const struct drm_bridge_funcs panel_bridge_bridge_funcs = {
 	.attach = panel_bridge_attach,
 	.detach = panel_bridge_detach,
+	.destroy = panel_bridge_destroy,
 	.atomic_pre_enable = panel_bridge_atomic_pre_enable,
 	.atomic_enable = panel_bridge_atomic_enable,
 	.atomic_disable = panel_bridge_atomic_disable,
@@ -286,19 +292,22 @@ struct drm_bridge *drm_panel_bridge_add_typed(struct drm_panel *panel,
 					      u32 connector_type)
 {
 	struct panel_bridge *panel_bridge;
+	int err;
 
 	if (!panel)
 		return ERR_PTR(-EINVAL);
 
-	panel_bridge = devm_kzalloc(panel->dev, sizeof(*panel_bridge),
-				    GFP_KERNEL);
+	panel_bridge = kzalloc(sizeof(*panel_bridge), GFP_KERNEL);
 	if (!panel_bridge)
 		return ERR_PTR(-ENOMEM);
 
+	err = drm_bridge_init(panel->dev, &panel_bridge->bridge, &panel_bridge_bridge_funcs);
+	if (err)
+		return ERR_PTR(err);
+
 	panel_bridge->connector_type = connector_type;
 	panel_bridge->panel = panel;
 
-	panel_bridge->bridge.funcs = &panel_bridge_bridge_funcs;
 	panel_bridge->bridge.of_node = panel->dev->of_node;
 	panel_bridge->bridge.ops = DRM_BRIDGE_OP_MODES;
 	panel_bridge->bridge.type = connector_type;
@@ -317,18 +326,13 @@ EXPORT_SYMBOL(drm_panel_bridge_add_typed);
  */
 void drm_panel_bridge_remove(struct drm_bridge *bridge)
 {
-	struct panel_bridge *panel_bridge;
-
 	if (!bridge)
 		return;
 
 	if (bridge->funcs != &panel_bridge_bridge_funcs)
 		return;
 
-	panel_bridge = drm_bridge_to_panel_bridge(bridge);
-
 	drm_bridge_remove(bridge);
-	devm_kfree(panel_bridge->panel->dev, bridge);
 }
 EXPORT_SYMBOL(drm_panel_bridge_remove);
 

-- 
2.34.1

[PATCH v5 08/10] drm/bridge: samsung-dsim: use supporting variable for out_bridge

[Luca Ceresoli]

Instead of using dsi->out_bridge during the bridge search process, use a
temporary variable and assign dsi->out_bridge only on successful
completion.

The main goal is to be able to drm_bridge_get() the out_bridge before
setting it in dsi->out_bridge, which is done in a later commit. Setting
dsi->out_bridge as in current code would leave a use-after-free window in
case the bridge is deallocated by some other thread between
'dsi->out_bridge = devm_drm_panel_bridge_add()' and drm_bridge_get().

This change additionally avoids leaving an ERR_PTR value in dsi->out_bridge
on failure. This is not necessarily a problem but it is not clean.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

This patch was added in v5.
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index f8b4fb8357659018ec0db65374ee5d05330639ae..c4d1563fd32019efde523dfc0863be044c05a826 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1705,6 +1705,7 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
 	struct device *dev = dsi->dev;
 	struct device_node *np = dev->of_node;
 	struct device_node *remote;
+	struct drm_bridge *out_bridge;
 	struct drm_panel *panel;
 	int ret;
 
@@ -1740,21 +1741,23 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
 
 	panel = of_drm_find_panel(remote);
 	if (!IS_ERR(panel)) {
-		dsi->out_bridge = devm_drm_panel_bridge_add(dev, panel);
+		out_bridge = devm_drm_panel_bridge_add(dev, panel);
 	} else {
-		dsi->out_bridge = of_drm_find_bridge(remote);
-		if (!dsi->out_bridge)
-			dsi->out_bridge = ERR_PTR(-EINVAL);
+		out_bridge = of_drm_find_bridge(remote);
+		if (!out_bridge)
+			out_bridge = ERR_PTR(-EINVAL);
 	}
 
 	of_node_put(remote);
 
-	if (IS_ERR(dsi->out_bridge)) {
-		ret = PTR_ERR(dsi->out_bridge);
+	if (IS_ERR(out_bridge)) {
+		ret = PTR_ERR(out_bridge);
 		DRM_DEV_ERROR(dev, "failed to find the bridge: %d\n", ret);
 		return ret;
 	}
 
+	dsi->out_bridge = out_bridge;
+
 	DRM_DEV_INFO(dev, "Attached %s device (lanes:%d bpp:%d mode-flags:0x%lx)\n",
 		     device->name, device->lanes,
 		     mipi_dsi_pixel_format_to_bpp(device->format),

-- 
2.34.1

[PATCH v5 09/10] drm/bridge: samsung-dsim: refcount the out_bridge

[Luca Ceresoli]

Refcount the out_bridge to avoid a use-after-free in case it is
hot-unplugged.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

This patch was added in v5.
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index c4d1563fd32019efde523dfc0863be044c05a826..4d32c453265931b5aecdc125623368fecacf4be3 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1756,6 +1756,7 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
 		return ret;
 	}
 
+	drm_bridge_get(out_bridge);
 	dsi->out_bridge = out_bridge;
 
 	DRM_DEV_INFO(dev, "Attached %s device (lanes:%d bpp:%d mode-flags:0x%lx)\n",
@@ -1774,13 +1775,13 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
 	if (!(device->mode_flags & MIPI_DSI_MODE_VIDEO)) {
 		ret = samsung_dsim_register_te_irq(dsi, &device->dev);
 		if (ret)
-			return ret;
+			goto err_put_bridge;
 	}
 
 	if (pdata->host_ops && pdata->host_ops->attach) {
 		ret = pdata->host_ops->attach(dsi, device);
 		if (ret)
-			return ret;
+			goto err_put_bridge;
 	}
 
 	dsi->lanes = device->lanes;
@@ -1788,6 +1789,10 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
 	dsi->mode_flags = device->mode_flags;
 
 	return 0;
+
+err_put_bridge:
+	drm_bridge_put_and_clear(dsi->out_bridge);
+	return ret;
 }
 
 static void samsung_dsim_unregister_te_irq(struct samsung_dsim *dsi)
@@ -1804,7 +1809,7 @@ static int samsung_dsim_host_detach(struct mipi_dsi_host *host,
 	struct samsung_dsim *dsi = host_to_dsi(host);
 	const struct samsung_dsim_plat_data *pdata = dsi->plat_data;
 
-	dsi->out_bridge = NULL;
+	drm_bridge_put_and_clear(dsi->out_bridge);
 
 	if (pdata->host_ops && pdata->host_ops->detach)
 		pdata->host_ops->detach(dsi, device);

-- 
2.34.1

[PATCH v5 10/10] drm/bridge: hotplug-bridge: add driver to support hot-pluggable DSI bridges

[Luca Ceresoli]

This driver implements the point of a DRM pipeline where a connector allows
removal of all the following bridges up to the panel.

The DRM subsystem currently allows hotplug of the monitor but not preceding
components. However there are embedded devices where the "tail" of the DRM
pipeline, including one or more bridges, can be physically removed:

 .------------------------.
 |   DISPLAY CONTROLLER   |
 | .---------.   .------. |
 | | ENCODER |<--| CRTC | |
 | '---------'   '------' |
 '------|-----------------'
        |
        |               HOTPLUG
        V              CONNECTOR
   .---------.        .--.    .-.        .---------.         .-------.
   | 0 to N  |        | _|   _| |        | 1 to N  |         |       |
   | BRIDGES |--DSI-->||_   |_  |--DSI-->| BRIDGES |--LVDS-->| PANEL |
   |         |        |  |    | |        |         |         |       |
   '---------'        '--'    '-'        '---------'         '-------'

 [--- fixed components --]  [----------- removable add-on -----------]

This driver supports such a device, where the final segment of a MIPI DSI
bus, including one or more bridges, can be physically disconnected and
reconnected at runtime, possibly with a different model.

The add-on supported by this driver has a MIPI DSI bus traversing the
hotplug connector and a DSI to LVDS bridge and an LVDS panel on the add-on.
Hovever this driver is designed to be as far as possible generic and
extendable to other busses that have no native hotplug and model ID
discovery.

This driver does not itself add and remove the bridges or panel on the
add-on: this needs to be done by other means, e.g. device tree overlay
runtime insertion and removal. The hotplug-bridge gets notified by the DRM
bridge core after a removable bridge gets added or before it is removed.

The hotplug-bridge role is to implement the "hot-pluggable connector" in
the bridge chain. In this position, what the hotplug-bridge should ideally
do is:

 * communicate with the previous component (bridge or encoder) so that it
   believes it always has a connected bridge following it and the DRM card
   is always present
 * be notified of the addition and removal of the following bridge and
   attach/detach to/from it
 * communicate with the following bridge so that it will attach and detach
   using the normal procedure (as if the entire pipeline were being created
   or destroyed, not only the tail)
 * instantiate two DRM connectors (similarly to what the DisplayPort MST
   code does):
   - a DSI connector representing the video lines of the hotplug connector;
     the status is always "disconnected" (no panel is ever attached
     directly to it)
   - an LSVD connector representing the classic connection to the panel;
     this gets added/removed whenever the add-on gets
     connected/disconnected; the status is always "connected" as the panel
     is always connected to the preceding bridge

However some aspects make it a bit more complex than that. Most notably:

 * the next bridge can be probed and removed at any moment and all probing
   sequences need to be handled
 * the DSI host/device registration process, which adds to the DRM bridge
   attach process, makes the initial card registration tricky
 * the need to register and deregister the following bridges at runtime
   without tearing down the whole DRM card prevents using some of the
   functions that are normally recommended
 * the automatic mechanism to call the appropriate .get_modes operation
   (typically provided by the panel bridge) cannot work as the panel can
   disappear and reappear as a different model, so an ad-hoc lookup is
   needed

The code handling these and other tricky aspects is accurately documented
by comments in the code.

The userspace representation when the add-on is not connected is:

  # modetest -c  | grep -i '^[a-z0-9]'
  Connectors:
  id    encoder status          name        size (mm)     modes   encoders
  38    0       disconnected    DSI-1       0x0           0       37

And when it is connected a new connector appears:

  # modetest -c  | grep -i '^[a-z0-9]'
  Connectors:
  id    encoder status          name        size (mm)     modes   encoders
  38    0       disconnected    DSI-1       0x0           0       37
  39    0       connected       LVDS-1      344x194       1       37

Co-developed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

---

Changed in v5:
 - use drm_bridge dynamic lifetime management
 - refcount bridge_modes and next_bridge via drm_bridge_get()/drm_bridge_put()
 - fix dynconn removal by using drm_connector_put() and making the .destroy
   callback work
 - add hotplug_bridge_grab() in hotplug_bridge_attach() to handle the case
   where the encoder driver is probed last
 - migrate platform driver from .remove_new to .remove

Changed in v4:
 - convert from generic notifiers to the new bridge_event_notify bridge
   func
 - improved and updated commit message, adding 'modetest' output
   with/without add-on
 - select DRM_DISPLAY_HELPER and DRM_BRIDGE_CONNECTOR, required after
   commit 9da7ec9b19d8 ("drm/bridge-connector: move to DRM_DISPLAY_HELPER
   module")

Changed in v3:
 - dynamically add/remove the LVDS connector on hot(un)plug
 - take the firmware node normally via dev->of_node instead of using
   device_set_node(); this makes code more self-contained and generic
 - minor rewordings and cleanups

Changed in v2:
 - change to be a platform device instantiated from the connector driver
   instead of a self-standing OF driver
 - add missing error handling for devm_drm_bridge_add()
 - various cleanups and style improvements
 - fix typo in comment
---
 MAINTAINERS                             |   5 +
 drivers/gpu/drm/bridge/Kconfig          |  17 +
 drivers/gpu/drm/bridge/Makefile         |   1 +
 drivers/gpu/drm/bridge/hotplug-bridge.c | 695 ++++++++++++++++++++++++++++++++
 4 files changed, 718 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 1675e44799e55d72711a6251f692a4a14bc3a84a..ab255f5696880258deee55d99ff0a7cde85efb7c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7182,6 +7182,11 @@ T:	git https://gitlab.freedesktop.org/drm/misc/kernel.git
 F:	Documentation/devicetree/bindings/display/panel/himax,hx8394.yaml
 F:	drivers/gpu/drm/panel/panel-himax-hx8394.c
 
+DRM DRIVER FOR HOTPLUG VIDEO CONNECTOR BRIDGE
+M:	Luca Ceresoli <luca.ceresoli@bootlin.com>
+S:	Maintained
+F:	drivers/gpu/drm/bridge/hotplug-bridge.c
+
 DRM DRIVER FOR HX8357D PANELS
 S:	Orphan
 T:	git https://gitlab.freedesktop.org/drm/misc/kernel.git
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 6b4664d91faa80f096ac6a0548ed342e802ae68b..f01971638d6818e33b32217922e165a8c18d51ee 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -90,6 +90,23 @@ config DRM_FSL_LDB
 	help
 	  Support for i.MX8MP DPI-to-LVDS on-SoC encoder.
 
+config DRM_HOTPLUG_BRIDGE
+	tristate "Hotplug DRM bridge support"
+	depends on OF
+	select DRM_PANEL_BRIDGE
+	select DRM_MIPI_DSI
+	select DRM_KMS_HELPER
+	select DRM_DISPLAY_HELPER
+	select DRM_BRIDGE_CONNECTOR
+	help
+	  Driver for a DRM bridge representing a physical connector that
+	  splits a DRM pipeline into a fixed part and a physically
+	  removable part. The fixed part includes up to the encoder and
+	  zero or more bridges. The removable part includes any following
+	  bridges up to the connector and panel and can be physically
+	  removed and connected at runtime, possibly with different
+	  components.
+
 config DRM_ITE_IT6263
 	tristate "ITE IT6263 LVDS/HDMI bridge"
 	depends on OF
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 97304b429a530c108dcbff906965cda091b0a7a2..2f6ae1a97d15045316ee191c04dbc65650162bab 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_DRM_CHRONTEL_CH7033) += chrontel-ch7033.o
 obj-$(CONFIG_DRM_CROS_EC_ANX7688) += cros-ec-anx7688.o
 obj-$(CONFIG_DRM_DISPLAY_CONNECTOR) += display-connector.o
 obj-$(CONFIG_DRM_FSL_LDB) += fsl-ldb.o
+obj-$(CONFIG_DRM_HOTPLUG_BRIDGE) += hotplug-bridge.o
 obj-$(CONFIG_DRM_ITE_IT6263) += ite-it6263.o
 obj-$(CONFIG_DRM_ITE_IT6505) += ite-it6505.o
 obj-$(CONFIG_DRM_LONTIUM_LT8912B) += lontium-lt8912b.o
diff --git a/drivers/gpu/drm/bridge/hotplug-bridge.c b/drivers/gpu/drm/bridge/hotplug-bridge.c
new file mode 100644
index 0000000000000000000000000000000000000000..f717206287fc598cf7a3c5ac5bf9e1be4c8540d9
--- /dev/null
+++ b/drivers/gpu/drm/bridge/hotplug-bridge.c
@@ -0,0 +1,695 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * A DRM bridge representing the split point between a fixed part of the
+ * DRM pipeline and a physically removable part. The fixed part includes up
+ * to the encoder and zero or more bridges. Insertion and removal of the
+ * "downstream" components happens via device driver probe/removal.
+ *
+ * Copyright (C) 2024, GE HealthCare
+ *
+ * Authors:
+ * Luca Ceresoli <luca.ceresoli@bootlin.com>
+ * Paul Kocialkowski <paul.kocialkowski@bootlin.com>
+ */
+
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_graph.h>
+#include <linux/platform_device.h>
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_bridge.h>
+#include <drm/drm_bridge_connector.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_of.h>
+#include <drm/drm_probe_helper.h>
+
+/*
+ * Internal hotplug-bridge data.
+ *
+ * We have two 'struct drm_connector' here:
+ * - fixconn represents the DSI video lines on the hotplug connector where
+ *   the removable part attaches. It is thus always instantiated.
+ * - dynconn represents the LVDS video lines where the panel is attached.
+ *   It is part of the removable part of the video pipeline and as such is
+ *   added and removed dynamically based on when the downstream devices
+ *   appear and disappear.
+ */
+struct hotplug_bridge {
+	struct device *dev;
+
+	/* Local bridge */
+	struct drm_bridge bridge;
+
+	/* Always-present connector (where the removal part will connect to) */
+	struct drm_connector *fixconn;
+
+	/* Downstream bridge (next in the chain) */
+	struct drm_bridge *next_bridge;
+	/* Protect next_bridge */
+	struct mutex next_bridge_mutex;
+
+	/* Pointer to the last bridge exposing OP_MODES */
+	struct drm_bridge *bridge_modes;
+
+	/* The "tail" connector that gets added/removed at runtime */
+	struct drm_connector dynconn;
+
+	/* Local DSI host, for the downstream DSI device to attach to */
+	struct mipi_dsi_host dsi_host;
+	/* Local DSI device, attached to the upstream DSI host */
+	struct mipi_dsi_device *dsi_dev;
+	/* Upstream DSI host (the actual DSI controller) */
+	struct mipi_dsi_host *prev_dsi_host;
+
+	struct work_struct hpd_work;
+};
+
+static struct hotplug_bridge *hotplug_bridge_from_drm_bridge(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct hotplug_bridge, bridge);
+}
+
+/* --------------------------------------------------------------------------
+ * dynconn implementation
+ */
+static struct hotplug_bridge *hotplug_bridge_from_dynconn(struct drm_connector *conn)
+{
+	return container_of(conn, struct hotplug_bridge, dynconn);
+}
+
+static int hotplug_bridge_dynconn_get_modes(struct drm_connector *connector)
+{
+	struct hotplug_bridge *hpb = hotplug_bridge_from_dynconn(connector);
+
+	if (hpb->bridge_modes)
+		return hpb->bridge_modes->funcs->get_modes(hpb->bridge_modes, connector);
+
+	return 0;
+}
+
+static const struct drm_connector_helper_funcs hotplug_bridge_dynconn_connector_helper_funcs = {
+	.get_modes = hotplug_bridge_dynconn_get_modes,
+};
+
+static void hotplug_bridge_dynconn_destroy(struct drm_connector *connector)
+{
+	drm_connector_unregister(connector);
+	drm_connector_cleanup(connector);
+}
+
+static const struct drm_connector_funcs dynconn_funcs = {
+	.destroy                    = hotplug_bridge_dynconn_destroy,
+	.atomic_duplicate_state     = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state       = drm_atomic_helper_connector_destroy_state,
+	.fill_modes                 = drm_helper_probe_single_connector_modes,
+};
+
+/*
+ * In non-removable pipelines using a "bridge connector",
+ * drm_bridge_connector_init() stores in the bridge_connector a pointer to
+ * the last bridge having OP_MODES (typically the panel bridge), so the
+ * .get_modes op will automatically be called on that bridge (it also takes
+ * pointers to other bridges which we don't care about). The "bridge
+ * connector" is too restrictive for our use case so we cannot use it. But
+ * we need a pointer to the modes-providing bridge, so we need to replicate
+ * that bit of its logic.
+ *
+ * If no modes bridge is found, nothing is done. This is allowed.
+ *
+ * Also get the modes bridge to tie its lifetime to ours.
+ */
+static void hotplug_bridge_dynconn_bridge_modes_get(struct hotplug_bridge *hpb)
+{
+	struct drm_bridge *bridge;
+
+	if (WARN_ON(!hpb->next_bridge || !hpb->bridge.encoder))
+		return;
+
+	drm_for_each_bridge_in_chain(hpb->bridge.encoder, bridge)
+		if (bridge->ops & DRM_BRIDGE_OP_MODES) {
+			drm_bridge_get(bridge);
+			hpb->bridge_modes = bridge;
+		}
+}
+
+static void hotplug_bridge_dynconn_bridge_modes_put(struct hotplug_bridge *hpb)
+{
+	if (hpb->bridge_modes)
+		drm_bridge_put_and_clear(hpb->bridge_modes);
+}
+
+static int hotplug_bridge_dynconn_add(struct hotplug_bridge *hpb)
+{
+	int err;
+
+	err = drm_connector_init(hpb->bridge.dev, &hpb->dynconn, &dynconn_funcs,
+				 DRM_MODE_CONNECTOR_LVDS);
+	if (err)
+		return err;
+
+	drm_atomic_helper_connector_reset(&hpb->dynconn);
+
+	drm_connector_helper_add(&hpb->dynconn,
+				 &hotplug_bridge_dynconn_connector_helper_funcs);
+
+	drm_connector_attach_encoder(&hpb->dynconn, hpb->bridge.encoder);
+	if (err)
+		goto err_cleanup;
+
+	hotplug_bridge_dynconn_bridge_modes_get(hpb);
+
+	err = drm_connector_register(&hpb->dynconn);
+	if (err)
+		goto err_cleanup;
+
+	return 0;
+
+err_cleanup:
+	drm_connector_cleanup(&hpb->dynconn);
+	hotplug_bridge_dynconn_bridge_modes_put(hpb);
+	return err;
+}
+
+/* ----------------------------------------------------------------------- */
+
+/*
+ * Attach the remote bridge to the encoder and to the next bridge in the
+ * chain, if possible. For this to succeed, we need to know:
+ *
+ * - the encoder, which is set at the first drm_bridge_attach() time
+ * - the next bridge, which is obtained via a notifier whenever the next
+ *   bridge is (re)probed, or at probe time in case it was probed before us
+ *
+ * In order to handle different execution sequences, this function can be
+ * called from multiple places and needs to check all the prerequisites
+ * every time, and it will act only if both are met.
+ *
+ * Must be called with hpb->next_bridge_mutex held.
+ *
+ * Returns 0 if the encoder was attached successfully, -ENODEV if any of
+ * the two prerequisites above is not met (no encoder or no next bridge),
+ * the error returned by drm_bridge_attach() otherwise.
+ */
+static int hotplug_bridge_attach_to_encoder_chain(struct hotplug_bridge *hpb)
+{
+	int ret;
+
+	if (!hpb->next_bridge || !hpb->bridge.encoder)
+		return -ENODEV;
+
+	ret = drm_bridge_attach(hpb->bridge.encoder, hpb->next_bridge, &hpb->bridge,
+				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+	if (ret)
+		return dev_err_probe(hpb->dev, ret, "drm_bridge_attach failed\n");
+
+	dev_dbg(hpb->dev, "attached to encoder chain\n");
+
+	return 0;
+}
+
+/*
+ * Stop the video pipeline and detach next_bridge.
+ *
+ * Must be called with hpb->next_bridge_mutex held.
+ */
+static void hotplug_bridge_detach_from_encoder_chain(struct hotplug_bridge *hpb)
+{
+	WARN_ON_ONCE(!hpb->next_bridge);
+
+	dev_dbg(hpb->dev, "detaching from encoder chain\n");
+
+	drm_atomic_helper_shutdown(hpb->bridge.dev);
+
+	drm_encoder_cleanup_from(hpb->bridge.encoder, hpb->next_bridge);
+}
+
+static void hotplug_bridge_grab(struct hotplug_bridge *hpb)
+{
+	struct device *dev = hpb->dev;
+	struct drm_bridge *bridge;
+	struct drm_panel *panel;
+	int err;
+
+	mutex_lock(&hpb->next_bridge_mutex);
+
+	if (hpb->next_bridge)
+		goto out_unlock;
+
+	/*
+	 * This is supposed to be replaced by devm_drm_of_get_bridge(), but
+	 * that is a devm_, and we need to remove the panel bridge also on
+	 * next_bridge disconnect.
+	 */
+	err = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, &bridge);
+	if (err)
+		goto out_unlock;
+
+	/* Convert the remote panel to a bridge */
+	if (panel)
+		bridge = drm_panel_bridge_add(panel);
+	if (IS_ERR(bridge))
+		goto out_unlock;
+
+	drm_bridge_get(bridge);
+	hpb->next_bridge = bridge;
+
+	dev_dbg(dev, "grabbed next bridge (%pOFn)\n", hpb->next_bridge->of_node);
+
+	hpb->bridge.pre_enable_prev_first = hpb->next_bridge->pre_enable_prev_first;
+
+	err = hotplug_bridge_attach_to_encoder_chain(hpb);
+	if (err)
+		goto err_panel_bridge_remove;
+
+	err = hotplug_bridge_dynconn_add(hpb);
+	if (err)
+		goto err_detach_from_encoder_chain;
+
+	queue_work(system_wq, &hpb->hpd_work);
+	goto out_unlock;
+
+err_detach_from_encoder_chain:
+	hotplug_bridge_detach_from_encoder_chain(hpb);
+err_panel_bridge_remove:
+	drm_panel_bridge_remove(hpb->next_bridge);
+	drm_bridge_put_and_clear(hpb->next_bridge);
+out_unlock:
+	mutex_unlock(&hpb->next_bridge_mutex);
+}
+
+/*
+ * Detach from the next bridge and remove the panel bridge, either on
+ * release or when the downstream bridge is being removed.
+ *
+ * Can be called in these ways:
+ *
+ * - bridge_being_removed is NULL: detach unconditionally
+ *   (this is useful on .remove() to teardown everything)
+ * - bridge_being_removed == hpb->next_bridge: detach
+ *   (the downstream bridge is being removed)
+ * - bridge_being_removed != hpb->next_bridge: do nothing
+ *   (the bridge being removed is not the downstream bridge)
+ *
+ * In all cases, does nothing when there is no downstream bridge.
+ */
+static void hotplug_bridge_release(struct hotplug_bridge *hpb,
+				   struct drm_bridge *bridge_being_removed)
+{
+	mutex_lock(&hpb->next_bridge_mutex);
+
+	if (!hpb->next_bridge)
+		goto out;
+
+	if (bridge_being_removed && bridge_being_removed != hpb->next_bridge)
+		goto out;
+
+	if (hpb->bridge_modes)
+		hotplug_bridge_dynconn_bridge_modes_put(hpb);
+
+	dev_dbg(hpb->dev, "releasing next bridge (%pOFn)\n", hpb->next_bridge->of_node);
+	hotplug_bridge_detach_from_encoder_chain(hpb);
+
+	dev_dbg(hpb->dev, "removing %s connector\n", hpb->dynconn.name);
+	drm_connector_put(&hpb->dynconn);
+
+	/*
+	 * This will check that the bridge actually belongs to panel-bridge
+	 * before doing anything with it, so we can safely always call it.
+	 */
+	drm_panel_bridge_remove(hpb->next_bridge);
+	drm_bridge_put_and_clear(hpb->next_bridge);
+
+	queue_work(system_wq, &hpb->hpd_work);
+
+out:
+	mutex_unlock(&hpb->next_bridge_mutex);
+}
+
+static void hotplug_bridge_bridge_event_notify(struct drm_bridge *bridge,
+					       enum drm_bridge_event_type event,
+					       struct drm_bridge *event_bridge)
+{
+	struct hotplug_bridge *hpb = container_of(bridge, struct hotplug_bridge, bridge);
+
+	switch (event) {
+	case DRM_EVENT_BRIDGE_ADD:
+		hotplug_bridge_grab(hpb);
+		break;
+	case DRM_EVENT_BRIDGE_REMOVE:
+		hotplug_bridge_release(hpb, event_bridge);
+		break;
+	}
+}
+
+static int hotplug_bridge_attach(struct drm_bridge *bridge,
+				 enum drm_bridge_attach_flags flags)
+{
+	struct hotplug_bridge *hpb = hotplug_bridge_from_drm_bridge(bridge);
+	struct device *dev = hpb->dev;
+	struct drm_connector *connector;
+	struct drm_encoder *encoder = hpb->bridge.encoder;
+	int err;
+
+	/* Encoder was not yet provided to our bridge */
+	if (!encoder)
+		return -ENODEV;
+
+	/* Connector was already created */
+	if (hpb->fixconn)
+		return dev_err_probe(dev, -EBUSY, "connector already created\n");
+
+	connector = drm_bridge_connector_init(bridge->dev, encoder);
+	if (IS_ERR(connector))
+		return dev_err_probe(dev, PTR_ERR(connector), "failed to initialize connector\n");
+
+	drm_connector_attach_encoder(connector, encoder);
+
+	hpb->fixconn = connector;
+
+	drm_connector_register(connector);
+
+	mutex_lock(&hpb->next_bridge_mutex);
+	err = hotplug_bridge_attach_to_encoder_chain(hpb);
+	mutex_unlock(&hpb->next_bridge_mutex);
+
+	/* -ENODEV is acceptable, in case next_bridge is not yet known */
+	if (err == -ENODEV)
+		err = 0;
+
+	/*
+	 * If the encoder driver is probed last, the
+	 * hotplug_bridge_attach_to_encoder_chain() call in
+	 * hotplug_bridge_grab() fails because hpb->bridge.encoder is still
+	 * NULL, and hotplug_bridge_grab() will not have another chance to
+	 * execute. So call it now, at the end of the encoder attach
+	 * process.
+	 */
+	hotplug_bridge_grab(hpb);
+
+	return err;
+}
+
+static void hotplug_bridge_detach(struct drm_bridge *bridge)
+{
+	struct hotplug_bridge *hpb = hotplug_bridge_from_drm_bridge(bridge);
+
+	mutex_lock(&hpb->next_bridge_mutex);
+	hotplug_bridge_detach_from_encoder_chain(hpb);
+	mutex_unlock(&hpb->next_bridge_mutex);
+
+	if (hpb->fixconn) {
+		drm_connector_unregister(hpb->fixconn);
+		drm_connector_cleanup(hpb->fixconn);
+		hpb->fixconn = NULL;
+	}
+}
+
+static void hotplug_bridge_destroy(struct drm_bridge *bridge)
+{
+	struct hotplug_bridge *hpb = hotplug_bridge_from_drm_bridge(bridge);
+
+	kfree(hpb);
+}
+
+/*
+ * The fixed connector is never attached to a panel, so it should always be
+ * reported as disconnected.
+ */
+static enum drm_connector_status hotplug_bridge_detect(struct drm_bridge *bridge)
+{
+	return connector_status_disconnected;
+}
+
+static void hotplug_bridge_hpd_work_func(struct work_struct *work)
+{
+	struct hotplug_bridge *hpb = container_of(work, struct hotplug_bridge, hpd_work);
+
+	if (hpb->bridge.dev)
+		drm_helper_hpd_irq_event(hpb->bridge.dev);
+}
+
+static const struct drm_bridge_funcs hotplug_bridge_funcs = {
+	.attach			= hotplug_bridge_attach,
+	.detach			= hotplug_bridge_detach,
+	.destroy                = hotplug_bridge_destroy,
+	.detect			= hotplug_bridge_detect,
+	.bridge_event_notify	= hotplug_bridge_bridge_event_notify,
+};
+
+static int hotplug_bridge_dsi_detach(struct mipi_dsi_host *host,
+				     struct mipi_dsi_device *device_remote)
+{
+	struct hotplug_bridge *hpb = dev_get_drvdata(host->dev);
+
+	if (!hpb->dsi_dev)
+		return -ENODEV;
+
+	mipi_dsi_detach(hpb->dsi_dev);
+	mipi_dsi_device_unregister(hpb->dsi_dev);
+	hpb->dsi_dev = NULL;
+
+	return 0;
+}
+
+/*
+ * Attach the local DSI device to the upstream DSI host, possibly with a
+ * "null" format.
+ *
+ * In "normal" bridges this function should be _only_ used as the .attach
+ * callback of hotplug_bridge_dsi_ops. But "normal" bridges have their
+ * downstream DSI device always connected, which we don't. When booting
+ * without anything connected downstream, our upstream bridge could be not
+ * even calling drm_bridge_add() until we do attach ourselves as a DSI
+ * device, preventing the whole DRM card from being instantiated.
+ *
+ * In order to always have a DRM card after boot, we do call this same
+ * function while probing in order to attach as a DSI device to the DSI
+ * master. However during probe we don't know the bus format yet. It would
+ * be nice to be able to update the format afterwards when a downstream DSI
+ * device is attaching to our local host, but there is no callback for
+ * that. To overcome this limitation, this function can be called in two
+ * ways:
+ *
+ * - during probe, to make the upstream bridge happy, when there is no
+ *   next_dsi_dev yet and thus the lanes/format/etc are unknown
+ * - as the mipi_dsi_host_ops.attach callback proper, as soon as the
+ *   next_dsi_dev is known
+ *
+ * The resulting call sequence is:
+ *
+ * 1. hotplug_bridge_dsi_attach() called by hotplug_bridge_probe() with
+ *    next_dsi_dev == NULL: we attach to the host but with a fake format
+ *    so the DRM card can be populated. hpb->dsi_dev becomes non-NULL.
+ * 2. hotplug_bridge_dsi_attach() called as .attach callback from a
+ *    downstream device when it becomes available: we need to detach in
+ *    order to re-attach with the format of the device. hpb->dsi_dev
+ *    is found non-NULL, then reused so it will be non-NULL again.
+ * 3. hotplug_bridge_dsi_detach() called as the .detach callback by a
+ *    downstream device: cleans up everything normally. hpb->dsi_dev goes
+ *    from non-NULL to NULL.
+ * 4. hotplug_bridge_dsi_attach() called by a downstream device: attaches
+ *    normally to the upstream DSI host. hpb->dsi_dev goes from NULL to
+ *    non-NULL.
+ *
+ * Steps 3 and 4 are the "normal" attach/detach steps as on "normal"
+ * bridges.
+ *
+ * Steps 1 and 2 happen only the first time, steps 3 and 4 will happen
+ * every time the downstream bridge disconnects and reconnects.
+ */
+static int hotplug_bridge_dsi_attach(struct mipi_dsi_host *host,
+				     struct mipi_dsi_device *next_dsi_dev)
+{
+	struct device *dev = host->dev;
+	struct hotplug_bridge *hpb = dev_get_drvdata(dev);
+	struct mipi_dsi_device *dsi_dev;
+	const struct mipi_dsi_device_info dsi_info = {
+		.type = "hotplug-bridge",
+		.channel = 0,
+		.node = NULL,
+	};
+	int err;
+
+	/*
+	 * Step 2 only (first time we are called for an actual device
+	 * attaching): clean up the fake attach done at step 1
+	 */
+	if (hpb->dsi_dev)
+		hotplug_bridge_dsi_detach(&hpb->dsi_host, NULL);
+
+	/* Register a local DSI device with the remote DSI host */
+	dsi_dev = mipi_dsi_device_register_full(hpb->prev_dsi_host,
+						&dsi_info);
+	if (IS_ERR(dsi_dev))
+		return PTR_ERR(dsi_dev);
+
+	/* At step 1 we have no downstream device to get the format from */
+	if (next_dsi_dev) {
+		dsi_dev->channel    = next_dsi_dev->channel;
+		dsi_dev->lanes      = next_dsi_dev->lanes;
+		dsi_dev->format     = next_dsi_dev->format;
+		dsi_dev->mode_flags = next_dsi_dev->mode_flags;
+	}
+
+	/* Attach our local DSI device to the remote DSI host */
+	err = mipi_dsi_attach(dsi_dev);
+	if (err) {
+		mipi_dsi_device_unregister(dsi_dev);
+		return dev_err_probe(dev, err, "failed to attach hotplug dsi device to host\n");
+	}
+
+	hpb->dsi_dev = dsi_dev;
+
+	return 0;
+}
+
+/*
+ * Propagate mipi_dsi_device_transfer() to the upstream DSI host.
+ *
+ * Reimplements identically the minimal needed part of
+ * mipi_dsi_device_transfer(), including the -ENOSYS return value.
+ */
+static ssize_t hotplug_bridge_dsi_transfer(struct mipi_dsi_host *host,
+					   const struct mipi_dsi_msg *msg)
+{
+	struct hotplug_bridge *hpb = dev_get_drvdata(host->dev);
+	const struct mipi_dsi_host_ops *ops;
+
+	if (!hpb->dsi_dev)
+		return -ENODEV;
+
+	ops = hpb->dsi_dev->host->ops;
+
+	if (!ops || !ops->transfer)
+		return -ENOSYS;
+
+	return ops->transfer(hpb->dsi_dev->host, msg);
+}
+
+static const struct mipi_dsi_host_ops hotplug_bridge_dsi_ops = {
+	.attach		= hotplug_bridge_dsi_attach,
+	.detach		= hotplug_bridge_dsi_detach,
+	.transfer	= hotplug_bridge_dsi_transfer,
+};
+
+/*
+ * Find the upstream DSI host and register our downstream-facing DSI host.
+ */
+static int hotplug_bridge_dsi_setup(struct hotplug_bridge *hpb)
+{
+	struct device *dev = hpb->dev;
+	struct device_node *endpoint;
+	struct device_node *node;
+
+	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
+	node = of_graph_get_remote_port_parent(endpoint);
+
+	hpb->prev_dsi_host = of_find_mipi_dsi_host_by_node(node);
+
+	of_node_put(node);
+	of_node_put(endpoint);
+
+	if (!hpb->prev_dsi_host)
+		return -EPROBE_DEFER;
+
+	hpb->dsi_host.dev = dev;
+	hpb->dsi_host.ops = &hotplug_bridge_dsi_ops;
+
+	return mipi_dsi_host_register(&hpb->dsi_host);
+}
+
+static void hotplug_bridge_dsi_cleanup(struct hotplug_bridge *hpb)
+{
+	mipi_dsi_host_unregister(&hpb->dsi_host);
+}
+
+static int hotplug_bridge_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct hotplug_bridge *hpb;
+	struct drm_bridge *bridge;
+	int err;
+
+	hpb = kzalloc(sizeof(*hpb), GFP_KERNEL);
+	if (!hpb)
+		return -ENOMEM;
+
+	err = drm_bridge_init(dev, &hpb->bridge, &hotplug_bridge_funcs);
+	if (err)
+		return err;
+
+	hpb->dev = dev;
+
+	mutex_init(&hpb->next_bridge_mutex);
+	INIT_WORK(&hpb->hpd_work, hotplug_bridge_hpd_work_func);
+
+	err = hotplug_bridge_dsi_setup(hpb);
+	if (err)
+		return dev_err_probe(dev, err, "failed to setup DSI\n");
+
+	bridge = &hpb->bridge;
+	bridge->of_node = dev->of_node;
+	bridge->type = DRM_MODE_CONNECTOR_DSI;
+	bridge->ops |= DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_HPD;
+
+	platform_set_drvdata(pdev, hpb);
+
+	err = devm_drm_bridge_add(dev, bridge);
+	if (err) {
+		dev_err_probe(dev, err, "failed adding bridge\n");
+		goto err_dsi_cleanup;
+	}
+
+	err = hotplug_bridge_dsi_attach(&hpb->dsi_host, NULL);
+	if (err) {
+		dev_err_probe(dev, err, "failed first attach to upstream DSI host\n");
+		goto err_dsi_cleanup;
+	}
+
+	/*
+	 * Since devm_drm_bridge_add() we can be notified of any bridges
+	 * appearing, but also check now, in case the next bridge was
+	 * probed earlier
+	 */
+	hotplug_bridge_grab(hpb);
+
+	return 0;
+
+err_dsi_cleanup:
+	hotplug_bridge_dsi_cleanup(hpb);
+	return err;
+}
+
+static void hotplug_bridge_remove(struct platform_device *pdev)
+{
+	struct hotplug_bridge *hpb = platform_get_drvdata(pdev);
+
+	cancel_work_sync(&hpb->hpd_work);
+
+	hotplug_bridge_release(hpb, NULL);
+
+	hotplug_bridge_dsi_cleanup(hpb);
+}
+
+static const struct platform_device_id hotplug_bridge_platform_ids[] = {
+	{ .name = "hotplug-dsi-bridge" },
+	{},
+};
+MODULE_DEVICE_TABLE(platform, hotplug_bridge_platform_ids);
+
+static struct platform_driver hotplug_bridge_driver = {
+	.probe		= hotplug_bridge_probe,
+	.remove		= hotplug_bridge_remove,
+	.id_table	= hotplug_bridge_platform_ids,
+	.driver		= {
+		.name		= "hotplug-drm-bridge",
+	},
+};
+
+module_platform_driver(hotplug_bridge_driver);
+
+MODULE_AUTHOR("Luca Ceresoli <luca.ceresoli@bootlin.com>");
+MODULE_AUTHOR("Paul Kocialkowski <paul.kocialkowski@bootlin.com>");
+MODULE_DESCRIPTION("Hotplug DRM Bridge");
+MODULE_LICENSE("GPL");

-- 
2.34.1

Re: [PATCH v5 03/10] drm/bridge: add support for refcounted DRM bridges

[Jani Nikula]

On Tue, 31 Dec 2024, Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> DRM bridges are currently considered as a fixed element of a DRM card, and
> thus their lifetime is assumed to extend for as long as the card
> exists. New use cases, such as hot-pluggable hardware with video bridges,
> require DRM bridges to be added and removed to a DRM card without tearing
> the card down. This is possible for connectors already (used by DP MST), so
> add this possibility to DRM bridges as well.
>
> Implementation is based on drm_connector_init() as far as it makes sense,
> and differs when it doesn't. A difference is that bridges are not exposed
> to userspace,hence struct drm_bridge does not embed a struct
> drm_mode_object which would provide the refcount and the free_cb. So here
> we add to struct drm_bridge just the refcount and free_cb fields (we don't
> need other struct drm_mode_object fields here) and instead of using the
> drm_mode_object_*() functions we reimplement from those functions the few
> lines that drm_bridge needs for refcounting.
>
> The function to enroll a private bridge driver data structure into
> refcounting is based on drm_connector_init() and so called
> drm_bridge_init() for symmetry, even though it does not initialize anything
> except the refcounting and the funcs pointer which is needed to access
> funcs->destroy.
>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
>
> ---
>
> This patch was added in v5.
> ---
>  drivers/gpu/drm/drm_bridge.c |  87 ++++++++++++++++++++++++++++++++++++
>  include/drm/drm_bridge.h     | 102 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 189 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index b1f0d25d55e23000521ac2ac37ee410348978ed4..6255ef59f73d8041a8cb7f2c6e23e5a67d1ae926 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -198,6 +198,85 @@
>  static DEFINE_MUTEX(bridge_lock);
>  static LIST_HEAD(bridge_list);
>  
> +static void drm_bridge_put_void(void *data)
> +{
> +	struct drm_bridge *bridge = (struct drm_bridge *)data;
> +
> +	drm_bridge_put(bridge);
> +}
> +
> +static void drm_bridge_free(struct kref *kref)
> +{
> +	struct drm_bridge *bridge = container_of(kref, struct drm_bridge, refcount);
> +
> +	DRM_DEBUG("bridge=%p\n", bridge);
> +
> +	WARN_ON(!bridge->funcs->destroy);

Please don't add new DRM_DEBUG or WARN_ON where you can use the
drm_dbg_* or drm_WARN_ON variants.

> +
> +	if (bridge->funcs->destroy)
> +		bridge->funcs->destroy(bridge);
> +}
> +
> +/**
> + * drm_bridge_init - Initialize bridge and move private driver data
> + *                   lifetime management to the DRM bridge core
> + *
> + * @dev: struct device of the device whose removal shall trigger deallocation
> + * @bridge: the bridge to initialize
> + * @funcs: funcs structure for @bridge, which must have a valid .destroy func
> + *
> + * Takes over lifetime of a private bridge driver struct which embeds a
> + * struct drm_bridge. To be called by bridge drivers just after having
> + * allocated such a private structure. Initializes refcount to 1 and
> + * installs a callback which will call funcs->destroy when refcount drops
> + * to zero.
> + *
> + * After calling this function a bridge becomes a bridge with dynamic
> + * lifetime (aka a refcounted bridge).
> + *
> + * Drivers calling this function:
> + *  - must not allocate the private structure using devm_*() functions
> + *  - must not deallocate the private structure on device removal
> + *  - must deallocate the private structure in funcs->destroy
> + *
> + * Drivers not calling this function:
> + *  - must take care of freeing their private structure either by allocating
> + *    it using devm_*() functions or free it explicitly on device removal
> + *    using kfree()
> + *  - must set funcs->destroy to NULL
> + *
> + * On failure, calls funcs->destroy, thus the caller does not need to free
> + * the driver private struct in case of error.
> + *
> + * Returns:
> + * Zero on success, error code on failure.
> + */
> +int drm_bridge_init(struct device *dev,
> +		    struct drm_bridge *bridge,
> +		    const struct drm_bridge_funcs *funcs)
> +{
> +	int err;
> +
> +	DRM_DEBUG("bridge=%p, funcs=%ps\n", bridge, funcs);
> +
> +	if (!(funcs && funcs->destroy)) {
> +		dev_warn(dev, "Missing funcs or destroy func pointer\n");
> +		return -EINVAL;
> +	}
> +
> +	bridge->free_cb = drm_bridge_free;
> +	kref_init(&bridge->refcount);
> +
> +	err = devm_add_action_or_reset(dev, drm_bridge_put_void, bridge);
> +	if (err)
> +		return err;
> +
> +	bridge->funcs = funcs;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_bridge_init);
> +
>  /**
>   * drm_bridge_add - add the given bridge to the global bridge list
>   *
> @@ -207,6 +286,10 @@ void drm_bridge_add(struct drm_bridge *bridge)
>  {
>  	struct drm_bridge *br, *tmp;
>  
> +	DRM_DEBUG("bridge=%p\n", bridge);
> +
> +	drm_bridge_get(bridge);
> +
>  	mutex_init(&bridge->hpd_mutex);
>  
>  	list_for_each_entry_safe(br, tmp, &bridge_list, list)
> @@ -251,6 +334,8 @@ void drm_bridge_remove(struct drm_bridge *bridge)
>  {
>  	struct drm_bridge *br, *tmp;
>  
> +	DRM_DEBUG("bridge=%p\n", bridge);
> +
>  	mutex_lock(&bridge_lock);
>  	list_del_init(&bridge->list);
>  	mutex_unlock(&bridge_lock);
> @@ -260,6 +345,8 @@ void drm_bridge_remove(struct drm_bridge *bridge)
>  			br->funcs->bridge_event_notify(br, DRM_EVENT_BRIDGE_REMOVE, bridge);
>  
>  	mutex_destroy(&bridge->hpd_mutex);
> +
> +	drm_bridge_put(bridge);
>  }
>  EXPORT_SYMBOL(drm_bridge_remove);
>  
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 6976bd842cedf9ce06abfb7306e7a3b4915f0378..a548a6acb02e3d70c8e34de965f648320420a7d5 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -31,6 +31,7 @@
>  #include <drm/drm_encoder.h>
>  #include <drm/drm_mode_object.h>
>  #include <drm/drm_modes.h>
> +#include <drm/drm_print.h>
>  
>  struct device_node;
>  
> @@ -89,6 +90,18 @@ struct drm_bridge_funcs {
>  	 */
>  	void (*detach)(struct drm_bridge *bridge);
>  
> +	/**
> +	 * @destroy:
> +	 *
> +	 * Clean up bridge resources for bridges with dynamic
> +	 * lifetime. This is called when the bridge refcount drops to
> +	 * zero. It is never called for bridges without dynamic lifetime.
> +	 *
> +	 * See drm_bridge_init() for info about bridges with dynamic
> +	 * lifetime.
> +	 */
> +	void (*destroy)(struct drm_bridge *bridge);
> +
>  	/**
>  	 * @mode_valid:
>  	 *
> @@ -810,6 +823,18 @@ struct drm_bridge {
>  	const struct drm_bridge_timings *timings;
>  	/** @funcs: control functions */
>  	const struct drm_bridge_funcs *funcs;
> +
> +	/**
> +	 * @refcount: reference count for bridges with dynamic lifetime
> +	 * (see drm_bridge_init)
> +	 */
> +	struct kref refcount;
> +	/**
> +	 * @free_cb: free function callback, only set for bridges with
> +	 * dynamic lifetime
> +	 */
> +	void (*free_cb)(struct kref *kref);
> +
>  	/** @driver_private: pointer to the bridge driver's internal context */
>  	void *driver_private;
>  	/** @ops: bitmask of operations supported by the bridge */
> @@ -890,6 +915,83 @@ drm_priv_to_bridge(struct drm_private_obj *priv)
>  	return container_of(priv, struct drm_bridge, base);
>  }
>  
> +static inline bool drm_bridge_is_refcounted(struct drm_bridge *bridge)
> +{
> +	return bridge->free_cb;
> +}
> +
> +/**
> + * drm_bridge_get - Acquire a bridge reference
> + * @bridge: DRM bridge
> + *
> + * This function increments the bridge's refcount.
> + *
> + * It does nothing on non-refcounted bridges. See drm_bridge_init().
> + */
> +static inline void drm_bridge_get(struct drm_bridge *bridge)
> +{
> +	if (!drm_bridge_is_refcounted(bridge))
> +		return;
> +
> +	DRM_DEBUG("bridge=%p GET\n", bridge);
> +
> +	kref_get(&bridge->refcount);
> +}
> +
> +/**
> + * drm_bridge_put - Release a bridge reference
> + * @bridge: DRM bridge
> + *
> + * This function decrements the bridge's reference count and frees the
> + * object if the reference count drops to zero.
> + *
> + * It does nothing on non-refcounted bridges. See drm_bridge_init().
> + *
> + * See also drm_bridge_put_and_clear() which is more handy in many cases.
> + */
> +static inline void drm_bridge_put(struct drm_bridge *bridge)
> +{
> +	if (!drm_bridge_is_refcounted(bridge))
> +		return;
> +
> +	DRM_DEBUG("bridge=%p PUT\n", bridge);
> +
> +	kref_put(&bridge->refcount, bridge->free_cb);
> +}
> +
> +/**
> + * drm_bridge_put_and_clear - Given a bridge pointer, clear the pointer
> + *                            then put the bridge
> + *
> + * @br_ptr: pointer to a struct drm_bridge (must be != NULL)
> + *
> + * Helper to put a DRM bridge (whose pointer is passed), but only after
> + * setting its pointer to NULL. Useful for drivers having struct drm_bridge
> + * pointers they need to dispose of, without leaving a use-after-free
> + * window where the pointed bridge might have been freed while still
> + * holding a pointer to it.
> + *
> + * For example a driver having this private struct::
> + *
> + *     struct my_bridge {
> + *         struct drm_bridge *remote_bridge;
> + *         ...
> + *     };
> + *
> + * can dispose of remote_bridge using::
> + *
> + *     drm_bridge_put_and_clear(my_bridge->remote_bridge);
> + */
> +#define drm_bridge_put_and_clear(br_ptr) do { \
> +	struct drm_bridge **brpp = &(br_ptr); \
> +	struct drm_bridge *brp = *brpp; \
> +	*brpp = NULL; \
> +	drm_bridge_put(brp); \
> +} while (0)
> +
> +int drm_bridge_init(struct device *dev,
> +		    struct drm_bridge *bridge,
> +		    const struct drm_bridge_funcs *funcs);
>  void drm_bridge_add(struct drm_bridge *bridge);
>  int devm_drm_bridge_add(struct device *dev, struct drm_bridge *bridge);
>  void drm_bridge_remove(struct drm_bridge *bridge);

-- 
Jani Nikula, Intel

Re: [PATCH v5 08/10] drm/bridge: samsung-dsim: use supporting variable for out_bridge

[Dmitry Baryshkov]

On Tue, Dec 31, 2024 at 11:40:02AM +0100, Luca Ceresoli wrote:
> Instead of using dsi->out_bridge during the bridge search process, use a
> temporary variable and assign dsi->out_bridge only on successful
> completion.
> 
> The main goal is to be able to drm_bridge_get() the out_bridge before
> setting it in dsi->out_bridge, which is done in a later commit. Setting
> dsi->out_bridge as in current code would leave a use-after-free window in
> case the bridge is deallocated by some other thread between
> 'dsi->out_bridge = devm_drm_panel_bridge_add()' and drm_bridge_get().

I don't think that's how refcounting should work. Any of the functions
that give you the bridge should also increase refcount, requiring manual
_put() call afterwards. We might need a separate API for that.

> 
> This change additionally avoids leaving an ERR_PTR value in dsi->out_bridge
> on failure. This is not necessarily a problem but it is not clean.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> ---
> 
> This patch was added in v5.
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index f8b4fb8357659018ec0db65374ee5d05330639ae..c4d1563fd32019efde523dfc0863be044c05a826 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1705,6 +1705,7 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
>  	struct device *dev = dsi->dev;
>  	struct device_node *np = dev->of_node;
>  	struct device_node *remote;
> +	struct drm_bridge *out_bridge;
>  	struct drm_panel *panel;
>  	int ret;
>  
> @@ -1740,21 +1741,23 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
>  
>  	panel = of_drm_find_panel(remote);
>  	if (!IS_ERR(panel)) {
> -		dsi->out_bridge = devm_drm_panel_bridge_add(dev, panel);
> +		out_bridge = devm_drm_panel_bridge_add(dev, panel);
>  	} else {
> -		dsi->out_bridge = of_drm_find_bridge(remote);
> -		if (!dsi->out_bridge)
> -			dsi->out_bridge = ERR_PTR(-EINVAL);
> +		out_bridge = of_drm_find_bridge(remote);
> +		if (!out_bridge)
> +			out_bridge = ERR_PTR(-EINVAL);
>  	}

While looking at this patch, I think we should migrate the driver to
drm_of_find_panel_or_bridge(). Then your patch might add a function
close to devm_drm_of_get_bridge() or drmm_of_get_bridge(). Otherwise we
end up having too much duplicate boilerplate code.

>  
>  	of_node_put(remote);
>  
> -	if (IS_ERR(dsi->out_bridge)) {
> -		ret = PTR_ERR(dsi->out_bridge);
> +	if (IS_ERR(out_bridge)) {
> +		ret = PTR_ERR(out_bridge);
>  		DRM_DEV_ERROR(dev, "failed to find the bridge: %d\n", ret);
>  		return ret;
>  	}
>  
> +	dsi->out_bridge = out_bridge;
> +
>  	DRM_DEV_INFO(dev, "Attached %s device (lanes:%d bpp:%d mode-flags:0x%lx)\n",
>  		     device->name, device->lanes,
>  		     mipi_dsi_pixel_format_to_bpp(device->format),
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v5 09/10] drm/bridge: samsung-dsim: refcount the out_bridge

[Dmitry Baryshkov]

On Tue, Dec 31, 2024 at 11:40:03AM +0100, Luca Ceresoli wrote:
> Refcount the out_bridge to avoid a use-after-free in case it is
> hot-unplugged.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> ---
> 
> This patch was added in v5.
> ---
>  drivers/gpu/drm/bridge/samsung-dsim.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index c4d1563fd32019efde523dfc0863be044c05a826..4d32c453265931b5aecdc125623368fecacf4be3 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1756,6 +1756,7 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
>  		return ret;
>  	}
>  
> +	drm_bridge_get(out_bridge);

Well... out_bridge might already be gone now. You got the pointer, but
it is not protected in any way. Gone.

>  	dsi->out_bridge = out_bridge;
>  
>  	DRM_DEV_INFO(dev, "Attached %s device (lanes:%d bpp:%d mode-flags:0x%lx)\n",
> @@ -1774,13 +1775,13 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
>  	if (!(device->mode_flags & MIPI_DSI_MODE_VIDEO)) {
>  		ret = samsung_dsim_register_te_irq(dsi, &device->dev);
>  		if (ret)
> -			return ret;
> +			goto err_put_bridge;
>  	}
>  
>  	if (pdata->host_ops && pdata->host_ops->attach) {
>  		ret = pdata->host_ops->attach(dsi, device);
>  		if (ret)
> -			return ret;
> +			goto err_put_bridge;
>  	}
>  
>  	dsi->lanes = device->lanes;
> @@ -1788,6 +1789,10 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
>  	dsi->mode_flags = device->mode_flags;
>  
>  	return 0;
> +
> +err_put_bridge:
> +	drm_bridge_put_and_clear(dsi->out_bridge);
> +	return ret;
>  }
>  
>  static void samsung_dsim_unregister_te_irq(struct samsung_dsim *dsi)
> @@ -1804,7 +1809,7 @@ static int samsung_dsim_host_detach(struct mipi_dsi_host *host,
>  	struct samsung_dsim *dsi = host_to_dsi(host);
>  	const struct samsung_dsim_plat_data *pdata = dsi->plat_data;
>  
> -	dsi->out_bridge = NULL;
> +	drm_bridge_put_and_clear(dsi->out_bridge);
>  
>  	if (pdata->host_ops && pdata->host_ops->detach)
>  		pdata->host_ops->detach(dsi, device);
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v5 10/10] drm/bridge: hotplug-bridge: add driver to support hot-pluggable DSI bridges

[Dmitry Baryshkov]

On Tue, Dec 31, 2024 at 11:40:04AM +0100, Luca Ceresoli wrote:
> This driver implements the point of a DRM pipeline where a connector allows
> removal of all the following bridges up to the panel.
> 
> The DRM subsystem currently allows hotplug of the monitor but not preceding
> components. However there are embedded devices where the "tail" of the DRM
> pipeline, including one or more bridges, can be physically removed:
> 
>  .------------------------.
>  |   DISPLAY CONTROLLER   |
>  | .---------.   .------. |
>  | | ENCODER |<--| CRTC | |
>  | '---------'   '------' |
>  '------|-----------------'
>         |
>         |               HOTPLUG
>         V              CONNECTOR
>    .---------.        .--.    .-.        .---------.         .-------.
>    | 0 to N  |        | _|   _| |        | 1 to N  |         |       |
>    | BRIDGES |--DSI-->||_   |_  |--DSI-->| BRIDGES |--LVDS-->| PANEL |
>    |         |        |  |    | |        |         |         |       |
>    '---------'        '--'    '-'        '---------'         '-------'
> 
>  [--- fixed components --]  [----------- removable add-on -----------]
> 
> This driver supports such a device, where the final segment of a MIPI DSI
> bus, including one or more bridges, can be physically disconnected and
> reconnected at runtime, possibly with a different model.
> 
> The add-on supported by this driver has a MIPI DSI bus traversing the
> hotplug connector and a DSI to LVDS bridge and an LVDS panel on the add-on.
> Hovever this driver is designed to be as far as possible generic and
> extendable to other busses that have no native hotplug and model ID
> discovery.
> 
> This driver does not itself add and remove the bridges or panel on the
> add-on: this needs to be done by other means, e.g. device tree overlay
> runtime insertion and removal. The hotplug-bridge gets notified by the DRM
> bridge core after a removable bridge gets added or before it is removed.
> 
> The hotplug-bridge role is to implement the "hot-pluggable connector" in
> the bridge chain. In this position, what the hotplug-bridge should ideally
> do is:
> 
>  * communicate with the previous component (bridge or encoder) so that it
>    believes it always has a connected bridge following it and the DRM card
>    is always present
>  * be notified of the addition and removal of the following bridge and
>    attach/detach to/from it
>  * communicate with the following bridge so that it will attach and detach
>    using the normal procedure (as if the entire pipeline were being created
>    or destroyed, not only the tail)
>  * instantiate two DRM connectors (similarly to what the DisplayPort MST
>    code does):
>    - a DSI connector representing the video lines of the hotplug connector;
>      the status is always "disconnected" (no panel is ever attached
>      directly to it)
>    - an LSVD connector representing the classic connection to the panel;
>      this gets added/removed whenever the add-on gets
>      connected/disconnected; the status is always "connected" as the panel
>      is always connected to the preceding bridge

I'd rather have just a single connector. MST connectors can be added and
gone as there is fit, so should be your LVDS panel-related connector.

I think with the bridge refcounting and with proper notifications there
should be a perfect way to do so. On bridge removal there should be a
call to the callback from drm_encoder (or drm_connector), which removes
corresponding connector (they are dynamic!), removes a part of the
bridge chain, notifying all parties while we are doing it.

Yes, this needs one major change to the existing drivers: we have been
pushing towards drm_bridge_connector to be created during DRM card
creation time. It might be a good time to actually make a step and make
drm_bridge_connector to be created by the drm_bridge_attach() if there
is none at the end of the drm_bridge chain (or if a special flag is
being passed to it). Then make hotplug bridge signal (e.g. via the type)
that the drm_bridge_connector should not be created by default. However
once the full bridge chain is initialized (how do we identify that,
BTW?) the hotplug bridge (or the core?) can create the
drm_bride_connector.

> However some aspects make it a bit more complex than that. Most notably:
> 
>  * the next bridge can be probed and removed at any moment and all probing
>    sequences need to be handled
>  * the DSI host/device registration process, which adds to the DRM bridge
>    attach process, makes the initial card registration tricky
>  * the need to register and deregister the following bridges at runtime
>    without tearing down the whole DRM card prevents using some of the
>    functions that are normally recommended
>  * the automatic mechanism to call the appropriate .get_modes operation
>    (typically provided by the panel bridge) cannot work as the panel can
>    disappear and reappear as a different model, so an ad-hoc lookup is
>    needed
> 
> The code handling these and other tricky aspects is accurately documented
> by comments in the code.
> 
> The userspace representation when the add-on is not connected is:
> 
>   # modetest -c  | grep -i '^[a-z0-9]'
>   Connectors:
>   id    encoder status          name        size (mm)     modes   encoders
>   38    0       disconnected    DSI-1       0x0           0       37

I'm really sorry if I missed that, but the cover letter doesn't explain,
why do you need this connector at all. I think it might confuse
userspace and users.

> 
> And when it is connected a new connector appears:
> 
>   # modetest -c  | grep -i '^[a-z0-9]'
>   Connectors:
>   id    encoder status          name        size (mm)     modes   encoders
>   38    0       disconnected    DSI-1       0x0           0       37
>   39    0       connected       LVDS-1      344x194       1       37
> 
> Co-developed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> ---
> 
> Changed in v5:
>  - use drm_bridge dynamic lifetime management
>  - refcount bridge_modes and next_bridge via drm_bridge_get()/drm_bridge_put()
>  - fix dynconn removal by using drm_connector_put() and making the .destroy
>    callback work
>  - add hotplug_bridge_grab() in hotplug_bridge_attach() to handle the case
>    where the encoder driver is probed last
>  - migrate platform driver from .remove_new to .remove
> 
> Changed in v4:
>  - convert from generic notifiers to the new bridge_event_notify bridge
>    func
>  - improved and updated commit message, adding 'modetest' output
>    with/without add-on
>  - select DRM_DISPLAY_HELPER and DRM_BRIDGE_CONNECTOR, required after
>    commit 9da7ec9b19d8 ("drm/bridge-connector: move to DRM_DISPLAY_HELPER
>    module")
> 
> Changed in v3:
>  - dynamically add/remove the LVDS connector on hot(un)plug
>  - take the firmware node normally via dev->of_node instead of using
>    device_set_node(); this makes code more self-contained and generic
>  - minor rewordings and cleanups
> 
> Changed in v2:
>  - change to be a platform device instantiated from the connector driver
>    instead of a self-standing OF driver
>  - add missing error handling for devm_drm_bridge_add()
>  - various cleanups and style improvements
>  - fix typo in comment
> ---
>  MAINTAINERS                             |   5 +
>  drivers/gpu/drm/bridge/Kconfig          |  17 +
>  drivers/gpu/drm/bridge/Makefile         |   1 +
>  drivers/gpu/drm/bridge/hotplug-bridge.c | 695 ++++++++++++++++++++++++++++++++
>  4 files changed, 718 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1675e44799e55d72711a6251f692a4a14bc3a84a..ab255f5696880258deee55d99ff0a7cde85efb7c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7182,6 +7182,11 @@ T:	git https://gitlab.freedesktop.org/drm/misc/kernel.git
>  F:	Documentation/devicetree/bindings/display/panel/himax,hx8394.yaml
>  F:	drivers/gpu/drm/panel/panel-himax-hx8394.c
>  
> +DRM DRIVER FOR HOTPLUG VIDEO CONNECTOR BRIDGE
> +M:	Luca Ceresoli <luca.ceresoli@bootlin.com>
> +S:	Maintained
> +F:	drivers/gpu/drm/bridge/hotplug-bridge.c
> +
>  DRM DRIVER FOR HX8357D PANELS
>  S:	Orphan
>  T:	git https://gitlab.freedesktop.org/drm/misc/kernel.git
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 6b4664d91faa80f096ac6a0548ed342e802ae68b..f01971638d6818e33b32217922e165a8c18d51ee 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -90,6 +90,23 @@ config DRM_FSL_LDB
>  	help
>  	  Support for i.MX8MP DPI-to-LVDS on-SoC encoder.
>  
> +config DRM_HOTPLUG_BRIDGE
> +	tristate "Hotplug DRM bridge support"
> +	depends on OF
> +	select DRM_PANEL_BRIDGE
> +	select DRM_MIPI_DSI
> +	select DRM_KMS_HELPER
> +	select DRM_DISPLAY_HELPER
> +	select DRM_BRIDGE_CONNECTOR
> +	help
> +	  Driver for a DRM bridge representing a physical connector that
> +	  splits a DRM pipeline into a fixed part and a physically
> +	  removable part. The fixed part includes up to the encoder and
> +	  zero or more bridges. The removable part includes any following
> +	  bridges up to the connector and panel and can be physically
> +	  removed and connected at runtime, possibly with different
> +	  components.
> +
>  config DRM_ITE_IT6263
>  	tristate "ITE IT6263 LVDS/HDMI bridge"
>  	depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 97304b429a530c108dcbff906965cda091b0a7a2..2f6ae1a97d15045316ee191c04dbc65650162bab 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_DRM_CHRONTEL_CH7033) += chrontel-ch7033.o
>  obj-$(CONFIG_DRM_CROS_EC_ANX7688) += cros-ec-anx7688.o
>  obj-$(CONFIG_DRM_DISPLAY_CONNECTOR) += display-connector.o
>  obj-$(CONFIG_DRM_FSL_LDB) += fsl-ldb.o
> +obj-$(CONFIG_DRM_HOTPLUG_BRIDGE) += hotplug-bridge.o
>  obj-$(CONFIG_DRM_ITE_IT6263) += ite-it6263.o
>  obj-$(CONFIG_DRM_ITE_IT6505) += ite-it6505.o
>  obj-$(CONFIG_DRM_LONTIUM_LT8912B) += lontium-lt8912b.o
> diff --git a/drivers/gpu/drm/bridge/hotplug-bridge.c b/drivers/gpu/drm/bridge/hotplug-bridge.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..f717206287fc598cf7a3c5ac5bf9e1be4c8540d9
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/hotplug-bridge.c
> @@ -0,0 +1,695 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * A DRM bridge representing the split point between a fixed part of the
> + * DRM pipeline and a physically removable part. The fixed part includes up
> + * to the encoder and zero or more bridges. Insertion and removal of the
> + * "downstream" components happens via device driver probe/removal.
> + *
> + * Copyright (C) 2024, GE HealthCare
> + *
> + * Authors:
> + * Luca Ceresoli <luca.ceresoli@bootlin.com>
> + * Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> + */
> +
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/platform_device.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_bridge_connector.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_probe_helper.h>
> +
> +/*
> + * Internal hotplug-bridge data.
> + *
> + * We have two 'struct drm_connector' here:
> + * - fixconn represents the DSI video lines on the hotplug connector where
> + *   the removable part attaches. It is thus always instantiated.
> + * - dynconn represents the LVDS video lines where the panel is attached.
> + *   It is part of the removable part of the video pipeline and as such is
> + *   added and removed dynamically based on when the downstream devices
> + *   appear and disappear.
> + */
> +struct hotplug_bridge {
> +	struct device *dev;
> +
> +	/* Local bridge */
> +	struct drm_bridge bridge;
> +
> +	/* Always-present connector (where the removal part will connect to) */
> +	struct drm_connector *fixconn;
> +
> +	/* Downstream bridge (next in the chain) */
> +	struct drm_bridge *next_bridge;
> +	/* Protect next_bridge */
> +	struct mutex next_bridge_mutex;
> +
> +	/* Pointer to the last bridge exposing OP_MODES */
> +	struct drm_bridge *bridge_modes;
> +
> +	/* The "tail" connector that gets added/removed at runtime */
> +	struct drm_connector dynconn;
> +
> +	/* Local DSI host, for the downstream DSI device to attach to */
> +	struct mipi_dsi_host dsi_host;
> +	/* Local DSI device, attached to the upstream DSI host */
> +	struct mipi_dsi_device *dsi_dev;
> +	/* Upstream DSI host (the actual DSI controller) */
> +	struct mipi_dsi_host *prev_dsi_host;
> +
> +	struct work_struct hpd_work;
> +};
> +
> +static struct hotplug_bridge *hotplug_bridge_from_drm_bridge(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct hotplug_bridge, bridge);
> +}
> +
> +/* --------------------------------------------------------------------------
> + * dynconn implementation
> + */
> +static struct hotplug_bridge *hotplug_bridge_from_dynconn(struct drm_connector *conn)
> +{
> +	return container_of(conn, struct hotplug_bridge, dynconn);
> +}
> +
> +static int hotplug_bridge_dynconn_get_modes(struct drm_connector *connector)
> +{
> +	struct hotplug_bridge *hpb = hotplug_bridge_from_dynconn(connector);
> +
> +	if (hpb->bridge_modes)
> +		return hpb->bridge_modes->funcs->get_modes(hpb->bridge_modes, connector);
> +
> +	return 0;
> +}
> +
> +static const struct drm_connector_helper_funcs hotplug_bridge_dynconn_connector_helper_funcs = {
> +	.get_modes = hotplug_bridge_dynconn_get_modes,
> +};
> +
> +static void hotplug_bridge_dynconn_destroy(struct drm_connector *connector)
> +{
> +	drm_connector_unregister(connector);
> +	drm_connector_cleanup(connector);
> +}
> +
> +static const struct drm_connector_funcs dynconn_funcs = {
> +	.destroy                    = hotplug_bridge_dynconn_destroy,
> +	.atomic_duplicate_state     = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state       = drm_atomic_helper_connector_destroy_state,
> +	.fill_modes                 = drm_helper_probe_single_connector_modes,
> +};
> +
> +/*
> + * In non-removable pipelines using a "bridge connector",
> + * drm_bridge_connector_init() stores in the bridge_connector a pointer to
> + * the last bridge having OP_MODES (typically the panel bridge), so the
> + * .get_modes op will automatically be called on that bridge (it also takes
> + * pointers to other bridges which we don't care about). The "bridge
> + * connector" is too restrictive for our use case so we cannot use it. But
> + * we need a pointer to the modes-providing bridge, so we need to replicate
> + * that bit of its logic.
> + *
> + * If no modes bridge is found, nothing is done. This is allowed.
> + *
> + * Also get the modes bridge to tie its lifetime to ours.
> + */
> +static void hotplug_bridge_dynconn_bridge_modes_get(struct hotplug_bridge *hpb)
> +{
> +	struct drm_bridge *bridge;
> +
> +	if (WARN_ON(!hpb->next_bridge || !hpb->bridge.encoder))
> +		return;
> +
> +	drm_for_each_bridge_in_chain(hpb->bridge.encoder, bridge)
> +		if (bridge->ops & DRM_BRIDGE_OP_MODES) {
> +			drm_bridge_get(bridge);
> +			hpb->bridge_modes = bridge;
> +		}
> +}
> +
> +static void hotplug_bridge_dynconn_bridge_modes_put(struct hotplug_bridge *hpb)
> +{
> +	if (hpb->bridge_modes)
> +		drm_bridge_put_and_clear(hpb->bridge_modes);
> +}
> +
> +static int hotplug_bridge_dynconn_add(struct hotplug_bridge *hpb)
> +{
> +	int err;
> +
> +	err = drm_connector_init(hpb->bridge.dev, &hpb->dynconn, &dynconn_funcs,
> +				 DRM_MODE_CONNECTOR_LVDS);
> +	if (err)
> +		return err;
> +
> +	drm_atomic_helper_connector_reset(&hpb->dynconn);
> +
> +	drm_connector_helper_add(&hpb->dynconn,
> +				 &hotplug_bridge_dynconn_connector_helper_funcs);
> +
> +	drm_connector_attach_encoder(&hpb->dynconn, hpb->bridge.encoder);
> +	if (err)
> +		goto err_cleanup;
> +
> +	hotplug_bridge_dynconn_bridge_modes_get(hpb);
> +
> +	err = drm_connector_register(&hpb->dynconn);
> +	if (err)
> +		goto err_cleanup;
> +
> +	return 0;
> +
> +err_cleanup:
> +	drm_connector_cleanup(&hpb->dynconn);
> +	hotplug_bridge_dynconn_bridge_modes_put(hpb);
> +	return err;
> +}
> +
> +/* ----------------------------------------------------------------------- */
> +
> +/*
> + * Attach the remote bridge to the encoder and to the next bridge in the
> + * chain, if possible. For this to succeed, we need to know:
> + *
> + * - the encoder, which is set at the first drm_bridge_attach() time
> + * - the next bridge, which is obtained via a notifier whenever the next
> + *   bridge is (re)probed, or at probe time in case it was probed before us
> + *
> + * In order to handle different execution sequences, this function can be
> + * called from multiple places and needs to check all the prerequisites
> + * every time, and it will act only if both are met.
> + *
> + * Must be called with hpb->next_bridge_mutex held.
> + *
> + * Returns 0 if the encoder was attached successfully, -ENODEV if any of
> + * the two prerequisites above is not met (no encoder or no next bridge),
> + * the error returned by drm_bridge_attach() otherwise.
> + */
> +static int hotplug_bridge_attach_to_encoder_chain(struct hotplug_bridge *hpb)
> +{
> +	int ret;
> +
> +	if (!hpb->next_bridge || !hpb->bridge.encoder)
> +		return -ENODEV;
> +
> +	ret = drm_bridge_attach(hpb->bridge.encoder, hpb->next_bridge, &hpb->bridge,
> +				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> +	if (ret)
> +		return dev_err_probe(hpb->dev, ret, "drm_bridge_attach failed\n");
> +
> +	dev_dbg(hpb->dev, "attached to encoder chain\n");
> +
> +	return 0;
> +}
> +
> +/*
> + * Stop the video pipeline and detach next_bridge.
> + *
> + * Must be called with hpb->next_bridge_mutex held.
> + */
> +static void hotplug_bridge_detach_from_encoder_chain(struct hotplug_bridge *hpb)
> +{
> +	WARN_ON_ONCE(!hpb->next_bridge);
> +
> +	dev_dbg(hpb->dev, "detaching from encoder chain\n");
> +
> +	drm_atomic_helper_shutdown(hpb->bridge.dev);
> +
> +	drm_encoder_cleanup_from(hpb->bridge.encoder, hpb->next_bridge);
> +}
> +
> +static void hotplug_bridge_grab(struct hotplug_bridge *hpb)
> +{
> +	struct device *dev = hpb->dev;
> +	struct drm_bridge *bridge;
> +	struct drm_panel *panel;
> +	int err;
> +
> +	mutex_lock(&hpb->next_bridge_mutex);
> +
> +	if (hpb->next_bridge)
> +		goto out_unlock;
> +
> +	/*
> +	 * This is supposed to be replaced by devm_drm_of_get_bridge(), but
> +	 * that is a devm_, and we need to remove the panel bridge also on
> +	 * next_bridge disconnect.
> +	 */
> +	err = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, &bridge);
> +	if (err)
> +		goto out_unlock;
> +
> +	/* Convert the remote panel to a bridge */
> +	if (panel)
> +		bridge = drm_panel_bridge_add(panel);
> +	if (IS_ERR(bridge))
> +		goto out_unlock;
> +
> +	drm_bridge_get(bridge);
> +	hpb->next_bridge = bridge;
> +
> +	dev_dbg(dev, "grabbed next bridge (%pOFn)\n", hpb->next_bridge->of_node);
> +
> +	hpb->bridge.pre_enable_prev_first = hpb->next_bridge->pre_enable_prev_first;
> +
> +	err = hotplug_bridge_attach_to_encoder_chain(hpb);
> +	if (err)
> +		goto err_panel_bridge_remove;
> +
> +	err = hotplug_bridge_dynconn_add(hpb);
> +	if (err)
> +		goto err_detach_from_encoder_chain;
> +
> +	queue_work(system_wq, &hpb->hpd_work);
> +	goto out_unlock;
> +
> +err_detach_from_encoder_chain:
> +	hotplug_bridge_detach_from_encoder_chain(hpb);
> +err_panel_bridge_remove:
> +	drm_panel_bridge_remove(hpb->next_bridge);
> +	drm_bridge_put_and_clear(hpb->next_bridge);
> +out_unlock:
> +	mutex_unlock(&hpb->next_bridge_mutex);
> +}
> +
> +/*
> + * Detach from the next bridge and remove the panel bridge, either on
> + * release or when the downstream bridge is being removed.
> + *
> + * Can be called in these ways:
> + *
> + * - bridge_being_removed is NULL: detach unconditionally
> + *   (this is useful on .remove() to teardown everything)
> + * - bridge_being_removed == hpb->next_bridge: detach
> + *   (the downstream bridge is being removed)
> + * - bridge_being_removed != hpb->next_bridge: do nothing
> + *   (the bridge being removed is not the downstream bridge)
> + *
> + * In all cases, does nothing when there is no downstream bridge.
> + */
> +static void hotplug_bridge_release(struct hotplug_bridge *hpb,
> +				   struct drm_bridge *bridge_being_removed)
> +{
> +	mutex_lock(&hpb->next_bridge_mutex);
> +
> +	if (!hpb->next_bridge)
> +		goto out;
> +
> +	if (bridge_being_removed && bridge_being_removed != hpb->next_bridge)
> +		goto out;
> +
> +	if (hpb->bridge_modes)
> +		hotplug_bridge_dynconn_bridge_modes_put(hpb);
> +
> +	dev_dbg(hpb->dev, "releasing next bridge (%pOFn)\n", hpb->next_bridge->of_node);
> +	hotplug_bridge_detach_from_encoder_chain(hpb);
> +
> +	dev_dbg(hpb->dev, "removing %s connector\n", hpb->dynconn.name);
> +	drm_connector_put(&hpb->dynconn);
> +
> +	/*
> +	 * This will check that the bridge actually belongs to panel-bridge
> +	 * before doing anything with it, so we can safely always call it.
> +	 */
> +	drm_panel_bridge_remove(hpb->next_bridge);
> +	drm_bridge_put_and_clear(hpb->next_bridge);
> +
> +	queue_work(system_wq, &hpb->hpd_work);
> +
> +out:
> +	mutex_unlock(&hpb->next_bridge_mutex);
> +}
> +
> +static void hotplug_bridge_bridge_event_notify(struct drm_bridge *bridge,
> +					       enum drm_bridge_event_type event,
> +					       struct drm_bridge *event_bridge)
> +{
> +	struct hotplug_bridge *hpb = container_of(bridge, struct hotplug_bridge, bridge);
> +
> +	switch (event) {
> +	case DRM_EVENT_BRIDGE_ADD:
> +		hotplug_bridge_grab(hpb);
> +		break;
> +	case DRM_EVENT_BRIDGE_REMOVE:
> +		hotplug_bridge_release(hpb, event_bridge);
> +		break;
> +	}
> +}
> +
> +static int hotplug_bridge_attach(struct drm_bridge *bridge,
> +				 enum drm_bridge_attach_flags flags)
> +{
> +	struct hotplug_bridge *hpb = hotplug_bridge_from_drm_bridge(bridge);
> +	struct device *dev = hpb->dev;
> +	struct drm_connector *connector;
> +	struct drm_encoder *encoder = hpb->bridge.encoder;
> +	int err;
> +
> +	/* Encoder was not yet provided to our bridge */
> +	if (!encoder)
> +		return -ENODEV;
> +
> +	/* Connector was already created */
> +	if (hpb->fixconn)
> +		return dev_err_probe(dev, -EBUSY, "connector already created\n");
> +
> +	connector = drm_bridge_connector_init(bridge->dev, encoder);
> +	if (IS_ERR(connector))
> +		return dev_err_probe(dev, PTR_ERR(connector), "failed to initialize connector\n");
> +
> +	drm_connector_attach_encoder(connector, encoder);
> +
> +	hpb->fixconn = connector;
> +
> +	drm_connector_register(connector);
> +
> +	mutex_lock(&hpb->next_bridge_mutex);
> +	err = hotplug_bridge_attach_to_encoder_chain(hpb);
> +	mutex_unlock(&hpb->next_bridge_mutex);
> +
> +	/* -ENODEV is acceptable, in case next_bridge is not yet known */
> +	if (err == -ENODEV)
> +		err = 0;
> +
> +	/*
> +	 * If the encoder driver is probed last, the
> +	 * hotplug_bridge_attach_to_encoder_chain() call in
> +	 * hotplug_bridge_grab() fails because hpb->bridge.encoder is still
> +	 * NULL, and hotplug_bridge_grab() will not have another chance to
> +	 * execute. So call it now, at the end of the encoder attach
> +	 * process.
> +	 */
> +	hotplug_bridge_grab(hpb);
> +
> +	return err;
> +}
> +
> +static void hotplug_bridge_detach(struct drm_bridge *bridge)
> +{
> +	struct hotplug_bridge *hpb = hotplug_bridge_from_drm_bridge(bridge);
> +
> +	mutex_lock(&hpb->next_bridge_mutex);
> +	hotplug_bridge_detach_from_encoder_chain(hpb);
> +	mutex_unlock(&hpb->next_bridge_mutex);
> +
> +	if (hpb->fixconn) {
> +		drm_connector_unregister(hpb->fixconn);
> +		drm_connector_cleanup(hpb->fixconn);
> +		hpb->fixconn = NULL;
> +	}
> +}
> +
> +static void hotplug_bridge_destroy(struct drm_bridge *bridge)
> +{
> +	struct hotplug_bridge *hpb = hotplug_bridge_from_drm_bridge(bridge);
> +
> +	kfree(hpb);
> +}
> +
> +/*
> + * The fixed connector is never attached to a panel, so it should always be
> + * reported as disconnected.
> + */
> +static enum drm_connector_status hotplug_bridge_detect(struct drm_bridge *bridge)
> +{
> +	return connector_status_disconnected;
> +}
> +
> +static void hotplug_bridge_hpd_work_func(struct work_struct *work)
> +{
> +	struct hotplug_bridge *hpb = container_of(work, struct hotplug_bridge, hpd_work);
> +
> +	if (hpb->bridge.dev)
> +		drm_helper_hpd_irq_event(hpb->bridge.dev);
> +}
> +
> +static const struct drm_bridge_funcs hotplug_bridge_funcs = {
> +	.attach			= hotplug_bridge_attach,
> +	.detach			= hotplug_bridge_detach,
> +	.destroy                = hotplug_bridge_destroy,
> +	.detect			= hotplug_bridge_detect,
> +	.bridge_event_notify	= hotplug_bridge_bridge_event_notify,
> +};
> +
> +static int hotplug_bridge_dsi_detach(struct mipi_dsi_host *host,
> +				     struct mipi_dsi_device *device_remote)
> +{
> +	struct hotplug_bridge *hpb = dev_get_drvdata(host->dev);
> +
> +	if (!hpb->dsi_dev)
> +		return -ENODEV;
> +
> +	mipi_dsi_detach(hpb->dsi_dev);
> +	mipi_dsi_device_unregister(hpb->dsi_dev);
> +	hpb->dsi_dev = NULL;
> +
> +	return 0;
> +}
> +
> +/*
> + * Attach the local DSI device to the upstream DSI host, possibly with a
> + * "null" format.
> + *
> + * In "normal" bridges this function should be _only_ used as the .attach
> + * callback of hotplug_bridge_dsi_ops. But "normal" bridges have their
> + * downstream DSI device always connected, which we don't. When booting
> + * without anything connected downstream, our upstream bridge could be not
> + * even calling drm_bridge_add() until we do attach ourselves as a DSI
> + * device, preventing the whole DRM card from being instantiated.
> + *
> + * In order to always have a DRM card after boot, we do call this same
> + * function while probing in order to attach as a DSI device to the DSI
> + * master. However during probe we don't know the bus format yet. It would
> + * be nice to be able to update the format afterwards when a downstream DSI
> + * device is attaching to our local host, but there is no callback for
> + * that. To overcome this limitation, this function can be called in two
> + * ways:
> + *
> + * - during probe, to make the upstream bridge happy, when there is no
> + *   next_dsi_dev yet and thus the lanes/format/etc are unknown
> + * - as the mipi_dsi_host_ops.attach callback proper, as soon as the
> + *   next_dsi_dev is known
> + *
> + * The resulting call sequence is:
> + *
> + * 1. hotplug_bridge_dsi_attach() called by hotplug_bridge_probe() with
> + *    next_dsi_dev == NULL: we attach to the host but with a fake format
> + *    so the DRM card can be populated. hpb->dsi_dev becomes non-NULL.
> + * 2. hotplug_bridge_dsi_attach() called as .attach callback from a
> + *    downstream device when it becomes available: we need to detach in
> + *    order to re-attach with the format of the device. hpb->dsi_dev
> + *    is found non-NULL, then reused so it will be non-NULL again.
> + * 3. hotplug_bridge_dsi_detach() called as the .detach callback by a
> + *    downstream device: cleans up everything normally. hpb->dsi_dev goes
> + *    from non-NULL to NULL.
> + * 4. hotplug_bridge_dsi_attach() called by a downstream device: attaches
> + *    normally to the upstream DSI host. hpb->dsi_dev goes from NULL to
> + *    non-NULL.
> + *
> + * Steps 3 and 4 are the "normal" attach/detach steps as on "normal"
> + * bridges.
> + *
> + * Steps 1 and 2 happen only the first time, steps 3 and 4 will happen
> + * every time the downstream bridge disconnects and reconnects.
> + */
> +static int hotplug_bridge_dsi_attach(struct mipi_dsi_host *host,
> +				     struct mipi_dsi_device *next_dsi_dev)
> +{
> +	struct device *dev = host->dev;
> +	struct hotplug_bridge *hpb = dev_get_drvdata(dev);
> +	struct mipi_dsi_device *dsi_dev;
> +	const struct mipi_dsi_device_info dsi_info = {
> +		.type = "hotplug-bridge",
> +		.channel = 0,
> +		.node = NULL,
> +	};
> +	int err;
> +
> +	/*
> +	 * Step 2 only (first time we are called for an actual device
> +	 * attaching): clean up the fake attach done at step 1
> +	 */
> +	if (hpb->dsi_dev)
> +		hotplug_bridge_dsi_detach(&hpb->dsi_host, NULL);
> +
> +	/* Register a local DSI device with the remote DSI host */
> +	dsi_dev = mipi_dsi_device_register_full(hpb->prev_dsi_host,
> +						&dsi_info);
> +	if (IS_ERR(dsi_dev))
> +		return PTR_ERR(dsi_dev);
> +
> +	/* At step 1 we have no downstream device to get the format from */
> +	if (next_dsi_dev) {
> +		dsi_dev->channel    = next_dsi_dev->channel;
> +		dsi_dev->lanes      = next_dsi_dev->lanes;
> +		dsi_dev->format     = next_dsi_dev->format;
> +		dsi_dev->mode_flags = next_dsi_dev->mode_flags;
> +	}
> +
> +	/* Attach our local DSI device to the remote DSI host */
> +	err = mipi_dsi_attach(dsi_dev);
> +	if (err) {
> +		mipi_dsi_device_unregister(dsi_dev);
> +		return dev_err_probe(dev, err, "failed to attach hotplug dsi device to host\n");
> +	}
> +
> +	hpb->dsi_dev = dsi_dev;
> +
> +	return 0;
> +}
> +
> +/*
> + * Propagate mipi_dsi_device_transfer() to the upstream DSI host.
> + *
> + * Reimplements identically the minimal needed part of
> + * mipi_dsi_device_transfer(), including the -ENOSYS return value.
> + */
> +static ssize_t hotplug_bridge_dsi_transfer(struct mipi_dsi_host *host,
> +					   const struct mipi_dsi_msg *msg)
> +{
> +	struct hotplug_bridge *hpb = dev_get_drvdata(host->dev);
> +	const struct mipi_dsi_host_ops *ops;
> +
> +	if (!hpb->dsi_dev)
> +		return -ENODEV;
> +
> +	ops = hpb->dsi_dev->host->ops;
> +
> +	if (!ops || !ops->transfer)
> +		return -ENOSYS;
> +
> +	return ops->transfer(hpb->dsi_dev->host, msg);
> +}
> +
> +static const struct mipi_dsi_host_ops hotplug_bridge_dsi_ops = {
> +	.attach		= hotplug_bridge_dsi_attach,
> +	.detach		= hotplug_bridge_dsi_detach,
> +	.transfer	= hotplug_bridge_dsi_transfer,
> +};
> +
> +/*
> + * Find the upstream DSI host and register our downstream-facing DSI host.
> + */
> +static int hotplug_bridge_dsi_setup(struct hotplug_bridge *hpb)
> +{
> +	struct device *dev = hpb->dev;
> +	struct device_node *endpoint;
> +	struct device_node *node;
> +
> +	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, -1);
> +	node = of_graph_get_remote_port_parent(endpoint);
> +
> +	hpb->prev_dsi_host = of_find_mipi_dsi_host_by_node(node);
> +
> +	of_node_put(node);
> +	of_node_put(endpoint);
> +
> +	if (!hpb->prev_dsi_host)
> +		return -EPROBE_DEFER;
> +
> +	hpb->dsi_host.dev = dev;
> +	hpb->dsi_host.ops = &hotplug_bridge_dsi_ops;
> +
> +	return mipi_dsi_host_register(&hpb->dsi_host);
> +}
> +
> +static void hotplug_bridge_dsi_cleanup(struct hotplug_bridge *hpb)
> +{
> +	mipi_dsi_host_unregister(&hpb->dsi_host);
> +}
> +
> +static int hotplug_bridge_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct hotplug_bridge *hpb;
> +	struct drm_bridge *bridge;
> +	int err;
> +
> +	hpb = kzalloc(sizeof(*hpb), GFP_KERNEL);
> +	if (!hpb)
> +		return -ENOMEM;
> +
> +	err = drm_bridge_init(dev, &hpb->bridge, &hotplug_bridge_funcs);
> +	if (err)
> +		return err;
> +
> +	hpb->dev = dev;
> +
> +	mutex_init(&hpb->next_bridge_mutex);
> +	INIT_WORK(&hpb->hpd_work, hotplug_bridge_hpd_work_func);
> +
> +	err = hotplug_bridge_dsi_setup(hpb);
> +	if (err)
> +		return dev_err_probe(dev, err, "failed to setup DSI\n");
> +
> +	bridge = &hpb->bridge;
> +	bridge->of_node = dev->of_node;
> +	bridge->type = DRM_MODE_CONNECTOR_DSI;
> +	bridge->ops |= DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_HPD;
> +
> +	platform_set_drvdata(pdev, hpb);
> +
> +	err = devm_drm_bridge_add(dev, bridge);
> +	if (err) {
> +		dev_err_probe(dev, err, "failed adding bridge\n");
> +		goto err_dsi_cleanup;
> +	}
> +
> +	err = hotplug_bridge_dsi_attach(&hpb->dsi_host, NULL);
> +	if (err) {
> +		dev_err_probe(dev, err, "failed first attach to upstream DSI host\n");
> +		goto err_dsi_cleanup;
> +	}
> +
> +	/*
> +	 * Since devm_drm_bridge_add() we can be notified of any bridges
> +	 * appearing, but also check now, in case the next bridge was
> +	 * probed earlier
> +	 */
> +	hotplug_bridge_grab(hpb);
> +
> +	return 0;
> +
> +err_dsi_cleanup:
> +	hotplug_bridge_dsi_cleanup(hpb);
> +	return err;
> +}
> +
> +static void hotplug_bridge_remove(struct platform_device *pdev)
> +{
> +	struct hotplug_bridge *hpb = platform_get_drvdata(pdev);
> +
> +	cancel_work_sync(&hpb->hpd_work);
> +
> +	hotplug_bridge_release(hpb, NULL);
> +
> +	hotplug_bridge_dsi_cleanup(hpb);
> +}
> +
> +static const struct platform_device_id hotplug_bridge_platform_ids[] = {
> +	{ .name = "hotplug-dsi-bridge" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(platform, hotplug_bridge_platform_ids);
> +
> +static struct platform_driver hotplug_bridge_driver = {
> +	.probe		= hotplug_bridge_probe,
> +	.remove		= hotplug_bridge_remove,
> +	.id_table	= hotplug_bridge_platform_ids,
> +	.driver		= {
> +		.name		= "hotplug-drm-bridge",
> +	},
> +};
> +
> +module_platform_driver(hotplug_bridge_driver);
> +
> +MODULE_AUTHOR("Luca Ceresoli <luca.ceresoli@bootlin.com>");
> +MODULE_AUTHOR("Paul Kocialkowski <paul.kocialkowski@bootlin.com>");
> +MODULE_DESCRIPTION("Hotplug DRM Bridge");
> +MODULE_LICENSE("GPL");
> 
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v5 04/10] drm/bridge: add documentation of refcounted bridges

[Randy Dunlap]

Hi--

On 12/31/24 2:39 AM, Luca Ceresoli wrote:
> Document in detail the new refcounted bridges as well as the "legacy" way.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> 
> ---
> 
> This patch was added in v5.
> ---
>  Documentation/gpu/drm-kms-helpers.rst |   6 ++
>  drivers/gpu/drm/drm_bridge.c          | 122 ++++++++++++++++++++++++++++++++++
>  2 files changed, 128 insertions(+)
> 
> diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
> index 8cf2f041af4704875910ce8228ae04615d0f21bd..ca2cfef2101988933e1464fe146997c1a661a117 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -151,6 +151,12 @@ Overview
>  .. kernel-doc:: drivers/gpu/drm/drm_bridge.c
>     :doc: overview
>  
> +Bridge lifecycle
> +----------------
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_bridge.c
> +   :doc: bridge lifecycle
> +
>  Display Driver Integration
>  --------------------------
>  
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 6255ef59f73d8041a8cb7f2c6e23e5a67d1ae926..e9f138aa5b3270b4e3a1a56dc8d4b7e5f993c929 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -60,6 +60,128 @@
>   * encoder chain.
>   */
>  
> +/**
> + * DOC: bridge lifecycle
> + *
> + * Allocation, initializion and teardown of a bridge can be implemented in

                  initialization

> + * one of two ways: *refcounted* mode and *legacy* mode.
> + *
> + * In **refcounted** mode:
> + *
> + * - each &struct drm_bridge is reference counted since its instantiation
> + * - any code taking a pointer to a bridge has get and put APIs to refcount
> + *   it and so ensure the bridge won't be deallocated while using it
> + * - deallocation is done when the last put happens and the refcount drops
> + *   to zero
> + * - the driver instantiating the bridge also holds a reference, but the
> + *   allocated struct can survive it
> + *
> + * A bridge using refcounted mode is called a *refcounted bridge*.
> + *
> + * In **legacy** mode the &struct drm_bridge lifetime is tied to the device
> + * instantiating it: it is allocated on probe and freed on removal. Any
> + * other kernel entities holding a pointer to the bridge could incur in
> + * use-after-free in case the bridge is deallocated at runtime.
> + *
> + * Legacy mode used to be the only one until refcounted bridges were
> + * introduced, hance the name. It is still fine in case the bridges are a

                  hence

> + * fixed part of the pipeline, i.e. if the bridges are removed only when
> + * tearing down the entire card. Refcounted bridges support both that case
> + * and the case of more dynamic hardware with bridges that can be removed
> + * at runtime without tearing down the entire card.
> + *
> + * Usage of refcounted bridges happens in two sides: the driver
> + * implementing the bridge and the code using the bridge.
> + *
> + * For *drivers implemeting the bridge*, in both refcounted and legacy

                   implementing

> + * modes the common and expected pattern is that the driver declares a
> + * driver-specific struct embedding a &struct drm_bridge. E.g.::


-- 
~Randy

Re: [PATCH v5 10/10] drm/bridge: hotplug-bridge: add driver to support hot-pluggable DSI bridges

[Luca Ceresoli]

Hi Dmitry,

On Tue, 31 Dec 2024 17:29:52 +0200
Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> On Tue, Dec 31, 2024 at 11:40:04AM +0100, Luca Ceresoli wrote:
> > This driver implements the point of a DRM pipeline where a connector allows
> > removal of all the following bridges up to the panel.
> > 
> > The DRM subsystem currently allows hotplug of the monitor but not preceding
> > components. However there are embedded devices where the "tail" of the DRM
> > pipeline, including one or more bridges, can be physically removed:
> > 
> >  .------------------------.
> >  |   DISPLAY CONTROLLER   |
> >  | .---------.   .------. |
> >  | | ENCODER |<--| CRTC | |
> >  | '---------'   '------' |
> >  '------|-----------------'
> >         |
> >         |               HOTPLUG
> >         V              CONNECTOR
> >    .---------.        .--.    .-.        .---------.         .-------.
> >    | 0 to N  |        | _|   _| |        | 1 to N  |         |       |
> >    | BRIDGES |--DSI-->||_   |_  |--DSI-->| BRIDGES |--LVDS-->| PANEL |
> >    |         |        |  |    | |        |         |         |       |
> >    '---------'        '--'    '-'        '---------'         '-------'
> > 
> >  [--- fixed components --]  [----------- removable add-on -----------]
> > 
> > This driver supports such a device, where the final segment of a MIPI DSI
> > bus, including one or more bridges, can be physically disconnected and
> > reconnected at runtime, possibly with a different model.
> > 
> > The add-on supported by this driver has a MIPI DSI bus traversing the
> > hotplug connector and a DSI to LVDS bridge and an LVDS panel on the add-on.
> > Hovever this driver is designed to be as far as possible generic and
> > extendable to other busses that have no native hotplug and model ID
> > discovery.
> > 
> > This driver does not itself add and remove the bridges or panel on the
> > add-on: this needs to be done by other means, e.g. device tree overlay
> > runtime insertion and removal. The hotplug-bridge gets notified by the DRM
> > bridge core after a removable bridge gets added or before it is removed.
> > 
> > The hotplug-bridge role is to implement the "hot-pluggable connector" in
> > the bridge chain. In this position, what the hotplug-bridge should ideally
> > do is:
> > 
> >  * communicate with the previous component (bridge or encoder) so that it
> >    believes it always has a connected bridge following it and the DRM card
> >    is always present
> >  * be notified of the addition and removal of the following bridge and
> >    attach/detach to/from it
> >  * communicate with the following bridge so that it will attach and detach
> >    using the normal procedure (as if the entire pipeline were being created
> >    or destroyed, not only the tail)
> >  * instantiate two DRM connectors (similarly to what the DisplayPort MST
> >    code does):
> >    - a DSI connector representing the video lines of the hotplug connector;
> >      the status is always "disconnected" (no panel is ever attached
> >      directly to it)
> >    - an LSVD connector representing the classic connection to the panel;
> >      this gets added/removed whenever the add-on gets
> >      connected/disconnected; the status is always "connected" as the panel
> >      is always connected to the preceding bridge  
> 
> I'd rather have just a single connector. MST connectors can be added and
> gone as there is fit, so should be your LVDS panel-related connector.

The plan we discussed at LPC 2024 is to eventually get rid of the first
connector (see "Roadmap and current status" in the cover letter), so
you can consider this legacy code. However the current implementation
won't work without this connector, so it is still there for the time
being. Pointing this out in a note in the commit message of this patch
would probably be useful to avoid future misunderstanding, so I'm
adding one for v6.

Thanks for reviewing!

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v5 08/10] drm/bridge: samsung-dsim: use supporting variable for out_bridge

[Luca Ceresoli]

Hi Dmitry,

On Tue, 31 Dec 2024 16:57:38 +0200
Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> On Tue, Dec 31, 2024 at 11:40:02AM +0100, Luca Ceresoli wrote:
> > Instead of using dsi->out_bridge during the bridge search process, use a
> > temporary variable and assign dsi->out_bridge only on successful
> > completion.
> > 
> > The main goal is to be able to drm_bridge_get() the out_bridge before
> > setting it in dsi->out_bridge, which is done in a later commit. Setting
> > dsi->out_bridge as in current code would leave a use-after-free window in
> > case the bridge is deallocated by some other thread between
> > 'dsi->out_bridge = devm_drm_panel_bridge_add()' and drm_bridge_get().  
> 
> I don't think that's how refcounting should work. Any of the functions
> that give you the bridge should also increase refcount, requiring manual
> _put() call afterwards. We might need a separate API for that.

You're perfectly right.

> > This change additionally avoids leaving an ERR_PTR value in dsi->out_bridge
> > on failure. This is not necessarily a problem but it is not clean.
> > 
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > 
> > ---
> > 
> > This patch was added in v5.
> > ---
> >  drivers/gpu/drm/bridge/samsung-dsim.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > index f8b4fb8357659018ec0db65374ee5d05330639ae..c4d1563fd32019efde523dfc0863be044c05a826 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -1705,6 +1705,7 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
> >  	struct device *dev = dsi->dev;
> >  	struct device_node *np = dev->of_node;
> >  	struct device_node *remote;
> > +	struct drm_bridge *out_bridge;
> >  	struct drm_panel *panel;
> >  	int ret;
> >  
> > @@ -1740,21 +1741,23 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
> >  
> >  	panel = of_drm_find_panel(remote);
> >  	if (!IS_ERR(panel)) {
> > -		dsi->out_bridge = devm_drm_panel_bridge_add(dev, panel);
> > +		out_bridge = devm_drm_panel_bridge_add(dev, panel);
> >  	} else {
> > -		dsi->out_bridge = of_drm_find_bridge(remote);
> > -		if (!dsi->out_bridge)
> > -			dsi->out_bridge = ERR_PTR(-EINVAL);
> > +		out_bridge = of_drm_find_bridge(remote);
> > +		if (!out_bridge)
> > +			out_bridge = ERR_PTR(-EINVAL);
> >  	}  
> 
> While looking at this patch, I think we should migrate the driver to
> drm_of_find_panel_or_bridge().

Indeed, the code here is duplicating drm_of_find_panel_or_bridge(). I'm
going to convert it.

> Then your patch might add a function
> close to devm_drm_of_get_bridge() or drmm_of_get_bridge().

...which would return a bridge pointer, with refcount already
incremented. Sure, except I think it should _not_ be a drmm, as
the bridge might itself disappear while the card keeps existing.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v5 04/10] drm/bridge: add documentation of refcounted bridges

[Luca Ceresoli]

Hello Randy,

On Tue, 31 Dec 2024 09:54:41 -0800
Randy Dunlap <rdunlap@infradead.org> wrote:

> Hi--
> 
> On 12/31/24 2:39 AM, Luca Ceresoli wrote:
> > Document in detail the new refcounted bridges as well as the "legacy" way.
> > 
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > 
> > ---

...

> > + * Allocation, initializion and teardown of a bridge can be implemented in  
> 
>                   initialization

Thanks for reviewing! I fixed locally the typos you spotted.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v5 03/10] drm/bridge: add support for refcounted DRM bridges

[Luca Ceresoli]

Hello Jani,

thanks for your review.

On Tue, 31 Dec 2024 13:11:31 +0200
Jani Nikula <jani.nikula@linux.intel.com> wrote:

> On Tue, 31 Dec 2024, Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> > DRM bridges are currently considered as a fixed element of a DRM card, and
> > thus their lifetime is assumed to extend for as long as the card
> > exists. New use cases, such as hot-pluggable hardware with video bridges,
> > require DRM bridges to be added and removed to a DRM card without tearing
> > the card down. This is possible for connectors already (used by DP MST), so
> > add this possibility to DRM bridges as well.
> >
> > Implementation is based on drm_connector_init() as far as it makes sense,
> > and differs when it doesn't. A difference is that bridges are not exposed
> > to userspace,hence struct drm_bridge does not embed a struct
> > drm_mode_object which would provide the refcount and the free_cb. So here
> > we add to struct drm_bridge just the refcount and free_cb fields (we don't
> > need other struct drm_mode_object fields here) and instead of using the
> > drm_mode_object_*() functions we reimplement from those functions the few
> > lines that drm_bridge needs for refcounting.
> >
> > The function to enroll a private bridge driver data structure into
> > refcounting is based on drm_connector_init() and so called
> > drm_bridge_init() for symmetry, even though it does not initialize anything
> > except the refcounting and the funcs pointer which is needed to access
> > funcs->destroy.
> >
> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> >
> > ---
> >
> > This patch was added in v5.
> > ---
> >  drivers/gpu/drm/drm_bridge.c |  87 ++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_bridge.h     | 102 +++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 189 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index b1f0d25d55e23000521ac2ac37ee410348978ed4..6255ef59f73d8041a8cb7f2c6e23e5a67d1ae926 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -198,6 +198,85 @@
> >  static DEFINE_MUTEX(bridge_lock);
> >  static LIST_HEAD(bridge_list);
> >  
> > +static void drm_bridge_put_void(void *data)
> > +{
> > +	struct drm_bridge *bridge = (struct drm_bridge *)data;
> > +
> > +	drm_bridge_put(bridge);
> > +}
> > +
> > +static void drm_bridge_free(struct kref *kref)
> > +{
> > +	struct drm_bridge *bridge = container_of(kref, struct drm_bridge, refcount);
> > +
> > +	DRM_DEBUG("bridge=%p\n", bridge);
> > +
> > +	WARN_ON(!bridge->funcs->destroy);  
> 
> Please don't add new DRM_DEBUG or WARN_ON where you can use the
> drm_dbg_* or drm_WARN_ON variants.

Good point. However drm_WARN_ON() cannot be used because it needs a
non-NULL struct drm_drm_device pointer which is not always available
here: in case of -EPROBE_DEFER it usually isn't. I guess I'll go for
drm_dbg_core() or drm_warn[_once](), even though none of them prints a
stack trace and I find that would be useful.

This is raising a loosely-related question about the DRM_DEBUG()s this
patch is adding, such as the one quoted above: would it make sense to
add a new drm_debug_category value for the bridge refcounting
functions? Or for bridges altogether? They are pretty different from
the core messages, and it may be useful to see only the refcounting
messages or only the core messages.

DRM_UT_BRIDGE?
DRM_UT_BRIDGE_REFCOUNT?

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v5 08/10] drm/bridge: samsung-dsim: use supporting variable for out_bridge

[Dmitry Baryshkov]

On Thu, Jan 02, 2025 at 01:01:49PM +0100, Luca Ceresoli wrote:
> Hi Dmitry,
> 
> On Tue, 31 Dec 2024 16:57:38 +0200
> Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> 
> > On Tue, Dec 31, 2024 at 11:40:02AM +0100, Luca Ceresoli wrote:
> > > Instead of using dsi->out_bridge during the bridge search process, use a
> > > temporary variable and assign dsi->out_bridge only on successful
> > > completion.
> > > 
> > > The main goal is to be able to drm_bridge_get() the out_bridge before
> > > setting it in dsi->out_bridge, which is done in a later commit. Setting
> > > dsi->out_bridge as in current code would leave a use-after-free window in
> > > case the bridge is deallocated by some other thread between
> > > 'dsi->out_bridge = devm_drm_panel_bridge_add()' and drm_bridge_get().  
> > 
> > I don't think that's how refcounting should work. Any of the functions
> > that give you the bridge should also increase refcount, requiring manual
> > _put() call afterwards. We might need a separate API for that.
> 
> You're perfectly right.
> 
> > > This change additionally avoids leaving an ERR_PTR value in dsi->out_bridge
> > > on failure. This is not necessarily a problem but it is not clean.
> > > 
> > > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > > 
> > > ---
> > > 
> > > This patch was added in v5.
> > > ---
> > >  drivers/gpu/drm/bridge/samsung-dsim.c | 15 +++++++++------
> > >  1 file changed, 9 insertions(+), 6 deletions(-)
> > > 

[...]

> > > @@ -1740,21 +1741,23 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
> > >  
> > >  	panel = of_drm_find_panel(remote);
> > >  	if (!IS_ERR(panel)) {
> > > -		dsi->out_bridge = devm_drm_panel_bridge_add(dev, panel);
> > > +		out_bridge = devm_drm_panel_bridge_add(dev, panel);
> > >  	} else {
> > > -		dsi->out_bridge = of_drm_find_bridge(remote);
> > > -		if (!dsi->out_bridge)
> > > -			dsi->out_bridge = ERR_PTR(-EINVAL);
> > > +		out_bridge = of_drm_find_bridge(remote);
> > > +		if (!out_bridge)
> > > +			out_bridge = ERR_PTR(-EINVAL);
> > >  	}  
> > 
> > While looking at this patch, I think we should migrate the driver to
> > drm_of_find_panel_or_bridge().
> 
> Indeed, the code here is duplicating drm_of_find_panel_or_bridge(). I'm
> going to convert it.
> 
> > Then your patch might add a function
> > close to devm_drm_of_get_bridge() or drmm_of_get_bridge().
> 
> ...which would return a bridge pointer, with refcount already
> incremented. Sure, except I think it should _not_ be a drmm, as
> the bridge might itself disappear while the card keeps existing.

Feel free to add new one.

> 
> Luca
> 
> -- 
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

-- 
With best wishes
Dmitry

Re: [PATCH v5 03/10] drm/bridge: add support for refcounted DRM bridges

[Jani Nikula]

On Thu, 02 Jan 2025, Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> Hello Jani,
>
> thanks for your review.
>
> On Tue, 31 Dec 2024 13:11:31 +0200
> Jani Nikula <jani.nikula@linux.intel.com> wrote:
>
>> On Tue, 31 Dec 2024, Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
>> > DRM bridges are currently considered as a fixed element of a DRM card, and
>> > thus their lifetime is assumed to extend for as long as the card
>> > exists. New use cases, such as hot-pluggable hardware with video bridges,
>> > require DRM bridges to be added and removed to a DRM card without tearing
>> > the card down. This is possible for connectors already (used by DP MST), so
>> > add this possibility to DRM bridges as well.
>> >
>> > Implementation is based on drm_connector_init() as far as it makes sense,
>> > and differs when it doesn't. A difference is that bridges are not exposed
>> > to userspace,hence struct drm_bridge does not embed a struct
>> > drm_mode_object which would provide the refcount and the free_cb. So here
>> > we add to struct drm_bridge just the refcount and free_cb fields (we don't
>> > need other struct drm_mode_object fields here) and instead of using the
>> > drm_mode_object_*() functions we reimplement from those functions the few
>> > lines that drm_bridge needs for refcounting.
>> >
>> > The function to enroll a private bridge driver data structure into
>> > refcounting is based on drm_connector_init() and so called
>> > drm_bridge_init() for symmetry, even though it does not initialize anything
>> > except the refcounting and the funcs pointer which is needed to access
>> > funcs->destroy.
>> >
>> > Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
>> >
>> > ---
>> >
>> > This patch was added in v5.
>> > ---
>> >  drivers/gpu/drm/drm_bridge.c |  87 ++++++++++++++++++++++++++++++++++++
>> >  include/drm/drm_bridge.h     | 102 +++++++++++++++++++++++++++++++++++++++++++
>> >  2 files changed, 189 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> > index b1f0d25d55e23000521ac2ac37ee410348978ed4..6255ef59f73d8041a8cb7f2c6e23e5a67d1ae926 100644
>> > --- a/drivers/gpu/drm/drm_bridge.c
>> > +++ b/drivers/gpu/drm/drm_bridge.c
>> > @@ -198,6 +198,85 @@
>> >  static DEFINE_MUTEX(bridge_lock);
>> >  static LIST_HEAD(bridge_list);
>> >  
>> > +static void drm_bridge_put_void(void *data)
>> > +{
>> > +	struct drm_bridge *bridge = (struct drm_bridge *)data;
>> > +
>> > +	drm_bridge_put(bridge);
>> > +}
>> > +
>> > +static void drm_bridge_free(struct kref *kref)
>> > +{
>> > +	struct drm_bridge *bridge = container_of(kref, struct drm_bridge, refcount);
>> > +
>> > +	DRM_DEBUG("bridge=%p\n", bridge);
>> > +
>> > +	WARN_ON(!bridge->funcs->destroy);  
>> 
>> Please don't add new DRM_DEBUG or WARN_ON where you can use the
>> drm_dbg_* or drm_WARN_ON variants.
>
> Good point. However drm_WARN_ON() cannot be used because it needs a
> non-NULL struct drm_drm_device pointer which is not always available
> here: in case of -EPROBE_DEFER it usually isn't. I guess I'll go for
> drm_dbg_core() or drm_warn[_once](), even though none of them prints a
> stack trace and I find that would be useful.

drm_dbg_* can handle NULL drm device; maybe drm_WARN* should be modified
to do so as well?

> This is raising a loosely-related question about the DRM_DEBUG()s this
> patch is adding, such as the one quoted above: would it make sense to
> add a new drm_debug_category value for the bridge refcounting
> functions? Or for bridges altogether? They are pretty different from
> the core messages, and it may be useful to see only the refcounting
> messages or only the core messages.
>
> DRM_UT_BRIDGE?
> DRM_UT_BRIDGE_REFCOUNT?

IMO the biggest benefit of new categories would be for very noisy
logging that you really only want enabled when debugging a specific
issue. Otherwise, the hard part about adding new categories is their
adoption.

For example, we now have DRM_UT_DP, but it's only haphazardly used here
and there. I don't really see a lot of point in having that separated
from DRM_UT_KMS. When would you want one but not the other? How would
you go about converting some KMS to DP logging, and why? What's special
about DP, why don't we have an HDMI category? Etc.

OTOH, DRM_UT_DP is also used for DP AUX transfer debug logging. I think
that would've been a good category on its own: Do you want noisy logging
about DPCD access or not? But not used for anything else.

Oh, having written the above, I looked up a18b21929453 ("drm/dp_helper:
Add DP aux channel tracing"). DRM_UT_DP *was* intended only for DP AUX
message tracing, but its naming unfortunately suggests a broader
category, and here we are.


BR,
Jani.


-- 
Jani Nikula, Intel

Re: [PATCH v5 04/10] drm/bridge: add documentation of refcounted bridges

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3455 bytes --]

Hi,

Most of these comments affect your earlier patches, but let's work on
the API-level view.

On Tue, Dec 31, 2024 at 11:39:58AM +0100, Luca Ceresoli wrote:
> + * When using refcounted mode, the driver should allocate ``struct
> + * my_bridge`` using regular allocation (as opposed to ``devm_`` or
> + * ``drmm_`` allocation), call drm_bridge_init() immediately afterwards to
> + * transfer lifecycle management to the DRM bridge core, and implement a
> + * ``.destroy`` function to deallocate the ``struct my_bridge``, as in this
> + * example::
> + *
> + *     static void my_bridge_destroy(struct drm_bridge *bridge)
> + *     {
> + *         kfree(container_of(bridge, struct my_bridge, bridge));
> + *     }
> + *
> + *     static const struct drm_bridge_funcs my_bridge_funcs = {
> + *         .destroy = my_bridge_destroy,
> + *         ...
> + *     };
> + *
> + *     static int my_bridge_probe(...)
> + *     {
> + *         struct my_bridge *mybr;
> + *         int err;
> + *
> + *         mybr = kzalloc(sizeof(*mybr), GFP_KERNEL);
> + *         if (!mybr)
> + *             return -ENOMEM;
> + *
> + *         err = drm_bridge_init(dev, &mybr->bridge, &my_bridge_funcs);
> + *         if (err)
> + *             return err;
> + *
> + *         ...
> + *         drm_bridge_add();
> + *         ...
> + *     }
> + *
> + *     static void my_bridge_remove()
> + *     {
> + *         struct my_bridge *mybr = ...;
> + *         drm_bridge_remove(&mybr->bridge);
> + *         // ... NO kfree here!
> + *     }

I'm a bit worried there, since that API is pretty difficult to get
right, and we don't have anything to catch bad patterns.

Let's take a step back. What we're trying to solve here is:

  1) We want to avoid any dangling pointers to a bridge if the bridge
     device is removed.

  2) To do so, we need to switch to reference counted allocations and
     pointers.

  3) Most bridges structures are allocated through devm_kzalloc, and they
     one that aren't are freed at remove time anyway, so the allocated
     structure will be gone when the device is removed.

  4) To properly track users, each user that will use a drm_bridge needs
     to take a reference.

AFAIU, the destroy introduction and the on-purpose omission of kfree in
remove is to solve 3.

Introducing a function that allocates the drm_bridge container struct
(like drmm_encoder_alloc for example), take a reference, register a devm
kfree action, and return the pointer to the driver structure would solve
that too pretty nicely.

So, something like:


struct driver_priv {
       struct drm_bridge bridge;

       ...
}

static int driver_probe(...)
{
	struct driver_priv *priv;
	struct drm_bridge *bridge;

        ....

	priv = devm_drm_bridge_alloc(dev, struct driver_priv, bridge);
	if (IS_ERR(priv))
	   return ERR_PTR(priv);
	bridge = &priv->bridge;

	...

	drm_bridge_add(bridge);
}

Would work just as well.

I also don't think we need explicit (at least for the common case)
drm_bridge_get and drm_bridge_put calls for bridge users.
drm_bridge_attach and drm_bridge_detach can get/put the reference
directly.

And we'll also need some flag in drm_bridge to indicate that the device
is gone, similar to what drm_dev_enter()/drm_dev_exit() provides,
because now your bridge driver sticks around for much longer than your
device so the expectation that your device managed resources (clocks,
registers, etc.) are always going to be around.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v5 04/10] drm/bridge: add documentation of refcounted bridges

[Dmitry Baryshkov]

On Mon, 6 Jan 2025 at 12:39, Maxime Ripard <mripard@kernel.org> wrote:
>
> Hi,
>
> Most of these comments affect your earlier patches, but let's work on
> the API-level view.
>
> On Tue, Dec 31, 2024 at 11:39:58AM +0100, Luca Ceresoli wrote:
> > + * When using refcounted mode, the driver should allocate ``struct
> > + * my_bridge`` using regular allocation (as opposed to ``devm_`` or
> > + * ``drmm_`` allocation), call drm_bridge_init() immediately afterwards to
> > + * transfer lifecycle management to the DRM bridge core, and implement a
> > + * ``.destroy`` function to deallocate the ``struct my_bridge``, as in this
> > + * example::
> > + *
> > + *     static void my_bridge_destroy(struct drm_bridge *bridge)
> > + *     {
> > + *         kfree(container_of(bridge, struct my_bridge, bridge));
> > + *     }
> > + *
> > + *     static const struct drm_bridge_funcs my_bridge_funcs = {
> > + *         .destroy = my_bridge_destroy,
> > + *         ...
> > + *     };
> > + *
> > + *     static int my_bridge_probe(...)
> > + *     {
> > + *         struct my_bridge *mybr;
> > + *         int err;
> > + *
> > + *         mybr = kzalloc(sizeof(*mybr), GFP_KERNEL);
> > + *         if (!mybr)
> > + *             return -ENOMEM;
> > + *
> > + *         err = drm_bridge_init(dev, &mybr->bridge, &my_bridge_funcs);
> > + *         if (err)
> > + *             return err;
> > + *
> > + *         ...
> > + *         drm_bridge_add();
> > + *         ...
> > + *     }
> > + *
> > + *     static void my_bridge_remove()
> > + *     {
> > + *         struct my_bridge *mybr = ...;
> > + *         drm_bridge_remove(&mybr->bridge);
> > + *         // ... NO kfree here!
> > + *     }
>
> I'm a bit worried there, since that API is pretty difficult to get
> right, and we don't have anything to catch bad patterns.
>
> Let's take a step back. What we're trying to solve here is:
>
>   1) We want to avoid any dangling pointers to a bridge if the bridge
>      device is removed.
>
>   2) To do so, we need to switch to reference counted allocations and
>      pointers.
>
>   3) Most bridges structures are allocated through devm_kzalloc, and they
>      one that aren't are freed at remove time anyway, so the allocated
>      structure will be gone when the device is removed.
>
>   4) To properly track users, each user that will use a drm_bridge needs
>      to take a reference.

5) Handle the disappearing next_bridge problem: probe() function gets
a pointer to the next bridge, but then for some reasons (e.g. because
of the other device being removed or because of some probe deferral)
the next_bridge driver gets unbdound and the next_bridge becomes
unusable before a call to drm_bridge_attach().

>
> AFAIU, the destroy introduction and the on-purpose omission of kfree in
> remove is to solve 3.
>
> Introducing a function that allocates the drm_bridge container struct
> (like drmm_encoder_alloc for example), take a reference, register a devm
> kfree action, and return the pointer to the driver structure would solve
> that too pretty nicely.
>
> So, something like:
>
>
> struct driver_priv {
>        struct drm_bridge bridge;
>
>        ...
> }
>
> static int driver_probe(...)
> {
>         struct driver_priv *priv;
>         struct drm_bridge *bridge;
>
>         ....
>
>         priv = devm_drm_bridge_alloc(dev, struct driver_priv, bridge);

Ah... And devm-cleanup will just drop a reference to that data,
freeing it when all refs are cleaned? Nice idea.

>         if (IS_ERR(priv))
>            return ERR_PTR(priv);
>         bridge = &priv->bridge;
>
>         ...
>
>         drm_bridge_add(bridge);
> }
>
> Would work just as well.
>
> I also don't think we need explicit (at least for the common case)
> drm_bridge_get and drm_bridge_put calls for bridge users.
> drm_bridge_attach and drm_bridge_detach can get/put the reference
> directly.

As I wrote previously, I think drm_bridge_attach() might be too late for that.
It sounds like drm_of_get_panel_or_bridge() and of_drm_find_bridge
should increment the refcount, possibly adding a devres action to put
the reference.

> And we'll also need some flag in drm_bridge to indicate that the device
> is gone, similar to what drm_dev_enter()/drm_dev_exit() provides,
> because now your bridge driver sticks around for much longer than your
> device so the expectation that your device managed resources (clocks,
> registers, etc.) are always going to be around.

-- 
With best wishes
Dmitry

Re: [PATCH v5 04/10] drm/bridge: add documentation of refcounted bridges

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4890 bytes --]

On Mon, Jan 06, 2025 at 02:24:00PM +0200, Dmitry Baryshkov wrote:
> On Mon, 6 Jan 2025 at 12:39, Maxime Ripard <mripard@kernel.org> wrote:
> >
> > Hi,
> >
> > Most of these comments affect your earlier patches, but let's work on
> > the API-level view.
> >
> > On Tue, Dec 31, 2024 at 11:39:58AM +0100, Luca Ceresoli wrote:
> > > + * When using refcounted mode, the driver should allocate ``struct
> > > + * my_bridge`` using regular allocation (as opposed to ``devm_`` or
> > > + * ``drmm_`` allocation), call drm_bridge_init() immediately afterwards to
> > > + * transfer lifecycle management to the DRM bridge core, and implement a
> > > + * ``.destroy`` function to deallocate the ``struct my_bridge``, as in this
> > > + * example::
> > > + *
> > > + *     static void my_bridge_destroy(struct drm_bridge *bridge)
> > > + *     {
> > > + *         kfree(container_of(bridge, struct my_bridge, bridge));
> > > + *     }
> > > + *
> > > + *     static const struct drm_bridge_funcs my_bridge_funcs = {
> > > + *         .destroy = my_bridge_destroy,
> > > + *         ...
> > > + *     };
> > > + *
> > > + *     static int my_bridge_probe(...)
> > > + *     {
> > > + *         struct my_bridge *mybr;
> > > + *         int err;
> > > + *
> > > + *         mybr = kzalloc(sizeof(*mybr), GFP_KERNEL);
> > > + *         if (!mybr)
> > > + *             return -ENOMEM;
> > > + *
> > > + *         err = drm_bridge_init(dev, &mybr->bridge, &my_bridge_funcs);
> > > + *         if (err)
> > > + *             return err;
> > > + *
> > > + *         ...
> > > + *         drm_bridge_add();
> > > + *         ...
> > > + *     }
> > > + *
> > > + *     static void my_bridge_remove()
> > > + *     {
> > > + *         struct my_bridge *mybr = ...;
> > > + *         drm_bridge_remove(&mybr->bridge);
> > > + *         // ... NO kfree here!
> > > + *     }
> >
> > I'm a bit worried there, since that API is pretty difficult to get
> > right, and we don't have anything to catch bad patterns.
> >
> > Let's take a step back. What we're trying to solve here is:
> >
> >   1) We want to avoid any dangling pointers to a bridge if the bridge
> >      device is removed.
> >
> >   2) To do so, we need to switch to reference counted allocations and
> >      pointers.
> >
> >   3) Most bridges structures are allocated through devm_kzalloc, and they
> >      one that aren't are freed at remove time anyway, so the allocated
> >      structure will be gone when the device is removed.
> >
> >   4) To properly track users, each user that will use a drm_bridge needs
> >      to take a reference.
> 
> 5) Handle the disappearing next_bridge problem: probe() function gets
> a pointer to the next bridge, but then for some reasons (e.g. because
> of the other device being removed or because of some probe deferral)
> the next_bridge driver gets unbdound and the next_bridge becomes
> unusable before a call to drm_bridge_attach().

Oh, right. We need to plumb it in drm_of_find_bridge somehow too.

> > AFAIU, the destroy introduction and the on-purpose omission of kfree in
> > remove is to solve 3.
> >
> > Introducing a function that allocates the drm_bridge container struct
> > (like drmm_encoder_alloc for example), take a reference, register a devm
> > kfree action, and return the pointer to the driver structure would solve
> > that too pretty nicely.
> >
> > So, something like:
> >
> >
> > struct driver_priv {
> >        struct drm_bridge bridge;
> >
> >        ...
> > }
> >
> > static int driver_probe(...)
> > {
> >         struct driver_priv *priv;
> >         struct drm_bridge *bridge;
> >
> >         ....
> >
> >         priv = devm_drm_bridge_alloc(dev, struct driver_priv, bridge);
> 
> Ah... And devm-cleanup will just drop a reference to that data,
> freeing it when all refs are cleaned? Nice idea.

Yup.

> >         if (IS_ERR(priv))
> >            return ERR_PTR(priv);
> >         bridge = &priv->bridge;
> >
> >         ...
> >
> >         drm_bridge_add(bridge);
> > }
> >
> > Would work just as well.
> >
> > I also don't think we need explicit (at least for the common case)
> > drm_bridge_get and drm_bridge_put calls for bridge users.
> > drm_bridge_attach and drm_bridge_detach can get/put the reference
> > directly.
> 
> As I wrote previously, I think drm_bridge_attach() might be too late for that.
> It sounds like drm_of_get_panel_or_bridge() and of_drm_find_bridge
> should increment the refcount, possibly adding a devres action to put
> the reference.

We probably need both. drm_bridge_attach adds the bridge pointer to the
encoder bridge_chain list, so if we had something like

bridge = drm_of_find_bridge();
drm_bridge_attach(encoder, bridge);
drm_bridge_put(bridge);

We could have a dangling pointer.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v5 04/10] drm/bridge: add documentation of refcounted bridges

[Dmitry Baryshkov]

On Mon, Jan 06, 2025 at 03:49:48PM +0100, Maxime Ripard wrote:
> On Mon, Jan 06, 2025 at 02:24:00PM +0200, Dmitry Baryshkov wrote:
> > On Mon, 6 Jan 2025 at 12:39, Maxime Ripard <mripard@kernel.org> wrote:
> > >
> > > Hi,
> > >
> > > Most of these comments affect your earlier patches, but let's work on
> > > the API-level view.
> > >
> > > On Tue, Dec 31, 2024 at 11:39:58AM +0100, Luca Ceresoli wrote:

> > >         if (IS_ERR(priv))
> > >            return ERR_PTR(priv);
> > >         bridge = &priv->bridge;
> > >
> > >         ...
> > >
> > >         drm_bridge_add(bridge);
> > > }
> > >
> > > Would work just as well.
> > >
> > > I also don't think we need explicit (at least for the common case)
> > > drm_bridge_get and drm_bridge_put calls for bridge users.
> > > drm_bridge_attach and drm_bridge_detach can get/put the reference
> > > directly.
> > 
> > As I wrote previously, I think drm_bridge_attach() might be too late for that.
> > It sounds like drm_of_get_panel_or_bridge() and of_drm_find_bridge
> > should increment the refcount, possibly adding a devres action to put
> > the reference.
> 
> We probably need both. drm_bridge_attach adds the bridge pointer to the
> encoder bridge_chain list, so if we had something like
> 
> bridge = drm_of_find_bridge();
> drm_bridge_attach(encoder, bridge);
> drm_bridge_put(bridge);
> 
> We could have a dangling pointer.

Yes... So, both drm_bridge_attach and drm_of_find_bridge() should take
the refcount.

Just as an idea, it might be nice to add refcounting to bridges_show(),
so that we can easily verify that refcounting works correctly.

-- 
With best wishes
Dmitry

Re: [PATCH v5 04/10] drm/bridge: add documentation of refcounted bridges

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1863 bytes --]

On Tue, Jan 07, 2025 at 12:35:15PM +0200, Dmitry Baryshkov wrote:
> On Mon, Jan 06, 2025 at 03:49:48PM +0100, Maxime Ripard wrote:
> > On Mon, Jan 06, 2025 at 02:24:00PM +0200, Dmitry Baryshkov wrote:
> > > On Mon, 6 Jan 2025 at 12:39, Maxime Ripard <mripard@kernel.org> wrote:
> > > >
> > > > Hi,
> > > >
> > > > Most of these comments affect your earlier patches, but let's work on
> > > > the API-level view.
> > > >
> > > > On Tue, Dec 31, 2024 at 11:39:58AM +0100, Luca Ceresoli wrote:
> 
> > > >         if (IS_ERR(priv))
> > > >            return ERR_PTR(priv);
> > > >         bridge = &priv->bridge;
> > > >
> > > >         ...
> > > >
> > > >         drm_bridge_add(bridge);
> > > > }
> > > >
> > > > Would work just as well.
> > > >
> > > > I also don't think we need explicit (at least for the common case)
> > > > drm_bridge_get and drm_bridge_put calls for bridge users.
> > > > drm_bridge_attach and drm_bridge_detach can get/put the reference
> > > > directly.
> > > 
> > > As I wrote previously, I think drm_bridge_attach() might be too late for that.
> > > It sounds like drm_of_get_panel_or_bridge() and of_drm_find_bridge
> > > should increment the refcount, possibly adding a devres action to put
> > > the reference.
> > 
> > We probably need both. drm_bridge_attach adds the bridge pointer to the
> > encoder bridge_chain list, so if we had something like
> > 
> > bridge = drm_of_find_bridge();
> > drm_bridge_attach(encoder, bridge);
> > drm_bridge_put(bridge);
> > 
> > We could have a dangling pointer.
> 
> Yes... So, both drm_bridge_attach and drm_of_find_bridge() should take
> the refcount.
> 
> Just as an idea, it might be nice to add refcounting to bridges_show(),
> so that we can easily verify that refcounting works correctly.

Yep, it looks like a good idea indeed.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v5 04/10] drm/bridge: add documentation of refcounted bridges

[Luca Ceresoli]

Hi Maxime, Dmitry,

thanks both for the useful review!

On Mon, 6 Jan 2025 14:24:00 +0200
Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> On Mon, 6 Jan 2025 at 12:39, Maxime Ripard <mripard@kernel.org> wrote:
> >
> > Hi,
> >
> > Most of these comments affect your earlier patches, but let's work on
> > the API-level view.
> >
> > On Tue, Dec 31, 2024 at 11:39:58AM +0100, Luca Ceresoli wrote:  
> > > + * When using refcounted mode, the driver should allocate ``struct
> > > + * my_bridge`` using regular allocation (as opposed to ``devm_`` or
> > > + * ``drmm_`` allocation), call drm_bridge_init() immediately afterwards to
> > > + * transfer lifecycle management to the DRM bridge core, and implement a
> > > + * ``.destroy`` function to deallocate the ``struct my_bridge``, as in this
> > > + * example::
> > > + *
> > > + *     static void my_bridge_destroy(struct drm_bridge *bridge)
> > > + *     {
> > > + *         kfree(container_of(bridge, struct my_bridge, bridge));
> > > + *     }
> > > + *
> > > + *     static const struct drm_bridge_funcs my_bridge_funcs = {
> > > + *         .destroy = my_bridge_destroy,
> > > + *         ...
> > > + *     };
> > > + *
> > > + *     static int my_bridge_probe(...)
> > > + *     {
> > > + *         struct my_bridge *mybr;
> > > + *         int err;
> > > + *
> > > + *         mybr = kzalloc(sizeof(*mybr), GFP_KERNEL);
> > > + *         if (!mybr)
> > > + *             return -ENOMEM;
> > > + *
> > > + *         err = drm_bridge_init(dev, &mybr->bridge, &my_bridge_funcs);
> > > + *         if (err)
> > > + *             return err;
> > > + *
> > > + *         ...
> > > + *         drm_bridge_add();
> > > + *         ...
> > > + *     }
> > > + *
> > > + *     static void my_bridge_remove()
> > > + *     {
> > > + *         struct my_bridge *mybr = ...;
> > > + *         drm_bridge_remove(&mybr->bridge);
> > > + *         // ... NO kfree here!
> > > + *     }  
> >
> > I'm a bit worried there, since that API is pretty difficult to get
> > right, and we don't have anything to catch bad patterns.
> >
> > Let's take a step back. What we're trying to solve here is:
> >
> >   1) We want to avoid any dangling pointers to a bridge if the bridge
> >      device is removed.
> >
> >   2) To do so, we need to switch to reference counted allocations and
> >      pointers.
> >
> >   3) Most bridges structures are allocated through devm_kzalloc, and they
> >      one that aren't are freed at remove time anyway, so the allocated
> >      structure will be gone when the device is removed.
> >
> >   4) To properly track users, each user that will use a drm_bridge needs
> >      to take a reference.  
> 
> 5) Handle the disappearing next_bridge problem: probe() function gets
> a pointer to the next bridge, but then for some reasons (e.g. because
> of the other device being removed or because of some probe deferral)
> the next_bridge driver gets unbdound and the next_bridge becomes
> unusable before a call to drm_bridge_attach().
> 
> >
> > AFAIU, the destroy introduction and the on-purpose omission of kfree in
> > remove is to solve 3.
> >
> > Introducing a function that allocates the drm_bridge container struct
> > (like drmm_encoder_alloc for example), take a reference, register a devm
> > kfree action, and return the pointer to the driver structure would solve
> > that too pretty nicely.
> >
> > So, something like:
> >
> >
> > struct driver_priv {
> >        struct drm_bridge bridge;
> >
> >        ...
> > }
> >
> > static int driver_probe(...)
> > {
> >         struct driver_priv *priv;
> >         struct drm_bridge *bridge;
> >
> >         ....
> >
> >         priv = devm_drm_bridge_alloc(dev, struct driver_priv, bridge);  
> 
> Ah... And devm-cleanup will just drop a reference to that data,
> freeing it when all refs are cleaned? Nice idea.

I like the idea. It's basically a macro wrapping the calls to kzalloc()
+ drm_bridge_init() that I proposed in this series. I had thought about
such an idea initially but I haven't seen such a macro in
drm_connector.h I didn't follow the idea.

I don't love the _alloc name though because it will be doing much more
than allocating. What about devm_drm_bridge_new()?

I understand _alloc is coherent with the drmm_encoder_alloc() and I
could survive that... but what about renaming that one to
drmm_encoder_new()?

Or maybe _create instead of _new, because _new is used for atomic
states, in opposition to _old.

> > And we'll also need some flag in drm_bridge to indicate that the device
> > is gone, similar to what drm_dev_enter()/drm_dev_exit() provides,
> > because now your bridge driver sticks around for much longer than your
> > device so the expectation that your device managed resources (clocks,
> > registers, etc.) are always going to be around.  

Yes, makes sense too. That should be a drm_bridge_enter/exit(), and
drm_bridge.c will need to be sprinkled with them I guess.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v5 04/10] drm/bridge: add documentation of refcounted bridges

[Luca Ceresoli]

On Tue, 7 Jan 2025 12:35:15 +0200
Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> On Mon, Jan 06, 2025 at 03:49:48PM +0100, Maxime Ripard wrote:
> > On Mon, Jan 06, 2025 at 02:24:00PM +0200, Dmitry Baryshkov wrote:  
> > > On Mon, 6 Jan 2025 at 12:39, Maxime Ripard <mripard@kernel.org> wrote:  
> > > >
> > > > Hi,
> > > >
> > > > Most of these comments affect your earlier patches, but let's work on
> > > > the API-level view.
> > > >
> > > > On Tue, Dec 31, 2024 at 11:39:58AM +0100, Luca Ceresoli wrote:  
> 
> > > >         if (IS_ERR(priv))
> > > >            return ERR_PTR(priv);
> > > >         bridge = &priv->bridge;
> > > >
> > > >         ...
> > > >
> > > >         drm_bridge_add(bridge);
> > > > }
> > > >
> > > > Would work just as well.
> > > >
> > > > I also don't think we need explicit (at least for the common case)
> > > > drm_bridge_get and drm_bridge_put calls for bridge users.
> > > > drm_bridge_attach and drm_bridge_detach can get/put the reference
> > > > directly.  
> > > 
> > > As I wrote previously, I think drm_bridge_attach() might be too late for that.
> > > It sounds like drm_of_get_panel_or_bridge() and of_drm_find_bridge
> > > should increment the refcount, possibly adding a devres action to put
> > > the reference.  
> > 
> > We probably need both. drm_bridge_attach adds the bridge pointer to the
> > encoder bridge_chain list, so if we had something like
> > 
> > bridge = drm_of_find_bridge();
> > drm_bridge_attach(encoder, bridge);
> > drm_bridge_put(bridge);
> > 
> > We could have a dangling pointer.  
> 
> Yes... So, both drm_bridge_attach and drm_of_find_bridge() should take
> the refcount.

Exactly, just like drm_bridge_add/remove() do.

> Just as an idea, it might be nice to add refcounting to bridges_show(),
> so that we can easily verify that refcounting works correctly.

Good point, cheap and useful, adding that too!

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v5 04/10] drm/bridge: add documentation of refcounted bridges

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 5701 bytes --]

On Wed, Jan 08, 2025 at 04:24:29PM +0100, Luca Ceresoli wrote:
> Hi Maxime, Dmitry,
> 
> thanks both for the useful review!
> 
> On Mon, 6 Jan 2025 14:24:00 +0200
> Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> 
> > On Mon, 6 Jan 2025 at 12:39, Maxime Ripard <mripard@kernel.org> wrote:
> > >
> > > Hi,
> > >
> > > Most of these comments affect your earlier patches, but let's work on
> > > the API-level view.
> > >
> > > On Tue, Dec 31, 2024 at 11:39:58AM +0100, Luca Ceresoli wrote:  
> > > > + * When using refcounted mode, the driver should allocate ``struct
> > > > + * my_bridge`` using regular allocation (as opposed to ``devm_`` or
> > > > + * ``drmm_`` allocation), call drm_bridge_init() immediately afterwards to
> > > > + * transfer lifecycle management to the DRM bridge core, and implement a
> > > > + * ``.destroy`` function to deallocate the ``struct my_bridge``, as in this
> > > > + * example::
> > > > + *
> > > > + *     static void my_bridge_destroy(struct drm_bridge *bridge)
> > > > + *     {
> > > > + *         kfree(container_of(bridge, struct my_bridge, bridge));
> > > > + *     }
> > > > + *
> > > > + *     static const struct drm_bridge_funcs my_bridge_funcs = {
> > > > + *         .destroy = my_bridge_destroy,
> > > > + *         ...
> > > > + *     };
> > > > + *
> > > > + *     static int my_bridge_probe(...)
> > > > + *     {
> > > > + *         struct my_bridge *mybr;
> > > > + *         int err;
> > > > + *
> > > > + *         mybr = kzalloc(sizeof(*mybr), GFP_KERNEL);
> > > > + *         if (!mybr)
> > > > + *             return -ENOMEM;
> > > > + *
> > > > + *         err = drm_bridge_init(dev, &mybr->bridge, &my_bridge_funcs);
> > > > + *         if (err)
> > > > + *             return err;
> > > > + *
> > > > + *         ...
> > > > + *         drm_bridge_add();
> > > > + *         ...
> > > > + *     }
> > > > + *
> > > > + *     static void my_bridge_remove()
> > > > + *     {
> > > > + *         struct my_bridge *mybr = ...;
> > > > + *         drm_bridge_remove(&mybr->bridge);
> > > > + *         // ... NO kfree here!
> > > > + *     }  
> > >
> > > I'm a bit worried there, since that API is pretty difficult to get
> > > right, and we don't have anything to catch bad patterns.
> > >
> > > Let's take a step back. What we're trying to solve here is:
> > >
> > >   1) We want to avoid any dangling pointers to a bridge if the bridge
> > >      device is removed.
> > >
> > >   2) To do so, we need to switch to reference counted allocations and
> > >      pointers.
> > >
> > >   3) Most bridges structures are allocated through devm_kzalloc, and they
> > >      one that aren't are freed at remove time anyway, so the allocated
> > >      structure will be gone when the device is removed.
> > >
> > >   4) To properly track users, each user that will use a drm_bridge needs
> > >      to take a reference.  
> > 
> > 5) Handle the disappearing next_bridge problem: probe() function gets
> > a pointer to the next bridge, but then for some reasons (e.g. because
> > of the other device being removed or because of some probe deferral)
> > the next_bridge driver gets unbdound and the next_bridge becomes
> > unusable before a call to drm_bridge_attach().
> > 
> > >
> > > AFAIU, the destroy introduction and the on-purpose omission of kfree in
> > > remove is to solve 3.
> > >
> > > Introducing a function that allocates the drm_bridge container struct
> > > (like drmm_encoder_alloc for example), take a reference, register a devm
> > > kfree action, and return the pointer to the driver structure would solve
> > > that too pretty nicely.
> > >
> > > So, something like:
> > >
> > >
> > > struct driver_priv {
> > >        struct drm_bridge bridge;
> > >
> > >        ...
> > > }
> > >
> > > static int driver_probe(...)
> > > {
> > >         struct driver_priv *priv;
> > >         struct drm_bridge *bridge;
> > >
> > >         ....
> > >
> > >         priv = devm_drm_bridge_alloc(dev, struct driver_priv, bridge);  
> > 
> > Ah... And devm-cleanup will just drop a reference to that data,
> > freeing it when all refs are cleaned? Nice idea.
> 
> I like the idea. It's basically a macro wrapping the calls to kzalloc()
> + drm_bridge_init() that I proposed in this series. I had thought about
> such an idea initially but I haven't seen such a macro in
> drm_connector.h I didn't follow the idea.
> 
> I don't love the _alloc name though because it will be doing much more
> than allocating. What about devm_drm_bridge_new()?
>
> I understand _alloc is coherent with the drmm_encoder_alloc() and I
> could survive that... but what about renaming that one to
> drmm_encoder_new()?

alloc is used pretty much every where for allocation + init, see CRTC,
planes, connectors, etc.

It might be unfortunate, but I don't think we should change that
convention.

> Or maybe _create instead of _new, because _new is used for atomic
> states, in opposition to _old.
> 
> > > And we'll also need some flag in drm_bridge to indicate that the device
> > > is gone, similar to what drm_dev_enter()/drm_dev_exit() provides,
> > > because now your bridge driver sticks around for much longer than your
> > > device so the expectation that your device managed resources (clocks,
> > > registers, etc.) are always going to be around.  
> 
> Yes, makes sense too. That should be a drm_bridge_enter/exit(), and
> drm_bridge.c will need to be sprinkled with them I guess.

The users would be the drivers, most likely. There's not much we can do
at the framework level, unfortunately.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v5 08/10] drm/bridge: samsung-dsim: use supporting variable for out_bridge

[Luca Ceresoli]

Hi Dmitry,

On Thu, 2 Jan 2025 13:01:49 +0100
Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:

> > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > index f8b4fb8357659018ec0db65374ee5d05330639ae..c4d1563fd32019efde523dfc0863be044c05a826 100644
> > > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > @@ -1705,6 +1705,7 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
> > >  	struct device *dev = dsi->dev;
> > >  	struct device_node *np = dev->of_node;
> > >  	struct device_node *remote;
> > > +	struct drm_bridge *out_bridge;
> > >  	struct drm_panel *panel;
> > >  	int ret;
> > >  
> > > @@ -1740,21 +1741,23 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
> > >  
> > >  	panel = of_drm_find_panel(remote);
> > >  	if (!IS_ERR(panel)) {
> > > -		dsi->out_bridge = devm_drm_panel_bridge_add(dev, panel);
> > > +		out_bridge = devm_drm_panel_bridge_add(dev, panel);
> > >  	} else {
> > > -		dsi->out_bridge = of_drm_find_bridge(remote);
> > > -		if (!dsi->out_bridge)
> > > -			dsi->out_bridge = ERR_PTR(-EINVAL);
> > > +		out_bridge = of_drm_find_bridge(remote);
> > > +		if (!out_bridge)
> > > +			out_bridge = ERR_PTR(-EINVAL);
> > >  	}    
> > 
> > While looking at this patch, I think we should migrate the driver to
> > drm_of_find_panel_or_bridge().  
> 
> Indeed, the code here is duplicating drm_of_find_panel_or_bridge(). I'm
> going to convert it.

Or maybe not. A similar work has been attempted in the past [0] and
then reverted. There are many subtleties one would need to take care of
before getting this right, I don't think opening this other can of
worms in the middle of the bridge refcounting work makes sense.

[0] https://patchwork.freedesktop.org/patch/482751/

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v5 08/10] drm/bridge: samsung-dsim: use supporting variable for out_bridge

[Luca Ceresoli]

Hello Dmitry, Maxime, All,

On Fri, 10 Jan 2025 11:58:19 +0100
Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:

> Hi Dmitry,
> 
> On Thu, 2 Jan 2025 13:01:49 +0100
> Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> 
> > > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > index f8b4fb8357659018ec0db65374ee5d05330639ae..c4d1563fd32019efde523dfc0863be044c05a826 100644
> > > > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > @@ -1705,6 +1705,7 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
> > > >  	struct device *dev = dsi->dev;
> > > >  	struct device_node *np = dev->of_node;
> > > >  	struct device_node *remote;
> > > > +	struct drm_bridge *out_bridge;
> > > >  	struct drm_panel *panel;
> > > >  	int ret;
> > > >  
> > > > @@ -1740,21 +1741,23 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
> > > >  
> > > >  	panel = of_drm_find_panel(remote);
> > > >  	if (!IS_ERR(panel)) {
> > > > -		dsi->out_bridge = devm_drm_panel_bridge_add(dev, panel);
> > > > +		out_bridge = devm_drm_panel_bridge_add(dev, panel);
> > > >  	} else {
> > > > -		dsi->out_bridge = of_drm_find_bridge(remote);
> > > > -		if (!dsi->out_bridge)
> > > > -			dsi->out_bridge = ERR_PTR(-EINVAL);
> > > > +		out_bridge = of_drm_find_bridge(remote);
> > > > +		if (!out_bridge)
> > > > +			out_bridge = ERR_PTR(-EINVAL);
> > > >  	}      
> > > 
> > > While looking at this patch, I think we should migrate the driver to
> > > drm_of_find_panel_or_bridge().    
> > 
> > Indeed, the code here is duplicating drm_of_find_panel_or_bridge(). I'm
> > going to convert it.  

I've been struggling to find a good way to handle the panel bridge
lifetime, and still haven't found a way that looks totally good.
Here's my analysis and some possible ways forward.

For "normal" bridges there is a device driver that probes, allocates a
struct drm_bridge and registers it via drm_bridge_add() and at that
point the bridge can be found by other drivers, such as the previous
bridge or the encoder. Those "other drivers" will obtain a pointer to
the struct drm_bridge and with refcounting they need to
drm_bridge_put() it.

So there are two clearly separate roles: the provider (bridge driver)
and the consumers (which gets/puts a pointer). So far so good.

And there are panels, which probe similarly as far as I can see.

And then there is the panel bridge. My understanding (which I'd love to
get clarified in case it is not accurate) is that DRM bridges expect to
always interact with "the next bridge", which cannot work for the last
bridge of course, and so the panel bridge wraps the panel pretending it
is a bridge.

This software structure is clearly not accurately modeling the
hardware (panel is not bridge), but it has been serving us so far.
However now I'm definitely hitting some troubles due to how the panel
bridge is created.

First: when the panel probes, no panel bridge is created. So other
bridges cannot find it as they would find other bridges, using
of_drm_find_bridge().

Second: to circumvent this, we have {drmm,devm_drm}_of_get_bridge()
which do (not counting error cases):

 1. call drm_of_find_panel_or_bridge() which returns:
    - a panel pointer, if found
    - otherwise a bridge pointer, if one already exists
 2. if a panel was found, it is assumed that no bridge exists yet (*)
    so one is created:
    2.1) call {drmm,devm_drm}_panel_bridge_add: a new panel bridge is
         allocated and its pointer returned
 3. the bridge obtained at 1 or 2 is returned

So, the pointer returned by {drmm,devm_drm}_of_get_bridge() can be a)
pre-existing or b) a panel bridge allocated automagically if there is a
panel. However the caller has no way to know whether a) or b) happened.
Yet a) and b) have different implications for the panel bridge lifetime
management and require that the returned pointer is disposed of in a
different way.

The fundamental design choice that is problematic with respect to
hotplugging is that the panel bridge (which is a struct drm_bridge
after all) is not created by the provider (the panel driver) but on the
fly by the first consumer that happens to need it. And the consumer is
not aware of this: it obtains a struct drm_bridge pointer and doesn't
know whether it was a) pre-existing or b) created on the fly.

So far this approach has been working because devm and drmm ensure the
panel bridge would be dealloacted at some point. However the devm and drmm
release actions are associated to the consumer struct device (the panel
bridge consumer), so if the panel bridge is removed and the consumer is
not, deallocation won't happen.

For hotplugging we cannot use drmm and devm and instead we use get/put,
to let the "next bridge" disappear with the previous one still present.
So the trivial idea is to add a drm_of_get_bridge(), similar to
{drmm,devm_drm}_of_get_bridge() except it uses plain
drm_panel_bridge_add() instead of devm/drmm variants. But then the
caller (which is the panel consumer) will have to dispose of the struct
drm_bridge pointer by calling:

 - drm_bridge_put() in case a)
 - drm_panel_bridge_remove in case b)

And that's the problem I need to solve.


First and foremost, do you think my analysis is correct?


(*) superficially this looks like another fundamental issue to me, but
    it is not my focus at the moment


Assuming it is, here are some possible ways to make the panel-bridge
work with hotplug.

 1. have drm_of_get_bridge() return an indication on how to dispose of
    the returned pointer
 2. add an ad-hoc remover function alongside drm_of_get_bridge()
 3. let all panel drivers automatically add a panel-bridge
 4. stop pretending there is always a "next bridge" after each bridge

Idea 1:

The new (non drmm/devm) drm_of_get_bridge() would return a flag to
indicate whether case a) or b) happened. Or it could return a function
pointer to be called to dispose of the returned pointer, to be
stored and called by the consumer.

I find this quite ugly and I'd call this a workaround rather than a
solution, but I'm open to discussion.

Idea 2:

I'm proposing to add drm_of_get_bridge(), which as a non-drmm, non-devm
variant to be used with refcounting. So the idea is to add alongside it
a corresponding removal function [drm_of_put_bridge()?]:

  drm_of_put_bridge(struct drm_bridge *bridge)
  {
      if (drm_bridge_is_panel(bridge))
          drm_panel_bridge_remove(bridge);
      drm_bridge_put(bridge);
  }

So the consumer would always have to call this function, which is as
automagic as *drm_of_get_bridge().

My concern is what would happen in case:

 * driver A calls drm_of_get_bridge() and a panel_bridge is created
 * driver B calls drm_of_get_bridge() on the same panel, the existing
   panel_bridge is returned

Both drivers would call drm_of_put_bridge -> drm_panel_bridge_remove,
so removing twice. However I don't think this is possible due to how the
*_drm_of_get_bridge() functions are currently implemented.

Even more, I don't think it is realistic that two different drivers call
*_drm_of_get_bridge() for the same panel. Is this assumption correct?

Idea 3: 

The idea is that if the panel driver framework always creates a panel
bridge, it will never need to be created on the fly automagically by
its consumers, so the whole problem would disappear. It also would be
better modeling the hardware: still wrapping a panel with a drm_bridge
that does not exist in the hardware, but at least having it created by
the provider driver and not by the consumer driver which happens to
look for it.

This looks like a promising and simple idea, so I tried a quick
implementation:

 void drm_panel_init(struct drm_panel *panel, struct device *dev,
                    const struct drm_panel_funcs *funcs, int connector_type)
 {
+       struct drm_bridge *bridge;
+
        INIT_LIST_HEAD(&panel->list);
        INIT_LIST_HEAD(&panel->followers);
        mutex_init(&panel->follower_lock);
        panel->dev = dev;
        panel->funcs = funcs;
        panel->connector_type = connector_type;
+
+       bridge = devm_drm_panel_bridge_add(panel->dev, panel);
+       WARN_ON(!bridge);
 }

This is somewhat working but it requires more work because:

 * as it is, it creates a circular dependency between drm_panel and the
   panel bridge, and modular builds will fail:

     depmod: ERROR: Cycle detected: drm -> drm_kms_helper -> drm

 * The panel bridge implementation should be made private to the panel
   driver only (possibly helping to solve the previous issue?)

 * Many drivers currently call devm_drm_panel_bridge_add() directly,
   they should probably call drm_of_get_bridge instead

 * drm_of_find_panel_or_bridge() should disappear: other drivers would
   just look for a bridge

Opinions about this idea?

Idea 4:

'stop pretending there is always a "next bridge" after each bridge'
looks like a _very_ long term goal, but it would be interesting to
discuss whether this is a correct idea.

If you've been reading thus far, thanks for your patience! I'll be very
glad to hear more opinions on all the above.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v5 08/10] drm/bridge: samsung-dsim: use supporting variable for out_bridge

[Dmitry Baryshkov]

On Thu, Jan 16, 2025 at 11:32:36AM +0100, Luca Ceresoli wrote:
> Hello Dmitry, Maxime, All,
> 
> On Fri, 10 Jan 2025 11:58:19 +0100
> Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> 
> > Hi Dmitry,
> > 
> > On Thu, 2 Jan 2025 13:01:49 +0100
> > Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> > 
> > > > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > > index f8b4fb8357659018ec0db65374ee5d05330639ae..c4d1563fd32019efde523dfc0863be044c05a826 100644
> > > > > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > > @@ -1705,6 +1705,7 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
> > > > >  	struct device *dev = dsi->dev;
> > > > >  	struct device_node *np = dev->of_node;
> > > > >  	struct device_node *remote;
> > > > > +	struct drm_bridge *out_bridge;
> > > > >  	struct drm_panel *panel;
> > > > >  	int ret;
> > > > >  
> > > > > @@ -1740,21 +1741,23 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
> > > > >  
> > > > >  	panel = of_drm_find_panel(remote);
> > > > >  	if (!IS_ERR(panel)) {
> > > > > -		dsi->out_bridge = devm_drm_panel_bridge_add(dev, panel);
> > > > > +		out_bridge = devm_drm_panel_bridge_add(dev, panel);
> > > > >  	} else {
> > > > > -		dsi->out_bridge = of_drm_find_bridge(remote);
> > > > > -		if (!dsi->out_bridge)
> > > > > -			dsi->out_bridge = ERR_PTR(-EINVAL);
> > > > > +		out_bridge = of_drm_find_bridge(remote);
> > > > > +		if (!out_bridge)
> > > > > +			out_bridge = ERR_PTR(-EINVAL);
> > > > >  	}      
> > > > 
> > > > While looking at this patch, I think we should migrate the driver to
> > > > drm_of_find_panel_or_bridge().    
> > > 
> > > Indeed, the code here is duplicating drm_of_find_panel_or_bridge(). I'm
> > > going to convert it.  
> 
> I've been struggling to find a good way to handle the panel bridge
> lifetime, and still haven't found a way that looks totally good.
> Here's my analysis and some possible ways forward.
> 
> For "normal" bridges there is a device driver that probes, allocates a
> struct drm_bridge and registers it via drm_bridge_add() and at that
> point the bridge can be found by other drivers, such as the previous
> bridge or the encoder. Those "other drivers" will obtain a pointer to
> the struct drm_bridge and with refcounting they need to
> drm_bridge_put() it.
> 
> So there are two clearly separate roles: the provider (bridge driver)
> and the consumers (which gets/puts a pointer). So far so good.
> 
> And there are panels, which probe similarly as far as I can see.
> 
> And then there is the panel bridge. My understanding (which I'd love to
> get clarified in case it is not accurate) is that DRM bridges expect to
> always interact with "the next bridge", which cannot work for the last
> bridge of course, and so the panel bridge wraps the panel pretending it
> is a bridge.

Well.. There are some drivers that interact with the drm_panel directly.
However I think the tendency was to migrate to always having the bridge
instead. I.e. the only reason to interact with the panel directly seems
to be able to access panel timings instead of modes.

[skipeed 1 & 2]

> 
> Idea 3: 
> 
> The idea is that if the panel driver framework always creates a panel
> bridge, it will never need to be created on the fly automagically by
> its consumers, so the whole problem would disappear. It also would be
> better modeling the hardware: still wrapping a panel with a drm_bridge
> that does not exist in the hardware, but at least having it created by
> the provider driver and not by the consumer driver which happens to
> look for it.

I think this is the best option up to now.

> 
> This looks like a promising and simple idea, so I tried a quick
> implementation:
> 
>  void drm_panel_init(struct drm_panel *panel, struct device *dev,
>                     const struct drm_panel_funcs *funcs, int connector_type)
>  {
> +       struct drm_bridge *bridge;
> +
>         INIT_LIST_HEAD(&panel->list);
>         INIT_LIST_HEAD(&panel->followers);
>         mutex_init(&panel->follower_lock);
>         panel->dev = dev;
>         panel->funcs = funcs;
>         panel->connector_type = connector_type;
> +
> +       bridge = devm_drm_panel_bridge_add(panel->dev, panel);
> +       WARN_ON(!bridge);
>  }
> 
> This is somewhat working but it requires more work because:
> 
>  * as it is, it creates a circular dependency between drm_panel and the
>    panel bridge, and modular builds will fail:
> 
>      depmod: ERROR: Cycle detected: drm -> drm_kms_helper -> drm
> 
>  * The panel bridge implementation should be made private to the panel
>    driver only (possibly helping to solve the previous issue?)

Or merge drm_panel.c into bridge/panel.c. The panel bridge already
exports non-standard API, so it should be fine to extend / change that
API. Likewise we might move drm_panel.c to drm_kms_helper.o, also
resolving the loop. There will be a need for a Kconfig update in both
cases.

>  * Many drivers currently call devm_drm_panel_bridge_add() directly,
>    they should probably call drm_of_get_bridge instead

I'd say, make it a stub that calls drm_of_get_bridge() with a possible
deprecation warning.

> 
>  * drm_of_find_panel_or_bridge() should disappear: other drivers would
>    just look for a bridge

Please keep it for now.

Some of the drivers think that it returns literally panel-or-bridge (but
not both). However if panel bridge is already created, it will return
both. So those drivers need to be updated.

But in a longer term indeed  it should fade away.

> 
> Opinions about this idea?
> 
> Idea 4:
> 
> 'stop pretending there is always a "next bridge" after each bridge'
> looks like a _very_ long term goal, but it would be interesting to
> discuss whether this is a correct idea.
> 
> If you've been reading thus far, thanks for your patience! I'll be very
> glad to hear more opinions on all the above.
> 
> Luca
> 
> -- 
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

-- 
With best wishes
Dmitry

Re: [PATCH v5 08/10] drm/bridge: samsung-dsim: use supporting variable for out_bridge

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 11921 bytes --]

Hi Luca,

On Thu, Jan 16, 2025 at 11:32:36AM +0100, Luca Ceresoli wrote:
> Hello Dmitry, Maxime, All,
> 
> On Fri, 10 Jan 2025 11:58:19 +0100
> Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> 
> > Hi Dmitry,
> > 
> > On Thu, 2 Jan 2025 13:01:49 +0100
> > Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> > 
> > > > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > > index f8b4fb8357659018ec0db65374ee5d05330639ae..c4d1563fd32019efde523dfc0863be044c05a826 100644
> > > > > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > > > > @@ -1705,6 +1705,7 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
> > > > >  	struct device *dev = dsi->dev;
> > > > >  	struct device_node *np = dev->of_node;
> > > > >  	struct device_node *remote;
> > > > > +	struct drm_bridge *out_bridge;
> > > > >  	struct drm_panel *panel;
> > > > >  	int ret;
> > > > >  
> > > > > @@ -1740,21 +1741,23 @@ static int samsung_dsim_host_attach(struct mipi_dsi_host *host,
> > > > >  
> > > > >  	panel = of_drm_find_panel(remote);
> > > > >  	if (!IS_ERR(panel)) {
> > > > > -		dsi->out_bridge = devm_drm_panel_bridge_add(dev, panel);
> > > > > +		out_bridge = devm_drm_panel_bridge_add(dev, panel);
> > > > >  	} else {
> > > > > -		dsi->out_bridge = of_drm_find_bridge(remote);
> > > > > -		if (!dsi->out_bridge)
> > > > > -			dsi->out_bridge = ERR_PTR(-EINVAL);
> > > > > +		out_bridge = of_drm_find_bridge(remote);
> > > > > +		if (!out_bridge)
> > > > > +			out_bridge = ERR_PTR(-EINVAL);
> > > > >  	}      
> > > > 
> > > > While looking at this patch, I think we should migrate the driver to
> > > > drm_of_find_panel_or_bridge().    
> > > 
> > > Indeed, the code here is duplicating drm_of_find_panel_or_bridge(). I'm
> > > going to convert it.  
> 
> I've been struggling to find a good way to handle the panel bridge
> lifetime, and still haven't found a way that looks totally good.
> Here's my analysis and some possible ways forward.
> 
> For "normal" bridges there is a device driver that probes, allocates a
> struct drm_bridge and registers it via drm_bridge_add() and at that
> point the bridge can be found by other drivers, such as the previous
> bridge or the encoder. Those "other drivers" will obtain a pointer to
> the struct drm_bridge and with refcounting they need to
> drm_bridge_put() it.
> 
> So there are two clearly separate roles: the provider (bridge driver)
> and the consumers (which gets/puts a pointer). So far so good.

Yeah, we agree so far :)

> And there are panels, which probe similarly as far as I can see.

Indeed.

> And then there is the panel bridge. My understanding (which I'd love to
> get clarified in case it is not accurate) is that DRM bridges expect to
> always interact with "the next bridge", which cannot work for the last
> bridge of course, and so the panel bridge wraps the panel pretending it
> is a bridge.
> 
> This software structure is clearly not accurately modeling the
> hardware (panel is not bridge),

We don't have a proper definition of what a bridge is, so as far as I'm
concerned, everything is a bridge :)

The name came from "external signal converters", but the API got reused
to support so many hardware components it's not meaningful anymore.

> but it has been serving us so far. However now I'm definitely hitting
> some troubles due to how the panel bridge is created.
> 
> First: when the panel probes, no panel bridge is created. So other
> bridges cannot find it as they would find other bridges, using
> of_drm_find_bridge().
> 
> Second: to circumvent this, we have {drmm,devm_drm}_of_get_bridge()
> which do (not counting error cases):
> 
>  1. call drm_of_find_panel_or_bridge() which returns:
>     - a panel pointer, if found
>     - otherwise a bridge pointer, if one already exists
>  2. if a panel was found, it is assumed that no bridge exists yet (*)
>     so one is created:
>     2.1) call {drmm,devm_drm}_panel_bridge_add: a new panel bridge is
>          allocated and its pointer returned
>  3. the bridge obtained at 1 or 2 is returned
> 
> So, the pointer returned by {drmm,devm_drm}_of_get_bridge() can be a)
> pre-existing or b) a panel bridge allocated automagically if there is a
> panel. However the caller has no way to know whether a) or b) happened.
> Yet a) and b) have different implications for the panel bridge lifetime
> management and require that the returned pointer is disposed of in a
> different way.
> 
> The fundamental design choice that is problematic with respect to
> hotplugging is that the panel bridge (which is a struct drm_bridge
> after all) is not created by the provider (the panel driver) but on the
> fly by the first consumer that happens to need it. And the consumer is
> not aware of this: it obtains a struct drm_bridge pointer and doesn't
> know whether it was a) pre-existing or b) created on the fly.

I'm not entirely sure why it matters? The only thing the consumer should
care about is that the bridge pointer it got is valid between the calls
to drmm_of_get_bridge() and drm_bridge_put(). As long as that statement
is true, why should we care about what or how that bridge was created?

> So far this approach has been working because devm and drmm ensure the
> panel bridge would be dealloacted at some point. However the devm and drmm
> release actions are associated to the consumer struct device (the panel
> bridge consumer), so if the panel bridge is removed and the consumer is
> not, deallocation won't happen.

Oh, right, if one doesn't call drm_bridge_put(), that will result in a
memory leak. The general topic we discuss and try to address here is
memory safety, and a memory leak is considered safe. It's also going to
get allocated only a couple of times anyway, so it's not a *huge*
concern.

And about how to actually fix it, there's two ways to go about it:

  * Either we do a coccinelle script and try to put all those
    drm_bridge_put() everywhere;

  * Or we create a devm/drmm action and drop the reference
    automatically.

The latter is obviously less intrusive, we would need to deprecate
devm_of_get_bridge() for it to be safe, and I'm not entirely sure it
would be enough, but it might just work.

> For hotplugging we cannot use drmm and devm and instead we use get/put,
> to let the "next bridge" disappear with the previous one still present.
> So the trivial idea is to add a drm_of_get_bridge(), similar to
> {drmm,devm_drm}_of_get_bridge() except it uses plain
> drm_panel_bridge_add() instead of devm/drmm variants. But then the
> caller (which is the panel consumer) will have to dispose of the struct
> drm_bridge pointer by calling:
> 
>  - drm_bridge_put() in case a)
>  - drm_panel_bridge_remove in case b)
> 
> And that's the problem I need to solve.

I'm not sure the problem is limited to panel_bridge. Your question is
essentially: how do I make sure a driver-specific init is properly freed
at drm_bridge_put time. This was done so far mostly at bridge remove
time, but we obviously can't do that anymore.

But we'd have the same issue if, say, we needed to remove a workqueue
from a driver.

I think we need a destroy() hook for bridges, just like we have for
connectors for example that would deal with calling
drm_panel_bridge_remove() if necessary, or any other driver-specific
sequence.

> First and foremost, do you think my analysis is correct?
>
> (*) superficially this looks like another fundamental issue to me, but
>     it is not my focus at the moment
>
> Assuming it is, here are some possible ways to make the panel-bridge
> work with hotplug.
> 
>  1. have drm_of_get_bridge() return an indication on how to dispose of
>     the returned pointer
>  2. add an ad-hoc remover function alongside drm_of_get_bridge()
>  3. let all panel drivers automatically add a panel-bridge
>  4. stop pretending there is always a "next bridge" after each bridge
> 
> Idea 1:
> 
> The new (non drmm/devm) drm_of_get_bridge() would return a flag to
> indicate whether case a) or b) happened. Or it could return a function
> pointer to be called to dispose of the returned pointer, to be
> stored and called by the consumer.
> 
> I find this quite ugly and I'd call this a workaround rather than a
> solution, but I'm open to discussion.
> 
> Idea 2:
> 
> I'm proposing to add drm_of_get_bridge(), which as a non-drmm, non-devm
> variant to be used with refcounting. So the idea is to add alongside it
> a corresponding removal function [drm_of_put_bridge()?]:
> 
>   drm_of_put_bridge(struct drm_bridge *bridge)
>   {
>       if (drm_bridge_is_panel(bridge))
>           drm_panel_bridge_remove(bridge);
>       drm_bridge_put(bridge);
>   }
> 
> So the consumer would always have to call this function, which is as
> automagic as *drm_of_get_bridge().
> 
> My concern is what would happen in case:
> 
>  * driver A calls drm_of_get_bridge() and a panel_bridge is created
>  * driver B calls drm_of_get_bridge() on the same panel, the existing
>    panel_bridge is returned
> 
> Both drivers would call drm_of_put_bridge -> drm_panel_bridge_remove,
> so removing twice. However I don't think this is possible due to how the
> *_drm_of_get_bridge() functions are currently implemented.
> 
> Even more, I don't think it is realistic that two different drivers call
> *_drm_of_get_bridge() for the same panel. Is this assumption correct?
> 
> Idea 3: 
> 
> The idea is that if the panel driver framework always creates a panel
> bridge, it will never need to be created on the fly automagically by
> its consumers, so the whole problem would disappear. It also would be
> better modeling the hardware: still wrapping a panel with a drm_bridge
> that does not exist in the hardware, but at least having it created by
> the provider driver and not by the consumer driver which happens to
> look for it.
> 
> This looks like a promising and simple idea, so I tried a quick
> implementation:
> 
>  void drm_panel_init(struct drm_panel *panel, struct device *dev,
>                     const struct drm_panel_funcs *funcs, int connector_type)
>  {
> +       struct drm_bridge *bridge;
> +
>         INIT_LIST_HEAD(&panel->list);
>         INIT_LIST_HEAD(&panel->followers);
>         mutex_init(&panel->follower_lock);
>         panel->dev = dev;
>         panel->funcs = funcs;
>         panel->connector_type = connector_type;
> +
> +       bridge = devm_drm_panel_bridge_add(panel->dev, panel);
> +       WARN_ON(!bridge);
>  }
>
> This is somewhat working but it requires more work because:
> 
>  * as it is, it creates a circular dependency between drm_panel and the
>    panel bridge, and modular builds will fail:
> 
>      depmod: ERROR: Cycle detected: drm -> drm_kms_helper -> drm
> 
>  * The panel bridge implementation should be made private to the panel
>    driver only (possibly helping to solve the previous issue?)
> 
>  * Many drivers currently call devm_drm_panel_bridge_add() directly,
>    they should probably call drm_of_get_bridge instead
> 
>  * drm_of_find_panel_or_bridge() should disappear: other drivers would
>    just look for a bridge
> 
> Opinions about this idea?
> 
> Idea 4:
> 
> 'stop pretending there is always a "next bridge" after each bridge'
> looks like a _very_ long term goal, but it would be interesting to
> discuss whether this is a correct idea.
> 
> If you've been reading thus far, thanks for your patience! I'll be very
> glad to hear more opinions on all the above.

I don't think we need any of them. Adding a mechanism similar to
drm_connector_free() seems to be what you're looking for.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v5 08/10] drm/bridge: samsung-dsim: use supporting variable for out_bridge

[Luca Ceresoli]

Hi Dmitry,

On Thu, 16 Jan 2025 12:56:25 +0200
Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

[...]

> > Idea 3: 
> > 
> > The idea is that if the panel driver framework always creates a panel
> > bridge, it will never need to be created on the fly automagically by
> > its consumers, so the whole problem would disappear. It also would be
> > better modeling the hardware: still wrapping a panel with a drm_bridge
> > that does not exist in the hardware, but at least having it created by
> > the provider driver and not by the consumer driver which happens to
> > look for it.  
> 
> I think this is the best option up to now.

Thanks for sharing your opinion. However a few points are not clear to
me, see below.

> > This looks like a promising and simple idea, so I tried a quick
> > implementation:
> > 
> >  void drm_panel_init(struct drm_panel *panel, struct device *dev,
> >                     const struct drm_panel_funcs *funcs, int connector_type)
> >  {
> > +       struct drm_bridge *bridge;
> > +
> >         INIT_LIST_HEAD(&panel->list);
> >         INIT_LIST_HEAD(&panel->followers);
> >         mutex_init(&panel->follower_lock);
> >         panel->dev = dev;
> >         panel->funcs = funcs;
> >         panel->connector_type = connector_type;
> > +
> > +       bridge = devm_drm_panel_bridge_add(panel->dev, panel);
> > +       WARN_ON(!bridge);
> >  }
> > 
> > This is somewhat working but it requires more work because:
> > 
> >  * as it is, it creates a circular dependency between drm_panel and the
> >    panel bridge, and modular builds will fail:
> > 
> >      depmod: ERROR: Cycle detected: drm -> drm_kms_helper -> drm
> > 
> >  * The panel bridge implementation should be made private to the panel
> >    driver only (possibly helping to solve the previous issue?)  
> 
> Or merge drm_panel.c into bridge/panel.c.

Not sure here you mean exactly what you wrote, or the other way around.
It looks more correct to me that the panel core (drm_panel.c) starts
exposing a bridge now, and not that the panel bridge which is just one
of many bridge drivers starts handling all the panel-related stuff.

> The panel bridge already
> exports non-standard API, so it should be fine to extend / change that
> API. Likewise we might move drm_panel.c to drm_kms_helper.o, also
> resolving the loop.

Again I'm not following I'm afraid. It would seem more logical to me to
move things from the helper into drm.o as they become more necessary
for common cases, rather than moving things out to a helper module and
then force all panel drivers to depend on the helper module.

Apologies, I'm perhaps not following your reasoning here. :(

> There will be a need for a Kconfig update in both
> cases.
> 
> >  * Many drivers currently call devm_drm_panel_bridge_add() directly,
> >    they should probably call drm_of_get_bridge instead  
> 
> I'd say, make it a stub that calls drm_of_get_bridge() with a possible
> deprecation warning.

I get the idea, but it would need to be implemented differently because
drm_of_get_bridge() calls devm_drm_panel_bridge_add(). They would loop
into each other. I think we might simply add a check at the beginning
of drm_panel_bridge_add_typed(), as in (pseudocode):

drm_panel_bridge_add_typed(struct drm_panel *panel, ...)
{
    if (panel_has_a_panel_bridge(panel))
        return panel->panel_bridge.bridge;

    // existing code
}

But that reopens the same issue: drm_panel_bridge_add_typed() would not
always call drm_bridge_add(), so the caller doesn't know how to dispose
of the returned pointer.

I think idea 3 can only work if the code understands that the panel
bridge is created by the panel and they _never_ have to create it
themselves. So handling the transition for all drivers would be quite
painful.

> >  * drm_of_find_panel_or_bridge() should disappear: other drivers would
> >    just look for a bridge  
> 
> Please keep it for now.
> 
> Some of the drivers think that it returns literally panel-or-bridge (but
> not both). However if panel bridge is already created, it will return
> both. So those drivers need to be updated.

I really believe it never returns both. The *bridge is set only when
ret != 0 here [0], which can happen only if a panel was not found.
Despite the slightly intricate logic of that function, I don't see how
it could return both in its current implementation.

[0]
https://elixir.bootlin.com/linux/v6.13-rc3/source/drivers/gpu/drm/drm_of.c#L273

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v5 08/10] drm/bridge: samsung-dsim: use supporting variable for out_bridge

[Luca Ceresoli]

Hi Maxime,

On Thu, 16 Jan 2025 13:26:25 +0100
Maxime Ripard <mripard@kernel.org> wrote:

[...]

> > And then there is the panel bridge. My understanding (which I'd love to
> > get clarified in case it is not accurate) is that DRM bridges expect to
> > always interact with "the next bridge", which cannot work for the last
> > bridge of course, and so the panel bridge wraps the panel pretending it
> > is a bridge.
> > 
> > This software structure is clearly not accurately modeling the
> > hardware (panel is not bridge),  
> 
> We don't have a proper definition of what a bridge is, so as far as I'm
> concerned, everything is a bridge :)
> 
> The name came from "external signal converters", but the API got reused
> to support so many hardware components it's not meaningful anymore.

So if I'm getting your point here, drm_bridge is actually the base
class for DRM devices in OOP jargon, or a "DRM subunit" in V4L2 jargon.
OK, that's fine for me (except maybe for a "we should rename" thought).

[...]

> > So far this approach has been working because devm and drmm ensure the
> > panel bridge would be dealloacted at some point. However the devm and drmm
> > release actions are associated to the consumer struct device (the panel
> > bridge consumer), so if the panel bridge is removed and the consumer is
> > not, deallocation won't happen.  
> 
> Oh, right, if one doesn't call drm_bridge_put(), that will result in a
> memory leak. The general topic we discuss and try to address here is
> memory safety, and a memory leak is considered safe. It's also going to
> get allocated only a couple of times anyway, so it's not a *huge*
> concern.
> 
> And about how to actually fix it, there's two ways to go about it:
> 
>   * Either we do a coccinelle script and try to put all those
>     drm_bridge_put() everywhere;
> 
>   * Or we create a devm/drmm action and drop the reference
>     automatically.
> 
> The latter is obviously less intrusive, we would need to deprecate
> devm_of_get_bridge() for it to be safe, and I'm not entirely sure it
> would be enough, but it might just work.
> 
> > For hotplugging we cannot use drmm and devm and instead we use get/put,
> > to let the "next bridge" disappear with the previous one still present.
> > So the trivial idea is to add a drm_of_get_bridge(), similar to
> > {drmm,devm_drm}_of_get_bridge() except it uses plain
> > drm_panel_bridge_add() instead of devm/drmm variants. But then the
> > caller (which is the panel consumer) will have to dispose of the struct
> > drm_bridge pointer by calling:
> > 
> >  - drm_bridge_put() in case a)
> >  - drm_panel_bridge_remove in case b)
> > 
> > And that's the problem I need to solve.  
> 
> I'm not sure the problem is limited to panel_bridge. Your question is
> essentially: how do I make sure a driver-specific init is properly freed
> at drm_bridge_put time. This was done so far mostly at bridge remove
> time, but we obviously can't do that anymore.
> 
> But we'd have the same issue if, say, we needed to remove a workqueue
> from a driver.
> 
> I think we need a destroy() hook for bridges, just like we have for
> connectors for example that would deal with calling
> drm_panel_bridge_remove() if necessary, or any other driver-specific
> sequence.

The .destroy hook looked appealing at first, however as I tried to
apply the idea to bridges I'm not sure it matches. Here's why.

The normal (and sane) flow for a bridge is:

 A) probe
    1. allocate private struct embedding struct drm_bridge
       (I have an _alloc() variant ready for v5 to improve this as you proposed)
    2. get resources, initialize struct fields
    3. drm_bridge_add(): publish bridge into global bridge_list

Now the bridge can be found and pointers taken and used.

And on hardware removal, in reverse order:
 
 B) remove (hardware is hot-unplugged)
    3. unpublish bridge
    2. release resources, cleanup
    1. kfree private struct

Some drivers do real stuff in B2, so it is important that B3 happens
before B2, isn't it? We don't want other drivers to find and use a
bridge that is being dismantled, or afterwards.

B3 should normally happen by removing the bridge from the global
bridge_list, or other bridges might find it. However setting the "gone"
bool and teaching of_drm_find_bridge() & Co to skip bridges with
gone==true would allow to postpone the actual removal, if needed.

With that said, with hotplugging there will be two distinct events:

 * hardware removal
 * last ref is put

The second event could happen way later than the first one. During the
time frame between the two events we need the bridge to be unpublished
and the bridge resources to be already released, as the hardware is
gone. We cannot do this at the last put, it's too late.

So I think the only sane sequence is:

 * on hardware removal:
     B3) unpublish bridge (drm_bridge_remove() or just set gone flag)
     B2) free resources, deinit whatever needed
 * when last ref is put
     B1) kfree (likely via devm)

So, back to the .destroy hook, it would fit at B2, and not at the last
put.

However in that place it seems unnecessary. The actions "on hardware
removal" (B3, B2) are done by the driver remove function, so they are
already driver specific without any additional hook. I'm however fine
to add the hook on hardware removal in case there's a good reason I
missed.

Do you think my reasoning is correct so far?

If you don't, can you clarify at which events (hardware removal VS last
put) the various actions (drm_bridge_remove, set gone flag, calling
.destroy, free resources and deinint, kfree) should be done?


(Side note: I've been pondering on why the .destroy hook works for
connectors and would not for bridges. I think it's due to the global
bridge_list, or because of the different lifetime management based on
drmm for connectors, or both.)


It may look as if my discussion is about bridges in general and not
about the panel bridge. However before delving into how to dispose of
a panel bridge we need to sort out how to dispose of a bridge in the
first place.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v5 08/10] drm/bridge: samsung-dsim: use supporting variable for out_bridge

[Dmitry Baryshkov]

On Tue, Jan 21, 2025 at 12:27:18PM +0100, Luca Ceresoli wrote:
> Hi Dmitry,
> 
> On Thu, 16 Jan 2025 12:56:25 +0200
> Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> 
> [...]
> 
> > > Idea 3: 
> > > 
> > > The idea is that if the panel driver framework always creates a panel
> > > bridge, it will never need to be created on the fly automagically by
> > > its consumers, so the whole problem would disappear. It also would be
> > > better modeling the hardware: still wrapping a panel with a drm_bridge
> > > that does not exist in the hardware, but at least having it created by
> > > the provider driver and not by the consumer driver which happens to
> > > look for it.  
> > 
> > I think this is the best option up to now.
> 
> Thanks for sharing your opinion. However a few points are not clear to
> me, see below.
> 
> > > This looks like a promising and simple idea, so I tried a quick
> > > implementation:
> > > 
> > >  void drm_panel_init(struct drm_panel *panel, struct device *dev,
> > >                     const struct drm_panel_funcs *funcs, int connector_type)
> > >  {
> > > +       struct drm_bridge *bridge;
> > > +
> > >         INIT_LIST_HEAD(&panel->list);
> > >         INIT_LIST_HEAD(&panel->followers);
> > >         mutex_init(&panel->follower_lock);
> > >         panel->dev = dev;
> > >         panel->funcs = funcs;
> > >         panel->connector_type = connector_type;
> > > +
> > > +       bridge = devm_drm_panel_bridge_add(panel->dev, panel);
> > > +       WARN_ON(!bridge);
> > >  }
> > > 
> > > This is somewhat working but it requires more work because:
> > > 
> > >  * as it is, it creates a circular dependency between drm_panel and the
> > >    panel bridge, and modular builds will fail:
> > > 
> > >      depmod: ERROR: Cycle detected: drm -> drm_kms_helper -> drm
> > > 
> > >  * The panel bridge implementation should be made private to the panel
> > >    driver only (possibly helping to solve the previous issue?)  
> > 
> > Or merge drm_panel.c into bridge/panel.c.
> 
> Not sure here you mean exactly what you wrote, or the other way around.
> It looks more correct to me that the panel core (drm_panel.c) starts
> exposing a bridge now, and not that the panel bridge which is just one
> of many bridge drivers starts handling all the panel-related stuff.

No, I actually meant what I wrote: merge drm_panel.c into
bridge/panel.c. Indeed we have some drivers that use panel directly.
However drm_bridge is a more generic interface. So, yes, I propose to
have a bridge driver which incorporates panel support.

> 
> > The panel bridge already
> > exports non-standard API, so it should be fine to extend / change that
> > API. Likewise we might move drm_panel.c to drm_kms_helper.o, also
> > resolving the loop.
> 
> Again I'm not following I'm afraid. It would seem more logical to me to
> move things from the helper into drm.o as they become more necessary
> for common cases, rather than moving things out to a helper module and
> then force all panel drivers to depend on the helper module.

There are a lot of DRM drivers which do not use panels (nor bridges).
Merging that code into drm.ko means that everybody ends up having that
piece of code in memory. Moving drm_panel.o out of drm.ko and
bridge/panel.o out of drm_kms_helper.ko would make the kernel slightly
smaller for those desktop-only users.

> 
> Apologies, I'm perhaps not following your reasoning here. :(
> 
> > There will be a need for a Kconfig update in both
> > cases.
> > 
> > >  * Many drivers currently call devm_drm_panel_bridge_add() directly,
> > >    they should probably call drm_of_get_bridge instead  
> > 
> > I'd say, make it a stub that calls drm_of_get_bridge() with a possible
> > deprecation warning.
> 
> I get the idea, but it would need to be implemented differently because
> drm_of_get_bridge() calls devm_drm_panel_bridge_add(). They would loop

If the drm_bridge is added during drm_panel_add(), then there is no need
to call devm_drm_panel_bridge_add() from drm_of_get_bridge().

> into each other. I think we might simply add a check at the beginning
> of drm_panel_bridge_add_typed(), as in (pseudocode):
> 
> drm_panel_bridge_add_typed(struct drm_panel *panel, ...)
> {
>     if (panel_has_a_panel_bridge(panel))
>         return panel->panel_bridge.bridge;
> 
>     // existing code
> }
> 
> But that reopens the same issue: drm_panel_bridge_add_typed() would not
> always call drm_bridge_add(), so the caller doesn't know how to dispose
> of the returned pointer.
> 
> I think idea 3 can only work if the code understands that the panel
> bridge is created by the panel and they _never_ have to create it
> themselves. So handling the transition for all drivers would be quite
> painful.

That's why I suggested just stubbing existing functions just to lookup
and return a bridge: drivers can transition one-by-one. The bridge will
be already there, created by the panel support code.

> 
> > >  * drm_of_find_panel_or_bridge() should disappear: other drivers would
> > >    just look for a bridge  
> > 
> > Please keep it for now.
> > 
> > Some of the drivers think that it returns literally panel-or-bridge (but
> > not both). However if panel bridge is already created, it will return
> > both. So those drivers need to be updated.
> 
> I really believe it never returns both. The *bridge is set only when
> ret != 0 here [0], which can happen only if a panel was not found.
> Despite the slightly intricate logic of that function, I don't see how
> it could return both in its current implementation.

Indeed. You are right, I skipped the if(ret) condition. More boilerplate
code in the drivers :-(

> 
> [0]
> https://elixir.bootlin.com/linux/v6.13-rc3/source/drivers/gpu/drm/drm_of.c#L273
> 
> Luca
> 
> -- 
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

-- 
With best wishes
Dmitry

Re: [PATCH v5 04/10] drm/bridge: add documentation of refcounted bridges

[Luca Ceresoli]

Hi Maxime,

On Wed, 8 Jan 2025 17:02:04 +0100
Maxime Ripard <mripard@kernel.org> wrote:

[...]

> > > > And we'll also need some flag in drm_bridge to indicate that the device
> > > > is gone, similar to what drm_dev_enter()/drm_dev_exit() provides,
> > > > because now your bridge driver sticks around for much longer than your
> > > > device so the expectation that your device managed resources (clocks,
> > > > registers, etc.) are always going to be around.    
> > 
> > Yes, makes sense too. That should be a drm_bridge_enter/exit(), and
> > drm_bridge.c will need to be sprinkled with them I guess.  
> 
> The users would be the drivers, most likely. There's not much we can do
> at the framework level, unfortunately.

Back to the idea of a "gone" flag, or perhaps an "unplugged" flag to
be consistent with the struct drm_device naming, and
drm_bridge_enter()/drm_bridge_exit(), I did a few experiments and have
a question.

In case:

  a) there is a notification callback to inform about bridges
     being removed, and
  b) all entities owning a struct drm_bridge pointer stop using
     that pointer when notified


With the above, there should be no need for
drm_bridge_enter()/drm_bridge_exit(). Nobody will be using a pointer to
a bridge that is being removed.

Now, about a), patch 1 in this series implements such a mechanism to
inform all bridges when a bridge is being removed. Note that the
"unplugged" flag would be set immediately after the notifier callback
is currently called: "unplugged == true" will never happen before the
callback, and after the callback there will be no pointer at all.

Patch 1 however is only notifying bridges, so other entities (e.g.
encoders) cannot be notified with this implementation. However a
different notification mechanism can be implemented. E.g. until v3 this
series was using a generic struct notifier_block for this goal [0], so
any part of the kernel can be notified.

About b), the notification appears simpler to implement in the various
drivers as it needs to be added in one place per driver. Also adding
drm_bridge_enter()/exit() can be trickier to get right for non-trivial
functions.

Do you see any drawback in using a notification mechanism instead of
drm_bridge_enter()/exit() + unplugged flag?

[0] https://lore.kernel.org/all/20240510-hotplug-drm-bridge-v2-2-ec32f2c66d56@bootlin.com/

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v5 04/10] drm/bridge: add documentation of refcounted bridges

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3141 bytes --]

Hi,

On Wed, Jan 22, 2025 at 05:12:30PM +0100, Luca Ceresoli wrote:
> On Wed, 8 Jan 2025 17:02:04 +0100
> Maxime Ripard <mripard@kernel.org> wrote:
> 
> [...]
> 
> > > > > And we'll also need some flag in drm_bridge to indicate that the device
> > > > > is gone, similar to what drm_dev_enter()/drm_dev_exit() provides,
> > > > > because now your bridge driver sticks around for much longer than your
> > > > > device so the expectation that your device managed resources (clocks,
> > > > > registers, etc.) are always going to be around.    
> > > 
> > > Yes, makes sense too. That should be a drm_bridge_enter/exit(), and
> > > drm_bridge.c will need to be sprinkled with them I guess.  
> > 
> > The users would be the drivers, most likely. There's not much we can do
> > at the framework level, unfortunately.
> 
> Back to the idea of a "gone" flag, or perhaps an "unplugged" flag to
> be consistent with the struct drm_device naming, and
> drm_bridge_enter()/drm_bridge_exit(), I did a few experiments and have
> a question.
> 
> In case:
> 
>   a) there is a notification callback to inform about bridges
>      being removed, and
>   b) all entities owning a struct drm_bridge pointer stop using
>      that pointer when notified
> 
> 
> With the above, there should be no need for
> drm_bridge_enter()/drm_bridge_exit(). Nobody will be using a pointer to
> a bridge that is being removed.
> 
> Now, about a), patch 1 in this series implements such a mechanism to
> inform all bridges when a bridge is being removed. Note that the
> "unplugged" flag would be set immediately after the notifier callback
> is currently called: "unplugged == true" will never happen before the
> callback, and after the callback there will be no pointer at all.
> 
> Patch 1 however is only notifying bridges, so other entities (e.g.
> encoders) cannot be notified with this implementation. However a
> different notification mechanism can be implemented. E.g. until v3 this
> series was using a generic struct notifier_block for this goal [0], so
> any part of the kernel can be notified.
> 
> About b), the notification appears simpler to implement in the various
> drivers as it needs to be added in one place per driver. Also adding
> drm_bridge_enter()/exit() can be trickier to get right for non-trivial
> functions.
> 
> Do you see any drawback in using a notification mechanism instead of
> drm_bridge_enter()/exit() + unplugged flag?

Yeah, because we're not considering the same thing :)

The issue you're talking about is that you want to be notified that the
next bridge has been removed and you shouldn't use the drm_bridge
pointer anymore.

A notification mechanism sounds like a good solution there.

The other issue we have is that now, we will have the drm_bridge pointer
still allocated and valid after its device has been removed.

In which case, you need to be able to tell the bridge driver whose
device got removed that the devm resources aren't there anymore, and it
shouldn't try to access them.

That's what drm_bridge_enter()/exit is here for.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v5 08/10] drm/bridge: samsung-dsim: use supporting variable for out_bridge

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 7332 bytes --]

On Tue, Jan 21, 2025 at 12:27:29PM +0100, Luca Ceresoli wrote:
> Hi Maxime,
> 
> On Thu, 16 Jan 2025 13:26:25 +0100
> Maxime Ripard <mripard@kernel.org> wrote:
> 
> [...]
> 
> > > And then there is the panel bridge. My understanding (which I'd love to
> > > get clarified in case it is not accurate) is that DRM bridges expect to
> > > always interact with "the next bridge", which cannot work for the last
> > > bridge of course, and so the panel bridge wraps the panel pretending it
> > > is a bridge.
> > > 
> > > This software structure is clearly not accurately modeling the
> > > hardware (panel is not bridge),  
> > 
> > We don't have a proper definition of what a bridge is, so as far as I'm
> > concerned, everything is a bridge :)
> > 
> > The name came from "external signal converters", but the API got reused
> > to support so many hardware components it's not meaningful anymore.
> 
> So if I'm getting your point here, drm_bridge is actually the base
> class for DRM devices in OOP jargon, or a "DRM subunit" in V4L2 jargon.
> OK, that's fine for me (except maybe for a "we should rename" thought).

To be clear, I'm not sure it's worth renaming drm_bridge to something
else, and I certainly don't consider it is a prerequisite to this
series.

> > > So far this approach has been working because devm and drmm ensure the
> > > panel bridge would be dealloacted at some point. However the devm and drmm
> > > release actions are associated to the consumer struct device (the panel
> > > bridge consumer), so if the panel bridge is removed and the consumer is
> > > not, deallocation won't happen.  
> > 
> > Oh, right, if one doesn't call drm_bridge_put(), that will result in a
> > memory leak. The general topic we discuss and try to address here is
> > memory safety, and a memory leak is considered safe. It's also going to
> > get allocated only a couple of times anyway, so it's not a *huge*
> > concern.
> > 
> > And about how to actually fix it, there's two ways to go about it:
> > 
> >   * Either we do a coccinelle script and try to put all those
> >     drm_bridge_put() everywhere;
> > 
> >   * Or we create a devm/drmm action and drop the reference
> >     automatically.
> > 
> > The latter is obviously less intrusive, we would need to deprecate
> > devm_of_get_bridge() for it to be safe, and I'm not entirely sure it
> > would be enough, but it might just work.
> > 
> > > For hotplugging we cannot use drmm and devm and instead we use get/put,
> > > to let the "next bridge" disappear with the previous one still present.
> > > So the trivial idea is to add a drm_of_get_bridge(), similar to
> > > {drmm,devm_drm}_of_get_bridge() except it uses plain
> > > drm_panel_bridge_add() instead of devm/drmm variants. But then the
> > > caller (which is the panel consumer) will have to dispose of the struct
> > > drm_bridge pointer by calling:
> > > 
> > >  - drm_bridge_put() in case a)
> > >  - drm_panel_bridge_remove in case b)
> > > 
> > > And that's the problem I need to solve.  
> > 
> > I'm not sure the problem is limited to panel_bridge. Your question is
> > essentially: how do I make sure a driver-specific init is properly freed
> > at drm_bridge_put time. This was done so far mostly at bridge remove
> > time, but we obviously can't do that anymore.
> > 
> > But we'd have the same issue if, say, we needed to remove a workqueue
> > from a driver.
> > 
> > I think we need a destroy() hook for bridges, just like we have for
> > connectors for example that would deal with calling
> > drm_panel_bridge_remove() if necessary, or any other driver-specific
> > sequence.
> 
> The .destroy hook looked appealing at first, however as I tried to
> apply the idea to bridges I'm not sure it matches. Here's why.
> 
> The normal (and sane) flow for a bridge is:
> 
>  A) probe
>     1. allocate private struct embedding struct drm_bridge
>        (I have an _alloc() variant ready for v5 to improve this as you proposed)
>     2. get resources, initialize struct fields
>     3. drm_bridge_add(): publish bridge into global bridge_list
> 
> Now the bridge can be found and pointers taken and used.

We agree so far.

> And on hardware removal, in reverse order:
>  
>  B) remove (hardware is hot-unplugged)
>     3. unpublish bridge
>     2. release resources, cleanup
>     1. kfree private struct

I think the sequence would rather be something like:

B') remove
  3. unpublish bridge
  2. release device resources
  1. release reference

C') last put
  2. release KMS resources
  1. kfree private struct

> Some drivers do real stuff in B2, so it is important that B3 happens
> before B2, isn't it? We don't want other drivers to find and use a
> bridge that is being dismantled, or afterwards.

Yeah, B3/B'3 should definitely happen first.

> B3 should normally happen by removing the bridge from the global
> bridge_list, or other bridges might find it. However setting the "gone"
> bool and teaching of_drm_find_bridge() & Co to skip bridges with
> gone==true would allow to postpone the actual removal, if needed.
> 
> With that said, with hotplugging there will be two distinct events:
> 
>  * hardware removal
>  * last ref is put
> 
> The second event could happen way later than the first one. During the
> time frame between the two events we need the bridge to be unpublished
> and the bridge resources to be already released, as the hardware is
> gone. We cannot do this at the last put, it's too late.
> 
> So I think the only sane sequence is:
> 
>  * on hardware removal:
>      B3) unpublish bridge (drm_bridge_remove() or just set gone flag)
>      B2) free resources, deinit whatever needed
>  * when last ref is put
>      B1) kfree (likely via devm)

No, devm will have destroyed it in B'2. We need to destroy it in the
cleanup hook of kref_put

> So, back to the .destroy hook, it would fit at B2, and not at the last
> put.

destroy would be called at C'2 time

> However in that place it seems unnecessary. The actions "on hardware
> removal" (B3, B2) are done by the driver remove function, so they are
> already driver specific without any additional hook. I'm however fine
> to add the hook on hardware removal in case there's a good reason I
> missed.
> 
> Do you think my reasoning is correct so far?
> 
> If you don't, can you clarify at which events (hardware removal VS last
> put) the various actions (drm_bridge_remove, set gone flag, calling
> .destroy, free resources and deinint, kfree) should be done?

I believe I did already, the gone flag should be set before B'2

Maxime
> 
> 
> (Side note: I've been pondering on why the .destroy hook works for
> connectors and would not for bridges. I think it's due to the global
> bridge_list, or because of the different lifetime management based on
> drmm for connectors, or both.)
> 
> 
> It may look as if my discussion is about bridges in general and not
> about the panel bridge. However before delving into how to dispose of
> a panel bridge we need to sort out how to dispose of a bridge in the
> first place.
> 
> Luca
> 
> -- 
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v5 08/10] drm/bridge: samsung-dsim: use supporting variable for out_bridge

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3155 bytes --]

On Tue, Jan 21, 2025 at 01:57:47PM +0200, Dmitry Baryshkov wrote:
> On Tue, Jan 21, 2025 at 12:27:18PM +0100, Luca Ceresoli wrote:
> > Hi Dmitry,
> > 
> > On Thu, 16 Jan 2025 12:56:25 +0200
> > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> > 
> > [...]
> > 
> > > > Idea 3: 
> > > > 
> > > > The idea is that if the panel driver framework always creates a panel
> > > > bridge, it will never need to be created on the fly automagically by
> > > > its consumers, so the whole problem would disappear. It also would be
> > > > better modeling the hardware: still wrapping a panel with a drm_bridge
> > > > that does not exist in the hardware, but at least having it created by
> > > > the provider driver and not by the consumer driver which happens to
> > > > look for it.  
> > > 
> > > I think this is the best option up to now.
> > 
> > Thanks for sharing your opinion. However a few points are not clear to
> > me, see below.
> > 
> > > > This looks like a promising and simple idea, so I tried a quick
> > > > implementation:
> > > > 
> > > >  void drm_panel_init(struct drm_panel *panel, struct device *dev,
> > > >                     const struct drm_panel_funcs *funcs, int connector_type)
> > > >  {
> > > > +       struct drm_bridge *bridge;
> > > > +
> > > >         INIT_LIST_HEAD(&panel->list);
> > > >         INIT_LIST_HEAD(&panel->followers);
> > > >         mutex_init(&panel->follower_lock);
> > > >         panel->dev = dev;
> > > >         panel->funcs = funcs;
> > > >         panel->connector_type = connector_type;
> > > > +
> > > > +       bridge = devm_drm_panel_bridge_add(panel->dev, panel);
> > > > +       WARN_ON(!bridge);
> > > >  }
> > > > 
> > > > This is somewhat working but it requires more work because:
> > > > 
> > > >  * as it is, it creates a circular dependency between drm_panel and the
> > > >    panel bridge, and modular builds will fail:
> > > > 
> > > >      depmod: ERROR: Cycle detected: drm -> drm_kms_helper -> drm
> > > > 
> > > >  * The panel bridge implementation should be made private to the panel
> > > >    driver only (possibly helping to solve the previous issue?)  
> > > 
> > > Or merge drm_panel.c into bridge/panel.c.
> > 
> > Not sure here you mean exactly what you wrote, or the other way around.
> > It looks more correct to me that the panel core (drm_panel.c) starts
> > exposing a bridge now, and not that the panel bridge which is just one
> > of many bridge drivers starts handling all the panel-related stuff.
> 
> No, I actually meant what I wrote: merge drm_panel.c into
> bridge/panel.c. Indeed we have some drivers that use panel directly.
> However drm_bridge is a more generic interface. So, yes, I propose to
> have a bridge driver which incorporates panel support.

It's a legitimate subject to discuss, but I'm not sure it's worth
focusing on this at this point though.

We should probably split it out into smaller chunks. Figuring out the
drivers lifetime, and reference counting API is hard enough as it is,
throwing panels into the mix at this point just adds more complexity.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v5 08/10] drm/bridge: samsung-dsim: use supporting variable for out_bridge

[Luca Ceresoli]

Hello Maxime,

thanks for the continued feedback.

On Tue, 28 Jan 2025 16:09:53 +0100
Maxime Ripard <mripard@kernel.org> wrote:

> To be clear, I'm not sure it's worth renaming drm_bridge to something
> else, and I certainly don't consider it is a prerequisite to this
> series.

Sure, I agree.

> > > > For hotplugging we cannot use drmm and devm and instead we use get/put,
> > > > to let the "next bridge" disappear with the previous one still present.
> > > > So the trivial idea is to add a drm_of_get_bridge(), similar to
> > > > {drmm,devm_drm}_of_get_bridge() except it uses plain
> > > > drm_panel_bridge_add() instead of devm/drmm variants. But then the
> > > > caller (which is the panel consumer) will have to dispose of the struct
> > > > drm_bridge pointer by calling:
> > > > 
> > > >  - drm_bridge_put() in case a)
> > > >  - drm_panel_bridge_remove in case b)
> > > > 
> > > > And that's the problem I need to solve.    
> > > 
> > > I'm not sure the problem is limited to panel_bridge. Your question is
> > > essentially: how do I make sure a driver-specific init is properly freed
> > > at drm_bridge_put time. This was done so far mostly at bridge remove
> > > time, but we obviously can't do that anymore.
> > > 
> > > But we'd have the same issue if, say, we needed to remove a workqueue
> > > from a driver.
> > > 
> > > I think we need a destroy() hook for bridges, just like we have for
> > > connectors for example that would deal with calling
> > > drm_panel_bridge_remove() if necessary, or any other driver-specific
> > > sequence.  
> > 
> > The .destroy hook looked appealing at first, however as I tried to
> > apply the idea to bridges I'm not sure it matches. Here's why.
> > 
> > The normal (and sane) flow for a bridge is:
> > 
> >  A) probe
> >     1. allocate private struct embedding struct drm_bridge
> >        (I have an _alloc() variant ready for v5 to improve this as you proposed)
> >     2. get resources, initialize struct fields
> >     3. drm_bridge_add(): publish bridge into global bridge_list
> > 
> > Now the bridge can be found and pointers taken and used.  
> 
> We agree so far.

Good :-)

> > And on hardware removal, in reverse order:
> >  
> >  B) remove (hardware is hot-unplugged)
> >     3. unpublish bridge
> >     2. release resources, cleanup
> >     1. kfree private struct  
> 
> I think the sequence would rather be something like:
> 
> B') remove
>   3. unpublish bridge
>   2. release device resources
>   1. release reference
> 
> C') last put
>   2. release KMS resources
>   1. kfree private struct

Just to ensure we are on the same page: patch 3 is already implementing
this model except for C'2.

Well, in reality it even implements a .destroy callback at C'2, even
though it was not meant for the usage you have in mind and it's
scheduled for removal in v6 -- even though as I said I'm OK in
re-adding it if it is useful.

Mainly I'm not sure I understand for which ultimate goal you propose to
postpone releasing KMS resources to C'.

Is it (1) because we _want_ to postpone releasing KMS resources? In this
case I don't know the use case, so if you have a practical example it
would probably help a lot.

Moreover, for the panel bridge specifically, it would mean postponing
the destruction of the struct panel_bridge, which however has a pointer
to the panel. But the panel is probably hot-unplugged at the same time
as the previous removable bridge(s), we'd have a time window between B'
and C' where there is a pointer to a freed struct panel. We'd need to
ensure that pointer is cleared at B'2, even though it is a "KMS
resource" and not a "device resource".

Or is it (2) because there are cases where we don't know how else we
could release the KMS resources? AFAIK all bridge drivers are able to
release everything in their remove function (B'2) with the exception of
the panel bridge, so this sounds like a workaround for just one user
that apparently we all agree should be improved on its own anyway.

Note I'm not strongly against (2), if it simplifies the path towards
dynamic bridge lifetime by postponing the panel bridge rework. I just
need to understand the plan.

Another question is what is a device resource and what is a KMS
resource. What's the boolean expression to classify a
resource in one or the other family? For example, in your example
quoted above ("But we'd have the same issue if, say, we needed to
remove a workqueue from a driver"), is the workqueue a KMS resource?

I need to understand your idea if I want to implement it.

> > Some drivers do real stuff in B2, so it is important that B3 happens
> > before B2, isn't it? We don't want other drivers to find and use a
> > bridge that is being dismantled, or afterwards.  
> 
> Yeah, B3/B'3 should definitely happen first.
> 
> > B3 should normally happen by removing the bridge from the global
> > bridge_list, or other bridges might find it. However setting the "gone"
> > bool and teaching of_drm_find_bridge() & Co to skip bridges with
> > gone==true would allow to postpone the actual removal, if needed.
> > 
> > With that said, with hotplugging there will be two distinct events:
> > 
> >  * hardware removal
> >  * last ref is put
> > 
> > The second event could happen way later than the first one. During the
> > time frame between the two events we need the bridge to be unpublished
> > and the bridge resources to be already released, as the hardware is
> > gone. We cannot do this at the last put, it's too late.
> > 
> > So I think the only sane sequence is:
> > 
> >  * on hardware removal:
> >      B3) unpublish bridge (drm_bridge_remove() or just set gone flag)
> >      B2) free resources, deinit whatever needed
> >  * when last ref is put
> >      B1) kfree (likely via devm)  
> 
> No, devm will have destroyed it in B'2. We need to destroy it in the
> cleanup hook of kref_put

devm will have destroyed what? Sorry I'm not following.

If you mean "it" == "the private struct", then no, this is not the
case. drm_bridge_init in patch 3 does not kfree the private struct but
instead registers a devm action to call drm_bridge_put. Then, at the
last put, drm_bridge_free() will actually kfree the private struct.

In this v5, kree()ing the private struct at the last put is done via
a callback. In my work towards v6 the principle is the same but I have
reworked it all, implementing a devm_drm_bridge_alloc() macro as you
suggested (BTW that was a great improvement, thanks) and removing the
.destroy callback as it was not needed.

In case it helps, here's a preview of my v6, with some added comments to
support this discussion:

/* Internal function (for refcounted bridges) */
void __drm_bridge_free(struct kref *kref)
{
        struct drm_bridge *bridge = container_of(kref, struct drm_bridge, refcount);
        void *container = ((void*)bridge) - bridge->container_offset;

        DRM_DEBUG("bridge=%p, container=%p FREE\n", bridge, container);

        kfree(container);
}
EXPORT_SYMBOL(__drm_bridge_free);

static inline void drm_bridge_put(struct drm_bridge *bridge)
{
        if (!drm_bridge_is_refcounted(bridge))
                return;

        DRM_DEBUG("bridge=%p PUT\n", bridge);

        kref_put(&bridge->refcount, __drm_bridge_free);
}

static void drm_bridge_put_void(void *data)
{
        struct drm_bridge *bridge = (struct drm_bridge *)data;

        drm_bridge_put(bridge);
}

// fold this into __devm_drm_bridge_alloc() or keep for consistency
// with drm_encoder.c?
static int __devm_drm_bridge_init(struct device *dev, struct drm_bridge *bridge,
                                  size_t offset, const struct drm_bridge_funcs *funcs)
{
        int err;

        bridge->container_offset = offset;
        kref_init(&bridge->refcount);
        bridge->is_refcounted = 1;

        err = devm_add_action_or_reset(dev, drm_bridge_put_void, bridge); // <== devm just puts one ref, does not kfree
        if (err)
                return err;

        bridge->funcs = funcs;

        return 0;
}

void *__devm_drm_bridge_alloc(struct device *dev, size_t size, size_t offset,
                              const struct drm_bridge_funcs *funcs)
{
        void *container;
        struct drm_bridge *bridge;
        int ret;

        if (!funcs) {
                dev_warn(dev, "Missing funcs pointer\n");
                return ERR_PTR(-EINVAL);
        }

        container = kzalloc(size, GFP_KERNEL);     // <== NOT allocating with devm
        if (!container)
                return ERR_PTR(-ENOMEM);

        bridge = container + offset;

        ret = __devm_drm_bridge_init(dev, bridge, offset, funcs);
        if (ret)
                return ERR_PTR(ret);

        DRM_DEBUG("bridge=%p, container=%p, funcs=%ps ALLOC\n", bridge, container, funcs);

        return container;
}
EXPORT_SYMBOL(__devm_drm_bridge_alloc);

#define devm_drm_bridge_alloc(dev, type, member, funcs) \
        ((type *)__devm_drm_bridge_alloc(dev, sizeof(type), \
                                         offsetof(type, member), funcs))

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v5 04/10] drm/bridge: add documentation of refcounted bridges

[Luca Ceresoli]

Hi Maxime,

On Tue, 28 Jan 2025 15:49:23 +0100
Maxime Ripard <mripard@kernel.org> wrote:

> Hi,
> 
> On Wed, Jan 22, 2025 at 05:12:30PM +0100, Luca Ceresoli wrote:
> > On Wed, 8 Jan 2025 17:02:04 +0100
> > Maxime Ripard <mripard@kernel.org> wrote:
> > 
> > [...]
> >   
> > > > > > And we'll also need some flag in drm_bridge to indicate that the device
> > > > > > is gone, similar to what drm_dev_enter()/drm_dev_exit() provides,
> > > > > > because now your bridge driver sticks around for much longer than your
> > > > > > device so the expectation that your device managed resources (clocks,
> > > > > > registers, etc.) are always going to be around.      
> > > > 
> > > > Yes, makes sense too. That should be a drm_bridge_enter/exit(), and
> > > > drm_bridge.c will need to be sprinkled with them I guess.    
> > > 
> > > The users would be the drivers, most likely. There's not much we can do
> > > at the framework level, unfortunately.  
> > 
> > Back to the idea of a "gone" flag, or perhaps an "unplugged" flag to
> > be consistent with the struct drm_device naming, and
> > drm_bridge_enter()/drm_bridge_exit(), I did a few experiments and have
> > a question.
> > 
> > In case:
> > 
> >   a) there is a notification callback to inform about bridges
> >      being removed, and
> >   b) all entities owning a struct drm_bridge pointer stop using
> >      that pointer when notified
> > 
> > 
> > With the above, there should be no need for
> > drm_bridge_enter()/drm_bridge_exit(). Nobody will be using a pointer to
> > a bridge that is being removed.
> > 
> > Now, about a), patch 1 in this series implements such a mechanism to
> > inform all bridges when a bridge is being removed. Note that the
> > "unplugged" flag would be set immediately after the notifier callback
> > is currently called: "unplugged == true" will never happen before the
> > callback, and after the callback there will be no pointer at all.
> > 
> > Patch 1 however is only notifying bridges, so other entities (e.g.
> > encoders) cannot be notified with this implementation. However a
> > different notification mechanism can be implemented. E.g. until v3 this
> > series was using a generic struct notifier_block for this goal [0], so
> > any part of the kernel can be notified.
> > 
> > About b), the notification appears simpler to implement in the various
> > drivers as it needs to be added in one place per driver. Also adding
> > drm_bridge_enter()/exit() can be trickier to get right for non-trivial
> > functions.
> > 
> > Do you see any drawback in using a notification mechanism instead of
> > drm_bridge_enter()/exit() + unplugged flag?  
> 
> Yeah, because we're not considering the same thing :)
> 
> The issue you're talking about is that you want to be notified that the
> next bridge has been removed and you shouldn't use the drm_bridge
> pointer anymore.
> 
> A notification mechanism sounds like a good solution there.
> 
> The other issue we have is that now, we will have the drm_bridge pointer
> still allocated and valid after its device has been removed.
> 
> In which case, you need to be able to tell the bridge driver whose
> device got removed that the devm resources aren't there anymore, and it
> shouldn't try to access them.
> 
> That's what drm_bridge_enter()/exit is here for.

Let me rephrase to check I got what you mean.

A) On bridge removal, use a notifier to notify all consumers of that
bridge that they have to stop using the pointer to the bridge about to
be removed.

B) Internally in the bridge driver (provider) use
drm_bridge_enter()/exit() to forbid access to resources when the
hardware is unplugged.

And also: bridge consumers won't need to use drm_bridge_enter()/exit()
as they will clear their pointer before setting the unplugged flag.

Is my understanding of your idea correct?

If it is, I tend to agree, and I like it.

I like it, except for one point  I'm afraid. Why do we need enter/exit
inside the driver (provider) code? At driver release, the driver
instance won't exist anymore. Sure the private struct embedding a
struct drm_bridge will be still allocated for some time, but the struct
device will not exist, and the device driver instance as well.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v5 04/10] drm/bridge: add documentation of refcounted bridges

[Dmitry Baryshkov]

On Wed, Jan 29, 2025 at 12:51:53PM +0100, Luca Ceresoli wrote:
> Hi Maxime,
> 
> On Tue, 28 Jan 2025 15:49:23 +0100
> Maxime Ripard <mripard@kernel.org> wrote:
> 
> > Hi,
> > 
> > On Wed, Jan 22, 2025 at 05:12:30PM +0100, Luca Ceresoli wrote:
> > > On Wed, 8 Jan 2025 17:02:04 +0100
> > > Maxime Ripard <mripard@kernel.org> wrote:
> > > 
> > > [...]
> > >   
> > > > > > > And we'll also need some flag in drm_bridge to indicate that the device
> > > > > > > is gone, similar to what drm_dev_enter()/drm_dev_exit() provides,
> > > > > > > because now your bridge driver sticks around for much longer than your
> > > > > > > device so the expectation that your device managed resources (clocks,
> > > > > > > registers, etc.) are always going to be around.      
> > > > > 
> > > > > Yes, makes sense too. That should be a drm_bridge_enter/exit(), and
> > > > > drm_bridge.c will need to be sprinkled with them I guess.    
> > > > 
> > > > The users would be the drivers, most likely. There's not much we can do
> > > > at the framework level, unfortunately.  
> > > 
> > > Back to the idea of a "gone" flag, or perhaps an "unplugged" flag to
> > > be consistent with the struct drm_device naming, and
> > > drm_bridge_enter()/drm_bridge_exit(), I did a few experiments and have
> > > a question.
> > > 
> > > In case:
> > > 
> > >   a) there is a notification callback to inform about bridges
> > >      being removed, and
> > >   b) all entities owning a struct drm_bridge pointer stop using
> > >      that pointer when notified
> > > 
> > > 
> > > With the above, there should be no need for
> > > drm_bridge_enter()/drm_bridge_exit(). Nobody will be using a pointer to
> > > a bridge that is being removed.
> > > 
> > > Now, about a), patch 1 in this series implements such a mechanism to
> > > inform all bridges when a bridge is being removed. Note that the
> > > "unplugged" flag would be set immediately after the notifier callback
> > > is currently called: "unplugged == true" will never happen before the
> > > callback, and after the callback there will be no pointer at all.
> > > 
> > > Patch 1 however is only notifying bridges, so other entities (e.g.
> > > encoders) cannot be notified with this implementation. However a
> > > different notification mechanism can be implemented. E.g. until v3 this
> > > series was using a generic struct notifier_block for this goal [0], so
> > > any part of the kernel can be notified.
> > > 
> > > About b), the notification appears simpler to implement in the various
> > > drivers as it needs to be added in one place per driver. Also adding
> > > drm_bridge_enter()/exit() can be trickier to get right for non-trivial
> > > functions.
> > > 
> > > Do you see any drawback in using a notification mechanism instead of
> > > drm_bridge_enter()/exit() + unplugged flag?  
> > 
> > Yeah, because we're not considering the same thing :)
> > 
> > The issue you're talking about is that you want to be notified that the
> > next bridge has been removed and you shouldn't use the drm_bridge
> > pointer anymore.
> > 
> > A notification mechanism sounds like a good solution there.
> > 
> > The other issue we have is that now, we will have the drm_bridge pointer
> > still allocated and valid after its device has been removed.
> > 
> > In which case, you need to be able to tell the bridge driver whose
> > device got removed that the devm resources aren't there anymore, and it
> > shouldn't try to access them.
> > 
> > That's what drm_bridge_enter()/exit is here for.
> 
> Let me rephrase to check I got what you mean.
> 
> A) On bridge removal, use a notifier to notify all consumers of that
> bridge that they have to stop using the pointer to the bridge about to
> be removed.
> 
> B) Internally in the bridge driver (provider) use
> drm_bridge_enter()/exit() to forbid access to resources when the
> hardware is unplugged.
> 
> And also: bridge consumers won't need to use drm_bridge_enter()/exit()
> as they will clear their pointer before setting the unplugged flag.
> 
> Is my understanding of your idea correct?
> 
> If it is, I tend to agree, and I like it.
> 
> I like it, except for one point  I'm afraid. Why do we need enter/exit
> inside the driver (provider) code? At driver release, the driver
> instance won't exist anymore. Sure the private struct embedding a
> struct drm_bridge will be still allocated for some time, but the struct
> device will not exist, and the device driver instance as well.

You have to sync several possible kinds of events: bridge calls from DRM
core, from HDMI audio, CEC, DP AUX _and_ completely async device
'remove' / unbind callbacks.

> 
> Luca
> 
> -- 
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

-- 
With best wishes
Dmitry

Re: [PATCH v5 04/10] drm/bridge: add documentation of refcounted bridges

[Luca Ceresoli]

On Wed, 29 Jan 2025 14:22:30 +0200
Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> On Wed, Jan 29, 2025 at 12:51:53PM +0100, Luca Ceresoli wrote:
> > Hi Maxime,
> > 
> > On Tue, 28 Jan 2025 15:49:23 +0100
> > Maxime Ripard <mripard@kernel.org> wrote:
> >   
> > > Hi,
> > > 
> > > On Wed, Jan 22, 2025 at 05:12:30PM +0100, Luca Ceresoli wrote:  
> > > > On Wed, 8 Jan 2025 17:02:04 +0100
> > > > Maxime Ripard <mripard@kernel.org> wrote:
> > > > 
> > > > [...]
> > > >     
> > > > > > > > And we'll also need some flag in drm_bridge to indicate that the device
> > > > > > > > is gone, similar to what drm_dev_enter()/drm_dev_exit() provides,
> > > > > > > > because now your bridge driver sticks around for much longer than your
> > > > > > > > device so the expectation that your device managed resources (clocks,
> > > > > > > > registers, etc.) are always going to be around.        
> > > > > > 
> > > > > > Yes, makes sense too. That should be a drm_bridge_enter/exit(), and
> > > > > > drm_bridge.c will need to be sprinkled with them I guess.      
> > > > > 
> > > > > The users would be the drivers, most likely. There's not much we can do
> > > > > at the framework level, unfortunately.    
> > > > 
> > > > Back to the idea of a "gone" flag, or perhaps an "unplugged" flag to
> > > > be consistent with the struct drm_device naming, and
> > > > drm_bridge_enter()/drm_bridge_exit(), I did a few experiments and have
> > > > a question.
> > > > 
> > > > In case:
> > > > 
> > > >   a) there is a notification callback to inform about bridges
> > > >      being removed, and
> > > >   b) all entities owning a struct drm_bridge pointer stop using
> > > >      that pointer when notified
> > > > 
> > > > 
> > > > With the above, there should be no need for
> > > > drm_bridge_enter()/drm_bridge_exit(). Nobody will be using a pointer to
> > > > a bridge that is being removed.
> > > > 
> > > > Now, about a), patch 1 in this series implements such a mechanism to
> > > > inform all bridges when a bridge is being removed. Note that the
> > > > "unplugged" flag would be set immediately after the notifier callback
> > > > is currently called: "unplugged == true" will never happen before the
> > > > callback, and after the callback there will be no pointer at all.
> > > > 
> > > > Patch 1 however is only notifying bridges, so other entities (e.g.
> > > > encoders) cannot be notified with this implementation. However a
> > > > different notification mechanism can be implemented. E.g. until v3 this
> > > > series was using a generic struct notifier_block for this goal [0], so
> > > > any part of the kernel can be notified.
> > > > 
> > > > About b), the notification appears simpler to implement in the various
> > > > drivers as it needs to be added in one place per driver. Also adding
> > > > drm_bridge_enter()/exit() can be trickier to get right for non-trivial
> > > > functions.
> > > > 
> > > > Do you see any drawback in using a notification mechanism instead of
> > > > drm_bridge_enter()/exit() + unplugged flag?    
> > > 
> > > Yeah, because we're not considering the same thing :)
> > > 
> > > The issue you're talking about is that you want to be notified that the
> > > next bridge has been removed and you shouldn't use the drm_bridge
> > > pointer anymore.
> > > 
> > > A notification mechanism sounds like a good solution there.
> > > 
> > > The other issue we have is that now, we will have the drm_bridge pointer
> > > still allocated and valid after its device has been removed.
> > > 
> > > In which case, you need to be able to tell the bridge driver whose
> > > device got removed that the devm resources aren't there anymore, and it
> > > shouldn't try to access them.
> > > 
> > > That's what drm_bridge_enter()/exit is here for.  
> > 
> > Let me rephrase to check I got what you mean.
> > 
> > A) On bridge removal, use a notifier to notify all consumers of that
> > bridge that they have to stop using the pointer to the bridge about to
> > be removed.
> > 
> > B) Internally in the bridge driver (provider) use
> > drm_bridge_enter()/exit() to forbid access to resources when the
> > hardware is unplugged.
> > 
> > And also: bridge consumers won't need to use drm_bridge_enter()/exit()
> > as they will clear their pointer before setting the unplugged flag.
> > 
> > Is my understanding of your idea correct?
> > 
> > If it is, I tend to agree, and I like it.
> > 
> > I like it, except for one point  I'm afraid. Why do we need enter/exit
> > inside the driver (provider) code? At driver release, the driver
> > instance won't exist anymore. Sure the private struct embedding a
> > struct drm_bridge will be still allocated for some time, but the struct
> > device will not exist, and the device driver instance as well.  
> 
> You have to sync several possible kinds of events: bridge calls from DRM
> core, from HDMI audio, CEC, DP AUX _and_ completely async device
> 'remove' / unbind callbacks.

Ah, yes, that make sense. Looks like the big picture w.r.t. notifiers
and enter/exit is clear -- until implementation time at least ;)

Thanks,
Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v5 08/10] drm/bridge: samsung-dsim: use supporting variable for out_bridge

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 10852 bytes --]

Hi Luca,

On Wed, Jan 29, 2025 at 12:51:46PM +0100, Luca Ceresoli wrote:
> > > > > For hotplugging we cannot use drmm and devm and instead we use get/put,
> > > > > to let the "next bridge" disappear with the previous one still present.
> > > > > So the trivial idea is to add a drm_of_get_bridge(), similar to
> > > > > {drmm,devm_drm}_of_get_bridge() except it uses plain
> > > > > drm_panel_bridge_add() instead of devm/drmm variants. But then the
> > > > > caller (which is the panel consumer) will have to dispose of the struct
> > > > > drm_bridge pointer by calling:
> > > > > 
> > > > >  - drm_bridge_put() in case a)
> > > > >  - drm_panel_bridge_remove in case b)
> > > > > 
> > > > > And that's the problem I need to solve.    
> > > > 
> > > > I'm not sure the problem is limited to panel_bridge. Your question is
> > > > essentially: how do I make sure a driver-specific init is properly freed
> > > > at drm_bridge_put time. This was done so far mostly at bridge remove
> > > > time, but we obviously can't do that anymore.
> > > > 
> > > > But we'd have the same issue if, say, we needed to remove a workqueue
> > > > from a driver.
> > > > 
> > > > I think we need a destroy() hook for bridges, just like we have for
> > > > connectors for example that would deal with calling
> > > > drm_panel_bridge_remove() if necessary, or any other driver-specific
> > > > sequence.  
> > > 
> > > The .destroy hook looked appealing at first, however as I tried to
> > > apply the idea to bridges I'm not sure it matches. Here's why.
> > > 
> > > The normal (and sane) flow for a bridge is:
> > > 
> > >  A) probe
> > >     1. allocate private struct embedding struct drm_bridge
> > >        (I have an _alloc() variant ready for v5 to improve this as you proposed)
> > >     2. get resources, initialize struct fields
> > >     3. drm_bridge_add(): publish bridge into global bridge_list
> > > 
> > > Now the bridge can be found and pointers taken and used.  
> > 
> > We agree so far.
> 
> Good :-)
> 
> > > And on hardware removal, in reverse order:
> > >  
> > >  B) remove (hardware is hot-unplugged)
> > >     3. unpublish bridge
> > >     2. release resources, cleanup
> > >     1. kfree private struct  
> > 
> > I think the sequence would rather be something like:
> > 
> > B') remove
> >   3. unpublish bridge
> >   2. release device resources
> >   1. release reference
> > 
> > C') last put
> >   2. release KMS resources
> >   1. kfree private struct
> 
> Just to ensure we are on the same page: patch 3 is already implementing
> this model except for C'2.
> 
> Well, in reality it even implements a .destroy callback at C'2, even
> though it was not meant for the usage you have in mind and it's
> scheduled for removal in v6 -- even though as I said I'm OK in
> re-adding it if it is useful.
> 
> Mainly I'm not sure I understand for which ultimate goal you propose to
> postpone releasing KMS resources to C'.
> 
> Is it (1) because we _want_ to postpone releasing KMS resources? In this
> case I don't know the use case, so if you have a practical example it
> would probably help a lot.

It's not that we want it, it's that it's already happening :)

The main DRM device is only torn down not when its devices goes away,
but when the last application closes its fd to the device file. Thus,
there's a significant window (possibly infinitely long) between the
device being removed and the DRM device being freed, during which the
application will still be able to issue ioctl that might reach the
driver.

Thus, we have two kind of resources: the ones tied to the device
(clocks, register mappings, etc.) that will go away when the device is
removed, and the ones tied to the DRM device (connectors, bridges, etc.)
that need to stick until the DRM device is free'd. It's the difference
between devm and drmm actions.

> Moreover, for the panel bridge specifically, it would mean postponing
> the destruction of the struct panel_bridge, which however has a pointer
> to the panel. But the panel is probably hot-unplugged at the same time
> as the previous removable bridge(s), we'd have a time window between B'
> and C' where there is a pointer to a freed struct panel. We'd need to
> ensure that pointer is cleared at B'2, even though it is a "KMS
> resource" and not a "device resource".

You're correct, but we have to start somewhere. Fixing the issue for
bridges only will already fix it for all setups using only bridges, even
if the ones using panels are still broken.

I'm also mentoring someone at the moment to fix this for panels, so it's
only temporary.

> Or is it (2) because there are cases where we don't know how else we
> could release the KMS resources? AFAIK all bridge drivers are able to
> release everything in their remove function (B'2) with the exception of
> the panel bridge, so this sounds like a workaround for just one user
> that apparently we all agree should be improved on its own anyway.
> 
> Note I'm not strongly against (2), if it simplifies the path towards
> dynamic bridge lifetime by postponing the panel bridge rework. I just
> need to understand the plan.
> 
> Another question is what is a device resource and what is a KMS
> resource. What's the boolean expression to classify a
> resource in one or the other family? For example, in your example
> quoted above ("But we'd have the same issue if, say, we needed to
> remove a workqueue from a driver"), is the workqueue a KMS resource?

It depends on what the workqueue is doing. If it's to handle atomic
commits like the writeback code, then it's KMS facing. If it's to handle
interrupts, it's device facing.

It's hard to come up with a boolean classification, but it's basically
"can any ioctl code path end up using that resource?".

> I need to understand your idea if I want to implement it.
> 
> > > Some drivers do real stuff in B2, so it is important that B3 happens
> > > before B2, isn't it? We don't want other drivers to find and use a
> > > bridge that is being dismantled, or afterwards.  
> > 
> > Yeah, B3/B'3 should definitely happen first.
> > 
> > > B3 should normally happen by removing the bridge from the global
> > > bridge_list, or other bridges might find it. However setting the "gone"
> > > bool and teaching of_drm_find_bridge() & Co to skip bridges with
> > > gone==true would allow to postpone the actual removal, if needed.
> > > 
> > > With that said, with hotplugging there will be two distinct events:
> > > 
> > >  * hardware removal
> > >  * last ref is put
> > > 
> > > The second event could happen way later than the first one. During the
> > > time frame between the two events we need the bridge to be unpublished
> > > and the bridge resources to be already released, as the hardware is
> > > gone. We cannot do this at the last put, it's too late.
> > > 
> > > So I think the only sane sequence is:
> > > 
> > >  * on hardware removal:
> > >      B3) unpublish bridge (drm_bridge_remove() or just set gone flag)
> > >      B2) free resources, deinit whatever needed
> > >  * when last ref is put
> > >      B1) kfree (likely via devm)  
> > 
> > No, devm will have destroyed it in B'2. We need to destroy it in the
> > cleanup hook of kref_put
> 
> devm will have destroyed what? Sorry I'm not following.
>
> If you mean "it" == "the private struct", then no, this is not the
> case. drm_bridge_init in patch 3 does not kfree the private struct but
> instead registers a devm action to call drm_bridge_put. Then, at the
> last put, drm_bridge_free() will actually kfree the private struct.
> 
> In this v5, kree()ing the private struct at the last put is done via
> a callback. In my work towards v6 the principle is the same but I have
> reworked it all, implementing a devm_drm_bridge_alloc() macro as you
> suggested (BTW that was a great improvement, thanks) and removing the
> .destroy callback as it was not needed.
> 
> In case it helps, here's a preview of my v6, with some added comments to
> support this discussion:
> 
> /* Internal function (for refcounted bridges) */
> void __drm_bridge_free(struct kref *kref)
> {
>         struct drm_bridge *bridge = container_of(kref, struct drm_bridge, refcount);
>         void *container = ((void*)bridge) - bridge->container_offset;
> 
>         DRM_DEBUG("bridge=%p, container=%p FREE\n", bridge, container);
> 
>         kfree(container);
> }
> EXPORT_SYMBOL(__drm_bridge_free);
> 
> static inline void drm_bridge_put(struct drm_bridge *bridge)
> {
>         if (!drm_bridge_is_refcounted(bridge))
>                 return;
> 
>         DRM_DEBUG("bridge=%p PUT\n", bridge);
> 
>         kref_put(&bridge->refcount, __drm_bridge_free);
> }
> 
> static void drm_bridge_put_void(void *data)
> {
>         struct drm_bridge *bridge = (struct drm_bridge *)data;
> 
>         drm_bridge_put(bridge);
> }
> 
> // fold this into __devm_drm_bridge_alloc() or keep for consistency
> // with drm_encoder.c?
> static int __devm_drm_bridge_init(struct device *dev, struct drm_bridge *bridge,
>                                   size_t offset, const struct drm_bridge_funcs *funcs)
> {
>         int err;
> 
>         bridge->container_offset = offset;
>         kref_init(&bridge->refcount);
>         bridge->is_refcounted = 1;
> 
>         err = devm_add_action_or_reset(dev, drm_bridge_put_void, bridge); // <== devm just puts one ref, does not kfree
>         if (err)
>                 return err;
> 
>         bridge->funcs = funcs;
> 
>         return 0;
> }
> 
> void *__devm_drm_bridge_alloc(struct device *dev, size_t size, size_t offset,
>                               const struct drm_bridge_funcs *funcs)
> {
>         void *container;
>         struct drm_bridge *bridge;
>         int ret;
> 
>         if (!funcs) {
>                 dev_warn(dev, "Missing funcs pointer\n");
>                 return ERR_PTR(-EINVAL);
>         }
> 
>         container = kzalloc(size, GFP_KERNEL);     // <== NOT allocating with devm
>         if (!container)
>                 return ERR_PTR(-ENOMEM);
> 
>         bridge = container + offset;
> 
>         ret = __devm_drm_bridge_init(dev, bridge, offset, funcs);
>         if (ret)
>                 return ERR_PTR(ret);
> 
>         DRM_DEBUG("bridge=%p, container=%p, funcs=%ps ALLOC\n", bridge, container, funcs);
> 
>         return container;
> }
> EXPORT_SYMBOL(__devm_drm_bridge_alloc);

Awesome, I guess we were actually understanding each other the whole
time then :)

I'm still kind of sure we'll require a destroy callback to call in
__drm_bridge_free, but if it works, I guess it's good enough for now.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v5 10/10] drm/bridge: hotplug-bridge: add driver to support hot-pluggable DSI bridges

[Luca Ceresoli]

Hello Dmitry, Maxime, DRM maintainers,

On Thu, 2 Jan 2025 13:01:40 +0100
Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:

> Hi Dmitry,
> 
> On Tue, 31 Dec 2024 17:29:52 +0200
> Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> 
> > On Tue, Dec 31, 2024 at 11:40:04AM +0100, Luca Ceresoli wrote:  
> > > This driver implements the point of a DRM pipeline where a connector allows
> > > removal of all the following bridges up to the panel.
> > > 
> > > The DRM subsystem currently allows hotplug of the monitor but not preceding
> > > components. However there are embedded devices where the "tail" of the DRM
> > > pipeline, including one or more bridges, can be physically removed:
> > > 
> > >  .------------------------.
> > >  |   DISPLAY CONTROLLER   |
> > >  | .---------.   .------. |
> > >  | | ENCODER |<--| CRTC | |
> > >  | '---------'   '------' |
> > >  '------|-----------------'
> > >         |
> > >         |               HOTPLUG
> > >         V              CONNECTOR
> > >    .---------.        .--.    .-.        .---------.         .-------.
> > >    | 0 to N  |        | _|   _| |        | 1 to N  |         |       |
> > >    | BRIDGES |--DSI-->||_   |_  |--DSI-->| BRIDGES |--LVDS-->| PANEL |
> > >    |         |        |  |    | |        |         |         |       |
> > >    '---------'        '--'    '-'        '---------'         '-------'
> > > 
> > >  [--- fixed components --]  [----------- removable add-on -----------]
> > > 
> > > This driver supports such a device, where the final segment of a MIPI DSI
> > > bus, including one or more bridges, can be physically disconnected and
> > > reconnected at runtime, possibly with a different model.
> > > 
> > > The add-on supported by this driver has a MIPI DSI bus traversing the
> > > hotplug connector and a DSI to LVDS bridge and an LVDS panel on the add-on.
> > > Hovever this driver is designed to be as far as possible generic and
> > > extendable to other busses that have no native hotplug and model ID
> > > discovery.
> > > 
> > > This driver does not itself add and remove the bridges or panel on the
> > > add-on: this needs to be done by other means, e.g. device tree overlay
> > > runtime insertion and removal. The hotplug-bridge gets notified by the DRM
> > > bridge core after a removable bridge gets added or before it is removed.
> > > 
> > > The hotplug-bridge role is to implement the "hot-pluggable connector" in
> > > the bridge chain. In this position, what the hotplug-bridge should ideally
> > > do is:
> > > 
> > >  * communicate with the previous component (bridge or encoder) so that it
> > >    believes it always has a connected bridge following it and the DRM card
> > >    is always present
> > >  * be notified of the addition and removal of the following bridge and
> > >    attach/detach to/from it
> > >  * communicate with the following bridge so that it will attach and detach
> > >    using the normal procedure (as if the entire pipeline were being created
> > >    or destroyed, not only the tail)
> > >  * instantiate two DRM connectors (similarly to what the DisplayPort MST
> > >    code does):
> > >    - a DSI connector representing the video lines of the hotplug connector;
> > >      the status is always "disconnected" (no panel is ever attached
> > >      directly to it)
> > >    - an LSVD connector representing the classic connection to the panel;
> > >      this gets added/removed whenever the add-on gets
> > >      connected/disconnected; the status is always "connected" as the panel
> > >      is always connected to the preceding bridge    
> > 
> > I'd rather have just a single connector. MST connectors can be added and
> > gone as there is fit, so should be your LVDS panel-related connector.  
> 
> The plan we discussed at LPC 2024 is to eventually get rid of the first
> connector (see "Roadmap and current status" in the cover letter), so
> you can consider this legacy code. However the current implementation
> won't work without this connector, so it is still there for the time
> being. Pointing this out in a note in the commit message of this patch
> would probably be useful to avoid future misunderstanding, so I'm
> adding one for v6.

Reviving this old thread for a specific question I need to clarify.
Before starting a work that I consider far from trivial I'd like to
make sure the requirement is clear.

There was a precise request by both Dmitry and (IIRC) Maxime to remove
the "always present, never connected" DSI connector.

[Recap of previous discussion: skip if unneeded]

The current status is that the hotplug-bridge, which can start without
an add-on plugged, adds a DSI connector unconditionally:

  # modetest -c  | grep -i '^[a-z0-9]'
  Connectors:
  id    encoder status          name        size (mm)     modes   encoders
  38    0       disconnected    DSI-1       0x0           0       37

That DSI connector status is always "unconnected" (in my implementation
at least) because it does never a panel _directly_ attached, only a
further bridge.

Then when the add-on is plugged, which contains a DSI-to-LVDS bridge, a
new LVDS connector is added:

  # modetest -c  | grep -i '^[a-z0-9]'
  Connectors:
  id    encoder status          name        size (mm)     modes   encoders
  38    0       disconnected    DSI-1       0x0           0       37
  39    0       connected       LVDS-1      344x194       1       37

The LVDS connector has a panel attached and provides the modes, so it
is "the connector" in the DRM logic. It is always in "connected" status
because it drives a panel that is always tied to the DSI-to-LVDS bridge.
It is removed when the add-on is removed and so the removable bridge(s)
disappear(s).

The request is to get rid of the DSI connector, because it is not a DRM
connector in the classic DRM sense (DRM connector ~= a modes +
connection status provider). That would mean without addon plugged
there is no DRM connector at all.

However for user space to be able to always have a card we need the
card to be populated even before the addon is plugged and to persist
after its removal. So, a card without any connectors.

[End of recap of previous discussion]

Now comes the question!

Based on the above, I understand that:

 * Current DRM code won't populate a card without at least a DRM
   connector
 * We now need to change the DRM code to allow populating a card,
   and expose it to user space, without a DRM connector
 * The previous bullet is a prerequisite to get rid of DSI connector as
   requested

Is my understanding correct?

Best regards,
Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v5 10/10] drm/bridge: hotplug-bridge: add driver to support hot-pluggable DSI bridges

[Luca Ceresoli]

Hello,

+Cc: Dmitry's current e-mail address

I replied to such an old thread that is had an old address for Dmitry.

On Tue, 9 Sep 2025 17:29:07 +0200
Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:

> Hello Dmitry, Maxime, DRM maintainers,
> 
> On Thu, 2 Jan 2025 13:01:40 +0100
> Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:
> 
> > Hi Dmitry,
> > 
> > On Tue, 31 Dec 2024 17:29:52 +0200
> > Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> >   
> > > On Tue, Dec 31, 2024 at 11:40:04AM +0100, Luca Ceresoli wrote:    
> > > > This driver implements the point of a DRM pipeline where a connector allows
> > > > removal of all the following bridges up to the panel.
> > > > 
> > > > The DRM subsystem currently allows hotplug of the monitor but not preceding
> > > > components. However there are embedded devices where the "tail" of the DRM
> > > > pipeline, including one or more bridges, can be physically removed:
> > > > 
> > > >  .------------------------.
> > > >  |   DISPLAY CONTROLLER   |
> > > >  | .---------.   .------. |
> > > >  | | ENCODER |<--| CRTC | |
> > > >  | '---------'   '------' |
> > > >  '------|-----------------'
> > > >         |
> > > >         |               HOTPLUG
> > > >         V              CONNECTOR
> > > >    .---------.        .--.    .-.        .---------.         .-------.
> > > >    | 0 to N  |        | _|   _| |        | 1 to N  |         |       |
> > > >    | BRIDGES |--DSI-->||_   |_  |--DSI-->| BRIDGES |--LVDS-->| PANEL |
> > > >    |         |        |  |    | |        |         |         |       |
> > > >    '---------'        '--'    '-'        '---------'         '-------'
> > > > 
> > > >  [--- fixed components --]  [----------- removable add-on -----------]
> > > > 
> > > > This driver supports such a device, where the final segment of a MIPI DSI
> > > > bus, including one or more bridges, can be physically disconnected and
> > > > reconnected at runtime, possibly with a different model.
> > > > 
> > > > The add-on supported by this driver has a MIPI DSI bus traversing the
> > > > hotplug connector and a DSI to LVDS bridge and an LVDS panel on the add-on.
> > > > Hovever this driver is designed to be as far as possible generic and
> > > > extendable to other busses that have no native hotplug and model ID
> > > > discovery.
> > > > 
> > > > This driver does not itself add and remove the bridges or panel on the
> > > > add-on: this needs to be done by other means, e.g. device tree overlay
> > > > runtime insertion and removal. The hotplug-bridge gets notified by the DRM
> > > > bridge core after a removable bridge gets added or before it is removed.
> > > > 
> > > > The hotplug-bridge role is to implement the "hot-pluggable connector" in
> > > > the bridge chain. In this position, what the hotplug-bridge should ideally
> > > > do is:
> > > > 
> > > >  * communicate with the previous component (bridge or encoder) so that it
> > > >    believes it always has a connected bridge following it and the DRM card
> > > >    is always present
> > > >  * be notified of the addition and removal of the following bridge and
> > > >    attach/detach to/from it
> > > >  * communicate with the following bridge so that it will attach and detach
> > > >    using the normal procedure (as if the entire pipeline were being created
> > > >    or destroyed, not only the tail)
> > > >  * instantiate two DRM connectors (similarly to what the DisplayPort MST
> > > >    code does):
> > > >    - a DSI connector representing the video lines of the hotplug connector;
> > > >      the status is always "disconnected" (no panel is ever attached
> > > >      directly to it)
> > > >    - an LSVD connector representing the classic connection to the panel;
> > > >      this gets added/removed whenever the add-on gets
> > > >      connected/disconnected; the status is always "connected" as the panel
> > > >      is always connected to the preceding bridge      
> > > 
> > > I'd rather have just a single connector. MST connectors can be added and
> > > gone as there is fit, so should be your LVDS panel-related connector.    
> > 
> > The plan we discussed at LPC 2024 is to eventually get rid of the first
> > connector (see "Roadmap and current status" in the cover letter), so
> > you can consider this legacy code. However the current implementation
> > won't work without this connector, so it is still there for the time
> > being. Pointing this out in a note in the commit message of this patch
> > would probably be useful to avoid future misunderstanding, so I'm
> > adding one for v6.  
> 
> Reviving this old thread for a specific question I need to clarify.
> Before starting a work that I consider far from trivial I'd like to
> make sure the requirement is clear.
> 
> There was a precise request by both Dmitry and (IIRC) Maxime to remove
> the "always present, never connected" DSI connector.
> 
> [Recap of previous discussion: skip if unneeded]
> 
> The current status is that the hotplug-bridge, which can start without
> an add-on plugged, adds a DSI connector unconditionally:
> 
>   # modetest -c  | grep -i '^[a-z0-9]'
>   Connectors:
>   id    encoder status          name        size (mm)     modes   encoders
>   38    0       disconnected    DSI-1       0x0           0       37
> 
> That DSI connector status is always "unconnected" (in my implementation
> at least) because it does never a panel _directly_ attached, only a
> further bridge.
> 
> Then when the add-on is plugged, which contains a DSI-to-LVDS bridge, a
> new LVDS connector is added:
> 
>   # modetest -c  | grep -i '^[a-z0-9]'
>   Connectors:
>   id    encoder status          name        size (mm)     modes   encoders
>   38    0       disconnected    DSI-1       0x0           0       37
>   39    0       connected       LVDS-1      344x194       1       37
> 
> The LVDS connector has a panel attached and provides the modes, so it
> is "the connector" in the DRM logic. It is always in "connected" status
> because it drives a panel that is always tied to the DSI-to-LVDS bridge.
> It is removed when the add-on is removed and so the removable bridge(s)
> disappear(s).
> 
> The request is to get rid of the DSI connector, because it is not a DRM
> connector in the classic DRM sense (DRM connector ~= a modes +
> connection status provider). That would mean without addon plugged
> there is no DRM connector at all.
> 
> However for user space to be able to always have a card we need the
> card to be populated even before the addon is plugged and to persist
> after its removal. So, a card without any connectors.
> 
> [End of recap of previous discussion]
> 
> Now comes the question!
> 
> Based on the above, I understand that:
> 
>  * Current DRM code won't populate a card without at least a DRM
>    connector
>  * We now need to change the DRM code to allow populating a card,
>    and expose it to user space, without a DRM connector
>  * The previous bullet is a prerequisite to get rid of DSI connector as
>    requested
> 
> Is my understanding correct?
> 
> Best regards,
> Luca
> 



-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com