On Mon, 12 Mar 2018 16:41:46 +0100 Guido Günther <a...@sigxcpu.org> 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 <i915_drm.h>
> #include <intel_bufmgr.h>
> -#elif HAVE_LIBDRM_FREEDRENO
> +#endif
> +#ifdef HAVE_LIBDRM_FREEDRENO
> #include <freedreno/freedreno_drmif.h>
> #endif
> +#ifdef HAVE_LIBDRM_ETNAVIV
> +#include <etnaviv_drmif.h>
> +#endif
> #include <drm_fourcc.h>
It's nice to see that the three can all be built at the same time.
>
> #include <wayland-client.h>
> @@ -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;
> dev->free_bo = fd_free_bo;
> @@ -316,6 +381,15 @@ drm_device_init(struct buffer *buf)
> dev->map_bo = fd_map_bo;
> dev->unmap_bo = fd_unmap_bo;
> }
> +#endif
> +#ifdef HAVE_LIBDRM_ETNAVIV
> + else if (!strcmp(dev->name, "etnaviv")) {
> + dev->alloc_bo = etna_alloc_bo;
> + dev->free_bo = etna_free_bo;
> + dev->export_bo_to_prime = etna_bo_export_to_prime;
> + dev->map_bo = etna_map_bo;
> + dev->unmap_bo = etna_unmap_bo;
> + }
> #endif
> else {
> fprintf(stderr, "Error: drm device %s unsupported.\n",
> diff --git a/configure.ac b/configure.ac
> index 0b326ccc..6880a4aa 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -384,19 +384,26 @@ AC_ARG_ENABLE(simple-dmabuf-drm-client,
> [do not build the simple dmabuf drm client]),,
> enable_simple_dmabuf_drm_client="auto")
> if ! test "x$enable_simple_dmabuf_drm_client" = "xno"; then
> - PKG_CHECK_MODULES(SIMPLE_DMABUF_DRM_CLIENT, [wayland-client libdrm egl],
> - [PKG_CHECK_MODULES(LIBDRM_PLATFORM, [libdrm_freedreno],
> - AC_DEFINE([HAVE_LIBDRM_FREEDRENO], [1], [Build freedreno dmabuf
> client]) have_simple_dmabuf_drm_client=freedreno,
> - [PKG_CHECK_MODULES(LIBDRM_PLATFORM, [libdrm_intel],
> - AC_DEFINE([HAVE_LIBDRM_INTEL], [1], [Build intel dmabuf client])
> have_simple_dmabuf_drm_client=intel,
> - have_simple_dmabuf_drm_client=unsupported)])],
> - have_simple_dmabuf_drm_client=unsupported)
> -
> - if test "x$have_simple_dmabuf_drm_client" = "xunsupported" -a
> "x$enable_simple_dmabuf_drm_client" = "xyes"; then
> - AC_MSG_ERROR([DRM dmabuf client explicitly enabled, but libdrm_intel or
> libdrm_freedreno not found])
> + PKG_CHECK_MODULES(SIMPLE_DMABUF_DRM_CLIENT, [wayland-client libdrm egl],
> [have_simple_dmabuf_libs=yes],
> + [have_simple_dmabuf_libs=no])
> +
> + PKG_CHECK_MODULES(LIBDRM_PLATFORM_FREEDRENO, [libdrm_freedreno],
> + AC_DEFINE([HAVE_LIBDRM_FREEDRENO], [1], [Build freedreno dmabuf
> client]) have_simple_dmabuf_drm_client=yes,
> + [have_freedreno=no])
> + PKG_CHECK_MODULES(LIBDRM_PLATFORM_ETNAVIV, [libdrm_etnaviv],
> + AC_DEFINE([HAVE_LIBDRM_ETNAVIV], [1], [Build etnaviv dmabuf client])
> have_simple_dmabuf_drm_client=yes,
> + [have_etnaviv=no])
> + PKG_CHECK_MODULES(LIBDRM_PLATFORM_INTEL, [libdrm_intel],
> + AC_DEFINE([HAVE_LIBDRM_INTEL], [1], [Build intel dmabuf client])
> have_simple_dmabuf_drm_client=yes,
> + [have_intel=no])
The have_*=no look unused.
> +
> + if test "x$have_simple_dmabuf_drm_client" != "xyes" -o \
> + "xhave_simple_dmabuf_libs" = "no" -a \
This seems to be missing a $.
> + "x$enable_simple_dmabuf_drm_client" = "xyes"; then
What's associativity of -o and -a?
I read the 'test' man page, and I still don't know - it actually says
each of -o and -a are ambiguous.
> + AC_MSG_ERROR([DRM dmabuf client explicitly enabled, but libdrm_intel,
> libdrm_freedreno or libdrm_etnaviv not found])
> fi
>
> - if test "x$have_simple_dmabuf_drm_client" = "xfreedreno" -o
> "x$have_simple_dmabuf_drm_client" = "xintel"; then
> + if test "x$have_simple_dmabuf_drm_client" = "xyes" -a
> "xhave_simple_dmabuf_libs"; then
Missing a $.
> enable_simple_dmabuf_drm_client="yes"
> fi
> fi
Thanks,
pq
pgpJ6A15vb5ZM.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
