[Mesa-dev] [PATCH 3/3] egl/sl: use kms_swrast with vgem instead of a random GPU

[Emil Velikov]

Hi Eric,

Thanks for having a look!

On Mon, 18 Feb 2019 at 17:05, Eric Engestrom <eric.engestrom at intel.com> wrote:
>
> On Tuesday, 2019-02-05 15:31:08 +0000, Emil Velikov wrote:
> > From: Emil Velikov <emil.velikov at collabora.com>
> >
> > VGEM and kms_swrast were introduced to work with one another.
> >
> > All we do is CPU rendering to dumb buffers. There is no reason to carve
> > out GPU memory, increasing the memory pressure on a device that could
> > make a better use of it.
> >
> > For kms_swrast to work properly we require the primary node, as the dumb
> > buffer ioctls are not exposed via the render node.
> >
> > Note that this requires libdrm commit 3df8a7f0 ("xf86drm: fallback to
> > MODALIAS for OF less platform devices")
>
> Without this, what happens? swrast stops working?
>
kms_swrast never worked OOTB - extra patches were needed.
There is no change to swrast itself.

> A couple style comments below, but this question is my main concern.
>

> > +      dri2_dpy->driver_name = loader_get_driver_for_fd(dri2_dpy->fd);
>
> Can you keep the else branch like before? Makes it more readable IMO,
> and avoids allocating memory just to free it a couple lines below.
>
We need to get the driver name first, since we compare it against
"vgem" on the very next line.
Fully admit though as-is the code isn't that easy to read.

> > +      if (swrast) {
> > +         /* Use kms swrast only with vgem */
> > +         if (strcmp(dri2_dpy->driver_name, "vgem") != 0) {
> > +            free(dri2_dpy->driver_name);
> > +            dri2_dpy->driver_name = NULL;
> > +         } else {
> > +            free(dri2_dpy->driver_name);
> > +            dri2_dpy->driver_name = strdup("kms_swrast");
>
> Again, IMO this would be more readable as "if vgem use kms_swrast"
> instead of "if not vgem skip else use kms_swrast".
>
> The above two comments combined give us this code instead:
>
>   if swrast
>     if driver == vgem
>       driver = kms_swrast
>   else
>     driver = get_driver(fd)
>
Sure can flip the if/else. Might as well add a local driver_name to
simplify the free()/NULL bits.

char *driver_name = loader_get_driver_for_fd(dri2_dpy->fd);
if (swrast) {
   /* Use kms_swrast only with vgem */
   if (strcmp(driver_name, "vgem") == 0)
      dri2_dpy->driver_name = strdup("kms_swrast");
   free(driver_name);
} else {
   /* Use the given hardware driver */
    dri2_dpy->driver_name = driver_name;
}

Will send out v2 in a moment.

Thanks
Emil