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,