| Message ID | 1575649974-31472-5-git-send-email-fabrizio.castro@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | Add dual-LVDS panel support to EK874 | expand |
Hi Fabrizio, Thank you for the patch. On Fri, Dec 06, 2019 at 04:32:51PM +0000, Fabrizio Castro wrote: > DT properties dual-lvds-even-pixels and dual-lvds-odd-pixels > can be used to work out if the driver needs to swap even > and odd pixels around. > > This patch makes use of the return value from function > drm_of_lvds_get_dual_link_pixel_order to determine if we > need to swap odd and even pixels around for things to work > properly. > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > --- > v3->v4: > * New patch extracted from patch: > "drm: rcar-du: lvds: Add dual-LVDS panels support" > --- > drivers/gpu/drm/rcar-du/rcar_lvds.c | 46 +++++++++++++++++++++++++++++-------- > 1 file changed, 37 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c > index 6c1f171..cb2147c 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c > @@ -71,6 +71,7 @@ struct rcar_lvds { > > struct drm_bridge *companion; > bool dual_link; > + bool stripe_swap_data; Should we merge those two fields in an int dual_link that would be set to DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS, DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS or a negative value (or maybe we the value of enum drm_lvds_dual_link_pixels could be modified so that 0 could mean single link) ? > }; > > #define bridge_to_rcar_lvds(b) \ > @@ -441,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 Maybe "ST_SWAP is reserved for the companion encoder, only set it in the primary encoder." ? > + */ > + lvdstripe = LVDSTRIPE_ST_ON | > + (lvds->companion && lvds->stripe_swap_data ? > + LVDSTRIPE_ST_SWAP : 0); To match the coding style of the rest of the driver, lvdstripe = LVDSTRIPE_ST_ON | (lvds->companion && lvds->stripe_swap_data ? LVDSTRIPE_ST_SWAP : 0); Even though not strictly required, could you surround this statement with { } as it spans quite a few lines with the comment ? > + rcar_lvds_write(lvds, LVDSTRIPE, lvdstripe); > } > > /* > @@ -702,17 +711,33 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds) > dual_link = drm_of_lvds_get_dual_link_pixel_order(p0, p1); > of_node_put(p0); > of_node_put(p1); > - if (dual_link >= DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) { > + > + switch (dual_link) { > + case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS: > + /* > + * By default we generate even pixels from this encoder and odd > + * pixels from the companion encoder, but since p0 is connected > + * to the port expecting ood pixels, and p1 is connected to the > + * port expecting even pixels, we need to swap even and odd > + * pixels around. > + */ > + lvds->stripe_swap_data = true; > + lvds->dual_link = true; > + break; > + case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS: > lvds->dual_link = true; > - } else if (lvds->next_bridge && lvds->next_bridge->timings) { > + break; > + default: > /* > * Early dual-link bridge specific implementations populate the > * timings field of drm_bridge, read the dual_link flag off the > * bridge directly for backward compatibility. > */ > - lvds->dual_link = lvds->next_bridge->timings->dual_link; > + if (lvds->next_bridge && lvds->next_bridge->timings) > + lvds->dual_link = lvds->next_bridge->timings->dual_link; > } > > + A single blank line is enough. > if (!lvds->dual_link) { > dev_dbg(dev, "Single-link configuration detected\n"); > goto done; > @@ -728,6 +753,9 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds) > "Dual-link configuration detected (companion encoder %pOF)\n", > companion); > > + if (lvds->stripe_swap_data) > + dev_dbg(dev, "Data swapping required\n"); > + > companion_lvds = bridge_to_rcar_lvds(lvds->companion); > > /*
Hi Laurent, Thank you for your feedback! > From: devicetree-owner@vger.kernel.org <devicetree-owner@vger.kernel.org> On Behalf Of Laurent Pinchart > Sent: 13 December 2019 21:41 > Subject: Re: [PATCH v4 4/7] drm: rcar-du: lvds: Allow for even and odd pixels swap > > Hi Fabrizio, > > Thank you for the patch. > > On Fri, Dec 06, 2019 at 04:32:51PM +0000, Fabrizio Castro wrote: > > DT properties dual-lvds-even-pixels and dual-lvds-odd-pixels > > can be used to work out if the driver needs to swap even > > and odd pixels around. > > > > This patch makes use of the return value from function > > drm_of_lvds_get_dual_link_pixel_order to determine if we > > need to swap odd and even pixels around for things to work > > properly. > > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > > > --- > > v3->v4: > > * New patch extracted from patch: > > "drm: rcar-du: lvds: Add dual-LVDS panels support" > > --- > > drivers/gpu/drm/rcar-du/rcar_lvds.c | 46 +++++++++++++++++++++++++++++-------- > > 1 file changed, 37 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c > > index 6c1f171..cb2147c 100644 > > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c > > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c > > @@ -71,6 +71,7 @@ struct rcar_lvds { > > > > struct drm_bridge *companion; > > bool dual_link; > > + bool stripe_swap_data; > > Should we merge those two fields in an int dual_link that would be set > to DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS, > DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS or a negative value (or maybe we the > value of enum drm_lvds_dual_link_pixels could be modified so that 0 > could mean single link) ? I see your point, and I think there is a third option, using definitions local to the RCAR LVDS driver (via an enum?). The reason for thinking about a third option is that option 1 could be a bit error prone, as negative values usually have special meaning. I like option 2, I find it neat, but if I did that then we would need to agree again on names, definitions, interfaces, etc., as the meaning of things will change slightly. Also, we will probably be fine with the helper and definitions we already agreed on. I think option 3 will offer a little bit of decoupling between the helper and the driver, and should have a limited effect on previous uses of dual_link. I'll make option 3 related changes in v5, if you don't like them then I think we should try option 2. > > > }; > > > > #define bridge_to_rcar_lvds(b) \ > > @@ -441,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 > > Maybe "ST_SWAP is reserved for the companion encoder, only set it in the > primary encoder." ? sure > > > + */ > > + lvdstripe = LVDSTRIPE_ST_ON | > > + (lvds->companion && lvds->stripe_swap_data ? > > + LVDSTRIPE_ST_SWAP : 0); > > To match the coding style of the rest of the driver, ok > > lvdstripe = LVDSTRIPE_ST_ON > | (lvds->companion && lvds->stripe_swap_data ? > LVDSTRIPE_ST_SWAP : 0); > > Even though not strictly required, could you surround this statement > with { } as it spans quite a few lines with the comment ? Will rework this slightly anyway to make room for option 3. > > > + rcar_lvds_write(lvds, LVDSTRIPE, lvdstripe); > > } > > > > /* > > @@ -702,17 +711,33 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds) > > dual_link = drm_of_lvds_get_dual_link_pixel_order(p0, p1); > > of_node_put(p0); > > of_node_put(p1); > > - if (dual_link >= DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) { > > + > > + switch (dual_link) { > > + case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS: > > + /* > > + * By default we generate even pixels from this encoder and odd > > + * pixels from the companion encoder, but since p0 is connected > > + * to the port expecting ood pixels, and p1 is connected to the > > + * port expecting even pixels, we need to swap even and odd > > + * pixels around. > > + */ > > + lvds->stripe_swap_data = true; > > + lvds->dual_link = true; > > + break; > > + case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS: > > lvds->dual_link = true; > > - } else if (lvds->next_bridge && lvds->next_bridge->timings) { > > + break; > > + default: > > /* > > * Early dual-link bridge specific implementations populate the > > * timings field of drm_bridge, read the dual_link flag off the > > * bridge directly for backward compatibility. > > */ > > - lvds->dual_link = lvds->next_bridge->timings->dual_link; > > + if (lvds->next_bridge && lvds->next_bridge->timings) > > + lvds->dual_link = lvds->next_bridge->timings->dual_link; > > } > > > > + > > A single blank line is enough. Oops Thanks, Fab > > > if (!lvds->dual_link) { > > dev_dbg(dev, "Single-link configuration detected\n"); > > goto done; > > @@ -728,6 +753,9 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds) > > "Dual-link configuration detected (companion encoder %pOF)\n", > > companion); > > > > + if (lvds->stripe_swap_data) > > + dev_dbg(dev, "Data swapping required\n"); > > + > > companion_lvds = bridge_to_rcar_lvds(lvds->companion); > > > > /* > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c index 6c1f171..cb2147c 100644 --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c @@ -71,6 +71,7 @@ struct rcar_lvds { struct drm_bridge *companion; bool dual_link; + bool stripe_swap_data; }; #define bridge_to_rcar_lvds(b) \ @@ -441,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); } /* @@ -702,17 +711,33 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds) dual_link = drm_of_lvds_get_dual_link_pixel_order(p0, p1); of_node_put(p0); of_node_put(p1); - if (dual_link >= DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) { + + switch (dual_link) { + case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS: + /* + * By default we generate even pixels from this encoder and odd + * pixels from the companion encoder, but since p0 is connected + * to the port expecting ood pixels, and p1 is connected to the + * port expecting even pixels, we need to swap even and odd + * pixels around. + */ + lvds->stripe_swap_data = true; + lvds->dual_link = true; + break; + case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS: lvds->dual_link = true; - } else if (lvds->next_bridge && lvds->next_bridge->timings) { + break; + default: /* * Early dual-link bridge specific implementations populate the * timings field of drm_bridge, read the dual_link flag off the * bridge directly for backward compatibility. */ - lvds->dual_link = lvds->next_bridge->timings->dual_link; + if (lvds->next_bridge && lvds->next_bridge->timings) + lvds->dual_link = lvds->next_bridge->timings->dual_link; } + if (!lvds->dual_link) { dev_dbg(dev, "Single-link configuration detected\n"); goto done; @@ -728,6 +753,9 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds) "Dual-link configuration detected (companion encoder %pOF)\n", companion); + if (lvds->stripe_swap_data) + dev_dbg(dev, "Data swapping required\n"); + companion_lvds = bridge_to_rcar_lvds(lvds->companion); /*
DT properties dual-lvds-even-pixels and dual-lvds-odd-pixels can be used to work out if the driver needs to swap even and odd pixels around. This patch makes use of the return value from function drm_of_lvds_get_dual_link_pixel_order to determine if we need to swap odd and even pixels around for things to work properly. Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> --- v3->v4: * New patch extracted from patch: "drm: rcar-du: lvds: Add dual-LVDS panels support" --- drivers/gpu/drm/rcar-du/rcar_lvds.c | 46 +++++++++++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 9 deletions(-)