From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH libdrm 2/3] tests/util: Make util_open() use drmDevice Date: Wed, 1 Feb 2017 16:59:32 +0100 Message-ID: <20170201155932.GA18725@ulmo.ba.sec> References: <20170130102923.1991-1-thierry.reding@gmail.com> <20170130102923.1991-2-thierry.reding@gmail.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0679939693==" Return-path: Received: from mail-wj0-x242.google.com (mail-wj0-x242.google.com [IPv6:2a00:1450:400c:c01::242]) by gabe.freedesktop.org (Postfix) with ESMTPS id 166276E827 for ; Wed, 1 Feb 2017 15:59:36 +0000 (UTC) Received: by mail-wj0-x242.google.com with SMTP id i7so13560812wjf.2 for ; Wed, 01 Feb 2017 07:59:36 -0800 (PST) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Emil Velikov Cc: ML dri-devel List-Id: dri-devel@lists.freedesktop.org --===============0679939693== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="1yeeQ81UyVL57Vl7" Content-Disposition: inline --1yeeQ81UyVL57Vl7 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Feb 01, 2017 at 12:47:21PM +0000, Emil Velikov wrote: > On 30 January 2017 at 10:29, Thierry Reding wr= ote: > > From: Thierry Reding > > > > The util_open() helper is used in a couple of test programs to open an > > appropriate device. It takes a device path and a module name, both are > > optional, as parameters. If a device path is specified, it will try to > > open the given device. Otherwise it will try all available devices. If > > only a specific subset is desired, the module parameter can be used as > > a filter. The function will use it to open only devices whose kernel > > driver matches the given module name. > > > > Instead of relying on the legacy drmOpen() function to do this, convert > > util_open() to use the new drmDevice helpers. This gets it functionally > > much closer to what other DRM/KMS users, such as the X.Org Server or a > > Wayland server, do. > > > > Signed-off-by: Thierry Reding > > --- > > tests/util/kms.c | 167 +++++++++++++++++++++++++++++++++++++++++------= -------- > > tests/util/kms.h | 43 ++++++++++++++ > > 2 files changed, 168 insertions(+), 42 deletions(-) > > > > diff --git a/tests/util/kms.c b/tests/util/kms.c > > index d866398237bb..c5d35ab616d1 100644 > > --- a/tests/util/kms.c > > +++ b/tests/util/kms.c >=20 > > +int util_get_devices(drmDevicePtr **devicesp, uint32_t flags) > Personally I would not have bothered with this helper, but it's nice to h= ave ;-) Yeah, I added this after realizing that I was typing the same code sequence for the second time. > > +{ >=20 > > + if (!devicesp) > > + return err; > > + >=20 > > + > > + if (devicesp) > With the "if !devicesp" check above this should always be true, no ? Yes, you're absolutely right. I added the above shortcut in a very late iteration and forgot to update the tail of the function. > > +int util_open_with_module(const char *device, const char *module) > Name and thus distinction vs util_open is blurred - here we require > non-null device... which we should check for. Yes, I've added a check for that now. > Sadly I'm out of ideas for better name(s) :-( I know this isn't ideal, but hopefully the doxygen in patch 3/3 will clarify somewhat how these are to be used. >=20 > > { > > - int fd; > > + int fd, err =3D 0; > > + > > + if (module) > > + printf("trying to open `%s' with `%s'...", device, module); > > + else > > + printf("trying to open `%s'...", device); > > + > > + fd =3D open(device, O_RDWR); > Not sure how much we should care here, but O_CLOEXEC would be nice. Yes, added. > > +int util_open(const char *device, const char *module) > > +{ >=20 >=20 > > + } else { > > + fd =3D util_open_with_module(device, module); > Nit: one can drop a level of indentation/brackets - I have no strong > preference either way. >=20 > int util_open(...) > { > if (device) > return util_open_with_module(device, module); >=20 > ... > get_devices > ... > } This looks better indeed. It's also nice how the implementation now very closely matches the description in the doxygen comment introduced in patch 3/3. > > --- a/tests/util/kms.h > > +++ b/tests/util/kms.h >=20 > > + > > +static inline char *util_device_get_node(drmDevicePtr device, > > + unsigned int type) > > +{ > > + if (type >=3D DRM_NODE_MAX) > > + return NULL; > > + > IMHO it's better to honour the ::available_nodes bitmask here, since > we already check if we're within range. This was meant to be a very low-level accessor that can be used to implement the iterators rather than be used standalone. If you look at the util_device_for_each_{,available}_node iterators, they make a distinction, though it is admittedly somewhat questionable how useful the iterator over all nodes, available or not, really is. It's probably best to just drop the util_device_for_each() iterator altogether. > This the above the series is > Reviewed-by: Emil Velikov Thanks, Thierry --1yeeQ81UyVL57Vl7 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAliSBeIACgkQ3SOs138+ s6Fibg/8DM4AYxzCHvEeJfUtK7Z520+kyxKPpeT2z2Ubz24AM+I79LtCLzPcv67n EABxeeQFF+xksSlNzs2P+WRTlSaIMzhXqtXX3uujOIw36h4XzsypopbmyvMTGr6o DbuxeqItfYP57u/zcn5JX5pIQMQqkG+kcMmDGmA+ctKS6oO6L/PIqVo6Xae0q3/V BFQUCwZroLVmjplYFdbUEzw+G9yl8ZPdeskm9bMQ0rrLd3wzjaSgdApVQLBZIHJY aHnA0neQzrzjBvj1U0KSXXxMZ53tfVbN3Wfj5ZrPjhPyRHtK5KbJxljicfUUGV0B PT2j/AFiSqk2dqlG+TdkCs+pj18Bmyhr8pUQjlTYzCGdJeb9JjYMjm8kvN6SF4yQ r+l31Pt3/ulRAb9Su7yxACBW75EXVqjW5efMuXYt/fi4qVN17Vem4T8qLim0USXw EkfT5gir2MP07tob2MaLCEl8CYqF3dPEHCSpI4PXWKJ4Z/A/lG1RFOL65E9d4J0N kw13HcEeyVsEGaTitaIS5d9W4dxLsNV0LsEJ3MyM3DmaYrPMVMxSnNtFPbJojckC RFY0ANi9akzEN9KvlvLwqJTAihVwohaAwsSucNMHeOa519YEhrbTVVsg2gX9lQzA rCNqQ36IoaVee7g6QjkQt5nfqDXlZ3tOMreBzT7D0gaMYP7k/NQ= =SdtL -----END PGP SIGNATURE----- --1yeeQ81UyVL57Vl7-- --===============0679939693== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0679939693==--