[v2,7/9] drm: rcar-du: lvds: Add dual-LVDS panels support
diff mbox series

Message ID 1565867073-24746-8-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. 15, 2019, 11:04 a.m. UTC
This patch adds support for dual-LVDS panels.

It's very important that we coordinate the efforts of both the
primary encoder and the companion encoder to get the right
picture on the panel, therefore this patch adds some code
to work out if even and odd pixels need swapping.
When the encoders are connected to a LVDS panel, the assumption
is that by default the panel expects even pixels (0, 2, 4, etc.)
on the first input port, and odd pixels (1, 3, 5, etc.) on the
seconds port. When DRM_LINK_DUAL_LVDS_ODD_EVEN is found among the
link flags, the display expects odd pixels on the first input
port, and even pixels on the second port. As a result, the way
the encoders are connected to the panel may trigger pixel (data)
swapping.

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

---
v1->v2:
* new patch, resulting from Laurent's feedback

 drivers/gpu/drm/rcar-du/rcar_lvds.c | 121 ++++++++++++++++++++++++------------
 1 file changed, 81 insertions(+), 40 deletions(-)

Comments

Laurent Pinchart Aug. 15, 2019, 1:08 p.m. UTC | #1
Hi Fabrizio,

Thank you for the patch.

On Thu, Aug 15, 2019 at 12:04:31PM +0100, Fabrizio Castro wrote:
> This patch adds support for dual-LVDS panels.
> 
> It's very important that we coordinate the efforts of both the
> primary encoder and the companion encoder to get the right
> picture on the panel, therefore this patch adds some code
> to work out if even and odd pixels need swapping.
> When the encoders are connected to a LVDS panel, the assumption
> is that by default the panel expects even pixels (0, 2, 4, etc.)
> on the first input port, and odd pixels (1, 3, 5, etc.) on the
> seconds port. When DRM_LINK_DUAL_LVDS_ODD_EVEN is found among the
> link flags, the display expects odd pixels on the first input
> port, and even pixels on the second port. As a result, the way
> the encoders are connected to the panel may trigger pixel (data)
> swapping.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> 
> ---
> v1->v2:
> * new patch, resulting from Laurent's feedback
> 
>  drivers/gpu/drm/rcar-du/rcar_lvds.c | 121 ++++++++++++++++++++++++------------
>  1 file changed, 81 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index 41d28f4..5c24884 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -22,6 +22,8 @@
>  #include <drm/drm_bridge.h>
>  #include <drm/drm_panel.h>
>  #include <drm/drm_probe_helper.h>
> +#include <drm/drm_timings.h>
> +#include <drm/drm_of.h>

Please keep the headers alphabetically sorted.

>  
>  #include "rcar_lvds.h"
>  #include "rcar_lvds_regs.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);

Let's sort out the alignment.

			lvdstripe = LVDSTRIPE_ST_ON
				  | (lvds->companion && lvds->stripe_swap_data ?
				     LVDSTRIPE_ST_SWAP : 0);

> +		rcar_lvds_write(lvds, LVDSTRIPE, lvdstripe);
>  	}
>  
>  	/*
> @@ -706,13 +717,31 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
>  	return ret;
>  }
>  
> +static int rcar_lvds_get_remote_port_reg(struct device_node *np)
> +{
> +	struct device_node *endpoint_node, *remote_endpoint;
> +	struct of_endpoint endpoint;
> +
> +	endpoint_node = of_graph_get_endpoint_by_regs(np, 1, 0);
> +	if (!endpoint_node)
> +		return -ENODEV;
> +	remote_endpoint = of_graph_get_remote_endpoint(endpoint_node);
> +	if (!remote_endpoint) {
> +		of_node_put(endpoint_node);
> +		return -ENODEV;
> +	}
> +	of_graph_parse_endpoint(remote_endpoint, &endpoint);
> +	of_node_put(endpoint_node);
> +	of_node_put(remote_endpoint);
> +
> +	return endpoint.port;
> +}
> +
>  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;
> +	const struct drm_timings *timings = NULL;
>  	int ret = 0;
>  
>  	local_output = of_graph_get_endpoint_by_regs(lvds->dev->of_node, 1, 0);
> @@ -740,45 +769,57 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
>  		goto done;
>  	}
>  
> -	remote_input = of_graph_get_remote_endpoint(local_output);
> +	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->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) {
> +		if (lvds->next_bridge)
> +			timings = lvds->next_bridge->timings;
> +		else
> +			timings = lvds->panel->timings;

I wonder if we should use devm_drm_panel_bridge_add() (or
drm_panel_bridge_add()) and use the bridge API only. It would require a
small change in the drm_panel_bridge to copy the timings pointer, but
apart from that I think it should be fine. If it creates too much churn
due to connector handling then we can skip it for now and I'll handle it
later (but I'd appreciate if you could copy the timings pointer in
drm_panel_bridge already).

> +		if (timings)
> +			lvds->dual_link = timings->dual_link;
> +	}
>  
> -	for_each_endpoint_of_node(remote, node) {
> -		if (node != remote_input) {
> +	if (lvds->dual_link) {
> +		ret = rcar_lvds_parse_dt_companion(lvds);
> +		if (lvds->companion && timings) {
> +			int our_port, comp_port;
> +			bool odd_even_flag = timings->link_flags &
> +						DRM_LINK_DUAL_LVDS_ODD_EVEN;
> +			our_port = rcar_lvds_get_remote_port_reg(
> +						lvds->dev->of_node);
> +			if (our_port < 0) {
> +				ret = our_port;
> +				goto done;
> +			}
> +			comp_port = rcar_lvds_get_remote_port_reg(
> +						lvds->companion->of_node);
> +			if (comp_port < 0) {
> +				ret = comp_port;
> +				goto done;
> +			}
>  			/*
> -			 * We've found one endpoint other than the input, this
> -			 * must be a bridge.
> +			 * We need to match the port where we generate even
> +			 * pixels (0, 2, 4, etc.) to the port where the sink
> +			 * expects to receive even pixels, same thing for the
> +			 * odd pixels. Swap the generation of even and odd
> +			 * pixels if the connection requires it.
> +			 * By default (when DRM_LINK_DUAL_LVDS_ODD_EVEN is not
> +			 * specified) the sink expects even pixels on the
> +			 * first input port, and odd pixels on the second port.

I see what you're trying to do, but I'm not sure I like it much :-S

Peeking into the remove DT node like that creates a dependency between
this driver and the DT bindings of all possible remote nodes. For this
to work, you would need to ensure that the odd/even mapping to ports is
common to all dual-link devices, and thus document that globally in the
DT bindings. I'm not sure if there's a way around it as hardware
connections could indeed switch the two lanes, so we need to model that
somehow. It could be modelled with a swap property in DT, but that would
still require a standard mapping of odd-even pixels to ports, so maybe
the easiest option is to document globally that port 0 on the sink is
for even pixels, and port 1 for odd pixels, and remove the
DRM_LINK_DUAL_LVDS_ODD_EVEN flag completely. But what will then happen
if you panel has more than two ports (for audio for instance, or for
other types of video links) ? It may not be possible to always use port
0 and 1 for the LVDS even and odd pixels in DT bindings of a particular
panel or bridge.

A creative solution is needed here.

>  			 */
> -			is_bridge = true;
> -			of_node_put(node);
> -			break;
> +			if (((comp_port - our_port > 0) &&  odd_even_flag) ||
> +			    ((comp_port - our_port < 0) && !odd_even_flag))
> +				lvds->stripe_swap_data = true;
>  		}
>  	}
>  
> -	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;
> -		}
> -	}
> -
> -	if (lvds->dual_link)
> -		ret = rcar_lvds_parse_dt_companion(lvds);
> -
>  done:
>  	of_node_put(local_output);
> -	of_node_put(remote_input);
>  	of_node_put(remote);
>  
>  	/*
Fabrizio Castro Aug. 15, 2019, 3:36 p.m. UTC | #2
Hello Laurent,

Thank you for your feedback!

> From: linux-renesas-soc-owner@vger.kernel.org <linux-renesas-soc-owner@vger.kernel.org> On Behalf Of Laurent Pinchart
> Sent: 15 August 2019 14:09
> Subject: Re: [PATCH v2 7/9] drm: rcar-du: lvds: Add dual-LVDS panels support
> 
> Hi Fabrizio,
> 
> Thank you for the patch.
> 
> On Thu, Aug 15, 2019 at 12:04:31PM +0100, Fabrizio Castro wrote:
> > This patch adds support for dual-LVDS panels.
> >
> > It's very important that we coordinate the efforts of both the
> > primary encoder and the companion encoder to get the right
> > picture on the panel, therefore this patch adds some code
> > to work out if even and odd pixels need swapping.
> > When the encoders are connected to a LVDS panel, the assumption
> > is that by default the panel expects even pixels (0, 2, 4, etc.)
> > on the first input port, and odd pixels (1, 3, 5, etc.) on the
> > seconds port. When DRM_LINK_DUAL_LVDS_ODD_EVEN is found among the
> > link flags, the display expects odd pixels on the first input
> > port, and even pixels on the second port. As a result, the way
> > the encoders are connected to the panel may trigger pixel (data)
> > swapping.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >
> > ---
> > v1->v2:
> > * new patch, resulting from Laurent's feedback
> >
> >  drivers/gpu/drm/rcar-du/rcar_lvds.c | 121 ++++++++++++++++++++++++------------
> >  1 file changed, 81 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > index 41d28f4..5c24884 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > @@ -22,6 +22,8 @@
> >  #include <drm/drm_bridge.h>
> >  #include <drm/drm_panel.h>
> >  #include <drm/drm_probe_helper.h>
> > +#include <drm/drm_timings.h>
> > +#include <drm/drm_of.h>
> 
> Please keep the headers alphabetically sorted.

Ok

> 
> >
> >  #include "rcar_lvds.h"
> >  #include "rcar_lvds_regs.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);
> 
> Let's sort out the alignment.
> 
> 			lvdstripe = LVDSTRIPE_ST_ON
> 				  | (lvds->companion && lvds->stripe_swap_data ?
> 				     LVDSTRIPE_ST_SWAP : 0);

Ok

> 
> > +		rcar_lvds_write(lvds, LVDSTRIPE, lvdstripe);
> >  	}
> >
> >  	/*
> > @@ -706,13 +717,31 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
> >  	return ret;
> >  }
> >
> > +static int rcar_lvds_get_remote_port_reg(struct device_node *np)
> > +{
> > +	struct device_node *endpoint_node, *remote_endpoint;
> > +	struct of_endpoint endpoint;
> > +
> > +	endpoint_node = of_graph_get_endpoint_by_regs(np, 1, 0);
> > +	if (!endpoint_node)
> > +		return -ENODEV;
> > +	remote_endpoint = of_graph_get_remote_endpoint(endpoint_node);
> > +	if (!remote_endpoint) {
> > +		of_node_put(endpoint_node);
> > +		return -ENODEV;
> > +	}
> > +	of_graph_parse_endpoint(remote_endpoint, &endpoint);
> > +	of_node_put(endpoint_node);
> > +	of_node_put(remote_endpoint);
> > +
> > +	return endpoint.port;
> > +}
> > +
> >  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;
> > +	const struct drm_timings *timings = NULL;
> >  	int ret = 0;
> >
> >  	local_output = of_graph_get_endpoint_by_regs(lvds->dev->of_node, 1, 0);
> > @@ -740,45 +769,57 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> >  		goto done;
> >  	}
> >
> > -	remote_input = of_graph_get_remote_endpoint(local_output);
> > +	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->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) {
> > +		if (lvds->next_bridge)
> > +			timings = lvds->next_bridge->timings;
> > +		else
> > +			timings = lvds->panel->timings;
> 
> I wonder if we should use devm_drm_panel_bridge_add() (or
> drm_panel_bridge_add()) and use the bridge API only. It would require a
> small change in the drm_panel_bridge to copy the timings pointer, but
> apart from that I think it should be fine. If it creates too much churn
> due to connector handling then we can skip it for now and I'll handle it
> later (but I'd appreciate if you could copy the timings pointer in
> drm_panel_bridge already).

Will look into this.

> 
> > +		if (timings)
> > +			lvds->dual_link = timings->dual_link;
> > +	}
> >
> > -	for_each_endpoint_of_node(remote, node) {
> > -		if (node != remote_input) {
> > +	if (lvds->dual_link) {
> > +		ret = rcar_lvds_parse_dt_companion(lvds);
> > +		if (lvds->companion && timings) {
> > +			int our_port, comp_port;
> > +			bool odd_even_flag = timings->link_flags &
> > +						DRM_LINK_DUAL_LVDS_ODD_EVEN;
> > +			our_port = rcar_lvds_get_remote_port_reg(
> > +						lvds->dev->of_node);
> > +			if (our_port < 0) {
> > +				ret = our_port;
> > +				goto done;
> > +			}
> > +			comp_port = rcar_lvds_get_remote_port_reg(
> > +						lvds->companion->of_node);
> > +			if (comp_port < 0) {
> > +				ret = comp_port;
> > +				goto done;
> > +			}
> >  			/*
> > -			 * We've found one endpoint other than the input, this
> > -			 * must be a bridge.
> > +			 * We need to match the port where we generate even
> > +			 * pixels (0, 2, 4, etc.) to the port where the sink
> > +			 * expects to receive even pixels, same thing for the
> > +			 * odd pixels. Swap the generation of even and odd
> > +			 * pixels if the connection requires it.
> > +			 * By default (when DRM_LINK_DUAL_LVDS_ODD_EVEN is not
> > +			 * specified) the sink expects even pixels on the
> > +			 * first input port, and odd pixels on the second port.
> 
> I see what you're trying to do, but I'm not sure I like it much :-S
> 
> Peeking into the remove DT node like that creates a dependency between
> this driver and the DT bindings of all possible remote nodes. For this
> to work, you would need to ensure that the odd/even mapping to ports is
> common to all dual-link devices, and thus document that globally in the
> DT bindings. I'm not sure if there's a way around it as hardware
> connections could indeed switch the two lanes, so we need to model that
> somehow. It could be modelled with a swap property in DT, but that would
> still require a standard mapping of odd-even pixels to ports, so maybe
> the easiest option is to document globally that port 0 on the sink is
> for even pixels, and port 1 for odd pixels, and remove the
> DRM_LINK_DUAL_LVDS_ODD_EVEN flag completely. But what will then happen
> if you panel has more than two ports (for audio for instance, or for
> other types of video links) ? It may not be possible to always use port
> 0 and 1 for the LVDS even and odd pixels in DT bindings of a particular
> panel or bridge.

This is the stickiest point of the whole series. The implementation within this
series allows for any number of ports on the sink, the LVDS ports don't have
to be port 0 and port 1, it's enough that the port for the even pixels comes
before the port of the odd pixels (but the logic can be inverted by means of
DRM_LINK_DUAL_LVDS_ODD_EVEN), and if you swap the lvds0 and lvds1
OF graph connections around, the pixels will be swapped automatically.
But of course, there is the dependency between the driver and dt-bindings
you were mentioning, and of top of that every driver would need to work
things out independently at this point in time.

> 
> A creative solution is needed here.

I may have an idea. What if we marked both ends of each OF graph link
with either "even-pixels;" or "odd-pixels;", and exported a function that
given the of_node of two endpoints returned if the link requires swapping?
There'd be no need for the flag at that point, the numbering of the ports
would not matter, and the DT would be comprehensive and very easy to read.

Let me please know your thoughts.

Thanks you for the patience
Fab

> 
> >  			 */
> > -			is_bridge = true;
> > -			of_node_put(node);
> > -			break;
> > +			if (((comp_port - our_port > 0) &&  odd_even_flag) ||
> > +			    ((comp_port - our_port < 0) && !odd_even_flag))
> > +				lvds->stripe_swap_data = true;
> >  		}
> >  	}
> >
> > -	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;
> > -		}
> > -	}
> > -
> > -	if (lvds->dual_link)
> > -		ret = rcar_lvds_parse_dt_companion(lvds);
> > -
> >  done:
> >  	of_node_put(local_output);
> > -	of_node_put(remote_input);
> >  	of_node_put(remote);
> >
> >  	/*
> 
> --
> Regards,
> 
> Laurent Pinchart
Laurent Pinchart Aug. 20, 2019, 4:04 p.m. UTC | #3
Hi Fabrizio,

On Thu, Aug 15, 2019 at 03:36:53PM +0000, Fabrizio Castro wrote:
> On 15 August 2019 14:09 Laurent Pinchart wrote:
> > On Thu, Aug 15, 2019 at 12:04:31PM +0100, Fabrizio Castro wrote:
> > > This patch adds support for dual-LVDS panels.
> > >
> > > It's very important that we coordinate the efforts of both the
> > > primary encoder and the companion encoder to get the right
> > > picture on the panel, therefore this patch adds some code
> > > to work out if even and odd pixels need swapping.
> > > When the encoders are connected to a LVDS panel, the assumption
> > > is that by default the panel expects even pixels (0, 2, 4, etc.)
> > > on the first input port, and odd pixels (1, 3, 5, etc.) on the
> > > seconds port. When DRM_LINK_DUAL_LVDS_ODD_EVEN is found among the
> > > link flags, the display expects odd pixels on the first input
> > > port, and even pixels on the second port. As a result, the way
> > > the encoders are connected to the panel may trigger pixel (data)
> > > swapping.
> > >
> > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > >
> > > ---
> > > v1->v2:
> > > * new patch, resulting from Laurent's feedback
> > >
> > >  drivers/gpu/drm/rcar-du/rcar_lvds.c | 121 ++++++++++++++++++++++++------------
> > >  1 file changed, 81 insertions(+), 40 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > > index 41d28f4..5c24884 100644
> > > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > > @@ -22,6 +22,8 @@
> > >  #include <drm/drm_bridge.h>
> > >  #include <drm/drm_panel.h>
> > >  #include <drm/drm_probe_helper.h>
> > > +#include <drm/drm_timings.h>
> > > +#include <drm/drm_of.h>
> > 
> > Please keep the headers alphabetically sorted.
> 
> Ok
> 
> > >
> > >  #include "rcar_lvds.h"
> > >  #include "rcar_lvds_regs.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);
> > 
> > Let's sort out the alignment.
> > 
> > 			lvdstripe = LVDSTRIPE_ST_ON
> > 				  | (lvds->companion && lvds->stripe_swap_data ?
> > 				     LVDSTRIPE_ST_SWAP : 0);
> 
> Ok
> 
> > > +		rcar_lvds_write(lvds, LVDSTRIPE, lvdstripe);
> > >  	}
> > >
> > >  	/*
> > > @@ -706,13 +717,31 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
> > >  	return ret;
> > >  }
> > >
> > > +static int rcar_lvds_get_remote_port_reg(struct device_node *np)
> > > +{
> > > +	struct device_node *endpoint_node, *remote_endpoint;
> > > +	struct of_endpoint endpoint;
> > > +
> > > +	endpoint_node = of_graph_get_endpoint_by_regs(np, 1, 0);
> > > +	if (!endpoint_node)
> > > +		return -ENODEV;
> > > +	remote_endpoint = of_graph_get_remote_endpoint(endpoint_node);
> > > +	if (!remote_endpoint) {
> > > +		of_node_put(endpoint_node);
> > > +		return -ENODEV;
> > > +	}
> > > +	of_graph_parse_endpoint(remote_endpoint, &endpoint);
> > > +	of_node_put(endpoint_node);
> > > +	of_node_put(remote_endpoint);
> > > +
> > > +	return endpoint.port;
> > > +}
> > > +
> > >  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;
> > > +	const struct drm_timings *timings = NULL;
> > >  	int ret = 0;
> > >
> > >  	local_output = of_graph_get_endpoint_by_regs(lvds->dev->of_node, 1, 0);
> > > @@ -740,45 +769,57 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> > >  		goto done;
> > >  	}
> > >
> > > -	remote_input = of_graph_get_remote_endpoint(local_output);
> > > +	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->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) {
> > > +		if (lvds->next_bridge)
> > > +			timings = lvds->next_bridge->timings;
> > > +		else
> > > +			timings = lvds->panel->timings;
> > 
> > I wonder if we should use devm_drm_panel_bridge_add() (or
> > drm_panel_bridge_add()) and use the bridge API only. It would require a
> > small change in the drm_panel_bridge to copy the timings pointer, but
> > apart from that I think it should be fine. If it creates too much churn
> > due to connector handling then we can skip it for now and I'll handle it
> > later (but I'd appreciate if you could copy the timings pointer in
> > drm_panel_bridge already).
> 
> Will look into this.
> 
> > > +		if (timings)
> > > +			lvds->dual_link = timings->dual_link;
> > > +	}
> > >
> > > -	for_each_endpoint_of_node(remote, node) {
> > > -		if (node != remote_input) {
> > > +	if (lvds->dual_link) {
> > > +		ret = rcar_lvds_parse_dt_companion(lvds);
> > > +		if (lvds->companion && timings) {
> > > +			int our_port, comp_port;
> > > +			bool odd_even_flag = timings->link_flags &
> > > +						DRM_LINK_DUAL_LVDS_ODD_EVEN;
> > > +			our_port = rcar_lvds_get_remote_port_reg(
> > > +						lvds->dev->of_node);
> > > +			if (our_port < 0) {
> > > +				ret = our_port;
> > > +				goto done;
> > > +			}
> > > +			comp_port = rcar_lvds_get_remote_port_reg(
> > > +						lvds->companion->of_node);
> > > +			if (comp_port < 0) {
> > > +				ret = comp_port;
> > > +				goto done;
> > > +			}
> > >  			/*
> > > -			 * We've found one endpoint other than the input, this
> > > -			 * must be a bridge.
> > > +			 * We need to match the port where we generate even
> > > +			 * pixels (0, 2, 4, etc.) to the port where the sink
> > > +			 * expects to receive even pixels, same thing for the
> > > +			 * odd pixels. Swap the generation of even and odd
> > > +			 * pixels if the connection requires it.
> > > +			 * By default (when DRM_LINK_DUAL_LVDS_ODD_EVEN is not
> > > +			 * specified) the sink expects even pixels on the
> > > +			 * first input port, and odd pixels on the second port.
> > 
> > I see what you're trying to do, but I'm not sure I like it much :-S
> > 
> > Peeking into the remove DT node like that creates a dependency between
> > this driver and the DT bindings of all possible remote nodes. For this
> > to work, you would need to ensure that the odd/even mapping to ports is
> > common to all dual-link devices, and thus document that globally in the
> > DT bindings. I'm not sure if there's a way around it as hardware
> > connections could indeed switch the two lanes, so we need to model that
> > somehow. It could be modelled with a swap property in DT, but that would
> > still require a standard mapping of odd-even pixels to ports, so maybe
> > the easiest option is to document globally that port 0 on the sink is
> > for even pixels, and port 1 for odd pixels, and remove the
> > DRM_LINK_DUAL_LVDS_ODD_EVEN flag completely. But what will then happen
> > if you panel has more than two ports (for audio for instance, or for
> > other types of video links) ? It may not be possible to always use port
> > 0 and 1 for the LVDS even and odd pixels in DT bindings of a particular
> > panel or bridge.
> 
> This is the stickiest point of the whole series. The implementation within this
> series allows for any number of ports on the sink, the LVDS ports don't have
> to be port 0 and port 1, it's enough that the port for the even pixels comes
> before the port of the odd pixels (but the logic can be inverted by means of
> DRM_LINK_DUAL_LVDS_ODD_EVEN), and if you swap the lvds0 and lvds1
> OF graph connections around, the pixels will be swapped automatically.
> But of course, there is the dependency between the driver and dt-bindings
> you were mentioning, and of top of that every driver would need to work
> things out independently at this point in time.
> 
> > A creative solution is needed here.
> 
> I may have an idea. What if we marked both ends of each OF graph link
> with either "even-pixels;" or "odd-pixels;", and exported a function that
> given the of_node of two endpoints returned if the link requires swapping?
> There'd be no need for the flag at that point, the numbering of the ports
> would not matter, and the DT would be comprehensive and very easy to read.
> 
> Let me please know your thoughts.

Taking one step back to look at the big picture, what we need is for the
source to know what pixel (odd or even) to send on each LVDS output. For
that it needs to know what pixel is expected by the sink the the inputs
connected to the source's outputs. From each source output we can easily
locate the DT nodes corresponding to the connected sink's input ports,
but that doesn't give us enough information. From there, we could

- Infer the odd/even pixel expected by the source based on the port
  numbers. This would require common DT bindings for all dual-link LVDS
  sinks that specify the port ordering (for instance the bindings could
  mandate that lowest numbered port correspond to even pixels).

- Read the odd/even pixel expected by the source from an explicit DT
  property, as proposed above. This would also require common DT
  bindings for all dual-link LVDS sinks that define these new
  properties. This would I think be better than implicitly infering it
  from DT port numbers.

- Retrieve the information from the sink drm_bridge at runtime. This
  would require a way to query the bridge for the mapping from port
  number to odd/even pixel. The DRM_LINK_DUAL_LVDS_ODD_EVEN flag could
  be used for that, and would then be defined as "the lowest numbered
  port corresponds to odd pixels and the highest numbered port
  corresponds to even pixels".

The second and third options would both work I think. The third one is
roughly what you've implemented, except that I think the flag
description should be clarified.

Sorry for circling back to your original proposal, I didn't understand
it properly :-)

> Thanks you for the patience
> 
> > >  			 */
> > > -			is_bridge = true;
> > > -			of_node_put(node);
> > > -			break;
> > > +			if (((comp_port - our_port > 0) &&  odd_even_flag) ||
> > > +			    ((comp_port - our_port < 0) && !odd_even_flag))
> > > +				lvds->stripe_swap_data = true;
> > >  		}
> > >  	}
> > >
> > > -	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;
> > > -		}
> > > -	}
> > > -
> > > -	if (lvds->dual_link)
> > > -		ret = rcar_lvds_parse_dt_companion(lvds);
> > > -
> > >  done:
> > >  	of_node_put(local_output);
> > > -	of_node_put(remote_input);
> > >  	of_node_put(remote);
> > >
> > >  	/*
Fabrizio Castro Aug. 21, 2019, 5 p.m. UTC | #4
Hi Laurent,

Thank you for getting back to me.

> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: 20 August 2019 17:04
> Subject: Re: [PATCH v2 7/9] drm: rcar-du: lvds: Add dual-LVDS panels support
> 
> Hi Fabrizio,
> 
> On Thu, Aug 15, 2019 at 03:36:53PM +0000, Fabrizio Castro wrote:
> > On 15 August 2019 14:09 Laurent Pinchart wrote:
> > > On Thu, Aug 15, 2019 at 12:04:31PM +0100, Fabrizio Castro wrote:
> > > > This patch adds support for dual-LVDS panels.
> > > >
> > > > It's very important that we coordinate the efforts of both the
> > > > primary encoder and the companion encoder to get the right
> > > > picture on the panel, therefore this patch adds some code
> > > > to work out if even and odd pixels need swapping.
> > > > When the encoders are connected to a LVDS panel, the assumption
> > > > is that by default the panel expects even pixels (0, 2, 4, etc.)
> > > > on the first input port, and odd pixels (1, 3, 5, etc.) on the
> > > > seconds port. When DRM_LINK_DUAL_LVDS_ODD_EVEN is found among the
> > > > link flags, the display expects odd pixels on the first input
> > > > port, and even pixels on the second port. As a result, the way
> > > > the encoders are connected to the panel may trigger pixel (data)
> > > > swapping.
> > > >
> > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > > >
> > > > ---
> > > > v1->v2:
> > > > * new patch, resulting from Laurent's feedback
> > > >
> > > >  drivers/gpu/drm/rcar-du/rcar_lvds.c | 121 ++++++++++++++++++++++++------------
> > > >  1 file changed, 81 insertions(+), 40 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > > > index 41d28f4..5c24884 100644
> > > > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > > > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > > > @@ -22,6 +22,8 @@
> > > >  #include <drm/drm_bridge.h>
> > > >  #include <drm/drm_panel.h>
> > > >  #include <drm/drm_probe_helper.h>
> > > > +#include <drm/drm_timings.h>
> > > > +#include <drm/drm_of.h>
> > >
> > > Please keep the headers alphabetically sorted.
> >
> > Ok
> >
> > > >
> > > >  #include "rcar_lvds.h"
> > > >  #include "rcar_lvds_regs.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);
> > >
> > > Let's sort out the alignment.
> > >
> > > 			lvdstripe = LVDSTRIPE_ST_ON
> > > 				  | (lvds->companion && lvds->stripe_swap_data ?
> > > 				     LVDSTRIPE_ST_SWAP : 0);
> >
> > Ok
> >
> > > > +		rcar_lvds_write(lvds, LVDSTRIPE, lvdstripe);
> > > >  	}
> > > >
> > > >  	/*
> > > > @@ -706,13 +717,31 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
> > > >  	return ret;
> > > >  }
> > > >
> > > > +static int rcar_lvds_get_remote_port_reg(struct device_node *np)
> > > > +{
> > > > +	struct device_node *endpoint_node, *remote_endpoint;
> > > > +	struct of_endpoint endpoint;
> > > > +
> > > > +	endpoint_node = of_graph_get_endpoint_by_regs(np, 1, 0);
> > > > +	if (!endpoint_node)
> > > > +		return -ENODEV;
> > > > +	remote_endpoint = of_graph_get_remote_endpoint(endpoint_node);
> > > > +	if (!remote_endpoint) {
> > > > +		of_node_put(endpoint_node);
> > > > +		return -ENODEV;
> > > > +	}
> > > > +	of_graph_parse_endpoint(remote_endpoint, &endpoint);
> > > > +	of_node_put(endpoint_node);
> > > > +	of_node_put(remote_endpoint);
> > > > +
> > > > +	return endpoint.port;
> > > > +}
> > > > +
> > > >  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;
> > > > +	const struct drm_timings *timings = NULL;
> > > >  	int ret = 0;
> > > >
> > > >  	local_output = of_graph_get_endpoint_by_regs(lvds->dev->of_node, 1, 0);
> > > > @@ -740,45 +769,57 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> > > >  		goto done;
> > > >  	}
> > > >
> > > > -	remote_input = of_graph_get_remote_endpoint(local_output);
> > > > +	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->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) {
> > > > +		if (lvds->next_bridge)
> > > > +			timings = lvds->next_bridge->timings;
> > > > +		else
> > > > +			timings = lvds->panel->timings;
> > >
> > > I wonder if we should use devm_drm_panel_bridge_add() (or
> > > drm_panel_bridge_add()) and use the bridge API only. It would require a
> > > small change in the drm_panel_bridge to copy the timings pointer, but
> > > apart from that I think it should be fine. If it creates too much churn
> > > due to connector handling then we can skip it for now and I'll handle it
> > > later (but I'd appreciate if you could copy the timings pointer in
> > > drm_panel_bridge already).
> >
> > Will look into this.
> >
> > > > +		if (timings)
> > > > +			lvds->dual_link = timings->dual_link;
> > > > +	}
> > > >
> > > > -	for_each_endpoint_of_node(remote, node) {
> > > > -		if (node != remote_input) {
> > > > +	if (lvds->dual_link) {
> > > > +		ret = rcar_lvds_parse_dt_companion(lvds);
> > > > +		if (lvds->companion && timings) {
> > > > +			int our_port, comp_port;
> > > > +			bool odd_even_flag = timings->link_flags &
> > > > +						DRM_LINK_DUAL_LVDS_ODD_EVEN;
> > > > +			our_port = rcar_lvds_get_remote_port_reg(
> > > > +						lvds->dev->of_node);
> > > > +			if (our_port < 0) {
> > > > +				ret = our_port;
> > > > +				goto done;
> > > > +			}
> > > > +			comp_port = rcar_lvds_get_remote_port_reg(
> > > > +						lvds->companion->of_node);
> > > > +			if (comp_port < 0) {
> > > > +				ret = comp_port;
> > > > +				goto done;
> > > > +			}
> > > >  			/*
> > > > -			 * We've found one endpoint other than the input, this
> > > > -			 * must be a bridge.
> > > > +			 * We need to match the port where we generate even
> > > > +			 * pixels (0, 2, 4, etc.) to the port where the sink
> > > > +			 * expects to receive even pixels, same thing for the
> > > > +			 * odd pixels. Swap the generation of even and odd
> > > > +			 * pixels if the connection requires it.
> > > > +			 * By default (when DRM_LINK_DUAL_LVDS_ODD_EVEN is not
> > > > +			 * specified) the sink expects even pixels on the
> > > > +			 * first input port, and odd pixels on the second port.
> > >
> > > I see what you're trying to do, but I'm not sure I like it much :-S
> > >
> > > Peeking into the remove DT node like that creates a dependency between
> > > this driver and the DT bindings of all possible remote nodes. For this
> > > to work, you would need to ensure that the odd/even mapping to ports is
> > > common to all dual-link devices, and thus document that globally in the
> > > DT bindings. I'm not sure if there's a way around it as hardware
> > > connections could indeed switch the two lanes, so we need to model that
> > > somehow. It could be modelled with a swap property in DT, but that would
> > > still require a standard mapping of odd-even pixels to ports, so maybe
> > > the easiest option is to document globally that port 0 on the sink is
> > > for even pixels, and port 1 for odd pixels, and remove the
> > > DRM_LINK_DUAL_LVDS_ODD_EVEN flag completely. But what will then happen
> > > if you panel has more than two ports (for audio for instance, or for
> > > other types of video links) ? It may not be possible to always use port
> > > 0 and 1 for the LVDS even and odd pixels in DT bindings of a particular
> > > panel or bridge.
> >
> > This is the stickiest point of the whole series. The implementation within this
> > series allows for any number of ports on the sink, the LVDS ports don't have
> > to be port 0 and port 1, it's enough that the port for the even pixels comes
> > before the port of the odd pixels (but the logic can be inverted by means of
> > DRM_LINK_DUAL_LVDS_ODD_EVEN), and if you swap the lvds0 and lvds1
> > OF graph connections around, the pixels will be swapped automatically.
> > But of course, there is the dependency between the driver and dt-bindings
> > you were mentioning, and of top of that every driver would need to work
> > things out independently at this point in time.
> >
> > > A creative solution is needed here.
> >
> > I may have an idea. What if we marked both ends of each OF graph link
> > with either "even-pixels;" or "odd-pixels;", and exported a function that
> > given the of_node of two endpoints returned if the link requires swapping?
> > There'd be no need for the flag at that point, the numbering of the ports
> > would not matter, and the DT would be comprehensive and very easy to read.
> >
> > Let me please know your thoughts.
> 
> Taking one step back to look at the big picture, what we need is for the
> source to know what pixel (odd or even) to send on each LVDS output. For
> that it needs to know what pixel is expected by the sink the the inputs
> connected to the source's outputs. From each source output we can easily
> locate the DT nodes corresponding to the connected sink's input ports,
> but that doesn't give us enough information. From there, we could
> 
> - Infer the odd/even pixel expected by the source based on the port
>   numbers. This would require common DT bindings for all dual-link LVDS
>   sinks that specify the port ordering (for instance the bindings could
>   mandate that lowest numbered port correspond to even pixels).
> 
> - Read the odd/even pixel expected by the source from an explicit DT
>   property, as proposed above. This would also require common DT
>   bindings for all dual-link LVDS sinks that define these new
>   properties. This would I think be better than implicitly infering it
>   from DT port numbers.
> 
> - Retrieve the information from the sink drm_bridge at runtime. This
>   would require a way to query the bridge for the mapping from port
>   number to odd/even pixel. The DRM_LINK_DUAL_LVDS_ODD_EVEN flag could
>   be used for that, and would then be defined as "the lowest numbered
>   port corresponds to odd pixels and the highest numbered port
>   corresponds to even pixels".
> 
> The second and third options would both work I think. The third one is
> roughly what you've implemented, except that I think the flag
> description should be clarified.

I think I like the second option better, I will give it a shot. I was thinking,
perhaps we could drop the 'dual_link' member altogether (from
drm_<whatever>_timings) in this case, as if we found information about
even and odd pixels in the DT we could work out if we the configuration is
dual-link, so there'd be no need to mark this in drivers.

What do you think?

Thanks,
Fab

> 
> Sorry for circling back to your original proposal, I didn't understand
> it properly :-)
> 
> > Thanks you for the patience
> >
> > > >  			 */
> > > > -			is_bridge = true;
> > > > -			of_node_put(node);
> > > > -			break;
> > > > +			if (((comp_port - our_port > 0) &&  odd_even_flag) ||
> > > > +			    ((comp_port - our_port < 0) && !odd_even_flag))
> > > > +				lvds->stripe_swap_data = true;
> > > >  		}
> > > >  	}
> > > >
> > > > -	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;
> > > > -		}
> > > > -	}
> > > > -
> > > > -	if (lvds->dual_link)
> > > > -		ret = rcar_lvds_parse_dt_companion(lvds);
> > > > -
> > > >  done:
> > > >  	of_node_put(local_output);
> > > > -	of_node_put(remote_input);
> > > >  	of_node_put(remote);
> > > >
> > > >  	/*
> 
> --
> Regards,
> 
> Laurent Pinchart

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 41d28f4..5c24884 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -22,6 +22,8 @@ 
 #include <drm/drm_bridge.h>
 #include <drm/drm_panel.h>
 #include <drm/drm_probe_helper.h>
+#include <drm/drm_timings.h>
+#include <drm/drm_of.h>
 
 #include "rcar_lvds.h"
 #include "rcar_lvds_regs.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);
 	}
 
 	/*
@@ -706,13 +717,31 @@  static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
 	return ret;
 }
 
+static int rcar_lvds_get_remote_port_reg(struct device_node *np)
+{
+	struct device_node *endpoint_node, *remote_endpoint;
+	struct of_endpoint endpoint;
+
+	endpoint_node = of_graph_get_endpoint_by_regs(np, 1, 0);
+	if (!endpoint_node)
+		return -ENODEV;
+	remote_endpoint = of_graph_get_remote_endpoint(endpoint_node);
+	if (!remote_endpoint) {
+		of_node_put(endpoint_node);
+		return -ENODEV;
+	}
+	of_graph_parse_endpoint(remote_endpoint, &endpoint);
+	of_node_put(endpoint_node);
+	of_node_put(remote_endpoint);
+
+	return endpoint.port;
+}
+
 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;
+	const struct drm_timings *timings = NULL;
 	int ret = 0;
 
 	local_output = of_graph_get_endpoint_by_regs(lvds->dev->of_node, 1, 0);
@@ -740,45 +769,57 @@  static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
 		goto done;
 	}
 
-	remote_input = of_graph_get_remote_endpoint(local_output);
+	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->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) {
+		if (lvds->next_bridge)
+			timings = lvds->next_bridge->timings;
+		else
+			timings = lvds->panel->timings;
+		if (timings)
+			lvds->dual_link = timings->dual_link;
+	}
 
-	for_each_endpoint_of_node(remote, node) {
-		if (node != remote_input) {
+	if (lvds->dual_link) {
+		ret = rcar_lvds_parse_dt_companion(lvds);
+		if (lvds->companion && timings) {
+			int our_port, comp_port;
+			bool odd_even_flag = timings->link_flags &
+						DRM_LINK_DUAL_LVDS_ODD_EVEN;
+			our_port = rcar_lvds_get_remote_port_reg(
+						lvds->dev->of_node);
+			if (our_port < 0) {
+				ret = our_port;
+				goto done;
+			}
+			comp_port = rcar_lvds_get_remote_port_reg(
+						lvds->companion->of_node);
+			if (comp_port < 0) {
+				ret = comp_port;
+				goto done;
+			}
 			/*
-			 * We've found one endpoint other than the input, this
-			 * must be a bridge.
+			 * We need to match the port where we generate even
+			 * pixels (0, 2, 4, etc.) to the port where the sink
+			 * expects to receive even pixels, same thing for the
+			 * odd pixels. Swap the generation of even and odd
+			 * pixels if the connection requires it.
+			 * By default (when DRM_LINK_DUAL_LVDS_ODD_EVEN is not
+			 * specified) the sink expects even pixels on the
+			 * first input port, and odd pixels on the second port.
 			 */
-			is_bridge = true;
-			of_node_put(node);
-			break;
+			if (((comp_port - our_port > 0) &&  odd_even_flag) ||
+			    ((comp_port - our_port < 0) && !odd_even_flag))
+				lvds->stripe_swap_data = true;
 		}
 	}
 
-	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;
-		}
-	}
-
-	if (lvds->dual_link)
-		ret = rcar_lvds_parse_dt_companion(lvds);
-
 done:
 	of_node_put(local_output);
-	of_node_put(remote_input);
 	of_node_put(remote);
 
 	/*