Re: [PATCH v2] of: Add videomode helper
Hi Sascha, On Thursday 13 September 2012 13:19:54 Sascha Hauer wrote: On Thu, Sep 13, 2012 at 01:54:07PM +0300, Tomi Valkeinen wrote: On Wed, 2012-07-04 at 09:56 +0200, Sascha Hauer wrote: This patch adds a helper function for parsing videomodes from the devicetree. The videomode can be either converted to a struct drm_display_mode or a struct fb_videomode. I have more or less the same generic comment for this as for the power sequences series discussed: this would add panel specific information into DT data, instead of the driver handling it. But, as also discussed in the thread, there are differing opinions on which way is better. With the panel timings I think the approach of moving them into DT is the best we can do. There are so many displays out there, patching the kernel each time a customer comes with some new display is no fun. For generic panels, sure, but for panels that require a specific driver (for instance because the panel implements vendor-specific comments) the mode(s) could be hardcoded in the panel driver. +int of_get_video_mode(struct device_node *np, struct drm_display_mode *dmode, + struct fb_videomode *fbmode); From caller's point of view I think it'd make more sense to have separate functions to get drm mode or fb mode. I don't think there's a case where the caller would want both. Ok, that makes sense. Then again, even better would be to have a common video mode struct used by both fb and drm... But I think that's been on the todo list of Laurent for a long time already =). Yes, indeed. We should go into that direction. We already realized that we want to have ranges instead of fixed values for the timings. -- Regards, Laurent Pinchart ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] of: Add videomode helper
On Wed, 2012-07-04 at 09:56 +0200, Sascha Hauer wrote: This patch adds a helper function for parsing videomodes from the devicetree. The videomode can be either converted to a struct drm_display_mode or a struct fb_videomode. I have more or less the same generic comment for this as for the power sequences series discussed: this would add panel specific information into DT data, instead of the driver handling it. But, as also discussed in the thread, there are differing opinions on which way is better. +int of_get_video_mode(struct device_node *np, struct drm_display_mode *dmode, + struct fb_videomode *fbmode); From caller's point of view I think it'd make more sense to have separate functions to get drm mode or fb mode. I don't think there's a case where the caller would want both. Then again, even better would be to have a common video mode struct used by both fb and drm... But I think that's been on the todo list of Laurent for a long time already =). Tomi signature.asc Description: This is a digitally signed message part ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] of: Add videomode helper
On Thu, Sep 13, 2012 at 01:54:07PM +0300, Tomi Valkeinen wrote: On Wed, 2012-07-04 at 09:56 +0200, Sascha Hauer wrote: This patch adds a helper function for parsing videomodes from the devicetree. The videomode can be either converted to a struct drm_display_mode or a struct fb_videomode. I have more or less the same generic comment for this as for the power sequences series discussed: this would add panel specific information into DT data, instead of the driver handling it. But, as also discussed in the thread, there are differing opinions on which way is better. With the panel timings I think the approach of moving them into DT is the best we can do. There are so many displays out there, patching the kernel each time a customer comes with some new display is no fun. +int of_get_video_mode(struct device_node *np, struct drm_display_mode *dmode, + struct fb_videomode *fbmode); From caller's point of view I think it'd make more sense to have separate functions to get drm mode or fb mode. I don't think there's a case where the caller would want both. Ok, that makes sense. Then again, even better would be to have a common video mode struct used by both fb and drm... But I think that's been on the todo list of Laurent for a long time already =). Yes, indeed. We should go into that direction. We already realized that we want to have ranges instead of fixed values for the timings. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] of: Add videomode helper
Hi Sascha,
On Friday 03 August 2012 09:38:44 Sascha Hauer wrote:
On Thu, Aug 02, 2012 at 01:35:40PM -0600, Stephen Warren wrote:
On 07/04/2012 01:56 AM, Sascha Hauer wrote:
This patch adds a helper function for parsing videomodes from the
devicetree. The videomode can be either converted to a struct
drm_display_mode or a struct fb_videomode.
diff --git a/Documentation/devicetree/bindings/video/displaymode
b/Documentation/devicetree/bindings/video/displaymode
+Required properties:
+ - xres, yres: Display resolution
+ - left-margin, right-margin, hsync-len: Horizontal Display timing
parameters + in pixels
+ upper-margin, lower-margin, vsync-len: Vertical display timing
parameters in + lines
Perhaps bike-shedding, but...
For the margin property names, wouldn't it be better to use terminology
more commonly used for video timings rather than Linux FB naming. In
other words naming like:
hactive, hsync-len, hfront-porch, hback-porch?
Can do. Just to make sure:
hactive == xres
hsync-len == hsync-len
hfront-porch == right-margin
hback-porch == left-margin
That's correct. On the vertical direction, vfront-porch == lower-margin and
vback-porch == upper-margin.
?
This node appears to describe a video mode, not a display, hence the
node name seems wrong.
Many displays can support multiple different video modes. As mentioned
elsewhere, properties like display width/height are properties of the
display not the mode.
So, I might expect something more like the following (various overhead
properties like reg/#address-cells etc. elided for simplicity):
disp: display {
width-mm = ...;
height-mm = ...;
modes {
mode@0 {
/* 1920x1080p24 */
clock = 5200;
xres = 1920;
yres = 1080;
left-margin = 25;
right-margin = 25;
hsync-len = 25;
lower-margin = 2;
upper-margin = 2;
vsync-len = 2;
hsync-active-high;
};
mode@1 {
};
};
};
Ok, we can do this.
display-connector {
display = disp;
// interface-specific properties such as pixel format here
};
Finally, have you considered just using an EDID instead of creating
something custom? I know that creating an EDID is harder than writing a
few simple properties into a DT, but EDIDs have the following advantages:
I have considered using EDID and I also tried it. It's painful. There
are no (open) tools available for creating EDID. That's something we
could change of course. Then when generating a devicetree there is
always an extra step involved creating the EDID blob. Once the EDID
blob is in devicetree it is not parsable anymore by mere humans, so
to see what we've got there is another tool involved to generate a
readable form again.
a) They're already standardized and very common.
Indeed, that's a big plus for EDID. I have no intention of removing EDID
data from the devicetree. There are situations where EDID is handy, for
example when you get EDID data provided by your vendor.
b) They allow other information such as a display's HDMI audio
capabilities to be represented, which can then feed into an ALSA driver.
c) The few LCD panel datasheets I've seen actually quote their
specification as an EDID already, so deriving the EDID may actually be
easy.
d) People familiar with displays are almost certainly familiar with
EDID's mode representation. There are many ways of representing display
modes (sync position vs. porch widths, htotal specified rather than
specifying all the components and hence htotal being calculated etc.).
Not everyone will be familiar with all representations. Conversion
errors are less likely if the target is EDID's familiar format.
You seem to think about a different class of displays for which EDID
indeed is a better way to handle. What I have to deal with here mostly
are dumb displays which:
- can only handle their native resolution
- Have no audio capabilities at all
- come with a datasheet which specify a min/typ/max triplet for
xres,hsync,..., often enough they are scanned to pdf from some previously
printed paper.
These displays are very common on embedded devices, probably that's the
reason I did not even think about the possibility that a single display
might have different modes.
e) You'll end up with exactly the same data as if you have a DDC-based
external monitor rather than an internal panel, so you end up getting to
a common path in display handling code much more quickly.
All we have in our display driver currently is:
edidp = of_get_property(np, edid, imxpd-edid_len);
if (edidp) {
imxpd-edid = kmemdup(edidp, imxpd-edid_len, GFP_KERNEL);
Re: [PATCH v2] of: Add videomode helper
Hi Sascha,
Sorry for the late reply.
On Thursday 05 July 2012 18:50:29 Sascha Hauer wrote:
On Thu, Jul 05, 2012 at 04:08:07PM +0200, Laurent Pinchart wrote:
+++ b/Documentation/devicetree/bindings/video/displaymode
@@ -0,0 +1,40 @@
+videomode bindings
+==
+
+Required properties:
+ - xres, yres: Display resolution
+ - left-margin, right-margin, hsync-len: Horizontal Display timing
parameters + in pixels
+ upper-margin, lower-margin, vsync-len: Vertical display timing
parameters in + lines
+ - clock: displayclock in Hz
+
+Optional properties:
+ - width-mm, height-mm: Display dimensions in mm
I've always had mixed feelings about the physical display dimension being
part of the display mode. Those are properties of the panel/display
instead of the mode. Storing them as part of the mode can be convenient,
but we then run into consistency issues (developers have to remember in
which display mode instances the values are available, and in which
instances they're set to 0 for instance). If we want to clean this up,
this patch would be a good occasion.
This sounds like a display node with one or more node subnodes, like:
display {
width_mm = ;
height_mm = ;
mode {
xres = ;
yres = ;
...
};
};
Is that what you mean or are you thinking of something else?
Yes, that's exactly what I meant.
--
Regards,
Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] of: Add videomode helper
On 08/03/2012 01:38 AM, Sascha Hauer wrote:
Hi Stephen,
On Thu, Aug 02, 2012 at 01:35:40PM -0600, Stephen Warren wrote:
On 07/04/2012 01:56 AM, Sascha Hauer wrote:
This patch adds a helper function for parsing videomodes from the
devicetree.
The videomode can be either converted to a struct drm_display_mode or a
struct fb_videomode.
diff --git a/Documentation/devicetree/bindings/video/displaymode
b/Documentation/devicetree/bindings/video/displaymode
+Required properties:
+ - xres, yres: Display resolution
+ - left-margin, right-margin, hsync-len: Horizontal Display timing
parameters
+ in pixels
+ upper-margin, lower-margin, vsync-len: Vertical display timing
parameters in
+ lines
Perhaps bike-shedding, but...
For the margin property names, wouldn't it be better to use terminology
more commonly used for video timings rather than Linux FB naming. In
other words naming like:
hactive, hsync-len, hfront-porch, hback-porch?
Can do. Just to make sure:
hactive == xres
hsync-len == hsync-len
hfront-porch == right-margin
hback-porch == left-margin
I believe so yes.
a) They're already standardized and very common.
Indeed, that's a big plus for EDID. I have no intention of removing EDID
data from the devicetree. There are situations where EDID is handy, for
example when you get EDID data provided by your vendor.
b) They allow other information such as a display's HDMI audio
capabilities to be represented, which can then feed into an ALSA driver.
c) The few LCD panel datasheets I've seen actually quote their
specification as an EDID already, so deriving the EDID may actually be easy.
d) People familiar with displays are almost certainly familiar with
EDID's mode representation. There are many ways of representing display
modes (sync position vs. porch widths, htotal specified rather than
specifying all the components and hence htotal being calculated etc.).
Not everyone will be familiar with all representations. Conversion
errors are less likely if the target is EDID's familiar format.
You seem to think about a different class of displays for which EDID
indeed is a better way to handle. What I have to deal with here mostly
are dumb displays which:
- can only handle their native resolution
- Have no audio capabilities at all
- come with a datasheet which specify a min/typ/max triplet for
xres,hsync,..., often enough they are scanned to pdf from some previously
printed paper.
These displays are very common on embedded devices, probably that's the
reason I did not even think about the possibility that a single display
might have different modes.
That's true, but as I mentioned, there are at least some dumb panels
(both I've seen recently) whose specification provides the EDID. I don't
know how common that is though, I must admit.
e) You'll end up with exactly the same data as if you have a DDC-based
external monitor rather than an internal panel, so you end up getting to
a common path in display handling code much more quickly.
All we have in our display driver currently is:
edidp = of_get_property(np, edid, imxpd-edid_len);
if (edidp) {
imxpd-edid = kmemdup(edidp, imxpd-edid_len, GFP_KERNEL);
} else {
ret = of_get_video_mode(np, imxpd-mode, NULL);
if (!ret)
imxpd-mode_valid = 1;
}
Presumably there's more to it though; something later checks
imxpd-mode_valid and if false, parses the EDID and sets up imxpd-mode,
etc.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] of: Add videomode helper
Hi Stephen,
On Thu, Aug 02, 2012 at 01:35:40PM -0600, Stephen Warren wrote:
On 07/04/2012 01:56 AM, Sascha Hauer wrote:
This patch adds a helper function for parsing videomodes from the
devicetree.
The videomode can be either converted to a struct drm_display_mode or a
struct fb_videomode.
diff --git a/Documentation/devicetree/bindings/video/displaymode
b/Documentation/devicetree/bindings/video/displaymode
+Required properties:
+ - xres, yres: Display resolution
+ - left-margin, right-margin, hsync-len: Horizontal Display timing
parameters
+ in pixels
+ upper-margin, lower-margin, vsync-len: Vertical display timing
parameters in
+ lines
Perhaps bike-shedding, but...
For the margin property names, wouldn't it be better to use terminology
more commonly used for video timings rather than Linux FB naming. In
other words naming like:
hactive, hsync-len, hfront-porch, hback-porch?
Can do. Just to make sure:
hactive == xres
hsync-len == hsync-len
hfront-porch == right-margin
hback-porch == left-margin
?
This node appears to describe a video mode, not a display, hence the
node name seems wrong.
Many displays can support multiple different video modes. As mentioned
elsewhere, properties like display width/height are properties of the
display not the mode.
So, I might expect something more like the following (various overhead
properties like reg/#address-cells etc. elided for simplicity):
disp: display {
width-mm = ...;
height-mm = ...;
modes {
mode@0 {
/* 1920x1080p24 */
clock = 5200;
xres = 1920;
yres = 1080;
left-margin = 25;
right-margin = 25;
hsync-len = 25;
lower-margin = 2;
upper-margin = 2;
vsync-len = 2;
hsync-active-high;
};
mode@1 {
};
};
};
Ok, we can do this.
display-connector {
display = disp;
// interface-specific properties such as pixel format here
};
Finally, have you considered just using an EDID instead of creating
something custom? I know that creating an EDID is harder than writing a
few simple properties into a DT, but EDIDs have the following advantages:
I have considered using EDID and I also tried it. It's painful. There
are no (open) tools available for creating EDID. That's something we
could change of course. Then when generating a devicetree there is
always an extra step involved creating the EDID blob. Once the EDID
blob is in devicetree it is not parsable anymore by mere humans, so
to see what we've got there is another tool involved to generate a
readable form again.
a) They're already standardized and very common.
Indeed, that's a big plus for EDID. I have no intention of removing EDID
data from the devicetree. There are situations where EDID is handy, for
example when you get EDID data provided by your vendor.
b) They allow other information such as a display's HDMI audio
capabilities to be represented, which can then feed into an ALSA driver.
c) The few LCD panel datasheets I've seen actually quote their
specification as an EDID already, so deriving the EDID may actually be easy.
d) People familiar with displays are almost certainly familiar with
EDID's mode representation. There are many ways of representing display
modes (sync position vs. porch widths, htotal specified rather than
specifying all the components and hence htotal being calculated etc.).
Not everyone will be familiar with all representations. Conversion
errors are less likely if the target is EDID's familiar format.
You seem to think about a different class of displays for which EDID
indeed is a better way to handle. What I have to deal with here mostly
are dumb displays which:
- can only handle their native resolution
- Have no audio capabilities at all
- come with a datasheet which specify a min/typ/max triplet for
xres,hsync,..., often enough they are scanned to pdf from some previously
printed paper.
These displays are very common on embedded devices, probably that's the
reason I did not even think about the possibility that a single display
might have different modes.
e) You'll end up with exactly the same data as if you have a DDC-based
external monitor rather than an internal panel, so you end up getting to
a common path in display handling code much more quickly.
All we have in our display driver currently is:
edidp = of_get_property(np, edid, imxpd-edid_len);
if (edidp) {
imxpd-edid = kmemdup(edidp, imxpd-edid_len, GFP_KERNEL);
} else {
ret = of_get_video_mode(np, imxpd-mode, NULL);
if (!ret)
imxpd-mode_valid = 1;
}
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions |
Re: [PATCH v2] of: Add videomode helper
On 07/04/2012 01:56 AM, Sascha Hauer wrote:
This patch adds a helper function for parsing videomodes from the devicetree.
The videomode can be either converted to a struct drm_display_mode or a
struct fb_videomode.
diff --git a/Documentation/devicetree/bindings/video/displaymode
b/Documentation/devicetree/bindings/video/displaymode
+Required properties:
+ - xres, yres: Display resolution
+ - left-margin, right-margin, hsync-len: Horizontal Display timing parameters
+ in pixels
+ upper-margin, lower-margin, vsync-len: Vertical display timing parameters
in
+ lines
Perhaps bike-shedding, but...
For the margin property names, wouldn't it be better to use terminology
more commonly used for video timings rather than Linux FB naming. In
other words naming like:
hactive, hsync-len, hfront-porch, hback-porch?
+ - clock: displayclock in Hz
+
+Optional properties:
+ - width-mm, height-mm: Display dimensions in mm
+ - hsync-active-high (bool): Hsync pulse is active high
+ - vsync-active-high (bool): Vsync pulse is active high
+ - interlaced (bool): This is an interlaced mode
+ - doublescan (bool): This is a doublescan mode
+
+There are different ways of describing a display mode. The devicetree
representation
+corresponds to the one used by the Linux Framebuffer framework described
here in
+Documentation/fb/framebuffer.txt. This representation has been chosen
because it's
+the only format which does not allow for inconsistent parameters.Unlike the
Framebuffer
+framework the devicetree has the clock in Hz instead of ps.
As Rob mentioned, I think there shouldn't be any mention of Linux FB here.
+
+Example:
+
+ display@0 {
This node appears to describe a video mode, not a display, hence the
node name seems wrong.
Many displays can support multiple different video modes. As mentioned
elsewhere, properties like display width/height are properties of the
display not the mode.
So, I might expect something more like the following (various overhead
properties like reg/#address-cells etc. elided for simplicity):
disp: display {
width-mm = ...;
height-mm = ...;
modes {
mode@0 {
/* 1920x1080p24 */
clock = 5200;
xres = 1920;
yres = 1080;
left-margin = 25;
right-margin = 25;
hsync-len = 25;
lower-margin = 2;
upper-margin = 2;
vsync-len = 2;
hsync-active-high;
};
mode@1 {
};
};
};
display-connector {
display = disp;
// interface-specific properties such as pixel format here
};
Finally, have you considered just using an EDID instead of creating
something custom? I know that creating an EDID is harder than writing a
few simple properties into a DT, but EDIDs have the following advantages:
a) They're already standardized and very common.
b) They allow other information such as a display's HDMI audio
capabilities to be represented, which can then feed into an ALSA driver.
c) The few LCD panel datasheets I've seen actually quote their
specification as an EDID already, so deriving the EDID may actually be easy.
d) People familiar with displays are almost certainly familiar with
EDID's mode representation. There are many ways of representing display
modes (sync position vs. porch widths, htotal specified rather than
specifying all the components and hence htotal being calculated etc.).
Not everyone will be familiar with all representations. Conversion
errors are less likely if the target is EDID's familiar format.
e) You'll end up with exactly the same data as if you have a DDC-based
external monitor rather than an internal panel, so you end up getting to
a common path in display handling code much more quickly.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] of: Add videomode helper
On 07/05/2012 08:51 AM, Rob Herring wrote:
On 07/04/2012 02:56 AM, Sascha Hauer wrote:
This patch adds a helper function for parsing videomodes from the devicetree.
The videomode can be either converted to a struct drm_display_mode or a
struct fb_videomode.
diff --git a/Documentation/devicetree/bindings/video/displaymode
b/Documentation/devicetree/bindings/video/displaymode
+Example:
+
+ display@0 {
+ /* 1920x1080p24 */
+ clock = 5200;
Should this use the clock binding? You probably need both constraints
and clock binding though.
I don't think the clock binding is appropriate here. This binding
specifies a desired video mode, and should specify just the expected
clock frequency for that mode. Perhaps any tolerance imposed by the
specification used to calculate the mode timing could be specified too,
but the allowed tolerance (for a mode to be recognized by the dispaly)
is more driven by the receiving device than the transmitting device.
The clock bindings should come into play in the display controller that
sends a signal in that display mode. Something like:
mode: hd1080p {
clock = 5200;
xres = 1920;
yres = 1080;
};
display-controller {
pixel-clock-source = clk ...; // common clock bindings here
default-mode = mode;
Often you don't know the frequency up front and/or have limited control
of the frequency (i.e. integer dividers). Then you have to adjust the
margins to get the desired refresh rate. To do that, you need to know
the ranges of values a panel can support. Perhaps you just assume you
can increase the right-margin and lower-margins as I think you will hit
pixel clock frequency max before any limit on margins.
I think this is more usually dealt with in HW design and matching
components.
The PLL/... feeding the display controller is going to have some known
specifications that imply which pixel clocks it can generate. HW
designers will pick a panel that accepts a clock the display controller
can generate. The driver for the display controller will just generate
as near of a pixel clock as it can to the rate specified in the mode
definition. I believe it'd be unusual for a display driver to start
fiddling with front-back porch (or margin) widths to munge the timing;
that kind of thing probably worked OK with analog displays, but with
digital displays where each pixel is clocked even in the margins, I
think that would cause more problems than it solved.
Similarly for external displays, the display controller will just pick
the nearest clock it can. If it can't generate a close enough clock, it
will just refuse to set the mode. In fact, a display controller driver
would typically validate this when the set of legal modes was enumerated
when initially detecting the display, so user-space typically wouldn't
even request invalid modes.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] of: Add videomode helper
Hi Sascha
On Wed, 4 Jul 2012, Sascha Hauer wrote:
This patch adds a helper function for parsing videomodes from the devicetree.
The videomode can be either converted to a struct drm_display_mode or a
struct fb_videomode.
Signed-off-by: Sascha Hauer s.ha...@pengutronix.de
---
changes since v1:
- use hyphens instead of underscores for property names
.../devicetree/bindings/video/displaymode | 40
drivers/of/Kconfig |5 +
drivers/of/Makefile|1 +
drivers/of/of_videomode.c | 108
include/linux/of_videomode.h | 19
5 files changed, 173 insertions(+)
create mode 100644 Documentation/devicetree/bindings/video/displaymode
create mode 100644 drivers/of/of_videomode.c
create mode 100644 include/linux/of_videomode.h
diff --git a/Documentation/devicetree/bindings/video/displaymode
b/Documentation/devicetree/bindings/video/displaymode
new file mode 100644
index 000..43cc17d
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/displaymode
@@ -0,0 +1,40 @@
+videomode bindings
+==
+
+Required properties:
+ - xres, yres: Display resolution
+ - left-margin, right-margin, hsync-len: Horizontal Display timing parameters
+ in pixels
+ upper-margin, lower-margin, vsync-len: Vertical display timing parameters
in
+ lines
+ - clock: displayclock in Hz
+
+Optional properties:
+ - width-mm, height-mm: Display dimensions in mm
+ - hsync-active-high (bool): Hsync pulse is active high
+ - vsync-active-high (bool): Vsync pulse is active high
How about
+ - hsync-active: Hsync pulse polarity: 1 for high, 0 for low
and similar for vsync-active? Which would then become
+ - interlaced (bool): This is an interlaced mode
+ - doublescan (bool): This is a doublescan mode
+
+There are different ways of describing a display mode. The devicetree
representation
+corresponds to the one used by the Linux Framebuffer framework described
here in
+Documentation/fb/framebuffer.txt. This representation has been chosen
because it's
+the only format which does not allow for inconsistent parameters.Unlike the
Framebuffer
+framework the devicetree has the clock in Hz instead of ps.
+
+Example:
+
+ display@0 {
+ /* 1920x1080p24 */
+ clock = 5200;
+ xres = 1920;
+ yres = 1080;
+ left-margin = 25;
+ right-margin = 25;
+ hsync-len = 25;
+ lower-margin = 2;
+ upper-margin = 2;
+ vsync-len = 2;
+ hsync-active-high;
+ hsync-active = 1;
+ };
+
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] of: Add videomode helper
Hi Guennadi, On Wed, Jul 11, 2012 at 10:34:54AM +0200, Guennadi Liakhovetski wrote: Hi Sascha + +Optional properties: + - width-mm, height-mm: Display dimensions in mm + - hsync-active-high (bool): Hsync pulse is active high + - vsync-active-high (bool): Vsync pulse is active high How about + - hsync-active: Hsync pulse polarity: 1 for high, 0 for low I am unsure if it's good to mix this with the other bool flags like: + - interlaced (bool): This is an interlaced mode + - doublescan (bool): This is a doublescan mode Which behave differently. 'interlaced' will be evaluated as true if the property is present, no matter which value it has. This might lead to confusion. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] of: Add videomode helper
On Wed, 11 Jul 2012, Sascha Hauer wrote: Hi Guennadi, On Wed, Jul 11, 2012 at 10:34:54AM +0200, Guennadi Liakhovetski wrote: Hi Sascha + +Optional properties: + - width-mm, height-mm: Display dimensions in mm + - hsync-active-high (bool): Hsync pulse is active high + - vsync-active-high (bool): Vsync pulse is active high How about + - hsync-active: Hsync pulse polarity: 1 for high, 0 for low I am unsure if it's good to mix this with the other bool flags like: + - interlaced (bool): This is an interlaced mode + - doublescan (bool): This is a doublescan mode Which behave differently. 'interlaced' will be evaluated as true if the property is present, no matter which value it has. This might lead to confusion. I don't feel strongly either way either. I don't think it'd be confusing - you have integer properties there too, and the logic in this case is not active high - yes or now, but rather active level - logical 1 (high) or 0 (low). But as I said - that was just an idea, unless someone has strong arguments either way - you're the original author, it's your call:-) Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] of: Add videomode helper
Hi Sascha,
Thanks for the patch.
On Wednesday 04 July 2012 09:56:35 Sascha Hauer wrote:
This patch adds a helper function for parsing videomodes from the
devicetree. The videomode can be either converted to a struct
drm_display_mode or a struct fb_videomode.
Signed-off-by: Sascha Hauer s.ha...@pengutronix.de
---
changes since v1:
- use hyphens instead of underscores for property names
.../devicetree/bindings/video/displaymode | 40
drivers/of/Kconfig |5 +
drivers/of/Makefile|1 +
drivers/of/of_videomode.c | 108 +
include/linux/of_videomode.h | 19
5 files changed, 173 insertions(+)
create mode 100644 Documentation/devicetree/bindings/video/displaymode
create mode 100644 drivers/of/of_videomode.c
create mode 100644 include/linux/of_videomode.h
diff --git a/Documentation/devicetree/bindings/video/displaymode
b/Documentation/devicetree/bindings/video/displaymode new file mode 100644
index 000..43cc17d
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/displaymode
@@ -0,0 +1,40 @@
+videomode bindings
+==
+
+Required properties:
+ - xres, yres: Display resolution
+ - left-margin, right-margin, hsync-len: Horizontal Display timing
parameters + in pixels
+ upper-margin, lower-margin, vsync-len: Vertical display timing
parameters in + lines
+ - clock: displayclock in Hz
+
+Optional properties:
+ - width-mm, height-mm: Display dimensions in mm
I've always had mixed feelings about the physical display dimension being part
of the display mode. Those are properties of the panel/display instead of the
mode. Storing them as part of the mode can be convenient, but we then run into
consistency issues (developers have to remember in which display mode
instances the values are available, and in which instances they're set to 0
for instance). If we want to clean this up, this patch would be a good
occasion.
+ - hsync-active-high (bool): Hsync pulse is active high
+ - vsync-active-high (bool): Vsync pulse is active high
+ - interlaced (bool): This is an interlaced mode
+ - doublescan (bool): This is a doublescan mode
+
+There are different ways of describing a display mode. The devicetree
representation +corresponds to the one used by the Linux Framebuffer
framework described here in +Documentation/fb/framebuffer.txt. This
representation has been chosen because it's +the only format which does not
allow for inconsistent parameters.Unlike the Framebuffer +framework the
devicetree has the clock in Hz instead of ps.
+
+Example:
+
+ display@0 {
+ /* 1920x1080p24 */
+ clock = 5200;
+ xres = 1920;
+ yres = 1080;
+ left-margin = 25;
+ right-margin = 25;
+ hsync-len = 25;
+ lower-margin = 2;
+ upper-margin = 2;
+ vsync-len = 2;
+ hsync-active-high;
+ };
+
--
Regards,
Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] of: Add videomode helper
On Thu, Jul 05, 2012 at 04:08:07PM +0200, Laurent Pinchart wrote:
Hi Sascha,
Thanks for the patch.
+++ b/Documentation/devicetree/bindings/video/displaymode
@@ -0,0 +1,40 @@
+videomode bindings
+==
+
+Required properties:
+ - xres, yres: Display resolution
+ - left-margin, right-margin, hsync-len: Horizontal Display timing
parameters + in pixels
+ upper-margin, lower-margin, vsync-len: Vertical display timing
parameters in + lines
+ - clock: displayclock in Hz
+
+Optional properties:
+ - width-mm, height-mm: Display dimensions in mm
I've always had mixed feelings about the physical display dimension being
part
of the display mode. Those are properties of the panel/display instead of the
mode. Storing them as part of the mode can be convenient, but we then run
into
consistency issues (developers have to remember in which display mode
instances the values are available, and in which instances they're set to 0
for instance). If we want to clean this up, this patch would be a good
occasion.
This sounds like a display node with one or more node subnodes, like:
display {
width_mm = ;
height_mm = ;
mode {
xres = ;
yres = ;
...
};
};
Is that what you mean or are you thinking of something else?
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] of: Add videomode helper
On Thu, Jul 05, 2012 at 09:51:39AM -0500, Rob Herring wrote:
On 07/04/2012 02:56 AM, Sascha Hauer wrote:
+
+There are different ways of describing a display mode. The devicetree
representation
+corresponds to the one used by the Linux Framebuffer framework described
here in
+Documentation/fb/framebuffer.txt. This representation has been chosen
because it's
+the only format which does not allow for inconsistent parameters.Unlike
the Framebuffer
+framework the devicetree has the clock in Hz instead of ps.
This implies you are putting linux settings into DT rather than
describing the h/w. I'm not saying the binding is wrong, but documenting
it this way makes it seem so.
The major reason to use these values was that they do not allow for
inconsistent values (as opposed to for example with hsync_start which you
would have to check for hsync_start = xres).
I could rephrase this if it looks too much like modelled-after-Linux
instead of modelled-after-hardware.
One important piece missing (and IIRC linux doesn't really support) is
defining the pixel format of the interface.
I could use this aswell. I think this can be specified as additional
properties later, right? I'm afraid this needs a lot of discussion so
we should delay this to the next round.
+Example:
+
+ display@0 {
+ /* 1920x1080p24 */
+ clock = 5200;
Should this use the clock binding? You probably need both constraints
and clock binding though.
Is the clock binding suitable for this? Here we are not interested where
the clock comes from, but instead which range is allowed.
Often you don't know the frequency up front and/or have limited control
of the frequency (i.e. integer dividers). Then you have to adjust the
margins to get the desired refresh rate. To do that, you need to know
the ranges of values a panel can support. Perhaps you just assume you
can increase the right-margin and lower-margins as I think you will hit
pixel clock frequency max before any limit on margins.
Most datasheets specify min,typ,max triplets. We could do this instead
of using single fixed values for the margins:
left_margin = 0 10 40;
Right now we have nothing in the kernel that could handle this, but
getting the interface to the devicetree right seems indeed important.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] of: Add videomode helper
On 07/04/2012 02:56 AM, Sascha Hauer wrote:
This patch adds a helper function for parsing videomodes from the devicetree.
The videomode can be either converted to a struct drm_display_mode or a
struct fb_videomode.
Signed-off-by: Sascha Hauer s.ha...@pengutronix.de
---
changes since v1:
- use hyphens instead of underscores for property names
.../devicetree/bindings/video/displaymode | 40
drivers/of/Kconfig |5 +
drivers/of/Makefile|1 +
drivers/of/of_videomode.c | 108
include/linux/of_videomode.h | 19
5 files changed, 173 insertions(+)
create mode 100644 Documentation/devicetree/bindings/video/displaymode
create mode 100644 drivers/of/of_videomode.c
create mode 100644 include/linux/of_videomode.h
diff --git a/Documentation/devicetree/bindings/video/displaymode
b/Documentation/devicetree/bindings/video/displaymode
new file mode 100644
index 000..43cc17d
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/displaymode
@@ -0,0 +1,40 @@
+videomode bindings
+==
+
+Required properties:
+ - xres, yres: Display resolution
+ - left-margin, right-margin, hsync-len: Horizontal Display timing parameters
+ in pixels
+ upper-margin, lower-margin, vsync-len: Vertical display timing parameters
in
+ lines
+ - clock: displayclock in Hz
+
+Optional properties:
+ - width-mm, height-mm: Display dimensions in mm
+ - hsync-active-high (bool): Hsync pulse is active high
+ - vsync-active-high (bool): Vsync pulse is active high
+ - interlaced (bool): This is an interlaced mode
+ - doublescan (bool): This is a doublescan mode
+
+There are different ways of describing a display mode. The devicetree
representation
+corresponds to the one used by the Linux Framebuffer framework described
here in
+Documentation/fb/framebuffer.txt. This representation has been chosen
because it's
+the only format which does not allow for inconsistent parameters.Unlike the
Framebuffer
+framework the devicetree has the clock in Hz instead of ps.
This implies you are putting linux settings into DT rather than
describing the h/w. I'm not saying the binding is wrong, but documenting
it this way makes it seem so.
One important piece missing (and IIRC linux doesn't really support) is
defining the pixel format of the interface.
+Example:
+
+ display@0 {
+ /* 1920x1080p24 */
+ clock = 5200;
Should this use the clock binding? You probably need both constraints
and clock binding though.
Often you don't know the frequency up front and/or have limited control
of the frequency (i.e. integer dividers). Then you have to adjust the
margins to get the desired refresh rate. To do that, you need to know
the ranges of values a panel can support. Perhaps you just assume you
can increase the right-margin and lower-margins as I think you will hit
pixel clock frequency max before any limit on margins.
Rob
+ xres = 1920;
+ yres = 1080;
+ left-margin = 25;
+ right-margin = 25;
+ hsync-len = 25;
+ lower-margin = 2;
+ upper-margin = 2;
+ vsync-len = 2;
+ hsync-active-high;
+ };
+
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index dfba3e6..a3acaa3 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -83,4 +83,9 @@ config OF_MTD
depends on MTD
def_bool y
+config OF_VIDEOMODE
+ def_bool y
+ help
+ helper to parse videomodes from the devicetree
+
endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index e027f44..80e6db3 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_OF_MDIO) += of_mdio.o
obj-$(CONFIG_OF_PCI) += of_pci.o
obj-$(CONFIG_OF_PCI_IRQ) += of_pci_irq.o
obj-$(CONFIG_OF_MTD) += of_mtd.o
+obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
new file mode 100644
index 000..50d3bd2
--- /dev/null
+++ b/drivers/of/of_videomode.c
@@ -0,0 +1,108 @@
+/*
+ * OF helpers for parsing display modes
+ *
+ * Copyright (c) 2012 Sascha Hauer s.ha...@pengutronix.de, Pengutronix
+ *
+ * This file is released under the GPLv2
+ */
+#include linux/of.h
+#include linux/fb.h
+#include linux/export.h
+#include drm/drmP.h
+#include drm/drm_crtc.h
+
+int of_get_video_mode(struct device_node *np, struct drm_display_mode *dmode,
+ struct fb_videomode *fbmode)
+{
+ int ret = 0;
+ u32 left_margin, xres, right_margin, hsync_len;
+ u32 upper_margin, yres, lower_margin, vsync_len;
+ u32 width_mm = 0, height_mm = 0;
+ u32 clock;
+ bool hah = false, vah = false, interlaced = false, doublescan = false;
+
+ if (!np)
