On Tue, Aug 26, 2014 at 12:46:32PM +0200, Martin Bugge (marbugge) wrote: > Hello Damien > > I'm writing to you as you seems to be one of latest maintainers of: > > include/linux/hdmi.h > drivers/video/hdmi.c > > I wanted to add "Source Product Description information frame" > logging for the drivers in > > ./drivers/media/i2c/adv7604.c > ./drivers/media/i2c/adv7842.c > > But was advised to contact you and use some of the defines in > include/linux/hdmi.h > > As you can see in: > > http://www.spinics.net/lists/linux-media/msg74727.html > > Would you consider the addition of a hdmi_spd_infoframe_log function > in hdmi.c a good idea ? > > And the same goes for other HDMI Information frames. I don't see why not. Currently the code there is somewhat geared for writing HDMI info frames, not really reading back/debug. The various structures are not a 1:1 mapping with how the fields are actually represented in an info frame packet, so you get a _pack() function to serialize the info frame structures. Something to consider, then, for your logging API would be to either: - Give a buffer/size to your log() function and decode a raw buffer - make an _unpack() function that takes a raw buffer and translate it into one of the various infoframe structure and then have a _log function that operates on the "unpacked" structure. Another thing to keep in mind is that those "unpacked" versions of the infoframes don't actually have all the fields. eg. /* * Data byte 1, bit 4 has to be set if we provide the active format * aspect ratio */ if (frame->active_aspect & 0xf) ptr[0] |= BIT(4); [...] ptr[1] = ((frame->colorimetry & 0x3) << 6) | ((frame->picture_aspect & 0x3) << 4) | (frame->active_aspect & 0xf); You can see that active_aspect needs two fields set, a "active aspect valid" bit (data byte 1, bit 4) and the actual active_aspect value. To prevent the user of the API to generate invalid infoframes (ie have a active_aspect value but forgetting to set the valid bit), I decided to "hide" the valid bit and set it automatically if active_aspect is set. This isn't just theoritical, we've had the bug that forgot the valid bit before. That's the details I can think about atm, I don't seen any reason to not improve that code to be able to dump info frames. I have a slight preference to have an _unpack() version and have the _log() functions operate on the "expoded" structures, maybe Thierry/Paulo (in Cc.) have some input as well. HTH, -- Damien _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
