Re: [PATCH weston v2 2/3] compositor-drm: Add scanout support for linux_dmabuf buffers

2016-10-06 Thread Esaki Tomohito 
Hi, Fabien

Thanks for your reviewing.

On 2016/10/05 0:45, Fabien DESSENNE wrote:
> On 09/30/2016 11:28 AM, Tomohito Esaki wrote:
>> +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;
> Just like in drm_fb_get_from_bo(), add an error log "bo geometry out of
> bounds"

I removed this logging in v2 because of the possibility of flooding the
compoitor log, as Pekka pointed out (see below).

Pekka wrote below for v1:

On 2016/06/15 17:20, Pekka Paalanen wrote:
>
> Is there a possibility that if this triggers, it will trigger every
> frame and flood the compositor log?
>
> If yes, I'd just remove this logging. The rest below are fine by me,
> since those should not be failing, right?

(https://lists.freedesktop.org/archives/wayland-devel/2016-June/029466.html)


Otherwise I will update patches as you suggested.

Best regards.

-- 
--
IGEL Co., Ltd.
Tomohito Esaki
e...@igel.co.jp
___
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

2016-10-06 Thread Esaki Tomohito 
Hi, Derek

Thanks for your reviewing and testing.

On 2016/10/04 3:20, Derek Foreman wrote:
> 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?

I tried testing on my ARM based platform and also on Intel Graphics and
AMD stacks. I modified simple-dmabuf-intel as follows:

* Create dmabuf buffer by using drmIoctl(fd, DRM_IOCTL_MODE_DUMB, )
  insted of the Intel specific API.
* Add full screen mode based on simple-egl.

I tried exit()ing with a full screen dmabuf surface on scanout and on
the Intel and AMD platforms weston crashed, but there wasn't any
problem on my ARM board. Our graphics drivers don't use mesa, which the
Intel and AMD do, so that may be a factor, but I'm not sure.

Best regards.

-- 
--
IGEL Co., Ltd.
Tomohito Esaki
e...@igel.co.jp
___
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

2016-10-04 Thread Fabien DESSENNE 
Hi


Thank you for the patch. Please, find some minor comments below.


On 09/30/2016 11:28 AM, Tomohito Esaki wrote:
> This implementations bypasses gbm and passes the dmabuf handles directly
> to libdrm for composition.

Maybe you can split the patch in two parts:
- one part dealing with drm_fb_create_dmabuf (since this part is used by 
both scanout and sprites)
- a second part dealing with scanout

>
> Signed-off-by: Tomohito Esaki 
> ---
>   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, _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, _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;
I do not think that this check is needed (you already checked format in 
the caller function + you can delegate this error handling to drmModeAddFB2)

> +
> + 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;
Just like in drm_fb_get_from_bo(), add an error log "bo geometry out of 
bounds"

> +
> + for (i = 0; i < dmabuf->attributes.n_planes; i++) {
> + ret = drmPrimeFDToHandle(backend->drm.fd,
> +  dmabuf->attributes.fd[i],
> +  [i]);
> + if (ret)
Add an error log here too (ex "drmPrimeFDToHandle failed")

> + goto done;
> + }
> +
> + ret = drmModeAddFB2(backend->drm.fd, width, height,
> + format, handles, dmabuf->attributes.stride,
> + dmabuf->attributes.offset, _id, 0);
Flags are ignore here (using "0"). Suggesting to use something like this:
 if (dmabuf->attributes.flags & 
ZWP_LINUX_BUFFER_PARAMS_V1_FLAGS_INTERLACED)
 flags |= DRM_MODE_FB_INTERLACED;

> + if (ret)
> + goto done;
Just like in drm_fb_get_from_bo(), add an error log  "addfb2 failed"

> +
> + 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_format(bo);
> + /* We relay on the GBM format enum and DRM format enum to be
> +identical */
>   
>   if (format == GBM_FORMAT_ARGB) {
>   /* We can scanout an ARGB buffer if the surface's
> @@ -521,12 +596,13 @@

Re: [PATCH weston v2 2/3] compositor-drm: Add scanout support for linux_dmabuf buffers

2016-10-03 Thread Derek Foreman 

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, _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, _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],
+[i]);
+   if (ret)
+   goto done;
+   }
+
+   ret = drmModeAddFB2(backend->drm.fd, width, height,
+   format, handles, dmabuf->attributes.stride,
+   dmabuf->attributes.offset, _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;

-

Re: [PATCH weston v2 2/3] compositor-drm: Add scanout support for linux_dmabuf buffers

2016-09-30 Thread Eric Engestrom 
On Fri, Sep 30, 2016 at 06:28:52PM +0900, Tomohito Esaki wrote:
> This implementations bypasses gbm and passes the dmabuf handles directly
> to libdrm for composition.
> 
> Signed-off-by: Tomohito Esaki 
> ---
>  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, _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, _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],
> +  [i]);
> + if (ret)
> + goto done;
> + }
> +
> + ret = drmModeAddFB2(backend->drm.fd, width, height,
> + format, handles, dmabuf->attributes.stride,
> + dmabuf->attributes.offset, _id, 0);
> + if (ret)
> + goto done;
> +
> + fb = zalloc(sizeof *fb);
> + if (!fb)
> + goto done;

Nit: indentation

> +
> + 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]);

No need for `continue`, you can remove it and invert the if.

> + }
> +
> + return fb;
> +}
> +
> +static void
> +drm_fb_destroy_dmabuf(struct drm_fb *fb)
> +{
> + drm_fb_destroy(fb, fb->fd);
> +}

Not sure this is really needed :)

> +
>  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_format(bo);
> + /* We relay on the GBM format enum and DRM format enum to be

"rely"

Otherwise it looks good :)

Cheers,
  Eric

> +identical */
>  
>   if (format == GBM_FORMAT_ARGB) {
>   /* We can scanout an ARGB buffer if the surface's
> @@ -521,12 +596,13 @@ drm_output_prepare_scanout_view(struct drm_output 
> *output,
>   struct drm_backend *b = to_drm_backend(output->base.compositor);
>   struct weston_buffer *buffer = ev->surface->buffer_ref.buffer;
>   struct weston_buffer_viewport *viewport = >surface->buffer_viewport;
> - struct gbm_bo *bo;
> + struct linux_dmabuf_buffer *dmabuf;
> + struct gbm_bo *bo = NULL;
>   uint32_t format;
>  
>   if (ev->geometry.x != output->base.x ||
>   ev->geometry.y != output->base.y ||
> - buffer == NULL || b->gbm == NULL ||
> + buffer == NULL ||
>   buffer->width != output->base.current_mode->width ||
>

[PATCH weston v2 2/3] compositor-drm: Add scanout support for linux_dmabuf buffers

2016-09-30 Thread Tomohito Esaki 
This implementations bypasses gbm and passes the dmabuf handles directly
to libdrm for composition.

Signed-off-by: Tomohito Esaki 
---
 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, _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, _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],
+[i]);
+   if (ret)
+   goto done;
+   }
+
+   ret = drmModeAddFB2(backend->drm.fd, width, height,
+   format, handles, dmabuf->attributes.stride,
+   dmabuf->attributes.offset, _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_format(bo);
+   /* We relay on the GBM format enum and DRM format enum to be
+  identical */
 
if (format == GBM_FORMAT_ARGB) {
/* We can scanout an ARGB buffer if the surface's
@@ -521,12 +596,13 @@ drm_output_prepare_scanout_view(struct drm_output *output,
struct drm_backend *b = to_drm_backend(output->base.compositor);
struct weston_buffer *buffer = ev->surface->buffer_ref.buffer;
struct weston_buffer_viewport *viewport = >surface->buffer_viewport;
-   struct gbm_bo *bo;
+   struct linux_dmabuf_buffer *dmabuf;
+   struct gbm_bo *bo = NULL;
uint32_t format;
 
if (ev->geometry.x != output->base.x ||
ev->geometry.y != output->base.y ||
-   buffer == NULL || b->gbm == NULL ||
+   buffer == NULL ||
buffer->width != output->base.current_mode->width ||
buffer->height != output->base.current_mode->height ||
output->base.transform != viewport->buffer.transform ||
@@ -536,22 +612,35 @@ drm_output_prepare_scanout_view(struct drm_output *output,
if (ev->geometry.scissor_enabled)
return NULL;
 
-   bo = gbm_bo_import(b->gbm, GBM_BO_IMPORT_WL_BUFFER,
-