From mboxrd@z Thu Jan 1 00:00:00 1970 From: Damien Lespiau Subject: Re: drivers/video/hdmi.c: Source Product Description (SPD) Information frame logging Date: Tue, 26 Aug 2014 14:31:02 +0100 Message-ID: <20140826133102.GF32146@strange.ger.corp.intel.com> References: <53FC6588.1060009@cisco.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 91BDE6E50B for ; Tue, 26 Aug 2014 06:31:05 -0700 (PDT) Content-Disposition: inline In-Reply-To: <53FC6588.1060009@cisco.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: "Martin Bugge (marbugge)" Cc: treding@nvidia.com, paulo.r.zanoni@intel.com, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org 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