RE: [PATCH v3 4/8] drm: rcar-du: lvds: Add dual-LVDS panels support

[Fabrizio Castro]

Hi Laurent,

> From: linux-renesas-soc-owner@xxxxxxxxxxxxxxx <linux-renesas-soc-owner@xxxxxxxxxxxxxxx> On Behalf Of Laurent Pinchart
> Sent: 07 November 2019 19:51
> Subject: Re: [PATCH v3 4/8] drm: rcar-du: lvds: Add dual-LVDS panels support
> 
> 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.

Sorry for the pain, I'll split this patch into smaller patches.

> 
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@xxxxxxxxxxxxxx>
> >
> > ---
> > 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 ?
> :-)

I gave that a try, it doesn't work for me, even after fixing the NULL
pointer. Perhaps we could finalize this series first and then we could figure
that patch out next?

> 
> > 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.

I'll be restructuring this code a little, and I'll move the put up here, as you suggested

> 
> > +	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.

Thanks

> 
> > +	/*
> > +	 * 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 ?

I don't like it either, but the companion encoder doesn't hold a reference to the
Primary encoder right now, so we would need to change strategy for this.
I think perhaps we could add this solution to the driver, and then we fix it
properly later on?

> 
> >
> >  	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.

Unfortunately I am breaking backward compatibility here. Will be more careful
in v4, sorry!

> 
> > -	} 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 ?

Can do


Thanks,
Fab

> 
> > +		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);
> >
> >  	/*
> 
> --
> Regards,
> 
> Laurent Pinchart