| Message ID | 1352971437-29877-2-git-send-email-s.trumtrar@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 15 Nov 2012 10:23:52 +0100, Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote: > Add display_timing structure and the according helper functions. This allows > the description of a display via its supported timing parameters. > > Every timing parameter can be specified as a single value or a range > <min typ max>. > > Also, add helper functions to convert from display timings to a generic videomode > structure. This videomode can then be converted to the corresponding subsystem > mode representation (e.g. fb_videomode). > > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de> Hmmm... here's my thoughts as an outside reviewer. Correct me if I'm making an incorrect assumption. It looks to me that the purpose of this entire series is to decode video timings from the device tree and (eventually) provide the data in the form 'struct videomode'. Correct? If so, then it looks over engineered. Creating new infrastructure to allocate, maintain, and free a new 'struct display_timings' doesn't make any sense when it is an intermediary data format that will never be used by drivers. Can the DT parsing code instead return a table of struct videomode? But, wait... struct videomode is also a new structure. So it looks like this series creates two new intermediary data structures; display_timings and videomode. And at least as far as I can see in this series struct fb_videomode is the only user. g.
Hi Grant, On Thursday 15 November 2012 15:47:53 Grant Likely wrote: > On Thu, 15 Nov 2012 10:23:52 +0100, Steffen Trumtrar wrote: > > Add display_timing structure and the according helper functions. This > > allows the description of a display via its supported timing parameters. > > > > Every timing parameter can be specified as a single value or a range > > <min typ max>. > > > > Also, add helper functions to convert from display timings to a generic > > videomode structure. This videomode can then be converted to the > > corresponding subsystem mode representation (e.g. fb_videomode). > > > > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de> > > Hmmm... here's my thoughts as an outside reviewer. Correct me if I'm > making an incorrect assumption. > > It looks to me that the purpose of this entire series is to decode video > timings from the device tree and (eventually) provide the data in the > form 'struct videomode'. Correct? > > If so, then it looks over engineered. Creating new infrastructure to > allocate, maintain, and free a new 'struct display_timings' doesn't make > any sense when it is an intermediary data format that will never be used > by drivers. > > Can the DT parsing code instead return a table of struct videomode? > > But, wait... struct videomode is also a new structure. So it looks like > this series creates two new intermediary data structures; > display_timings and videomode. And at least as far as I can see in this > series struct fb_videomode is the only user. struct videomode is supposed to slowly replace the various video mode structures we currently have in the kernel (struct drm_mode_modeinfo, struct fb_videomode and struct v4l2_bt_timings), at least where possible (userspace APIs can't be broken). This will make it possible to reuse code across the DRM, FB and V4L2 subsystems, such as the EDID parser or HDMI encoder drivers. This rationale might not be clearly explained in the commit message, but having a shared video mode structure is pretty important.
On Thu, 15 Nov 2012 17:00:57 +0100, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Grant, > > On Thursday 15 November 2012 15:47:53 Grant Likely wrote: > > On Thu, 15 Nov 2012 10:23:52 +0100, Steffen Trumtrar wrote: > > > Add display_timing structure and the according helper functions. This > > > allows the description of a display via its supported timing parameters. > > > > > > Every timing parameter can be specified as a single value or a range > > > <min typ max>. > > > > > > Also, add helper functions to convert from display timings to a generic > > > videomode structure. This videomode can then be converted to the > > > corresponding subsystem mode representation (e.g. fb_videomode). > > > > > > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de> > > > > Hmmm... here's my thoughts as an outside reviewer. Correct me if I'm > > making an incorrect assumption. > > > > It looks to me that the purpose of this entire series is to decode video > > timings from the device tree and (eventually) provide the data in the > > form 'struct videomode'. Correct? > > > > If so, then it looks over engineered. Creating new infrastructure to > > allocate, maintain, and free a new 'struct display_timings' doesn't make > > any sense when it is an intermediary data format that will never be used > > by drivers. > > > > Can the DT parsing code instead return a table of struct videomode? > > > > But, wait... struct videomode is also a new structure. So it looks like > > this series creates two new intermediary data structures; > > display_timings and videomode. And at least as far as I can see in this > > series struct fb_videomode is the only user. > > struct videomode is supposed to slowly replace the various video mode > structures we currently have in the kernel (struct drm_mode_modeinfo, struct > fb_videomode and struct v4l2_bt_timings), at least where possible (userspace > APIs can't be broken). This will make it possible to reuse code across the > DRM, FB and V4L2 subsystems, such as the EDID parser or HDMI encoder drivers. > This rationale might not be clearly explained in the commit message, but > having a shared video mode structure is pretty important. Okay that make sense. What about struct display_timings? g.
On Thursday 15 November 2012 18:03:59 Grant Likely wrote: > On Thu, 15 Nov 2012 17:00:57 +0100, Laurent Pinchart wrote: > > On Thursday 15 November 2012 15:47:53 Grant Likely wrote: > > > On Thu, 15 Nov 2012 10:23:52 +0100, Steffen Trumtrar wrote: > > > > Add display_timing structure and the according helper functions. This > > > > allows the description of a display via its supported timing > > > > parameters. > > > > > > > > Every timing parameter can be specified as a single value or a range > > > > <min typ max>. > > > > > > > > Also, add helper functions to convert from display timings to a > > > > generic videomode structure. This videomode can then be converted to > > > > the corresponding subsystem mode representation (e.g. fb_videomode). > > > > > > > > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de> > > > > > > Hmmm... here's my thoughts as an outside reviewer. Correct me if I'm > > > making an incorrect assumption. > > > > > > It looks to me that the purpose of this entire series is to decode video > > > timings from the device tree and (eventually) provide the data in the > > > form 'struct videomode'. Correct? > > > > > > If so, then it looks over engineered. Creating new infrastructure to > > > allocate, maintain, and free a new 'struct display_timings' doesn't make > > > any sense when it is an intermediary data format that will never be used > > > by drivers. > > > > > > Can the DT parsing code instead return a table of struct videomode? > > > > > > But, wait... struct videomode is also a new structure. So it looks like > > > this series creates two new intermediary data structures; > > > display_timings and videomode. And at least as far as I can see in this > > > series struct fb_videomode is the only user. > > > > struct videomode is supposed to slowly replace the various video mode > > structures we currently have in the kernel (struct drm_mode_modeinfo, > > struct fb_videomode and struct v4l2_bt_timings), at least where possible > > (userspace APIs can't be broken). This will make it possible to reuse > > code across the DRM, FB and V4L2 subsystems, such as the EDID parser or > > HDMI encoder drivers. This rationale might not be clearly explained in > > the commit message, but having a shared video mode structure is pretty > > important. > > Okay that make sense. What about struct display_timings? I'll let Steffen answer on that one as I (currently) have no strong feeling about it. If we were to remove it we would need to carry the number of timings, native timing and timings array around. That wouldn't be very practical for drivers, but it probably all depends on the usage drivers will make of that information.
Hi Grant, On Thu, Nov 15, 2012 at 06:03:59PM +0000, Grant Likely wrote: > On Thu, 15 Nov 2012 17:00:57 +0100, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Grant, > > > > On Thursday 15 November 2012 15:47:53 Grant Likely wrote: > > > On Thu, 15 Nov 2012 10:23:52 +0100, Steffen Trumtrar wrote: > > > > Add display_timing structure and the according helper functions. This > > > > allows the description of a display via its supported timing parameters. > > > > > > > > Every timing parameter can be specified as a single value or a range > > > > <min typ max>. > > > > > > > > Also, add helper functions to convert from display timings to a generic > > > > videomode structure. This videomode can then be converted to the > > > > corresponding subsystem mode representation (e.g. fb_videomode). > > > > > > > > Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de> > > > > > > Hmmm... here's my thoughts as an outside reviewer. Correct me if I'm > > > making an incorrect assumption. > > > > > > It looks to me that the purpose of this entire series is to decode video > > > timings from the device tree and (eventually) provide the data in the > > > form 'struct videomode'. Correct? > > > For the time being it is straight from devicetree via struct videomode to struct drm_display_mode or fb_videomode. Correct. > > > If so, then it looks over engineered. Creating new infrastructure to > > > allocate, maintain, and free a new 'struct display_timings' doesn't make > > > any sense when it is an intermediary data format that will never be used > > > by drivers. > > > > > > Can the DT parsing code instead return a table of struct videomode? > > > See below. > > > But, wait... struct videomode is also a new structure. So it looks like > > > this series creates two new intermediary data structures; > > > display_timings and videomode. And at least as far as I can see in this > > > series struct fb_videomode is the only user. struct drm_display_mode is also a user in this series see 5/6 and 6/6. > > > > struct videomode is supposed to slowly replace the various video mode > > structures we currently have in the kernel (struct drm_mode_modeinfo, struct > > fb_videomode and struct v4l2_bt_timings), at least where possible (userspace > > APIs can't be broken). This will make it possible to reuse code across the > > DRM, FB and V4L2 subsystems, such as the EDID parser or HDMI encoder drivers. > > This rationale might not be clearly explained in the commit message, but > > having a shared video mode structure is pretty important. > That. > Okay that make sense. What about struct display_timings? > The reason for defining an intermediary step is because of the different things that are described: - struct display_timing describes the signal ranges a display supports - struct display_timings describes all timing settings of a display - struct videomode describes one single mode generated from that settings It is possible to generate multiple struct videomodes from one struct display_timing based on the circumstances. And that is a task for the driver using the display_timing infos. This means drivers are supposed to use struct display_timings if they need to generate a struct videomode from the timing ranges of one entry. This is just the first step in that direction. I hope this makes the need for struct display_timings a little clearer. The other solution would be the one Laurent suggested and pass multiple values around. Which in my opinion doesn't make it better, more practical or cleaner. Regards, Steffen
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig index d08d799..2a23b18 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 23e948e..fc30439 100644 --- a/drivers/video/Makefile +++ b/drivers/video/Makefile @@ -167,3 +167,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..ac9bbbc --- /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/display_timing.h> +#include <linux/export.h> +#include <linux/slab.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..087374a --- /dev/null +++ b/drivers/video/videomode.c @@ -0,0 +1,45 @@ +/* + * 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/errno.h> +#include <linux/display_timing.h> +#include <linux/kernel.h> +#include <linux/videomode.h> + +int videomode_from_timing(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, 0); + vm->hactive = display_timing_get_value(&dt->hactive, 0); + vm->hfront_porch = display_timing_get_value(&dt->hfront_porch, 0); + vm->hback_porch = display_timing_get_value(&dt->hback_porch, 0); + vm->hsync_len = display_timing_get_value(&dt->hsync_len, 0); + + vm->vactive = display_timing_get_value(&dt->vactive, 0); + vm->vfront_porch = display_timing_get_value(&dt->vfront_porch, 0); + vm->vback_porch = display_timing_get_value(&dt->vback_porch, 0); + vm->vsync_len = display_timing_get_value(&dt->vsync_len, 0); + + vm->vah = dt->vsync_pol_active; + vm->hah = dt->hsync_pol_active; + vm->de = dt->de_pol_active; + vm->pixelclk_pol = dt->pixelclk_pol; + + vm->interlaced = dt->interlaced; + vm->doublescan = dt->doublescan; + + return 0; +} +EXPORT_SYMBOL_GPL(videomode_from_timing); diff --git a/include/linux/display_timing.h b/include/linux/display_timing.h new file mode 100644 index 0000000..caee2a8 --- /dev/null +++ b/include/linux/display_timing.h @@ -0,0 +1,69 @@ +/* + * Copyright 2012 Steffen Trumtrar <s.trumtrar@pengutronix.de> + * + * description of display timings + * + * This file is released under the GPLv2 + */ + +#ifndef __LINUX_DISPLAY_TIMINGS_H +#define __LINUX_DISPLAY_TIMINGS_H + +#include <linux/types.h> + +struct timing_entry { + u32 min; + u32 typ; + u32 max; +}; + +struct display_timing { + struct timing_entry pixelclock; + + struct timing_entry hactive; + struct timing_entry hfront_porch; + struct timing_entry hback_porch; + struct timing_entry hsync_len; + + struct timing_entry vactive; + struct timing_entry vfront_porch; + struct timing_entry vback_porch; + struct timing_entry vsync_len; + + unsigned int vsync_pol_active; + unsigned int hsync_pol_active; + unsigned int de_pol_active; + unsigned int pixelclk_pol; + bool interlaced; + bool doublescan; +}; + +struct display_timings { + unsigned int num_timings; + unsigned int native_mode; + + struct display_timing **timings; +}; + +/* + * placeholder function until ranges are really needed + * the index parameter should then be used to select one of [min typ max] + */ +static inline u32 display_timing_get_value(struct timing_entry *te, + unsigned int index) +{ + return te->typ; +} + +static inline struct display_timing *display_timings_get(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/linux/videomode.h b/include/linux/videomode.h new file mode 100644 index 0000000..704db7b --- /dev/null +++ b/include/linux/videomode.h @@ -0,0 +1,40 @@ +/* + * 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/display_timing.h> + +struct videomode { + u32 pixelclock; + u32 refreshrate; + + u32 hactive; + u32 hfront_porch; + u32 hback_porch; + u32 hsync_len; + + u32 vactive; + u32 vfront_porch; + u32 vback_porch; + u32 vsync_len; + + u32 hah; + u32 vah; + u32 de; + u32 pixelclk_pol; + + bool interlaced; + bool doublescan; +}; + +int videomode_from_timing(struct display_timings *disp, struct videomode *vm, + unsigned int index); + +#endif
Add display_timing structure and the according helper functions. This allows the description of a display via its supported timing parameters. Every timing parameter can be specified as a single value or a range <min typ max>. Also, add helper functions to convert from display timings to a generic videomode structure. This videomode can then be converted to the corresponding subsystem mode representation (e.g. fb_videomode). Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de> --- drivers/video/Kconfig | 6 ++++ drivers/video/Makefile | 2 ++ drivers/video/display_timing.c | 24 ++++++++++++++ drivers/video/videomode.c | 45 ++++++++++++++++++++++++++ include/linux/display_timing.h | 69 ++++++++++++++++++++++++++++++++++++++++ include/linux/videomode.h | 40 +++++++++++++++++++++++ 6 files changed, 186 insertions(+) create mode 100644 drivers/video/display_timing.c create mode 100644 drivers/video/videomode.c create mode 100644 include/linux/display_timing.h create mode 100644 include/linux/videomode.h