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

[Emil Velikov]

On 30 January 2017 at 10:29, Thierry Reding <thierry.reding at gmail.com> wrote:
> From: Thierry Reding <treding at 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 at 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 ;-)

> +{

> +    if (!devicesp)
> +        return err;
> +

> +
> +    if (devicesp)
With the "if !devicesp" check above this should always be true, no ?



> +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.
Sadly I'm out of ideas for better name(s) :-(

>  {
> -    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.


> +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
   ...
}


> --- 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 the above the series is
Reviewed-by: Emil Velikov <emil.l.velikov at gmail.com>

-Emil