On Tue, Aug 26, 2014 at 03:31:02PM +0200, Damien Lespiau wrote:

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.

I do have a preference for adding an _unpack() variant, too. Although as you pointed out there could be difficulties in reconstructing that from the raw data given that we don't have a 1:1 encoding. For example, what does the _unpack() do if the active aspect is set but not the valid bit?

One advantage perhaps would be that _unpack()/_pack() could act as a sanitizer.

Thierry

----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. -----------------------------------------------------------------------------------