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

[Emil Velikov]

On 12 October 2016 at 18:59, Axel Davy <axel.davy at ens.fr> wrote:
> On 12/10/2016 19:15, Emil Velikov wrote:
>>
>> On 11 October 2016 at 19:54, Axel Davy <axel.davy at ens.fr> wrote:
>>>
>>> On 11/10/2016 20:31, Emil Velikov wrote:
>>>>
>>>>    -   udev = udev_new();
>>>> -   if (!udev)
>>>> -      goto prime_clean;
>>>> +   if (drmGetDevice(default_fd, &device) != 0)
>>>> +      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;
>>>> -
>>>> -   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;
>>>> +      /* Hmm... detection for 2-7 seems to be broken. Nicely done. */
>>>
>>> For DRI2, DRI_PRIME takes a number corresponding to the device number as
>>> configured in xorg (and you have xrandr commands to configure the
>>> behaviour). This doesn't work with the DRI3 scheme. However for
>>> retrocompatibility, after several discussions, we decided to let DRI2's
>>> most
>>> used command 'DRI_PRIME=1' to still sorta work on DRI3, but the meaning
>>> changed: DRI_PRIME=1 => 'give me any device that is not the
>>> compositor/xorg
>>> device'
>>
>> No argument/objection against DRI_PRIME with DRI3, yet the current
>> hard coded DRI_PRIME=1 _only_ check is iffy. As per the spec there can
>> be 8 devices (0-8) with 0 always being the xorg/compositor one and 1-7
>> as 'other' devices. Yet here we only consider 1 and ignore everything
>> else :-\
>
> Do you suggest to have DRI_PRIME=2 (up to 8) be the same than DRI_PRIME=1 ?
>
Where/how did you read that ? It sounds like you've not read through
the spec, have you ?

I'm saying that the DRI2(DriverPrime) spec allows for 8 devices in
total. The 0 one is always the xserver one and you have 7 "others".
Atm things are completely bonkers, since you have this half a**ed
broken heuristics which ignores anything but 1. Admittedly there
aren't many users with 3 or more GPUs but if they were things just
won't work :-\

Furthermore atm you get a walk over the renderD* (missing D in the
libudev pattern ;-) devices which does not relate to what you get as
the 1 device in any other DRI_PRIME use case.

Or in other words, if you use DRI_PRIME=1 with libGL/EGL/gbm you can
get different result than when doing the same with the
vdpau/xvmc/omx/vaapi drivers.

Even within libGL (DRI2) things are quite magical , fortunately you
get the same device if the loader fails.

>>
>>>> +      is_different_device = 1;
>>>> +   } else {
>>>> +      default_tag = drm_construct_id_path_tag(device);
>>>> +      if (default_tag == NULL)
>>>> +         goto err;
>>>>    -   device_name = get_render_node_from_id_path_tag(udev,
>>>> -                                                  prime,
>>>> -                                                  another_tag);
>>>> -   if (device_name == NULL) {
>>>> +      if (strcmp(default_tag, prime) != 0) {
>>>> +         free(default_tag);
>>>> +         goto err;
>>>> +      }
>>>> +
>>>> +      free(default_tag);
>>>> +      /* we are to get a new fd (render-node) of the same device */
>>>>          is_different_device = 0;
>>>> -      goto default_device_clean;
>>>> +      // XXX: WTF ^^ so one uses the new model only to point to the
>>>> exact
>>>> same
>>>> +      // device and not the other more/less powerful GPU ?
>>>
>>> This case if when one has in his configuration file that the app should
>>> run
>>> on device XXX, which happens to be the one in use by the compositor.
>>> This case needed to be handled, and while it may not very seem useful
>>> now,
>>> it could be in the future an interesting case (On wayland, you could have
>>> the compositor use a card on some screens, and another card on the other,
>>> etc. In this case it makes sense for the user to specify the card, and it
>>> may or may not be the card used for the given screen).
>>>
>> There's something lost in the translation here:
>>
>> Currently in case of non DRI_PRIME (i.e. path tag) we dive into
>> get_render_node_from_id_path_tag(), loop and trigger on !another_tag
>> && !strcmp(id_path_tag, id_path_tag_tmp) (another_tag is zero here).
>>
>> Or in other words the function returns the render node, of the exact
>> _same_ device. Which makes the whole new method of using pci-...
>> dubious since one cannot select the 'other' device :-\
>>
>> That is unless I'm missing something ?
>>
>> -Emil
>>
>
> I'm not sure to understand exactly what you want to say.
>
> Here is a summary of current behaviour.
>
>
> device_id (env var, or dri conf) can be set to "1" (another device than
> default) or to an id path tag pci-XXX. The legacy env var DRI_PRIME
> overrides the setting if set (but is used the same way exactly).
>
> If never the value doesn't match 1, or a valid id path tag, you get the
> default device.
>
> If you set 1, it picks any device that has a different id path tag than the
> default device. This is obviously limited if you get many devices in the
> computer, and in this case you should rather use id path tags.
>
> Else it tries to pick the device with the specified id path tag.
>
> For the case when the specified id path tag is the id path tag of the
> default device, the default device is taken. It was decided to implement by
> taking the render node of the device in this case. I don't remember exactly
> why it was chosen instead of just taking the default fd. I think returning
> the default fd is fine as well.
>
> Were you suggesting that the call to get_render_node_from_id_path_tag is
> useless when we detect that the id path tag is the same than for the default
> device ? In that case the code is equivalent to just take the render-node
> version of device. If you decide to just return the default fd in that
> situation, you can just avoid the call.
>
Hmm seems like I misread things on my end, namely:

I misread prime (below) as default_device_id_path_tag :-\
get_render_node_from_id_path_tag(...prime...)

-Emil