Re: [PATCH weston v2 3/3] compositor-drm: Support linux_dmabuf output for sprite planes without gbm
On 09/30/2016 08:49 PM, Derek Foreman wrote:
> On 30/09/16 04:28 AM, Tomohito Esaki wrote:
>> Multiplanar formats are supported by using drmModeAddFB2 and bypassing
>> gbm. If drmModeAddFB2 isn't available, the existing gbm bo import path
>> is used and multiplanar formats are unsupported.
> I'm not sure we should be doing anything with the existing sprite code
> at all (except perhaps removing it, though I'm sure that's an unpopular
> point of view :) since it's not really viable without atomic mode
> setting, and is currently disabled until someone uses a debug key to
> enable it.
>
> Pekka - I can't recall, is the atomic mode setting series going to build
> on the current sprite stuff or blow it away and start over?
>
> I'm wondering if we should just tear out the existing sprite code in the
> meantime...
>
> Thanks,
> Derek
Hi Derek,
I do not know all the history of Weston, and I am not aware of all the
reasons that made the sprites "broken". I have always been told that it
was not a good idea to enable sprites because it was not working as
expected unless we have the atomic mode.
Anyway, with my limited background, i'd like to share my point of view:
Firstly, sprite works quite fine (with a bunch of fixes that I will be
glad to share once sprites are accepted by all) : I can have up to 3
sprites (inluding gstreamer video playback with linux-dmabuf) + cursor
working smoothly on two parallel outputs (extended desktop) So, sprites
are probably a bit broken, but not totally out of order. We just still
need to improve it.
Secondly, it is a (very) long time now, that I am waiting for atomic. I
discussed a bit about it recently with Pekka and my understanding is
that because people are quite busy, atomic is not about to land in main
in a short time.
So, for theses two reasons, I think that is a good idea to improve
sprites now : let's keep them marked as broken for the time being.
The improvements proposed by Tomohito will serve as a basis for sprites
in atomic (I can confirm that the atomic WIP branch does not blow away
the sprite part, the prepare_overlay_view is still there)
For what it worth, I have recently played with the atomic WIP branch,
ported it to 1.11, and enabled linux-dmabuf in sprites. Not 100% perfect
but it works. I know that I am not the only one who did this job
recently, so I guess that we shall not have so many problem to find
people to contribute to the atomic revival (with unbroken sprites inside)
BR
Fabien, a sprite fan ;)
>> Signed-off-by: Tomohito Esaki
>> ---
>> libweston/compositor-drm.c | 53
>> +++---
>> 1 file changed, 26 insertions(+), 27 deletions(-)
>>
>> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
>> index b15fa01..f0e6f7c 100644
>> --- a/libweston/compositor-drm.c
>> +++ b/libweston/compositor-drm.c
>> @@ -1008,11 +1008,9 @@ page_flip_handler(int fd, unsigned int frame,
>>
>> static uint32_t
>> drm_output_check_sprite_format(struct drm_sprite *s,
>> - struct weston_view *ev, struct gbm_bo *bo)
>> + struct weston_view *ev, uint32_t format)
>> {
>> -uint32_t i, format;
>> -
>> -format = gbm_bo_get_format(bo);
>> +uint32_t i;
>>
>> if (format == GBM_FORMAT_ARGB) {
>> pixman_region32_t r;
>> @@ -1053,15 +1051,12 @@ drm_output_prepare_overlay_view(struct drm_output
>> *output,
>> struct drm_sprite *s;
>> struct linux_dmabuf_buffer *dmabuf;
>> int found = 0;
>> -struct gbm_bo *bo;
>> +struct gbm_bo *bo = NULL;
>> pixman_region32_t dest_rect, src_rect;
>> pixman_box32_t *box, tbox;
>> uint32_t format;
>> wl_fixed_t sx1, sy1, sx2, sy2;
>>
>> -if (b->gbm == NULL)
>> -return NULL;
>> -
>> if (viewport->buffer.transform != output->base.transform)
>> return NULL;
>>
>> @@ -1101,15 +1096,9 @@ drm_output_prepare_overlay_view(struct drm_output
>> *output,
>> if (!found)
>> return NULL;
>>
>> -if ((dmabuf = linux_dmabuf_buffer_get(buffer_resource))) {
>> +if ((dmabuf = linux_dmabuf_buffer_get(buffer_resource)) &&
>> +b->no_addfb2 && b->gbm) {
>> #ifdef HAVE_GBM_FD_IMPORT
>> -/* XXX: TODO:
>> - *
>> - * Use AddFB2 directly, do not go via GBM.
>> - * Add support for multiplanar formats.
>> - * Both require refactoring in the DRM-backend to
>> - * support a mix of gbm_bos and drmfbs.
>> - */
>> struct gbm_import_fd_data gbm_dmabuf = {
>> .fd = dmabuf->attributes.fd[0],
>> .width = dmabuf->attributes.width,
>> @@ -1126,22 +1115,32 @@ drm_output_prepare_overlay_view(struct drm_output
>> *output,
>> #else
>> return NULL;
>> #endif
>> -} else {
>> +} else if (b->gbm) {
>> bo = gbm_bo_import(b->gbm, GBM_BO_IMPORT_WL_
Re: [weston 2/2] gl-renderer: add support of WL_SHM_FORMAT_NV12
Hi Vincent, On 5 August 2016 at 10:21, Vincent Abriou wrote: > This patch allow weston to accept WL_SHM_FORMAT_NV12 buffers. > > It has been tested on top of weston-1.11 Both of these patches have some quite long lines: could you please wrap to 80 characters? > @@ -1352,6 +1353,9 @@ gl_renderer_attach_shm(struct weston_surface *es, > struct weston_buffer *buffer, > buffer->height = wl_shm_buffer_get_height(shm_buffer); > > num_planes = 1; > + gl_format[0] = GL_LUMINANCE; > + gl_format[1] = GL_LUMINANCE; > + gl_format[2] = GL_LUMINANCE; It would be good to use GL_R8/RG8_EXT here rather than LUMINANCE/LUMINANCE_ALPHA here (if GL_EXT_texture_rg is present), as I remember the latter two being deprecated and/or having odd sampling semantics. Also, you might as well just remove this magic up-front initialisation anyway, since most formats don't use LUMINANCE. With those fixed, for the series: Reviewed-by: Daniel Stone Cheers, Daniel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston v2 3/3] compositor-drm: Support linux_dmabuf output for sprite planes without gbm
On 03/10/16 06:03 AM, Fabien DESSENNE wrote:
On 09/30/2016 08:49 PM, Derek Foreman wrote:
On 30/09/16 04:28 AM, Tomohito Esaki wrote:
Multiplanar formats are supported by using drmModeAddFB2 and bypassing
gbm. If drmModeAddFB2 isn't available, the existing gbm bo import path
is used and multiplanar formats are unsupported.
I'm not sure we should be doing anything with the existing sprite code
at all (except perhaps removing it, though I'm sure that's an unpopular
point of view :) since it's not really viable without atomic mode
setting, and is currently disabled until someone uses a debug key to
enable it.
Pekka - I can't recall, is the atomic mode setting series going to build
on the current sprite stuff or blow it away and start over?
I'm wondering if we should just tear out the existing sprite code in the
meantime...
Thanks,
Derek
Hi Derek,
I do not know all the history of Weston, and I am not aware of all the
reasons that made the sprites "broken". I have always been told that it
was not a good idea to enable sprites because it was not working as
expected unless we have the atomic mode.
Sprites are broken because without atomic mode setting it's impossible
to update all active sprites, well, atomically.
There's no way to ensure they all change in the same screen refresh
interval - we may have tearing, or on some driver stacks I think we end
up with vblank waits during plane setting (I think this bit us with the
cursor plane on intel drivers a while ago, and the cursor plane was made
a special case to help...)
Since tearing is something wayland was intended to do away with
entirely, having an option to enable to bring it back seems like a bad
move in weston.
Anyway, with my limited background, i'd like to share my point of view:
Firstly, sprite works quite fine (with a bunch of fixes that I will be
glad to share once sprites are accepted by all) : I can have up to 3
sprites (inluding gstreamer video playback with linux-dmabuf) + cursor
working smoothly on two parallel outputs (extended desktop) So, sprites
are probably a bit broken, but not totally out of order. We just still
need to improve it.
Unfortunately there are things (the tearing) that are unimprovable
without atomic underneath, so I'm worried about the testability of
sprite changes without atomic mode setting landing first.
Secondly, it is a (very) long time now, that I am waiting for atomic. I
discussed a bit about it recently with Pekka and my understanding is
that because people are quite busy, atomic is not about to land in main
in a short time.
This truly is a shame, I'd hoped to see it by now as well.
So, for theses two reasons, I think that is a good idea to improve
sprites now : let's keep them marked as broken for the time being.
The improvements proposed by Tomohito will serve as a basis for sprites
in atomic (I can confirm that the atomic WIP branch does not blow away
the sprite part, the prepare_overlay_view is still there)
For what it worth, I have recently played with the atomic WIP branch,
ported it to 1.11, and enabled linux-dmabuf in sprites. Not 100% perfect
but it works. I know that I am not the only one who did this job
recently, so I guess that we shall not have so many problem to find
people to contribute to the atomic revival (with unbroken sprites inside)
My fingers are crossed - I see this (both atomic mode setting, and the
ability to put dmabuf buffers into planes) as incredibly important work too.
I won't bother posting any patches to remove existing sprite
functionality - it seems they wouldn't be well received. :)
Thanks,
Derek
BR
Fabien, a sprite fan ;)
Signed-off-by: Tomohito Esaki
---
libweston/compositor-drm.c | 53 +++---
1 file changed, 26 insertions(+), 27 deletions(-)
diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
index b15fa01..f0e6f7c 100644
--- a/libweston/compositor-drm.c
+++ b/libweston/compositor-drm.c
@@ -1008,11 +1008,9 @@ page_flip_handler(int fd, unsigned int frame,
static uint32_t
drm_output_check_sprite_format(struct drm_sprite *s,
- struct weston_view *ev, struct gbm_bo *bo)
+ struct weston_view *ev, uint32_t format)
{
- uint32_t i, format;
-
- format = gbm_bo_get_format(bo);
+ uint32_t i;
if (format == GBM_FORMAT_ARGB) {
pixman_region32_t r;
@@ -1053,15 +1051,12 @@ drm_output_prepare_overlay_view(struct drm_output
*output,
struct drm_sprite *s;
struct linux_dmabuf_buffer *dmabuf;
int found = 0;
- struct gbm_bo *bo;
+ struct gbm_bo *bo = NULL;
pixman_region32_t dest_rect, src_rect;
pixman_box32_t *box, tbox;
uint32_t format;
wl_fixed_t sx1, sy1, sx2, sy2;
- if (b->gbm == NULL)
- return NULL;
-
if (viewport->buffer.transform != output->base.transform)
retu
Re: [PATCH weston v2 3/3] compositor-drm: Support linux_dmabuf output for sprite planes without gbm
Hi,
On 3 October 2016 at 15:01, Derek Foreman wrote:
> On 03/10/16 06:03 AM, Fabien DESSENNE wrote:
>> I do not know all the history of Weston, and I am not aware of all the
>> reasons that made the sprites "broken". I have always been told that it
>> was not a good idea to enable sprites because it was not working as
>> expected unless we have the atomic mode.
>
> Sprites are broken because without atomic mode setting it's impossible to
> update all active sprites, well, atomically.
>
> There's no way to ensure they all change in the same screen refresh interval
> - we may have tearing, or on some driver stacks I think we end up with
> vblank waits during plane setting (I think this bit us with the cursor plane
> on intel drivers a while ago, and the cursor plane was made a special case
> to help...)
>
> Since tearing is something wayland was intended to do away with entirely,
> having an option to enable to bring it back seems like a bad move in weston.
Yes. The semantics are incredibly underspecified, and the drivers
diverge so much in implementation that we can never really have a
reliable implementation. If you don't have a vblank wait, then you
really need to do a pageflip of the primary plane as well to get the
event to drive things off. Or use the vblank callbacks, which is
really broken and racy. There's no way to ever fix that API properly.
>> Secondly, it is a (very) long time now, that I am waiting for atomic. I
>> discussed a bit about it recently with Pekka and my understanding is
>> that because people are quite busy, atomic is not about to land in main
>> in a short time.
>
> This truly is a shame, I'd hoped to see it by now as well.
Er, yeah.
>> So, for theses two reasons, I think that is a good idea to improve
>> sprites now : let's keep them marked as broken for the time being.
>> The improvements proposed by Tomohito will serve as a basis for sprites
>> in atomic (I can confirm that the atomic WIP branch does not blow away
>> the sprite part, the prepare_overlay_view is still there)
>>
>> For what it worth, I have recently played with the atomic WIP branch,
>> ported it to 1.11, and enabled linux-dmabuf in sprites. Not 100% perfect
>> but it works. I know that I am not the only one who did this job
>> recently, so I guess that we shall not have so many problem to find
>> people to contribute to the atomic revival (with unbroken sprites inside)
>
> My fingers are crossed - I see this (both atomic mode setting, and the
> ability to put dmabuf buffers into planes) as incredibly important work too.
Yes - I think the dmabuf patches from Tomohito-san are very useful,
and we should push those forward.
> I won't bother posting any patches to remove existing sprite functionality -
> it seems they wouldn't be well received. :)
Honestly, I've been leaning towards ripping them out as well.
Unfortunately a lot of things have come up this year which have kept
me from spending quality time with the atomic code, but I'm now back
on it and setting myself pretty aggressive targets to get new
revisions out there. It's long past due, and really needs to be
merged.
What held things up last time, is that I finally hit the breaking
point with drm_{output,sprite}::{current,next}. The entire setup is so
fragile that the only proper fix - which aligns nicely with the
kernel's atomic API - is to split things up into three state members:
one each for actually-on-screen-now,
submitted-to-kernel-but-not-there-yet, and in-flight. Doing this was a
lot of churn, and hit a lot of fragile points, especially in the
sprite code. Then when we're finished, we end up with two codepaths
for sprites. compositor-drm becomes pretty huge.
Given that we've had it not just disabled by default, but requiring
either patches or user intervention _at every run_ to enable, and that
the users (e.g. ST) here have competent atomic KMS drivers, I'd be
very much tempted to make my own life easier by removing it as a
prelude to the atomic patches, and building back up from there.
Anyway, I'll review these patches this week, and send out v(n+1) of
atomic next. Thanks!
Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston v2 1/3] compositor-drm: refactor destroy drm_fb function
On 30/09/16 04:28 AM, Tomohito Esaki wrote:
The drm_fb destroy callback to mostly the same thing regardless of
whether the buffer is a dumb buffer or gbm buffer. This patch refactors
the common parts into a new function that can be called for both cases.
Signed-off-by: Tomohito Esaki
---
libweston/compositor-drm.c | 31 ---
1 file changed, 16 insertions(+), 15 deletions(-)
Looks functionally identical and a little simpler, so:
Reviewed-by: Derek Foreman
Thanks,
Derek
diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
index 8319d7c..a707fc4 100644
--- a/libweston/compositor-drm.c
+++ b/libweston/compositor-drm.c
@@ -261,17 +261,23 @@ drm_sprite_crtc_supported(struct drm_output *output,
uint32_t supported)
}
static void
-drm_fb_destroy_callback(struct gbm_bo *bo, void *data)
+drm_fb_destroy(struct drm_fb *fb, int fd)
{
- struct drm_fb *fb = data;
- struct gbm_device *gbm = gbm_bo_get_device(bo);
-
if (fb->fb_id)
- drmModeRmFB(gbm_device_get_fd(gbm), fb->fb_id);
+ drmModeRmFB(fd, fb->fb_id);
weston_buffer_reference(&fb->buffer_ref, NULL);
- free(data);
+ free(fb);
+}
+
+static void
+drm_fb_destroy_gbm_fb(struct gbm_bo *bo, void *data)
+{
+ struct drm_fb *fb = data;
+ struct gbm_device *gbm = gbm_bo_get_device(bo);
+
+ drm_fb_destroy(fb, gbm_device_get_fd(gbm));
}
static struct drm_fb *
@@ -370,22 +376,17 @@ static void
drm_fb_destroy_dumb(struct drm_fb *fb)
{
struct drm_mode_destroy_dumb destroy_arg;
+ int fd = fb->fd;
if (!fb->map)
return;
- if (fb->fb_id)
- drmModeRmFB(fb->fd, fb->fb_id);
-
- weston_buffer_reference(&fb->buffer_ref, NULL);
-
munmap(fb->map, fb->size);
-
memset(&destroy_arg, 0, sizeof(destroy_arg));
destroy_arg.handle = fb->handle;
- drmIoctl(fb->fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroy_arg);
- free(fb);
+ drm_fb_destroy(fb, fd);
+ drmIoctl(fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroy_arg);
}
static struct drm_fb *
@@ -446,7 +447,7 @@ drm_fb_get_from_bo(struct gbm_bo *bo,
goto err_free;
}
- gbm_bo_set_user_data(bo, fb, drm_fb_destroy_callback);
+ gbm_bo_set_user_data(bo, fb, drm_fb_destroy_gbm_fb);
return fb;
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston v2 2/3] compositor-drm: Add scanout support for linux_dmabuf buffers
On 30/09/16 04:28 AM, Tomohito Esaki wrote:
This implementations bypasses gbm and passes the dmabuf handles directly
to libdrm for composition.
Signed-off-by: Tomohito Esaki
Very cool work!
Acked-by: Derek Foreman
I don't see anything here that Eric hasn't already mentioned, but I'm
having a problem testing it.
Running terminology (an EFL terminal emulator) with:
ELM_ACCEL=shm EVAS_WAYLAND_SHM_USE_DMABUF=1 terminology
if I hit f11 to go fullscreen, then ctrl-d to exit while fullscreen,
weston exit()s directly from eglSwapBuffers() which logs:
intel_do_flush_locked failed: No such file or directory
While it was fullscreen damage tracking was completely flaky as well.
I'm not 100% confident in EFL's damage updates, but it works well
fullscreen without this weston patch...
I've also tested with the intel-simple-dmabuf.c included with weston - I
just added a zxdg_toplevel_v6_set_fullscreen(window->xdg_toplevel,
NULL); at the very end of create_window(), and changed the
create_window() call to have the same resolution as my screen - it also
hits the scanout path.
If I mess up redraw() to call exit(1); after drawing 100 frames I get
the same weston crash with intel_do_flush_locked failed.
I don't have a lot of time to dig into this right now - can you test an
exit() with a full screen dmabuf surface on scanout on your driver stack?
Thanks,
Derek
---
libweston/compositor-drm.c | 125 ++---
1 file changed, 107 insertions(+), 18 deletions(-)
diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
index a707fc4..b15fa01 100644
--- a/libweston/compositor-drm.c
+++ b/libweston/compositor-drm.c
@@ -151,6 +151,9 @@ struct drm_fb {
/* Used by dumb fbs */
void *map;
+
+ /* Used by dmabuf */
+ bool is_dmabuf;
};
struct drm_edid {
@@ -389,6 +392,76 @@ drm_fb_destroy_dumb(struct drm_fb *fb)
drmIoctl(fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroy_arg);
}
+static inline void
+close_drm_handle(int fd, uint32_t handle)
+{
+ struct drm_gem_close gem_close = { .handle = handle };
+ int ret;
+
+ ret = drmIoctl(fd, DRM_IOCTL_GEM_CLOSE, &gem_close);
+ if (ret)
+ weston_log("DRM_IOCTL_GEM_CLOSE failed.(%s)\n",
+ strerror(errno));
+}
+
+static struct drm_fb *
+drm_fb_create_dmabuf(struct linux_dmabuf_buffer *dmabuf,
+struct drm_backend *backend, uint32_t format)
+{
+ struct drm_fb *fb = NULL;
+ uint32_t width, height, fb_id, handles[4] = {0};
+ int i, ret;
+
+ if (!format)
+ return NULL;
+
+ width = dmabuf->attributes.width;
+ height = dmabuf->attributes.height;
+ if (backend->min_width > width ||
+ width > backend->max_width ||
+ backend->min_height > height ||
+ height > backend->max_height)
+ return NULL;
+
+ for (i = 0; i < dmabuf->attributes.n_planes; i++) {
+ ret = drmPrimeFDToHandle(backend->drm.fd,
+dmabuf->attributes.fd[i],
+&handles[i]);
+ if (ret)
+ goto done;
+ }
+
+ ret = drmModeAddFB2(backend->drm.fd, width, height,
+ format, handles, dmabuf->attributes.stride,
+ dmabuf->attributes.offset, &fb_id, 0);
+ if (ret)
+ goto done;
+
+ fb = zalloc(sizeof *fb);
+ if (!fb)
+ goto done;
+
+ fb->fb_id = fb_id;
+ fb->stride = dmabuf->attributes.stride[0];
+ fb->fd = backend->drm.fd;
+ fb->is_dmabuf = true;
+
+done:
+ for (i = 0; i < dmabuf->attributes.n_planes; i++) {
+ if (!handles[i])
+ continue;
+ close_drm_handle(backend->drm.fd, handles[i]);
+ }
+
+ return fb;
+}
+
+static void
+drm_fb_destroy_dmabuf(struct drm_fb *fb)
+{
+ drm_fb_destroy(fb, fb->fd);
+}
+
static struct drm_fb *
drm_fb_get_from_bo(struct gbm_bo *bo,
struct drm_backend *backend, uint32_t format)
@@ -475,6 +548,8 @@ drm_output_release_fb(struct drm_output *output, struct
drm_fb *fb)
if (fb->map &&
(fb != output->dumb[0] && fb != output->dumb[1])) {
drm_fb_destroy_dumb(fb);
+ } else if (fb->is_dmabuf) {
+ drm_fb_destroy_dmabuf(fb);
} else if (fb->bo) {
if (fb->is_client_buffer)
gbm_bo_destroy(fb->bo);
@@ -486,12 +561,12 @@ drm_output_release_fb(struct drm_output *output, struct
drm_fb *fb)
static uint32_t
drm_output_check_scanout_format(struct drm_output *output,
- struct weston_surface *es, struct gbm_bo *bo)
+ struct weston_surface *es, uint32_t format)
{
- uint32_t format;
pixman_region32_t r;
- format = gbm_bo_get_f
