Re: [PATCH v2 06/10] drm: rcar-du: lvds: Add support for dual-link mode

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jacopo,

On Tue, May 28, 2019 at 11:35:50AM +0200, Jacopo Mondi wrote:
> Hi Laurent,
>  a small note.
> 
> On Sun, May 12, 2019 at 12:06:58AM +0300, Laurent Pinchart wrote:
> > In dual-link mode the LVDS0 encoder transmits even-numbered pixels, and
> > sends odd-numbered pixels to the LVDS1 encoder for transmission on a
> > separate link.
> >
> > To implement support for this mode of operation, determine if the LVDS
> > connection operates in dual-link mode by querying the next device in the
> > pipeline, locate the companion encoder, and control it directly through
> > its bridge operations.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  drivers/gpu/drm/rcar-du/rcar_lvds.c | 104 ++++++++++++++++++++++++----
> >  drivers/gpu/drm/rcar-du/rcar_lvds.h |   5 ++
> >  2 files changed, 96 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > index a331f0c32187..f7e4710fe33f 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> > @@ -66,6 +66,9 @@ struct rcar_lvds {
> >
> >  	struct drm_display_mode display_mode;
> >  	enum rcar_lvds_mode mode;
> > +
> > +	struct drm_bridge *companion;
> > +	bool dual_link;
> >  };
> >
> >  #define bridge_to_rcar_lvds(bridge) \
> > @@ -400,11 +403,6 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
> >  {
> >  	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
> >  	const struct drm_display_mode *mode = &lvds->display_mode;
> > -	/*
> > -	 * FIXME: We should really retrieve the CRTC through the state, but how
> > -	 * do we get a state pointer?
> > -	 */
> > -	struct drm_crtc *crtc = lvds->bridge.encoder->crtc;
> >  	u32 lvdhcr;
> >  	u32 lvdcr0;
> >  	int ret;
> > @@ -413,6 +411,10 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
> >  	if (ret < 0)
> >  		return;
> >
> > +	/* Enable the companion LVDS encoder in dual-link mode. */
> > +	if (lvds->dual_link && lvds->companion)
> > +		lvds->companion->funcs->enable(lvds->companion);
> > +
> >  	/*
> >  	 * Hardcode the channels and control signals routing for now.
> >  	 *
> > @@ -435,17 +437,33 @@ static void rcar_lvds_enable(struct drm_bridge *bridge)
> >  	rcar_lvds_write(lvds, LVDCHCR, lvdhcr);
> >
> >  	if (lvds->info->quirks & RCAR_LVDS_QUIRK_DUAL_LINK) {
> > -		/* Disable dual-link mode. */
> > -		rcar_lvds_write(lvds, LVDSTRIPE, 0);
> > +		/*
> > +		 * 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);
> >  	}
> >
> > -	/* PLL clock configuration. */
> > -	lvds->info->pll_setup(lvds, mode->clock * 1000);
> > +	/*
> > +	 * PLL clock configuration on all instances but the companion in
> > +	 * dual-link mode.
> > +	 */
> > +	if (!lvds->dual_link || lvds->companion)
> > +		lvds->info->pll_setup(lvds, mode->clock * 1000);
> >
> >  	/* Set the LVDS mode and select the input. */
> >  	lvdcr0 = lvds->mode << LVDCR0_LVMD_SHIFT;
> > -	if (drm_crtc_index(crtc) == 2)
> > -		lvdcr0 |= LVDCR0_DUSEL;
> > +
> > +	if (lvds->bridge.encoder) {
> > +		/*
> > +		 * FIXME: We should really retrieve the CRTC through the state,
> > +		 * but how do we get a state pointer?
> > +		 */
> > +		if (drm_crtc_index(lvds->bridge.encoder->crtc) == 2)
> > +			lvdcr0 |= LVDCR0_DUSEL;
> > +	}
> > +
> >  	rcar_lvds_write(lvds, LVDCR0, lvdcr0);
> >
> >  	/* Turn all the channels on. */
> > @@ -512,6 +530,10 @@ static void rcar_lvds_disable(struct drm_bridge *bridge)
> >  	rcar_lvds_write(lvds, LVDCR1, 0);
> >  	rcar_lvds_write(lvds, LVDPLLCR, 0);
> >
> > +	/* Disable the companion LVDS encoder in dual-link mode. */
> > +	if (lvds->dual_link && lvds->companion)
> > +		lvds->companion->funcs->disable(lvds->companion);
> > +
> >  	clk_disable_unprepare(lvds->clocks.mod);
> >  }
> >
> > @@ -628,10 +650,54 @@ static const struct drm_bridge_funcs rcar_lvds_bridge_ops = {
> >  	.mode_set = rcar_lvds_mode_set,
> >  };
> >
> > +bool rcar_lvds_dual_link(struct drm_bridge *bridge)
> > +{
> > +	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
> > +
> > +	return lvds->dual_link;
> > +}
> > +EXPORT_SYMBOL_GPL(rcar_lvds_dual_link);
> > +
> >  /* -----------------------------------------------------------------------------
> >   * Probe & Remove
> >   */
> >
> > +static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds)
> > +{
> > +	const struct of_device_id *match;
> > +	struct device_node *companion;
> > +	struct device *dev = lvds->dev;
> > +	int ret = 0;
> > +
> > +	/* Locate the companion LVDS encoder for dual-link operation, if any. */
> > +	companion = of_parse_phandle(dev->of_node, "renesas,companion", 0);
> > +	if (!companion)
> > +		return -ENODEV;
> > +
> > +	/*
> > +	 * Sanity check: the companion encoder must have the same compatible
> > +	 * string.
> > +	 */
> > +	match = of_match_device(dev->driver->of_match_table, dev);
> > +	if (!of_device_is_compatible(companion, match->compatible)) {
> > +		ret = -ENODEV;
> > +		goto done;
> > +	}
> > +
> > +	lvds->companion = of_drm_find_bridge(companion);
> > +	if (!lvds->companion) {
> > +		ret = -EPROBE_DEFER;
> > +		goto done;
> > +	}
> > +
> > +	dev_dbg(dev, "Found companion encoder %pOF\n", companion);
> > +
> > +done:
> > +	of_node_put(companion);
> > +
> > +	return ret;
> > +}
> > +
> >  static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> >  {
> >  	struct device_node *local_output = NULL;
> > @@ -682,14 +748,26 @@ static int rcar_lvds_parse_dt(struct rcar_lvds *lvds)
> >
> >  	if (is_bridge) {
> >  		lvds->next_bridge = of_drm_find_bridge(remote);
> > -		if (!lvds->next_bridge)
> > +		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))
> > +		if (IS_ERR(lvds->panel)) {
> >  			ret = PTR_ERR(lvds->panel);
> > +			goto done;
> > +		}
> >  	}
> >
> > +	if (lvds->dual_link)
> > +		ret = rcar_lvds_parse_dt_companion(lvds);
> 
> Looking at the error path here below, for E3/D3, -ENODEV gets
> sanitized to return 0 as we want this method to return success even if
> no endpoint is there, when using the LVDS encoder provided clock to
> back-feed the DU.
> 
> This is not the case for the companion property imho. If the property is
> specified, it should be sane and -ENODEV is worth being propagated to
> the caller.

But in that case the LVDS encoder will fail to probe, and so the DU
channel will not be able to output on the DPAD. Isn't it worth
supporting that ? Otherwise I can simply make
rcar_lvds_parse_dt_companion() return -EINVAL or -ENXIO and it should be
fine with the existing error handling path.

> You could move the error handling bits in the error path:
> 
> 	/*
> 	 * On D3/E3 the LVDS encoder provides a clock to the DU, which can be
> 	 * used for the DPAD output even when the LVDS output is not connected.
> 	 * Don't fail probe in that case as the DU will need the bridge to
> 	 * control the clock.
> 	 */
> 	if (lvds->info->quirks & RCAR_LVDS_QUIRK_EXT_PLL)
> 		return ret == -ENODEV ? 0 : ret;
> 
> before the of_node_put() sequences, and add one more label, to skip
> the above part in case parse_dt_companion() fails.
> 
> Apart from this you can retain my tag if you like.
> Thanks
>    j
> 
> > +
> >  done:
> >  	of_node_put(local_output);
> >  	of_node_put(remote_input);
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.h b/drivers/gpu/drm/rcar-du/rcar_lvds.h
> > index a709cae1bc32..222ec0e60785 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.h
> > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.h
> > @@ -15,6 +15,7 @@ struct drm_bridge;
> >  #if IS_ENABLED(CONFIG_DRM_RCAR_LVDS)
> >  int rcar_lvds_clk_enable(struct drm_bridge *bridge, unsigned long freq);
> >  void rcar_lvds_clk_disable(struct drm_bridge *bridge);
> > +bool rcar_lvds_dual_link(struct drm_bridge *bridge);
> >  #else
> >  static inline int rcar_lvds_clk_enable(struct drm_bridge *bridge,
> >  				       unsigned long freq)
> > @@ -22,6 +23,10 @@ static inline int rcar_lvds_clk_enable(struct drm_bridge *bridge,
> >  	return -ENOSYS;
> >  }
> >  static inline void rcar_lvds_clk_disable(struct drm_bridge *bridge) { }
> > +static inline bool rcar_lvds_dual_link(struct drm_bridge *bridge)
> > +{
> > +	return false;
> > +}
> >  #endif /* CONFIG_DRM_RCAR_LVDS */
> >
> >  #endif /* __RCAR_LVDS_H__ */

-- 
Regards,

Laurent Pinchart