Re: [PATCH/RFC 11/15] drm: rcar-du: lvds: Add support for dual-link mode

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

 



Hi Laurent,

On Fri, Mar 08, 2019 at 08:12:39PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Fri, Mar 08, 2019 at 06:20:23PM +0100, Jacopo Mondi wrote:
> > On Thu, Mar 07, 2019 at 01:23:41AM +0200, 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@xxxxxxxxxxxxxxxx>
> > > ---
> > >  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 5ac92ee15be0..90d33514bb3e 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;
> >
> > I wonder if, in all this patch, you could not use "lvds->companion" in
> > place of "lvds->dual_link", and thus drop that from timings. This mean
> > "renesas,companion" would only be used when operating in dual link
> > mode, which might be less nice (and could actually called differently
> > in that case).
> >
> > Both the THC631024 and the rcar_du-lvds driver would decide
> > independently if they operate in dual link mode or not (one counting
> > its endpoints, the other inspecting the "renesas,companion" property).
>
> I decided to specify the companion in DT regardless of which operating
> mode is used, as it's a property of the LVDS encoder, not of the
> operating mode. Furthermore, I can foresee setups where the mode would
> be selected dynamically at runtime. Our development boards don't allow
> that as they hardcode the mode using DIP switches, but nothing would
> prevent the mode selection signals to be connected to GPIOs. I thus
> think lvds->companion should point to the companion unconditionally, and
> lvds->dual_link should select the operating mode. The dual_link field is
> currently set at probe time as I have no need for dynamic configuration
> of the mode and no mean of testing it, so I decided not to implement
> dynamic switching yet.
>

Fine, I tried thinking up a bit if the "renesas,companion" property
could have been made an endpoint property, to be specified in both
the rcar-lvds endpoints and in the DU endpoints, so that the
rcar_du_encoder does not need to pick into the lvds-encoder to know
if it is operating in dual link mode or not and skip creation of the
encoder associated to LVDS1 in such a case.

Furthermore, we could make a "vstripe-even" "vstripe-odd" endpoint
properties, that would allow you to control the ST_SWAP field of
LVDSTRIPE register, and create another endpoint property that contains
the phandle to the companion, like "dual-link-companion" or
"dual-link-slave". Those properties would need to be specified in both
DU and LVDS endpoints though, which might be clunky, but that would
possibly save a few cross-driver calls.

Anyway, just putting a few more options on the table, if you have
already considered those feel free to stick to this implementation...

> > The only place where this might be tricky is the here above
> >
> >  +	/*
> >  +	 * 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);
> >
> > but that would remove the need for the thc63 bridge to report its
> > operating mode in timings...
> >
> > >  	} 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)
> >
> > Note: if (!is_bridge) you would never set lvds->dual_link, so this
> > should be moved inside the here above "if (is_bridge)"
>
> I don't yet, but nothing prevents a panel from operating in dual mode,
> even if not implemented yet. That's why I've move this check out of the
> bridge/panel conditional code.

I see, still right now is of no use. It's fine though, really minor
stuff.

Thanks
   j

>
> > > +		ret = rcar_lvds_parse_dt_companion(lvds);
> > > +
> > >  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

Attachment: signature.asc
Description: PGP signature


[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