Many panel data sheets additionally to typical values list allowed ranges for timings such as hsync/vsync lengths, porches, and the pixel clock rate. These can be stored in a struct display_timing, to be used by an encoder mode_fixup callback to clamp user provided timing values or to validate workarounds for clock source limitations.
This patch adds a new drm_panel_funcs callback that returns the panels' available display_timing entries.
Signed-off-by: Philipp Zabel p.zabel@pengutronix.de --- include/drm/drm_panel.h | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 1fbcc96..13ff44b 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -29,6 +29,7 @@ struct drm_connector; struct drm_device; struct drm_panel; +struct display_timing;
/** * struct drm_panel_funcs - perform operations on a given panel @@ -38,6 +39,8 @@ struct drm_panel; * @enable: enable panel (turn on back light, etc.) * @get_modes: add modes to the connector that the panel is attached to and * return the number of modes added + * @get_timings: copy display timings into the provided array and return + * the number of display timings available * * The .prepare() function is typically called before the display controller * starts to transmit video data. Panel drivers can use this to turn the panel @@ -68,6 +71,8 @@ struct drm_panel_funcs { int (*prepare)(struct drm_panel *panel); int (*enable)(struct drm_panel *panel); int (*get_modes)(struct drm_panel *panel); + int (*get_timings)(struct drm_panel *panel, unsigned int num_timings, + struct display_timing *timings); };
struct drm_panel {
Many panel data sheets additionally to typical values list allowed ranges for timings such as hsync/vsync lengths, porches, and the pixel clock rate. This patch allows simple panels to store a struct display_timing in place of the struct drm_display_mode used before. The get_modes panel_ops callback calculates the drm_display_mode list from the typical timings and the get_timings callback returns the timings to the connected encoder for mode fixup and validation.
Signed-off-by: Philipp Zabel p.zabel@pengutronix.de --- drivers/gpu/drm/panel/panel-simple.c | 40 ++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index c4b6167..895af09 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -33,9 +33,14 @@ #include <drm/drm_mipi_dsi.h> #include <drm/drm_panel.h>
+#include <video/display_timing.h> +#include <video/videomode.h> + struct panel_desc { const struct drm_display_mode *modes; unsigned int num_modes; + const struct display_timing *timings; + unsigned int num_timings;
unsigned int bpc;
@@ -92,6 +97,25 @@ static int panel_simple_get_fixed_modes(struct panel_simple *panel) if (!panel->desc) return 0;
+ for (i = 0; i < panel->desc->num_timings; i++) { + const struct display_timing *dt = &panel->desc->timings[i]; + struct videomode vm; + + videomode_from_timing(dt, &vm); + mode = drm_mode_create(drm); + if (!mode) { + dev_err(drm->dev, "failed to add mode %ux%u\n", + dt->hactive.typ, dt->vactive.typ); + continue; + } + + drm_display_mode_from_videomode(&vm, mode); + drm_mode_set_name(mode); + + drm_mode_probed_add(connector, mode); + num++; + } + for (i = 0; i < panel->desc->num_modes; i++) { const struct drm_display_mode *m = &panel->desc->modes[i];
@@ -221,12 +245,28 @@ static int panel_simple_get_modes(struct drm_panel *panel) return num; }
+static int panel_simple_get_timings(struct drm_panel *panel, + unsigned int num_timings, + struct display_timing *timings) +{ + struct panel_simple *p = to_panel_simple(panel); + int i; + + if (timings) { + for (i = 0; i < min(num_timings, p->desc->num_timings); i++) + timings[i] = p->desc->timings[i]; + } + + return p->desc->num_timings; +} + static const struct drm_panel_funcs panel_simple_funcs = { .disable = panel_simple_disable, .unprepare = panel_simple_unprepare, .prepare = panel_simple_prepare, .enable = panel_simple_enable, .get_modes = panel_simple_get_modes, + .get_timings = panel_simple_get_timings, };
static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
The HannStar HSD070PWW1 LVDS panel data sheet lists allowed ranges additionally to the typical values for pixel clock rate (64.3 MHz ... 82 MHz) and blanking intervals (54 to 681 clocks horizontally, 3 to 23 lines vertically). This patch replaces this panels' drm_display_mode entry with a display_timing entry to describe acceptable timings. Since the HSYNC/VSYNC are unused, the distribution between front porches, back porches, and sync pulse lengths was chosen at will.
Signed-off-by: Philipp Zabel p.zabel@pengutronix.de --- drivers/gpu/drm/panel/panel-simple.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 895af09..47ecdca 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -600,22 +600,22 @@ static const struct panel_desc foxlink_fl500wvr00_a0t = { }, };
-static const struct drm_display_mode hannstar_hsd070pww1_mode = { - .clock = 71100, - .hdisplay = 1280, - .hsync_start = 1280 + 1, - .hsync_end = 1280 + 1 + 158, - .htotal = 1280 + 1 + 158 + 1, - .vdisplay = 800, - .vsync_start = 800 + 1, - .vsync_end = 800 + 1 + 21, - .vtotal = 800 + 1 + 21 + 1, - .vrefresh = 60, +static const struct display_timing hannstar_hsd070pww1_timing = { + .pixelclock = { 64300000, 71100000, 82000000 }, + .hactive = { 1280, 1280, 1280 }, + .hfront_porch = { 1, 1, 10 }, + .hback_porch = { 1, 1, 10 }, + .hsync_len = { 52, 158, 661 }, + .vactive = { 800, 800, 800 }, + .vfront_porch = { 1, 1, 10 }, + .vback_porch = { 1, 1, 10 }, + .vsync_len = { 1, 21, 203 }, + .flags = DISPLAY_FLAGS_DE_HIGH, };
static const struct panel_desc hannstar_hsd070pww1 = { - .modes = &hannstar_hsd070pww1_mode, - .num_modes = 1, + .timings = &hannstar_hsd070pww1_timing, + .num_timings = 1, .bpc = 6, .size = { .width = 151,
On Thu, Dec 11, 2014 at 06:32:44PM +0100, Philipp Zabel wrote:
Many panel data sheets additionally to typical values list allowed ranges for timings such as hsync/vsync lengths, porches, and the pixel clock rate. These can be stored in a struct display_timing, to be used by an encoder mode_fixup callback to clamp user provided timing values or to validate workarounds for clock source limitations.
This patch adds a new drm_panel_funcs callback that returns the panels' available display_timing entries.
Signed-off-by: Philipp Zabel p.zabel@pengutronix.de
include/drm/drm_panel.h | 5 +++++ 1 file changed, 5 insertions(+)
Sorry for taking so long to get back on this. I generally like the idea, though a couple of things are unclear to me.
In struct display_timing we have:
struct timing_entry hactive; ... struct timing_entry vactive;
I wonder if that really makes sense. Are there really datasheets that provide a valid range for the number of horizontal and vertical active pixels?
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 1fbcc96..13ff44b 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -29,6 +29,7 @@ struct drm_connector; struct drm_device; struct drm_panel; +struct display_timing;
/**
- struct drm_panel_funcs - perform operations on a given panel
@@ -38,6 +39,8 @@ struct drm_panel;
- @enable: enable panel (turn on back light, etc.)
- @get_modes: add modes to the connector that the panel is attached to and
- return the number of modes added
- @get_timings: copy display timings into the provided array and return
- the number of display timings available
- The .prepare() function is typically called before the display controller
- starts to transmit video data. Panel drivers can use this to turn the panel
@@ -68,6 +71,8 @@ struct drm_panel_funcs { int (*prepare)(struct drm_panel *panel); int (*enable)(struct drm_panel *panel); int (*get_modes)(struct drm_panel *panel);
- int (*get_timings)(struct drm_panel *panel, unsigned int num_timings,
struct display_timing *timings);
Also are there even panels that support more than one set of timings? I've never heard of panels that are actually able to do more than one mode, so I'm wondering if num_timings isn't overdoing it a little here. I guess it doesn't hurt all that much, though.
Thierry
Hi Thierry,
Am Dienstag, den 03.02.2015, 14:30 +0100 schrieb Thierry Reding:
On Thu, Dec 11, 2014 at 06:32:44PM +0100, Philipp Zabel wrote:
Many panel data sheets additionally to typical values list allowed ranges for timings such as hsync/vsync lengths, porches, and the pixel clock rate. These can be stored in a struct display_timing, to be used by an encoder mode_fixup callback to clamp user provided timing values or to validate workarounds for clock source limitations.
This patch adds a new drm_panel_funcs callback that returns the panels' available display_timing entries.
Signed-off-by: Philipp Zabel p.zabel@pengutronix.de
include/drm/drm_panel.h | 5 +++++ 1 file changed, 5 insertions(+)
Sorry for taking so long to get back on this. I generally like the idea, though a couple of things are unclear to me.
In struct display_timing we have:
struct timing_entry hactive; ... struct timing_entry vactive;
I wonder if that really makes sense. Are there really datasheets that provide a valid range for the number of horizontal and vertical active pixels?
I often see datasheets that give minimum, typical and maximum values also for the horizontal and vertical active pixels, but those are usually the same value. I don't know if there are any panels that really have documented ranges here.
[...]
@@ -68,6 +71,8 @@ struct drm_panel_funcs { int (*prepare)(struct drm_panel *panel); int (*enable)(struct drm_panel *panel); int (*get_modes)(struct drm_panel *panel);
- int (*get_timings)(struct drm_panel *panel, unsigned int num_timings,
struct display_timing *timings);Also are there even panels that support more than one set of timings? I've never heard of panels that are actually able to do more than one mode, so I'm wondering if num_timings isn't overdoing it a little here. I guess it doesn't hurt all that much, though.
I have not seen any yet that reasonably should be driven by the simple-panel driver. I was thinking about the Pixel Qi panel, but it turns out those just have a single timing and use the subpixels in B/W reflective mode.
regards Philipp
Hi Thierry,
do you have any further thoughts on this?
Am Dienstag, den 03.02.2015, 14:30 +0100 schrieb Thierry Reding:
On Thu, Dec 11, 2014 at 06:32:44PM +0100, Philipp Zabel wrote:
Many panel data sheets additionally to typical values list allowed ranges for timings such as hsync/vsync lengths, porches, and the pixel clock rate. These can be stored in a struct display_timing, to be used by an encoder mode_fixup callback to clamp user provided timing values or to validate workarounds for clock source limitations.
This patch adds a new drm_panel_funcs callback that returns the panels' available display_timing entries.
Signed-off-by: Philipp Zabel p.zabel@pengutronix.de
include/drm/drm_panel.h | 5 +++++ 1 file changed, 5 insertions(+)
Sorry for taking so long to get back on this. I generally like the idea, though a couple of things are unclear to me.
In struct display_timing we have:
struct timing_entry hactive; ... struct timing_entry vactive;
I wonder if that really makes sense. Are there really datasheets that provide a valid range for the number of horizontal and vertical active pixels?
I could send a patch to change hactive/vactive to a single value and change Documentation/devicetree/bindings/video/display-timing.txt to clarify ranges are not allowed for these properties until somebody digs out a panel that actually needs this. Adding Steffen to Cc: in case there was a reason other than symmetry.
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 1fbcc96..13ff44b 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -29,6 +29,7 @@ struct drm_connector; struct drm_device; struct drm_panel; +struct display_timing;
/**
- struct drm_panel_funcs - perform operations on a given panel
@@ -38,6 +39,8 @@ struct drm_panel;
- @enable: enable panel (turn on back light, etc.)
- @get_modes: add modes to the connector that the panel is attached to and
- return the number of modes added
- @get_timings: copy display timings into the provided array and return
- the number of display timings available
- The .prepare() function is typically called before the display controller
- starts to transmit video data. Panel drivers can use this to turn the panel
@@ -68,6 +71,8 @@ struct drm_panel_funcs { int (*prepare)(struct drm_panel *panel); int (*enable)(struct drm_panel *panel); int (*get_modes)(struct drm_panel *panel);
- int (*get_timings)(struct drm_panel *panel, unsigned int num_timings,
struct display_timing *timings);Also are there even panels that support more than one set of timings? I've never heard of panels that are actually able to do more than one mode, so I'm wondering if num_timings isn't overdoing it a little here. I guess it doesn't hurt all that much, though.
Would you prefer struct display_timing *(*get_timing)(struct drm_panel *panel); ?
regards Philipp
Hi Philipp,
On Mon, 23 Feb 2015 15:04:32 +0100 Philipp Zabel p.zabel@pengutronix.de wrote:
Hi Thierry,
do you have any further thoughts on this?
Am Dienstag, den 03.02.2015, 14:30 +0100 schrieb Thierry Reding:
On Thu, Dec 11, 2014 at 06:32:44PM +0100, Philipp Zabel wrote:
Many panel data sheets additionally to typical values list allowed ranges for timings such as hsync/vsync lengths, porches, and the pixel clock rate. These can be stored in a struct display_timing, to be used by an encoder mode_fixup callback to clamp user provided timing values or to validate workarounds for clock source limitations.
This patch adds a new drm_panel_funcs callback that returns the panels' available display_timing entries.
Anthony recently noticed that the edt_etm0700g0dh6 panel he's connecting on his sama5 board defines typical timings that are not supported by Atmel's display controller. This patch could help us deal with such cases. Could you keep me (and Anthony) in Cc of your future versions ?
Thanks,
Boris
Hi Boris,
Am Donnerstag, den 26.02.2015, 14:51 +0100 schrieb Boris Brezillon:
Hi Philipp,
On Mon, 23 Feb 2015 15:04:32 +0100 Philipp Zabel p.zabel@pengutronix.de wrote:
Hi Thierry,
do you have any further thoughts on this?
Am Dienstag, den 03.02.2015, 14:30 +0100 schrieb Thierry Reding:
On Thu, Dec 11, 2014 at 06:32:44PM +0100, Philipp Zabel wrote:
Many panel data sheets additionally to typical values list allowed ranges for timings such as hsync/vsync lengths, porches, and the pixel clock rate. These can be stored in a struct display_timing, to be used by an encoder mode_fixup callback to clamp user provided timing values or to validate workarounds for clock source limitations.
This patch adds a new drm_panel_funcs callback that returns the panels' available display_timing entries.
Anthony recently noticed that the edt_etm0700g0dh6 panel he's connecting on his sama5 board defines typical timings that are not supported by Atmel's display controller. This patch could help us deal with such cases. Could you keep me (and Anthony) in Cc of your future versions ?
Will do.
regards Philipp
Hi Thierry,
Am Montag, den 23.02.2015, 15:04 +0100 schrieb Philipp Zabel:
Hi Thierry,
do you have any further thoughts on this?
[...]
Sorry for taking so long to get back on this. I generally like the idea, though a couple of things are unclear to me.
In struct display_timing we have:
struct timing_entry hactive; ... struct timing_entry vactive;
I wonder if that really makes sense. Are there really datasheets that provide a valid range for the number of horizontal and vertical active pixels?
I could send a patch to change hactive/vactive to a single value and change Documentation/devicetree/bindings/video/display-timing.txt to clarify ranges are not allowed for these properties until somebody digs out a panel that actually needs this. Adding Steffen to Cc: in case there was a reason other than symmetry.
That seems not to be the case so far.
[...]
/**
- struct drm_panel_funcs - perform operations on a given panel
@@ -38,6 +39,8 @@ struct drm_panel;
- @enable: enable panel (turn on back light, etc.)
- @get_modes: add modes to the connector that the panel is attached to and
- return the number of modes added
- @get_timings: copy display timings into the provided array and return
- the number of display timings available
- The .prepare() function is typically called before the display controller
- starts to transmit video data. Panel drivers can use this to turn the panel
@@ -68,6 +71,8 @@ struct drm_panel_funcs { int (*prepare)(struct drm_panel *panel); int (*enable)(struct drm_panel *panel); int (*get_modes)(struct drm_panel *panel);
- int (*get_timings)(struct drm_panel *panel, unsigned int num_timings,
struct display_timing *timings);Also are there even panels that support more than one set of timings? I've never heard of panels that are actually able to do more than one mode, so I'm wondering if num_timings isn't overdoing it a little here. I guess it doesn't hurt all that much, though.
Would you prefer struct display_timing *(*get_timing)(struct drm_panel *panel); ?
I'd like to resend this. Please let me know if you want me to change this function prototype.
regards Philipp
On Tue, Mar 03, 2015 at 12:49:43PM +0100, Philipp Zabel wrote:
Hi Thierry,
Am Montag, den 23.02.2015, 15:04 +0100 schrieb Philipp Zabel:
Hi Thierry,
do you have any further thoughts on this?
[...]
Sorry for taking so long to get back on this. I generally like the idea, though a couple of things are unclear to me.
In struct display_timing we have:
struct timing_entry hactive; ... struct timing_entry vactive;
I wonder if that really makes sense. Are there really datasheets that provide a valid range for the number of horizontal and vertical active pixels?
I could send a patch to change hactive/vactive to a single value and change Documentation/devicetree/bindings/video/display-timing.txt to clarify ranges are not allowed for these properties until somebody digs out a panel that actually needs this. Adding Steffen to Cc: in case there was a reason other than symmetry.
That seems not to be the case so far.
[...]
/**
- struct drm_panel_funcs - perform operations on a given panel
@@ -38,6 +39,8 @@ struct drm_panel;
- @enable: enable panel (turn on back light, etc.)
- @get_modes: add modes to the connector that the panel is attached to and
- return the number of modes added
- @get_timings: copy display timings into the provided array and return
- the number of display timings available
- The .prepare() function is typically called before the display controller
- starts to transmit video data. Panel drivers can use this to turn the panel
@@ -68,6 +71,8 @@ struct drm_panel_funcs { int (*prepare)(struct drm_panel *panel); int (*enable)(struct drm_panel *panel); int (*get_modes)(struct drm_panel *panel);
- int (*get_timings)(struct drm_panel *panel, unsigned int num_timings,
struct display_timing *timings);Also are there even panels that support more than one set of timings? I've never heard of panels that are actually able to do more than one mode, so I'm wondering if num_timings isn't overdoing it a little here. I guess it doesn't hurt all that much, though.
Would you prefer struct display_timing *(*get_timing)(struct drm_panel *panel); ?
I'd like to resend this. Please let me know if you want me to change this function prototype.
I have no objections to keeping the current prototype. It's something we can always fixup if we want to. Also keeping the symmetry with min/max values for hactive and vactive is okay in my opinion.
Were there any other remaining points? If not I'll just apply this as is.
Thierry
Am Dienstag, den 24.03.2015, 12:34 +0100 schrieb Thierry Reding: [...]
Would you prefer struct display_timing *(*get_timing)(struct drm_panel *panel); ?
I'd like to resend this. Please let me know if you want me to change this function prototype.
I have no objections to keeping the current prototype. It's something we can always fixup if we want to. Also keeping the symmetry with min/max values for hactive and vactive is okay in my opinion.
Were there any other remaining points? If not I'll just apply this as is.
No, I'm happy if you apply this as is.
thanks Philipp
On Tue, Mar 24, 2015 at 12:52:44PM +0100, Philipp Zabel wrote:
Am Dienstag, den 24.03.2015, 12:34 +0100 schrieb Thierry Reding: [...]
Would you prefer struct display_timing *(*get_timing)(struct drm_panel *panel); ?
I'd like to resend this. Please let me know if you want me to change this function prototype.
I have no objections to keeping the current prototype. It's something we can always fixup if we want to. Also keeping the symmetry with min/max values for hactive and vactive is okay in my opinion.
Were there any other remaining points? If not I'll just apply this as is.
No, I'm happy if you apply this as is.
Done. I touched up a couple of things in the commit messages and made a minor change to the loop which copies the timings in the simple-panel driver (extract the min(num_timings, p->desc->num_timings) out of the for statement). Nothing major, but it might still be good if you could test, just to make sure I didn't make a mess.
This should be in tomorrow's linux-next, but if you want to take a peek before that, you can grab it from here:
git://anongit.freedesktop.org/tegra/linux.git#drm/panel/for-next
Thierry
Hi Thierry,
Am Dienstag, den 24.03.2015, 13:40 +0100 schrieb Thierry Reding:
On Tue, Mar 24, 2015 at 12:52:44PM +0100, Philipp Zabel wrote:
Am Dienstag, den 24.03.2015, 12:34 +0100 schrieb Thierry Reding: [...]
Would you prefer struct display_timing *(*get_timing)(struct drm_panel *panel); ?
I'd like to resend this. Please let me know if you want me to change this function prototype.
I have no objections to keeping the current prototype. It's something we can always fixup if we want to. Also keeping the symmetry with min/max values for hactive and vactive is okay in my opinion.
Were there any other remaining points? If not I'll just apply this as is.
No, I'm happy if you apply this as is.
Done. I touched up a couple of things in the commit messages and made a minor change to the loop which copies the timings in the simple-panel driver (extract the min(num_timings, p->desc->num_timings) out of the for statement). Nothing major, but it might still be good if you could test, just to make sure I didn't make a mess.
Thanks for the cleanup, it still seems to work well.
regards Philipp
dri-devel@lists.freedesktop.org
-
Boris Brezillon -
Philipp Zabel -
Thierry Reding