On Wed, Feb 01, 2017 at 12:47:21PM +0000, Emil Velikov wrote: > On 30 January 2017 at 10:29, Thierry Reding <thierry.red...@gmail.com> wrote: > > From: Thierry Reding <tred...@nvidia.com> > > > > 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 <tred...@nvidia.com> > > --- > > 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 > > > +int util_get_devices(drmDevicePtr **devicesp, uint32_t flags) > Personally I would not have bothered with this helper, but it's nice to have > ;-)
Yeah, I added this after realizing that I was typing the same code
sequence for the second time.
> > +{
>
> > + if (!devicesp)
> > + return err;
> > +
>
> > +
> > + 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.
>
> > {
> > - int fd;
> > + int fd, err = 0;
> > +
> > + if (module)
> > + printf("trying to open `%s' with `%s'...", device, module);
> > + else
> > + printf("trying to open `%s'...", device);
> > +
> > + fd = 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)
> > +{
>
>
> > + } else {
> > + fd = util_open_with_module(device, module);
> Nit: one can drop a level of indentation/brackets - I have no strong
> preference either way.
>
> int util_open(...)
> {
> if (device)
> return util_open_with_module(device, module);
>
> ...
> 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
>
> > +
> > +static inline char *util_device_get_node(drmDevicePtr device,
> > + unsigned int type)
> > +{
> > + if (type >= 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 <emil.l.veli...@gmail.com>
Thanks,
Thierry
signature.asc
Description: PGP signature
_______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
