* drivers/video/hdmi.c: Source Product Description (SPD) Information frame logging @ 2014-08-26 10:46 Martin Bugge (marbugge) 2014-08-26 13:31 ` Damien Lespiau 0 siblings, 1 reply; 3+ messages in thread From: Martin Bugge (marbugge) @ 2014-08-26 10:46 UTC (permalink / raw) To: Damien; +Cc: dri-devel 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: drivers/video/hdmi.c: Source Product Description (SPD) Information frame logging 2014-08-26 10:46 drivers/video/hdmi.c: Source Product Description (SPD) Information frame logging Martin Bugge (marbugge) @ 2014-08-26 13:31 ` Damien Lespiau 2014-09-01 7:18 ` Thierry Reding 0 siblings, 1 reply; 3+ messages in thread From: Damien Lespiau @ 2014-08-26 13:31 UTC (permalink / raw) To: Martin Bugge (marbugge); +Cc: treding, paulo.r.zanoni, dri-devel 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: drivers/video/hdmi.c: Source Product Description (SPD) Information frame logging 2014-08-26 13:31 ` Damien Lespiau @ 2014-09-01 7:18 ` Thierry Reding 0 siblings, 0 replies; 3+ messages in thread From: Thierry Reding @ 2014-09-01 7:18 UTC (permalink / raw) To: Damien Lespiau; +Cc: Martin Bugge (marbugge), paulo.r.zanoni, dri-devel [-- 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-09-01 7:18 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-08-26 10:46 drivers/video/hdmi.c: Source Product Description (SPD) Information frame logging Martin Bugge (marbugge) 2014-08-26 13:31 ` Damien Lespiau 2014-09-01 7:18 ` Thierry Reding
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.