New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add initial linux_dmabuf protocol support #698

Merged
merged 5 commits into from Mar 15, 2018

Conversation

Projects
None yet
4 participants
@agx
Contributor

agx commented Mar 1, 2018

Tested with ./weston-simple-dmabuf-drm

Todo

  • fix FIXMEs
  • style cleanups
  • check build without drm headers
  • more tests
  • drop renderer indirection
  • check error handling (logged errors vs. errors reported to clients)
    -later: look at YUV
    Initial feedback would be welcome.
static void wl_buffer_destroy(struct wl_client *client,
struct wl_resource *resource)
{

This comment has been minimized.

@emersion

emersion Mar 1, 2018

Member

Style nitpick: no newlines between ) and {

return NULL;
}
buffer = wl_resource_get_user_data(resource);

This comment has been minimized.

@emersion

emersion Mar 1, 2018

Member
}
buffer = wl_resource_get_user_data(resource);
assert(buffer);

This comment has been minimized.

@emersion

emersion Mar 1, 2018

Member

This is not necessary.

}
static void
params_destroy(struct wl_client *client, struct wl_resource *resource) {

This comment has been minimized.

@emersion

emersion Mar 1, 2018

Member

Style nitpick: no newline between static void and the rest of the line

}
assert(buffer->params_resource == params_resource);
assert(!buffer->buffer_resource);

This comment has been minimized.

@emersion

emersion Mar 1, 2018

Member

These assertions could be moved in the *_from_resource helper

/* initial buffer import */
if (!wlr_renderer_import_dmabuf(buffer->renderer, buffer))
goto err_failed;

This comment has been minimized.

@emersion

emersion Mar 1, 2018

Member

Style nitpick: always use brackets

return;
err_failed:
if (buffer_id == 0)

This comment has been minimized.

@emersion

emersion Mar 1, 2018

Member

Style nitpick: use brackets

@@ -13,6 +13,9 @@
#include "render/gles2.h"
#include "util/signal.h"
// TODO: what aout non linux, only needed for DRM_FORMAT_*
#include <libdrm/drm_fourcc.h>

This comment has been minimized.

@emersion

emersion Mar 1, 2018

Member

Wayland WL_SHM_FORMAT_* formats match Linux DRM_* enums, so I don't think including this file is necessary.

bool wlr_renderer_import_dmabuf(struct wlr_renderer *r,
struct wlr_dmabuf_buffer *dmabuf) {
return r->impl->import_dmabuf(r, dmabuf);

This comment has been minimized.

@emersion

emersion Mar 1, 2018

Member

Check if this function is NULL, return false if so?

@emersion emersion requested a review from ascent12 Mar 1, 2018

uint32_t offset[MAX_LINUX_BUFFER_PLANES];
uint32_t stride[MAX_LINUX_BUFFER_PLANES];
uint64_t modifier[MAX_LINUX_BUFFER_PLANES];
int fd[MAX_LINUX_BUFFER_PLANES];

This comment has been minimized.

@emersion

emersion Mar 1, 2018

Member

You could maybe instead use a struct for offset, stride, modifier and fd?

This comment has been minimized.

@ascent12

ascent12 Mar 2, 2018

Collaborator

It's much more consistent with other dmabuf APIs if it stays this way.

@ddevault

This comment has been minimized.

Member

ddevault commented Mar 2, 2018

Looking good so far. Take care to review the style guide, please.

@ascent12

This comment has been minimized.

Collaborator

ascent12 commented Mar 2, 2018

I would prefer if this was in terms of wlr_egl instead of wlr_renderer.
All of the gles2 dmabuf functions are just calling EGL functions. It's already capable of importing EGLImages, so we can just leave it at that. wlr_renderer is "supposed" (I don't think it actually is right now) to be an optional type, so I don't want it tangled with any of our other interfaces.

My work on Vulkan will certainly require rethinking part of this interface, and its relationship between the renderer, but that work probably won't land any time soon, so it shouldn't be considered a blocker.

@agx

This comment has been minimized.

Contributor

agx commented Mar 2, 2018

@ascent12

This comment has been minimized.

Collaborator

ascent12 commented Mar 2, 2018

Yeah, the relationship between wlr_egl and wlr_renderer isn't very obvious and not spelled out anywhere.
wlr_egl is geared towards integrating with the platform/backends, while wlr_renderer is just drawing stuff.
Importing dmabufs fits into 'integrating with the platform'.

The idea was that a library user could use wlr_egl with their own renderer code if they wanted to do something more advanced.

@agx

This comment has been minimized.

Contributor

agx commented Mar 5, 2018

@ascent12 Thanks for spelling this out so clearly. I'll drop the renderer indirection.

struct wlr_gles2_renderer *r = (struct wlr_gles2_renderer *)server->renderer;

This comment has been minimized.

@emersion

emersion Mar 6, 2018

Member

You can use wlr_backend_get_egl instead of casting the renderer.

@@ -8,6 +8,7 @@ void wlr_matrix_identity(float (*output)[16]);
void wlr_matrix_translate(float (*output)[16], float x, float y, float z);
void wlr_matrix_scale(float (*output)[16], float x, float y, float z);
void wlr_matrix_rotate(float (*output)[16], float radians);
void wlr_matrix_invert_y(float (*output)[16]);

This comment has been minimized.

@emersion

emersion Mar 7, 2018

Member

Seems that this one isn't relevant anymore.

@agx

This comment has been minimized.

Contributor

agx commented Mar 7, 2018

uint32_t version = wl_resource_get_version(linux_dmabuf_resource);
struct wlr_dmabuf_buffer *buffer = calloc(1, sizeof *buffer);
if (!buffer)

This comment has been minimized.

@ddevault

ddevault Mar 11, 2018

Member

Braces mandatory

goto err_out;
}
if ((uint64_t) buffer->attributes.offset[0] +

This comment has been minimized.

@ddevault

ddevault Mar 11, 2018

Member

No space after cast

}
struct wlr_linux_dmabuf *wlr_linux_dmabuf_create(struct wl_display *display,
struct wlr_egl *egl) {

This comment has been minimized.

@ddevault

ddevault Mar 11, 2018

Member

Not sure what happened here

@ddevault

This comment has been minimized.

Member

ddevault commented Mar 11, 2018

Style issues aside this LGTM

@agx

This comment has been minimized.

Contributor

agx commented Mar 11, 2018

@SirCmpwn thanks. I've cleaned up these style issues plus some more and shuffeled the patches around grouped by functionality.

@agx agx changed the title from [WiP] Add linux_dmabuf protocol support to Add initial linux_dmabuf protocol support Mar 11, 2018

int wlr_renderer_get_dmabuf_formats(struct wlr_renderer *r, int **formats);
int wlr_renderer_get_dmabuf_modifiers(struct wlr_renderer *r, int format,
uint64_t **modifiers);

This comment has been minimized.

@emersion

emersion Mar 11, 2018

Member

These three functions don't exist anymore.

#ifndef WLR_TYPES_WLR_SIMPLE_DMABUF_H
#define WLR_TYPES_WLR_SIMPLE_DMABUF_H
#define MAX_LINUX_BUFFER_PLANES 4

This comment has been minimized.

@emersion

emersion Mar 11, 2018

Member

This should be prefixed by WLR_LINUX_DMABUF since this is exported.

static void
handle_buffer_destroy(struct wl_resource *buffer_resource)
{

This comment has been minimized.

@emersion

emersion Mar 11, 2018

Member

Style nit: should be all three on the same line

};
struct wlr_dmabuf_buffer *wlr_dmabuf_buffer_from_params_resource(
struct wl_resource *params_resource) {

This comment has been minimized.

@emersion

emersion Mar 11, 2018

Member

Style nit: missing an extra tab here, so we see the difference between the function signature and the function body.

static void handle_params_destroy(struct wl_resource *params_resource) {
struct wlr_dmabuf_buffer *buffer = wl_resource_get_user_data(params_resource);
/* Check for NULL since wlr_dmabuf_buffer_from_params_resource will choke */
if (!buffer) {

This comment has been minimized.

@emersion

emersion Mar 11, 2018

Member

I'd prefer not casting here but rather write:

if (!wl_resource_get_user_data(params_resource)) {

So we can't by mistake use the unchecked struct wlr_dmabuf_buffer *

static void linux_dmabuf_create_params(struct wl_client *client,
struct wl_resource *linux_dmabuf_resource,
uint32_t params_id) {
struct wlr_linux_dmabuf *linux_dmabuf = wl_resource_get_user_data(linux_dmabuf_resource);

This comment has been minimized.

@emersion

emersion Mar 11, 2018

Member

There should be a wlr_linux_dmabuf_from_resource

goto err;
}
for (int i = 0; i < MAX_LINUX_BUFFER_PLANES; i++)

This comment has been minimized.

@emersion

emersion Mar 11, 2018

Member

Style nit: brackets wanted!

buffer->params_resource = wl_resource_create(client,
&zwp_linux_buffer_params_v1_interface,
version, params_id);
if (!buffer->params_resource)

This comment has been minimized.

@emersion

emersion Mar 11, 2018

Member

Style nit: ditto

static const struct zwp_linux_dmabuf_v1_interface linux_dmabuf_impl = {
linux_dmabuf_destroy,
linux_dmabuf_create_params

This comment has been minimized.

@emersion

emersion Mar 11, 2018

Member

Style nit: this isn't a tab

modifier_hi,
modifier_lo);
}
if (modifiers != &modifier_invalid)

This comment has been minimized.

@emersion

emersion Mar 11, 2018

Member

Style nit: brackets

wl_resource_set_implementation(resource, &linux_dmabuf_impl,
linux_dmabuf, NULL);
if (version < ZWP_LINUX_DMABUF_V1_MODIFIER_SINCE_VERSION)

This comment has been minimized.

@emersion

emersion Mar 11, 2018

Member

Style nit: brackets

@agx

This comment has been minimized.

Contributor

agx commented Mar 12, 2018

Besides the style fixes (@emersion thanks for review) I've moved the check for YUV surfaces from gles2_texture_upload_dmabuf to wlr_egl_check_import_dmabuf so the client knows that we can't render the texture instead of just having a warning in the logs

@emersion

Sorry for the late final review.

Apart from the few nitpicks, LGTM!

@@ -61,6 +64,31 @@ EGLSurface wlr_egl_create_surface(struct wlr_egl *egl, void *window);
EGLImageKHR wlr_egl_create_image(struct wlr_egl *egl,
EGLenum target, EGLClientBuffer buffer, const EGLint *attribs);
/**
* Creates an egl image from the given dmabuf attributes. Check usability
* of the dmabuf with egl_check_import_dmabuf once first.

This comment has been minimized.

@emersion

emersion Mar 14, 2018

Member

Nitpick: should be with wlr_egl_check_import_dmabuf

/**
* Get the available dmabuf formats
*/
int wlr_egl_get_dmabuf_formats(struct wlr_egl *egl, int **formats);

This comment has been minimized.

@emersion

emersion Mar 14, 2018

Member

Nitpick: here's an extra tab here

@@ -0,0 +1,80 @@
#ifndef WLR_TYPES_WLR_SIMPLE_DMABUF_H
#define WLR_TYPES_WLR_SIMPLE_DMABUF_H

This comment has been minimized.

@emersion

emersion Mar 14, 2018

Member

Nitpick: should be WLR_TYPES_WLR_LINUX_DMABUF_H

* Create linux-dmabuf interface
*/
struct wlr_linux_dmabuf *wlr_linux_dmabuf_create(struct wl_display *display,
struct wlr_egl *egl);

This comment has been minimized.

@emersion

emersion Mar 14, 2018

Member

Nitpick: no need to indent the second line with so many tabs/spaces, one is enough.

wl_resource_post_error(params_resource,
ZWP_LINUX_BUFFER_PARAMS_V1_ERROR_ALREADY_USED,
"params was already used to create a wl_buffer");
return;

This comment has been minimized.

@emersion

emersion Mar 14, 2018

Member

This branch is never reached, because wlr_dmabuf_buffer_from_params_resource asserts that the buffer isn't NULL.

A fix would be to replace the condition by if (!wl_resource_get_user_data(params_resource)) { and move the wlr_dmabuf_buffer_from_params_resource call below.

@agx

This comment has been minimized.

Contributor

agx commented Mar 15, 2018

@emersion thanks for the review. I've fixed that and also plugged a resoure leak in wlr_egl_check_import_dmabuf where we I did not free the eglimage in case of a YUV format, dropped the unused user_data and fixed typo comments.

@agx

This comment has been minimized.

Contributor

agx commented Mar 15, 2018

I have an additional patch that adds support for multiple planes:

agx@a279202

shall I put that on on top or use a different PR? I can't really test this since I dont have NV12 support but it gets as far as with weston so it shouldn't make things worse.

@@ -325,6 +325,10 @@ static void wlr_surface_apply_damage(struct wlr_surface *surface,
surface->current->buffer)) {
wlr_texture_upload_drm(surface->texture, surface->current->buffer);
goto release;
} else if (wlr_dmabuf_buffer_from_buffer_resource(
surface->current->buffer)) {

This comment has been minimized.

@emersion

emersion Mar 15, 2018

Member

wlr_dmabuf_buffer_from_buffer_resource aborts if the resource isn't a dmabuf buffer or if the buffer is NULL.

You seem to assume here (and in other places too, grep for wlr_dmabuf_buffer_from_buffer_resource) that it returns NULL instead of aborting. Maybe a wlr_dmabuf_resource_is_buffer could be used instead?

agx added some commits Feb 23, 2018

Add initial linux_dmabuf protocol support
Tested with

    ./weston-simple-dmabuf-drm
    ./weston-simple-dmabuf-drm --import-immediate=1
    ./weston-simple-dmabuf-drm --y-inverted=1
    (and combinations)

Supports only single plane XRGB dmabufs for now.
Indent GLSL by two spaces
since this is the most established indentation
@agx

This comment has been minimized.

Contributor

agx commented Mar 15, 2018

@emersion a leftover from when the wlr_dmabuf_buffer_from_buffer_resource didn't have the asserts. wlr_dmabuf_resource_is_buffer sounds good so I added it.

@emersion emersion merged commit de0e40d into swaywm:master Mar 15, 2018

1 check passed

builds.sr.ht builds.sr.ht job completed successfully
Details
@emersion

This comment has been minimized.

Member

emersion commented Mar 15, 2018

Thanks!

I have an additional patch that adds support for multiple planes

Let's do this in a new PR, make sure to provide a test plan!

@ddevault

This comment has been minimized.

Member

ddevault commented Mar 15, 2018

Thanks!

@agx

This comment has been minimized.

Contributor

agx commented Mar 15, 2018

That was 2d0db16 in particular. Thanks for fixing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment