Hi Grant, On Thu, Nov 15, 2012 at 06:03:59PM +0000, Grant Likely wrote: > On Thu, 15 Nov 2012 17:00:57 +0100, Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> wrote: > > Hi Grant, > > > > On Thursday 15 November 2012 15:47:53 Grant Likely wrote: > > > On Thu, 15 Nov 2012 10:23:52 +0100, Steffen Trumtrar wrote: > > > > Add display_timing structure and the according helper functions. This > > > > allows the description of a display via its supported timing parameters. > > > > > > > > Every timing parameter can be specified as a single value or a range > > > > <min typ max>. > > > > > > > > Also, add helper functions to convert from display timings to a generic > > > > videomode structure. This videomode can then be converted to the > > > > corresponding subsystem mode representation (e.g. fb_videomode). > > > > > > > > Signed-off-by: Steffen Trumtrar <s.trumtrar@xxxxxxxxxxxxxx> > > > > > > Hmmm... here's my thoughts as an outside reviewer. Correct me if I'm > > > making an incorrect assumption. > > > > > > It looks to me that the purpose of this entire series is to decode video > > > timings from the device tree and (eventually) provide the data in the > > > form 'struct videomode'. Correct? > > > For the time being it is straight from devicetree via struct videomode to struct drm_display_mode or fb_videomode. Correct. > > > If so, then it looks over engineered. Creating new infrastructure to > > > allocate, maintain, and free a new 'struct display_timings' doesn't make > > > any sense when it is an intermediary data format that will never be used > > > by drivers. > > > > > > Can the DT parsing code instead return a table of struct videomode? > > > See below. > > > But, wait... struct videomode is also a new structure. So it looks like > > > this series creates two new intermediary data structures; > > > display_timings and videomode. And at least as far as I can see in this > > > series struct fb_videomode is the only user. struct drm_display_mode is also a user in this series see 5/6 and 6/6. > > > > struct videomode is supposed to slowly replace the various video mode > > structures we currently have in the kernel (struct drm_mode_modeinfo, struct > > fb_videomode and struct v4l2_bt_timings), at least where possible (userspace > > APIs can't be broken). This will make it possible to reuse code across the > > DRM, FB and V4L2 subsystems, such as the EDID parser or HDMI encoder drivers. > > This rationale might not be clearly explained in the commit message, but > > having a shared video mode structure is pretty important. > That. > Okay that make sense. What about struct display_timings? > The reason for defining an intermediary step is because of the different things that are described: - struct display_timing describes the signal ranges a display supports - struct display_timings describes all timing settings of a display - struct videomode describes one single mode generated from that settings It is possible to generate multiple struct videomodes from one struct display_timing based on the circumstances. And that is a task for the driver using the display_timing infos. This means drivers are supposed to use struct display_timings if they need to generate a struct videomode from the timing ranges of one entry. This is just the first step in that direction. I hope this makes the need for struct display_timings a little clearer. The other solution would be the one Laurent suggested and pass multiple values around. Which in my opinion doesn't make it better, more practical or cleaner. Regards, Steffen -- 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-5555 | _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
