Re: [PATCH weston] simple-dmabuf-drm: use GBM generic calls
Hi everyone, On 11 July 2018 at 09:33, Daniel Stone wrote: > Hi, > On Wed, 11 Jul 2018 at 09:16, Pekka Paalanen wrote: >> On Tue, 10 Jul 2018 17:53:15 +0200 Emilio Pozuelo Monfort >> wrote: >> > No need to write libdrm driver specific code for each supported >> > driver, we can just let GBM call the right one for us now. >> > >> > Signed-off-by: Emilio Pozuelo Monfort >> > --- >> > >> > Hi, >> > >> > This simplifies the code a lot, using gbm_bo as Emil suggested. Some >> > problems >> > I still see: >> > >> > - NV12 doesn't work, it seems that >> > backends/dri/gbm_dri.c:gbm_format_to_dri_format() >> > doesn't support it. >> >> I think NV12 and other less common formats, should someone add them in >> this program, should not be lost. That may be part of the reason GBM >> wasn't used: the need to test YUV formats, and non-GPU-renderable >> formats in general. > > Honestly, most of the reason was because I wrote the original client > in about 15 minutes on a very short bus trip. You can probably see > that in comments like 'XXX: would be nice to draw something that > changes here'. So I don't think you can really read too much into the > original code. > > Other great reasons include: > - we wanted to explicitly get modifiers allocated, and this was > before the GBM modifiers interface existed > - at the time, I was doing bring-up of a GBM implementation which > wasn't usable by generic clients (privileged allocation only) > - YUV format support > > I'm pretty ambivalent about it though. V4L2 and Vivid might well cover > the YUV case well enough, and even if not, GBM should still be able to > allocate R/RG buffers. > Darn, I did not spot NV12 in weston. Mesa is perfectly capable of using YUV formats, so it should be a matter of adding the odd GBM bits. That said, it might be more involving than what you have time for, so feel free to put this patch on hold. Thanks Emil ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston] simple-dmabuf-drm: use GBM generic calls
Hi, On Tue, Jul 10, 2018 at 05:53:15PM +0200, Emilio Pozuelo Monfort wrote: > No need to write libdrm driver specific code for each supported > driver, we can just let GBM call the right one for us now. > > Signed-off-by: Emilio Pozuelo Monfort > --- > > Hi, > > This simplifies the code a lot, using gbm_bo as Emil suggested. Some problems > I still see: > > - NV12 doesn't work, it seems that > backends/dri/gbm_dri.c:gbm_format_to_dri_format() > doesn't support it. Simple means of testing NV12 is a useful feature so it'd be sad to see that go. Using weston-simple-dmabuf-v4l is much more involved as it requires more setup. Cheers, -- Guido ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston] simple-dmabuf-drm: use GBM generic calls
Hi, On Wed, 11 Jul 2018 at 09:16, Pekka Paalanen wrote: > On Tue, 10 Jul 2018 17:53:15 +0200 Emilio Pozuelo Monfort > wrote: > > No need to write libdrm driver specific code for each supported > > driver, we can just let GBM call the right one for us now. > > > > Signed-off-by: Emilio Pozuelo Monfort > > --- > > > > Hi, > > > > This simplifies the code a lot, using gbm_bo as Emil suggested. Some > > problems > > I still see: > > > > - NV12 doesn't work, it seems that > > backends/dri/gbm_dri.c:gbm_format_to_dri_format() > > doesn't support it. > > I think NV12 and other less common formats, should someone add them in > this program, should not be lost. That may be part of the reason GBM > wasn't used: the need to test YUV formats, and non-GPU-renderable > formats in general. Honestly, most of the reason was because I wrote the original client in about 15 minutes on a very short bus trip. You can probably see that in comments like 'XXX: would be nice to draw something that changes here'. So I don't think you can really read too much into the original code. Other great reasons include: - we wanted to explicitly get modifiers allocated, and this was before the GBM modifiers interface existed - at the time, I was doing bring-up of a GBM implementation which wasn't usable by generic clients (privileged allocation only) - YUV format support I'm pretty ambivalent about it though. V4L2 and Vivid might well cover the YUV case well enough, and even if not, GBM should still be able to allocate R/RG buffers. Cheers, Daniel ___ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston] simple-dmabuf-drm: use GBM generic calls
Adding more people to CC.
On Tue, 10 Jul 2018 17:53:15 +0200
Emilio Pozuelo Monfort wrote:
> No need to write libdrm driver specific code for each supported
> driver, we can just let GBM call the right one for us now.
>
> Signed-off-by: Emilio Pozuelo Monfort
> ---
>
> Hi,
>
> This simplifies the code a lot, using gbm_bo as Emil suggested. Some problems
> I still see:
>
> - NV12 doesn't work, it seems that
> backends/dri/gbm_dri.c:gbm_format_to_dri_format()
> doesn't support it.
I think NV12 and other less common formats, should someone add them in
this program, should not be lost. That may be part of the reason GBM
wasn't used: the need to test YUV formats, and non-GPU-renderable
formats in general.
>
> - We are still linking to libdrm, that's just to get the DRM_FORMAT_*
> definitions.
> I suppose we could switch those to GBM_FORMAT_* instead.
We have precedent in the build system of using libdrm only for headers
but not link to it, specifically to get the pixel format codes. Just
leave out the $(LIBDRM_LIBS) or such.
>
> - Not sure if I need to pass any flags to gbm_bo_create or gbm_bo_map.
>
> This works fine for me with XRGB.
>
> Thoughts? Comments?
>
> Thanks,
> Emilio
Thanks,
pq
>
> clients/simple-dmabuf-drm.c | 320 +++-
> configure.ac| 2 +-
> 2 files changed, 27 insertions(+), 295 deletions(-)
>
> diff --git a/clients/simple-dmabuf-drm.c b/clients/simple-dmabuf-drm.c
> index fcab30e5..cdee27b3 100644
> --- a/clients/simple-dmabuf-drm.c
> +++ b/clients/simple-dmabuf-drm.c
> @@ -41,18 +41,7 @@
> #include
> #include
>
> -#include
> -
> -#ifdef HAVE_LIBDRM_INTEL
> -#include
> -#include
> -#endif
> -#ifdef HAVE_LIBDRM_FREEDRENO
> -#include
> -#endif
> -#ifdef HAVE_LIBDRM_ETNAVIV
> -#include
> -#endif
> +#include
> #include
>
> #include
> @@ -66,14 +55,10 @@
> #define DRM_FORMAT_MOD_LINEAR 0
> #endif
>
> -struct buffer;
> -
> /* Possible options that affect the displayed image */
> #define OPT_Y_INVERTED 1 /* contents has y axis inverted */
> #define OPT_IMMEDIATE 2 /* create wl_buffer immediately */
>
> -#define ALIGN(v, a) ((v + a - 1) & ~(a - 1))
> -
> struct display {
> struct wl_display *display;
> struct wl_registry *registry;
> @@ -88,46 +73,22 @@ struct display {
> int req_dmabuf_modifiers;
> };
>
> -struct drm_device {
> - int fd;
> - char *name;
> -
> - int (*alloc_bo)(struct buffer *buf);
> - void (*free_bo)(struct buffer *buf);
> - int (*export_bo_to_prime)(struct buffer *buf);
> - int (*map_bo)(struct buffer *buf);
> - void (*unmap_bo)(struct buffer *buf);
> - void (*device_destroy)(struct buffer *buf);
> -};
> -
> struct buffer {
> struct wl_buffer *buffer;
> int busy;
>
> - struct drm_device *dev;
> int drm_fd;
> -
> -#ifdef HAVE_LIBDRM_INTEL
> - drm_intel_bufmgr *bufmgr;
> - drm_intel_bo *intel_bo;
> -#endif /* HAVE_LIBDRM_INTEL */
> -#if HAVE_LIBDRM_FREEDRENO
> - struct fd_device *fd_dev;
> - 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 */
> + struct gbm_device *dev;
> + struct gbm_bo *bo;
>
> uint32_t gem_handle;
> int dmabuf_fd;
> - uint8_t *mmap;
> + void *mmap;
>
> int width;
> int height;
> int bpp;
> - unsigned long stride;
> + uint32_t stride;
> int format;
> };
>
> @@ -163,170 +124,6 @@ static const struct wl_buffer_listener buffer_listener
> = {
> buffer_release
> };
>
> -
> -#ifdef HAVE_LIBDRM_INTEL
> -static int
> -intel_alloc_bo(struct buffer *my_buf)
> -{
> - /* XXX: try different tiling modes for testing FB modifiers. */
> - uint32_t tiling = I915_TILING_NONE;
> -
> - assert(my_buf->bufmgr);
> -
> - 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->intel_bo->size);
> -
> - if (!my_buf->intel_bo)
> - return 0;
> -
> - if (tiling != I915_TILING_NONE)
> - return 0;
> -
> - return 1;
> -}
> -
> -static void
> -intel_free_bo(struct buffer *my_buf)
> -{
> - 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->intel_bo) != 0)
> - return 0;
> -
> - my_buf->mmap = my_buf->intel_bo->virtual;
> -
> - return 1;
> -}
> -
> -static int
> -intel_bo_export_to_prime(struct buffer *buffer)
> -{
> -
[PATCH weston] simple-dmabuf-drm: use GBM generic calls
No need to write libdrm driver specific code for each supported
driver, we can just let GBM call the right one for us now.
Signed-off-by: Emilio Pozuelo Monfort
---
Hi,
This simplifies the code a lot, using gbm_bo as Emil suggested. Some problems
I still see:
- NV12 doesn't work, it seems that
backends/dri/gbm_dri.c:gbm_format_to_dri_format()
doesn't support it.
- We are still linking to libdrm, that's just to get the DRM_FORMAT_*
definitions.
I suppose we could switch those to GBM_FORMAT_* instead.
- Not sure if I need to pass any flags to gbm_bo_create or gbm_bo_map.
This works fine for me with XRGB.
Thoughts? Comments?
Thanks,
Emilio
clients/simple-dmabuf-drm.c | 320 +++-
configure.ac| 2 +-
2 files changed, 27 insertions(+), 295 deletions(-)
diff --git a/clients/simple-dmabuf-drm.c b/clients/simple-dmabuf-drm.c
index fcab30e5..cdee27b3 100644
--- a/clients/simple-dmabuf-drm.c
+++ b/clients/simple-dmabuf-drm.c
@@ -41,18 +41,7 @@
#include
#include
-#include
-
-#ifdef HAVE_LIBDRM_INTEL
-#include
-#include
-#endif
-#ifdef HAVE_LIBDRM_FREEDRENO
-#include
-#endif
-#ifdef HAVE_LIBDRM_ETNAVIV
-#include
-#endif
+#include
#include
#include
@@ -66,14 +55,10 @@
#define DRM_FORMAT_MOD_LINEAR 0
#endif
-struct buffer;
-
/* Possible options that affect the displayed image */
#define OPT_Y_INVERTED 1 /* contents has y axis inverted */
#define OPT_IMMEDIATE 2 /* create wl_buffer immediately */
-#define ALIGN(v, a) ((v + a - 1) & ~(a - 1))
-
struct display {
struct wl_display *display;
struct wl_registry *registry;
@@ -88,46 +73,22 @@ struct display {
int req_dmabuf_modifiers;
};
-struct drm_device {
- int fd;
- char *name;
-
- int (*alloc_bo)(struct buffer *buf);
- void (*free_bo)(struct buffer *buf);
- int (*export_bo_to_prime)(struct buffer *buf);
- int (*map_bo)(struct buffer *buf);
- void (*unmap_bo)(struct buffer *buf);
- void (*device_destroy)(struct buffer *buf);
-};
-
struct buffer {
struct wl_buffer *buffer;
int busy;
- struct drm_device *dev;
int drm_fd;
-
-#ifdef HAVE_LIBDRM_INTEL
- drm_intel_bufmgr *bufmgr;
- drm_intel_bo *intel_bo;
-#endif /* HAVE_LIBDRM_INTEL */
-#if HAVE_LIBDRM_FREEDRENO
- struct fd_device *fd_dev;
- 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 */
+ struct gbm_device *dev;
+ struct gbm_bo *bo;
uint32_t gem_handle;
int dmabuf_fd;
- uint8_t *mmap;
+ void *mmap;
int width;
int height;
int bpp;
- unsigned long stride;
+ uint32_t stride;
int format;
};
@@ -163,170 +124,6 @@ static const struct wl_buffer_listener buffer_listener = {
buffer_release
};
-
-#ifdef HAVE_LIBDRM_INTEL
-static int
-intel_alloc_bo(struct buffer *my_buf)
-{
- /* XXX: try different tiling modes for testing FB modifiers. */
- uint32_t tiling = I915_TILING_NONE;
-
- assert(my_buf->bufmgr);
-
- 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->intel_bo->size);
-
- if (!my_buf->intel_bo)
- return 0;
-
- if (tiling != I915_TILING_NONE)
- return 0;
-
- return 1;
-}
-
-static void
-intel_free_bo(struct buffer *my_buf)
-{
- 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->intel_bo) != 0)
- return 0;
-
- my_buf->mmap = my_buf->intel_bo->virtual;
-
- return 1;
-}
-
-static int
-intel_bo_export_to_prime(struct buffer *buffer)
-{
- 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->intel_bo);
-}
-
-static void
-intel_device_destroy(struct buffer *my_buf)
-{
- drm_intel_bufmgr_destroy(my_buf->bufmgr);
-}
-
-#endif /* HAVE_LIBDRM_INTEL */
-#ifdef HAVE_LIBDRM_FREEDRENO
-
-static int
-fd_alloc_bo(struct buffer *buf)
-{
- int flags = DRM_FREEDRENO_GEM_CACHE_WCOMBINE;
- int size;
-
- buf->fd_dev = fd_device_new(buf->drm_fd);
- buf->stride = ALIGN(buf->width, 32) * buf->bpp / 8;
- size = buf->stride * buf->height;
- buf->fd_dev = fd_device_new(buf->drm_fd);
- buf->fd_bo = fd_bo_new(buf->fd_dev, size, flags);
-
- if
