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
