From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH 05/12] video/hdmi: Don't let the user of this API create invalid infoframes Date: Thu, 15 Aug 2013 16:44:09 +0200 Message-ID: <20130815144408.GA16951@ulmo> References: <1376500755-30227-1-git-send-email-damien.lespiau@intel.com> <1376500755-30227-6-git-send-email-damien.lespiau@intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0809513722==" Return-path: In-Reply-To: <1376500755-30227-6-git-send-email-damien.lespiau@intel.com> 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: Damien Lespiau Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org --===============0809513722== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="TB36FDmn/VVEgNH/" Content-Disposition: inline --TB36FDmn/VVEgNH/ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Aug 14, 2013 at 06:19:08PM +0100, Damien Lespiau wrote: > To set the active aspect ratio value in the AVI infoframe today, you not > only have to set the active_aspect field, but also the active_info_valid > bit. Out of the 1 user of this API, we had 100% misuse, forgetting the > _valid bit. This was fixed in: >=20 > Author: Damien Lespiau > Date: Tue Aug 6 20:32:17 2013 +0100 >=20 > drm: Don't generate invalid AVI infoframes for CEA modes >=20 > We can do better and derive the _valid bit from the user wanting to set > the active aspect ratio. [...] > diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c [...] > @@ -96,7 +96,9 @@ ssize_t hdmi_avi_infoframe_pack(struct hdmi_avi_infofra= me *frame, void *buffer, > =20 > ptr[0] =3D ((frame->colorspace & 0x3) << 5) | (frame->scan_mode & 0x3); > =20 > - if (frame->active_info_valid) > + /* Data byte 1, bit 4 has to be set if we provide the active format > + * aspect ratio */ Nit: According to the coding style rules this should be: /* * Data byte 1, ... * ... ratio. */ > + if (frame->active_aspect & 0xf) > ptr[0] |=3D BIT(4); > =20 > if (frame->horizontal_bar_valid) > diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h > index bc6743e..931474c6 100644 > --- a/include/linux/hdmi.h > +++ b/include/linux/hdmi.h > @@ -109,7 +109,6 @@ struct hdmi_avi_infoframe { > unsigned char version; > unsigned char length; > enum hdmi_colorspace colorspace; > - bool active_info_valid; When I initially came up with these data structure I wanted them to explicitly expose all the fields that CEA/HDMI specifies. The idea was that a user would actually be allowed to generate invalid infoframes if they chose to by modifying the hdmi_avi_infoframe structure directly. Instead I'd prefer to see this handled by some higher level function. Thierry --TB36FDmn/VVEgNH/ Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.20 (GNU/Linux) iQIcBAEBAgAGBQJSDOk4AAoJEN0jrNd/PrOhVJsP/1uPxibyEvk35sdySIhLuD0F HBUB8F5Y78idSG/2VJwUfuLwcN19E0kAxU6Hrz7CxJXaZVvDQwGMSpvmrj3LIecE E3BMYORof5yiqm18PqRzMtX4pq2sYjz2lzOPgFebv7aB9NMzWjOfdKSkoPBWtJue PrGaEUIs0DLTW3ZPDh5r1zo1rOJVpK2r5wGBvzJVj6Ohbp6jcJWb9yxe2QAMMWy/ UrTlw6H91gy7wKGmkSZbiYfTdD6v8bVV+8VmHB6mTGHoRWoK+P3/RTmJdmDTm+8U PErTPkxIUZFePuLsQwvmS1U2QCOR+7pMjHLd9b3T9YJFTBS84nS0ovZ5qCgYTtsL ArWckhOtDHBN6g9rguivN8QuDnKZLYJpOTU2/9D/eRKqPOxsOkMFVfRDhUP6VAbl seY6X6UtHnSaP98eZtGYvoTNTy+rROg6rD15Er/r7ni6xwiTVNN5+pabGwidaTqO 7P/dBQCkOlXxsvctrfSTMpmAl7I/6ctlW4ym+CK0jX2cwCUtyeYXH3TRo+n+AvdU aUWxDFq5azCzcVPa3J/C5pGmpM+0vLHlTLNMsoPDJ8fM35mimwmad3dtr0Jt0U+F eq9bMnrrVRj9Iprl0y4c80wZxUK1S9rCI9UF8RnPHqrvhO6M+sFtLRDaI+cmkWJH QFRO90Ke6dAFyX6cQZ75 =tXoE -----END PGP SIGNATURE----- --TB36FDmn/VVEgNH/-- --===============0809513722== 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 --===============0809513722==--