Re: drivers/video/hdmi.c: Source Product Description (SPD) Information frame logging

[Damien Lespiau <damien.lespiau@intel.com>]

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