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+rene...@ideasonboard.com>
> > > ---
> > >  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

Reply via email to