[PATCH 4/7] gl-renderer: Add support for EGLDevice+EGLOutput
Daniel Stone
daniel at fooishbar.org
Tue Mar 22 10:53:38 UTC 2016
Hi Miguel,
On 21 March 2016 at 16:37, Miguel A. Vico <mvicomoya at nvidia.com> wrote:
> EGLDevice provides means to enumerate native devices, and then create
> an EGL display connection from them.
>
> Similarly, EGLOutput will provide means to access different
> portions of display control hardware associated with an EGLDevice.
>
> For instance, EGLOutputLayer represents a portion of display
> control hardware that accepts an image as input and processes it
> for presentation on a display device.
>
> EGLStream implements a mechanism to communicate frame producers and
> frame consumers. By attaching an EGLOutputLayer consumer to a stream,
> a producer will be able to present frames on a display device.
>
> Thus, a compositor could produce frames and feed them to an
> EGLOutputLayer through an EGLStream for presentation on a display
> device.
>
> In a similar way, by attaching a GLTexture consumer to a stream, a
> producer (wayland client) could feed frames to a texture, which in
> turn can be used by a compositor to prepare the final frame to be
> presented.
>
> This change adds required logic to support presentation approach
> described above.
>
> Note that some unpublished EGL extensions were needed:
>
> - EGL_NV_stream_attrib:
> https://github.com/aritger/eglstreams-kms-example/blob/master/proposed-extensions/EGL_NV_stream_attrib.txt
>
> - EGL_EXT_stream_acquire_mode:
> https://github.com/aritger/eglstreams-kms-example/blob/master/proposed-extensions/EGL_EXT_stream_acquire_mode.txt
>
> - EGL_NV_output_drm_flip_event:
> https://github.com/aritger/eglstreams-kms-example/blob/master/proposed-extensions/EGL_NV_output_drm_flip_event.txt
>
> Also, in order to allow wl_buffers to be bound to EGLStreams, we
> kludged eglQueryWaylandBufferWL(EGL_WAYLAND_BUFFER_WL) to return
> the stream file descriptor.
Ugh, that's pretty unpleasant.
> We think the proper way to handle this should be:
>
> - Update WL_bind_wayland_display such that eglQueryWaylandBufferWL() accepts
> a new attribute EGL_WAYLAND_BUFFER_TYPE_WL, returning
> EGL_WAYLAND_BUFFER_EGLIMAGE_WL for the non-stream case.
>
> - Add a new WL_wayland_buffer_eglstream extension, which would define
> EGL_WAYLAND_BUFFER_EGLSTREAM_WL as a return value for
> EGL_WAYLAND_BUFFER_TYPE_WL, and yet another attribute
> EGL_WAYLAND_BUFFER_EGLSTREAM_FD_WL to query the stream file descriptor.
I disagree quite strongly with this, since it has unpleasant
compatibility implications. I'd rather see a new EGL_WAYLAND_STREAM_WL
token or similar, since a stream is by definition not a buffer.
> @@ -84,6 +85,8 @@ struct gl_output_state {
> struct gl_border_image borders[4];
> enum gl_border_status border_status;
>
> + EGLStreamKHR egl_stream;
> +
> struct weston_matrix output_matrix;
> };
>
> @@ -159,6 +162,9 @@ struct gl_surface_state {
> int height; /* in pixels */
> int y_inverted;
>
> + EGLStreamKHR egl_stream;
> + bool stream_frame_acquired;
> +
> struct weston_surface *surface;
>
> struct wl_listener surface_destroy_listener;
> @@ -202,6 +208,36 @@ struct gl_renderer {
>
> int has_configless_context;
>
> + PFNEGLCREATESTREAMKHRPROC create_stream;
> + PFNEGLDESTROYSTREAMKHRPROC destroy_stream;
> + PFNEGLQUERYSTREAMKHRPROC query_stream;
> + int has_egl_stream;
> +
> + PFNEGLCREATESTREAMPRODUCERSURFACEKHRPROC create_stream_producer_surface;
> + int has_egl_stream_producer_eglsurface;
> +
> + PFNEGLSTREAMCONSUMEROUTPUTEXTPROC stream_consumer_output;
> + int has_egl_stream_consumer_egloutput;
> +
> + PFNEGLSTREAMCONSUMERACQUIREKHRPROC stream_consumer_acquire;
> +#ifdef EGL_EXT_stream_acquire_mode
> + PFNEGLSTREAMCONSUMERACQUIREATTRIBEXTPROC stream_consumer_acquire_attrib;
> +#endif
> + int has_egl_stream_acquire_mode;
> +
> + int has_egl_output_drm;
> + int has_egl_output_drm_flip_event;
> +
> + PFNEGLGETOUTPUTLAYERSEXTPROC get_output_layers;
> + PFNEGLQUERYOUTPUTLAYERATTRIBEXTPROC query_output_layer_attrib;
> + int has_egl_output_base;
> +
> + PFNEGLCREATESTREAMFROMFILEDESCRIPTORKHRPROC create_stream_from_file_descriptor;
> + int has_egl_stream_cross_process_fd;
> +
> + PFNEGLSTREAMCONSUMERGLTEXTUREEXTERNALKHRPROC stream_consumer_gltexture;
> + int has_egl_stream_consumer_gltexture;
> +
These all need conditional declarations in headers, to support
building with other EGL implementations.
> @@ -743,6 +780,21 @@ draw_view(struct weston_view *ev, struct weston_output *output,
> if (!gs->shader)
> return;
>
> + /* If using EGLStreams, only continue if we are certain we have something to
> + * render, i.e. a new frame is available or an old one was previously
> + * acquired and is ready to be re-used */
> + if (gs->egl_stream != EGL_NO_STREAM_KHR) {
> + if (gr->query_stream(gr->egl_display,
> + gs->egl_stream,
> + EGL_STREAM_STATE_KHR,
> + &stream_state) != EGL_TRUE)
> + return;
> +
> + if (stream_state != EGL_STREAM_STATE_NEW_FRAME_AVAILABLE_KHR &&
> + !gs->stream_frame_acquired)
> + return;
> + }
This is really broken. If you're finding yourself in this situation,
you need to work out how you ended up there (presumably re-rendering
with no changes, e.g. when switching on triangle-fan debug?) and work
a way through that. Just refusing to draw means that surfaces will
silently disappear when you hit this case: not nice.
> @@ -774,6 +826,16 @@ draw_view(struct weston_view *ev, struct weston_output *output,
> glTexParameteri(gs->target, GL_TEXTURE_MAG_FILTER, filter);
> }
>
> + /* If using EGLStreams, we need to acquire the new frame, if any */
> + if (gs->egl_stream != EGL_NO_STREAM_KHR &&
> + stream_state == EGL_STREAM_STATE_NEW_FRAME_AVAILABLE_KHR) {
> + if (gr->stream_consumer_acquire(gr->egl_display,
> + gs->egl_stream) != EGL_TRUE)
> + goto out;
> +
> + gs->stream_frame_acquired = true;
> + }
A mid-review thought that's just occurred to me: what actually
enforces synchronisation here ... ?
Let's say you're in the middle of a resize. From client code, you would do:
- wl_surface_attach(old_buffer)
- wl_surface_commit()
- [resize event comes in]
- wl_surface_attach(new_buffer)
- wl_surface_set_input_region(...)
- wl_surface_set_opaque_region)
- wl_surface_commit()
The buffer attach is latched on the wl_surface_commit() call, as you
can probably see from the pretty extensive surface_commit ->
weston_surface_commit -> weston_surface_commit_state tree inside
src/compositor.c. On top of that, you have the added complexity of
subsurfaces, whose surface contents can optionally be staged and _not_
displayed even after commit, in order to synchronise resize and
reconfiguration with its parent surface.
>From what I can see, Streams just ignores that and takes whatever
content the client has posted, bypassing the fairly painstaking logic
inside compositor.c, which is kind of a showstopper. But then again, I
don't see a way to actually implement this with streams at all. Any
thoughts?
(As an aside, I'm assuming you still have to do wl_surface_attach +
wl_surface_damage + wl_surface_commit inside your EGLStreams
implementation to get Weston to paint at all: what exactly _do_ you do
here? Do you just fake up dummy wl_buffer objects?)
> @@ -1193,6 +1255,37 @@ gl_renderer_repaint_output(struct weston_output *output,
> }
>
> static int
> +gl_renderer_output_stream_flip(struct weston_output *output,
> + void *flip_data)
> +{
> +#ifdef EGL_EXT_stream_acquire_mode
> + struct gl_output_state *go = get_output_state(output);
> + struct weston_compositor *compositor = output->compositor;
> + struct gl_renderer *gr = get_renderer(compositor);
> +
> + EGLAttrib acquire_attribs[3] = { EGL_NONE };
> +
> +#ifdef EGL_NV_output_drm_flip_event
> + if (gr->has_egl_output_drm_flip_event) {
> + acquire_attribs[0] = EGL_DRM_FLIP_EVENT_DATA_NV;
> + acquire_attribs[1] = (EGLAttrib)flip_data;
> + acquire_attribs[2] = EGL_NONE;
> + }
> +#endif
> +
> + if (go->egl_stream != EGL_NO_STREAM_KHR)
> + if (EGL_TRUE != gr->stream_consumer_acquire_attrib(gr->egl_display,
> + go->egl_stream,
> + acquire_attribs))
> + return -1;
> +
> + return 0;
As an aside, it would be much easier to review this code if it was
split into two logical chunks: the stream consumer (EGLStream client
-> Wayland compositor) and the stream producer (EGLStream Wayland
compositor -> KMS target). It's especially hard when this relies on a
brand-new extension where you call StreamConsumerAcquireAttrib to
schedule a flip from the producer side ...
Anyway, what happens when EGL_NV_output_drm_flip_event isn't
supported? Surely it'd be fairly useless with no pageflip events -
unless you just rendered everything immediately ...
> @@ -1884,6 +1977,72 @@ gl_renderer_attach_dmabuf(struct weston_surface *surface,
> }
>
> static void
> +gl_renderer_attach_egl_fd_texture(struct weston_surface *es,
Please change the title to make it clear that this is about Streams,
and use underscores rather than camelCase for the variable name.
Otherwise it's too easy to assume that it's about dmabuf.
> + struct weston_buffer *buffer,
> + EGLNativeFileDescriptorKHR streamFd)
> +{
> + struct weston_compositor *ec = es->compositor;
> + struct gl_renderer *gr = get_renderer(ec);
> + struct gl_surface_state *gs = get_surface_state(es);
> +
> + /* If an invalid streamFd is provided, either there was an error creating
> + * the buffer or this buffer is already attached. Either case, there's
> + * nothing to be done */
> + if (streamFd < 0)
> + return;
This I would describe as a fairly cavalier approach to error handling.
If it's already attached to the buffer - shouldn't the renderer know
that? If it's an invalid stream, this means the surface will just
disappear and not be rendered, which is really quite bad - invalid
buffers result in events being sent back to the client normally ...
> + gs->shader = &gr->texture_shader_egl_external;
> + gs->target = GL_TEXTURE_EXTERNAL_OES;
Shouldn't we be testing for OES_EGL_image_external here? Which also
means that we're forced into LINEAR or NEAREST filtering - bad news
for downscaling. :(
> + buffer->legacy_buffer = (void *)buffer->resource;
> + gr->query_buffer(gr->egl_display, buffer->legacy_buffer,
> + EGL_WIDTH, &buffer->width);
> + gr->query_buffer(gr->egl_display, buffer->legacy_buffer,
> + EGL_HEIGHT, &buffer->height);
As said earlier, what happens when the producer posts a new buffer
into the stream with a different size in between the query here, and
rendering it later?
> + buffer->y_inverted = 0;
Why not the QueryWaylandBufferWL query for this? If NVIDIA don't
generate any Y-inverted frames through Streams ever, then just return
EGL_FALSE from your implementation.
> @@ -2587,9 +2764,92 @@ gl_renderer_create_window_surface(struct gl_renderer *gr,
> return egl_surface;
> }
>
> +static EGLSurface
> +gl_renderer_create_stream_surface(struct gl_renderer *gr,
> + uint32_t plane_id,
> + uint32_t crtc_id,
> + EGLint width, EGLint height,
> + EGLStreamKHR *egl_stream)
> +{
> + EGLint stream_attribs[] = {
> + EGL_STREAM_FIFO_LENGTH_KHR, 1,
> +#ifdef EGL_EXT_stream_acquire_mode
> + EGL_CONSUMER_AUTO_ACQUIRE_EXT, EGL_FALSE,
> +#endif
Again, it seems like this can't really function properly without
stream_acquire_mode!
> + if (num_layers < 1) {
> + weston_log("Unable to find output layers.\n");
> + goto err_egl_create_surf_stream;
> + }
Shouldn't this check be != 1, given that you just take the first layer?
> + egl_surface =
> + gl_renderer_create_stream_surface(gr,
> + plane_id, crtc_id,
> + output->current_mode->width,
> + output->current_mode->height,
> + &egl_stream);
> +
> + ret = gl_renderer_output_create(output, egl_surface, egl_stream);
> + if (ret < 0) {
> + if (egl_surface != EGL_NO_SURFACE)
> + eglDestroySurface(gr->egl_display, egl_surface);
> + if (egl_stream != EGL_NO_STREAM_KHR)
> + gr->destroy_stream(gr->egl_display, egl_stream);
> + }
Same error-handling gripe: if egl_surface isn't NULL, don't even
attempt to call gl_renderer_output_create.
> +static int
> +gl_renderer_get_devices(EGLint max_devices, EGLDeviceEXT *devices,
> + EGLint *num_devices)
> +{
> + const char *extensions;
> + PFNEGLQUERYDEVICESEXTPROC query_devices;
> +
> + extensions = (const char *)eglQueryString(EGL_NO_DISPLAY, EGL_EXTENSIONS);
> + if (!extensions) {
> + weston_log("Retrieving EGL extension string failed.\n");
> + return -1;
> + }
> +
> + if (!check_extension(extensions, "EGL_EXT_device_base")) {
> + weston_log("EGL_EXT_device_base not supported\n");
> + return -1;
> + }
> +
> + query_devices = (void *) eglGetProcAddress("eglQueryDevicesEXT");
> + if (!query_devices) {
> + weston_log("Failed to get eglQueryDevicesEXT function\n");
> + return -1;
> + }
> +
> + if (query_devices(max_devices, devices, num_devices) != EGL_TRUE) {
> + weston_log("Failed to query EGL Devices\n");
> + gl_renderer_print_egl_error_state();
> + return -1;
> + }
> +
> + return 0;
> +}
I'd certainly like to see EGLDevice handling separated out, since
that's something we want regardless of EGLStreams, to be able to
enumerate render devices separately from scanout devices.
> + (*drm_device_file) = query_device_string(device, EGL_DRM_DEVICE_FILE_EXT);
> + if (*drm_device_file == NULL) {
> + weston_log("Failed to query DRM device name.\n");
> + gl_renderer_print_egl_error_state();
> + return -1;
> + }
> +
> + return 0;
> +}
It's a real shame that this returns a filename rather than an open fd,
but that's my fault for not looking more closely before it was
ratified.
> WL_EXPORT struct gl_renderer_interface gl_renderer_interface = {
> .opaque_attribs = gl_renderer_opaque_attribs,
> .alpha_attribs = gl_renderer_alpha_attribs,
> + .opaque_stream_attribs = gl_renderer_opaque_stream_attribs,
This probably calls for opaque_attribs and alpha_attribs to be renamed
(in a separate patch) to contain _surface_. And does this means that
Streams + alpha is unsupported?
Cheers,
Daniel
More information about the wayland-devel
mailing list