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

[Philipp Zabel <p.zabel@pengutronix.de>]

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

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