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

[Robert Foss]

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?

> Best regards,
> Tomasz
>