[v3,4/8] drm: rcar-du: lvds: Add dual-LVDS panels support
diff mbox series

Message ID 1567017402-5895-5-git-send-email-fabrizio.castro@bp.renesas.com
State New
Delegated to: Kieran Bingham
Headers show
Series
  • Add dual-LVDS panel support to EK874
Related show

Commit Message

Fabrizio Castro Aug. 28, 2019, 6:36 p.m. UTC
The driver doesn't support dual-link LVDS displays, and the way
it identifies bridges won't allow for dual-LVDS displays to be
connected. Also, it's not possible to swap even and odd pixels
around in case the wiring isn't taking advantage of the default
hardware configuration. Further more, the "mode" of the companion
encoder should be same as the mode of the primary encoder.

Rework the driver to improve all of the above, so that it can
support dual-LVDS displays.

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>

---
v2->v3:
* reworked to take advantange of the new dt-bindings
* squashed in the patche for fixing the companion's mode

Laurent,

unfortunately the best way to get the companion encoder to use
the same mode as the primary encoder is setting the mode directly
without calling into rcar_lvds_mode_set for the companion encoder,
as the below test fails for the companion encoder in
rcar_lvds_get_lvds_mode:
if (!info->num_bus_formats || !info->bus_formats)

Anyhow, setting the mode for the companion encoder doesn't seem
to be mandary according to the experiments I have been running,
but the HW User's Manual doesn't really say much about this,
therefore I think the safest option is still to set the mode for
the companion encoder.

Thanks,
Fab
---
 drivers/gpu/drm/rcar-du/rcar_lvds.c | 110 +++++++++++++++++++++---------------
 1 file changed, 65 insertions(+), 45 deletions(-)

Comments

Laurent Pinchart Nov. 7, 2019, 7:50 p.m. UTC | #1
Hi Fabrizio,

Thank you for the patch.

On Wed, Aug 28, 2019 at 07:36:38PM +0100, Fabrizio Castro wrote:
> The driver doesn't support dual-link LVDS displays, and the way
> it identifies bridges won't allow for dual-LVDS displays to be
> connected. Also, it's not possible to swap even and odd pixels
> around in case the wiring isn't taking advantage of the default
> hardware configuration. Further more, the "mode" of the companion
> encoder should be same as the mode of the primary encoder.
> 
> Rework the driver to improve all of the above, so that it can
> support dual-LVDS displays.

That's lots of changes in one patch, could it be split to ease review ?
Also, should the commit message be reworded to explain what the patch
does, instead of enumerating issues ? When there's a single issue being
addressed in a patch it's usually fine, but there the change is larger,
without an explanation of how you intend to fix the issues I can't tell
if the code really matches your intent.

> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> 
> ---
> v2->v3:
> * reworked to take advantange of the new dt-bindings
> * squashed in the patche for fixing the companion's mode
> 
> Laurent,
> 
> unfortunately the best way to get the companion encoder to use
> the same mode as the primary encoder is setting the mode directly
> without calling into rcar_lvds_mode_set for the companion encoder,
> as the below test fails for the companion encoder in
> rcar_lvds_get_lvds_mode:
> if (!info->num_bus_formats || !info->bus_formats)

Would "[PATCH] drm: rcar-du: lvds: Get mode from state" help here ?
Maybe you could review that patch, I could then include it in my -next
branch, your work would be simplified, and everybody would be happy ?
:-)

> Anyhow, setting the mode for the companion encoder doesn't seem
> to be mandary according to the experiments I have been running,
> but the HW User's Manual doesn't really say much about this,
> therefore I think the safest option is still to set the mode for
> the companion encoder.

I agree it should be done.

> ---
>  drivers/gpu/drm/rcar-du/rcar_lvds.c | 110 +++++++++++++++++++++---------------
>  1 file changed, 65 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index 3fe0b86..dfec5e7 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -20,6 +20,8 @@
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_bridge.h>
> +#include <drm/drm_bus_timings.h>
> +#include <drm/drm_of.h>
>  #include <drm/drm_panel.h>
>  #include <drm/drm_probe_helper.h>
>  
> @@ -69,6 +71,7 @@ struct rcar_lvds {
>  
>  	struct drm_bridge *companion;
>  	bool dual_link;
> +	bool stripe_swap_data;
>  };
>  
>  #define bridge_to_rcar_lvds(b) \
> @@ -439,12 +442,20 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
>  	rcar_lvds_write(lvds, LVDCHCR, lvdhcr);
>  
>  	if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) {
> -		/*
> -		 * Configure vertical stripe based on the mode of operation of
> -		 * the connected device.
> -		 */
> -		rcar_lvds_write(lvds, LVDSTRIPE,
> -				lvds->dual_link ? LVDSTRIPE_ST_ON : 0);
> +		u32 lvdstripe = 0;
> +
> +		if (lvds->dual_link)
> +			/*
> +			 * Configure vertical stripe based on the mode of
> +			 * operation of the connected device.
> +			 *
> +			 * ST_SWAP from LVD1STRIPE is reserved, do not set
> +			 * in the companion LVDS
> +			 */
> +			lvdstripe = LVDSTRIPE_ST_ON |
> +				(lvds->companion && lvds->stripe_swap_data ?
> +				 LVDSTRIPE_ST_SWAP : 0);
> +		rcar_lvds_write(lvds, LVDSTRIPE, lvdstripe);
>  	}
>  
>  	/*
> @@ -603,6 +614,11 @@ static void rcar_lvds_mode_set(struct drm_bridge *bridge,
>  	lvds->display_mode = *adjusted_mode;
>  
>  	rcar_lvds_get_lvds_mode(lvds);
> +	if (lvds->companion) {
> +		struct rcar_lvds *companion_lvds = bridge_to_rcar_lvds(
> +							lvds->companion);
> +		companion_lvds->mode = lvds->mode;
> +	}
>  }
>  
>  static int rcar_lvds_attach(struct drm_bridge *bridge)
> @@ -667,9 +683,10 @@ EXPORT_SYMBOL_GPL(rcar_lvds_dual_link);
>  static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
>  {
>  	const struct of_device_id *match;
> -	struct device_node *companion;
> +	struct device_node *companion, *p0 = NULL, *p1 = NULL;
>  	struct device *dev = lvds->dev;
> -	int ret = 0;
> +	struct rcar_lvds *companion_lvds;
> +	int ret = 0, dual_link;
>  
>  	/* Locate the companion LVDS encoder for dual-link operation, if any. */
>  	companion = of_parse_phandle(dev->of_node, "renesas,companion", 0);
> @@ -687,16 +704,50 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
>  		goto done;
>  	}
>  
> +	/*
> +	 * We need to work out if the sink is expecting us to function in
> +	 * dual-link mode. We do this by looking at the DT port nodes we are
> +	 * connected to, if they are marked as expecting even pixels and
> +	 * odd pixels than we need to enable vertical stripe output
> +	 */
> +	p0 = of_graph_get_port_by_id(dev->of_node, 1);
> +	p1 = of_graph_get_port_by_id(companion, 1);
> +	dual_link = drm_of_lvds_get_dual_link_configuration(p0, p1);

You can call of_node_put(p0) and of_node_put(p1) here instead of adding
them at the end of the function.

> +	if (dual_link >= DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) {
> +		dev_dbg(dev, "Dual-link configuration detected\n");
> +		lvds->dual_link = true;
> +	} else {
> +		/* dual-link mode is not required */
> +		dev_dbg(dev, "Single-link configuration detected\n");
> +		goto done;
> +	}

Missing blank line here.

> +	/*
> +	 * We may need to swap even and odd pixels around in case the wiring
> +	 * doesn't match the default configuration.
> +	 * By default we generate even pixels from this encoder and odd pixels
> +	 * from the companion encoder, but if p0 is connected to the port
> +	 * expecting ood pixels, and p1 is connected to the port expecting even
> +	 * pixels, then we need to swap even and odd pixels around
> +	 */
> +	if (dual_link == DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS) {
> +		dev_dbg(dev, "Data swapping required\n");
> +		lvds->stripe_swap_data = true;
> +	}
> +
>  	lvds->companion = of_drm_find_bridge(companion);
>  	if (!lvds->companion) {
>  		ret = -EPROBE_DEFER;
>  		goto done;
>  	}
> +	companion_lvds = bridge_to_rcar_lvds(lvds->companion);
> +	companion_lvds->dual_link = lvds->dual_link;

I don't like poking directly in the companion like this :-( Can't we let
the companion detect dual link mode itself ?

>  
>  	dev_dbg(dev, "Found companion encoder %pOF\n", companion);
>  
>  done:
>  	of_node_put(companion);
> +	of_node_put(p0);
> +	of_node_put(p1);
>  
>  	return ret;
>  }
> @@ -704,10 +755,7 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
>  static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
>  {
>  	struct device_node *local_output = NULL;
> -	struct device_node *remote_input = NULL;
>  	struct device_node *remote = NULL;
> -	struct device_node *node;
> -	bool is_bridge = false;
>  	int ret = 0;
>  
>  	local_output = of_graph_get_endpoint_by_regs(lvds->dev->of_node, 1, 0);
> @@ -735,45 +783,17 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
>  		goto done;
>  	}
>  

I think you can also drop all the code above.

> -	remote_input = of_graph_get_remote_endpoint(local_output);
> -
> -	for_each_endpoint_of_node(remote, node) {
> -		if (node != remote_input) {
> -			/*
> -			 * We've found one endpoint other than the input, this
> -			 * must be a bridge.
> -			 */
> -			is_bridge = true;
> -			of_node_put(node);
> -			break;
> -		}
> -	}
> -
> -	if (is_bridge) {
> -		lvds->next_bridge = of_drm_find_bridge(remote);
> -		if (!lvds->next_bridge) {
> -			ret = -EPROBE_DEFER;
> -			goto done;
> -		}
> -
> -		if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
> -			lvds->dual_link = lvds->next_bridge->timings
> -					? lvds->next_bridge->timings->dual_link
> -					: false;

Aren't you breaking backward compatibility with this change ? Unless I'm
mistaken you're now requiring the new DT properties, and the existing DT
that include a thc63lvd1024 won't have them.

> -	} else {
> -		lvds->panel = of_drm_find_panel(remote);
> -		if (IS_ERR(lvds->panel)) {
> -			ret = PTR_ERR(lvds->panel);
> -			goto done;
> -		}
> +	ret = drm_of_find_panel_or_bridge(lvds->dev->of_node, 1, 0,
> +					  &lvds->panel, &lvds->next_bridge);
> +	if (ret) {
> +		ret = -EPROBE_DEFER;

Shouldn't you return ret instead of overriding it ?

> +		goto done;
>  	}
> -
> -	if (lvds->dual_link)
> +	if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
>  		ret = rcar_lvds_parse_dt_companion(lvds);
>  
>  done:
>  	of_node_put(local_output);
> -	of_node_put(remote_input);
>  	of_node_put(remote);
>  
>  	/*

Patch
diff mbox series

diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 3fe0b86..dfec5e7 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -20,6 +20,8 @@ 
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
+#include <drm/drm_bus_timings.h>
+#include <drm/drm_of.h>
 #include <drm/drm_panel.h>
 #include <drm/drm_probe_helper.h>
 
@@ -69,6 +71,7 @@  struct rcar_lvds {
 
 	struct drm_bridge *companion;
 	bool dual_link;
+	bool stripe_swap_data;
 };
 
 #define bridge_to_rcar_lvds(b) \
@@ -439,12 +442,20 @@  static void rcar_lvds_enable(struct drm_bridge *bridge)
 	rcar_lvds_write(lvds, LVDCHCR, lvdhcr);
 
 	if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) {
-		/*
-		 * Configure vertical stripe based on the mode of operation of
-		 * the connected device.
-		 */
-		rcar_lvds_write(lvds, LVDSTRIPE,
-				lvds->dual_link ? LVDSTRIPE_ST_ON : 0);
+		u32 lvdstripe = 0;
+
+		if (lvds->dual_link)
+			/*
+			 * Configure vertical stripe based on the mode of
+			 * operation of the connected device.
+			 *
+			 * ST_SWAP from LVD1STRIPE is reserved, do not set
+			 * in the companion LVDS
+			 */
+			lvdstripe = LVDSTRIPE_ST_ON |
+				(lvds->companion && lvds->stripe_swap_data ?
+				 LVDSTRIPE_ST_SWAP : 0);
+		rcar_lvds_write(lvds, LVDSTRIPE, lvdstripe);
 	}
 
 	/*
@@ -603,6 +614,11 @@  static void rcar_lvds_mode_set(struct drm_bridge *bridge,
 	lvds->display_mode = *adjusted_mode;
 
 	rcar_lvds_get_lvds_mode(lvds);
+	if (lvds->companion) {
+		struct rcar_lvds *companion_lvds = bridge_to_rcar_lvds(
+							lvds->companion);
+		companion_lvds->mode = lvds->mode;
+	}
 }
 
 static int rcar_lvds_attach(struct drm_bridge *bridge)
@@ -667,9 +683,10 @@  EXPORT_SYMBOL_GPL(rcar_lvds_dual_link);
 static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
 {
 	const struct of_device_id *match;
-	struct device_node *companion;
+	struct device_node *companion, *p0 = NULL, *p1 = NULL;
 	struct device *dev = lvds->dev;
-	int ret = 0;
+	struct rcar_lvds *companion_lvds;
+	int ret = 0, dual_link;
 
 	/* Locate the companion LVDS encoder for dual-link operation, if any. */
 	companion = of_parse_phandle(dev->of_node, "renesas,companion", 0);
@@ -687,16 +704,50 @@  static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
 		goto done;
 	}
 
+	/*
+	 * We need to work out if the sink is expecting us to function in
+	 * dual-link mode. We do this by looking at the DT port nodes we are
+	 * connected to, if they are marked as expecting even pixels and
+	 * odd pixels than we need to enable vertical stripe output
+	 */
+	p0 = of_graph_get_port_by_id(dev->of_node, 1);
+	p1 = of_graph_get_port_by_id(companion, 1);
+	dual_link = drm_of_lvds_get_dual_link_configuration(p0, p1);
+	if (dual_link >= DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) {
+		dev_dbg(dev, "Dual-link configuration detected\n");
+		lvds->dual_link = true;
+	} else {
+		/* dual-link mode is not required */
+		dev_dbg(dev, "Single-link configuration detected\n");
+		goto done;
+	}
+	/*
+	 * We may need to swap even and odd pixels around in case the wiring
+	 * doesn't match the default configuration.
+	 * By default we generate even pixels from this encoder and odd pixels
+	 * from the companion encoder, but if p0 is connected to the port
+	 * expecting ood pixels, and p1 is connected to the port expecting even
+	 * pixels, then we need to swap even and odd pixels around
+	 */
+	if (dual_link == DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS) {
+		dev_dbg(dev, "Data swapping required\n");
+		lvds->stripe_swap_data = true;
+	}
+
 	lvds->companion = of_drm_find_bridge(companion);
 	if (!lvds->companion) {
 		ret = -EPROBE_DEFER;
 		goto done;
 	}
+	companion_lvds = bridge_to_rcar_lvds(lvds->companion);
+	companion_lvds->dual_link = lvds->dual_link;
 
 	dev_dbg(dev, "Found companion encoder %pOF\n", companion);
 
 done:
 	of_node_put(companion);
+	of_node_put(p0);
+	of_node_put(p1);
 
 	return ret;
 }
@@ -704,10 +755,7 @@  static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
 static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
 {
 	struct device_node *local_output = NULL;
-	struct device_node *remote_input = NULL;
 	struct device_node *remote = NULL;
-	struct device_node *node;
-	bool is_bridge = false;
 	int ret = 0;
 
 	local_output = of_graph_get_endpoint_by_regs(lvds->dev->of_node, 1, 0);
@@ -735,45 +783,17 @@  static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
 		goto done;
 	}
 
-	remote_input = of_graph_get_remote_endpoint(local_output);
-
-	for_each_endpoint_of_node(remote, node) {
-		if (node != remote_input) {
-			/*
-			 * We've found one endpoint other than the input, this
-			 * must be a bridge.
-			 */
-			is_bridge = true;
-			of_node_put(node);
-			break;
-		}
-	}
-
-	if (is_bridge) {
-		lvds->next_bridge = of_drm_find_bridge(remote);
-		if (!lvds->next_bridge) {
-			ret = -EPROBE_DEFER;
-			goto done;
-		}
-
-		if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
-			lvds->dual_link = lvds->next_bridge->timings
-					? lvds->next_bridge->timings->dual_link
-					: false;
-	} else {
-		lvds->panel = of_drm_find_panel(remote);
-		if (IS_ERR(lvds->panel)) {
-			ret = PTR_ERR(lvds->panel);
-			goto done;
-		}
+	ret = drm_of_find_panel_or_bridge(lvds->dev->of_node, 1, 0,
+					  &lvds->panel, &lvds->next_bridge);
+	if (ret) {
+		ret = -EPROBE_DEFER;
+		goto done;
 	}
-
-	if (lvds->dual_link)
+	if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK)
 		ret = rcar_lvds_parse_dt_companion(lvds);
 
 done:
 	of_node_put(local_output);
-	of_node_put(remote_input);
 	of_node_put(remote);
 
 	/*