RE: [PATCH v4 4/7] drm: rcar-du: lvds: Allow for even and odd pixels swap
Hi Laurent,
Thank you for your feedback!
> From: devicetree-ow...@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 +, 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
> >
> > ---
> > 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);
> &g
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 +, 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
>
> ---
> 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)
[PATCH v4 4/7] drm: rcar-du: lvds: Allow for even and odd pixels swap
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
---
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;
};
#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);
/*
--
2.7.4
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
