RE: [PATCH v2 7/9] drm: rcar-du: lvds: Add dual-LVDS panels support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Laurent,

Thank you for getting back to me.

> From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxx>
> > > >
> > > > ---
> > > > 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




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux