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

[Emil Velikov]

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