[Mesa-dev] [PATCH 0/7] DRI3 support for EGL (v3)

[Martin Peres]

On 30/10/15 21:26, Kristian Høgsberg wrote:
> On Fri, Oct 30, 2015 at 06:03:31PM +0200, Martin Peres wrote:
>> First of all, I would like to thank Boyan for his work here. I rebased his patch
>> series, fixed minor issues here and there and reviewed it. You can check the
>> changes in every patch but the biggest changes are related to the build system.
>>
>> Speaking about the build system it seems like scons is not building the EGL X11
>> platform, so I did not bother adding support for the X11 DRI3 there.
>>
>> I tested the code using GLES apps and ran a full piglit/cts run on all the
>> platforms. Nothing display-related failed.
>>
>> Finally, I added a patch that improves the reporting of which DRI version is
>> being used. I tested that the reporting was accurate by checking which node
>> was being used by running the application with ezbench's env_dump
>> (/utils/env_dump).
>>
>> I intend to push that series at the end of next week unless someone opposes.
> On the whole, this looks ok. I was a little surprised to see the dri3
> helpers added to libloader, but I guess that can work, as long as we
> drop the libloader_la_LIBADD += $(XCB_DRI3_LIBS) and add the libs to
> users of libloader.la that need them.

Yep, sounds good!

>
> One change I'd like to see is that we embed loader_dri3_drawable in
> dri3_drawable and change loader_dri3_create_drawable() to not allocate
> memory and call it loader_dri3_init_drawable().

Yes, I started work on this direction on Friday but I felt un-easy with the
deallocation part. I guess a fini() function will do just fine.

Thanks for calling me on this.
>
> Also, I think we could trim the number of vfuncs in
> loader_dri3_vtable. We don't need virtual getters for stuff that
> doesn't change like get_dri_screen, is_different_gpu. The
> loader_dri3_drawable always has the most up to date width and height,
> so we can just cache them in the struct and drop get_width and
> get_height.

Sounds good.

>
> Finally, we're getting to many vtables in this area. It's not a
> problem with this patch, but the patch is kinda the final straw. I
> think one thing we'll want to do is to see if we can kill
> dri2_dyp->vtbl in favor of handling the dri2_drv->base.API callbacks
> directly in the platform_*.c implementations. For example:
>
> static _EGLSurface*
> dri2_create_window_surface(_EGLDriver *drv, _EGLDisplay *dpy,
>                             _EGLConfig *conf, void *native_window,
>                             const EGLint *attrib_list)
> {
>     struct dri2_egl_display *dri2_dpy = dri2_egl_display(dpy);
>     return dri2_dpy->vtbl->create_window_surface(drv, dpy, conf, native_window,
>                                                  attrib_list);
> }
>
> doesn't add any value over just setting
> dri2_drv->base.API.CreateWindowSurface in platform_x11.c.  Functions
> like this one account for pretty much all of dri2_dpy->vtbl.

Right, this is something I have been wondering about when working
on the cpu usage. I am looking for a tool that could tell me where most
of the cache misses are located so as we can start optimizing the hotest
codepath. However, I am afraid we just have thousands of paper cuts...

>
> Anyway, with just the LIBADD and loader_dri3_init_drawable() changes
> I'd be ok with landing this;
>
> Reviewed-by: Kristian Høgsberg <krh at bitplanet.net>

Thanks for the review! I will try to change as little code as possible 
from Boyan
because I do not want him to take the blame for code that I added. I 
will instead
introduce new patches when possible.

>
>> Boyan Ding (6):
>>    loader: Add dri3 helper
>>    glx/dri3: Convert to use dri3 helper in loader library
>>    egl_dri2: Add a function to let platform code return dri drawable from
>>      _EGLSurface
>>    egl/x11: Implement dri3 support with loader's dri3 helper
>>    loader/dri3: Expose function to create __DRIimage from pixmap
>>    egl/x11_dri3: Implement EGL_KHR_image_pixmap
>>
>> Martin Peres (1):
>>    egl: make it clear which platform x11 backend is being used (dri2 or
>>      3)
>>
>>   configure.ac                             |   13 +-
>>   src/egl/Makefile.am                      |    9 +-
>>   src/egl/drivers/dri2/egl_dri2.c          |  118 ++-
>>   src/egl/drivers/dri2/egl_dri2.h          |   19 +-
>>   src/egl/drivers/dri2/platform_android.c  |    1 +
>>   src/egl/drivers/dri2/platform_drm.c      |    1 +
>>   src/egl/drivers/dri2/platform_wayland.c  |    2 +
>>   src/egl/drivers/dri2/platform_x11.c      |  112 ++-
>>   src/egl/drivers/dri2/platform_x11_dri3.c |  591 ++++++++++++
>>   src/egl/drivers/dri2/platform_x11_dri3.h |   44 +
>>   src/glx/dri3_glx.c                       | 1446 ++++--------------------------
>>   src/glx/dri3_priv.h                      |   96 +-
>>   src/loader/Makefile.am                   |    9 +
>>   src/loader/loader_dri3_helper.c          | 1421 +++++++++++++++++++++++++++++
>>   src/loader/loader_dri3_helper.h          |  235 +++++
>>   15 files changed, 2713 insertions(+), 1404 deletions(-)
>>   create mode 100644 src/egl/drivers/dri2/platform_x11_dri3.c
>>   create mode 100644 src/egl/drivers/dri2/platform_x11_dri3.h
>>   create mode 100644 src/loader/loader_dri3_helper.c
>>   create mode 100644 src/loader/loader_dri3_helper.h
>>
>> -- 
>> 2.6.2
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev