From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: drivers/video/hdmi.c: Source Product Description (SPD) Information frame logging Date: Mon, 1 Sep 2014 09:18:15 +0200 Message-ID: <20140901071814.GB24226@ulmo.nvidia.com> References: <53FC6588.1060009@cisco.com> <20140826133102.GF32146@strange.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1219227697==" Return-path: Received: from hqemgate16.nvidia.com (hqemgate16.nvidia.com [216.228.121.65]) by gabe.freedesktop.org (Postfix) with ESMTP id 57CD26E277 for ; Mon, 1 Sep 2014 00:18:41 -0700 (PDT) In-Reply-To: <20140826133102.GF32146@strange.ger.corp.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Damien Lespiau Cc: "Martin Bugge (marbugge)" , "paulo.r.zanoni@intel.com" , "dri-devel@lists.freedesktop.org" List-Id: dri-devel@lists.freedesktop.org --===============1219227697== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="Pd0ReVV5GZGQvF3a" Content-Disposition: inline --Pd0ReVV5GZGQvF3a Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 > >=20 > > I'm writing to you as you seems to be one of latest maintainers of: > >=20 > > include/linux/hdmi.h > > drivers/video/hdmi.c > >=20 > > I wanted to add "Source Product Description information frame" > > logging for the drivers in > >=20 > > ./drivers/media/i2c/adv7604.c > > ./drivers/media/i2c/adv7842.c > >=20 > > But was advised to contact you and use some of the defines in > > include/linux/hdmi.h > >=20 > > As you can see in: > >=20 > > http://www.spinics.net/lists/linux-media/msg74727.html > >=20 > > Would you consider the addition of a hdmi_spd_infoframe_log function > > in hdmi.c a good idea ? > >=20 > > And the same goes for other HDMI Information frames. >=20 > I don't see why not. Currently the code there is somewhat geared for > writing HDMI info frames, not really reading back/debug. >=20 > 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: >=20 > - 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. >=20 > Another thing to keep in mind is that those "unpacked" versions of the > infoframes don't actually have all the fields. eg. >=20 > /* > * Data byte 1, bit 4 has to be set if we provide the active fo= rmat > * aspect ratio > */ > if (frame->active_aspect & 0xf) > ptr[0] |=3D BIT(4); >=20 > [...] >=20 > ptr[1] =3D ((frame->colorimetry & 0x3) << 6) | > ((frame->picture_aspect & 0x3) << 4) | > (frame->active_aspect & 0xf); >=20 > You can see that active_aspect needs two fields set, a "active aspect v= alid" > bit (data byte 1, bit 4) and the actual active_aspect value. To prevent= =20the > user of the API to generate invalid infoframes (ie have a active_aspect= =20value > but forgetting to set the valid bit), I decided to "hide" the valid bit= =20and set > it automatically if active_aspect is set. This isn't just theoritical, = we've > had the bug that forgot the valid bit before. >=20 > That's the details I can think about atm, I don't seen any reason to no= t > improve that code to be able to dump info frames. I have a slight prefe= rence to > have an _unpack() version and have the _log() functions operate on the > "expoded" structures, maybe Thierry/Paulo (in Cc.) have some input as w= ell. 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 m= ay contain confidential information. Any unauthorized review, use, disclosure or di= stribution is prohibited. If you are not the intended recipient, please contact the= =20sender by reply email and destroy all copies of the original message. -------------------------------------------------------------------------= ---------- --Pd0ReVV5GZGQvF3a Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJUBB22AAoJEN0jrNd/PrOhmPcP/jR/XImZDwPfyRIzjOJFgiM4 sM7t7SpaDKZUyYnIpHqc3c76lMs2x/g4HRO4TF/Jq2PeG4MZ4gBFTzeCz/f8AO2c XaGIRKQ1be2IWFN0BOG994ehM+mPyojul6LyGIJBM31iHI2NixX4CH3a82Kt46ke Zn1Q6Wd/GQtwX+wF9RCw6om/sO8fj3gDW/LmR60IMaUlg3N7PGs1ZJRRLgQ5cWmB 8KNIOWAdrgkgh+pb7VDsc49a2+XOqdUQIxyC91V683TTU8IgMbVWFqyzYogvxXbg sn637xzTyv1Yom159KP157HW12dTMiEgOacc0IzvdOr4HC7o51wTRoReo0eUEsEP 8EmZ3VWamTUv/le9DSOYO362YkAzYO3Qujuy2XRAStbhREfT3I2OOeen/GhuwWdj Ig/2NSeeaR7q8uaIbIEqBQWQdRVUMZtCSFiGLZeNSs1Q3Gu6n3zkOk546jzgUM93 c7Nj2uUyN/GOKPhaKxm7P6WW+UczZEEGBPB+Bjii/TjDZxY4u6qJUJ00zMJJxIcB yxpSWZfDLMwre3FHa4n5psO6VP0BkjUTKPVKuPtosreb4T6Ebmo4P9IYApE1mXTK eK7kOV5Tx3z6uo6P0bTZxJVwKwqQqsiLdGadbNoQQHebQdVegxGTiObty1QIYieq xUc0Bp/HRC8xrEynmShz =otwe -----END PGP SIGNATURE----- --Pd0ReVV5GZGQvF3a-- --===============1219227697== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel --===============1219227697==--