[PATCH 4/7] gl-renderer: Add support for EGLDevice+EGLOutput
Miguel Angel Vico
mvicomoya at nvidia.com
Tue Mar 22 22:47:25 UTC 2016
Hi Daniel,
On Tue, 22 Mar 2016 10:53:38 +0000
Daniel Stone <daniel at fooishbar.org> wrote:
> 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.
IIUC, EGL_WAYLAND_BUFFER_WL is actually used to create an EGLImage from
a wl_buffer. I don't see how adding a new EGL_WAYLAND_STREAM_WL token
would help here.
We could instead add a new eglQueryWaylandStreamWL() function to query
attributes of a stream, and leave the WL_bind_wayland_display
extension untouched.
>
> > @@ -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.
It builds with Khronos headers, but for sure it doesn't mean it will
build with other EGL implementations. I'll add the conditionals.
>
> > @@ -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.
Thanks for pointing this out, we'll revisit our implementation and try
to fix this.
>
> > @@ -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?)
As discussed over IRC, your concern is genuine and we are aware of
certain limitations of our current implementation when it comes to
synchronizing weston surface attachments with EGLStream.
We will be working on it and come up with a better solution in future
releases.
>
> > @@ -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 ...
I will split this patch into 3 separate ones:
* EGLDevice enumeration
* Stream producer logic
* Stream consumer logic
>
> 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 ...
Well, if EGL_NV_output_drm_flip_event isn't supported, the compositor
should schedule output repaints using a timer-driven approach (as other
compositors already do).
>
> > @@ -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.
Sure.
>
> > + 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 ...
Due to an ordering issue between when a client creates the window
surface and when the surface configure callbacks are set up, it might
happen that an attach operation is performed before a configure, so
that a weston surface might never get mapped to an output if no
subsequent attach is issued.
For the EGLStream case, we might need to re-attach the same buffer
(actually a stream), and the renderer won't catch that.
>
> > + 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. :(
Yeah, EGL_KHR_stream_consumer_gltexture states gltexture consumers can
only be bound to GL_TEXTURE_EXTERNAL_OES textues. So yes, we're forced
into LINEAR or NEAREST filtering.
>
> > + 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?
Yeah, something we need to revisit.
>
> > + 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.
True, will fix.
>
> > @@ -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!
Yeah, without EGL_EXT_stream_acquire_mode, frames would be presented as
soon as they are made available by the producer. Issuing page flips
would be unnecessary, but we should reschedule output repaints in
weston in some other way (not relying on page flip events).
>
> > + 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?
Yeah, I think it might be clearer to check for "!= 1" instead of "< 1".
>
> > + 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.
Sure. Will fix.
>
> > +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.
Agreed. As said above, I'll split this patch into several pieces.
>
> > + (*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?
Alpha is actually supported with streams, but since compositor-drm is
only making use of opaque attributes, I didn't feel the need of adding
alpha stream attributes which wouldn't be used.
I'll add them if that makes things clearer.
Thanks!
>
> Cheers,
> Daniel
--
Miguel
NVIDIA GmbH, Wuerselen, Germany, Amtsgericht Aachen, HRB 8361
Managing Director: Karen Theresa Burns
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
More information about the wayland-devel
mailing list