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

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

[Martin Bugge (marbugge)]

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.

Best regards
Martin Bugge

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

[Damien Lespiau]

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

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3720 bytes --]

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.
-----------------------------------------------------------------------------------

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel