Hi Eric,
Thanks for having a look!
On Mon, 18 Feb 2019 at 17:05, Eric Engestrom <eric.engest...@intel.com> wrote:
>
> On Tuesday, 2019-02-05 15:31:08 +0000, Emil Velikov wrote:
> > From: Emil Velikov <emil.veli...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
