Re: [PATCH v2] of: Add videomode helper

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Sascha,

On Friday 03 August 2012 09:38:44 Sascha Hauer wrote:
> On Thu, Aug 02, 2012 at 01:35:40PM -0600, Stephen Warren wrote:
> > On 07/04/2012 01:56 AM, Sascha Hauer wrote:
> > > This patch adds a helper function for parsing videomodes from the
> > > devicetree. The videomode can be either converted to a struct
> > > drm_display_mode or a struct fb_videomode.
> > > 
> > > diff --git a/Documentation/devicetree/bindings/video/displaymode
> > > b/Documentation/devicetree/bindings/video/displaymode
> > > 
> > > +Required properties:
> > > + - xres, yres: Display resolution
> > > + - left-margin, right-margin, hsync-len: Horizontal Display timing
> > > parameters +   in pixels
> > > +   upper-margin, lower-margin, vsync-len: Vertical display timing
> > > parameters in +   lines
> > 
> > Perhaps bike-shedding, but...
> > 
> > For the margin property names, wouldn't it be better to use terminology
> > more commonly used for video timings rather than Linux FB naming. In
> > other words naming like:
> > 
> > hactive, hsync-len, hfront-porch, hback-porch?
> 
> Can do. Just to make sure:
> 
> hactive = xres
> hsync-len = hsync-len
> hfront-porch = right-margin
> hback-porch = left-margin

That's correct. On the vertical direction, vfront-porch = lower-margin and 
vback-porch = upper-margin.

> 
> ?
> 
> > This node appears to describe a video mode, not a display, hence the
> > node name seems wrong.
> > 
> > Many displays can support multiple different video modes. As mentioned
> > elsewhere, properties like display width/height are properties of the
> > display not the mode.
> > 
> > So, I might expect something more like the following (various overhead
> > properties like reg/#address-cells etc. elided for simplicity):
> > 
> > disp: display {
> > 
> >     width-mm = <...>;
> >     height-mm = <...>;
> >     modes {
> >     
> >         mode@0 {
> > 		
> > 		/* 1920x1080p24 */
> > 		clock = <52000000>;
> > 		xres = <1920>;
> > 		yres = <1080>;
> > 		left-margin = <25>;
> > 		right-margin = <25>;
> > 		hsync-len = <25>;
> > 		lower-margin = <2>;
> > 		upper-margin = <2>;
> > 		vsync-len = <2>;
> > 		hsync-active-high;
> > 		
> >         };
> >         mode@1 {
> >         };
> >     
> >     };
> > 
> > };
> 
> Ok, we can do this.
> 
> > display-connector {
> > 
> >     display = <&disp>;
> >     // interface-specific properties such as pixel format here
> > 
> > };
> > 
> > Finally, have you considered just using an EDID instead of creating
> > something custom? I know that creating an EDID is harder than writing a
> 
> > few simple properties into a DT, but EDIDs have the following advantages:
> I have considered using EDID and I also tried it. It's painful. There
> are no (open) tools available for creating EDID. That's something we
> could change of course. Then when generating a devicetree there is
> always an extra step involved creating the EDID blob. Once the EDID
> blob is in devicetree it is not parsable anymore by mere humans, so
> to see what we've got there is another tool involved to generate a
> readable form again.
> 
> > a) They're already standardized and very common.
> 
> Indeed, that's a big plus for EDID. I have no intention of removing EDID
> data from the devicetree. There are situations where EDID is handy, for
> example when you get EDID data provided by your vendor.
> 
> > b) They allow other information such as a display's HDMI audio
> > capabilities to be represented, which can then feed into an ALSA driver.
> > 
> > c) The few LCD panel datasheets I've seen actually quote their
> > specification as an EDID already, so deriving the EDID may actually be
> > easy.
> > 
> > d) People familiar with displays are almost certainly familiar with
> > EDID's mode representation. There are many ways of representing display
> > modes (sync position vs. porch widths, htotal specified rather than
> > specifying all the components and hence htotal being calculated etc.).
> > Not everyone will be familiar with all representations. Conversion
> > errors are less likely if the target is EDID's familiar format.
> 
> You seem to think about a different class of displays for which EDID
> indeed is a better way to handle. What I have to deal with here mostly
> are dumb displays which:
> 
> - can only handle their native resolution
> - Have no audio capabilities at all
> - come with a datasheet which specify a min/typ/max triplet for
>   xres,hsync,..., often enough they are scanned to pdf from some previously
>   printed paper.
> 
> These displays are very common on embedded devices, probably that's the
> reason I did not even think about the possibility that a single display
> might have different modes.
> 
> > e) You'll end up with exactly the same data as if you have a DDC-based
> > external monitor rather than an internal panel, so you end up getting to
> > a common path in display handling code much more quickly.
> 
> All we have in our display driver currently is:
> 
> 	edidp = of_get_property(np, "edid", &imxpd->edid_len);
> 	if (edidp) {
> 		imxpd->edid = kmemdup(edidp, imxpd->edid_len, GFP_KERNEL);
> 	} else {
> 		ret = of_get_video_mode(np, &imxpd->mode, NULL);
> 		if (!ret)
> 			imxpd->mode_valid = 1;
> 	}

-- 
Regards,

Laurent Pinchart