[Mesa-dev] [RFC 1/5] st/dri: Allow kms_swrast to work without a device FD

[Emil Velikov]

On 5 July 2018 at 16:25, Robert Foss <robert.foss at collabora.com> wrote:
> Hey Tomasz,
>
>
> On 05/07/18 15:07, Tomasz Figa wrote:
>>
>> Hi Emil, Robert,
>>
>> On Thu, Jul 5, 2018 at 9:57 PM Emil Velikov <emil.l.velikov at gmail.com>
>> wrote:
>>>
>>>
>>> On 5 July 2018 at 12:32, Robert Foss <robert.foss at collabora.com> wrote:
>>>>
>>>> Hey Eric!
>>>>
>>>> On 05/07/18 12:35, Eric Engestrom wrote:
>>>>>
>>>>>
>>>>> On Thursday, 2018-07-05 12:07:36 +0200, Robert Foss wrote:
>>>>>>
>>>>>>
>>>>>> From: Tomeu Vizoso <tomeu.vizoso at collabora.com>
>>>>>>
>>>>>> A KMS device isn't strictly needed for the kms_swrast to work, so stop
>>>>>> failing to init if the FD is -1. Also don't call DRM_IOCTL_GET_CAP in
>>>>>> that case.
>>>>>>
>>>>>> This allows the driver to be used in machines where no KMS device at
>>>>>> all
>>>>>> is present.
>>>>>>
>>>>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso at collabora.com>
>>>>>> ---
>>>>>>    src/gallium/state_trackers/dri/dri2.c | 6 ++++--
>>>>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/src/gallium/state_trackers/dri/dri2.c
>>>>>> b/src/gallium/state_trackers/dri/dri2.c
>>>>>> index 58a6757f037..c262c0ca118 100644
>>>>>> --- a/src/gallium/state_trackers/dri/dri2.c
>>>>>> +++ b/src/gallium/state_trackers/dri/dri2.c
>>>>>> @@ -2189,7 +2189,8 @@ dri_kms_init_screen(__DRIscreen * sPriv)
>>>>>>         sPriv->driverPrivate = (void *)screen;
>>>>>>    -   if (screen->fd < 0 || (fd = fcntl(screen->fd, F_DUPFD_CLOEXEC,
>>>>>> 3))
>>>>>> < 0)
>>>>>> +   /* We don't really need a device FD if we are soft-rendering */
>>>>>> +   if (screen->fd >= 0 && (fd = fcntl(screen->fd, F_DUPFD_CLOEXEC,
>>>>>> 3)) <
>>>>>> 0)
>>>>>>          goto free_screen;
>>>>>>         if (pipe_loader_sw_probe_kms(&screen->dev, fd)) {
>>>>>
>>>>>
>>>>>
>>>>> Aren't you going to use an uninitialised `fd` here if `screen->fd < 0`?
>>>>
>>>>
>>>>
>>>>  From my understanding during dri2_initialize_android(),
>>>> droid_open_device[1]
>>>> will always allocate an FD or fail, before a screen is created in
>>>> dri2_create_screen[2].
>>>>
>>>> But maybe there is some part I don't quite understand.
>>>>
>>> You're tracking a single instance instead of looking at the function in
>>> itself.
>>> When screen->fd is invalid, fd will be uninitialised. The
>>> pipe_loader_sw_probe_kms() call will then use that uninitialised
>>> value.
>>>
>>> I believe a better solution is to have distinct dri/null/kms backends
>>> to swrast, instead of hacking it in this way.
>>> Some ideas are listed in here [1].
>>>
>>> -Emil
>>>
>>> [1]
>>> https://gitlab.collabora.com/tomeu/mesa/commit/54adda6a4d7b5c783d54dfd37d38d1a5a0f3187f
>>
>>
>> First of all, do we really need this patch at all? If you ended up
>> booting Android with Mesa, you must have had a DRM driver for the
>> display anyway, so what prevents you from using it as the KMS device
>> for kms_swrast?
>>
>
> Just dropping this patch does indeed cause no regressions.
> Emil: Do you see any problems with that?
>
None. The distinct dri/null/.... thing mentioned earlier is orthogonal
to the Android support.

-Emil