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

[Emil Velikov]

On 18 October 2016 at 09:49, Nicolai Hähnle <nhaehnle at gmail.com> 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
>
> [snip]
>>
>> @@ -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" */
>> +#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);
>
>
> Defensive programming would suggest to use snprintf.
>
Correct. It's more like extra defensive in this case but will fix.

> [snip]
>
>> @@ -345,55 +299,66 @@ int loader_get_user_preferred_fd(int default_fd, int
>> *different_device)
>>        return default_fd;
>>     }
>>
>> -   udev = udev_new();
>> -   if (!udev)
>> -      goto prime_clean;
>> +   default_tag = drm_get_id_path_tag_for_fd(default_fd);
>> +   if (default_tag == NULL)
>> +      goto err;
>>
>> -   default_device_id_path_tag = get_id_path_tag_from_fd(udev,
>> default_fd);
>> -   if (!default_device_id_path_tag)
>> -      goto udev_clean;
>> +   num_devices = drmGetDevices(devices, MAX_DRM_DEVICES);
>> +   if (num_devices < 0)
>> +      goto err;
>>
>> -   is_different_device = 1;
>>     /* two format are supported:
>>      * "1": choose any other card than the card used by default.
>>      * id_path_tag: (for example "pci-0000_02_00_0") choose the card
>>      * with this id_path_tag.
>>      */
>>     if (!strcmp(prime,"1")) {
>> -      free(prime);
>> -      prime = strdup(default_device_id_path_tag);
>> -      /* request a card with a different card than the default card */
>> -      another_tag = 1;
>> -   } else if (!strcmp(default_device_id_path_tag, prime))
>> -      /* we are to get a new fd (render-node) of the same device */
>> -      is_different_device = 0;
>> -
>> -   device_name = get_render_node_from_id_path_tag(udev,
>> -                                                  prime,
>> -                                                  another_tag);
>> -   if (device_name == NULL) {
>> -      is_different_device = 0;
>> -      goto default_device_clean;
>> +      /* Hmm... detection for 2-7 seems to be broken. Oh well ...
>> +       * Pick the first render device that is not our own.
>> +       */
>> +      for (i = 0; i < num_devices; i++) {
>> +         if (devices[i]->available_nodes & 1 << DRM_NODE_RENDER &&
>> +             !drm_device_matches_tag(devices[i], default_tag)) {
>> +
>> +            found = true;
>> +            break;
>> +         }
>> +      }
>> +   } else {
>> +      for (i = 0; i < num_devices; i++) {
>> +         if (devices[i]->available_nodes & 1 << DRM_NODE_RENDER &&
>> +            drm_device_matches_tag(devices[i], prime)) {
>> +
>> +            found = true;
>> +            break;
>> +         }
>> +      }
>
>
> I feel like it would be helpful to have a warning here if the device was not
> found. This could avoid some confusion when people inevitably typo their
> prime setting.
>
Original code does not have such a message, so let's add it as follow-up ?

Emil