Re: [weston PATCH] simple-dmabuf-drm: support etnaviv drm as well

2018-03-15 Thread Emil Velikov 
On 15 March 2018 at 13:58, Pekka Paalanen  wrote:
> On Mon, 12 Mar 2018 16:41:46 +0100
> Guido Günther  wrote:
>
>> Since freedreno and etnaviv can live in parallel allow to build in
>> different DRM backends.
>> ---
>>  Makefile.am |   6 ++-
>>  clients/simple-dmabuf-drm.c | 122 
>> +++-
>>  configure.ac|  29 +++
>>  3 files changed, 121 insertions(+), 36 deletions(-)
>
> Hi,
>
> this seems mostly fine, but there are some issues noted below. It would
> be appropriate to split this into two patches: "allow intel and
> freedreno to be built together" and "add etnaviv support".
>
Just throwing it out there - one might be able to utilise the
gbm_bo_{un,}map API.
Haven't looked at the weston code, yet ^^ was sufficient for the
piglit tests + kmscube.

Less code, fewer vendor quirks ;-)

HTH
-Emil
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [weston PATCH] simple-dmabuf-drm: support etnaviv drm as well

2018-03-15 Thread Pekka Paalanen 
On Mon, 12 Mar 2018 16:41:46 +0100
Guido Günther  wrote:

> Since freedreno and etnaviv can live in parallel allow to build in
> different DRM backends.
> ---
>  Makefile.am |   6 ++-
>  clients/simple-dmabuf-drm.c | 122 
> +++-
>  configure.ac|  29 +++
>  3 files changed, 121 insertions(+), 36 deletions(-)

Hi,

this seems mostly fine, but there are some issues noted below. It would
be appropriate to split this into two patches: "allow intel and
freedreno to be built together" and "add etnaviv support".

> diff --git a/Makefile.am b/Makefile.am
> index e028a2a1..19319fa2 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -624,7 +624,11 @@ nodist_weston_simple_dmabuf_drm_SOURCES =
> \
>   protocol/linux-dmabuf-unstable-v1-protocol.c \
>   protocol/linux-dmabuf-unstable-v1-client-protocol.h
>  weston_simple_dmabuf_drm_CFLAGS = $(AM_CFLAGS) 
> $(SIMPLE_DMABUF_DRM_CLIENT_CFLAGS)
> -weston_simple_dmabuf_drm_LDADD = $(SIMPLE_DMABUF_DRM_CLIENT_LIBS) 
> $(LIBDRM_PLATFORM_LIBS) libshared.la
> +weston_simple_dmabuf_drm_LDADD = $(SIMPLE_DMABUF_DRM_CLIENT_LIBS) \
> + $(LIBDRM_PLATFORM_FREEDRENO_LIBS) \
> + $(LIBDRM_PLATFORM_ETNAVIV_LIBS)   \
> + $(LIBDRM_PLATFORM_INTEL_LIBS) \
> + libshared.la
>  endif
>  
>  if BUILD_SIMPLE_DMABUF_V4L_CLIENT
> diff --git a/clients/simple-dmabuf-drm.c b/clients/simple-dmabuf-drm.c
> index 1073930f..8b7acd39 100644
> --- a/clients/simple-dmabuf-drm.c
> +++ b/clients/simple-dmabuf-drm.c
> @@ -44,9 +44,13 @@
>  #ifdef HAVE_LIBDRM_INTEL
>  #include 
>  #include 
> -#elif HAVE_LIBDRM_FREEDRENO
> +#endif
> +#ifdef HAVE_LIBDRM_FREEDRENO
>  #include 
>  #endif
> +#ifdef HAVE_LIBDRM_ETNAVIV
> +#include 
> +#endif
>  #include 

It's nice to see that the three can all be built at the same time.

>  
>  #include 
> @@ -93,11 +97,16 @@ struct buffer {
>  
>  #ifdef HAVE_LIBDRM_INTEL
>   drm_intel_bufmgr *bufmgr;
> - drm_intel_bo *bo;
> -#elif HAVE_LIBDRM_FREEDRENO
> + drm_intel_bo *intel_bo;
> +#endif /* HAVE_LIBDRM_INTEL */
> +#if HAVE_LIBDRM_FREEDRENO
>   struct fd_device *fd_dev;
> - struct fd_bo *bo;
> + struct fd_bo *fd_bo;
>  #endif /* HAVE_LIBDRM_FREEDRENO */
> +#if HAVE_LIBDRM_ETNAVIV
> + struct etna_device *etna_dev;
> + struct etna_bo *etna_bo;
> +#endif /* HAVE_LIBDRM_ETNAVIV */

The mechanical renames seem to be fine.

> @@ -246,7 +256,57 @@ static
>  void fd_unmap_bo(struct buffer *buf)
>  {
>  }
> -#endif
> +#endif /* HAVE_LIBDRM_FREEDRENO */
> +#ifdef HAVE_LIBDRM_ETNAVIV
> +#define ALIGN(v, a) ((v + a - 1) & ~(a - 1))
> +
> +static
> +int etna_alloc_bo(struct buffer *buf)

It seems freedreno was consistently using an incorrect style and you
copied it. Please follow the intel style instead, that is:

static int
etna_alloc_bo(...

That's what we use everywhere in weston.

> +{
> + int flags = DRM_ETNA_GEM_CACHE_WC;
> + int size = buf->width * buf->height * buf->bpp / 8;
> + buf->etna_dev = etna_device_new(buf->drm_fd);
> +
> + buf->etna_bo = etna_bo_new(buf->etna_dev, size, flags);
> +
> + if (!buf->etna_bo)
> + return 0;
> + buf->stride = ALIGN(buf->width, 32) * buf->bpp / 8;

ALIGN rounds up, right?

So, if you allocate 'size' bytes, and then the caller computes stride *
height, they end up accessing out of bounds, no?

> + return 1;
> +}
> +
> +static
> +void etna_free_bo(struct buffer *buf)
> +{
> + etna_bo_del(buf->etna_bo);
> +}
> +
> +static
> +int etna_bo_export_to_prime(struct buffer *buf)
> +{
> + buf->dmabuf_fd = etna_bo_dmabuf(buf->etna_bo);
> + if (buf->dmabuf_fd > 0)
> + return 0;
> +
> + return 1;
> +}
> +
> +static
> +int etna_map_bo(struct buffer *buf)
> +{
> + buf->mmap = etna_bo_map(buf->etna_bo);
> +
> + if (buf->mmap != NULL)
> + return 1;
> +
> + return 0;
> +}
> +
> +static
> +void etna_unmap_bo(struct buffer *buf)
> +{
> +}
> +#endif /* HAVE_LIBDRM_ENTAVIV */
>  
>  static void
>  fill_content(struct buffer *my_buf)
> @@ -278,9 +338,13 @@ drm_device_destroy(struct buffer *buf)
>  {
>  #ifdef HAVE_LIBDRM_INTEL
>   drm_intel_bufmgr_destroy(buf->bufmgr);
> -#elif HAVE_LIBDRM_FREEDRENO
> +#endif
> +#ifdef HAVE_LIBDRM_FREEDRENO
>   fd_device_del(buf->fd_dev);
>  #endif
> +#ifdef HAVE_LIBDRM_ETNAVIV
> + etna_device_del(buf->etna_dev);
> +#endif

Are all these del functions safe to call with NULL, or why wouldn't
they crash is more than one was supported?

I think it might be better to have these called via a vfunc.

>  
>   close(buf->drm_fd);
>  }
> @@ -308,7 +372,8 @@ drm_device_init(struct buffer *buf)
>   dev->map_bo = intel_map_bo;
>   dev->unmap_bo = intel_unmap_bo;
>   }
> -#elif HAVE_LIBDRM_FREEDRENO
> +#endif
> +#ifdef HAVE_LIBDRM_FREEDRENO
>   else if (!strcmp(dev->name, "msm")) {
>   dev->alloc_bo = fd_alloc_bo;
>

[weston PATCH] simple-dmabuf-drm: support etnaviv drm as well

2018-03-12 Thread Guido Günther 
Since freedreno and etnaviv can live in parallel allow to build in
different DRM backends.
---
 Makefile.am |   6 ++-
 clients/simple-dmabuf-drm.c | 122 +++-
 configure.ac|  29 +++
 3 files changed, 121 insertions(+), 36 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index e028a2a1..19319fa2 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -624,7 +624,11 @@ nodist_weston_simple_dmabuf_drm_SOURCES =  \
protocol/linux-dmabuf-unstable-v1-protocol.c \
protocol/linux-dmabuf-unstable-v1-client-protocol.h
 weston_simple_dmabuf_drm_CFLAGS = $(AM_CFLAGS) 
$(SIMPLE_DMABUF_DRM_CLIENT_CFLAGS)
-weston_simple_dmabuf_drm_LDADD = $(SIMPLE_DMABUF_DRM_CLIENT_LIBS) 
$(LIBDRM_PLATFORM_LIBS) libshared.la
+weston_simple_dmabuf_drm_LDADD = $(SIMPLE_DMABUF_DRM_CLIENT_LIBS) \
+   $(LIBDRM_PLATFORM_FREEDRENO_LIBS) \
+   $(LIBDRM_PLATFORM_ETNAVIV_LIBS)   \
+   $(LIBDRM_PLATFORM_INTEL_LIBS) \
+   libshared.la
 endif
 
 if BUILD_SIMPLE_DMABUF_V4L_CLIENT
diff --git a/clients/simple-dmabuf-drm.c b/clients/simple-dmabuf-drm.c
index 1073930f..8b7acd39 100644
--- a/clients/simple-dmabuf-drm.c
+++ b/clients/simple-dmabuf-drm.c
@@ -44,9 +44,13 @@
 #ifdef HAVE_LIBDRM_INTEL
 #include 
 #include 
-#elif HAVE_LIBDRM_FREEDRENO
+#endif
+#ifdef HAVE_LIBDRM_FREEDRENO
 #include 
 #endif
+#ifdef HAVE_LIBDRM_ETNAVIV
+#include 
+#endif
 #include 
 
 #include 
@@ -93,11 +97,16 @@ struct buffer {
 
 #ifdef HAVE_LIBDRM_INTEL
drm_intel_bufmgr *bufmgr;
-   drm_intel_bo *bo;
-#elif HAVE_LIBDRM_FREEDRENO
+   drm_intel_bo *intel_bo;
+#endif /* HAVE_LIBDRM_INTEL */
+#if HAVE_LIBDRM_FREEDRENO
struct fd_device *fd_dev;
-   struct fd_bo *bo;
+   struct fd_bo *fd_bo;
 #endif /* HAVE_LIBDRM_FREEDRENO */
+#if HAVE_LIBDRM_ETNAVIV
+   struct etna_device *etna_dev;
+   struct etna_bo *etna_bo;
+#endif /* HAVE_LIBDRM_ETNAVIV */
 
uint32_t gem_handle;
int dmabuf_fd;
@@ -152,15 +161,15 @@ intel_alloc_bo(struct buffer *my_buf)
 
assert(my_buf->bufmgr);
 
-   my_buf->bo = drm_intel_bo_alloc_tiled(my_buf->bufmgr, "test",
- my_buf->width, my_buf->height,
- (my_buf->bpp / 8), ,
- _buf->stride, 0);
+   my_buf->intel_bo = drm_intel_bo_alloc_tiled(my_buf->bufmgr, "test",
+   my_buf->width, 
my_buf->height,
+   (my_buf->bpp / 8), ,
+   _buf->stride, 0);
 
printf("buffer allocated w %d, h %d, stride %lu, size %lu\n",
-  my_buf->width, my_buf->height, my_buf->stride, my_buf->bo->size);
+  my_buf->width, my_buf->height, my_buf->stride, 
my_buf->intel_bo->size);
 
-   if (!my_buf->bo)
+   if (!my_buf->intel_bo)
return 0;
 
if (tiling != I915_TILING_NONE)
@@ -172,16 +181,16 @@ intel_alloc_bo(struct buffer *my_buf)
 static void
 intel_free_bo(struct buffer *my_buf)
 {
-   drm_intel_bo_unreference(my_buf->bo);
+   drm_intel_bo_unreference(my_buf->intel_bo);
 }
 
 static int
 intel_map_bo(struct buffer *my_buf)
 {
-   if (drm_intel_gem_bo_map_gtt(my_buf->bo) != 0)
+   if (drm_intel_gem_bo_map_gtt(my_buf->intel_bo) != 0)
return 0;
 
-   my_buf->mmap = my_buf->bo->virtual;
+   my_buf->mmap = my_buf->intel_bo->virtual;
 
return 1;
 }
@@ -189,15 +198,16 @@ intel_map_bo(struct buffer *my_buf)
 static int
 intel_bo_export_to_prime(struct buffer *buffer)
 {
-   return drm_intel_bo_gem_export_to_prime(buffer->bo, >dmabuf_fd);
+   return drm_intel_bo_gem_export_to_prime(buffer->intel_bo, 
>dmabuf_fd);
 }
 
 static void
 intel_unmap_bo(struct buffer *my_buf)
 {
-   drm_intel_gem_bo_unmap_gtt(my_buf->bo);
+   drm_intel_gem_bo_unmap_gtt(my_buf->intel_bo);
 }
-#elif HAVE_LIBDRM_FREEDRENO
+#endif /* HAVE_LIBDRM_INTEL */
+#ifdef HAVE_LIBDRM_FREEDRENO
 #define ALIGN(v, a) ((v + a - 1) & ~(a - 1))
 
 static
@@ -207,9 +217,9 @@ int fd_alloc_bo(struct buffer *buf)
int size = buf->width * buf->height * buf->bpp / 8;
buf->fd_dev = fd_device_new(buf->drm_fd);
 
-   buf->bo = fd_bo_new(buf->fd_dev, size, flags);
+   buf->fd_bo = fd_bo_new(buf->fd_dev, size, flags);
 
-   if (!buf->bo)
+   if (!buf->fd_bo)
return 0;
buf->stride = ALIGN(buf->width, 32) * buf->bpp / 8;
return 1;
@@ -218,13 +228,13 @@ int fd_alloc_bo(struct buffer *buf)
 static
 void fd_free_bo(struct buffer *buf)
 {
-   fd_bo_del(buf->bo);
+   fd_bo_del(buf->fd_bo);
 }
 
 static
 int fd_bo_export_to_prime(struct buffer *buf)
 {
-   buf->dmabuf_fd = fd_bo_dmabuf(buf->bo);
+   buf->dmabuf_fd = fd_bo_dmabuf(buf->fd_bo);
if (buf->dmabuf_fd > 0)
return 0;
 
@@