[PATCH v3 0/2] Support DRM bridges on NVIDIA Tegra

[PATCH v3 0/2] 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:

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 (2):
  drm/tegra: output: Don't leak OF node on error
  drm/tegra: output: rgb: Support LVDS encoder bridge

 drivers/gpu/drm/tegra/drm.h    |  2 ++
 drivers/gpu/drm/tegra/output.c | 19 ++++++++++++++-----
 drivers/gpu/drm/tegra/rgb.c    | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 50 insertions(+), 5 deletions(-)

-- 
2.26.0

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

[PATCH v3 1/2] 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.

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 v3 2/2] drm/tegra: output: rgb: Support LVDS encoder bridge

[Dmitry Osipenko]

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

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpu/drm/tegra/drm.h    |  2 ++
 drivers/gpu/drm/tegra/output.c | 10 ++++++++++
 drivers/gpu/drm/tegra/rgb.c    | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+)

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..37fc6b8c173f 100644
--- a/drivers/gpu/drm/tegra/output.c
+++ b/drivers/gpu/drm/tegra/output.c
@@ -180,6 +180,16 @@ int tegra_output_init(struct drm_device *drm, struct tegra_output *output)
 	int connector_type;
 	int err;
 
+	if (output->bridge) {
+		err = drm_bridge_attach(&output->encoder, output->bridge,
+					NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+		if (err) {
+			dev_err(output->dev, "cannot connect bridge: %d\n",
+				err);
+			return err;
+		}
+	}
+
 	if (output->panel) {
 		err = drm_panel_attach(output->panel, &output->connector);
 		if (err < 0)
diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
index 0562a7eb793f..0df213e92664 100644
--- a/drivers/gpu/drm/tegra/rgb.c
+++ b/drivers/gpu/drm/tegra/rgb.c
@@ -5,6 +5,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/of_graph.h>
 
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_panel.h>
@@ -210,6 +211,7 @@ static const struct drm_encoder_helper_funcs tegra_rgb_encoder_helper_funcs = {
 
 int tegra_dc_rgb_probe(struct tegra_dc *dc)
 {
+	const unsigned int encoder_port = 0, panel_port = 1;
 	struct device_node *np;
 	struct tegra_rgb *rgb;
 	int err;
@@ -226,6 +228,38 @@ int tegra_dc_rgb_probe(struct tegra_dc *dc)
 	rgb->output.of_node = np;
 	rgb->dc = dc;
 
+	/*
+	 * Tegra devices that have LVDS panel utilize LVDS-encoder bridge
+	 * for converting 24/18 RGB data-lanes into 8 lanes. Encoder usually
+	 * have a powerdown control which needs to be enabled in order to
+	 * transfer 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 may utilize output->encoder->panel graph.
+	 *
+	 * For older device-trees we fall back to use nvidia,panel phandle.
+	 */
+	np = of_graph_get_remote_node(rgb->output.of_node, encoder_port, -1);
+	if (np) {
+		rgb->output.bridge = of_drm_find_bridge(np);
+		of_node_put(np);
+
+		if (!rgb->output.bridge)
+			return -EPROBE_DEFER;
+
+		np = of_graph_get_remote_node(rgb->output.bridge->of_node,
+					      panel_port, -1);
+		if (np) {
+			rgb->output.panel = of_drm_find_panel(np);
+			of_node_put(np);
+
+			if (IS_ERR(rgb->output.panel))
+				return PTR_ERR(rgb->output.panel);
+		}
+	}
+
 	err = tegra_output_probe(&rgb->output);
 	if (err < 0)
 		return err;
-- 
2.26.0

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

Re: [PATCH v3 1/2] drm/tegra: output: Don't leak OF node on error

[Laurent Pinchart]

Hi Dmitry,

Thank you for the patch.

On Thu, Apr 16, 2020 at 08:24:04PM +0300, Dmitry Osipenko wrote:
> The OF node should be put before returning error in tegra_output_probe(),
> otherwise node's refcount will be leaked.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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,

-- 
Regards,

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

Re: [PATCH v3 1/2] drm/tegra: output: Don't leak OF node on error

[Dmitry Osipenko]

16.04.2020 20:27, Laurent Pinchart пишет:
> Hi Dmitry,
> 
> Thank you for the patch.
> 
> On Thu, Apr 16, 2020 at 08:24:04PM +0300, Dmitry Osipenko wrote:
>> The OF node should be put before returning error in tegra_output_probe(),
>> otherwise node's refcount will be leaked.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Hello Laurent,

That is the fastest kernel review I ever got, thank you :)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 2/2] drm/tegra: output: rgb: Support LVDS encoder bridge

[Laurent Pinchart]

Hi Dmitry,

Thank you for the patch.

On Thu, Apr 16, 2020 at 08:24:05PM +0300, Dmitry Osipenko wrote:
> Newer Tegra device-trees will specify a video output graph that involves
> LVDS encoder bridge, This patch adds support for the LVDS encoder bridge
> to the RGB output, allowing us to model display hardware properly.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/gpu/drm/tegra/drm.h    |  2 ++
>  drivers/gpu/drm/tegra/output.c | 10 ++++++++++
>  drivers/gpu/drm/tegra/rgb.c    | 34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 46 insertions(+)
> 
> 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..37fc6b8c173f 100644
> --- a/drivers/gpu/drm/tegra/output.c
> +++ b/drivers/gpu/drm/tegra/output.c
> @@ -180,6 +180,16 @@ int tegra_output_init(struct drm_device *drm, struct tegra_output *output)
>  	int connector_type;
>  	int err;
>  
> +	if (output->bridge) {
> +		err = drm_bridge_attach(&output->encoder, output->bridge,
> +					NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);

Using DRM_BRIDGE_ATTACH_NO_CONNECTOR is definitely the way to go, but
please be aware that it will require creating a connector and an encoder
manually (which I think this driver already does), and using the bridge
operations to implement the connector operations. I highly recommend
using the DRM bridge connector helper for this purpose.

> +		if (err) {
> +			dev_err(output->dev, "cannot connect bridge: %d\n",
> +				err);
> +			return err;
> +		}
> +	}
> +
>  	if (output->panel) {

May I also recommend switching to the DRM panel bridge helper ? It will
simplify the code.

>  		err = drm_panel_attach(output->panel, &output->connector);
>  		if (err < 0)
> diff --git a/drivers/gpu/drm/tegra/rgb.c b/drivers/gpu/drm/tegra/rgb.c
> index 0562a7eb793f..0df213e92664 100644
> --- a/drivers/gpu/drm/tegra/rgb.c
> +++ b/drivers/gpu/drm/tegra/rgb.c
> @@ -5,6 +5,7 @@
>   */
>  
>  #include <linux/clk.h>
> +#include <linux/of_graph.h>
>  
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_panel.h>
> @@ -210,6 +211,7 @@ static const struct drm_encoder_helper_funcs tegra_rgb_encoder_helper_funcs = {
>  
>  int tegra_dc_rgb_probe(struct tegra_dc *dc)
>  {
> +	const unsigned int encoder_port = 0, panel_port = 1;
>  	struct device_node *np;
>  	struct tegra_rgb *rgb;
>  	int err;
> @@ -226,6 +228,38 @@ int tegra_dc_rgb_probe(struct tegra_dc *dc)
>  	rgb->output.of_node = np;
>  	rgb->dc = dc;
>  
> +	/*
> +	 * Tegra devices that have LVDS panel utilize LVDS-encoder bridge
> +	 * for converting 24/18 RGB data-lanes into 8 lanes. Encoder usually
> +	 * have a powerdown control which needs to be enabled in order to
> +	 * transfer 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 may utilize output->encoder->panel graph.
> +	 *
> +	 * For older device-trees we fall back to use nvidia,panel phandle.
> +	 */
> +	np = of_graph_get_remote_node(rgb->output.of_node, encoder_port, -1);
> +	if (np) {
> +		rgb->output.bridge = of_drm_find_bridge(np);
> +		of_node_put(np);
> +
> +		if (!rgb->output.bridge)
> +			return -EPROBE_DEFER;
> +
> +		np = of_graph_get_remote_node(rgb->output.bridge->of_node,
> +					      panel_port, -1);

This shouldn't be needed, the LVDS codec bridge driver should already
get the panel and handle it internally. You only need to handle panels
in this driver when they're connected directly to the RGB input.

> +		if (np) {
> +			rgb->output.panel = of_drm_find_panel(np);
> +			of_node_put(np);
> +
> +			if (IS_ERR(rgb->output.panel))
> +				return PTR_ERR(rgb->output.panel);
> +		}
> +	}
> +
>  	err = tegra_output_probe(&rgb->output);
>  	if (err < 0)
>  		return err;

-- 
Regards,

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

Re: [PATCH v3 1/2] drm/tegra: output: Don't leak OF node on error

[Laurent Pinchart]

Hi Dmitry,

On Thu, Apr 16, 2020 at 08:30:24PM +0300, Dmitry Osipenko wrote:
> 16.04.2020 20:27, Laurent Pinchart пишет:
> > On Thu, Apr 16, 2020 at 08:24:04PM +0300, Dmitry Osipenko wrote:
> >> The OF node should be put before returning error in tegra_output_probe(),
> >> otherwise node's refcount will be leaked.
> >>
> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Hello Laurent,
> 
> That is the fastest kernel review I ever got, thank you :)

I think I'm also guilty for the slowest kernel reviews ever, so maybe in
the end it will even out :-)

-- 
Regards,

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

Re: [PATCH v3 2/2] drm/tegra: output: rgb: Support LVDS encoder bridge

[Dmitry Osipenko]

16.04.2020 20:41, Laurent Pinchart пишет:
...
>> +	if (output->bridge) {
>> +		err = drm_bridge_attach(&output->encoder, output->bridge,
>> +					NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> 
> Using DRM_BRIDGE_ATTACH_NO_CONNECTOR is definitely the way to go, but
> please be aware that it will require creating a connector and an encoder
> manually (which I think this driver already does), and using the bridge
> operations to implement the connector operations. I highly recommend
> using the DRM bridge connector helper for this purpose.

Okay, I missed that there is a DRM bridge connector helper. Thank you
very much for the suggestion, I'll switch to the bridge helper in v4.

>> +		if (err) {
>> +			dev_err(output->dev, "cannot connect bridge: %d\n",
>> +				err);
>> +			return err;
>> +		}
>> +	}
>> +
>>  	if (output->panel) {
> 
> May I also recommend switching to the DRM panel bridge helper ? It will
> simplify the code.

Could you please clarify what is the "DRM panel bridge helper"?

I think we won't need any additional helpers after switching to the
bridge connector helper, no?

...
>> +		np = of_graph_get_remote_node(rgb->output.bridge->of_node,
>> +					      panel_port, -1);
> 
> This shouldn't be needed, the LVDS codec bridge driver should already
> get the panel and handle it internally. You only need to handle panels
> in this driver when they're connected directly to the RGB input.

Indeed, it won't be needed if we will use the bridge connector helper.
Thank you very much for the clarifications!
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 2/2] drm/tegra: output: rgb: Support LVDS encoder bridge

[Dmitry Osipenko]

16.04.2020 21:52, Dmitry Osipenko пишет:
...
>> May I also recommend switching to the DRM panel bridge helper ? It will
>> simplify the code.
> 
> Could you please clarify what is the "DRM panel bridge helper"?
> 
> I think we won't need any additional helpers after switching to the
> bridge connector helper, no?

Actually, I now see that the panel needs to be manually attached to the
connector.

Still it's not apparent to me how to get panel out of the bridge. It
looks like there is no such "panel helper" for the bridge API or I just
can't find it.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 2/2] drm/tegra: output: rgb: Support LVDS encoder bridge

[Sam Ravnborg]

Hi Dimitry.

On Thu, Apr 16, 2020 at 11:21:40PM +0300, Dmitry Osipenko wrote:
> 16.04.2020 21:52, Dmitry Osipenko пишет:
> ...
> >> May I also recommend switching to the DRM panel bridge helper ? It will
> >> simplify the code.
> > 
> > Could you please clarify what is the "DRM panel bridge helper"?
> > 
> > I think we won't need any additional helpers after switching to the
> > bridge connector helper, no?
> 
> Actually, I now see that the panel needs to be manually attached to the
> connector.
> 
> Still it's not apparent to me how to get panel out of the bridge. It
> looks like there is no such "panel helper" for the bridge API or I just
> can't find it.

Take a look in bridge/panel.c
I think devm_drm_panel_bridge_add() is what you are after.

	Sam

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

Re: [PATCH v3 2/2] drm/tegra: output: rgb: Support LVDS encoder bridge

[Laurent Pinchart]

Hi Dmitry,

On Thu, Apr 16, 2020 at 11:21:40PM +0300, Dmitry Osipenko wrote:
> 16.04.2020 21:52, Dmitry Osipenko пишет:
> ...
> >> May I also recommend switching to the DRM panel bridge helper ? It will
> >> simplify the code.
> > 
> > Could you please clarify what is the "DRM panel bridge helper"?
> > 
> > I think we won't need any additional helpers after switching to the
> > bridge connector helper, no?
> 
> Actually, I now see that the panel needs to be manually attached to the
> connector.

The DRM panel bridge helper creates a bridge from a panel (with
devm_drm_panel_bridge_add()). You can then attach that bridge to the
chain, like any other bridge, and the enable/disable operations will be
called automatically without any need to call the panel enable/disable
manually as done currently.

> Still it's not apparent to me how to get panel out of the bridge. It
> looks like there is no such "panel helper" for the bridge API or I just
> can't find it.

You don't need to get a panel out of the bridge. You should get the
bridge as done today, and wrap it in a bridge with
devm_drm_panel_bridge_add().

-- 
Regards,

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

Re: [PATCH v3 2/2] drm/tegra: output: rgb: Support LVDS encoder bridge

[Dmitry Osipenko]

16.04.2020 23:50, Laurent Pinchart пишет:
> Hi Dmitry,
> 
> On Thu, Apr 16, 2020 at 11:21:40PM +0300, Dmitry Osipenko wrote:
>> 16.04.2020 21:52, Dmitry Osipenko пишет:
>> ...
>>>> May I also recommend switching to the DRM panel bridge helper ? It will
>>>> simplify the code.
>>>
>>> Could you please clarify what is the "DRM panel bridge helper"?
>>>
>>> I think we won't need any additional helpers after switching to the
>>> bridge connector helper, no?
>>
>> Actually, I now see that the panel needs to be manually attached to the
>> connector.
> 
> The DRM panel bridge helper creates a bridge from a panel (with
> devm_drm_panel_bridge_add()). You can then attach that bridge to the
> chain, like any other bridge, and the enable/disable operations will be
> called automatically without any need to call the panel enable/disable
> manually as done currently.
> 
>> Still it's not apparent to me how to get panel out of the bridge. It
>> looks like there is no such "panel helper" for the bridge API or I just
>> can't find it.
> 
> You don't need to get a panel out of the bridge. You should get the
> bridge as done today,

You mean "get the panel", correct?

> and wrap it in a bridge with
> devm_drm_panel_bridge_add().
> 

The lvds-codec already wraps panel into the bridge using
devm_drm_panel_bridge_add() and chains it for us, please see
lvds_codec_probe() / lvds_codec_attach().

Does it mean that lvds-codec is not supporting the new model?

Everything works nicely using the old model, where bridge creates
connector and attaches panel to it for us.

[I'm still not sure what is the point to use the new model in a case of
a simple chain of bridges]

Using the new model, the connector isn't created by the bridge, so I
need to duplicate that creation effort in the driver. Once the bridge
connector is manually created, I need to attach panel to this connector,
but panel is reachable only via the remote bridge (which wraps the panel).

driver connector -> LVDS bridge -> panel bridge -> panel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 2/2] drm/tegra: output: rgb: Support LVDS encoder bridge

[Laurent Pinchart]

Hi Dmitry,

On Fri, Apr 17, 2020 at 12:15:33AM +0300, Dmitry Osipenko wrote:
> 16.04.2020 23:50, Laurent Pinchart пишет:
> > On Thu, Apr 16, 2020 at 11:21:40PM +0300, Dmitry Osipenko wrote:
> >> 16.04.2020 21:52, Dmitry Osipenko пишет:
> >> ...
> >>>> May I also recommend switching to the DRM panel bridge helper ? It will
> >>>> simplify the code.
> >>>
> >>> Could you please clarify what is the "DRM panel bridge helper"?
> >>>
> >>> I think we won't need any additional helpers after switching to the
> >>> bridge connector helper, no?
> >>
> >> Actually, I now see that the panel needs to be manually attached to the
> >> connector.
> > 
> > The DRM panel bridge helper creates a bridge from a panel (with
> > devm_drm_panel_bridge_add()). You can then attach that bridge to the
> > chain, like any other bridge, and the enable/disable operations will be
> > called automatically without any need to call the panel enable/disable
> > manually as done currently.
> > 
> >> Still it's not apparent to me how to get panel out of the bridge. It
> >> looks like there is no such "panel helper" for the bridge API or I just
> >> can't find it.
> > 
> > You don't need to get a panel out of the bridge. You should get the
> > bridge as done today,
> 
> You mean "get the panel", correct?

Yes, sorry.

> > and wrap it in a bridge with
> > devm_drm_panel_bridge_add().
> > 
> 
> The lvds-codec already wraps panel into the bridge using
> devm_drm_panel_bridge_add() and chains it for us, please see
> lvds_codec_probe() / lvds_codec_attach().
> 
> Does it mean that lvds-codec is not supporting the new model?

No, that *is* the new model :-) As I think I've commented during review,
when you have an LVDS encoder and a panel, you don't need to handle the
panel at all. When you have an RGB panel connected directly to the RGB
output, you need to wrap it in a bridge, exactly the same way as
lvds-codec does for its panel.

> Everything works nicely using the old model, where bridge creates
> connector and attaches panel to it for us.
> 
> [I'm still not sure what is the point to use the new model in a case of
> a simple chain of bridges]
> 
> Using the new model, the connector isn't created by the bridge, so I
> need to duplicate that creation effort in the driver. Once the bridge
> connector is manually created, I need to attach panel to this connector,
> but panel is reachable only via the remote bridge (which wraps the panel).
> 
> driver connector -> LVDS bridge -> panel bridge -> panel

With the new model,

1. The display driver and the bridge drivers need to get hold of the
   bridge directly connected to their output (for instance with
   of_drm_find_panel()). If the output is connected to a panel, they
   need to wrap that panel in a bridge (with
   devm_drm_panel_bridge_add()). I plan, in the future, to make creation
   of panel bridges automatic, so drivers won't have to care.

2. The display driver needs to create a dummy drm_encoder for each of
   its outputs (for instance with drm_simple_encoder_init()).

3. The display driver needs to create a drm_connector for each of its
   outputs, and implement connector operations by delegating them to the
   bridges in the pipeline. Unless there's a good reason not to do so,
   this should be done with drm_bridge_connector_init().

That's it. Every driver then focusses on its own needs, bridge drivers
handle only the device they're associated with, and the DRM core and DRM
bridge connector helper will handle all the rest.

-- 
Regards,

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

Re: [PATCH v3 2/2] drm/tegra: output: rgb: Support LVDS encoder bridge

[Dmitry Osipenko]

17.04.2020 00:39, Laurent Pinchart пишет:
> Hi Dmitry,
> 
> On Fri, Apr 17, 2020 at 12:15:33AM +0300, Dmitry Osipenko wrote:
>> 16.04.2020 23:50, Laurent Pinchart пишет:
>>> On Thu, Apr 16, 2020 at 11:21:40PM +0300, Dmitry Osipenko wrote:
>>>> 16.04.2020 21:52, Dmitry Osipenko пишет:
>>>> ...
>>>>>> May I also recommend switching to the DRM panel bridge helper ? It will
>>>>>> simplify the code.
>>>>>
>>>>> Could you please clarify what is the "DRM panel bridge helper"?
>>>>>
>>>>> I think we won't need any additional helpers after switching to the
>>>>> bridge connector helper, no?
>>>>
>>>> Actually, I now see that the panel needs to be manually attached to the
>>>> connector.
>>>
>>> The DRM panel bridge helper creates a bridge from a panel (with
>>> devm_drm_panel_bridge_add()). You can then attach that bridge to the
>>> chain, like any other bridge, and the enable/disable operations will be
>>> called automatically without any need to call the panel enable/disable
>>> manually as done currently.
>>>
>>>> Still it's not apparent to me how to get panel out of the bridge. It
>>>> looks like there is no such "panel helper" for the bridge API or I just
>>>> can't find it.
>>>
>>> You don't need to get a panel out of the bridge. You should get the
>>> bridge as done today,
>>
>> You mean "get the panel", correct?
> 
> Yes, sorry.
> 
>>> and wrap it in a bridge with
>>> devm_drm_panel_bridge_add().
>>>
>>
>> The lvds-codec already wraps panel into the bridge using
>> devm_drm_panel_bridge_add() and chains it for us, please see
>> lvds_codec_probe() / lvds_codec_attach().
>>
>> Does it mean that lvds-codec is not supporting the new model?
> 
> No, that *is* the new model :-) As I think I've commented during review,
> when you have an LVDS encoder and a panel, you don't need to handle the
> panel at all. When you have an RGB panel connected directly to the RGB
> output, you need to wrap it in a bridge, exactly the same way as
> lvds-codec does for its panel.
> 
>> Everything works nicely using the old model, where bridge creates
>> connector and attaches panel to it for us.
>>
>> [I'm still not sure what is the point to use the new model in a case of
>> a simple chain of bridges]
>>
>> Using the new model, the connector isn't created by the bridge, so I
>> need to duplicate that creation effort in the driver. Once the bridge
>> connector is manually created, I need to attach panel to this connector,
>> but panel is reachable only via the remote bridge (which wraps the panel).
>>
>> driver connector -> LVDS bridge -> panel bridge -> panel
> 
> With the new model,
> 
> 1. The display driver and the bridge drivers need to get hold of the
>    bridge directly connected to their output (for instance with
>    of_drm_find_panel()). If the output is connected to a panel, they
>    need to wrap that panel in a bridge (with
>    devm_drm_panel_bridge_add()). I plan, in the future, to make creation
>    of panel bridges automatic, so drivers won't have to care.
> 
> 2. The display driver needs to create a dummy drm_encoder for each of
>    its outputs (for instance with drm_simple_encoder_init()).
> 
> 3. The display driver needs to create a drm_connector for each of its
>    outputs, and implement connector operations by delegating them to the
>    bridges in the pipeline. Unless there's a good reason not to do so,
>    this should be done with drm_bridge_connector_init().
> 
> That's it. Every driver then focusses on its own needs, bridge drivers
> handle only the device they're associated with, and the DRM core and DRM
> bridge connector helper will handle all the rest.
> 

Thank you very much for the clarification again! :)

Now I realized what was the missing part.. in my case display panel is
mounted upside-down and display output needs to be rotated by 180°. I
have a local patch (hack) that adds orientation-property support to the
panel-lvds driver and previously the panel's orientation was assigned to
the connector using drm_panel_attach(), but now this attachment doesn't
happen and I found that panel_lvds_get_modes() missed
drm_connector_set_panel_orientation() in my patch. Everything is okay
once the panel_lvds_get_modes() is corrected. I'll prepare the v4.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel