[RFC PATCH 0/2] drm: add support for for clk and de polarity & gpu: ipu-v3: use clock and de polarity

[RFC PATCH 0/2] drm: add support for for clk and de polarity & gpu: ipu-v3: use clock and de polarity

[Manfred Schlaegl]

Hello!

These patches address a problem we ran into using parallel displays
with Freescale i.MX53 and i.MX6 SoC's.
In short: We wanted to change the clock signal polarity by using
display-timing in the devicetree description, but the output signal stayed
unchanged.

Parallel displays may have different polarities for clock and data enable
signals. (Clock polarity is also a topic on LVDS displays). This is the
reason why there are some properties in devicetree display-timing [1]
called
 * pixelclk-active .. pixel clock polarity
 * de-active .. data enable pulse polarity

This properties are correctly represented in struct display_timings [2]
and struct videomode [3] by using enum display_flags [3].
But when it comes to struct drm_display_mode [4] there are no
representations for this.
The properties are "lost in conversion" and never reach imx drm. (or 
other drm drivers).

Changing this would be especially important for embedded devices where
parallel(RGB) and LVDS displays are still widely used and drm already
plays an important role.

Following two patches will
 1. Introduce representation of clock and data enable polarities
     in struct drm_display_mode analog to hsync/vsync signals.
   * "drm: add support for for clk and de polarity"
 2. Implicitly enable usage of the newly introduced flags in 
     imx drm (with respect to devicetree compatibility.)
   * "gpu: ipu-v3: use clock and de polarity from videomode"

I'm aware that introducing new flags in struct drm_display_mode affects
user space too and therefore has to be considered carefully.
As I'm not really deep in drm user space your suggestions are very
welcome.

best regards,
manfred


[1] Documentation/devicetree/bindings/video/display-timing.txt
[2] include/video/display_timing.h
[3] include/video/videomode.h
[4] include/drm/drm_modes.h

[RFC PATCH 1/2] drm: add support for for clk and de polarity

[Manfred Schlaegl]

To get full support for parallel and LVDS displays with drm:
Add representation for clock and data enable polarity in drm_display_mode
flags (similar to HSYNC/VSYNC polarity) and update conversion functions
from/to videomode accordingly.

This is especially important for embedded devices where parallel(RGB) and
LVDS displays are still widely used and drm already plays an important
role.

Tested on Freescale i.MX53(parallel) and i.MX6(LVDS).

Background:
There was the ability to set polarity of clock and data enable signals
in devicetree(display-timing), struct display_timing and struct videomode,
but there was no representation for this in struct drm_display_mode.
Example on Freescale i.MX53/i.MX6 SoC's:
 * A parallel display using different clock polarity is set up using
   display-timing in devicetree
 * ipuv3 parallel outputs clock with wrong polarity

Signed-off-by: Manfred Schlaegl <manfred.schlaegl-RbZlAiThDcE@public.gmane.org>
---
 drivers/gpu/drm/drm_modes.c | 16 ++++++++++++++++
 include/uapi/drm/drm_mode.h |  5 +++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index cd74a09..dbb28b7 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -605,6 +605,14 @@ void drm_display_mode_from_videomode(const struct videomode *vm,
 		dmode->flags |= DRM_MODE_FLAG_PVSYNC;
 	else if (vm->flags & DISPLAY_FLAGS_VSYNC_LOW)
 		dmode->flags |= DRM_MODE_FLAG_NVSYNC;
+	if (vm->flags & DISPLAY_FLAGS_DE_HIGH)
+		dmode->flags |= DRM_MODE_FLAG_PDE;
+	else if (vm->flags & DISPLAY_FLAGS_DE_LOW)
+		dmode->flags |= DRM_MODE_FLAG_NDE;
+	if (vm->flags & DISPLAY_FLAGS_PIXDATA_POSEDGE)
+		dmode->flags |= DRM_MODE_FLAG_PPIXDATA;
+	else if (vm->flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE)
+		dmode->flags |= DRM_MODE_FLAG_NPIXDATA;
 	if (vm->flags & DISPLAY_FLAGS_INTERLACED)
 		dmode->flags |= DRM_MODE_FLAG_INTERLACE;
 	if (vm->flags & DISPLAY_FLAGS_DOUBLESCAN)
@@ -646,6 +654,14 @@ void drm_display_mode_to_videomode(const struct drm_display_mode *dmode,
 		vm->flags |= DISPLAY_FLAGS_VSYNC_HIGH;
 	else if (dmode->flags & DRM_MODE_FLAG_NVSYNC)
 		vm->flags |= DISPLAY_FLAGS_VSYNC_LOW;
+	if (dmode->flags & DRM_MODE_FLAG_PDE)
+		vm->flags |= DISPLAY_FLAGS_DE_HIGH;
+	else if (dmode->flags & DRM_MODE_FLAG_NDE)
+		vm->flags |= DISPLAY_FLAGS_DE_LOW;
+	if (dmode->flags & DRM_MODE_FLAG_PPIXDATA)
+		vm->flags |= DISPLAY_FLAGS_PIXDATA_POSEDGE;
+	else if (dmode->flags & DRM_MODE_FLAG_NPIXDATA)
+		vm->flags |= DISPLAY_FLAGS_PIXDATA_NEGEDGE;
 	if (dmode->flags & DRM_MODE_FLAG_INTERLACE)
 		vm->flags |= DISPLAY_FLAGS_INTERLACED;
 	if (dmode->flags & DRM_MODE_FLAG_DBLSCAN)
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 359107a..cb4912b 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -72,6 +72,11 @@
 #define  DRM_MODE_FLAG_3D_L_DEPTH_GFX_GFX_DEPTH	(6<<14)
 #define  DRM_MODE_FLAG_3D_TOP_AND_BOTTOM	(7<<14)
 #define  DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF	(8<<14)
+/* flags for display data enable and clock polarity */
+#define DRM_MODE_FLAG_PDE			(1<<19)
+#define DRM_MODE_FLAG_NDE			(1<<20)
+#define DRM_MODE_FLAG_PPIXDATA			(1<<21)
+#define DRM_MODE_FLAG_NPIXDATA			(1<<22)
 
 
 /* DPMS flags */
-- 
2.1.4

Re: [RFC PATCH 1/2] drm: add support for for clk and de polarity

[Philipp Zabel]

Am Mittwoch, den 15.07.2015, 17:50 +0200 schrieb Manfred Schlaegl:
> To get full support for parallel and LVDS displays with drm:
> Add representation for clock and data enable polarity in drm_display_mode
> flags (similar to HSYNC/VSYNC polarity) and update conversion functions
> from/to videomode accordingly.
> 
> This is especially important for embedded devices where parallel(RGB) and
> LVDS displays are still widely used and drm already plays an important
> role.
> 
> Tested on Freescale i.MX53(parallel) and i.MX6(LVDS).
> 
> Background:
> There was the ability to set polarity of clock and data enable signals
> in devicetree(display-timing), struct display_timing and struct videomode,
> but there was no representation for this in struct drm_display_mode.
> Example on Freescale i.MX53/i.MX6 SoC's:
>  * A parallel display using different clock polarity is set up using
>    display-timing in devicetree
>  * ipuv3 parallel outputs clock with wrong polarity
> 
> Signed-off-by: Manfred Schlaegl <manfred.schlaegl@gmx.at>

Any comments on whether data enable and pixel clock polarity flags can
be added to the visible DRM_MODE_FLAGs, and if not, where else this
information should be kept? struct drm_display_info?

This patch and the following IPUv3 patch are useful and necessary for
quite some panels connected to i.MX SoCs, but adding DRM_MODE_FLAGs is
somewhat out of my jurisdiction.

best regards
Philipp

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 1/2] drm: add support for for clk and de polarity

[Manfred Schlaegl]

On 2015-11-25 18:22, Philipp Zabel wrote:
> Am Mittwoch, den 15.07.2015, 17:50 +0200 schrieb Manfred Schlaegl:
>> To get full support for parallel and LVDS displays with drm:
>> Add representation for clock and data enable polarity in drm_display_mode
>> flags (similar to HSYNC/VSYNC polarity) and update conversion functions
>> from/to videomode accordingly.
>>
>> This is especially important for embedded devices where parallel(RGB) and
>> LVDS displays are still widely used and drm already plays an important
>> role.
>>
>> Tested on Freescale i.MX53(parallel) and i.MX6(LVDS).
>>
>> Background:
>> There was the ability to set polarity of clock and data enable signals
>> in devicetree(display-timing), struct display_timing and struct videomode,
>> but there was no representation for this in struct drm_display_mode.
>> Example on Freescale i.MX53/i.MX6 SoC's:
>>  * A parallel display using different clock polarity is set up using
>>    display-timing in devicetree
>>  * ipuv3 parallel outputs clock with wrong polarity
>>
>> Signed-off-by: Manfred Schlaegl <manfred.schlaegl@gmx.at>
> 
> Any comments on whether data enable and pixel clock polarity flags can
> be added to the visible DRM_MODE_FLAGs, and if not, where else this
> information should be kept? struct drm_display_info?
> 
> This patch and the following IPUv3 patch are useful and necessary for
> quite some panels connected to i.MX SoCs, but adding DRM_MODE_FLAGs is
> somewhat out of my jurisdiction.
> 
> best regards
> Philipp
> 

Good to see that this discussion is triggered.

State of the sent patches respective to git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git (78c4a49)
 * [RFC PATCH 1/2] drm: add support for for clk and de polarity
   * applies unmodified
 * [RFC PATCH 2/2] gpu: ipu-v3: use clock and de polarity from videomode
   * does not apply -> but this can be fixed with corrections of line offsets (using patch)
From a code review I think the modifications must work, but I did no testing on kernel versions newer than 4.1.13.
On demand, I can redo tests on some of our i.MX53 (parallel) and i.MX6 (lvds) boards and send actual patches.

best regards,
Manfred

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 1/2] drm: add support for for clk and de polarity

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2650 bytes --]



On 26/11/15 16:20, Manfred Schlaegl wrote:
> On 2015-11-25 18:22, Philipp Zabel wrote:
>> Am Mittwoch, den 15.07.2015, 17:50 +0200 schrieb Manfred Schlaegl:
>>> To get full support for parallel and LVDS displays with drm:
>>> Add representation for clock and data enable polarity in drm_display_mode
>>> flags (similar to HSYNC/VSYNC polarity) and update conversion functions
>>> from/to videomode accordingly.
>>>
>>> This is especially important for embedded devices where parallel(RGB) and
>>> LVDS displays are still widely used and drm already plays an important
>>> role.
>>>
>>> Tested on Freescale i.MX53(parallel) and i.MX6(LVDS).
>>>
>>> Background:
>>> There was the ability to set polarity of clock and data enable signals
>>> in devicetree(display-timing), struct display_timing and struct videomode,
>>> but there was no representation for this in struct drm_display_mode.
>>> Example on Freescale i.MX53/i.MX6 SoC's:
>>>  * A parallel display using different clock polarity is set up using
>>>    display-timing in devicetree
>>>  * ipuv3 parallel outputs clock with wrong polarity
>>>
>>> Signed-off-by: Manfred Schlaegl <manfred.schlaegl@gmx.at>
>>
>> Any comments on whether data enable and pixel clock polarity flags can
>> be added to the visible DRM_MODE_FLAGs, and if not, where else this
>> information should be kept? struct drm_display_info?
>>
>> This patch and the following IPUv3 patch are useful and necessary for
>> quite some panels connected to i.MX SoCs, but adding DRM_MODE_FLAGs is
>> somewhat out of my jurisdiction.
>>
>> best regards
>> Philipp
>>
> 
> Good to see that this discussion is triggered.

I seem to have missed this one. This is important for omapdrm also.
We've had similar patch in TI's linux for a while, but I have never had
time to start upstreaming it.

Two comments:

The "pixclock polarity" could be explained a bit, as it's not really
about polarity. This was discussed when the display-timings stuff was
worked on, and display-timings.txt explains what the "pixelclk-active"
property means.

So here I think you could maybe have a comment pointing to
display-timings.txt, or perhaps a short comment about what the flag is.
Or if you come up with a great name for the define, that's good too =).

The other comment is not about this patch as such, but similar flags
that OMAP has, and possibly some other platforms too:

1) sync signals driven on rising or falling edge of pixel clock
2) hsync and vsync happen at the same time or hsync happens first,
followed by vsync

Any other platforms have similar features?

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC PATCH 1/2] drm: add support for for clk and de polarity

[Philipp Zabel]

Am Freitag, den 27.11.2015, 09:37 +0200 schrieb Tomi Valkeinen:
> On 26/11/15 16:20, Manfred Schlaegl wrote:
> > Good to see that this discussion is triggered.
> 
> I seem to have missed this one. This is important for omapdrm also.
> We've had similar patch in TI's linux for a while, but I have never had
> time to start upstreaming it.
> 
> Two comments:
> 
> The "pixclock polarity" could be explained a bit, as it's not really
> about polarity. This was discussed when the display-timings stuff was
> worked on, and display-timings.txt explains what the "pixelclk-active"
> property means.

Yes, the relevant part of this setting is whether the panel will sample
the data bus on the falling or rising edge of the pixel clock signal.
The display interface has guarantee that the data bus is stable around
that time.

> So here I think you could maybe have a comment pointing to
> display-timings.txt, or perhaps a short comment about what the flag is.
> Or if you come up with a great name for the define, that's good too =).

We have the choice of describing the flag from point of view of the
display controller (as the DISPLAY_FLAGS do), from point of view of the
panel, or using a somewhat neutral description like in the device tree.
Which choice I'd prefer depends on whether the flags go into
drm_display_mode / drm_mode_modeinfo or in drm_display_info.

In any case, I think that it'd be better to talk about driving or
sampling data on rising or falling edges instead of clock polarity.

> The other comment is not about this patch as such, but similar flags
> that OMAP has, and possibly some other platforms too:
> 
> 1) sync signals driven on rising or falling edge of pixel clock
> 2) hsync and vsync happen at the same time or hsync happens first,
> followed by vsync
> 
> Any other platforms have similar features?

The i.MX6 display interface consists of a number of rather freely
configurable signal generators, so all of this should be possible to do.

regards
Philipp

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH 2/2] gpu: ipu-v3: use clock and de polarity from videomode

[Manfred Schlaegl]

This patch depends on "drm: add support for for clk and de polarity".
Since "drm: add support for for clk and de polarity", clock and data
polarity set in devicetree are passed correctly through drm_display_mode
to videomode flags used by ipuv3.
Removes custom configuration flags for clock and de polarities and use
polarity flags from given videomode.
Keeps compatibility to current settings and device tree descriptions,
by setting data enable polarity to high, if not explicitly set to low by
flags.

display-timings                 resulting polarity of pixel clock
                                past    now
pixelclk-active unset           0       0
pixelclk-active = 0             0       0
pixelclk-active = 1             0       1       <--- corrected

display-timings                 resulting polarity of data enable
                                past    now
de-active unset                 1       1
de-active = 0                   1       0       <--- corrected
de-active = 1                   1       1

Tested on Freescale i.MX53(parallel) and i.MX6(LVDS).

Signed-off-by: Manfred Schlaegl <manfred.schlaegl@gmx.at>
---
 drivers/gpu/drm/imx/ipuv3-crtc.c | 2 --
 drivers/gpu/ipu-v3/ipu-di.c      | 4 ++--
 include/video/imx-ipu-v3.h       | 2 --
 3 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index 7bc8301..6813271 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -172,8 +172,6 @@ static int ipu_crtc_mode_set(struct drm_crtc *crtc,
 	else
 		sig_cfg.clkflags = 0;
 
-	sig_cfg.enable_pol = 1;
-	sig_cfg.clk_pol = 0;
 	sig_cfg.bus_format = ipu_crtc->bus_format;
 	sig_cfg.v_to_h_sync = 0;
 	sig_cfg.hsync_pin = ipu_crtc->di_hsync_pin;
diff --git a/drivers/gpu/ipu-v3/ipu-di.c b/drivers/gpu/ipu-v3/ipu-di.c
index 2970c6b..0b1a7d2 100644
--- a/drivers/gpu/ipu-v3/ipu-di.c
+++ b/drivers/gpu/ipu-v3/ipu-di.c
@@ -621,7 +621,7 @@ int ipu_di_init_sync_panel(struct ipu_di *di, struct ipu_di_signal_cfg *sig)
 		}
 	}
 
-	if (sig->clk_pol)
+	if (sig->mode.flags & DISPLAY_FLAGS_PIXDATA_POSEDGE)
 		di_gen |= DI_GEN_POLARITY_DISP_CLK;
 
 	ipu_di_write(di, di_gen, DI_GENERAL);
@@ -632,7 +632,7 @@ int ipu_di_init_sync_panel(struct ipu_di *di, struct ipu_di_signal_cfg *sig)
 	reg = ipu_di_read(di, DI_POL);
 	reg &= ~(DI_POL_DRDY_DATA_POLARITY | DI_POL_DRDY_POLARITY_15);
 
-	if (sig->enable_pol)
+	if (!(sig->mode.flags & DISPLAY_FLAGS_DE_LOW))
 		reg |= DI_POL_DRDY_POLARITY_15;
 	if (sig->data_pol)
 		reg |= DI_POL_DRDY_DATA_POLARITY;
diff --git a/include/video/imx-ipu-v3.h b/include/video/imx-ipu-v3.h
index 85dedca..4178fa2 100644
--- a/include/video/imx-ipu-v3.h
+++ b/include/video/imx-ipu-v3.h
@@ -34,8 +34,6 @@ enum ipuv3_type {
  */
 struct ipu_di_signal_cfg {
 	unsigned data_pol:1;	/* true = inverted */
-	unsigned clk_pol:1;	/* true = rising edge */
-	unsigned enable_pol:1;
 
 	struct videomode mode;
 
-- 
2.1.4