[PATCH 0/3] drm/bridge: Create a function to abstract panels away

[PATCH 0/3] drm/bridge: Create a function to abstract panels away

[Maxime Ripard]

Hi,

This series used to be part of the DSI probe order series, but got removed
since it wasn't useful there anymore.

However, I still believe there is value in moving towards merging bridges and
panels by only making encoder (or upstream bridges) manipulate bridges.

The first patch creates a new helper that does just this by looking for a
bridge and a panel, and if a panel is found create a panel_bridge to return
that bridge instead.

The next two patches convert the vc4 encoders to use it.

If it's accepted, I plan on converting all the relevant users over time.

Let me know what you think,
Maxime

Maxime Ripard (3):
  drm/bridge: Add a function to abstract away panels
  drm/vc4: dpi: Switch to devm_drm_of_get_bridge
  drm/vc4: dsi: Switch to devm_drm_of_get_bridge

 drivers/gpu/drm/drm_bridge.c  | 42 +++++++++++++++++++++++++++++++----
 drivers/gpu/drm/drm_of.c      |  3 +++
 drivers/gpu/drm/vc4/vc4_dpi.c | 15 ++++---------
 drivers/gpu/drm/vc4/vc4_drv.c |  2 ++
 drivers/gpu/drm/vc4/vc4_dsi.c | 28 ++++-------------------
 include/drm/drm_bridge.h      |  2 ++
 6 files changed, 53 insertions(+), 39 deletions(-)

-- 
2.31.1

[PATCH 1/3] drm/bridge: Add a function to abstract away panels

[Maxime Ripard]

Display drivers so far need to have a lot of boilerplate to first
retrieve either the panel or bridge that they are connected to using
drm_of_find_panel_or_bridge(), and then either deal with each with ad-hoc
functions or create a drm panel bridge through drm_panel_bridge_add.

In order to reduce the boilerplate and hopefully create a path of least
resistance towards using the DRM panel bridge layer, let's create the
function devm_drm_of_get_next to reduce that boilerplate.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/drm_bridge.c | 42 ++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/drm_of.c     |  3 +++
 include/drm/drm_bridge.h     |  2 ++
 3 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index a8ed66751c2d..10ddca4638b0 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -28,6 +28,7 @@
 #include <drm/drm_atomic_state_helper.h>
 #include <drm/drm_bridge.h>
 #include <drm/drm_encoder.h>
+#include <drm/drm_of.h>
 #include <drm/drm_print.h>
 
 #include "drm_crtc_internal.h"
@@ -51,10 +52,8 @@
  *
  * Display drivers are responsible for linking encoders with the first bridge
  * in the chains. This is done by acquiring the appropriate bridge with
- * of_drm_find_bridge() or drm_of_find_panel_or_bridge(), or creating it for a
- * panel with drm_panel_bridge_add_typed() (or the managed version
- * devm_drm_panel_bridge_add_typed()). Once acquired, the bridge shall be
- * attached to the encoder with a call to drm_bridge_attach().
+ * devm_drm_of_get_bridge(). Once acquired, the bridge shall be attached to the
+ * encoder with a call to drm_bridge_attach().
  *
  * Bridges are responsible for linking themselves with the next bridge in the
  * chain, if any. This is done the same way as for encoders, with the call to
@@ -1233,6 +1232,41 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
 	return NULL;
 }
 EXPORT_SYMBOL(of_drm_find_bridge);
+
+/**
+ * devm_drm_of_get_bridge - Return next bridge in the chain
+ * @dev: device to tie the bridge lifetime to
+ * @np: device tree node containing encoder output ports
+ * @port: port in the device tree node
+ * @endpoint: endpoint in the device tree node
+ *
+ * Given a DT node's port and endpoint number, finds the connected node
+ * and returns the associated bridge if any, or creates and returns a
+ * drm panel bridge instance if a panel is connected.
+ *
+ * Returns a pointer to the bridge if successful, or an error pointer
+ * otherwise.
+ */
+struct drm_bridge *devm_drm_of_get_bridge(struct device *dev,
+					  struct device_node *np,
+					  unsigned int port,
+					  unsigned int endpoint)
+{
+	struct drm_bridge *bridge;
+	struct drm_panel *panel;
+	int ret;
+
+	ret = drm_of_find_panel_or_bridge(np, port, endpoint,
+					  &panel, &bridge);
+	if (ret)
+		return ERR_PTR(ret);
+
+	if (panel)
+		bridge = devm_drm_panel_bridge_add(dev, panel);
+
+	return bridge;
+}
+EXPORT_SYMBOL(devm_drm_of_get_bridge);
 #endif
 
 MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>");
diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index 997b8827fed2..37c34146eea8 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -231,6 +231,9 @@ EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint);
  * return either the associated struct drm_panel or drm_bridge device. Either
  * @panel or @bridge must not be NULL.
  *
+ * This function is deprecated and should not be used in new drivers. Use
+ * devm_drm_of_get_bridge() instead.
+ *
  * Returns zero if successful, or one of the standard error codes if it fails.
  */
 int drm_of_find_panel_or_bridge(const struct device_node *np,
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 46bdfa48c413..f70c88ca96ef 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -911,6 +911,8 @@ struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
 struct drm_bridge *devm_drm_panel_bridge_add_typed(struct device *dev,
 						   struct drm_panel *panel,
 						   u32 connector_type);
+struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, struct device_node *node,
+					unsigned int port, unsigned int endpoint);
 struct drm_connector *drm_panel_bridge_connector(struct drm_bridge *bridge);
 #endif
 
-- 
2.31.1

[PATCH 2/3] drm/vc4: dpi: Switch to devm_drm_of_get_bridge

[Maxime Ripard]

The new devm_drm_of_get_bridge removes most of the boilerplate we
have to deal with. Let's switch to it.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_dpi.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dpi.c b/drivers/gpu/drm/vc4/vc4_dpi.c
index a90f2545baee..c180eb60bee8 100644
--- a/drivers/gpu/drm/vc4/vc4_dpi.c
+++ b/drivers/gpu/drm/vc4/vc4_dpi.c
@@ -229,26 +229,19 @@ static const struct of_device_id vc4_dpi_dt_match[] = {
 static int vc4_dpi_init_bridge(struct vc4_dpi *dpi)
 {
 	struct device *dev = &dpi->pdev->dev;
-	struct drm_panel *panel;
 	struct drm_bridge *bridge;
-	int ret;
 
-	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,
-					  &panel, &bridge);
-	if (ret) {
+	bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0);
+	if (IS_ERR(bridge)) {
 		/* If nothing was connected in the DT, that's not an
 		 * error.
 		 */
-		if (ret == -ENODEV)
+		if (PTR_ERR(bridge) == -ENODEV)
 			return 0;
 		else
-			return ret;
+			return PTR_ERR(bridge);
 	}
 
-	if (panel)
-		bridge = drm_panel_bridge_add_typed(panel,
-						    DRM_MODE_CONNECTOR_DPI);
-
 	return drm_bridge_attach(dpi->encoder, bridge, NULL, 0);
 }
 
-- 
2.31.1

[PATCH 3/3] drm/vc4: dsi: Switch to devm_drm_of_get_bridge

[Maxime Ripard]

The new devm_drm_of_get_bridge removes most of the boilerplate we
have to deal with. Let's switch to it.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_drv.c |  2 ++
 drivers/gpu/drm/vc4/vc4_dsi.c | 28 ++++------------------------
 2 files changed, 6 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index 16abc3a3d601..96c526f1022e 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -25,7 +25,9 @@
 #include <linux/device.h>
 #include <linux/dma-mapping.h>
 #include <linux/io.h>
+#include <linux/i2c.h>
 #include <linux/module.h>
+#include <linux/of_graph.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index a185027911ce..a229da58962a 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -1497,7 +1497,6 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
 	struct drm_device *drm = dev_get_drvdata(master);
 	struct vc4_dsi *dsi = dev_get_drvdata(dev);
 	struct vc4_dsi_encoder *vc4_dsi_encoder;
-	struct drm_panel *panel;
 	const struct of_device_id *match;
 	dma_cap_mask_t dma_mask;
 	int ret;
@@ -1609,27 +1608,9 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
 		return ret;
 	}
 
-	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,
-					  &panel, &dsi->bridge);
-	if (ret) {
-		/* If the bridge or panel pointed by dev->of_node is not
-		 * enabled, just return 0 here so that we don't prevent the DRM
-		 * dev from being registered. Of course that means the DSI
-		 * encoder won't be exposed, but that's not a problem since
-		 * nothing is connected to it.
-		 */
-		if (ret == -ENODEV)
-			return 0;
-
-		return ret;
-	}
-
-	if (panel) {
-		dsi->bridge = devm_drm_panel_bridge_add_typed(dev, panel,
-							      DRM_MODE_CONNECTOR_DSI);
-		if (IS_ERR(dsi->bridge))
-			return PTR_ERR(dsi->bridge);
-	}
+	dsi->bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0);
+	if (IS_ERR(dsi->bridge))
+		return PTR_ERR(dsi->bridge);
 
 	/* The esc clock rate is supposed to always be 100Mhz. */
 	ret = clk_set_rate(dsi->escape_clock, 100 * 1000000);
@@ -1667,8 +1648,7 @@ static void vc4_dsi_unbind(struct device *dev, struct device *master,
 {
 	struct vc4_dsi *dsi = dev_get_drvdata(dev);
 
-	if (dsi->bridge)
-		pm_runtime_disable(dev);
+	pm_runtime_disable(dev);
 
 	/*
 	 * Restore the bridge_chain so the bridge detach procedure can happen
-- 
2.31.1

Re: [PATCH 1/3] drm/bridge: Add a function to abstract away panels

[Sam Ravnborg]

Hi Maxime,
On Fri, Sep 10, 2021 at 03:09:39PM +0200, Maxime Ripard wrote:
> Display drivers so far need to have a lot of boilerplate to first
> retrieve either the panel or bridge that they are connected to using
> drm_of_find_panel_or_bridge(), and then either deal with each with ad-hoc
> functions or create a drm panel bridge through drm_panel_bridge_add.
> 
> In order to reduce the boilerplate and hopefully create a path of least
> resistance towards using the DRM panel bridge layer, let's create the
> function devm_drm_of_get_next to reduce that boilerplate.
s/devm_drm_of_get_next/devm_drm_of_get_bridge/

> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

I like how this makes panels and bridges much more alike for the display
drivers.

With the changelog fixed:
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

(I assume you will apply yourself - otherwise tell me to do it).
> ---
>  drivers/gpu/drm/drm_bridge.c | 42 ++++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/drm_of.c     |  3 +++
>  include/drm/drm_bridge.h     |  2 ++
>  3 files changed, 43 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index a8ed66751c2d..10ddca4638b0 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -28,6 +28,7 @@
>  #include <drm/drm_atomic_state_helper.h>
>  #include <drm/drm_bridge.h>
>  #include <drm/drm_encoder.h>
> +#include <drm/drm_of.h>
>  #include <drm/drm_print.h>
>  
>  #include "drm_crtc_internal.h"
> @@ -51,10 +52,8 @@
>   *
>   * Display drivers are responsible for linking encoders with the first bridge
>   * in the chains. This is done by acquiring the appropriate bridge with
> - * of_drm_find_bridge() or drm_of_find_panel_or_bridge(), or creating it for a
> - * panel with drm_panel_bridge_add_typed() (or the managed version
> - * devm_drm_panel_bridge_add_typed()). Once acquired, the bridge shall be
> - * attached to the encoder with a call to drm_bridge_attach().
> + * devm_drm_of_get_bridge(). Once acquired, the bridge shall be attached to the
> + * encoder with a call to drm_bridge_attach().
>   *
>   * Bridges are responsible for linking themselves with the next bridge in the
>   * chain, if any. This is done the same way as for encoders, with the call to
> @@ -1233,6 +1232,41 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
>  	return NULL;
>  }
>  EXPORT_SYMBOL(of_drm_find_bridge);
> +
> +/**
> + * devm_drm_of_get_bridge - Return next bridge in the chain
> + * @dev: device to tie the bridge lifetime to
> + * @np: device tree node containing encoder output ports
> + * @port: port in the device tree node
> + * @endpoint: endpoint in the device tree node
> + *
> + * Given a DT node's port and endpoint number, finds the connected node
> + * and returns the associated bridge if any, or creates and returns a
> + * drm panel bridge instance if a panel is connected.
> + *
> + * Returns a pointer to the bridge if successful, or an error pointer
> + * otherwise.
> + */
> +struct drm_bridge *devm_drm_of_get_bridge(struct device *dev,
> +					  struct device_node *np,
> +					  unsigned int port,
> +					  unsigned int endpoint)
> +{
> +	struct drm_bridge *bridge;
> +	struct drm_panel *panel;
> +	int ret;
> +
> +	ret = drm_of_find_panel_or_bridge(np, port, endpoint,
> +					  &panel, &bridge);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	if (panel)
> +		bridge = devm_drm_panel_bridge_add(dev, panel);
> +
> +	return bridge;
> +}
> +EXPORT_SYMBOL(devm_drm_of_get_bridge);
>  #endif
>  
>  MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>");
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 997b8827fed2..37c34146eea8 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -231,6 +231,9 @@ EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint);
>   * return either the associated struct drm_panel or drm_bridge device. Either
>   * @panel or @bridge must not be NULL.
>   *
> + * This function is deprecated and should not be used in new drivers. Use
> + * devm_drm_of_get_bridge() instead.
> + *
>   * Returns zero if successful, or one of the standard error codes if it fails.
>   */
>  int drm_of_find_panel_or_bridge(const struct device_node *np,
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 46bdfa48c413..f70c88ca96ef 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -911,6 +911,8 @@ struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
>  struct drm_bridge *devm_drm_panel_bridge_add_typed(struct device *dev,
>  						   struct drm_panel *panel,
>  						   u32 connector_type);
> +struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, struct device_node *node,
> +					unsigned int port, unsigned int endpoint);
>  struct drm_connector *drm_panel_bridge_connector(struct drm_bridge *bridge);
>  #endif
>  
> -- 
> 2.31.1

Re: [PATCH 2/3] drm/vc4: dpi: Switch to devm_drm_of_get_bridge

[Sam Ravnborg]

On Fri, Sep 10, 2021 at 03:09:40PM +0200, Maxime Ripard wrote:
> The new devm_drm_of_get_bridge removes most of the boilerplate we
> have to deal with. Let's switch to it.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
Acked-by: Sam Ravnborg <sam@ravnborg.org>
> ---
>  drivers/gpu/drm/vc4/vc4_dpi.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_dpi.c b/drivers/gpu/drm/vc4/vc4_dpi.c
> index a90f2545baee..c180eb60bee8 100644
> --- a/drivers/gpu/drm/vc4/vc4_dpi.c
> +++ b/drivers/gpu/drm/vc4/vc4_dpi.c
> @@ -229,26 +229,19 @@ static const struct of_device_id vc4_dpi_dt_match[] = {
>  static int vc4_dpi_init_bridge(struct vc4_dpi *dpi)
>  {
>  	struct device *dev = &dpi->pdev->dev;
> -	struct drm_panel *panel;
>  	struct drm_bridge *bridge;
> -	int ret;
>  
> -	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,
> -					  &panel, &bridge);
> -	if (ret) {
> +	bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0);
> +	if (IS_ERR(bridge)) {
>  		/* If nothing was connected in the DT, that's not an
>  		 * error.
>  		 */
> -		if (ret == -ENODEV)
> +		if (PTR_ERR(bridge) == -ENODEV)
>  			return 0;
>  		else
> -			return ret;
> +			return PTR_ERR(bridge);
>  	}
>  
> -	if (panel)
> -		bridge = drm_panel_bridge_add_typed(panel,
> -						    DRM_MODE_CONNECTOR_DPI);
> -
>  	return drm_bridge_attach(dpi->encoder, bridge, NULL, 0);
>  }
>  
> -- 
> 2.31.1

Re: [PATCH 3/3] drm/vc4: dsi: Switch to devm_drm_of_get_bridge

[Sam Ravnborg]

On Fri, Sep 10, 2021 at 03:09:41PM +0200, Maxime Ripard wrote:
> The new devm_drm_of_get_bridge removes most of the boilerplate we
> have to deal with. Let's switch to it.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

With the includes updated:

Acked-by: Sam Ravnborg <sam@ravnborg.org>

	Sam

> ---
>  drivers/gpu/drm/vc4/vc4_drv.c |  2 ++
>  drivers/gpu/drm/vc4/vc4_dsi.c | 28 ++++------------------------
>  2 files changed, 6 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
> index 16abc3a3d601..96c526f1022e 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.c
> +++ b/drivers/gpu/drm/vc4/vc4_drv.c
> @@ -25,7 +25,9 @@
>  #include <linux/device.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/io.h>
> +#include <linux/i2c.h>
I fail to see why this include is needed?

>  #include <linux/module.h>
> +#include <linux/of_graph.h>
I think this should be drm_bridge.h.
And drm_bridge.h hopefully pulls in what is needed.

>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> index a185027911ce..a229da58962a 100644
> --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
> @@ -1497,7 +1497,6 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
>  	struct drm_device *drm = dev_get_drvdata(master);
>  	struct vc4_dsi *dsi = dev_get_drvdata(dev);
>  	struct vc4_dsi_encoder *vc4_dsi_encoder;
> -	struct drm_panel *panel;
>  	const struct of_device_id *match;
>  	dma_cap_mask_t dma_mask;
>  	int ret;
> @@ -1609,27 +1608,9 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
>  		return ret;
>  	}
>  
> -	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,
> -					  &panel, &dsi->bridge);
> -	if (ret) {
> -		/* If the bridge or panel pointed by dev->of_node is not
> -		 * enabled, just return 0 here so that we don't prevent the DRM
> -		 * dev from being registered. Of course that means the DSI
> -		 * encoder won't be exposed, but that's not a problem since
> -		 * nothing is connected to it.
> -		 */
> -		if (ret == -ENODEV)
> -			return 0;
> -
> -		return ret;
> -	}
> -
> -	if (panel) {
> -		dsi->bridge = devm_drm_panel_bridge_add_typed(dev, panel,
> -							      DRM_MODE_CONNECTOR_DSI);
> -		if (IS_ERR(dsi->bridge))
> -			return PTR_ERR(dsi->bridge);
> -	}
> +	dsi->bridge = devm_drm_of_get_bridge(dev, dev->of_node, 0, 0);
> +	if (IS_ERR(dsi->bridge))
> +		return PTR_ERR(dsi->bridge);
>  
>  	/* The esc clock rate is supposed to always be 100Mhz. */
>  	ret = clk_set_rate(dsi->escape_clock, 100 * 1000000);
> @@ -1667,8 +1648,7 @@ static void vc4_dsi_unbind(struct device *dev, struct device *master,
>  {
>  	struct vc4_dsi *dsi = dev_get_drvdata(dev);
>  
> -	if (dsi->bridge)
> -		pm_runtime_disable(dev);
> +	pm_runtime_disable(dev);
>  
>  	/*
>  	 * Restore the bridge_chain so the bridge detach procedure can happen
> -- 
> 2.31.1

Re: [PATCH 1/3] drm/bridge: Add a function to abstract away panels

[Sam Ravnborg]

On Fri, Sep 10, 2021 at 03:09:39PM +0200, Maxime Ripard wrote:
> Display drivers so far need to have a lot of boilerplate to first
> retrieve either the panel or bridge that they are connected to using
> drm_of_find_panel_or_bridge(), and then either deal with each with ad-hoc
> functions or create a drm panel bridge through drm_panel_bridge_add.
> 
> In order to reduce the boilerplate and hopefully create a path of least
> resistance towards using the DRM panel bridge layer, let's create the
> function devm_drm_of_get_next to reduce that boilerplate.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/gpu/drm/drm_bridge.c | 42 ++++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/drm_of.c     |  3 +++
>  include/drm/drm_bridge.h     |  2 ++
>  3 files changed, 43 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index a8ed66751c2d..10ddca4638b0 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -28,6 +28,7 @@
>  #include <drm/drm_atomic_state_helper.h>
>  #include <drm/drm_bridge.h>
>  #include <drm/drm_encoder.h>
> +#include <drm/drm_of.h>
>  #include <drm/drm_print.h>
>  
>  #include "drm_crtc_internal.h"
> @@ -51,10 +52,8 @@
>   *
>   * Display drivers are responsible for linking encoders with the first bridge
>   * in the chains. This is done by acquiring the appropriate bridge with
> - * of_drm_find_bridge() or drm_of_find_panel_or_bridge(), or creating it for a
> - * panel with drm_panel_bridge_add_typed() (or the managed version
> - * devm_drm_panel_bridge_add_typed()). Once acquired, the bridge shall be
> - * attached to the encoder with a call to drm_bridge_attach().
> + * devm_drm_of_get_bridge(). Once acquired, the bridge shall be attached to the
> + * encoder with a call to drm_bridge_attach().
>   *
>   * Bridges are responsible for linking themselves with the next bridge in the
>   * chain, if any. This is done the same way as for encoders, with the call to
> @@ -1233,6 +1232,41 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
>  	return NULL;
>  }
>  EXPORT_SYMBOL(of_drm_find_bridge);
> +
> +/**
> + * devm_drm_of_get_bridge - Return next bridge in the chain
> + * @dev: device to tie the bridge lifetime to
> + * @np: device tree node containing encoder output ports
> + * @port: port in the device tree node
> + * @endpoint: endpoint in the device tree node
> + *
> + * Given a DT node's port and endpoint number, finds the connected node
> + * and returns the associated bridge if any, or creates and returns a
> + * drm panel bridge instance if a panel is connected.
> + *
> + * Returns a pointer to the bridge if successful, or an error pointer
> + * otherwise.
> + */
> +struct drm_bridge *devm_drm_of_get_bridge(struct device *dev,
> +					  struct device_node *np,
> +					  unsigned int port,
> +					  unsigned int endpoint)
One extra comment.
In of_graph_get_remote_node() port is u32
In drm_of_find_panel_or_bridge() port is int
Here port is unsigned int

My OCD would benefit if we used u32 here - as this is consistent with
the base function of_graph_get_remote_node().

	Sam


> +{
> +	struct drm_bridge *bridge;
> +	struct drm_panel *panel;
> +	int ret;
> +
> +	ret = drm_of_find_panel_or_bridge(np, port, endpoint,
> +					  &panel, &bridge);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	if (panel)
> +		bridge = devm_drm_panel_bridge_add(dev, panel);
> +
> +	return bridge;
> +}
> +EXPORT_SYMBOL(devm_drm_of_get_bridge);
>  #endif
>  
>  MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>");
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 997b8827fed2..37c34146eea8 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -231,6 +231,9 @@ EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint);
>   * return either the associated struct drm_panel or drm_bridge device. Either
>   * @panel or @bridge must not be NULL.
>   *
> + * This function is deprecated and should not be used in new drivers. Use
> + * devm_drm_of_get_bridge() instead.
> + *
>   * Returns zero if successful, or one of the standard error codes if it fails.
>   */
>  int drm_of_find_panel_or_bridge(const struct device_node *np,
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 46bdfa48c413..f70c88ca96ef 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -911,6 +911,8 @@ struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
>  struct drm_bridge *devm_drm_panel_bridge_add_typed(struct device *dev,
>  						   struct drm_panel *panel,
>  						   u32 connector_type);
> +struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, struct device_node *node,
> +					unsigned int port, unsigned int endpoint);
>  struct drm_connector *drm_panel_bridge_connector(struct drm_bridge *bridge);
>  #endif
>  
> -- 
> 2.31.1

Re: [PATCH 3/3] drm/vc4: dsi: Switch to devm_drm_of_get_bridge

[Maxime Ripard]

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

Hi,

On Sun, Sep 12, 2021 at 09:46:46PM +0200, Sam Ravnborg wrote:
> On Fri, Sep 10, 2021 at 03:09:41PM +0200, Maxime Ripard wrote:
> > The new devm_drm_of_get_bridge removes most of the boilerplate we
> > have to deal with. Let's switch to it.
> > 
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> 
> With the includes updated:

I'm not sure why they were even here to begin with, I've removed them.

> Acked-by: Sam Ravnborg <sam@ravnborg.org>

Applied the series with your comments addressed, thanks!
Maxime

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

Re: [PATCH 1/3] drm/bridge: Add a function to abstract away panels

[Patrik Jakobsson]

On Fri, Sep 10, 2021 at 3:10 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> Display drivers so far need to have a lot of boilerplate to first
> retrieve either the panel or bridge that they are connected to using
> drm_of_find_panel_or_bridge(), and then either deal with each with ad-hoc
> functions or create a drm panel bridge through drm_panel_bridge_add.
>
> In order to reduce the boilerplate and hopefully create a path of least
> resistance towards using the DRM panel bridge layer, let's create the
> function devm_drm_of_get_next to reduce that boilerplate.

Hi Maxime,

This patch creates a dependency error for me:

depmod: ERROR: Cycle detected: drm_kms_helper -> drm -> drm_kms_helper
depmod: ERROR: Cycle detected: cec
depmod: ERROR: Found 2 modules in dependency cycles!

Cheers
Patrik

>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/gpu/drm/drm_bridge.c | 42 ++++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/drm_of.c     |  3 +++
>  include/drm/drm_bridge.h     |  2 ++
>  3 files changed, 43 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index a8ed66751c2d..10ddca4638b0 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -28,6 +28,7 @@
>  #include <drm/drm_atomic_state_helper.h>
>  #include <drm/drm_bridge.h>
>  #include <drm/drm_encoder.h>
> +#include <drm/drm_of.h>
>  #include <drm/drm_print.h>
>
>  #include "drm_crtc_internal.h"
> @@ -51,10 +52,8 @@
>   *
>   * Display drivers are responsible for linking encoders with the first bridge
>   * in the chains. This is done by acquiring the appropriate bridge with
> - * of_drm_find_bridge() or drm_of_find_panel_or_bridge(), or creating it for a
> - * panel with drm_panel_bridge_add_typed() (or the managed version
> - * devm_drm_panel_bridge_add_typed()). Once acquired, the bridge shall be
> - * attached to the encoder with a call to drm_bridge_attach().
> + * devm_drm_of_get_bridge(). Once acquired, the bridge shall be attached to the
> + * encoder with a call to drm_bridge_attach().
>   *
>   * Bridges are responsible for linking themselves with the next bridge in the
>   * chain, if any. This is done the same way as for encoders, with the call to
> @@ -1233,6 +1232,41 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
>         return NULL;
>  }
>  EXPORT_SYMBOL(of_drm_find_bridge);
> +
> +/**
> + * devm_drm_of_get_bridge - Return next bridge in the chain
> + * @dev: device to tie the bridge lifetime to
> + * @np: device tree node containing encoder output ports
> + * @port: port in the device tree node
> + * @endpoint: endpoint in the device tree node
> + *
> + * Given a DT node's port and endpoint number, finds the connected node
> + * and returns the associated bridge if any, or creates and returns a
> + * drm panel bridge instance if a panel is connected.
> + *
> + * Returns a pointer to the bridge if successful, or an error pointer
> + * otherwise.
> + */
> +struct drm_bridge *devm_drm_of_get_bridge(struct device *dev,
> +                                         struct device_node *np,
> +                                         unsigned int port,
> +                                         unsigned int endpoint)
> +{
> +       struct drm_bridge *bridge;
> +       struct drm_panel *panel;
> +       int ret;
> +
> +       ret = drm_of_find_panel_or_bridge(np, port, endpoint,
> +                                         &panel, &bridge);
> +       if (ret)
> +               return ERR_PTR(ret);
> +
> +       if (panel)
> +               bridge = devm_drm_panel_bridge_add(dev, panel);
> +
> +       return bridge;
> +}
> +EXPORT_SYMBOL(devm_drm_of_get_bridge);
>  #endif
>
>  MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>");
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 997b8827fed2..37c34146eea8 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -231,6 +231,9 @@ EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint);
>   * return either the associated struct drm_panel or drm_bridge device. Either
>   * @panel or @bridge must not be NULL.
>   *
> + * This function is deprecated and should not be used in new drivers. Use
> + * devm_drm_of_get_bridge() instead.
> + *
>   * Returns zero if successful, or one of the standard error codes if it fails.
>   */
>  int drm_of_find_panel_or_bridge(const struct device_node *np,
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 46bdfa48c413..f70c88ca96ef 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -911,6 +911,8 @@ struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
>  struct drm_bridge *devm_drm_panel_bridge_add_typed(struct device *dev,
>                                                    struct drm_panel *panel,
>                                                    u32 connector_type);
> +struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, struct device_node *node,
> +                                       unsigned int port, unsigned int endpoint);
>  struct drm_connector *drm_panel_bridge_connector(struct drm_bridge *bridge);
>  #endif
>
> --
> 2.31.1
>

Re: [PATCH 1/3] drm/bridge: Add a function to abstract away panels

[Maxime Ripard]

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

Hi Patrik,

On Tue, Sep 21, 2021 at 02:47:49PM +0200, Patrik Jakobsson wrote:
> On Fri, Sep 10, 2021 at 3:10 PM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > Display drivers so far need to have a lot of boilerplate to first
> > retrieve either the panel or bridge that they are connected to using
> > drm_of_find_panel_or_bridge(), and then either deal with each with ad-hoc
> > functions or create a drm panel bridge through drm_panel_bridge_add.
> >
> > In order to reduce the boilerplate and hopefully create a path of least
> > resistance towards using the DRM panel bridge layer, let's create the
> > function devm_drm_of_get_next to reduce that boilerplate.
> 
> Hi Maxime,
> 
> This patch creates a dependency error for me:
> 
> depmod: ERROR: Cycle detected: drm_kms_helper -> drm -> drm_kms_helper
> depmod: ERROR: Cycle detected: cec
> depmod: ERROR: Found 2 modules in dependency cycles!

I just pushed a fix to drm-misc-fixes that should address it

Maxime

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

Re: [PATCH 1/3] drm/bridge: Add a function to abstract away panels

[Patrik Jakobsson]

On Wed, Sep 22, 2021 at 10:23 AM Maxime Ripard <maxime@cerno.tech> wrote:
>
> Hi Patrik,
>
> On Tue, Sep 21, 2021 at 02:47:49PM +0200, Patrik Jakobsson wrote:
> > On Fri, Sep 10, 2021 at 3:10 PM Maxime Ripard <maxime@cerno.tech> wrote:
> > >
> > > Display drivers so far need to have a lot of boilerplate to first
> > > retrieve either the panel or bridge that they are connected to using
> > > drm_of_find_panel_or_bridge(), and then either deal with each with ad-hoc
> > > functions or create a drm panel bridge through drm_panel_bridge_add.
> > >
> > > In order to reduce the boilerplate and hopefully create a path of least
> > > resistance towards using the DRM panel bridge layer, let's create the
> > > function devm_drm_of_get_next to reduce that boilerplate.
> >
> > Hi Maxime,
> >
> > This patch creates a dependency error for me:
> >
> > depmod: ERROR: Cycle detected: drm_kms_helper -> drm -> drm_kms_helper
> > depmod: ERROR: Cycle detected: cec
> > depmod: ERROR: Found 2 modules in dependency cycles!
>
> I just pushed a fix to drm-misc-fixes that should address it

Great, thanks!

>
> Maxime

Re: [PATCH 1/3] drm/bridge: Add a function to abstract away panels

[Laurent Pinchart]

Hi Maxime,

Thank you for the patch.

I know this has already been merged, but I have a question.

On Fri, Sep 10, 2021 at 03:09:39PM +0200, Maxime Ripard wrote:
> Display drivers so far need to have a lot of boilerplate to first
> retrieve either the panel or bridge that they are connected to using
> drm_of_find_panel_or_bridge(), and then either deal with each with ad-hoc
> functions or create a drm panel bridge through drm_panel_bridge_add.
> 
> In order to reduce the boilerplate and hopefully create a path of least
> resistance towards using the DRM panel bridge layer, let's create the
> function devm_drm_of_get_next to reduce that boilerplate.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/gpu/drm/drm_bridge.c | 42 ++++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/drm_of.c     |  3 +++
>  include/drm/drm_bridge.h     |  2 ++
>  3 files changed, 43 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index a8ed66751c2d..10ddca4638b0 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -28,6 +28,7 @@
>  #include <drm/drm_atomic_state_helper.h>
>  #include <drm/drm_bridge.h>
>  #include <drm/drm_encoder.h>
> +#include <drm/drm_of.h>
>  #include <drm/drm_print.h>
>  
>  #include "drm_crtc_internal.h"
> @@ -51,10 +52,8 @@
>   *
>   * Display drivers are responsible for linking encoders with the first bridge
>   * in the chains. This is done by acquiring the appropriate bridge with
> - * of_drm_find_bridge() or drm_of_find_panel_or_bridge(), or creating it for a
> - * panel with drm_panel_bridge_add_typed() (or the managed version
> - * devm_drm_panel_bridge_add_typed()). Once acquired, the bridge shall be
> - * attached to the encoder with a call to drm_bridge_attach().
> + * devm_drm_of_get_bridge(). Once acquired, the bridge shall be attached to the
> + * encoder with a call to drm_bridge_attach().
>   *
>   * Bridges are responsible for linking themselves with the next bridge in the
>   * chain, if any. This is done the same way as for encoders, with the call to
> @@ -1233,6 +1232,41 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
>  	return NULL;
>  }
>  EXPORT_SYMBOL(of_drm_find_bridge);
> +
> +/**
> + * devm_drm_of_get_bridge - Return next bridge in the chain
> + * @dev: device to tie the bridge lifetime to
> + * @np: device tree node containing encoder output ports
> + * @port: port in the device tree node
> + * @endpoint: endpoint in the device tree node
> + *
> + * Given a DT node's port and endpoint number, finds the connected node
> + * and returns the associated bridge if any, or creates and returns a
> + * drm panel bridge instance if a panel is connected.
> + *
> + * Returns a pointer to the bridge if successful, or an error pointer
> + * otherwise.
> + */
> +struct drm_bridge *devm_drm_of_get_bridge(struct device *dev,
> +					  struct device_node *np,
> +					  unsigned int port,
> +					  unsigned int endpoint)
> +{
> +	struct drm_bridge *bridge;
> +	struct drm_panel *panel;
> +	int ret;
> +
> +	ret = drm_of_find_panel_or_bridge(np, port, endpoint,
> +					  &panel, &bridge);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	if (panel)
> +		bridge = devm_drm_panel_bridge_add(dev, panel);
> +
> +	return bridge;

I really like the idea, I've wanted to do something like this for a long
time. I however wonder if this is the best approach, or if we could get
the panel core to register the bridge itself. The part that bothers me
here is the assymetry in the lifetime of the bridges, the returned
pointer is either looked up or allocated.

Bridge lifetime is such a mess that it may not make a big difference,
but eventually we'll have to address that problem globally.

> +}
> +EXPORT_SYMBOL(devm_drm_of_get_bridge);
>  #endif
>  
>  MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>");
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index 997b8827fed2..37c34146eea8 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -231,6 +231,9 @@ EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint);
>   * return either the associated struct drm_panel or drm_bridge device. Either
>   * @panel or @bridge must not be NULL.
>   *
> + * This function is deprecated and should not be used in new drivers. Use
> + * devm_drm_of_get_bridge() instead.
> + *
>   * Returns zero if successful, or one of the standard error codes if it fails.
>   */
>  int drm_of_find_panel_or_bridge(const struct device_node *np,
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 46bdfa48c413..f70c88ca96ef 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -911,6 +911,8 @@ struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
>  struct drm_bridge *devm_drm_panel_bridge_add_typed(struct device *dev,
>  						   struct drm_panel *panel,
>  						   u32 connector_type);
> +struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, struct device_node *node,
> +					unsigned int port, unsigned int endpoint);
>  struct drm_connector *drm_panel_bridge_connector(struct drm_bridge *bridge);
>  #endif
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] drm/bridge: Add a function to abstract away panels

[Maxime Ripard]

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

On Thu, Sep 23, 2021 at 03:29:37AM +0300, Laurent Pinchart wrote:
> Hi Maxime,
> 
> Thank you for the patch.
> 
> I know this has already been merged, but I have a question.
> 
> On Fri, Sep 10, 2021 at 03:09:39PM +0200, Maxime Ripard wrote:
> > Display drivers so far need to have a lot of boilerplate to first
> > retrieve either the panel or bridge that they are connected to using
> > drm_of_find_panel_or_bridge(), and then either deal with each with ad-hoc
> > functions or create a drm panel bridge through drm_panel_bridge_add.
> > 
> > In order to reduce the boilerplate and hopefully create a path of least
> > resistance towards using the DRM panel bridge layer, let's create the
> > function devm_drm_of_get_next to reduce that boilerplate.
> >
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 42 ++++++++++++++++++++++++++++++++----
> >  drivers/gpu/drm/drm_of.c     |  3 +++
> >  include/drm/drm_bridge.h     |  2 ++
> >  3 files changed, 43 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index a8ed66751c2d..10ddca4638b0 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -28,6 +28,7 @@
> >  #include <drm/drm_atomic_state_helper.h>
> >  #include <drm/drm_bridge.h>
> >  #include <drm/drm_encoder.h>
> > +#include <drm/drm_of.h>
> >  #include <drm/drm_print.h>
> >  
> >  #include "drm_crtc_internal.h"
> > @@ -51,10 +52,8 @@
> >   *
> >   * Display drivers are responsible for linking encoders with the first bridge
> >   * in the chains. This is done by acquiring the appropriate bridge with
> > - * of_drm_find_bridge() or drm_of_find_panel_or_bridge(), or creating it for a
> > - * panel with drm_panel_bridge_add_typed() (or the managed version
> > - * devm_drm_panel_bridge_add_typed()). Once acquired, the bridge shall be
> > - * attached to the encoder with a call to drm_bridge_attach().
> > + * devm_drm_of_get_bridge(). Once acquired, the bridge shall be attached to the
> > + * encoder with a call to drm_bridge_attach().
> >   *
> >   * Bridges are responsible for linking themselves with the next bridge in the
> >   * chain, if any. This is done the same way as for encoders, with the call to
> > @@ -1233,6 +1232,41 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
> >  	return NULL;
> >  }
> >  EXPORT_SYMBOL(of_drm_find_bridge);
> > +
> > +/**
> > + * devm_drm_of_get_bridge - Return next bridge in the chain
> > + * @dev: device to tie the bridge lifetime to
> > + * @np: device tree node containing encoder output ports
> > + * @port: port in the device tree node
> > + * @endpoint: endpoint in the device tree node
> > + *
> > + * Given a DT node's port and endpoint number, finds the connected node
> > + * and returns the associated bridge if any, or creates and returns a
> > + * drm panel bridge instance if a panel is connected.
> > + *
> > + * Returns a pointer to the bridge if successful, or an error pointer
> > + * otherwise.
> > + */
> > +struct drm_bridge *devm_drm_of_get_bridge(struct device *dev,
> > +					  struct device_node *np,
> > +					  unsigned int port,
> > +					  unsigned int endpoint)
> > +{
> > +	struct drm_bridge *bridge;
> > +	struct drm_panel *panel;
> > +	int ret;
> > +
> > +	ret = drm_of_find_panel_or_bridge(np, port, endpoint,
> > +					  &panel, &bridge);
> > +	if (ret)
> > +		return ERR_PTR(ret);
> > +
> > +	if (panel)
> > +		bridge = devm_drm_panel_bridge_add(dev, panel);
> > +
> > +	return bridge;
> 
> I really like the idea, I've wanted to do something like this for a long
> time. I however wonder if this is the best approach, or if we could get
> the panel core to register the bridge itself. The part that bothers me
> here is the assymetry in the lifetime of the bridges, the returned
> pointer is either looked up or allocated.
> 
> Bridge lifetime is such a mess that it may not make a big difference,
> but eventually we'll have to address that problem globally.

We can't have Rust soon enough :)

Does it really matter though? I thought the bridges couldn't be unloaded
from a DRM device anyway, so for all practical purposes this will be
removed at around the same time: when the whole DRM device is going to
be teared down?

Maxime

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

Re: [PATCH 1/3] drm/bridge: Add a function to abstract away panels

[Laurent Pinchart]

On Mon, Sep 27, 2021 at 09:43:44PM +0200, Maxime Ripard wrote:
> On Thu, Sep 23, 2021 at 03:29:37AM +0300, Laurent Pinchart wrote:
> > Hi Maxime,
> > 
> > Thank you for the patch.
> > 
> > I know this has already been merged, but I have a question.
> > 
> > On Fri, Sep 10, 2021 at 03:09:39PM +0200, Maxime Ripard wrote:
> > > Display drivers so far need to have a lot of boilerplate to first
> > > retrieve either the panel or bridge that they are connected to using
> > > drm_of_find_panel_or_bridge(), and then either deal with each with ad-hoc
> > > functions or create a drm panel bridge through drm_panel_bridge_add.
> > > 
> > > In order to reduce the boilerplate and hopefully create a path of least
> > > resistance towards using the DRM panel bridge layer, let's create the
> > > function devm_drm_of_get_next to reduce that boilerplate.
> > >
> > > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > > ---
> > >  drivers/gpu/drm/drm_bridge.c | 42 ++++++++++++++++++++++++++++++++----
> > >  drivers/gpu/drm/drm_of.c     |  3 +++
> > >  include/drm/drm_bridge.h     |  2 ++
> > >  3 files changed, 43 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > > index a8ed66751c2d..10ddca4638b0 100644
> > > --- a/drivers/gpu/drm/drm_bridge.c
> > > +++ b/drivers/gpu/drm/drm_bridge.c
> > > @@ -28,6 +28,7 @@
> > >  #include <drm/drm_atomic_state_helper.h>
> > >  #include <drm/drm_bridge.h>
> > >  #include <drm/drm_encoder.h>
> > > +#include <drm/drm_of.h>
> > >  #include <drm/drm_print.h>
> > >  
> > >  #include "drm_crtc_internal.h"
> > > @@ -51,10 +52,8 @@
> > >   *
> > >   * Display drivers are responsible for linking encoders with the first bridge
> > >   * in the chains. This is done by acquiring the appropriate bridge with
> > > - * of_drm_find_bridge() or drm_of_find_panel_or_bridge(), or creating it for a
> > > - * panel with drm_panel_bridge_add_typed() (or the managed version
> > > - * devm_drm_panel_bridge_add_typed()). Once acquired, the bridge shall be
> > > - * attached to the encoder with a call to drm_bridge_attach().
> > > + * devm_drm_of_get_bridge(). Once acquired, the bridge shall be attached to the
> > > + * encoder with a call to drm_bridge_attach().
> > >   *
> > >   * Bridges are responsible for linking themselves with the next bridge in the
> > >   * chain, if any. This is done the same way as for encoders, with the call to
> > > @@ -1233,6 +1232,41 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
> > >  	return NULL;
> > >  }
> > >  EXPORT_SYMBOL(of_drm_find_bridge);
> > > +
> > > +/**
> > > + * devm_drm_of_get_bridge - Return next bridge in the chain
> > > + * @dev: device to tie the bridge lifetime to
> > > + * @np: device tree node containing encoder output ports
> > > + * @port: port in the device tree node
> > > + * @endpoint: endpoint in the device tree node
> > > + *
> > > + * Given a DT node's port and endpoint number, finds the connected node
> > > + * and returns the associated bridge if any, or creates and returns a
> > > + * drm panel bridge instance if a panel is connected.
> > > + *
> > > + * Returns a pointer to the bridge if successful, or an error pointer
> > > + * otherwise.
> > > + */
> > > +struct drm_bridge *devm_drm_of_get_bridge(struct device *dev,
> > > +					  struct device_node *np,
> > > +					  unsigned int port,
> > > +					  unsigned int endpoint)
> > > +{
> > > +	struct drm_bridge *bridge;
> > > +	struct drm_panel *panel;
> > > +	int ret;
> > > +
> > > +	ret = drm_of_find_panel_or_bridge(np, port, endpoint,
> > > +					  &panel, &bridge);
> > > +	if (ret)
> > > +		return ERR_PTR(ret);
> > > +
> > > +	if (panel)
> > > +		bridge = devm_drm_panel_bridge_add(dev, panel);
> > > +
> > > +	return bridge;
> > 
> > I really like the idea, I've wanted to do something like this for a long
> > time. I however wonder if this is the best approach, or if we could get
> > the panel core to register the bridge itself. The part that bothers me
> > here is the assymetry in the lifetime of the bridges, the returned
> > pointer is either looked up or allocated.
> > 
> > Bridge lifetime is such a mess that it may not make a big difference,
> > but eventually we'll have to address that problem globally.
> 
> We can't have Rust soon enough :)

:-) Jokes aside, Rust or C, this would need a design overhaul as a first
step.

> Does it really matter though? I thought the bridges couldn't be unloaded
> from a DRM device anyway, so for all practical purposes this will be
> removed at around the same time: when the whole DRM device is going to
> be teared down?

Try to unbind a bridge device from its driver in sysfs, and it won't be
pretty.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] drm/bridge: Add a function to abstract away panels

[Andrzej Hajda]

W dniu 23.09.2021 o 02:29, Laurent Pinchart pisze:
> Hi Maxime,
>
> Thank you for the patch.
>
> I know this has already been merged, but I have a question.
>
> On Fri, Sep 10, 2021 at 03:09:39PM +0200, Maxime Ripard wrote:
>> Display drivers so far need to have a lot of boilerplate to first
>> retrieve either the panel or bridge that they are connected to using
>> drm_of_find_panel_or_bridge(), and then either deal with each with ad-hoc
>> functions or create a drm panel bridge through drm_panel_bridge_add.
>>
>> In order to reduce the boilerplate and hopefully create a path of least
>> resistance towards using the DRM panel bridge layer, let's create the
>> function devm_drm_of_get_next to reduce that boilerplate.
>>
>> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
>> ---
>>   drivers/gpu/drm/drm_bridge.c | 42 ++++++++++++++++++++++++++++++++----
>>   drivers/gpu/drm/drm_of.c     |  3 +++
>>   include/drm/drm_bridge.h     |  2 ++
>>   3 files changed, 43 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index a8ed66751c2d..10ddca4638b0 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -28,6 +28,7 @@
>>   #include <drm/drm_atomic_state_helper.h>
>>   #include <drm/drm_bridge.h>
>>   #include <drm/drm_encoder.h>
>> +#include <drm/drm_of.h>
>>   #include <drm/drm_print.h>
>>   
>>   #include "drm_crtc_internal.h"
>> @@ -51,10 +52,8 @@
>>    *
>>    * Display drivers are responsible for linking encoders with the first bridge
>>    * in the chains. This is done by acquiring the appropriate bridge with
>> - * of_drm_find_bridge() or drm_of_find_panel_or_bridge(), or creating it for a
>> - * panel with drm_panel_bridge_add_typed() (or the managed version
>> - * devm_drm_panel_bridge_add_typed()). Once acquired, the bridge shall be
>> - * attached to the encoder with a call to drm_bridge_attach().
>> + * devm_drm_of_get_bridge(). Once acquired, the bridge shall be attached to the
>> + * encoder with a call to drm_bridge_attach().
>>    *
>>    * Bridges are responsible for linking themselves with the next bridge in the
>>    * chain, if any. This is done the same way as for encoders, with the call to
>> @@ -1233,6 +1232,41 @@ struct drm_bridge *of_drm_find_bridge(struct device_node *np)
>>   	return NULL;
>>   }
>>   EXPORT_SYMBOL(of_drm_find_bridge);
>> +
>> +/**
>> + * devm_drm_of_get_bridge - Return next bridge in the chain
>> + * @dev: device to tie the bridge lifetime to
>> + * @np: device tree node containing encoder output ports
>> + * @port: port in the device tree node
>> + * @endpoint: endpoint in the device tree node
>> + *
>> + * Given a DT node's port and endpoint number, finds the connected node
>> + * and returns the associated bridge if any, or creates and returns a
>> + * drm panel bridge instance if a panel is connected.
>> + *
>> + * Returns a pointer to the bridge if successful, or an error pointer
>> + * otherwise.
>> + */
>> +struct drm_bridge *devm_drm_of_get_bridge(struct device *dev,
>> +					  struct device_node *np,
>> +					  unsigned int port,
>> +					  unsigned int endpoint)
>> +{
>> +	struct drm_bridge *bridge;
>> +	struct drm_panel *panel;
>> +	int ret;
>> +
>> +	ret = drm_of_find_panel_or_bridge(np, port, endpoint,
>> +					  &panel, &bridge);
>> +	if (ret)
>> +		return ERR_PTR(ret);
>> +
>> +	if (panel)
>> +		bridge = devm_drm_panel_bridge_add(dev, panel);
>> +
>> +	return bridge;
> I really like the idea, I've wanted to do something like this for a long
> time. I however wonder if this is the best approach, or if we could get
> the panel core to register the bridge itself. The part that bothers me
> here is the assymetry in the lifetime of the bridges, the returned
> pointer is either looked up or allocated.


When drm_panel_bridge was proposed 1st time I suggested that instead of 
it we should just merge panels with bridges (maybe with new name, my 
favorite is drm_sink), but I was outvoted :)

Apparently with this patch we go to the same direction: since panels 
will be accessed only via drm_panel_bridge they will become actually 
bridges :) Somebody could then clean-up the whole intermediate code.


Regards

Andrzej


>
> Bridge lifetime is such a mess that it may not make a big difference,
> but eventually we'll have to address that problem globally.
>
>> +}
>> +EXPORT_SYMBOL(devm_drm_of_get_bridge);
>>   #endif
>>   
>>   MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@samsung.com>");
>> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
>> index 997b8827fed2..37c34146eea8 100644
>> --- a/drivers/gpu/drm/drm_of.c
>> +++ b/drivers/gpu/drm/drm_of.c
>> @@ -231,6 +231,9 @@ EXPORT_SYMBOL_GPL(drm_of_encoder_active_endpoint);
>>    * return either the associated struct drm_panel or drm_bridge device. Either
>>    * @panel or @bridge must not be NULL.
>>    *
>> + * This function is deprecated and should not be used in new drivers. Use
>> + * devm_drm_of_get_bridge() instead.
>> + *
>>    * Returns zero if successful, or one of the standard error codes if it fails.
>>    */
>>   int drm_of_find_panel_or_bridge(const struct device_node *np,
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index 46bdfa48c413..f70c88ca96ef 100644
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -911,6 +911,8 @@ struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
>>   struct drm_bridge *devm_drm_panel_bridge_add_typed(struct device *dev,
>>   						   struct drm_panel *panel,
>>   						   u32 connector_type);
>> +struct drm_bridge *devm_drm_of_get_bridge(struct device *dev, struct device_node *node,
>> +					unsigned int port, unsigned int endpoint);
>>   struct drm_connector *drm_panel_bridge_connector(struct drm_bridge *bridge);
>>   #endif
>>