[PATCH v4 0/3] Support DRM bridges on NVIDIA Tegra

[PATCH v4 0/3] Support DRM bridges on NVIDIA Tegra

[Dmitry Osipenko]

Hello,

This small series adds initial support for the DRM bridges to NVIDIA Tegra
DRM driver. This is required by newer device-trees where we model the LVDS
encoder bridge properly.

Changelog:

v4: - Following review comments that were made by Laurent Pinchart to the v3,
      we now create and use the "bridge connector".

v3: - Following recommendation from Sam Ravnborg, the new bridge attachment
      model is now being used, i.e. we ask bridge to *not* create a connector
      using the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.

    - The bridge is now created only for the RGB (LVDS) output, and only
      when necessary. For now we don't need bridges for HDMI or DSI outputs.

    - I noticed that we're leaking OF node in the panel's error code path,
      this is fixed now by the new patch "Don't leak OF node on error".

v2: - Added the new "rgb: Don't register connector if bridge is used"
      patch, which hides the unused connector provided by the Tegra DRM
      driver when bridge is used, since bridge provides its own connector
      to us.

    - Please notice that the first "Support DRM bridges" patch was previously
      sent out as a standalone v1 change.

Dmitry Osipenko (3):
  drm/tegra: output: Don't leak OF node on error
  drm/tegra: output: Support DRM bridges
  drm/tegra: output: rgb: Support LVDS encoder bridge

 drivers/gpu/drm/tegra/drm.h    |  2 ++
 drivers/gpu/drm/tegra/output.c | 22 +++++++++----
 drivers/gpu/drm/tegra/rgb.c    | 58 ++++++++++++++++++++++++++++------
 3 files changed, 67 insertions(+), 15 deletions(-)

-- 
2.26.0

[PATCH v4 0/3] Support DRM bridges on NVIDIA Tegra

[Dmitry Osipenko]

Hello,

This small series adds initial support for the DRM bridges to NVIDIA Tegra
DRM driver. This is required by newer device-trees where we model the LVDS
encoder bridge properly.

Changelog:

v4: - Following review comments that were made by Laurent Pinchart to the v3,
      we now create and use the "bridge connector".

v3: - Following recommendation from Sam Ravnborg, the new bridge attachment
      model is now being used, i.e. we ask bridge to *not* create a connector
      using the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.

    - The bridge is now created only for the RGB (LVDS) output, and only
      when necessary. For now we don't need bridges for HDMI or DSI outputs.

    - I noticed that we're leaking OF node in the panel's error code path,
      this is fixed now by the new patch "Don't leak OF node on error".

v2: - Added the new "rgb: Don't register connector if bridge is used"
      patch, which hides the unused connector provided by the Tegra DRM
      driver when bridge is used, since bridge provides its own connector
      to us.

    - Please notice that the first "Support DRM bridges" patch was previously
      sent out as a standalone v1 change.

Dmitry Osipenko (3):
  drm/tegra: output: Don't leak OF node on error
  drm/tegra: output: Support DRM bridges
  drm/tegra: output: rgb: Support LVDS encoder bridge

 drivers/gpu/drm/tegra/drm.h    |  2 ++
 drivers/gpu/drm/tegra/output.c | 22 +++++++++----
 drivers/gpu/drm/tegra/rgb.c    | 58 ++++++++++++++++++++++++++++------
 3 files changed, 67 insertions(+), 15 deletions(-)

-- 
2.26.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 1/3] drm/tegra: output: Don't leak OF node on error

[Dmitry Osipenko]

The OF node should be put before returning error in tegra_output_probe(),
otherwise node's refcount will be leaked.

Reviewed-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/gpu/drm/tegra/output.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
index e36e5e7c2f69..a6a711d54e88 100644
--- a/drivers/gpu/drm/tegra/output.c
+++ b/drivers/gpu/drm/tegra/output.c
@@ -102,10 +102,10 @@ int tegra_output_probe(struct tegra_output *output)
 	panel = of_parse_phandle(output->of_node, "nvidia,panel", 0);
 	if (panel) {
 		output->panel = of_drm_find_panel(panel);
+		of_node_put(panel);
+
 		if (IS_ERR(output->panel))
 			return PTR_ERR(output->panel);
-
-		of_node_put(panel);
 	}
 
 	output->edid = of_get_property(output->of_node, "nvidia,edid", &size);
@@ -113,13 +113,12 @@ int tegra_output_probe(struct tegra_output *output)
 	ddc = of_parse_phandle(output->of_node, "nvidia,ddc-i2c-bus", 0);
 	if (ddc) {
 		output->ddc = of_find_i2c_adapter_by_node(ddc);
+		of_node_put(ddc);
+
 		if (!output->ddc) {
 			err = -EPROBE_DEFER;
-			of_node_put(ddc);
 			return err;
 		}
-
-		of_node_put(ddc);
 	}
 
 	output->hpd_gpio = devm_gpiod_get_from_of_node(output->dev,
-- 
2.26.0

[PATCH v4 1/3] drm/tegra: output: Don't leak OF node on error

[Dmitry Osipenko]

The OF node should be put before returning error in tegra_output_probe(),
otherwise node's refcount will be leaked.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpu/drm/tegra/output.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
index e36e5e7c2f69..a6a711d54e88 100644
--- a/drivers/gpu/drm/tegra/output.c
+++ b/drivers/gpu/drm/tegra/output.c
@@ -102,10 +102,10 @@ int tegra_output_probe(struct tegra_output *output)
 	panel = of_parse_phandle(output->of_node, "nvidia,panel", 0);
 	if (panel) {
 		output->panel = of_drm_find_panel(panel);
+		of_node_put(panel);
+
 		if (IS_ERR(output->panel))
 			return PTR_ERR(output->panel);
-
-		of_node_put(panel);
 	}
 
 	output->edid = of_get_property(output->of_node, "nvidia,edid", &size);
@@ -113,13 +113,12 @@ int tegra_output_probe(struct tegra_output *output)
 	ddc = of_parse_phandle(output->of_node, "nvidia,ddc-i2c-bus", 0);
 	if (ddc) {
 		output->ddc = of_find_i2c_adapter_by_node(ddc);
+		of_node_put(ddc);
+
 		if (!output->ddc) {
 			err = -EPROBE_DEFER;
-			of_node_put(ddc);
 			return err;
 		}
-
-		of_node_put(ddc);
 	}
 
 	output->hpd_gpio = devm_gpiod_get_from_of_node(output->dev,
-- 
2.26.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 2/3] drm/tegra: output: Support DRM bridges

[Dmitry Osipenko]

Newer Tegra device-trees will specify a video output graph which involves
a bridge. This patch adds initial support for the DRM bridges to the
Tegra's DRM output.

Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/gpu/drm/tegra/drm.h    |  2 ++
 drivers/gpu/drm/tegra/output.c | 13 ++++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
index 804869799305..cccd368b6752 100644
--- a/drivers/gpu/drm/tegra/drm.h
+++ b/drivers/gpu/drm/tegra/drm.h
@@ -12,6 +12,7 @@
 #include <linux/of_gpio.h>
 
 #include <drm/drm_atomic.h>
+#include <drm/drm_bridge.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_encoder.h>
 #include <drm/drm_fb_helper.h>
@@ -116,6 +117,7 @@ struct tegra_output {
 	struct device_node *of_node;
 	struct device *dev;
 
+	struct drm_bridge *bridge;
 	struct drm_panel *panel;
 	struct i2c_adapter *ddc;
 	const struct edid *edid;
diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
index a6a711d54e88..ec0cd4a1ced1 100644
--- a/drivers/gpu/drm/tegra/output.c
+++ b/drivers/gpu/drm/tegra/output.c
@@ -5,6 +5,7 @@
  */
 
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_of.h>
 #include <drm/drm_panel.h>
 #include <drm/drm_simple_kms_helper.h>
 
@@ -92,13 +93,23 @@ static irqreturn_t hpd_irq(int irq, void *data)
 
 int tegra_output_probe(struct tegra_output *output)
 {
-	struct device_node *ddc, *panel;
+	struct device_node *ddc, *panel, *port;
 	unsigned long flags;
 	int err, size;
 
 	if (!output->of_node)
 		output->of_node = output->dev->of_node;
 
+	port = of_get_child_by_name(output->of_node, "port");
+	if (port) {
+		err = drm_of_find_panel_or_bridge(output->of_node, 0, 0, NULL,
+						  &output->bridge);
+		of_node_put(port);
+
+		if (err)
+			return err;
+	}
+
 	panel = of_parse_phandle(output->of_node, "nvidia,panel", 0);
 	if (panel) {
 		output->panel = of_drm_find_panel(panel);
-- 
2.26.0

[PATCH v4 2/3] drm/tegra: output: Support DRM bridges

[Dmitry Osipenko]

Newer Tegra device-trees will specify a video output graph which involves
a bridge. This patch adds initial support for the DRM bridges to the
Tegra's DRM output.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpu/drm/tegra/drm.h    |  2 ++
 drivers/gpu/drm/tegra/output.c | 13 ++++++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
index 804869799305..cccd368b6752 100644
--- a/drivers/gpu/drm/tegra/drm.h
+++ b/drivers/gpu/drm/tegra/drm.h
@@ -12,6 +12,7 @@
 #include <linux/of_gpio.h>
 
 #include <drm/drm_atomic.h>
+#include <drm/drm_bridge.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_encoder.h>
 #include <drm/drm_fb_helper.h>
@@ -116,6 +117,7 @@ struct tegra_output {
 	struct device_node *of_node;
 	struct device *dev;
 
+	struct drm_bridge *bridge;
 	struct drm_panel *panel;
 	struct i2c_adapter *ddc;
 	const struct edid *edid;
diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
index a6a711d54e88..ec0cd4a1ced1 100644
--- a/drivers/gpu/drm/tegra/output.c
+++ b/drivers/gpu/drm/tegra/output.c
@@ -5,6 +5,7 @@
  */
 
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_of.h>
 #include <drm/drm_panel.h>
 #include <drm/drm_simple_kms_helper.h>
 
@@ -92,13 +93,23 @@ static irqreturn_t hpd_irq(int irq, void *data)
 
 int tegra_output_probe(struct tegra_output *output)
 {
-	struct device_node *ddc, *panel;
+	struct device_node *ddc, *panel, *port;
 	unsigned long flags;
 	int err, size;
 
 	if (!output->of_node)
 		output->of_node = output->dev->of_node;
 
+	port = of_get_child_by_name(output->of_node, "port");
+	if (port) {
+		err = drm_of_find_panel_or_bridge(output->of_node, 0, 0, NULL,
+						  &output->bridge);
+		of_node_put(port);
+
+		if (err)
+			return err;
+	}
+
 	panel = of_parse_phandle(output->of_node, "nvidia,panel", 0);
 	if (panel) {
 		output->panel = of_drm_find_panel(panel);
-- 
2.26.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 3/3] drm/tegra: output: rgb: Support LVDS encoder bridge

[Dmitry Osipenko]

Newer Tegra device-trees will specify a video output graph, which involves
LVDS encoder bridge. This patch adds support for the LVDS encoder bridge
to the RGB output, allowing us to model the display hardware properly.

Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/gpu/drm/tegra/rgb.c | 58 +++++++++++++++++++++++++++++++------
 1 file changed, 49 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
index 0562a7eb793f..f28a226914c0 100644
--- a/drivers/gpu/drm/tegra/rgb.c
+++ b/drivers/gpu/drm/tegra/rgb.c
@@ -7,6 +7,7 @@
 #include <linux/clk.h>
 
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_bridge_connector.h>
 #include <drm/drm_panel.h>
 #include <drm/drm_simple_kms_helper.h>
 
@@ -267,24 +268,63 @@ int tegra_dc_rgb_remove(struct tegra_dc *dc)
 int tegra_dc_rgb_init(struct drm_device *drm, struct tegra_dc *dc)
 {
 	struct tegra_output *output = dc->rgb;
+	struct drm_connector *connector;
 	int err;
 
 	if (!dc->rgb)
 		return -ENODEV;
 
-	drm_connector_init(drm, &output->connector, &tegra_rgb_connector_funcs,
-			   DRM_MODE_CONNECTOR_LVDS);
-	drm_connector_helper_add(&output->connector,
-				 &tegra_rgb_connector_helper_funcs);
-	output->connector.dpms = DRM_MODE_DPMS_OFF;
-
 	drm_simple_encoder_init(drm, &output->encoder, DRM_MODE_ENCODER_LVDS);
 	drm_encoder_helper_add(&output->encoder,
 			       &tegra_rgb_encoder_helper_funcs);
 
-	drm_connector_attach_encoder(&output->connector,
-					  &output->encoder);
-	drm_connector_register(&output->connector);
+	/*
+	 * Tegra devices that have LVDS panel utilize LVDS encoder bridge
+	 * for converting up to 28 LCD LVTTL lanes into 5/4 LVDS lanes that
+	 * go to display panel's receiver.
+	 *
+	 * Encoder usually have a power-down control which needs to be enabled
+	 * in order to transmit data to the panel. Historically devices that
+	 * use an older device-tree version didn't model the bridge, assuming
+	 * that encoder is turned ON by default, while today's DRM allows us
+	 * to model LVDS encoder properly.
+	 *
+	 * Newer device-trees utilize LVDS encoder bridge, which provides
+	 * us with a connector and handles the display panel.
+	 *
+	 * For older device-trees we fall back to our own connector and use
+	 * nvidia,panel phandle.
+	 */
+	if (output->bridge) {
+		err = drm_bridge_attach(&output->encoder, output->bridge,
+					NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+		if (err) {
+			dev_err(output->dev, "failed to attach bridge: %d\n",
+				err);
+			return err;
+		}
+
+		connector = drm_bridge_connector_init(drm, &output->encoder);
+		if (IS_ERR(connector)) {
+			dev_err(output->dev,
+				"failed to initialize bridge connector: %pe\n",
+				connector);
+			return PTR_ERR(connector);
+		}
+
+		drm_connector_attach_encoder(connector, &output->encoder);
+	} else {
+		drm_connector_init(drm, &output->connector,
+				   &tegra_rgb_connector_funcs,
+				   DRM_MODE_CONNECTOR_LVDS);
+		drm_connector_helper_add(&output->connector,
+					 &tegra_rgb_connector_helper_funcs);
+		output->connector.dpms = DRM_MODE_DPMS_OFF;
+
+		drm_connector_attach_encoder(&output->connector,
+					     &output->encoder);
+		drm_connector_register(&output->connector);
+	}
 
 	err = tegra_output_init(drm, output);
 	if (err < 0) {
-- 
2.26.0

[PATCH v4 3/3] drm/tegra: output: rgb: Support LVDS encoder bridge

[Dmitry Osipenko]

Newer Tegra device-trees will specify a video output graph, which involves
LVDS encoder bridge. This patch adds support for the LVDS encoder bridge
to the RGB output, allowing us to model the display hardware properly.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpu/drm/tegra/rgb.c | 58 +++++++++++++++++++++++++++++++------
 1 file changed, 49 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
index 0562a7eb793f..f28a226914c0 100644
--- a/drivers/gpu/drm/tegra/rgb.c
+++ b/drivers/gpu/drm/tegra/rgb.c
@@ -7,6 +7,7 @@
 #include <linux/clk.h>
 
 #include <drm/drm_atomic_helper.h>
+#include <drm/drm_bridge_connector.h>
 #include <drm/drm_panel.h>
 #include <drm/drm_simple_kms_helper.h>
 
@@ -267,24 +268,63 @@ int tegra_dc_rgb_remove(struct tegra_dc *dc)
 int tegra_dc_rgb_init(struct drm_device *drm, struct tegra_dc *dc)
 {
 	struct tegra_output *output = dc->rgb;
+	struct drm_connector *connector;
 	int err;
 
 	if (!dc->rgb)
 		return -ENODEV;
 
-	drm_connector_init(drm, &output->connector, &tegra_rgb_connector_funcs,
-			   DRM_MODE_CONNECTOR_LVDS);
-	drm_connector_helper_add(&output->connector,
-				 &tegra_rgb_connector_helper_funcs);
-	output->connector.dpms = DRM_MODE_DPMS_OFF;
-
 	drm_simple_encoder_init(drm, &output->encoder, DRM_MODE_ENCODER_LVDS);
 	drm_encoder_helper_add(&output->encoder,
 			       &tegra_rgb_encoder_helper_funcs);
 
-	drm_connector_attach_encoder(&output->connector,
-					  &output->encoder);
-	drm_connector_register(&output->connector);
+	/*
+	 * Tegra devices that have LVDS panel utilize LVDS encoder bridge
+	 * for converting up to 28 LCD LVTTL lanes into 5/4 LVDS lanes that
+	 * go to display panel's receiver.
+	 *
+	 * Encoder usually have a power-down control which needs to be enabled
+	 * in order to transmit data to the panel. Historically devices that
+	 * use an older device-tree version didn't model the bridge, assuming
+	 * that encoder is turned ON by default, while today's DRM allows us
+	 * to model LVDS encoder properly.
+	 *
+	 * Newer device-trees utilize LVDS encoder bridge, which provides
+	 * us with a connector and handles the display panel.
+	 *
+	 * For older device-trees we fall back to our own connector and use
+	 * nvidia,panel phandle.
+	 */
+	if (output->bridge) {
+		err = drm_bridge_attach(&output->encoder, output->bridge,
+					NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+		if (err) {
+			dev_err(output->dev, "failed to attach bridge: %d\n",
+				err);
+			return err;
+		}
+
+		connector = drm_bridge_connector_init(drm, &output->encoder);
+		if (IS_ERR(connector)) {
+			dev_err(output->dev,
+				"failed to initialize bridge connector: %pe\n",
+				connector);
+			return PTR_ERR(connector);
+		}
+
+		drm_connector_attach_encoder(connector, &output->encoder);
+	} else {
+		drm_connector_init(drm, &output->connector,
+				   &tegra_rgb_connector_funcs,
+				   DRM_MODE_CONNECTOR_LVDS);
+		drm_connector_helper_add(&output->connector,
+					 &tegra_rgb_connector_helper_funcs);
+		output->connector.dpms = DRM_MODE_DPMS_OFF;
+
+		drm_connector_attach_encoder(&output->connector,
+					     &output->encoder);
+		drm_connector_register(&output->connector);
+	}
 
 	err = tegra_output_init(drm, output);
 	if (err < 0) {
-- 
2.26.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 3/3] drm/tegra: output: rgb: Support LVDS encoder bridge

[Laurent Pinchart]

Hi Dmitry,

Thank you for the patch.

On Fri, Apr 17, 2020 at 08:52:38PM +0300, Dmitry Osipenko wrote:
> Newer Tegra device-trees will specify a video output graph, which involves
> LVDS encoder bridge. This patch adds support for the LVDS encoder bridge
> to the RGB output, allowing us to model the display hardware properly.
> 
> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/gpu/drm/tegra/rgb.c | 58 +++++++++++++++++++++++++++++++------
>  1 file changed, 49 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
> index 0562a7eb793f..f28a226914c0 100644
> --- a/drivers/gpu/drm/tegra/rgb.c
> +++ b/drivers/gpu/drm/tegra/rgb.c
> @@ -7,6 +7,7 @@
>  #include <linux/clk.h>
>  
>  #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge_connector.h>
>  #include <drm/drm_panel.h>
>  #include <drm/drm_simple_kms_helper.h>
>  
> @@ -267,24 +268,63 @@ int tegra_dc_rgb_remove(struct tegra_dc *dc)
>  int tegra_dc_rgb_init(struct drm_device *drm, struct tegra_dc *dc)
>  {
>  	struct tegra_output *output = dc->rgb;
> +	struct drm_connector *connector;
>  	int err;
>  
>  	if (!dc->rgb)
>  		return -ENODEV;
>  
> -	drm_connector_init(drm, &output->connector, &tegra_rgb_connector_funcs,
> -			   DRM_MODE_CONNECTOR_LVDS);
> -	drm_connector_helper_add(&output->connector,
> -				 &tegra_rgb_connector_helper_funcs);
> -	output->connector.dpms = DRM_MODE_DPMS_OFF;
> -
>  	drm_simple_encoder_init(drm, &output->encoder, DRM_MODE_ENCODER_LVDS);
>  	drm_encoder_helper_add(&output->encoder,
>  			       &tegra_rgb_encoder_helper_funcs);
>  
> -	drm_connector_attach_encoder(&output->connector,
> -					  &output->encoder);
> -	drm_connector_register(&output->connector);
> +	/*
> +	 * Tegra devices that have LVDS panel utilize LVDS encoder bridge
> +	 * for converting up to 28 LCD LVTTL lanes into 5/4 LVDS lanes that
> +	 * go to display panel's receiver.
> +	 *
> +	 * Encoder usually have a power-down control which needs to be enabled
> +	 * in order to transmit data to the panel. Historically devices that
> +	 * use an older device-tree version didn't model the bridge, assuming
> +	 * that encoder is turned ON by default, while today's DRM allows us
> +	 * to model LVDS encoder properly.
> +	 *
> +	 * Newer device-trees utilize LVDS encoder bridge, which provides
> +	 * us with a connector and handles the display panel.
> +	 *
> +	 * For older device-trees we fall back to our own connector and use
> +	 * nvidia,panel phandle.

As I tried to explain before, if you wrap the panel in a bridge with
drm_panel_bridge_add() (or the devm_ variant), you will always have a
bridge associated with the output, and will be able to remove your
custom connector implementation. I thus recommend converting to
drm_panel_bridge_add() either as part of this patch series, or just
after it, to get full benefits.

With the assumption that this will be handled,

Reviewed-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>

> +	 */
> +	if (output->bridge) {
> +		err = drm_bridge_attach(&output->encoder, output->bridge,
> +					NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> +		if (err) {
> +			dev_err(output->dev, "failed to attach bridge: %d\n",
> +				err);
> +			return err;
> +		}
> +
> +		connector = drm_bridge_connector_init(drm, &output->encoder);
> +		if (IS_ERR(connector)) {
> +			dev_err(output->dev,
> +				"failed to initialize bridge connector: %pe\n",
> +				connector);
> +			return PTR_ERR(connector);
> +		}
> +
> +		drm_connector_attach_encoder(connector, &output->encoder);
> +	} else {
> +		drm_connector_init(drm, &output->connector,
> +				   &tegra_rgb_connector_funcs,
> +				   DRM_MODE_CONNECTOR_LVDS);
> +		drm_connector_helper_add(&output->connector,
> +					 &tegra_rgb_connector_helper_funcs);
> +		output->connector.dpms = DRM_MODE_DPMS_OFF;
> +
> +		drm_connector_attach_encoder(&output->connector,
> +					     &output->encoder);
> +		drm_connector_register(&output->connector);
> +	}
>  
>  	err = tegra_output_init(drm, output);
>  	if (err < 0) {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 3/3] drm/tegra: output: rgb: Support LVDS encoder bridge

[Laurent Pinchart]

Hi Dmitry,

Thank you for the patch.

On Fri, Apr 17, 2020 at 08:52:38PM +0300, Dmitry Osipenko wrote:
> Newer Tegra device-trees will specify a video output graph, which involves
> LVDS encoder bridge. This patch adds support for the LVDS encoder bridge
> to the RGB output, allowing us to model the display hardware properly.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/gpu/drm/tegra/rgb.c | 58 +++++++++++++++++++++++++++++++------
>  1 file changed, 49 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
> index 0562a7eb793f..f28a226914c0 100644
> --- a/drivers/gpu/drm/tegra/rgb.c
> +++ b/drivers/gpu/drm/tegra/rgb.c
> @@ -7,6 +7,7 @@
>  #include <linux/clk.h>
>  
>  #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge_connector.h>
>  #include <drm/drm_panel.h>
>  #include <drm/drm_simple_kms_helper.h>
>  
> @@ -267,24 +268,63 @@ int tegra_dc_rgb_remove(struct tegra_dc *dc)
>  int tegra_dc_rgb_init(struct drm_device *drm, struct tegra_dc *dc)
>  {
>  	struct tegra_output *output = dc->rgb;
> +	struct drm_connector *connector;
>  	int err;
>  
>  	if (!dc->rgb)
>  		return -ENODEV;
>  
> -	drm_connector_init(drm, &output->connector, &tegra_rgb_connector_funcs,
> -			   DRM_MODE_CONNECTOR_LVDS);
> -	drm_connector_helper_add(&output->connector,
> -				 &tegra_rgb_connector_helper_funcs);
> -	output->connector.dpms = DRM_MODE_DPMS_OFF;
> -
>  	drm_simple_encoder_init(drm, &output->encoder, DRM_MODE_ENCODER_LVDS);
>  	drm_encoder_helper_add(&output->encoder,
>  			       &tegra_rgb_encoder_helper_funcs);
>  
> -	drm_connector_attach_encoder(&output->connector,
> -					  &output->encoder);
> -	drm_connector_register(&output->connector);
> +	/*
> +	 * Tegra devices that have LVDS panel utilize LVDS encoder bridge
> +	 * for converting up to 28 LCD LVTTL lanes into 5/4 LVDS lanes that
> +	 * go to display panel's receiver.
> +	 *
> +	 * Encoder usually have a power-down control which needs to be enabled
> +	 * in order to transmit data to the panel. Historically devices that
> +	 * use an older device-tree version didn't model the bridge, assuming
> +	 * that encoder is turned ON by default, while today's DRM allows us
> +	 * to model LVDS encoder properly.
> +	 *
> +	 * Newer device-trees utilize LVDS encoder bridge, which provides
> +	 * us with a connector and handles the display panel.
> +	 *
> +	 * For older device-trees we fall back to our own connector and use
> +	 * nvidia,panel phandle.

As I tried to explain before, if you wrap the panel in a bridge with
drm_panel_bridge_add() (or the devm_ variant), you will always have a
bridge associated with the output, and will be able to remove your
custom connector implementation. I thus recommend converting to
drm_panel_bridge_add() either as part of this patch series, or just
after it, to get full benefits.

With the assumption that this will be handled,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	 */
> +	if (output->bridge) {
> +		err = drm_bridge_attach(&output->encoder, output->bridge,
> +					NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> +		if (err) {
> +			dev_err(output->dev, "failed to attach bridge: %d\n",
> +				err);
> +			return err;
> +		}
> +
> +		connector = drm_bridge_connector_init(drm, &output->encoder);
> +		if (IS_ERR(connector)) {
> +			dev_err(output->dev,
> +				"failed to initialize bridge connector: %pe\n",
> +				connector);
> +			return PTR_ERR(connector);
> +		}
> +
> +		drm_connector_attach_encoder(connector, &output->encoder);
> +	} else {
> +		drm_connector_init(drm, &output->connector,
> +				   &tegra_rgb_connector_funcs,
> +				   DRM_MODE_CONNECTOR_LVDS);
> +		drm_connector_helper_add(&output->connector,
> +					 &tegra_rgb_connector_helper_funcs);
> +		output->connector.dpms = DRM_MODE_DPMS_OFF;
> +
> +		drm_connector_attach_encoder(&output->connector,
> +					     &output->encoder);
> +		drm_connector_register(&output->connector);
> +	}
>  
>  	err = tegra_output_init(drm, output);
>  	if (err < 0) {

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/3] drm/tegra: output: Support DRM bridges

[Laurent Pinchart]

Hi Dmitry,

Thank you for the patch.

On Fri, Apr 17, 2020 at 08:52:37PM +0300, Dmitry Osipenko wrote:
> Newer Tegra device-trees will specify a video output graph which involves
> a bridge. This patch adds initial support for the DRM bridges to the
> Tegra's DRM output.
> 
> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/gpu/drm/tegra/drm.h    |  2 ++
>  drivers/gpu/drm/tegra/output.c | 13 ++++++++++++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
> index 804869799305..cccd368b6752 100644
> --- a/drivers/gpu/drm/tegra/drm.h
> +++ b/drivers/gpu/drm/tegra/drm.h
> @@ -12,6 +12,7 @@
>  #include <linux/of_gpio.h>
>  
>  #include <drm/drm_atomic.h>
> +#include <drm/drm_bridge.h>

You could add a forward declaration of struct drm_bridge instead, that
can lower the compilation time a little bit.

>  #include <drm/drm_edid.h>
>  #include <drm/drm_encoder.h>
>  #include <drm/drm_fb_helper.h>
> @@ -116,6 +117,7 @@ struct tegra_output {
>  	struct device_node *of_node;
>  	struct device *dev;
>  
> +	struct drm_bridge *bridge;
>  	struct drm_panel *panel;
>  	struct i2c_adapter *ddc;
>  	const struct edid *edid;
> diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
> index a6a711d54e88..ec0cd4a1ced1 100644
> --- a/drivers/gpu/drm/tegra/output.c
> +++ b/drivers/gpu/drm/tegra/output.c
> @@ -5,6 +5,7 @@
>   */
>  
>  #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_of.h>
>  #include <drm/drm_panel.h>
>  #include <drm/drm_simple_kms_helper.h>
>  
> @@ -92,13 +93,23 @@ static irqreturn_t hpd_irq(int irq, void *data)
>  
>  int tegra_output_probe(struct tegra_output *output)
>  {
> -	struct device_node *ddc, *panel;
> +	struct device_node *ddc, *panel, *port;
>  	unsigned long flags;
>  	int err, size;
>  
>  	if (!output->of_node)
>  		output->of_node = output->dev->of_node;
>  
> +	port = of_get_child_by_name(output->of_node, "port");

Do you need to check for the presence of a port node first ? Can you
just check the return value of drm_of_find_panel_or_bridge(), and fall
back to "nvidia,panel" if it returns -ENODEV ?

> +	if (port) {
> +		err = drm_of_find_panel_or_bridge(output->of_node, 0, 0, NULL,
> +						  &output->bridge);
> +		of_node_put(port);
> +
> +		if (err)
> +			return err;
> +	}
> +
>  	panel = of_parse_phandle(output->of_node, "nvidia,panel", 0);
>  	if (panel) {
>  		output->panel = of_drm_find_panel(panel);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 2/3] drm/tegra: output: Support DRM bridges

[Laurent Pinchart]

Hi Dmitry,

Thank you for the patch.

On Fri, Apr 17, 2020 at 08:52:37PM +0300, Dmitry Osipenko wrote:
> Newer Tegra device-trees will specify a video output graph which involves
> a bridge. This patch adds initial support for the DRM bridges to the
> Tegra's DRM output.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/gpu/drm/tegra/drm.h    |  2 ++
>  drivers/gpu/drm/tegra/output.c | 13 ++++++++++++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tegra/drm.h b/drivers/gpu/drm/tegra/drm.h
> index 804869799305..cccd368b6752 100644
> --- a/drivers/gpu/drm/tegra/drm.h
> +++ b/drivers/gpu/drm/tegra/drm.h
> @@ -12,6 +12,7 @@
>  #include <linux/of_gpio.h>
>  
>  #include <drm/drm_atomic.h>
> +#include <drm/drm_bridge.h>

You could add a forward declaration of struct drm_bridge instead, that
can lower the compilation time a little bit.

>  #include <drm/drm_edid.h>
>  #include <drm/drm_encoder.h>
>  #include <drm/drm_fb_helper.h>
> @@ -116,6 +117,7 @@ struct tegra_output {
>  	struct device_node *of_node;
>  	struct device *dev;
>  
> +	struct drm_bridge *bridge;
>  	struct drm_panel *panel;
>  	struct i2c_adapter *ddc;
>  	const struct edid *edid;
> diff --git a/drivers/gpu/drm/tegra/output.c b/drivers/gpu/drm/tegra/output.c
> index a6a711d54e88..ec0cd4a1ced1 100644
> --- a/drivers/gpu/drm/tegra/output.c
> +++ b/drivers/gpu/drm/tegra/output.c
> @@ -5,6 +5,7 @@
>   */
>  
>  #include <drm/drm_atomic_helper.h>
> +#include <drm/drm_of.h>
>  #include <drm/drm_panel.h>
>  #include <drm/drm_simple_kms_helper.h>
>  
> @@ -92,13 +93,23 @@ static irqreturn_t hpd_irq(int irq, void *data)
>  
>  int tegra_output_probe(struct tegra_output *output)
>  {
> -	struct device_node *ddc, *panel;
> +	struct device_node *ddc, *panel, *port;
>  	unsigned long flags;
>  	int err, size;
>  
>  	if (!output->of_node)
>  		output->of_node = output->dev->of_node;
>  
> +	port = of_get_child_by_name(output->of_node, "port");

Do you need to check for the presence of a port node first ? Can you
just check the return value of drm_of_find_panel_or_bridge(), and fall
back to "nvidia,panel" if it returns -ENODEV ?

> +	if (port) {
> +		err = drm_of_find_panel_or_bridge(output->of_node, 0, 0, NULL,
> +						  &output->bridge);
> +		of_node_put(port);
> +
> +		if (err)
> +			return err;
> +	}
> +
>  	panel = of_parse_phandle(output->of_node, "nvidia,panel", 0);
>  	if (panel) {
>  		output->panel = of_drm_find_panel(panel);

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/3] drm/tegra: output: Support DRM bridges

[Dmitry Osipenko]

Hello Laurent,

17.04.2020 22:30, Laurent Pinchart пишет:
...
>>  #include <drm/drm_atomic.h>
>> +#include <drm/drm_bridge.h>
> 
> You could add a forward declaration of struct drm_bridge instead, that
> can lower the compilation time a little bit.

This include is not only for the struct, but also for the
drm_bridge_attach(). It looks to me that it should be nicer to keep the
include here.

...
>> +	port = of_get_child_by_name(output->of_node, "port");
> 
> Do you need to check for the presence of a port node first ? Can you
> just check the return value of drm_of_find_panel_or_bridge(), and fall
> back to "nvidia,panel" if it returns -ENODEV ?

Without the check, the drm_of_find_panel_or_bridge() prints a very noisy
error message about missing port node for every output that doesn't have
a graph specified in a device-tree (HDMI, DSI and etc).

https://elixir.bootlin.com/linux/v5.7-rc1/source/drivers/of/property.c#L621

Re: [PATCH v4 2/3] drm/tegra: output: Support DRM bridges

[Dmitry Osipenko]

Hello Laurent,

17.04.2020 22:30, Laurent Pinchart пишет:
...
>>  #include <drm/drm_atomic.h>
>> +#include <drm/drm_bridge.h>
> 
> You could add a forward declaration of struct drm_bridge instead, that
> can lower the compilation time a little bit.

This include is not only for the struct, but also for the
drm_bridge_attach(). It looks to me that it should be nicer to keep the
include here.

...
>> +	port = of_get_child_by_name(output->of_node, "port");
> 
> Do you need to check for the presence of a port node first ? Can you
> just check the return value of drm_of_find_panel_or_bridge(), and fall
> back to "nvidia,panel" if it returns -ENODEV ?

Without the check, the drm_of_find_panel_or_bridge() prints a very noisy
error message about missing port node for every output that doesn't have
a graph specified in a device-tree (HDMI, DSI and etc).

https://elixir.bootlin.com/linux/v5.7-rc1/source/drivers/of/property.c#L621
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 3/3] drm/tegra: output: rgb: Support LVDS encoder bridge

[Dmitry Osipenko]

17.04.2020 22:24, Laurent Pinchart пишет:
...
> As I tried to explain before, if you wrap the panel in a bridge with
> drm_panel_bridge_add() (or the devm_ variant), you will always have a
> bridge associated with the output, and will be able to remove your
> custom connector implementation. I thus recommend converting to
> drm_panel_bridge_add() either as part of this patch series, or just
> after it, to get full benefits.
> 
> With the assumption that this will be handled,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>

Thanks you very much!

Yes, I got yours point about wrapping panel into the bridge. But I don't
think that it's worth the effort right now because each Tegra output has
it's own implantation of the connector and it should be cleaner not to
touch that code.

Secondly, I don't have hardware to test all available panel output types
on Tegra and the benefits of messing with all that code are a bit dim to me.

I can make a patch to wrap the RGB panel into a bridge, but this should
make code a bit inconsistent in regards to not having a common code path
for the "legacy" nvidia,panel. So perhaps it's better to leave it all
as-is for now.

Re: [PATCH v4 3/3] drm/tegra: output: rgb: Support LVDS encoder bridge

[Dmitry Osipenko]

17.04.2020 22:24, Laurent Pinchart пишет:
...
> As I tried to explain before, if you wrap the panel in a bridge with
> drm_panel_bridge_add() (or the devm_ variant), you will always have a
> bridge associated with the output, and will be able to remove your
> custom connector implementation. I thus recommend converting to
> drm_panel_bridge_add() either as part of this patch series, or just
> after it, to get full benefits.
> 
> With the assumption that this will be handled,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks you very much!

Yes, I got yours point about wrapping panel into the bridge. But I don't
think that it's worth the effort right now because each Tegra output has
it's own implantation of the connector and it should be cleaner not to
touch that code.

Secondly, I don't have hardware to test all available panel output types
on Tegra and the benefits of messing with all that code are a bit dim to me.

I can make a patch to wrap the RGB panel into a bridge, but this should
make code a bit inconsistent in regards to not having a common code path
for the "legacy" nvidia,panel. So perhaps it's better to leave it all
as-is for now.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/3] drm/tegra: output: Support DRM bridges

[Laurent Pinchart]

Hi Dmitry,

On Fri, Apr 17, 2020 at 10:41:59PM +0300, Dmitry Osipenko wrote:
> 17.04.2020 22:30, Laurent Pinchart пишет:
> ...
> >>  #include <drm/drm_atomic.h>
> >> +#include <drm/drm_bridge.h>
> > 
> > You could add a forward declaration of struct drm_bridge instead, that
> > can lower the compilation time a little bit.
> 
> This include is not only for the struct, but also for the
> drm_bridge_attach(). It looks to me that it should be nicer to keep the
> include here.

drm_bridge_attach() is called from .c files. In the .h file you can use
a forward declaration. It's entirely up to you, but as a general rule, I
personally try to use forward structure declarations in .h files as much
as possible.

> ...
> >> +	port = of_get_child_by_name(output->of_node, "port");
> > 
> > Do you need to check for the presence of a port node first ? Can you
> > just check the return value of drm_of_find_panel_or_bridge(), and fall
> > back to "nvidia,panel" if it returns -ENODEV ?
> 
> Without the check, the drm_of_find_panel_or_bridge() prints a very noisy
> error message about missing port node for every output that doesn't have
> a graph specified in a device-tree (HDMI, DSI and etc).
> 
> https://elixir.bootlin.com/linux/v5.7-rc1/source/drivers/of/property.c#L621

Ah yes indeed. That's not very nice.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 2/3] drm/tegra: output: Support DRM bridges

[Laurent Pinchart]

Hi Dmitry,

On Fri, Apr 17, 2020 at 10:41:59PM +0300, Dmitry Osipenko wrote:
> 17.04.2020 22:30, Laurent Pinchart пишет:
> ...
> >>  #include <drm/drm_atomic.h>
> >> +#include <drm/drm_bridge.h>
> > 
> > You could add a forward declaration of struct drm_bridge instead, that
> > can lower the compilation time a little bit.
> 
> This include is not only for the struct, but also for the
> drm_bridge_attach(). It looks to me that it should be nicer to keep the
> include here.

drm_bridge_attach() is called from .c files. In the .h file you can use
a forward declaration. It's entirely up to you, but as a general rule, I
personally try to use forward structure declarations in .h files as much
as possible.

> ...
> >> +	port = of_get_child_by_name(output->of_node, "port");
> > 
> > Do you need to check for the presence of a port node first ? Can you
> > just check the return value of drm_of_find_panel_or_bridge(), and fall
> > back to "nvidia,panel" if it returns -ENODEV ?
> 
> Without the check, the drm_of_find_panel_or_bridge() prints a very noisy
> error message about missing port node for every output that doesn't have
> a graph specified in a device-tree (HDMI, DSI and etc).
> 
> https://elixir.bootlin.com/linux/v5.7-rc1/source/drivers/of/property.c#L621

Ah yes indeed. That's not very nice.

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 3/3] drm/tegra: output: rgb: Support LVDS encoder bridge

[Laurent Pinchart]

On Fri, Apr 17, 2020 at 11:11:06PM +0300, Dmitry Osipenko wrote:
> 17.04.2020 22:24, Laurent Pinchart пишет:
> ...
> > As I tried to explain before, if you wrap the panel in a bridge with
> > drm_panel_bridge_add() (or the devm_ variant), you will always have a
> > bridge associated with the output, and will be able to remove your
> > custom connector implementation. I thus recommend converting to
> > drm_panel_bridge_add() either as part of this patch series, or just
> > after it, to get full benefits.
> > 
> > With the assumption that this will be handled,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
> 
> Thanks you very much!
> 
> Yes, I got yours point about wrapping panel into the bridge. But I don't
> think that it's worth the effort right now because each Tegra output has
> it's own implantation of the connector and it should be cleaner not to
> touch that code.
> 
> Secondly, I don't have hardware to test all available panel output types
> on Tegra and the benefits of messing with all that code are a bit dim to me.
> 
> I can make a patch to wrap the RGB panel into a bridge, but this should
> make code a bit inconsistent in regards to not having a common code path
> for the "legacy" nvidia,panel. So perhaps it's better to leave it all
> as-is for now.

I had a brief look at the code, converting the different output types
one by one would be a better way forward than not doing anything at all
in my opinion :-) Once you convert the first output it will also serve
as an example on how to do it, and hopefully other developers with
access to hardware could then do more conversions.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 3/3] drm/tegra: output: rgb: Support LVDS encoder bridge

[Laurent Pinchart]

On Fri, Apr 17, 2020 at 11:11:06PM +0300, Dmitry Osipenko wrote:
> 17.04.2020 22:24, Laurent Pinchart пишет:
> ...
> > As I tried to explain before, if you wrap the panel in a bridge with
> > drm_panel_bridge_add() (or the devm_ variant), you will always have a
> > bridge associated with the output, and will be able to remove your
> > custom connector implementation. I thus recommend converting to
> > drm_panel_bridge_add() either as part of this patch series, or just
> > after it, to get full benefits.
> > 
> > With the assumption that this will be handled,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Thanks you very much!
> 
> Yes, I got yours point about wrapping panel into the bridge. But I don't
> think that it's worth the effort right now because each Tegra output has
> it's own implantation of the connector and it should be cleaner not to
> touch that code.
> 
> Secondly, I don't have hardware to test all available panel output types
> on Tegra and the benefits of messing with all that code are a bit dim to me.
> 
> I can make a patch to wrap the RGB panel into a bridge, but this should
> make code a bit inconsistent in regards to not having a common code path
> for the "legacy" nvidia,panel. So perhaps it's better to leave it all
> as-is for now.

I had a brief look at the code, converting the different output types
one by one would be a better way forward than not doing anything at all
in my opinion :-) Once you convert the first output it will also serve
as an example on how to do it, and hopefully other developers with
access to hardware could then do more conversions.

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 3/3] drm/tegra: output: rgb: Support LVDS encoder bridge

[Dmitry Osipenko]

17.04.2020 23:34, Laurent Pinchart пишет:
> On Fri, Apr 17, 2020 at 11:11:06PM +0300, Dmitry Osipenko wrote:
>> 17.04.2020 22:24, Laurent Pinchart пишет:
>> ...
>>> As I tried to explain before, if you wrap the panel in a bridge with
>>> drm_panel_bridge_add() (or the devm_ variant), you will always have a
>>> bridge associated with the output, and will be able to remove your
>>> custom connector implementation. I thus recommend converting to
>>> drm_panel_bridge_add() either as part of this patch series, or just
>>> after it, to get full benefits.
>>>
>>> With the assumption that this will be handled,
>>>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
>>
>> Thanks you very much!
>>
>> Yes, I got yours point about wrapping panel into the bridge. But I don't
>> think that it's worth the effort right now because each Tegra output has
>> it's own implantation of the connector and it should be cleaner not to
>> touch that code.
>>
>> Secondly, I don't have hardware to test all available panel output types
>> on Tegra and the benefits of messing with all that code are a bit dim to me.
>>
>> I can make a patch to wrap the RGB panel into a bridge, but this should
>> make code a bit inconsistent in regards to not having a common code path
>> for the "legacy" nvidia,panel. So perhaps it's better to leave it all
>> as-is for now.
> 
> I had a brief look at the code, converting the different output types
> one by one would be a better way forward than not doing anything at all
> in my opinion :-) Once you convert the first output it will also serve
> as an example on how to do it, and hopefully other developers with
> access to hardware could then do more conversions.
> 

Thierry, would you want to have RGB panel converted into a bridge? If
yes, I'll make a v5 with that change.

Re: [PATCH v4 3/3] drm/tegra: output: rgb: Support LVDS encoder bridge

[Dmitry Osipenko]

17.04.2020 23:34, Laurent Pinchart пишет:
> On Fri, Apr 17, 2020 at 11:11:06PM +0300, Dmitry Osipenko wrote:
>> 17.04.2020 22:24, Laurent Pinchart пишет:
>> ...
>>> As I tried to explain before, if you wrap the panel in a bridge with
>>> drm_panel_bridge_add() (or the devm_ variant), you will always have a
>>> bridge associated with the output, and will be able to remove your
>>> custom connector implementation. I thus recommend converting to
>>> drm_panel_bridge_add() either as part of this patch series, or just
>>> after it, to get full benefits.
>>>
>>> With the assumption that this will be handled,
>>>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>
>> Thanks you very much!
>>
>> Yes, I got yours point about wrapping panel into the bridge. But I don't
>> think that it's worth the effort right now because each Tegra output has
>> it's own implantation of the connector and it should be cleaner not to
>> touch that code.
>>
>> Secondly, I don't have hardware to test all available panel output types
>> on Tegra and the benefits of messing with all that code are a bit dim to me.
>>
>> I can make a patch to wrap the RGB panel into a bridge, but this should
>> make code a bit inconsistent in regards to not having a common code path
>> for the "legacy" nvidia,panel. So perhaps it's better to leave it all
>> as-is for now.
> 
> I had a brief look at the code, converting the different output types
> one by one would be a better way forward than not doing anything at all
> in my opinion :-) Once you convert the first output it will also serve
> as an example on how to do it, and hopefully other developers with
> access to hardware could then do more conversions.
> 

Thierry, would you want to have RGB panel converted into a bridge? If
yes, I'll make a v5 with that change.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/3] drm/tegra: output: Support DRM bridges

[Dmitry Osipenko]

17.04.2020 23:31, Laurent Pinchart пишет:
> Hi Dmitry,
> 
> On Fri, Apr 17, 2020 at 10:41:59PM +0300, Dmitry Osipenko wrote:
>> 17.04.2020 22:30, Laurent Pinchart пишет:
>> ...
>>>>  #include <drm/drm_atomic.h>
>>>> +#include <drm/drm_bridge.h>
>>>
>>> You could add a forward declaration of struct drm_bridge instead, that
>>> can lower the compilation time a little bit.
>>
>> This include is not only for the struct, but also for the
>> drm_bridge_attach(). It looks to me that it should be nicer to keep the
>> include here.
> 
> drm_bridge_attach() is called from .c files. In the .h file you can use
> a forward declaration. It's entirely up to you, but as a general rule, I
> personally try to use forward structure declarations in .h files as much
> as possible.

The current Tegra DRM code is a bit inconsistent in regards to having
forward declarations, it doesn't have them more than have.

I'll add a forward declaration if there will be need to make a v5, ok?

>> ...
>>>> +	port = of_get_child_by_name(output->of_node, "port");
>>>
>>> Do you need to check for the presence of a port node first ? Can you
>>> just check the return value of drm_of_find_panel_or_bridge(), and fall
>>> back to "nvidia,panel" if it returns -ENODEV ?
>>
>> Without the check, the drm_of_find_panel_or_bridge() prints a very noisy
>> error message about missing port node for every output that doesn't have
>> a graph specified in a device-tree (HDMI, DSI and etc).
>>
>> https://elixir.bootlin.com/linux/v5.7-rc1/source/drivers/of/property.c#L621
> 
> Ah yes indeed. That's not very nice.
> 

Please let me know if you'll have a better idea about how this could be
handled.

Re: [PATCH v4 2/3] drm/tegra: output: Support DRM bridges

[Dmitry Osipenko]

17.04.2020 23:31, Laurent Pinchart пишет:
> Hi Dmitry,
> 
> On Fri, Apr 17, 2020 at 10:41:59PM +0300, Dmitry Osipenko wrote:
>> 17.04.2020 22:30, Laurent Pinchart пишет:
>> ...
>>>>  #include <drm/drm_atomic.h>
>>>> +#include <drm/drm_bridge.h>
>>>
>>> You could add a forward declaration of struct drm_bridge instead, that
>>> can lower the compilation time a little bit.
>>
>> This include is not only for the struct, but also for the
>> drm_bridge_attach(). It looks to me that it should be nicer to keep the
>> include here.
> 
> drm_bridge_attach() is called from .c files. In the .h file you can use
> a forward declaration. It's entirely up to you, but as a general rule, I
> personally try to use forward structure declarations in .h files as much
> as possible.

The current Tegra DRM code is a bit inconsistent in regards to having
forward declarations, it doesn't have them more than have.

I'll add a forward declaration if there will be need to make a v5, ok?

>> ...
>>>> +	port = of_get_child_by_name(output->of_node, "port");
>>>
>>> Do you need to check for the presence of a port node first ? Can you
>>> just check the return value of drm_of_find_panel_or_bridge(), and fall
>>> back to "nvidia,panel" if it returns -ENODEV ?
>>
>> Without the check, the drm_of_find_panel_or_bridge() prints a very noisy
>> error message about missing port node for every output that doesn't have
>> a graph specified in a device-tree (HDMI, DSI and etc).
>>
>> https://elixir.bootlin.com/linux/v5.7-rc1/source/drivers/of/property.c#L621
> 
> Ah yes indeed. That's not very nice.
> 

Please let me know if you'll have a better idea about how this could be
handled.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/3] drm/tegra: output: Support DRM bridges

[Laurent Pinchart]

Hi Dmitry,

On Fri, Apr 17, 2020 at 11:52:11PM +0300, Dmitry Osipenko wrote:
> 17.04.2020 23:31, Laurent Pinchart пишет:
> > On Fri, Apr 17, 2020 at 10:41:59PM +0300, Dmitry Osipenko wrote:
> >> 17.04.2020 22:30, Laurent Pinchart пишет:
> >> ...
> >>>>  #include <drm/drm_atomic.h>
> >>>> +#include <drm/drm_bridge.h>
> >>>
> >>> You could add a forward declaration of struct drm_bridge instead, that
> >>> can lower the compilation time a little bit.
> >>
> >> This include is not only for the struct, but also for the
> >> drm_bridge_attach(). It looks to me that it should be nicer to keep the
> >> include here.
> > 
> > drm_bridge_attach() is called from .c files. In the .h file you can use
> > a forward declaration. It's entirely up to you, but as a general rule, I
> > personally try to use forward structure declarations in .h files as much
> > as possible.
> 
> The current Tegra DRM code is a bit inconsistent in regards to having
> forward declarations, it doesn't have them more than have.
> 
> I'll add a forward declaration if there will be need to make a v5, ok?

It's up to you, you don't have to use a forward declaration if you don't
want to, I was just pointing out what I think is a best practice rule
:-)

> >> ...
> >>>> +	port = of_get_child_by_name(output->of_node, "port");
> >>>
> >>> Do you need to check for the presence of a port node first ? Can you
> >>> just check the return value of drm_of_find_panel_or_bridge(), and fall
> >>> back to "nvidia,panel" if it returns -ENODEV ?
> >>
> >> Without the check, the drm_of_find_panel_or_bridge() prints a very noisy
> >> error message about missing port node for every output that doesn't have
> >> a graph specified in a device-tree (HDMI, DSI and etc).
> >>
> >> https://elixir.bootlin.com/linux/v5.7-rc1/source/drivers/of/property.c#L621
> > 
> > Ah yes indeed. That's not very nice.
> 
> Please let me know if you'll have a better idea about how this could be
> handled.

It should be good enough as-is I think. You may however want to support
both "port" and "ports", as even when there's a single port node, it
could be put inside a ports node.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 2/3] drm/tegra: output: Support DRM bridges

[Laurent Pinchart]

Hi Dmitry,

On Fri, Apr 17, 2020 at 11:52:11PM +0300, Dmitry Osipenko wrote:
> 17.04.2020 23:31, Laurent Pinchart пишет:
> > On Fri, Apr 17, 2020 at 10:41:59PM +0300, Dmitry Osipenko wrote:
> >> 17.04.2020 22:30, Laurent Pinchart пишет:
> >> ...
> >>>>  #include <drm/drm_atomic.h>
> >>>> +#include <drm/drm_bridge.h>
> >>>
> >>> You could add a forward declaration of struct drm_bridge instead, that
> >>> can lower the compilation time a little bit.
> >>
> >> This include is not only for the struct, but also for the
> >> drm_bridge_attach(). It looks to me that it should be nicer to keep the
> >> include here.
> > 
> > drm_bridge_attach() is called from .c files. In the .h file you can use
> > a forward declaration. It's entirely up to you, but as a general rule, I
> > personally try to use forward structure declarations in .h files as much
> > as possible.
> 
> The current Tegra DRM code is a bit inconsistent in regards to having
> forward declarations, it doesn't have them more than have.
> 
> I'll add a forward declaration if there will be need to make a v5, ok?

It's up to you, you don't have to use a forward declaration if you don't
want to, I was just pointing out what I think is a best practice rule
:-)

> >> ...
> >>>> +	port = of_get_child_by_name(output->of_node, "port");
> >>>
> >>> Do you need to check for the presence of a port node first ? Can you
> >>> just check the return value of drm_of_find_panel_or_bridge(), and fall
> >>> back to "nvidia,panel" if it returns -ENODEV ?
> >>
> >> Without the check, the drm_of_find_panel_or_bridge() prints a very noisy
> >> error message about missing port node for every output that doesn't have
> >> a graph specified in a device-tree (HDMI, DSI and etc).
> >>
> >> https://elixir.bootlin.com/linux/v5.7-rc1/source/drivers/of/property.c#L621
> > 
> > Ah yes indeed. That's not very nice.
> 
> Please let me know if you'll have a better idea about how this could be
> handled.

It should be good enough as-is I think. You may however want to support
both "port" and "ports", as even when there's a single port node, it
could be put inside a ports node.

-- 
Regards,

Laurent Pinchart
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 2/3] drm/tegra: output: Support DRM bridges

[Dmitry Osipenko]

17.04.2020 23:58, Laurent Pinchart пишет:
> Hi Dmitry,
> 
> On Fri, Apr 17, 2020 at 11:52:11PM +0300, Dmitry Osipenko wrote:
>> 17.04.2020 23:31, Laurent Pinchart пишет:
>>> On Fri, Apr 17, 2020 at 10:41:59PM +0300, Dmitry Osipenko wrote:
>>>> 17.04.2020 22:30, Laurent Pinchart пишет:
>>>> ...
>>>>>>  #include <drm/drm_atomic.h>
>>>>>> +#include <drm/drm_bridge.h>
>>>>>
>>>>> You could add a forward declaration of struct drm_bridge instead, that
>>>>> can lower the compilation time a little bit.
>>>>
>>>> This include is not only for the struct, but also for the
>>>> drm_bridge_attach(). It looks to me that it should be nicer to keep the
>>>> include here.
>>>
>>> drm_bridge_attach() is called from .c files. In the .h file you can use
>>> a forward declaration. It's entirely up to you, but as a general rule, I
>>> personally try to use forward structure declarations in .h files as much
>>> as possible.
>>
>> The current Tegra DRM code is a bit inconsistent in regards to having
>> forward declarations, it doesn't have them more than have.
>>
>> I'll add a forward declaration if there will be need to make a v5, ok?
> 
> It's up to you, you don't have to use a forward declaration if you don't
> want to, I was just pointing out what I think is a best practice rule
> :-)

Alright, then I'll leave the include as-is in this patch since it should
be better to keep the code consistent even if it's a bit less optimal
than it could be, IMO.

We may return to cleaning up of driver includes later on.

>>>> ...
>>>>>> +	port = of_get_child_by_name(output->of_node, "port");
>>>>>
>>>>> Do you need to check for the presence of a port node first ? Can you
>>>>> just check the return value of drm_of_find_panel_or_bridge(), and fall
>>>>> back to "nvidia,panel" if it returns -ENODEV ?
>>>>
>>>> Without the check, the drm_of_find_panel_or_bridge() prints a very noisy
>>>> error message about missing port node for every output that doesn't have
>>>> a graph specified in a device-tree (HDMI, DSI and etc).
>>>>
>>>> https://elixir.bootlin.com/linux/v5.7-rc1/source/drivers/of/property.c#L621
>>>
>>> Ah yes indeed. That's not very nice.
>>
>> Please let me know if you'll have a better idea about how this could be
>> handled.
> 
> It should be good enough as-is I think. You may however want to support
> both "port" and "ports", as even when there's a single port node, it
> could be put inside a ports node.
> 

I'll make a v5 that will have additional patches for making
drm_of_find_panel_or_bridge() to better handle that case.

While at it, I'll also add a patch that will wrap RGB panel into bridge.

Thank you very much for the reviews!

Re: [PATCH v4 2/3] drm/tegra: output: Support DRM bridges

[Dmitry Osipenko]

17.04.2020 23:58, Laurent Pinchart пишет:
> Hi Dmitry,
> 
> On Fri, Apr 17, 2020 at 11:52:11PM +0300, Dmitry Osipenko wrote:
>> 17.04.2020 23:31, Laurent Pinchart пишет:
>>> On Fri, Apr 17, 2020 at 10:41:59PM +0300, Dmitry Osipenko wrote:
>>>> 17.04.2020 22:30, Laurent Pinchart пишет:
>>>> ...
>>>>>>  #include <drm/drm_atomic.h>
>>>>>> +#include <drm/drm_bridge.h>
>>>>>
>>>>> You could add a forward declaration of struct drm_bridge instead, that
>>>>> can lower the compilation time a little bit.
>>>>
>>>> This include is not only for the struct, but also for the
>>>> drm_bridge_attach(). It looks to me that it should be nicer to keep the
>>>> include here.
>>>
>>> drm_bridge_attach() is called from .c files. In the .h file you can use
>>> a forward declaration. It's entirely up to you, but as a general rule, I
>>> personally try to use forward structure declarations in .h files as much
>>> as possible.
>>
>> The current Tegra DRM code is a bit inconsistent in regards to having
>> forward declarations, it doesn't have them more than have.
>>
>> I'll add a forward declaration if there will be need to make a v5, ok?
> 
> It's up to you, you don't have to use a forward declaration if you don't
> want to, I was just pointing out what I think is a best practice rule
> :-)

Alright, then I'll leave the include as-is in this patch since it should
be better to keep the code consistent even if it's a bit less optimal
than it could be, IMO.

We may return to cleaning up of driver includes later on.

>>>> ...
>>>>>> +	port = of_get_child_by_name(output->of_node, "port");
>>>>>
>>>>> Do you need to check for the presence of a port node first ? Can you
>>>>> just check the return value of drm_of_find_panel_or_bridge(), and fall
>>>>> back to "nvidia,panel" if it returns -ENODEV ?
>>>>
>>>> Without the check, the drm_of_find_panel_or_bridge() prints a very noisy
>>>> error message about missing port node for every output that doesn't have
>>>> a graph specified in a device-tree (HDMI, DSI and etc).
>>>>
>>>> https://elixir.bootlin.com/linux/v5.7-rc1/source/drivers/of/property.c#L621
>>>
>>> Ah yes indeed. That's not very nice.
>>
>> Please let me know if you'll have a better idea about how this could be
>> handled.
> 
> It should be good enough as-is I think. You may however want to support
> both "port" and "ports", as even when there's a single port node, it
> could be put inside a ports node.
> 

I'll make a v5 that will have additional patches for making
drm_of_find_panel_or_bridge() to better handle that case.

While at it, I'll also add a patch that will wrap RGB panel into bridge.

Thank you very much for the reviews!
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel