On Fri, Nov 23, 2012 at 10:24:22AM +0100, Christian König wrote: > Hi Thierry, > > On 21.11.2012 16:01, Thierry Reding wrote: > >This small series is very much work in progress, but I still wanted to > >get feedback in this early stage to gather requirements from the folks > >working on the display drivers that these helpers target. > > > >Patch 1 in the series adds a generic helper to pack a structure that > >describes an HDMI AVI infoframe into the binary format as specified in > >the HDMI specification. The resulting binary buffer should be easily > >programmable into the HDMI controller. > > > >Patch 2 provides a helper to fill an HDMI AVI infoframe with data from > >a struct drm_display_mode. > > > >This is all pretty rough right now, but I think some feedback would be > >good at this point, to see if the design is at all sensible. I should > >also mention that I haven't actually tested this on real hardware yet. > >Furthermore I have plans to add something similar for the other types > >of infoframes specified by HDMI once the direction becomes clearer. > In general I like the idea of storing the informations in a C struct > and only packing it into the binary form when needed. > > I would rather like to see a complete implementation of all the > interesting HDMI packets, including the necessary > calculations/tables for audio timing recovery etc before it gets > committed upstream. I'm afraid I'm not very familiar with those packets. Also I haven't seen any actual users of audio timing recovery packets so far, so I'm not sure that adding support for them would be very useful. What I have added so far is support for audio and vendor-specific infoframes, since those have actual users. I also noticed that i915 implements SPD infoframes as well, so I can extract that code into the generic helpers as well. > Not sure about the separate configuration option. I'm not so much > into the config/build system of linux (I know that it is rather > complicated), but in general I would like to see that activated > automatically as soon as any driver starts using it (or at least the > driver depending on that option to be active). The configuration option isn't supposed to be user visible. Drivers that make use of this are supposed to select HDMI or DRM_HDMI to cause the generic code to be pulled in. > Also two notes about the code in itself: > 1. hdmi_avi_infoframe_pack: gets the size of the target buffer, but > unfortunately doesn't checks it. That should already be fixed in the latest code in my local working branch. > 2. Separate the CRC calculation, we probably need that more than once. I've also pulled that out into a separate function. I haven't made it a public function yet, but I don't think that'll be necessary either since it is used internally by the various infoframe packing functions. Thierry
Attachment:
pgp3h9vutsbEF.pgp
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
