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

[Tomasz Figa]

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?

Best regards,
Tomasz