From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH v3 3/9] video: Add generic HDMI infoframe helpers Date: Fri, 18 Jan 2013 12:50:04 +0200 Message-ID: <20130118105004.GW3503@intel.com> References: <1358173828-31674-1-git-send-email-thierry.reding@avionic-design.de> <1358173828-31674-4-git-send-email-thierry.reding@avionic-design.de> <20130116132714.GM3503@intel.com> <20130118085636.GA16395@avionic-0098.adnet.avionic-design.de> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTP id 7A44AE6380 for ; Fri, 18 Jan 2013 02:50:08 -0800 (PST) Content-Disposition: inline In-Reply-To: <20130118085636.GA16395@avionic-0098.adnet.avionic-design.de> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Thierry Reding Cc: Paulo Zanoni , dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Fri, Jan 18, 2013 at 09:56:36AM +0100, Thierry Reding wrote: > On Wed, Jan 16, 2013 at 03:27:14PM +0200, Ville Syrj=E4l=E4 wrote: > > On Mon, Jan 14, 2013 at 03:30:22PM +0100, Thierry Reding wrote: > > > Add generic helpers to pack HDMI infoframes into binary buffers. > [...] > > > +/** > > > + * hdmi_avi_infoframe_init() - initialize an HDMI AVI infoframe > > > + * @frame: HDMI AVI infoframe > > > + * > > > + * Returns 0 on success or a negative error code on failure. > > > + */ > > > +int hdmi_avi_infoframe_init(struct hdmi_avi_infoframe *frame) > > > +{ > > > + if (!frame) > > > + return -EINVAL; > > = > > There's quite a bit of error checking all around which seems a bit > > pointless since this is all internal stuff. Any errors are bugs in > > some driver code, so just letting the thing oops would be fine in my > > opinion. > = > Okay, I can remove some of the more obvious checks. Checking for space > requirements might still be useful, so I'll leave that in. > = > > > +ssize_t hdmi_spd_infoframe_pack(struct hdmi_spd_infoframe *frame, vo= id *buffer, > > > + size_t size) > [...] > > > + for (i =3D 0; i < sizeof(frame->vendor); i++) > > > + ptr[i] =3D frame->vendor[i]; > > > + > > > + for (i =3D 0; i < sizeof(frame->product); i++) > > > + ptr[8 + i] =3D frame->product[i]; > > = > > memcpy() > = > Done. > = > > > +enum hdmi_spd_sdi { > > > + HDMI_SPD_SDI_UNKNOWN, > > > + HDMI_SPD_SDI_DSTB, > > > + HDMI_SPD_SDI_DVDP, > > > + HDMI_SPD_SDI_DVHS, > > > + HDMI_SPD_SDI_HDDVR, > > > + HDMI_SPD_SDI_DVC, > > > + HDMI_SPD_SDI_DSC, > > > + HDMI_SPD_SDI_VCD, > > > + HDMI_SPD_SDI_GAME, > > > + HDMI_SPD_SDI_PC, > > > + HDMI_SPD_SDI_BD, > > > + HDMI_SPD_SDI_SCD, > > = > > I believe SACD is a more correct name. > = > Done. > = > > HD DVD and PMP are missing from this list. > = > I can't find any reference to those in CEA-861-D. HDMI 1.4 doesn't have > them either. Can you provide any pointers? CEA-861-E has them. > = > > > +enum hdmi_audio_coding_type { > > > + HDMI_AUDIO_CODING_TYPE_STREAM, > > > + HDMI_AUDIO_CODING_TYPE_IEC_60958, > > > + HDMI_AUDIO_CODING_TYPE_AC3, > > > + HDMI_AUDIO_CODING_TYPE_MPEG1, > > > + HDMI_AUDIO_CODING_TYPE_MP3, > > > + HDMI_AUDIO_CODING_TYPE_MPEG2, > > > + HDMI_AUDIO_CODING_TYPE_AAC, > > > + HDMI_AUDIO_CODING_TYPE_DTS, > > > + HDMI_AUDIO_CODING_TYPE_ATRAC, > > > + HDMI_AUDIO_CODING_TYPE_ONE_BIT_AUDIO, > > > + HDMI_AUDIO_CODING_TYPE_DOLBY_DIGITAL_PLUS, > > > + HDMI_AUDIO_CODING_TYPE_DTS_HD, > > > + HDMI_AUDIO_CODING_TYPE_MAT_MLP, > > > + HDMI_AUDIO_CODING_TYPE_DST, > > > + HDMI_AUDIO_CODING_TYPE_WMPRO, > > = > > These don't always quite match the names in CEA-861-E. Wouldn't it > > be better to use matching names? > = > Are HD-DVD and PMP above listed in CEA-861-E? I'll need to find a copy > of it and will go over this again and match the names to those in the > document. Great. > > Also the audio coding extension type is missing. > = > I assume this is also part of CEA-861-E as I can't find a reference in > -D? Yeah, I only checked against CEA-861-E. -- = Ville Syrj=E4l=E4 Intel OTC