diff mbox

[v17,2/7] video: add display_timing and videomode

Message ID 1359104515-8907-3-git-send-email-s.trumtrar@pengutronix.de (mailing list archive)
State New, archived
Headers show

Commit Message

Steffen Trumtrar Jan. 25, 2013, 9:01 a.m. UTC
Add display_timing structure and the according helper functions. This allows
the description of a display via its supported timing parameters.

Also, add helper functions to convert from display timings to a generic videomode
structure.

The struct display_timing specifies all needed parameters to describe the signal
properties of a display in one mode. This includes
    - ranges for signals that may have min-, max- and typical values
    - single integers for signals that can be on, off or are ignored
    - booleans for signals that are either on or off

As a display may support multiple modes like this, a struct display_timings is
added, that holds all given struct display_timing pointers and declares the
native mode of the display.

Although a display may state that a signal can be in a range, it is driven with
fixed values that indicate a videomode. Therefore graphic drivers don't need all
the information of struct display_timing, but would generate a videomode from
the given set of supported signal timings and work with that.

The video subsystems all define their own structs that describe a mode and work
with that (e.g. fb_videomode or drm_display_mode). To slowly replace all those
various structures and allow code reuse across those subsystems, add struct
videomode as a generic description.

This patch only includes the most basic fields in struct videomode. All missing
fields that are needed to have a really generic video mode description can be
added at a later stage.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
Reviewed-by: Thierry Reding <thierry.reding@avionic-design.de>
Acked-by: Thierry Reding <thierry.reding@avionic-design.de>
Tested-by: Thierry Reding <thierry.reding@avionic-design.de>
Tested-by: Philipp Zabel <p.zabel@pengutronix.de>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Tested-by: Afzal Mohammed <Afzal@ti.com>
Tested-by: Rob Clark <robclark@gmail.com>
Tested-by: Leela Krishna Amudala <leelakrishna.a@gmail.com>
---
 drivers/video/Kconfig          |    6 ++
 drivers/video/Makefile         |    2 +
 drivers/video/display_timing.c |   24 ++++++++
 drivers/video/videomode.c      |   39 +++++++++++++
 include/video/display_timing.h |  124 ++++++++++++++++++++++++++++++++++++++++
 include/video/videomode.h      |   48 ++++++++++++++++
 6 files changed, 243 insertions(+)
 create mode 100644 drivers/video/display_timing.c
 create mode 100644 drivers/video/videomode.c
 create mode 100644 include/video/display_timing.h
 create mode 100644 include/video/videomode.h

Comments

Tomi Valkeinen Feb. 18, 2013, 2:09 p.m. UTC | #1
Hi Steffen,

On 2013-01-25 11:01, Steffen Trumtrar wrote:

> +/* VESA display monitor timing parameters */
> +#define VESA_DMT_HSYNC_LOW		BIT(0)
> +#define VESA_DMT_HSYNC_HIGH		BIT(1)
> +#define VESA_DMT_VSYNC_LOW		BIT(2)
> +#define VESA_DMT_VSYNC_HIGH		BIT(3)
> +
> +/* display specific flags */
> +#define DISPLAY_FLAGS_DE_LOW		BIT(0)	/* data enable flag */
> +#define DISPLAY_FLAGS_DE_HIGH		BIT(1)
> +#define DISPLAY_FLAGS_PIXDATA_POSEDGE	BIT(2)	/* drive data on pos. edge */
> +#define DISPLAY_FLAGS_PIXDATA_NEGEDGE	BIT(3)	/* drive data on neg. edge */
> +#define DISPLAY_FLAGS_INTERLACED	BIT(4)
> +#define DISPLAY_FLAGS_DOUBLESCAN	BIT(5)

<snip>

> +	unsigned int dmt_flags;	/* VESA DMT flags */
> +	unsigned int data_flags; /* video data flags */

Why did you go for this approach? To be able to represent
true/false/not-specified?

Would it be simpler to just have "flags" field? What does it give us to
have those two separately?

Should the above say raising edge/falling edge instead of positive
edge/negative edge?

 Tomi

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomi Valkeinen Feb. 27, 2013, 3:45 p.m. UTC | #2
Ping.

On 2013-02-18 16:09, Tomi Valkeinen wrote:
> Hi Steffen,
> 
> On 2013-01-25 11:01, Steffen Trumtrar wrote:
> 
>> +/* VESA display monitor timing parameters */
>> +#define VESA_DMT_HSYNC_LOW		BIT(0)
>> +#define VESA_DMT_HSYNC_HIGH		BIT(1)
>> +#define VESA_DMT_VSYNC_LOW		BIT(2)
>> +#define VESA_DMT_VSYNC_HIGH		BIT(3)
>> +
>> +/* display specific flags */
>> +#define DISPLAY_FLAGS_DE_LOW		BIT(0)	/* data enable flag */
>> +#define DISPLAY_FLAGS_DE_HIGH		BIT(1)
>> +#define DISPLAY_FLAGS_PIXDATA_POSEDGE	BIT(2)	/* drive data on pos. edge */
>> +#define DISPLAY_FLAGS_PIXDATA_NEGEDGE	BIT(3)	/* drive data on neg. edge */
>> +#define DISPLAY_FLAGS_INTERLACED	BIT(4)
>> +#define DISPLAY_FLAGS_DOUBLESCAN	BIT(5)
> 
> <snip>
> 
>> +	unsigned int dmt_flags;	/* VESA DMT flags */
>> +	unsigned int data_flags; /* video data flags */
> 
> Why did you go for this approach? To be able to represent
> true/false/not-specified?
> 
> Would it be simpler to just have "flags" field? What does it give us to
> have those two separately?
> 
> Should the above say raising edge/falling edge instead of positive
> edge/negative edge?
> 
>  Tomi
>
Steffen Trumtrar Feb. 27, 2013, 4:05 p.m. UTC | #3
Ah, sorry. Forgot to answer this.

On Wed, Feb 27, 2013 at 05:45:31PM +0200, Tomi Valkeinen wrote:
> Ping.
> 
> On 2013-02-18 16:09, Tomi Valkeinen wrote:
> > Hi Steffen,
> > 
> > On 2013-01-25 11:01, Steffen Trumtrar wrote:
> > 
> >> +/* VESA display monitor timing parameters */
> >> +#define VESA_DMT_HSYNC_LOW		BIT(0)
> >> +#define VESA_DMT_HSYNC_HIGH		BIT(1)
> >> +#define VESA_DMT_VSYNC_LOW		BIT(2)
> >> +#define VESA_DMT_VSYNC_HIGH		BIT(3)
> >> +
> >> +/* display specific flags */
> >> +#define DISPLAY_FLAGS_DE_LOW		BIT(0)	/* data enable flag */
> >> +#define DISPLAY_FLAGS_DE_HIGH		BIT(1)
> >> +#define DISPLAY_FLAGS_PIXDATA_POSEDGE	BIT(2)	/* drive data on pos. edge */
> >> +#define DISPLAY_FLAGS_PIXDATA_NEGEDGE	BIT(3)	/* drive data on neg. edge */
> >> +#define DISPLAY_FLAGS_INTERLACED	BIT(4)
> >> +#define DISPLAY_FLAGS_DOUBLESCAN	BIT(5)
> > 
> > <snip>
> > 
> >> +	unsigned int dmt_flags;	/* VESA DMT flags */
> >> +	unsigned int data_flags; /* video data flags */
> > 
> > Why did you go for this approach? To be able to represent
> > true/false/not-specified?
> > 

We decided somewhere between v3 and v8 (I think), that those flags can be
high/low/ignored.

> > Would it be simpler to just have "flags" field? What does it give us to
> > have those two separately?
> > 

I decided to split them, so it is clear that some flags are VESA defined and
the others are "invented" for the display-timings framework and may be
extended.

> > Should the above say raising edge/falling edge instead of positive
> > edge/negative edge?
> > 

Hm, I used posedge/negedge because it is shorter (and because of my Verilog past
pretty natural to me :-) ). I don't know what others are thinking though.

Regards,
Steffen
Tomi Valkeinen Feb. 27, 2013, 4:13 p.m. UTC | #4
On 2013-02-27 18:05, Steffen Trumtrar wrote:
> Ah, sorry. Forgot to answer this.
> 
> On Wed, Feb 27, 2013 at 05:45:31PM +0200, Tomi Valkeinen wrote:
>> Ping.
>>
>> On 2013-02-18 16:09, Tomi Valkeinen wrote:
>>> Hi Steffen,
>>>
>>> On 2013-01-25 11:01, Steffen Trumtrar wrote:
>>>
>>>> +/* VESA display monitor timing parameters */
>>>> +#define VESA_DMT_HSYNC_LOW		BIT(0)
>>>> +#define VESA_DMT_HSYNC_HIGH		BIT(1)
>>>> +#define VESA_DMT_VSYNC_LOW		BIT(2)
>>>> +#define VESA_DMT_VSYNC_HIGH		BIT(3)
>>>> +
>>>> +/* display specific flags */
>>>> +#define DISPLAY_FLAGS_DE_LOW		BIT(0)	/* data enable flag */
>>>> +#define DISPLAY_FLAGS_DE_HIGH		BIT(1)
>>>> +#define DISPLAY_FLAGS_PIXDATA_POSEDGE	BIT(2)	/* drive data on pos. edge */
>>>> +#define DISPLAY_FLAGS_PIXDATA_NEGEDGE	BIT(3)	/* drive data on neg. edge */
>>>> +#define DISPLAY_FLAGS_INTERLACED	BIT(4)
>>>> +#define DISPLAY_FLAGS_DOUBLESCAN	BIT(5)
>>>
>>> <snip>
>>>
>>>> +	unsigned int dmt_flags;	/* VESA DMT flags */
>>>> +	unsigned int data_flags; /* video data flags */
>>>
>>> Why did you go for this approach? To be able to represent
>>> true/false/not-specified?
>>>
> 
> We decided somewhere between v3 and v8 (I think), that those flags can be
> high/low/ignored.

Okay. Why aren't they enums, though? That always makes more clear which
defines are to be used with which fields.

>>> Would it be simpler to just have "flags" field? What does it give us to
>>> have those two separately?
>>>
> 
> I decided to split them, so it is clear that some flags are VESA defined and
> the others are "invented" for the display-timings framework and may be
> extended.

Hmm... Okay. Is it relevant that they are VESA defined? It just feels to
complicate handling the flags =).

>>> Should the above say raising edge/falling edge instead of positive
>>> edge/negative edge?
>>>
> 
> Hm, I used posedge/negedge because it is shorter (and because of my Verilog past
> pretty natural to me :-) ). I don't know what others are thinking though.

I guess it's quite clear, but it's still different terms than used
elsewhere, e.g. documentation for videomodes.

Another thing I noticed while using the new videomode, display_timings.h
has a few names that are quite short and generic. Like "TE_MIN", which
is now a global define. And "timing_entry". Either name could be well
used internally in some .c file, and could easily clash.

 Tomi
Steffen Trumtrar March 5, 2013, 9:24 a.m. UTC | #5
Hi!

On Wed, Feb 27, 2013 at 06:13:49PM +0200, Tomi Valkeinen wrote:
> On 2013-02-27 18:05, Steffen Trumtrar wrote:
> > Ah, sorry. Forgot to answer this.
> > 
> > On Wed, Feb 27, 2013 at 05:45:31PM +0200, Tomi Valkeinen wrote:
> >> Ping.
> >>
> >> On 2013-02-18 16:09, Tomi Valkeinen wrote:
> >>> Hi Steffen,
> >>>
> >>> On 2013-01-25 11:01, Steffen Trumtrar wrote:
> >>>
> >>>> +/* VESA display monitor timing parameters */
> >>>> +#define VESA_DMT_HSYNC_LOW		BIT(0)
> >>>> +#define VESA_DMT_HSYNC_HIGH		BIT(1)
> >>>> +#define VESA_DMT_VSYNC_LOW		BIT(2)
> >>>> +#define VESA_DMT_VSYNC_HIGH		BIT(3)
> >>>> +
> >>>> +/* display specific flags */
> >>>> +#define DISPLAY_FLAGS_DE_LOW		BIT(0)	/* data enable flag */
> >>>> +#define DISPLAY_FLAGS_DE_HIGH		BIT(1)
> >>>> +#define DISPLAY_FLAGS_PIXDATA_POSEDGE	BIT(2)	/* drive data on pos. edge */
> >>>> +#define DISPLAY_FLAGS_PIXDATA_NEGEDGE	BIT(3)	/* drive data on neg. edge */
> >>>> +#define DISPLAY_FLAGS_INTERLACED	BIT(4)
> >>>> +#define DISPLAY_FLAGS_DOUBLESCAN	BIT(5)
> >>>
> >>> <snip>
> >>>
> >>>> +	unsigned int dmt_flags;	/* VESA DMT flags */
> >>>> +	unsigned int data_flags; /* video data flags */
> >>>
> >>> Why did you go for this approach? To be able to represent
> >>> true/false/not-specified?
> >>>
> > 
> > We decided somewhere between v3 and v8 (I think), that those flags can be
> > high/low/ignored.
> 
> Okay. Why aren't they enums, though? That always makes more clear which
> defines are to be used with which fields.
> 

Hm...

> >>> Would it be simpler to just have "flags" field? What does it give us to
> >>> have those two separately?
> >>>
> > 
> > I decided to split them, so it is clear that some flags are VESA defined and
> > the others are "invented" for the display-timings framework and may be
> > extended.
> 
> Hmm... Okay. Is it relevant that they are VESA defined? It just feels to
> complicate handling the flags =).
> 
> >>> Should the above say raising edge/falling edge instead of positive
> >>> edge/negative edge?
> >>>
> > 
> > Hm, I used posedge/negedge because it is shorter (and because of my Verilog past
> > pretty natural to me :-) ). I don't know what others are thinking though.
> 
> I guess it's quite clear, but it's still different terms than used
> elsewhere, e.g. documentation for videomodes.
> 
> Another thing I noticed while using the new videomode, display_timings.h
> has a few names that are quite short and generic. Like "TE_MIN", which
> is now a global define. And "timing_entry". Either name could be well
> used internally in some .c file, and could easily clash.
> 

Yes. You are correct.
Everyone using this is welcome to send patches now :-)

Regards,
Steffen
diff mbox

Patch

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index e7068c5..09a8f0d 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -33,6 +33,12 @@  config VIDEO_OUTPUT_CONTROL
 	  This framework adds support for low-level control of the video 
 	  output switch.
 
+config DISPLAY_TIMING
+       bool
+
+config VIDEOMODE
+       bool
+
 menuconfig FB
 	tristate "Support for frame buffer devices"
 	---help---
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index 768a137..e0dd820 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -168,3 +168,5 @@  obj-$(CONFIG_FB_VIRTUAL)          += vfb.o
 
 #video output switch sysfs driver
 obj-$(CONFIG_VIDEO_OUTPUT_CONTROL) += output.o
+obj-$(CONFIG_DISPLAY_TIMING) += display_timing.o
+obj-$(CONFIG_VIDEOMODE) += videomode.o
diff --git a/drivers/video/display_timing.c b/drivers/video/display_timing.c
new file mode 100644
index 0000000..5e1822c
--- /dev/null
+++ b/drivers/video/display_timing.c
@@ -0,0 +1,24 @@ 
+/*
+ * generic display timing functions
+ *
+ * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>, Pengutronix
+ *
+ * This file is released under the GPLv2
+ */
+
+#include <linux/export.h>
+#include <linux/slab.h>
+#include <video/display_timing.h>
+
+void display_timings_release(struct display_timings *disp)
+{
+	if (disp->timings) {
+		unsigned int i;
+
+		for (i = 0; i < disp->num_timings; i++)
+			kfree(disp->timings[i]);
+		kfree(disp->timings);
+	}
+	kfree(disp);
+}
+EXPORT_SYMBOL_GPL(display_timings_release);
diff --git a/drivers/video/videomode.c b/drivers/video/videomode.c
new file mode 100644
index 0000000..21c47a2
--- /dev/null
+++ b/drivers/video/videomode.c
@@ -0,0 +1,39 @@ 
+/*
+ * generic display timing functions
+ *
+ * Copyright (c) 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>, Pengutronix
+ *
+ * This file is released under the GPLv2
+ */
+
+#include <linux/errno.h>
+#include <linux/export.h>
+#include <video/display_timing.h>
+#include <video/videomode.h>
+
+int videomode_from_timing(const struct display_timings *disp,
+			  struct videomode *vm, unsigned int index)
+{
+	struct display_timing *dt;
+
+	dt = display_timings_get(disp, index);
+	if (!dt)
+		return -EINVAL;
+
+	vm->pixelclock = display_timing_get_value(&dt->pixelclock, TE_TYP);
+	vm->hactive = display_timing_get_value(&dt->hactive, TE_TYP);
+	vm->hfront_porch = display_timing_get_value(&dt->hfront_porch, TE_TYP);
+	vm->hback_porch = display_timing_get_value(&dt->hback_porch, TE_TYP);
+	vm->hsync_len = display_timing_get_value(&dt->hsync_len, TE_TYP);
+
+	vm->vactive = display_timing_get_value(&dt->vactive, TE_TYP);
+	vm->vfront_porch = display_timing_get_value(&dt->vfront_porch, TE_TYP);
+	vm->vback_porch = display_timing_get_value(&dt->vback_porch, TE_TYP);
+	vm->vsync_len = display_timing_get_value(&dt->vsync_len, TE_TYP);
+
+	vm->dmt_flags = dt->dmt_flags;
+	vm->data_flags = dt->data_flags;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(videomode_from_timing);
diff --git a/include/video/display_timing.h b/include/video/display_timing.h
new file mode 100644
index 0000000..71e9a38
--- /dev/null
+++ b/include/video/display_timing.h
@@ -0,0 +1,124 @@ 
+/*
+ * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
+ *
+ * description of display timings
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __LINUX_DISPLAY_TIMING_H
+#define __LINUX_DISPLAY_TIMING_H
+
+#include <linux/bitops.h>
+#include <linux/types.h>
+
+/* VESA display monitor timing parameters */
+#define VESA_DMT_HSYNC_LOW		BIT(0)
+#define VESA_DMT_HSYNC_HIGH		BIT(1)
+#define VESA_DMT_VSYNC_LOW		BIT(2)
+#define VESA_DMT_VSYNC_HIGH		BIT(3)
+
+/* display specific flags */
+#define DISPLAY_FLAGS_DE_LOW		BIT(0)	/* data enable flag */
+#define DISPLAY_FLAGS_DE_HIGH		BIT(1)
+#define DISPLAY_FLAGS_PIXDATA_POSEDGE	BIT(2)	/* drive data on pos. edge */
+#define DISPLAY_FLAGS_PIXDATA_NEGEDGE	BIT(3)	/* drive data on neg. edge */
+#define DISPLAY_FLAGS_INTERLACED	BIT(4)
+#define DISPLAY_FLAGS_DOUBLESCAN	BIT(5)
+
+/*
+ * A single signal can be specified via a range of minimal and maximal values
+ * with a typical value, that lies somewhere inbetween.
+ */
+struct timing_entry {
+	u32 min;
+	u32 typ;
+	u32 max;
+};
+
+enum timing_entry_index {
+	TE_MIN = 0,
+	TE_TYP = 1,
+	TE_MAX = 2,
+};
+
+/*
+ * Single "mode" entry. This describes one set of signal timings a display can
+ * have in one setting. This struct can later be converted to struct videomode
+ * (see include/video/videomode.h). As each timing_entry can be defined as a
+ * range, one struct display_timing may become multiple struct videomodes.
+ *
+ * Example: hsync active high, vsync active low
+ *
+ *				    Active Video
+ * Video  ______________________XXXXXXXXXXXXXXXXXXXXXX_____________________
+ *	  |<- sync ->|<- back ->|<----- active ----->|<- front ->|<- sync..
+ *	  |	     |	 porch  |		     |	 porch	 |
+ *
+ * HSync _|¯¯¯¯¯¯¯¯¯¯|___________________________________________|¯¯¯¯¯¯¯¯¯
+ *
+ * VSync ¯|__________|¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯¯|_________
+ */
+struct display_timing {
+	struct timing_entry pixelclock;
+
+	struct timing_entry hactive;		/* hor. active video */
+	struct timing_entry hfront_porch;	/* hor. front porch */
+	struct timing_entry hback_porch;	/* hor. back porch */
+	struct timing_entry hsync_len;		/* hor. sync len */
+
+	struct timing_entry vactive;		/* ver. active video */
+	struct timing_entry vfront_porch;	/* ver. front porch */
+	struct timing_entry vback_porch;	/* ver. back porch */
+	struct timing_entry vsync_len;		/* ver. sync len */
+
+	unsigned int dmt_flags;			/* VESA DMT flags */
+	unsigned int data_flags;		/* video data flags */
+};
+
+/*
+ * This describes all timing settings a display provides.
+ * The native_mode is the default setting for this display.
+ * Drivers that can handle multiple videomodes should work with this struct and
+ * convert each entry to the desired end result.
+ */
+struct display_timings {
+	unsigned int num_timings;
+	unsigned int native_mode;
+
+	struct display_timing **timings;
+};
+
+/* get value specified by index from struct timing_entry */
+static inline u32 display_timing_get_value(const struct timing_entry *te,
+					   enum timing_entry_index index)
+{
+	switch (index) {
+	case TE_MIN:
+		return te->min;
+		break;
+	case TE_TYP:
+		return te->typ;
+		break;
+	case TE_MAX:
+		return te->max;
+		break;
+	default:
+		return te->typ;
+	}
+}
+
+/* get one entry from struct display_timings */
+static inline struct display_timing *display_timings_get(const struct
+							 display_timings *disp,
+							 unsigned int index)
+{
+	if (disp->num_timings > index)
+		return disp->timings[index];
+	else
+		return NULL;
+}
+
+void display_timings_release(struct display_timings *disp);
+
+#endif
diff --git a/include/video/videomode.h b/include/video/videomode.h
new file mode 100644
index 0000000..a421562
--- /dev/null
+++ b/include/video/videomode.h
@@ -0,0 +1,48 @@ 
+/*
+ * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de>
+ *
+ * generic videomode description
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __LINUX_VIDEOMODE_H
+#define __LINUX_VIDEOMODE_H
+
+#include <linux/types.h>
+#include <video/display_timing.h>
+
+/*
+ * Subsystem independent description of a videomode.
+ * Can be generated from struct display_timing.
+ */
+struct videomode {
+	unsigned long pixelclock;	/* pixelclock in Hz */
+
+	u32 hactive;
+	u32 hfront_porch;
+	u32 hback_porch;
+	u32 hsync_len;
+
+	u32 vactive;
+	u32 vfront_porch;
+	u32 vback_porch;
+	u32 vsync_len;
+
+	unsigned int dmt_flags;	/* VESA DMT flags */
+	unsigned int data_flags; /* video data flags */
+};
+
+/**
+ * videomode_from_timing - convert display timing to videomode
+ * @disp: structure with all possible timing entries
+ * @vm: return value
+ * @index: index into the list of display timings in devicetree
+ *
+ * DESCRIPTION:
+ * This function converts a struct display_timing to a struct videomode.
+ */
+int videomode_from_timing(const struct display_timings *disp,
+			  struct videomode *vm, unsigned int index);
+
+#endif