| Message ID | 1576527154-18391-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 Mon, Dec 16, 2019 at 08:12:32PM +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> > > --- > v4->v5: > * Addressed comments from Laurent's review > > v3->v4: > * New patch extracted from patch: > "drm: rcar-du: lvds: Add dual-LVDS panels support" > --- > drivers/gpu/drm/rcar-du/rcar_lvds.c | 67 +++++++++++++++++++++++++++++-------- > 1 file changed, 53 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c > index 3eb208e..c6a38c3 100644 > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c > @@ -37,6 +37,12 @@ enum rcar_lvds_mode { > RCAR_LVDS_MODE_VESA = 4, > }; > > +enum rcar_lvds_link_type { > + RCAR_LVDS_SINGLE_LINK = 0, > + RCAR_LVDS_DUAL_LINK_EVEN_ODD_PIXELS = 1, > + RCAR_LVDS_DUAL_LINK_ODD_EVEN_PIXELS = 2, > +}; > + > #define RCAR_LVDS_QUIRK_LANES BIT(0) /* LVDS lanes 1 and 3 inverted */ > #define RCAR_LVDS_QUIRK_GEN3_LVEN BIT(1) /* LVEN bit needs to be set on R8A77970/R8A7799x */ > #define RCAR_LVDS_QUIRK_PWD BIT(2) /* PWD bit available (all of Gen3 but E3) */ > @@ -67,7 +73,7 @@ struct rcar_lvds { > } clocks; > > struct drm_bridge *companion; > - bool dual_link; > + enum rcar_lvds_link_type dual_link; Do you think we should rename this to link_type (and test for lvds->link_type != RCAR_LVDS_SINGLE_LINK) instead of lvds->dual_link) ? Apart from that, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > }; > > #define bridge_to_rcar_lvds(b) \ > @@ -484,12 +490,31 @@ static void rcar_lvds_atomic_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) { > + /* > + * By default we generate even pixels from the primary > + * encoder and odd pixels from the companion encoder. > + * Swap pixels around if the sink requires odd pixels > + * from the primary encoder and even pixels from the > + * companion encoder. > + */ > + bool swap_pixels = lvds->dual_link == > + RCAR_LVDS_DUAL_LINK_ODD_EVEN_PIXELS; > + > + /* > + * Configure vertical stripe since we are dealing with > + * an LVDS dual-link connection. > + * > + * ST_SWAP is reserved for the companion encoder, only > + * set it in the primary encoder. > + */ > + lvdstripe = LVDSTRIPE_ST_ON > + | (lvds->companion && swap_pixels ? > + LVDSTRIPE_ST_SWAP : 0); > + } > + rcar_lvds_write(lvds, LVDSTRIPE, lvdstripe); > } > > /* > @@ -716,15 +741,26 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds) > of_node_put(port0); > of_node_put(port1); > > - if (dual_link >= DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) > - lvds->dual_link = true; > - else if (lvds->next_bridge && lvds->next_bridge->timings) > + switch (dual_link) { > + case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS: > + lvds->dual_link = RCAR_LVDS_DUAL_LINK_ODD_EVEN_PIXELS; > + break; > + case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS: > + lvds->dual_link = RCAR_LVDS_DUAL_LINK_EVEN_ODD_PIXELS; > + 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. > + * timings field of drm_bridge. If the flag is set, we assume > + * that we are expected to generate even pixels from the primary > + * encoder, and odd pixels from the companion encoder. > */ > - lvds->dual_link = lvds->next_bridge->timings->dual_link; > + if (lvds->next_bridge && lvds->next_bridge->timings && > + lvds->next_bridge->timings->dual_link) > + lvds->dual_link = RCAR_LVDS_DUAL_LINK_EVEN_ODD_PIXELS; > + else > + lvds->dual_link = RCAR_LVDS_SINGLE_LINK; > + } > > if (!lvds->dual_link) { > dev_dbg(dev, "Single-link configuration detected\n"); > @@ -741,6 +777,9 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds) > "Dual-link configuration detected (companion encoder %pOF)\n", > companion); > > + if (lvds->dual_link == RCAR_LVDS_DUAL_LINK_ODD_EVEN_PIXELS) > + dev_dbg(dev, "Data swapping required\n"); > + > /* > * FIXME: We should not be messing with the companion encoder private > * data from the primary encoder, we should rather let the companion > @@ -751,7 +790,7 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds) > * for the time being. > */ > companion_lvds = bridge_to_rcar_lvds(lvds->companion); > - companion_lvds->dual_link = true; > + companion_lvds->dual_link = lvds->dual_link; > > done: > of_node_put(companion);
Hi Laurent, Thank you for your feedback! > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Sent: 16 December 2019 21:35 > Subject: Re: [PATCH v5 4/6] drm: rcar-du: lvds: Allow for even and odd pixels swap > > Hi Fabrizio, > > Thank you for the patch. > > On Mon, Dec 16, 2019 at 08:12:32PM +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> > > > > --- > > v4->v5: > > * Addressed comments from Laurent's review > > > > v3->v4: > > * New patch extracted from patch: > > "drm: rcar-du: lvds: Add dual-LVDS panels support" > > --- > > drivers/gpu/drm/rcar-du/rcar_lvds.c | 67 +++++++++++++++++++++++++++++-------- > > 1 file changed, 53 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c > > index 3eb208e..c6a38c3 100644 > > --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c > > +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c > > @@ -37,6 +37,12 @@ enum rcar_lvds_mode { > > RCAR_LVDS_MODE_VESA = 4, > > }; > > > > +enum rcar_lvds_link_type { > > + RCAR_LVDS_SINGLE_LINK = 0, > > + RCAR_LVDS_DUAL_LINK_EVEN_ODD_PIXELS = 1, > > + RCAR_LVDS_DUAL_LINK_ODD_EVEN_PIXELS = 2, > > +}; > > + > > #define RCAR_LVDS_QUIRK_LANES BIT(0) /* LVDS lanes 1 and 3 inverted */ > > #define RCAR_LVDS_QUIRK_GEN3_LVEN BIT(1) /* LVEN bit needs to be set on R8A77970/R8A7799x */ > > #define RCAR_LVDS_QUIRK_PWD BIT(2) /* PWD bit available (all of Gen3 but E3) */ > > @@ -67,7 +73,7 @@ struct rcar_lvds { > > } clocks; > > > > struct drm_bridge *companion; > > - bool dual_link; > > + enum rcar_lvds_link_type dual_link; > > Do you think we should rename this to link_type (and test for > lvds->link_type != RCAR_LVDS_SINGLE_LINK) instead of lvds->dual_link) ? > Apart from that, I'll send v6 to address that. I will also rebase on top of: https://patchwork.kernel.org/patch/11295991/ and will use that as a dependency for v6 Cheers, Fab > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > }; > > > > #define bridge_to_rcar_lvds(b) \ > > @@ -484,12 +490,31 @@ static void rcar_lvds_atomic_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) { > > + /* > > + * By default we generate even pixels from the primary > > + * encoder and odd pixels from the companion encoder. > > + * Swap pixels around if the sink requires odd pixels > > + * from the primary encoder and even pixels from the > > + * companion encoder. > > + */ > > + bool swap_pixels = lvds->dual_link == > > + RCAR_LVDS_DUAL_LINK_ODD_EVEN_PIXELS; > > + > > + /* > > + * Configure vertical stripe since we are dealing with > > + * an LVDS dual-link connection. > > + * > > + * ST_SWAP is reserved for the companion encoder, only > > + * set it in the primary encoder. > > + */ > > + lvdstripe = LVDSTRIPE_ST_ON > > + | (lvds->companion && swap_pixels ? > > + LVDSTRIPE_ST_SWAP : 0); > > + } > > + rcar_lvds_write(lvds, LVDSTRIPE, lvdstripe); > > } > > > > /* > > @@ -716,15 +741,26 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds) > > of_node_put(port0); > > of_node_put(port1); > > > > - if (dual_link >= DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) > > - lvds->dual_link = true; > > - else if (lvds->next_bridge && lvds->next_bridge->timings) > > + switch (dual_link) { > > + case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS: > > + lvds->dual_link = RCAR_LVDS_DUAL_LINK_ODD_EVEN_PIXELS; > > + break; > > + case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS: > > + lvds->dual_link = RCAR_LVDS_DUAL_LINK_EVEN_ODD_PIXELS; > > + 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. > > + * timings field of drm_bridge. If the flag is set, we assume > > + * that we are expected to generate even pixels from the primary > > + * encoder, and odd pixels from the companion encoder. > > */ > > - lvds->dual_link = lvds->next_bridge->timings->dual_link; > > + if (lvds->next_bridge && lvds->next_bridge->timings && > > + lvds->next_bridge->timings->dual_link) > > + lvds->dual_link = RCAR_LVDS_DUAL_LINK_EVEN_ODD_PIXELS; > > + else > > + lvds->dual_link = RCAR_LVDS_SINGLE_LINK; > > + } > > > > if (!lvds->dual_link) { > > dev_dbg(dev, "Single-link configuration detected\n"); > > @@ -741,6 +777,9 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds) > > "Dual-link configuration detected (companion encoder %pOF)\n", > > companion); > > > > + if (lvds->dual_link == RCAR_LVDS_DUAL_LINK_ODD_EVEN_PIXELS) > > + dev_dbg(dev, "Data swapping required\n"); > > + > > /* > > * FIXME: We should not be messing with the companion encoder private > > * data from the primary encoder, we should rather let the companion > > @@ -751,7 +790,7 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds) > > * for the time being. > > */ > > companion_lvds = bridge_to_rcar_lvds(lvds->companion); > > - companion_lvds->dual_link = true; > > + companion_lvds->dual_link = lvds->dual_link; > > > > done: > > of_node_put(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 3eb208e..c6a38c3 100644 --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c @@ -37,6 +37,12 @@ enum rcar_lvds_mode { RCAR_LVDS_MODE_VESA = 4, }; +enum rcar_lvds_link_type { + RCAR_LVDS_SINGLE_LINK = 0, + RCAR_LVDS_DUAL_LINK_EVEN_ODD_PIXELS = 1, + RCAR_LVDS_DUAL_LINK_ODD_EVEN_PIXELS = 2, +}; + #define RCAR_LVDS_QUIRK_LANES BIT(0) /* LVDS lanes 1 and 3 inverted */ #define RCAR_LVDS_QUIRK_GEN3_LVEN BIT(1) /* LVEN bit needs to be set on R8A77970/R8A7799x */ #define RCAR_LVDS_QUIRK_PWD BIT(2) /* PWD bit available (all of Gen3 but E3) */ @@ -67,7 +73,7 @@ struct rcar_lvds { } clocks; struct drm_bridge *companion; - bool dual_link; + enum rcar_lvds_link_type dual_link; }; #define bridge_to_rcar_lvds(b) \ @@ -484,12 +490,31 @@ static void rcar_lvds_atomic_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) { + /* + * By default we generate even pixels from the primary + * encoder and odd pixels from the companion encoder. + * Swap pixels around if the sink requires odd pixels + * from the primary encoder and even pixels from the + * companion encoder. + */ + bool swap_pixels = lvds->dual_link == + RCAR_LVDS_DUAL_LINK_ODD_EVEN_PIXELS; + + /* + * Configure vertical stripe since we are dealing with + * an LVDS dual-link connection. + * + * ST_SWAP is reserved for the companion encoder, only + * set it in the primary encoder. + */ + lvdstripe = LVDSTRIPE_ST_ON + | (lvds->companion && swap_pixels ? + LVDSTRIPE_ST_SWAP : 0); + } + rcar_lvds_write(lvds, LVDSTRIPE, lvdstripe); } /* @@ -716,15 +741,26 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds) of_node_put(port0); of_node_put(port1); - if (dual_link >= DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS) - lvds->dual_link = true; - else if (lvds->next_bridge && lvds->next_bridge->timings) + switch (dual_link) { + case DRM_LVDS_DUAL_LINK_ODD_EVEN_PIXELS: + lvds->dual_link = RCAR_LVDS_DUAL_LINK_ODD_EVEN_PIXELS; + break; + case DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS: + lvds->dual_link = RCAR_LVDS_DUAL_LINK_EVEN_ODD_PIXELS; + 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. + * timings field of drm_bridge. If the flag is set, we assume + * that we are expected to generate even pixels from the primary + * encoder, and odd pixels from the companion encoder. */ - lvds->dual_link = lvds->next_bridge->timings->dual_link; + if (lvds->next_bridge && lvds->next_bridge->timings && + lvds->next_bridge->timings->dual_link) + lvds->dual_link = RCAR_LVDS_DUAL_LINK_EVEN_ODD_PIXELS; + else + lvds->dual_link = RCAR_LVDS_SINGLE_LINK; + } if (!lvds->dual_link) { dev_dbg(dev, "Single-link configuration detected\n"); @@ -741,6 +777,9 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds) "Dual-link configuration detected (companion encoder %pOF)\n", companion); + if (lvds->dual_link == RCAR_LVDS_DUAL_LINK_ODD_EVEN_PIXELS) + dev_dbg(dev, "Data swapping required\n"); + /* * FIXME: We should not be messing with the companion encoder private * data from the primary encoder, we should rather let the companion @@ -751,7 +790,7 @@ static int rcar_lvds_parse_dt_companion(struct rcar_lvds *lvds) * for the time being. */ companion_lvds = bridge_to_rcar_lvds(lvds->companion); - companion_lvds->dual_link = true; + companion_lvds->dual_link = lvds->dual_link; done: of_node_put(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> --- v4->v5: * Addressed comments from Laurent's review v3->v4: * New patch extracted from patch: "drm: rcar-du: lvds: Add dual-LVDS panels support" --- drivers/gpu/drm/rcar-du/rcar_lvds.c | 67 +++++++++++++++++++++++++++++-------- 1 file changed, 53 insertions(+), 14 deletions(-)