Re: [PATCH libdrm 2/3] tests/util: Make util_open() use drmDevice

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4195 bytes --]

On Wed, Feb 01, 2017 at 12:47:21PM +0000, Emil Velikov wrote:
> On 30 January 2017 at 10:29, Thierry Reding <thierry.reding@gmail.com> wrote:
> > From: Thierry Reding <treding@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 <treding@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.velikov@gmail.com>

Thanks,
Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel