Re: [PATCH xserver 5/5] xwayland: refactor egl_backends for wayland registry
On 5 June 2018 at 18:28, Olivier Fourdan wrote:
> Hi Emil,
>
> Many thanks for your detailed review!
>
> On Tue, Jun 5, 2018 at 12:37 PM, Emil Velikov
> wrote:
>> Hi Olivier,
>>
>> There's a handful of mostly trivial suggestions below. The idea itself seems
>> reasonable IMHO. One gripe is that we're 'leaking' twice as much as before.
>>
>> Namely: even if the current backend cleans-up after itself (it some cases it
>> does not), the other backend 'leaks'. Not sure if/how much we should care in
>> that case.
>>
>> Was skimming if we cannot move more init_egl/init_screen tripping points
>> and I've noticed a few gotchas/bugs. None of which are requirement for the
>> series, although great to have.
>>
>>
>> xwl_glamor_gbm_init_egl
>> - if !render_node error path is leaking
>
> humm, not sure, leaking what? we haven't allocated anything yet, have we?
> But anyhow, it's unralted to this refactoring I think.
>
You are right, this and the other bits listed just below are preexisting.
Thanks for tackling them.
The following are set during init_registry (wl_drm path) and
effectively leaked. Then again, there's plenty of small bits like that
throughout the code base.
I would _not_ bother with them bth.
::device_name -> strdup(device/node name)
::drm_fd -> open(^^)
::drm -> wl_registry_bind
>>> @@ -119,8 +134,8 @@ xwl_glamor_allow_commits(struct xwl_window *xwl_window)
>>> {
>>> struct xwl_screen *xwl_screen = xwl_window->xwl_screen;
>>>
>>> -if (xwl_screen->egl_backend.allow_commits)
>>> -return xwl_screen->egl_backend.allow_commits(xwl_window);
>>> +if (xwl_screen->egl_backend && xwl_screen->egl_backend->allow_commits)
>> Why the NULL check? Unless I'm missing something that will never happen.
>
> We can have no “egl_backend” set at all if “-shm” was specified.
>
> Either we keep that test here or we need to check for
> "xwl_screen->glamor" in xwl_screen_post_damage(), I'll change for the
> later for clarity.
>
Ah, yes. Thanks.
-Emil
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver 5/5] xwayland: refactor egl_backends for wayland registry
Hi Emil,
Many thanks for your detailed review!
On Tue, Jun 5, 2018 at 12:37 PM, Emil Velikov wrote:
> Hi Olivier,
>
> There's a handful of mostly trivial suggestions below. The idea itself seems
> reasonable IMHO. One gripe is that we're 'leaking' twice as much as before.
>
> Namely: even if the current backend cleans-up after itself (it some cases it
> does not), the other backend 'leaks'. Not sure if/how much we should care in
> that case.
>
> Was skimming if we cannot move more init_egl/init_screen tripping points
> and I've noticed a few gotchas/bugs. None of which are requirement for the
> series, although great to have.
>
>
> xwl_glamor_gbm_init_egl
> - if !render_node error path is leaking
humm, not sure, leaking what? we haven't allocated anything yet, have we?
But anyhow, it's unralted to this refactoring I think.
> - surely we'd want to error out when GL_OES_EGL_image is missing
> xwl_glamor_gbm_init_screen
Sure, will add separate patch for that, seems it got lost with commit
1545e2dba ("xwayland: Decouple GBM from glamor").
> - no need to RegisterPrivateKey/AddCallback if a rendernode is opened
Yeap, will add a separate patch for this.
> xwl_glamor_eglstream_init_egl
> - using EGL_IMG_context_priority w/o checking for it
> xwl_glamor_eglstream_init_screen
Ditto.
> - the ->controller check is no longer needed
Ditto.
>> +static Bool
>> +xwl_glamor_gbm_has_egl_extension(void)
>> +{
>> +return epoxy_has_egl_extension(NULL, "EGL_MESA_platform_gbm");
> I'd also check for EGL_KHR_platform_gbm - it's trivial enough ;-)
Sure!
>> @@ -71,9 +71,24 @@ xwl_glamor_init_wl_registry(struct xwl_screen *xwl_screen,
>> uint32_t id, const char *interface,
>> uint32_t version)
>> {
>> -if (xwl_screen->egl_backend.init_wl_registry)
>> -xwl_screen->egl_backend.init_wl_registry(xwl_screen, registry,
>> - interface, id, version);
>> +#ifdef GLAMOR_HAS_GBM
>> +if (xwl_screen->gbm_backend.is_available &&
>> +xwl_screen->gbm_backend.init_wl_registry &&
>> +xwl_screen->gbm_backend.init_wl_registry(xwl_screen,
>> + registry,
>> + id,
>> + interface,
>> + version)); /* no-op */
>> +#endif
>> +#ifdef XWL_HAS_EGLSTREAM
>> +else if (xwl_screen->eglstreams_backend.is_available &&
>> + xwl_screen->eglstreams_backend.init_wl_registry &&
>> + xwl_screen->eglstreams_backend.init_wl_registry(xwl_screen,
>> + registry,
>> + id,
>> + interface,
>> + version)); /*
>> no-op */
>> +#endif
> Both ifdef guards can go.
Well, the idea was to save a few conditionals if the support was not
enabled at build time, but I can certainly remove those ifdefs.
>> @@ -119,8 +134,8 @@ xwl_glamor_allow_commits(struct xwl_window *xwl_window)
>> {
>> struct xwl_screen *xwl_screen = xwl_window->xwl_screen;
>>
>> -if (xwl_screen->egl_backend.allow_commits)
>> -return xwl_screen->egl_backend.allow_commits(xwl_window);
>> +if (xwl_screen->egl_backend && xwl_screen->egl_backend->allow_commits)
> Why the NULL check? Unless I'm missing something that will never happen.
We can have no “egl_backend” set at all if “-shm” was specified.
Either we keep that test here or we need to check for
"xwl_screen->glamor" in xwl_screen_post_damage(), I'll change for the
later for clarity.
>> @@ -165,17 +180,35 @@ glamor_egl_fd_name_from_pixmap(ScreenPtr screen,
>> void
>> xwl_glamor_init_backends(struct xwl_screen *xwl_screen, Bool use_eglstreams)
>> {
>
> General nit:
> Drop the return type of xwl_glamor_init_$backend now that we check
> ::is_available.
Sure, it's unused anyway.
> Instead of relying on the coded probe order (alongside the
> is_available = false workaround),
> we could use -eglstream to control which backend is checked/probed first.
>
> If that fails we fall-back to the other.
>
> If that sounds reasonable, then the following can be reworked as follows:
Sounds reasonable.
>> +#ifdef GLAMOR_HAS_GBM
>> +/* Init GBM even if "-eglstream" is requested, as EGL streams may fail
>> */
>> +xwl_glamor_init_gbm(xwl_screen);
> Here we add:
>
> if (!xwl_screen->gbm_backend.is_available && !use_eglstreams)
> ErrorF(xwayland glamor: GBM (default) is not available);
>
>> +#endif
>> #ifdef XWL_HAS_EGLSTREAM
>> +xwl_glamor_init_eglstream(xwl_screen);
>
>> if (use_eglstreams) {
>> -if (!xwl_glamor_init_eglstream(xwl_screen)) {
>> -ErrorF("xwayland glamor: failed to setup eglstream backend\n");
Re: [PATCH xserver 5/5] xwayland: refactor egl_backends for wayland registry
Hi Olivier,
There's a handful of mostly trivial suggestions below. The idea itself seems
reasonable IMHO. One gripe is that we're 'leaking' twice as much as before.
Namely: even if the current backend cleans-up after itself (it some cases it
does not), the other backend 'leaks'. Not sure if/how much we should care in
that case.
Was skimming if we cannot move more init_egl/init_screen tripping points
and I've noticed a few gotchas/bugs. None of which are requirement for the
series, although great to have.
xwl_glamor_gbm_init_egl
- if !render_node error path is leaking
- surely we'd want to error out when GL_OES_EGL_image is missing
xwl_glamor_gbm_init_screen
- no need to RegisterPrivateKey/AddCallback if a rendernode is opened
xwl_glamor_eglstream_init_egl
- using EGL_IMG_context_priority w/o checking for it
xwl_glamor_eglstream_init_screen
- the ->controller check is no longer needed
On 1 June 2018 at 15:31, Olivier Fourdan wrote:
> +static Bool
> +xwl_glamor_gbm_has_egl_extension(void)
> +{
> +return epoxy_has_egl_extension(NULL, "EGL_MESA_platform_gbm");
I'd also check for EGL_KHR_platform_gbm - it's trivial enough ;-)
> @@ -71,9 +71,24 @@ xwl_glamor_init_wl_registry(struct xwl_screen *xwl_screen,
> uint32_t id, const char *interface,
> uint32_t version)
> {
> -if (xwl_screen->egl_backend.init_wl_registry)
> -xwl_screen->egl_backend.init_wl_registry(xwl_screen, registry,
> - interface, id, version);
> +#ifdef GLAMOR_HAS_GBM
> +if (xwl_screen->gbm_backend.is_available &&
> +xwl_screen->gbm_backend.init_wl_registry &&
> +xwl_screen->gbm_backend.init_wl_registry(xwl_screen,
> + registry,
> + id,
> + interface,
> + version)); /* no-op */
> +#endif
> +#ifdef XWL_HAS_EGLSTREAM
> +else if (xwl_screen->eglstreams_backend.is_available &&
> + xwl_screen->eglstreams_backend.init_wl_registry &&
> + xwl_screen->eglstreams_backend.init_wl_registry(xwl_screen,
> + registry,
> + id,
> + interface,
> + version)); /*
> no-op */
> +#endif
Both ifdef guards can go.
> @@ -119,8 +134,8 @@ xwl_glamor_allow_commits(struct xwl_window *xwl_window)
> {
> struct xwl_screen *xwl_screen = xwl_window->xwl_screen;
>
> -if (xwl_screen->egl_backend.allow_commits)
> -return xwl_screen->egl_backend.allow_commits(xwl_window);
> +if (xwl_screen->egl_backend && xwl_screen->egl_backend->allow_commits)
Why the NULL check? Unless I'm missing something that will never happen.
> @@ -165,17 +180,35 @@ glamor_egl_fd_name_from_pixmap(ScreenPtr screen,
> void
> xwl_glamor_init_backends(struct xwl_screen *xwl_screen, Bool use_eglstreams)
> {
General nit:
Drop the return type of xwl_glamor_init_$backend now that we check
::is_available.
Instead of relying on the coded probe order (alongside the
is_available = false workaround),
we could use -eglstream to control which backend is checked/probed first.
If that fails we fall-back to the other.
If that sounds reasonable, then the following can be reworked as follows:
> +#ifdef GLAMOR_HAS_GBM
> +/* Init GBM even if "-eglstream" is requested, as EGL streams may fail */
> +xwl_glamor_init_gbm(xwl_screen);
Here we add:
if (!xwl_screen->gbm_backend.is_available && !use_eglstreams)
ErrorF(xwayland glamor: GBM (default) is not available);
> +#endif
> #ifdef XWL_HAS_EGLSTREAM
> +xwl_glamor_init_eglstream(xwl_screen);
> if (use_eglstreams) {
> -if (!xwl_glamor_init_eglstream(xwl_screen)) {
> -ErrorF("xwayland glamor: failed to setup eglstream backend\n");
> -use_eglstreams = FALSE;
> -}
> +/* Force GBM backend off */
> +xwl_screen->gbm_backend.is_available = FALSE;
This is no longer needed, and we can now fold the two conditionals -
just like the GBM codepath.
> +if (!xwl_screen->eglstreams_backend.is_available)
> +ErrorF("xwayland glamor: EGLstreams requested but not
> available\n");
> }
> #endif
> -if (!use_eglstreams && !xwl_glamor_init_gbm(xwl_screen)) {
> -ErrorF("xwayland glamor: failed to setup GBM backend, falling back
> to sw accel\n");
> -xwl_screen->glamor = 0;
> +}
> +
> +void
> +xwl_glamor_select_backend(struct xwl_screen *xwl_screen, Bool use_eglstreams)
> +{
> +if (xwl_screen->egl_backend == NULL &&
> xwl_screen->gbm_backend.is_available) {
> +if (xwl_glamor_has_wl_interfaces(xwl_screen,
> _screen->gbm_backend))
> +
Re: [PATCH xserver 5/5] xwayland: refactor egl_backends for wayland registry
On Fri, 2018-06-01 at 16:31 +0200, Olivier Fourdan wrote:
> To be able to check for availability of the Wayland interfaces required
> to run a given EGL backend (either GBM or EGL streams for now), we need
> to have each backend structures and vfuncs in place before we enter the
> Wayland registry dance.
>
> That basically means that we should init all backends at first, connect
> to the Wayland compositor and query the available interfaces and then
> decide which backend is available and should be used (or none if either
> the Wayland interfaces or the EGL extensions are not available).
>
> For this purpose, hold an egl_backend struct for each backend we are to
> consider prior to connect to the Wayland display so that, when we get to
> query the Wayland interfaces, everything is in place for each backend to
> handle the various Wayland interfaces.
>
> Eventually, when we need to chose which EGL backend to use for glamor,
> the available Wayland interfaces and EGL extensions available are all
> known to Xwayland.
>
> Signed-off-by: Olivier Fourdan
> ---
> hw/xwayland/xwayland-glamor-eglstream.c | 27 ++
> hw/xwayland/xwayland-glamor-gbm.c | 38 ++
> hw/xwayland/xwayland-glamor.c | 69 ++---
> hw/xwayland/xwayland-present.c | 7 +--
> hw/xwayland/xwayland.c | 10 ++--
> hw/xwayland/xwayland.h | 15 --
> 6 files changed, 118 insertions(+), 48 deletions(-)
>
> diff --git a/hw/xwayland/xwayland-glamor-eglstream.c b/hw/xwayland/xwayland-
> glamor-eglstream.c
> index 85b562c31..ee03c4a86 100644
> --- a/hw/xwayland/xwayland-glamor-eglstream.c
> +++ b/hw/xwayland/xwayland-glamor-eglstream.c
> @@ -638,11 +638,11 @@ const struct wl_eglstream_display_listener
> eglstream_display_listener = {
> .swapinterval_override =
> xwl_eglstream_display_handle_swapinterval_override,
> };
>
> -static void
> +static Bool
> xwl_glamor_eglstream_init_wl_registry(struct xwl_screen *xwl_screen,
>struct wl_registry *wl_registry,
> - const char *name,
> - uint32_t id, uint32_t version)
> + uint32_t id, const char *name,
> + uint32_t version)
> {
> struct xwl_eglstream_private *xwl_eglstream =
> xwl_eglstream_get(xwl_screen);
> @@ -654,10 +654,15 @@ xwl_glamor_eglstream_init_wl_registry(struct
> xwl_screen *xwl_screen,
> wl_eglstream_display_add_listener(xwl_eglstream->display,
>_display_listener,
>xwl_screen);
> +return TRUE;
> } else if (strcmp(name, "wl_eglstream_controller") == 0) {
> xwl_eglstream->controller = wl_registry_bind(
> wl_registry, id, _eglstream_controller_interface, version);
> +return TRUE;
> }
> +
> +/* no match */
> +return FALSE;
> }
>
> static Bool
> @@ -896,6 +901,7 @@ xwl_glamor_init_eglstream(struct xwl_screen *xwl_screen)
> struct xwl_eglstream_private *xwl_eglstream;
> EGLDeviceEXT egl_device;
>
> +xwl_screen->eglstreams_backend.is_available = FALSE;
> egl_device = xwl_eglstream_get_device(xwl_screen);
> if (egl_device == EGL_NO_DEVICE_EXT)
> return FALSE;
> @@ -915,13 +921,14 @@ xwl_glamor_init_eglstream(struct xwl_screen
> *xwl_screen)
> xwl_eglstream->egl_device = egl_device;
> xorg_list_init(_eglstream->pending_streams);
>
> -xwl_screen->egl_backend.init_egl = xwl_glamor_eglstream_init_egl;
> -xwl_screen->egl_backend.init_wl_registry =
> xwl_glamor_eglstream_init_wl_registry;
> -xwl_screen->egl_backend.has_wl_interfaces =
> xwl_glamor_eglstream_has_wl_interfaces;
> -xwl_screen->egl_backend.init_screen = xwl_glamor_eglstream_init_screen;
> -xwl_screen->egl_backend.get_wl_buffer_for_pixmap =
> xwl_glamor_eglstream_get_wl_buffer_for_pixmap;
> -xwl_screen->egl_backend.post_damage = xwl_glamor_eglstream_post_damage;
> -xwl_screen->egl_backend.allow_commits =
> xwl_glamor_eglstream_allow_commits;
> +xwl_screen->eglstreams_backend.init_egl =
> xwl_glamor_eglstream_init_egl;
> +xwl_screen->eglstreams_backend.init_wl_registry =
> xwl_glamor_eglstream_init_wl_registry;
> +xwl_screen->eglstreams_backend.has_wl_interfaces =
> xwl_glamor_eglstream_has_wl_interfaces;
> +xwl_screen->eglstreams_backend.init_screen =
> xwl_glamor_eglstream_init_screen;
> +xwl_screen->eglstreams_backend.get_wl_buffer_for_pixmap =
> xwl_glamor_eglstream_get_wl_buffer_for_pixmap;
> +xwl_screen->eglstreams_backend.post_damage =
> xwl_glamor_eglstream_post_damage;
> +xwl_screen->eglstreams_backend.allow_commits =
> xwl_glamor_eglstream_allow_commits;
> +xwl_screen->eglstreams_backend.is_available = TRUE;
>
> ErrorF("glamor: Using nvidia's eglstream interface,
[PATCH xserver 5/5] xwayland: refactor egl_backends for wayland registry
To be able to check for availability of the Wayland interfaces required
to run a given EGL backend (either GBM or EGL streams for now), we need
to have each backend structures and vfuncs in place before we enter the
Wayland registry dance.
That basically means that we should init all backends at first, connect
to the Wayland compositor and query the available interfaces and then
decide which backend is available and should be used (or none if either
the Wayland interfaces or the EGL extensions are not available).
For this purpose, hold an egl_backend struct for each backend we are to
consider prior to connect to the Wayland display so that, when we get to
query the Wayland interfaces, everything is in place for each backend to
handle the various Wayland interfaces.
Eventually, when we need to chose which EGL backend to use for glamor,
the available Wayland interfaces and EGL extensions available are all
known to Xwayland.
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-glamor-eglstream.c | 27 ++
hw/xwayland/xwayland-glamor-gbm.c | 38 ++
hw/xwayland/xwayland-glamor.c | 69 ++---
hw/xwayland/xwayland-present.c | 7 +--
hw/xwayland/xwayland.c | 10 ++--
hw/xwayland/xwayland.h | 15 --
6 files changed, 118 insertions(+), 48 deletions(-)
diff --git a/hw/xwayland/xwayland-glamor-eglstream.c
b/hw/xwayland/xwayland-glamor-eglstream.c
index 85b562c31..ee03c4a86 100644
--- a/hw/xwayland/xwayland-glamor-eglstream.c
+++ b/hw/xwayland/xwayland-glamor-eglstream.c
@@ -638,11 +638,11 @@ const struct wl_eglstream_display_listener
eglstream_display_listener = {
.swapinterval_override =
xwl_eglstream_display_handle_swapinterval_override,
};
-static void
+static Bool
xwl_glamor_eglstream_init_wl_registry(struct xwl_screen *xwl_screen,
struct wl_registry *wl_registry,
- const char *name,
- uint32_t id, uint32_t version)
+ uint32_t id, const char *name,
+ uint32_t version)
{
struct xwl_eglstream_private *xwl_eglstream =
xwl_eglstream_get(xwl_screen);
@@ -654,10 +654,15 @@ xwl_glamor_eglstream_init_wl_registry(struct xwl_screen
*xwl_screen,
wl_eglstream_display_add_listener(xwl_eglstream->display,
_display_listener,
xwl_screen);
+return TRUE;
} else if (strcmp(name, "wl_eglstream_controller") == 0) {
xwl_eglstream->controller = wl_registry_bind(
wl_registry, id, _eglstream_controller_interface, version);
+return TRUE;
}
+
+/* no match */
+return FALSE;
}
static Bool
@@ -896,6 +901,7 @@ xwl_glamor_init_eglstream(struct xwl_screen *xwl_screen)
struct xwl_eglstream_private *xwl_eglstream;
EGLDeviceEXT egl_device;
+xwl_screen->eglstreams_backend.is_available = FALSE;
egl_device = xwl_eglstream_get_device(xwl_screen);
if (egl_device == EGL_NO_DEVICE_EXT)
return FALSE;
@@ -915,13 +921,14 @@ xwl_glamor_init_eglstream(struct xwl_screen *xwl_screen)
xwl_eglstream->egl_device = egl_device;
xorg_list_init(_eglstream->pending_streams);
-xwl_screen->egl_backend.init_egl = xwl_glamor_eglstream_init_egl;
-xwl_screen->egl_backend.init_wl_registry =
xwl_glamor_eglstream_init_wl_registry;
-xwl_screen->egl_backend.has_wl_interfaces =
xwl_glamor_eglstream_has_wl_interfaces;
-xwl_screen->egl_backend.init_screen = xwl_glamor_eglstream_init_screen;
-xwl_screen->egl_backend.get_wl_buffer_for_pixmap =
xwl_glamor_eglstream_get_wl_buffer_for_pixmap;
-xwl_screen->egl_backend.post_damage = xwl_glamor_eglstream_post_damage;
-xwl_screen->egl_backend.allow_commits = xwl_glamor_eglstream_allow_commits;
+xwl_screen->eglstreams_backend.init_egl = xwl_glamor_eglstream_init_egl;
+xwl_screen->eglstreams_backend.init_wl_registry =
xwl_glamor_eglstream_init_wl_registry;
+xwl_screen->eglstreams_backend.has_wl_interfaces =
xwl_glamor_eglstream_has_wl_interfaces;
+xwl_screen->eglstreams_backend.init_screen =
xwl_glamor_eglstream_init_screen;
+xwl_screen->eglstreams_backend.get_wl_buffer_for_pixmap =
xwl_glamor_eglstream_get_wl_buffer_for_pixmap;
+xwl_screen->eglstreams_backend.post_damage =
xwl_glamor_eglstream_post_damage;
+xwl_screen->eglstreams_backend.allow_commits =
xwl_glamor_eglstream_allow_commits;
+xwl_screen->eglstreams_backend.is_available = TRUE;
ErrorF("glamor: Using nvidia's eglstream interface, direct rendering
impossible.\n");
ErrorF("glamor: Performance may be affected. Ask your vendor to support
GBM!\n");
diff --git a/hw/xwayland/xwayland-glamor-gbm.c
b/hw/xwayland/xwayland-glamor-gbm.c
index f34c45b9a..3566af0bb 100644
---
