Re: [PATCH 1/3] drm/panel: Add display_timing support

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2517 bytes --]

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

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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