Re: [PATCH RFC v2 05/19] drm/panel: embed a drm_bridge into every drm_panel

["Luca Ceresoli" <luca.ceresoli@bootlin.com>]

Hi Maxime,

thanks for keeping the round-trip time short for this discussion!

On Thu Sep 3, 2026 at 12:22 PM CEST, Maxime Ripard wrote:
> On Thu, Sep 03, 2026 at 10:11:08AM +0200, Luca Ceresoli wrote:
>> Adding a drm_panel does currently not add a panel_bridge wrapping
>> it. Usually the panel_bridge creation happens later, when some other driver
>> (e.g. the previous bridge or the encoder) calls *_of_get_bridge() and the
>> following element in the pipeline is a panel.
>>
>> This has some drawbacks:
>>
>>  * the bridge API is currently the best practice to access various
>>    components of the pipeline, especially with complex cards where bridges
>>    can be combined in different ways on different hardware
>>  * the panel_bridge is not created in the context of the driver of the
>>    underlying physical device (the panel driver), but of some other driver
>>  * that other driver is not aware of whether the returned drm_bridge
>>    pointer is a panel_bridge created on the fly, a pre-existing
>>    panel_bridge or a non-panel bridge
>>  * removal of a panel_bridge requires calling drm_panel_bridge_remove(),
>>    but that other driver doesn't know whether this is needed because it
>>    doesn't know whether it has created a panel_bridge or not
>>
>> Other drivers call [a variant of] drm_panel_bridge_add(), which also has
>> some of the above drawbacks.
>>
>> So far the current approach was working mostly because devm and drmm ensure
>> the panel bridge would be dealloacted at some later point. However with the
>> upcoming implementation of bridge hotplug and dynamic bridge lifetime this
>> will get more complicated.
>>
>> Switch to the new approach: embed a drm_bridge inside every drm_panel,
>> which behaves just like the current drm_panel_bridge.
>>
>> Do this by copying and adapting the code from bridge/panel.c, using
>> function names that are more suitable within drm_panel.c and doing the
>> minimal adaptation needed.
>>
>> Currently drm_bridge and drm_panel have independent refcounted
>> allocation. As they now become a single struct, just change
>> drm_panel_get/put() to get/put the bridge. As a result, the refcount for a
>> drm_bridge embedded in a drm_panel is:
>>
>>   bridge.refcount == number of drm_bridge_get() calls
>>                    + number of drm_panel_get() calls
>>                    - number of drm_bridge_put() calls
>>                    - number of drm_panel_put() calls
>>
>> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
>
> So there's a lot to unravel, and I wished you had split it, but I can't
> find a good way to split it either.

As I see it, this could be maybe split in 2:

 - copy all needed symbols from bridge/panel.c (or move or whatever, see
   discussion below) without using them
 - change _alloc, _get, _put etc to actually embed the bridge and start
   using the copied/moved symbols

Would you prefer that?

Note the second bullet, which is the core change, is pretty small, I think
a <100 lines diff, but I don't think it can be split further.

>> This patch is new in v2, and replaces "drm/bridge: panel: add a
>> panel_bridge to every panel" which was based on a different approach.
>> ---
>>  drivers/gpu/drm/drm_panel.c | 258 +++++++++++++++++++++++++++++++++++++++++---
>>  include/drm/drm_panel.h     |  33 ++++--
>>  2 files changed, 266 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
>> index f8f6082e637f..9b86195f9f66 100644
>> --- a/drivers/gpu/drm/drm_panel.c
>> +++ b/drivers/gpu/drm/drm_panel.c
>> @@ -22,15 +22,19 @@
>>   */
>>
>>  #include <linux/backlight.h>
>> +#include <linux/debugfs.h>
>>  #include <linux/err.h>
>>  #include <linux/export.h>
>>  #include <linux/module.h>
>>  #include <linux/of.h>
>>
>> +#include <drm/drm_atomic_helper.h>
>>  #include <drm/drm_crtc.h>
>> +#include <drm/drm_modeset_helper_vtables.h>
>>  #include <drm/drm_of.h>
>>  #include <drm/drm_panel.h>
>>  #include <drm/drm_print.h>
>> +#include <drm/drm_probe_helper.h>
>>
>>  static DEFINE_MUTEX(panel_lock);
>>  static LIST_HEAD(panel_list);
>> @@ -46,6 +50,18 @@ static LIST_HEAD(panel_list);
>>   * take look at drm_panel_bridge_add() and devm_drm_panel_bridge_add().
>>   */
>>
>> +static inline struct drm_panel *
>> +drm_bridge_to_panel(const struct drm_bridge *bridge)
>> +{
>> +	return container_of(bridge, struct drm_panel, bridge);
>> +}
>> +
>> +static inline struct drm_panel *
>> +drm_connector_to_panel(const struct drm_connector *connector)
>> +{
>> +	return container_of(connector, struct drm_panel, connector);
>> +}
>> +
>>  /**
>>   * drm_panel_init - initialize a panel
>>   * @panel: DRM panel
>> @@ -86,6 +102,7 @@ void drm_panel_add(struct drm_panel *panel)
>>  	mutex_lock(&panel_lock);
>>  	list_add_tail(&panel->list, &panel_list);
>>  	mutex_unlock(&panel_lock);
>> +	drm_bridge_add(&panel->bridge);
>>  }
>>  EXPORT_SYMBOL(drm_panel_add);
>>
>> @@ -97,6 +114,7 @@ EXPORT_SYMBOL(drm_panel_add);
>>   */
>>  void drm_panel_remove(struct drm_panel *panel)
>>  {
>> +	drm_bridge_remove(&panel->bridge);
>>  	mutex_lock(&panel_lock);
>>  	list_del_init(&panel->list);
>>  	mutex_unlock(&panel_lock);
>> @@ -370,13 +388,198 @@ int drm_panel_get_modes(struct drm_panel *panel,
>>  }
>>  EXPORT_SYMBOL(drm_panel_get_modes);
>>
>> -static void __drm_panel_free(struct kref *kref)
>> +static int drm_panel_bridge_connector_get_modes(struct drm_connector *connector)
>> +{
>> +	struct drm_panel *drm_panel = drm_connector_to_panel(connector);
>> +
>> +	return drm_panel_get_modes(drm_panel, connector);
>> +}
>> +
>> +/**
>> + * drm_bridge_set_connector_orientation - Set the connector panel
>> + * orientation from the bridge that can be transformed to drm_panel.
>> + *
>> + * @bridge: The drm_bridge for a drm_panel.
>> + * @connector: The connector to be set panel orientation.
>> + *
>> + * Returns 0 on success, negative errno on failure.
>> + */
>> +int drm_bridge_set_connector_orientation(const struct drm_bridge *bridge,
>> +					 struct drm_connector *connector)
>> +{
>> +	struct drm_panel *panel = drm_bridge_to_panel(bridge);
>> +
>> +	return drm_connector_set_orientation_from_panel(connector, panel);
>> +}
>> +EXPORT_SYMBOL(drm_bridge_set_connector_orientation);
>
> I don't think we should create new ones. Just move the code from
> bridge/panel here and remove it there. The only thing left will be the
> panel_bridge_add() variants that become almost trivial now, and you can
> cleanup the drivers in later patches.

What do you mean by "move" exactly? Really move all relevant functions from
bridge/panel.c into drm_panel.c, so evey user driver will depend on the new
drm_panel module instead of the old one for these symbols?

> This also allows to get rid of all the symbol renaming, which isn't
> great in itself, but also the existing names were good so it's hard to
> come with better ones.

I think the names in bridge/panel.c make sense there, but not in
drm_panel.c. Here's my interpretation (using atomic_enable as an example):

 - panel_bridge_atomic_enable
   ^^^^^^^^^^^^ <- this is about the panel_bridge

 - drm_panel_bridge_atomic_enable
   ^^^^^^^^^        <- this is drm_panel stuff
             ^^^^^^ <- and specifically about the bridge embedded in drm_panel

Does it make sense to you too in this perspective?

Luca

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