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@xxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
