[PATCH] drm: rcar-du: Support panels connected directly to the DPAD outputs

[PATCH] drm: rcar-du: Support panels connected directly to the DPAD outputs

[Laurent Pinchart]

The R-Car DU driver assumes that a bridge is always connected to the DU
output. This is valid for the LVDS and HDMI outputs, but the DPAD
outputs can be connected directly to a panel, in which case no bridge is
available.

To support this use case, detect whether the entities connected to the
DU DPAD outputs are encoders or panels based on the number of ports of
their DT node, and retrieve the corresponding type of DRM objects. For
panels, additionally create panel bridge instances.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 54 ++++++++++++++++++++---
 1 file changed, 48 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
index 8ee4e762f4e5..595ecfa1ff0e 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
@@ -28,13 +28,33 @@ static const struct drm_encoder_funcs encoder_funcs = {
 	.destroy = drm_encoder_cleanup,
 };
 
+static unsigned int rcar_du_encoder_count_ports(struct device_node *node)
+{
+	struct device_node *ports;
+	struct device_node *port;
+	unsigned int num_ports = 0;
+
+	ports = of_get_child_by_name(node, "ports");
+	if (!ports)
+		ports = of_node_get(node);
+
+	for_each_child_of_node(ports, port) {
+		if (of_node_name_eq(port, "port"))
+			num_ports++;
+	}
+
+	of_node_put(node);
+
+	return num_ports;
+}
+
 int rcar_du_encoder_init(struct rcar_du_device *rcdu,
 			 enum rcar_du_output output,
 			 struct device_node *enc_node)
 {
 	struct rcar_du_encoder *renc;
 	struct drm_encoder *encoder;
-	struct drm_bridge *bridge = NULL;
+	struct drm_bridge *bridge;
 	int ret;
 
 	renc = devm_kzalloc(rcdu->dev, sizeof(*renc), GFP_KERNEL);
@@ -48,11 +68,33 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
 	dev_dbg(rcdu->dev, "initializing encoder %pOF for output %u\n",
 		enc_node, output);
 
-	/* Locate the DRM bridge from the encoder DT node. */
-	bridge = of_drm_find_bridge(enc_node);
-	if (!bridge) {
-		ret = -EPROBE_DEFER;
-		goto done;
+	/*
+	 * Locate the DRM bridge from the DT node. For the DPAD outputs, if the
+	 * DT node has a single port, consider it describes a panel and create a
+	 * panel bridge.
+	 */
+	if ((output == RCAR_DU_OUTPUT_DPAD0 ||
+	     output == RCAR_DU_OUTPUT_DPAD1) &&
+	    rcar_du_encoder_count_ports(enc_node) == 1) {
+		struct drm_panel *panel = of_drm_find_panel(enc_node);
+
+		if (IS_ERR(panel)) {
+			ret = PTR_ERR(panel);
+			goto done;
+		}
+
+		bridge = devm_drm_panel_bridge_add(rcdu->dev, panel,
+						   DRM_MODE_CONNECTOR_DPI);
+		if (IS_ERR(bridge)) {
+			ret = PTR_ERR(bridge);
+			goto done;
+		}
+	} else {
+		bridge = of_drm_find_bridge(enc_node);
+		if (!bridge) {
+			ret = -EPROBE_DEFER;
+			goto done;
+		}
 	}
 
 	ret = drm_encoder_init(rcdu->ddev, encoder, &encoder_funcs,
-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm: rcar-du: Support panels connected directly to the DPAD outputs

[Key, Kevin]

Tested and confirmed the correct operation the patched driver on the
Renesas r8a7796-m3ulcb platform.

Tested-by: Kevin Key <kevin.key@gentex.com>

On Sat, 2019-03-02 at 18:17 +0200, Laurent Pinchart wrote:
> The R-Car DU driver assumes that a bridge is always connected to the
> DU
> output. This is valid for the LVDS and HDMI outputs, but the DPAD
> outputs can be connected directly to a panel, in which case no bridge
> is
> available.
>
> To support this use case, detect whether the entities connected to
> the
> DU DPAD outputs are encoders or panels based on the number of ports
> of
> their DT node, and retrieve the corresponding type of DRM objects.
> For
> panels, additionally create panel bridge instances.
>
> Signed-off-by: Laurent Pinchart <
> laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 54 ++++++++++++++++++++-
> --
>  1 file changed, 48 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> index 8ee4e762f4e5..595ecfa1ff0e 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> @@ -28,13 +28,33 @@ static const struct drm_encoder_funcs
> encoder_funcs = {
>         .destroy = drm_encoder_cleanup,
>  };
>
> +static unsigned int rcar_du_encoder_count_ports(struct device_node
> *node)
> +{
> +       struct device_node *ports;
> +       struct device_node *port;
> +       unsigned int num_ports = 0;
> +
> +       ports = of_get_child_by_name(node, "ports");
> +       if (!ports)
> +               ports = of_node_get(node);
> +
> +       for_each_child_of_node(ports, port) {
> +               if (of_node_name_eq(port, "port"))
> +                       num_ports++;
> +       }
> +
> +       of_node_put(node);
> +
> +       return num_ports;
> +}
> +
>  int rcar_du_encoder_init(struct rcar_du_device *rcdu,
>                          enum rcar_du_output output,
>                          struct device_node *enc_node)
>  {
>         struct rcar_du_encoder *renc;
>         struct drm_encoder *encoder;
> -       struct drm_bridge *bridge = NULL;
> +       struct drm_bridge *bridge;
>         int ret;
>
>         renc = devm_kzalloc(rcdu->dev, sizeof(*renc), GFP_KERNEL);
> @@ -48,11 +68,33 @@ int rcar_du_encoder_init(struct rcar_du_device
> *rcdu,
>         dev_dbg(rcdu->dev, "initializing encoder %pOF for output
> %u\n",
>                 enc_node, output);
>
> -       /* Locate the DRM bridge from the encoder DT node. */
> -       bridge = of_drm_find_bridge(enc_node);
> -       if (!bridge) {
> -               ret = -EPROBE_DEFER;
> -               goto done;
> +       /*
> +        * Locate the DRM bridge from the DT node. For the DPAD
> outputs, if the
> +        * DT node has a single port, consider it describes a panel
> and create a
> +        * panel bridge.
> +        */
> +       if ((output == RCAR_DU_OUTPUT_DPAD0 ||
> +            output == RCAR_DU_OUTPUT_DPAD1) &&
> +           rcar_du_encoder_count_ports(enc_node) == 1) {
> +               struct drm_panel *panel =
> of_drm_find_panel(enc_node);
> +
> +               if (IS_ERR(panel)) {
> +                       ret = PTR_ERR(panel);
> +                       goto done;
> +               }
> +
> +               bridge = devm_drm_panel_bridge_add(rcdu->dev, panel,
> +                                                  DRM_MODE_CONNECTOR
> _DPI);
> +               if (IS_ERR(bridge)) {
> +                       ret = PTR_ERR(bridge);
> +                       goto done;
> +               }
> +       } else {
> +               bridge = of_drm_find_bridge(enc_node);
> +               if (!bridge) {
> +                       ret = -EPROBE_DEFER;
> +                       goto done;
> +               }
>         }
>
>         ret = drm_encoder_init(rcdu->ddev, encoder, &encoder_funcs,
> --
> Regards,
>
> Laurent Pinchart
>
>
>
> This e-mail was received from outside of Gentex Corporation. Use
> caution when clicking on links/attachments.
THE INFORMATION CONTAINED IN THIS E-MAIL MESSAGE AND ANY ATTACHMENTS SENT FROM GENTEX CORPORATION IS GENTEX CONFIDENTIAL INFORMATION INTENDED ONLY FOR THE PERSONAL USE OF THE INDIVIDUAL OR ENTITY NAMED ABOVE. If you are not the intended recipient, you are hereby notified that any review, distribution, or copying of this communication is strictly prohibited. If you have received this communication in error, please immediately notify the sender by return e-mail, and delete this e-mail message and any attachments from your computer.<>

Re: [PATCH] drm: rcar-du: Support panels connected directly to the DPAD outputs

[Jacopo Mondi]

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

Hi Laurent,

On Sat, Mar 02, 2019 at 06:17:25PM +0200, Laurent Pinchart wrote:
> The R-Car DU driver assumes that a bridge is always connected to the DU
> output. This is valid for the LVDS and HDMI outputs, but the DPAD
> outputs can be connected directly to a panel, in which case no bridge is
> available.
>
> To support this use case, detect whether the entities connected to the
> DU DPAD outputs are encoders or panels based on the number of ports of
> their DT node, and retrieve the corresponding type of DRM objects. For
> panels, additionally create panel bridge instances.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 54 ++++++++++++++++++++---
>  1 file changed, 48 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> index 8ee4e762f4e5..595ecfa1ff0e 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> @@ -28,13 +28,33 @@ static const struct drm_encoder_funcs encoder_funcs = {
>  	.destroy = drm_encoder_cleanup,
>  };
>
> +static unsigned int rcar_du_encoder_count_ports(struct device_node *node)
> +{
> +	struct device_node *ports;
> +	struct device_node *port;
> +	unsigned int num_ports = 0;
> +
> +	ports = of_get_child_by_name(node, "ports");
> +	if (!ports)
> +		ports = of_node_get(node);
> +
> +	for_each_child_of_node(ports, port) {
> +		if (of_node_name_eq(port, "port"))
> +			num_ports++;
> +	}
> +
> +	of_node_put(node);
> +
> +	return num_ports;
> +}
> +
>  int rcar_du_encoder_init(struct rcar_du_device *rcdu,
>  			 enum rcar_du_output output,
>  			 struct device_node *enc_node)
>  {

You received a Tested-by, so I might surely be mistaken, but the
caller of this function skips all remote nodes with a single port,
doesn't it?
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/rcar-du/rcar_du_kms.c#L308

I don't have any DTS with a panel connected to the DPAD output so, as
a test, I removed all endpoints except endpoint@0 from the 'hdmi0' device
node, and the 'rcar_du_encoder_init()' function never gets called and DU
probing fails with:
rcar-du feb00000.display: no encoder found for endpoint /soc/display@feb00000/ports/port@1/endpoint, skipping

What am I missing?

Thanks
   j


>  	struct rcar_du_encoder *renc;
>  	struct drm_encoder *encoder;
> -	struct drm_bridge *bridge = NULL;
> +	struct drm_bridge *bridge;
>  	int ret;
>
>  	renc = devm_kzalloc(rcdu->dev, sizeof(*renc), GFP_KERNEL);
> @@ -48,11 +68,33 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
>  	dev_dbg(rcdu->dev, "initializing encoder %pOF for output %u\n",
>  		enc_node, output);
>
> -	/* Locate the DRM bridge from the encoder DT node. */
> -	bridge = of_drm_find_bridge(enc_node);
> -	if (!bridge) {
> -		ret = -EPROBE_DEFER;
> -		goto done;
> +	/*
> +	 * Locate the DRM bridge from the DT node. For the DPAD outputs, if the
> +	 * DT node has a single port, consider it describes a panel and create a
> +	 * panel bridge.
> +	 */
> +	if ((output == RCAR_DU_OUTPUT_DPAD0 ||
> +	     output == RCAR_DU_OUTPUT_DPAD1) &&
> +	    rcar_du_encoder_count_ports(enc_node) == 1) {

Could this be cached?

> +		struct drm_panel *panel = of_drm_find_panel(enc_node);
> +
> +		if (IS_ERR(panel)) {
> +			ret = PTR_ERR(panel);
> +			goto done;
> +		}
> +
> +		bridge = devm_drm_panel_bridge_add(rcdu->dev, panel,
> +						   DRM_MODE_CONNECTOR_DPI);
> +		if (IS_ERR(bridge)) {
> +			ret = PTR_ERR(bridge);
> +			goto done;
> +		}
> +	} else {
> +		bridge = of_drm_find_bridge(enc_node);
> +		if (!bridge) {
> +			ret = -EPROBE_DEFER;
> +			goto done;
> +		}
>  	}
>
>  	ret = drm_encoder_init(rcdu->ddev, encoder, &encoder_funcs,
> --
> Regards,
>
> Laurent Pinchart
>

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

Re: [PATCH] drm: rcar-du: Support panels connected directly to the DPAD outputs

[Kieran Bingham]

Hi Laurent,

Thank you for the patch,

On 02/03/2019 16:17, Laurent Pinchart wrote:
> The R-Car DU driver assumes that a bridge is always connected to the DU
> output. This is valid for the LVDS and HDMI outputs, but the DPAD
> outputs can be connected directly to a panel, in which case no bridge is
> available.
> 
> To support this use case, detect whether the entities connected to the
> DU DPAD outputs are encoders or panels based on the number of ports of
> their DT node, and retrieve the corresponding type of DRM objects. For
> panels, additionally create panel bridge instances.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

Except for some minor wording on the comment, which I'll let you handle,

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>


> ---
>  drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 54 ++++++++++++++++++++---
>  1 file changed, 48 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> index 8ee4e762f4e5..595ecfa1ff0e 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> @@ -28,13 +28,33 @@ static const struct drm_encoder_funcs encoder_funcs = {
>  	.destroy = drm_encoder_cleanup,
>  };
>  
> +static unsigned int rcar_du_encoder_count_ports(struct device_node *node)
> +{
> +	struct device_node *ports;
> +	struct device_node *port;
> +	unsigned int num_ports = 0;
> +
> +	ports = of_get_child_by_name(node, "ports");
> +	if (!ports)
> +		ports = of_node_get(node);
> +
> +	for_each_child_of_node(ports, port) {
> +		if (of_node_name_eq(port, "port"))
> +			num_ports++;
> +	}
> +
> +	of_node_put(node);
> +
> +	return num_ports;
> +}
> +
>  int rcar_du_encoder_init(struct rcar_du_device *rcdu,
>  			 enum rcar_du_output output,
>  			 struct device_node *enc_node)
>  {
>  	struct rcar_du_encoder *renc;
>  	struct drm_encoder *encoder;
> -	struct drm_bridge *bridge = NULL;
> +	struct drm_bridge *bridge;
>  	int ret;
>  
>  	renc = devm_kzalloc(rcdu->dev, sizeof(*renc), GFP_KERNEL);
> @@ -48,11 +68,33 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
>  	dev_dbg(rcdu->dev, "initializing encoder %pOF for output %u\n",
>  		enc_node, output);
>  
> -	/* Locate the DRM bridge from the encoder DT node. */
> -	bridge = of_drm_find_bridge(enc_node);
> -	if (!bridge) {
> -		ret = -EPROBE_DEFER;
> -		goto done;
> +	/*
> +	 * Locate the DRM bridge from the DT node. For the DPAD outputs, if the
> +	 * DT node has a single port, consider it describes a panel and create a


consider it as describing? (if it's an assumption that has been made)

consider if it describes? (if it's conditional)

I'm not sure I fully understand the intent of the sentence to correct it
- but it needs a little tweak. The use of the word 'consider' makes me
assume that it might not need a panel bridge, and some further decision
needs to be made, but the code looks like it assumes one port means
there should be a panel - and a bridge will be added.



> +	 * panel bridge.
> +	 */
> +	if ((output == RCAR_DU_OUTPUT_DPAD0 ||
> +	     output == RCAR_DU_OUTPUT_DPAD1) &&
> +	    rcar_du_encoder_count_ports(enc_node) == 1) {
> +		struct drm_panel *panel = of_drm_find_panel(enc_node);
> +
> +		if (IS_ERR(panel)) {
> +			ret = PTR_ERR(panel);
> +			goto done;
> +		}
> +
> +		bridge = devm_drm_panel_bridge_add(rcdu->dev, panel,
> +						   DRM_MODE_CONNECTOR_DPI);
> +		if (IS_ERR(bridge)) {
> +			ret = PTR_ERR(bridge);
> +			goto done;
> +		}
> +	} else {
> +		bridge = of_drm_find_bridge(enc_node);
> +		if (!bridge) {
> +			ret = -EPROBE_DEFER;
> +			goto done;
> +		}
>  	}
>  
>  	ret = drm_encoder_init(rcdu->ddev, encoder, &encoder_funcs,
> 

-- 
Regards
--
Kieran

Re: [PATCH] drm: rcar-du: Support panels connected directly to the DPAD outputs

[Laurent Pinchart]

Hi Kieran,

On Wed, Mar 06, 2019 at 10:07:12AM +0000, Kieran Bingham wrote:
> On 02/03/2019 16:17, Laurent Pinchart wrote:
> > The R-Car DU driver assumes that a bridge is always connected to the DU
> > output. This is valid for the LVDS and HDMI outputs, but the DPAD
> > outputs can be connected directly to a panel, in which case no bridge is
> > available.
> > 
> > To support this use case, detect whether the entities connected to the
> > DU DPAD outputs are encoders or panels based on the number of ports of
> > their DT node, and retrieve the corresponding type of DRM objects. For
> > panels, additionally create panel bridge instances.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> Except for some minor wording on the comment, which I'll let you handle,
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 54 ++++++++++++++++++++---
> >  1 file changed, 48 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> > index 8ee4e762f4e5..595ecfa1ff0e 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> > @@ -28,13 +28,33 @@ static const struct drm_encoder_funcs encoder_funcs = {
> >  	.destroy = drm_encoder_cleanup,
> >  };
> >  
> > +static unsigned int rcar_du_encoder_count_ports(struct device_node *node)
> > +{
> > +	struct device_node *ports;
> > +	struct device_node *port;
> > +	unsigned int num_ports = 0;
> > +
> > +	ports = of_get_child_by_name(node, "ports");
> > +	if (!ports)
> > +		ports = of_node_get(node);
> > +
> > +	for_each_child_of_node(ports, port) {
> > +		if (of_node_name_eq(port, "port"))
> > +			num_ports++;
> > +	}
> > +
> > +	of_node_put(node);
> > +
> > +	return num_ports;
> > +}
> > +
> >  int rcar_du_encoder_init(struct rcar_du_device *rcdu,
> >  			 enum rcar_du_output output,
> >  			 struct device_node *enc_node)
> >  {
> >  	struct rcar_du_encoder *renc;
> >  	struct drm_encoder *encoder;
> > -	struct drm_bridge *bridge = NULL;
> > +	struct drm_bridge *bridge;
> >  	int ret;
> >  
> >  	renc = devm_kzalloc(rcdu->dev, sizeof(*renc), GFP_KERNEL);
> > @@ -48,11 +68,33 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
> >  	dev_dbg(rcdu->dev, "initializing encoder %pOF for output %u\n",
> >  		enc_node, output);
> >  
> > -	/* Locate the DRM bridge from the encoder DT node. */
> > -	bridge = of_drm_find_bridge(enc_node);
> > -	if (!bridge) {
> > -		ret = -EPROBE_DEFER;
> > -		goto done;
> > +	/*
> > +	 * Locate the DRM bridge from the DT node. For the DPAD outputs, if the
> > +	 * DT node has a single port, consider it describes a panel and create a
> 
> 
> consider it as describing? (if it's an assumption that has been made)
> 
> consider if it describes? (if it's conditional)
> 
> I'm not sure I fully understand the intent of the sentence to correct it
> - but it needs a little tweak. The use of the word 'consider' makes me
> assume that it might not need a panel bridge, and some further decision
> needs to be made, but the code looks like it assumes one port means
> there should be a panel - and a bridge will be added.

Should I write it as "assume that it ..." ? I considered "consider" as
being less conditional than "assume", but I'll trust your judgement on
that.

> > +	 * panel bridge.
> > +	 */
> > +	if ((output == RCAR_DU_OUTPUT_DPAD0 ||
> > +	     output == RCAR_DU_OUTPUT_DPAD1) &&
> > +	    rcar_du_encoder_count_ports(enc_node) == 1) {
> > +		struct drm_panel *panel = of_drm_find_panel(enc_node);
> > +
> > +		if (IS_ERR(panel)) {
> > +			ret = PTR_ERR(panel);
> > +			goto done;
> > +		}
> > +
> > +		bridge = devm_drm_panel_bridge_add(rcdu->dev, panel,
> > +						   DRM_MODE_CONNECTOR_DPI);
> > +		if (IS_ERR(bridge)) {
> > +			ret = PTR_ERR(bridge);
> > +			goto done;
> > +		}
> > +	} else {
> > +		bridge = of_drm_find_bridge(enc_node);
> > +		if (!bridge) {
> > +			ret = -EPROBE_DEFER;
> > +			goto done;
> > +		}
> >  	}
> >  
> >  	ret = drm_encoder_init(rcdu->ddev, encoder, &encoder_funcs,

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm: rcar-du: Support panels connected directly to the DPAD outputs

[Laurent Pinchart]

Hi Jacopo,

On Wed, Mar 06, 2019 at 11:03:32AM +0100, Jacopo Mondi wrote:
> On Sat, Mar 02, 2019 at 06:17:25PM +0200, Laurent Pinchart wrote:
> > The R-Car DU driver assumes that a bridge is always connected to the DU
> > output. This is valid for the LVDS and HDMI outputs, but the DPAD
> > outputs can be connected directly to a panel, in which case no bridge is
> > available.
> >
> > To support this use case, detect whether the entities connected to the
> > DU DPAD outputs are encoders or panels based on the number of ports of
> > their DT node, and retrieve the corresponding type of DRM objects. For
> > panels, additionally create panel bridge instances.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 54 ++++++++++++++++++++---
> >  1 file changed, 48 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> > index 8ee4e762f4e5..595ecfa1ff0e 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> > @@ -28,13 +28,33 @@ static const struct drm_encoder_funcs encoder_funcs = {
> >  	.destroy = drm_encoder_cleanup,
> >  };
> >
> > +static unsigned int rcar_du_encoder_count_ports(struct device_node *node)
> > +{
> > +	struct device_node *ports;
> > +	struct device_node *port;
> > +	unsigned int num_ports = 0;
> > +
> > +	ports = of_get_child_by_name(node, "ports");
> > +	if (!ports)
> > +		ports = of_node_get(node);
> > +
> > +	for_each_child_of_node(ports, port) {
> > +		if (of_node_name_eq(port, "port"))
> > +			num_ports++;
> > +	}
> > +
> > +	of_node_put(node);
> > +
> > +	return num_ports;
> > +}
> > +
> >  int rcar_du_encoder_init(struct rcar_du_device *rcdu,
> >  			 enum rcar_du_output output,
> >  			 struct device_node *enc_node)
> >  {
> 
> You received a Tested-by, so I might surely be mistaken, but the
> caller of this function skips all remote nodes with a single port,
> doesn't it?
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/rcar-du/rcar_du_kms.c#L308
> 
> I don't have any DTS with a panel connected to the DPAD output so, as
> a test, I removed all endpoints except endpoint@0 from the 'hdmi0' device
> node, and the 'rcar_du_encoder_init()' function never gets called and DU
> probing fails with:
> rcar-du feb00000.display: no encoder found for endpoint /soc/display@feb00000/ports/port@1/endpoint, skipping
> 
> What am I missing?

You're missing drm-next :-)

https://cgit.freedesktop.org/drm/drm/tree/drivers/gpu/drm/rcar-du/rcar_du_kms.c#n331

> >  	struct rcar_du_encoder *renc;
> >  	struct drm_encoder *encoder;
> > -	struct drm_bridge *bridge = NULL;
> > +	struct drm_bridge *bridge;
> >  	int ret;
> >
> >  	renc = devm_kzalloc(rcdu->dev, sizeof(*renc), GFP_KERNEL);
> > @@ -48,11 +68,33 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
> >  	dev_dbg(rcdu->dev, "initializing encoder %pOF for output %u\n",
> >  		enc_node, output);
> >
> > -	/* Locate the DRM bridge from the encoder DT node. */
> > -	bridge = of_drm_find_bridge(enc_node);
> > -	if (!bridge) {
> > -		ret = -EPROBE_DEFER;
> > -		goto done;
> > +	/*
> > +	 * Locate the DRM bridge from the DT node. For the DPAD outputs, if the
> > +	 * DT node has a single port, consider it describes a panel and create a
> > +	 * panel bridge.
> > +	 */
> > +	if ((output == RCAR_DU_OUTPUT_DPAD0 ||
> > +	     output == RCAR_DU_OUTPUT_DPAD1) &&
> > +	    rcar_du_encoder_count_ports(enc_node) == 1) {
> 
> Could this be cached?

How so ? This function is run once per enc_node, so the cache wouldn't
be used again, would it ?

> > +		struct drm_panel *panel = of_drm_find_panel(enc_node);
> > +
> > +		if (IS_ERR(panel)) {
> > +			ret = PTR_ERR(panel);
> > +			goto done;
> > +		}
> > +
> > +		bridge = devm_drm_panel_bridge_add(rcdu->dev, panel,
> > +						   DRM_MODE_CONNECTOR_DPI);
> > +		if (IS_ERR(bridge)) {
> > +			ret = PTR_ERR(bridge);
> > +			goto done;
> > +		}
> > +	} else {
> > +		bridge = of_drm_find_bridge(enc_node);
> > +		if (!bridge) {
> > +			ret = -EPROBE_DEFER;
> > +			goto done;
> > +		}
> >  	}
> >
> >  	ret = drm_encoder_init(rcdu->ddev, encoder, &encoder_funcs,

-- 
Regards,

Laurent Pinchart

Re: [PATCH] drm: rcar-du: Support panels connected directly to the DPAD outputs

[Jacopo Mondi]

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

Hi Laurent!

On Wed, Mar 06, 2019 at 03:13:55PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Wed, Mar 06, 2019 at 11:03:32AM +0100, Jacopo Mondi wrote:
> > On Sat, Mar 02, 2019 at 06:17:25PM +0200, Laurent Pinchart wrote:
> > > The R-Car DU driver assumes that a bridge is always connected to the DU
> > > output. This is valid for the LVDS and HDMI outputs, but the DPAD
> > > outputs can be connected directly to a panel, in which case no bridge is
> > > available.
> > >
> > > To support this use case, detect whether the entities connected to the
> > > DU DPAD outputs are encoders or panels based on the number of ports of
> > > their DT node, and retrieve the corresponding type of DRM objects. For
> > > panels, additionally create panel bridge instances.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > ---
> > >  drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 54 ++++++++++++++++++++---
> > >  1 file changed, 48 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> > > index 8ee4e762f4e5..595ecfa1ff0e 100644
> > > --- a/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> > > +++ b/drivers/gpu/drm/rcar-du/rcar_du_encoder.c
> > > @@ -28,13 +28,33 @@ static const struct drm_encoder_funcs encoder_funcs = {
> > >  	.destroy = drm_encoder_cleanup,
> > >  };
> > >
> > > +static unsigned int rcar_du_encoder_count_ports(struct device_node *node)
> > > +{
> > > +	struct device_node *ports;
> > > +	struct device_node *port;
> > > +	unsigned int num_ports = 0;
> > > +
> > > +	ports = of_get_child_by_name(node, "ports");
> > > +	if (!ports)
> > > +		ports = of_node_get(node);
> > > +
> > > +	for_each_child_of_node(ports, port) {
> > > +		if (of_node_name_eq(port, "port"))
> > > +			num_ports++;
> > > +	}
> > > +
> > > +	of_node_put(node);
> > > +
> > > +	return num_ports;
> > > +}
> > > +
> > >  int rcar_du_encoder_init(struct rcar_du_device *rcdu,
> > >  			 enum rcar_du_output output,
> > >  			 struct device_node *enc_node)
> > >  {
> >
> > You received a Tested-by, so I might surely be mistaken, but the
> > caller of this function skips all remote nodes with a single port,
> > doesn't it?
> > https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/rcar-du/rcar_du_kms.c#L308
> >
> > I don't have any DTS with a panel connected to the DPAD output so, as
> > a test, I removed all endpoints except endpoint@0 from the 'hdmi0' device
> > node, and the 'rcar_du_encoder_init()' function never gets called and DU
> > probing fails with:
> > rcar-du feb00000.display: no encoder found for endpoint /soc/display@feb00000/ports/port@1/endpoint, skipping
> >
> > What am I missing?
>
> You're missing drm-next :-)
>
> https://cgit.freedesktop.org/drm/drm/tree/drivers/gpu/drm/rcar-du/rcar_du_kms.c#n331

Oh, I see!

Thanks, with this clarified, please add
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
  j
>
> > >  	struct rcar_du_encoder *renc;
> > >  	struct drm_encoder *encoder;
> > > -	struct drm_bridge *bridge = NULL;
> > > +	struct drm_bridge *bridge;
> > >  	int ret;
> > >
> > >  	renc = devm_kzalloc(rcdu->dev, sizeof(*renc), GFP_KERNEL);
> > > @@ -48,11 +68,33 @@ int rcar_du_encoder_init(struct rcar_du_device *rcdu,
> > >  	dev_dbg(rcdu->dev, "initializing encoder %pOF for output %u\n",
> > >  		enc_node, output);
> > >
> > > -	/* Locate the DRM bridge from the encoder DT node. */
> > > -	bridge = of_drm_find_bridge(enc_node);
> > > -	if (!bridge) {
> > > -		ret = -EPROBE_DEFER;
> > > -		goto done;
> > > +	/*
> > > +	 * Locate the DRM bridge from the DT node. For the DPAD outputs, if the
> > > +	 * DT node has a single port, consider it describes a panel and create a
> > > +	 * panel bridge.
> > > +	 */
> > > +	if ((output == RCAR_DU_OUTPUT_DPAD0 ||
> > > +	     output == RCAR_DU_OUTPUT_DPAD1) &&
> > > +	    rcar_du_encoder_count_ports(enc_node) == 1) {
> >
> > Could this be cached?
>
> How so ? This function is run once per enc_node, so the cache wouldn't
> be used again, would it ?
>
> > > +		struct drm_panel *panel = of_drm_find_panel(enc_node);
> > > +
> > > +		if (IS_ERR(panel)) {
> > > +			ret = PTR_ERR(panel);
> > > +			goto done;
> > > +		}
> > > +
> > > +		bridge = devm_drm_panel_bridge_add(rcdu->dev, panel,
> > > +						   DRM_MODE_CONNECTOR_DPI);
> > > +		if (IS_ERR(bridge)) {
> > > +			ret = PTR_ERR(bridge);
> > > +			goto done;
> > > +		}
> > > +	} else {
> > > +		bridge = of_drm_find_bridge(enc_node);
> > > +		if (!bridge) {
> > > +			ret = -EPROBE_DEFER;
> > > +			goto done;
> > > +		}
> > >  	}
> > >
> > >  	ret = drm_encoder_init(rcdu->ddev, encoder, &encoder_funcs,
>
> --
> Regards,
>
> Laurent Pinchart

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