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

[Nicolai Hähnle]

On 18.10.2016 18:01, Emil Velikov wrote:
> 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.

Thanks :)


>> [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 ?

Fine by me.

Cheers,
Nicolai

>
> Emil
>