[Mesa-dev] [PATCH v2 05/16] loader: reimplement loader_get_user_preferred_fd via libdrm

[Emil Velikov]

On 14 October 2016 at 20:21, Axel Davy <axel.davy at ens.fr> wrote:
> On 14/10/2016 20:21, Emil Velikov wrote:
>>
>> From: Emil Velikov <emil.velikov at collabora.com>
>>
>> Currently not everyone has libudev and with follow-up patches we'll
>> completely remove the divergent codepaths.
>>
>> Use the libdrm drm device API to construct the required ID_PATH_TAG-like
>> string, to preserve the current functionality for libudev users and
>> allow others to benefit from it as well.
>>
>> v2: Drop ranty comments, pick the correct device
>>
>> Cc: Axel Davy <axel.davy at ens.fr>
>> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
>> ---
>>   src/loader/loader.c | 247
>> ++++++++++++++++++++++------------------------------
>>   1 file changed, 106 insertions(+), 141 deletions(-)
>>
>> diff --git a/src/loader/loader.c b/src/loader/loader.c
>> index ad4f946..06df05b 100644
>> --- a/src/loader/loader.c
>> +++ b/src/loader/loader.c
>> @@ -69,16 +69,11 @@
>>   #include <sys/stat.h>
>>   #include <stdarg.h>
>>   #include <stdio.h>
>> +#include <stdbool.h>
>>   #include <string.h>
>>   #ifdef HAVE_LIBUDEV
>>   #include <assert.h>
>>   #include <dlfcn.h>
>> -#include <unistd.h>
>> -#include <stdlib.h>
>> -#ifdef USE_DRICONF
>> -#include "xmlconfig.h"
>> -#include "xmlpool.h"
>> -#endif
>>   #endif
>>   #ifdef MAJOR_IN_MKDEV
>>   #include <sys/mkdev.h>
>> @@ -89,7 +84,13 @@
>>   #include "loader.h"
>>     #ifdef HAVE_LIBDRM
>> +#include <stdlib.h>
>> +#include <unistd.h>
>>   #include <xf86drm.h>
>> +#ifdef USE_DRICONF
>> +#include "xmlconfig.h"
>> +#include "xmlpool.h"
>> +#endif
>>   #endif
>>     #define __IS_LOADER
>> @@ -203,99 +204,9 @@ udev_device_new_from_fd(struct udev *udev, int fd)
>>        return device;
>>   }
>> +#endif
>>   -static char *
>> -get_render_node_from_id_path_tag(struct udev *udev,
>> -                                 char *id_path_tag,
>> -                                 char another_tag)
>> -{
>> -   struct udev_device *device;
>> -   struct udev_enumerate *e;
>> -   struct udev_list_entry *entry;
>> -   const char *path, *id_path_tag_tmp;
>> -   char *path_res;
>> -   char found = 0;
>> -   UDEV_SYMBOL(struct udev_enumerate *, udev_enumerate_new,
>> -               (struct udev *));
>> -   UDEV_SYMBOL(int, udev_enumerate_add_match_subsystem,
>> -               (struct udev_enumerate *, const char *));
>> -   UDEV_SYMBOL(int, udev_enumerate_add_match_sysname,
>> -               (struct udev_enumerate *, const char *));
>> -   UDEV_SYMBOL(int, udev_enumerate_scan_devices,
>> -               (struct udev_enumerate *));
>> -   UDEV_SYMBOL(struct udev_list_entry *, udev_enumerate_get_list_entry,
>> -               (struct udev_enumerate *));
>> -   UDEV_SYMBOL(void, udev_enumerate_unref,
>> -               (struct udev_enumerate *));
>> -   UDEV_SYMBOL(struct udev_list_entry *, udev_list_entry_get_next,
>> -               (struct udev_list_entry *));
>> -   UDEV_SYMBOL(const char *, udev_list_entry_get_name,
>> -               (struct udev_list_entry *));
>> -   UDEV_SYMBOL(struct udev_device *, udev_device_new_from_syspath,
>> -               (struct udev *, const char *));
>> -   UDEV_SYMBOL(const char *, udev_device_get_property_value,
>> -               (struct udev_device *, const char *));
>> -   UDEV_SYMBOL(const char *, udev_device_get_devnode,
>> -               (struct udev_device *));
>> -   UDEV_SYMBOL(struct udev_device *, udev_device_unref,
>> -               (struct udev_device *));
>> -
>> -   e = udev_enumerate_new(udev);
>> -   udev_enumerate_add_match_subsystem(e, "drm");
>> -   udev_enumerate_add_match_sysname(e, "render*");
>> -
>> -   udev_enumerate_scan_devices(e);
>> -   udev_list_entry_foreach(entry, udev_enumerate_get_list_entry(e)) {
>> -      path = udev_list_entry_get_name(entry);
>> -      device = udev_device_new_from_syspath(udev, path);
>> -      if (!device)
>> -         continue;
>> -      id_path_tag_tmp = udev_device_get_property_value(device,
>> "ID_PATH_TAG");
>> -      if (id_path_tag_tmp) {
>> -         if ((!another_tag && !strcmp(id_path_tag, id_path_tag_tmp)) ||
>> -             (another_tag && strcmp(id_path_tag, id_path_tag_tmp))) {
>> -            found = 1;
>> -            break;
>> -         }
>> -      }
>> -      udev_device_unref(device);
>> -   }
>> -
>> -   udev_enumerate_unref(e);
>> -
>> -   if (found) {
>> -      path_res = strdup(udev_device_get_devnode(device));
>> -      udev_device_unref(device);
>> -      return path_res;
>> -   }
>> -   return NULL;
>> -}
>> -
>> -static char *
>> -get_id_path_tag_from_fd(struct udev *udev, int fd)
>> -{
>> -   struct udev_device *device;
>> -   const char *id_path_tag_tmp;
>> -   char *id_path_tag;
>> -   UDEV_SYMBOL(const char *, udev_device_get_property_value,
>> -               (struct udev_device *, const char *));
>> -   UDEV_SYMBOL(struct udev_device *, udev_device_unref,
>> -               (struct udev_device *));
>> -
>> -   device = udev_device_new_from_fd(udev, fd);
>> -   if (!device)
>> -      return NULL;
>> -
>> -   id_path_tag_tmp = udev_device_get_property_value(device,
>> "ID_PATH_TAG");
>> -   if (!id_path_tag_tmp)
>> -      return NULL;
>> -
>> -   id_path_tag = strdup(id_path_tag_tmp);
>> -
>> -   udev_device_unref(device);
>> -   return id_path_tag;
>> -}
>> -
>> +#if defined(HAVE_LIBDRM)
>>   #ifdef USE_DRICONF
>>   static const char __driConfigOptionsLoader[] =
>>   DRI_CONF_BEGIN
>> @@ -321,17 +232,60 @@ static char *loader_get_dri_config_device_id(void)
>>   }
>>   #endif
>>   +static char *drm_construct_id_path_tag(drmDevicePtr device)
>> +{
>> +/* Length of "pci-xxxx_xx_xx_x\n" */
>
> I assume you want to say:
>
> "pci-xxxx_xx_xx_x" + terminal byte
>
> Thus it should be
>
> "pci-xxxx_xx_xx_x\0"
>
Yes, using \0 was the goal.

>> +#define PCI_ID_PATH_TAG_LENGTH 17
>> +   char *tag = NULL;
>> +
>> +   if (device->bustype == DRM_BUS_PCI) {
>> +        tag = calloc(PCI_ID_PATH_TAG_LENGTH, sizeof(char));
>> +        if (tag == NULL)
>> +            return NULL;
>> +
>> +        sprintf(tag, "pci-%04x_%02x_%02x_%1u",
>> device->businfo.pci->domain,
>> +                device->businfo.pci->bus, device->businfo.pci->dev,
>> +                device->businfo.pci->func);
>> +   }
>
>
> I haven't tested, but I guess you have compared to what udev does to have
> the equivalence.
> The advantage of the udev path is that usb graphic cards are supposed to
> work with the system,
> but I've no idea what's the status of hotplug support for usb graphic cards
> in the kernel.
>
> I guess support for such systems can be introduced when needed.
>
Pretty much my line of thought as well. If we have (m)any such users
they're more than welcome to send patches, ask for assistance or test
patches.

> The code looks good. With the minor nitpick fixed, this patch is:
> Reviewed-by: Axel Davy <axel.davy at ens.fr>
>
Thanks. If you can skim through any of the other patches that'll be appreciated.
Emil