[PATCH weston 1/2] compositor-drm: Add scanout support for linux_dmabuf buffers
Pekka Paalanen
ppaalanen at gmail.com
Wed Jun 15 08:20:56 UTC 2016
On Fri, 27 May 2016 18:52:27 +0900
Tomohito Esaki <etom at igel.co.jp> wrote:
> This implementations bypasses gbm and passes the dmabuf handles directly
> to libdrm for composition.
>
> Signed-off-by: Tomohito Esaki <etom at igel.co.jp>
Hi,
thank you for working on this, it has been on the todo for quite a
while. The patch seems good in its essence, but needs some polishing,
below.
> ---
> src/compositor-drm.c | 128 +++++++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 108 insertions(+), 20 deletions(-)
>
> diff --git a/src/compositor-drm.c b/src/compositor-drm.c
> index 893877d..cd2fb71 100644
> --- a/src/compositor-drm.c
> +++ b/src/compositor-drm.c
> @@ -150,6 +150,9 @@ struct drm_fb {
>
> /* Used by dumb fbs */
> void *map;
> +
> + /* Used by dmabuf */
> + struct linux_dmabuf_buffer *dmabuf;
> };
>
> struct drm_edid {
> @@ -340,6 +343,84 @@ drm_fb_destroy_dumb(struct drm_fb *fb)
> free(fb);
> }
>
> +static inline void close_drm_handle(int fd, uint32_t handle)
Needs line break after 'void'.
> +{
> + struct drm_gem_close gem_close = {.handle = handle};
Add spaces inside the braces.
> + 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;
> + 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) {
> + weston_log("dmabuf geometry out of bounds\n");
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?
> + return NULL;
> + }
> +
> + for (i = 0; i < dmabuf->attributes.n_planes; i++) {
> + ret = drmPrimeFDToHandle(backend->drm.fd,
> + dmabuf->attributes.fd[i],
> + &handles[i]);
> + if (ret) {
> + weston_log("drmPrimeFDToHandle() failed. %s\n",
> + strerror(errno));
> + goto failed;
> + }
> + }
> +
> + fb = zalloc(sizeof *fb);
> + if (!fb)
> + goto failed;
> +
> + ret = drmModeAddFB2(backend->drm.fd, width, height,
> + format, handles, dmabuf->attributes.stride,
> + dmabuf->attributes.offset, &fb_id, 0);
> + if (ret) {
> + weston_log("drmModeAddFB2 failed. %s\n", strerror(errno));
> + free(fb);
> + goto failed;
> + }
The handles are not needed after this point, AFAIU.
Are you not leaking the handles? Should they be not closed regardless of
success or failure?
> +
> + fb->fb_id = fb_id;
> + fb->stride = dmabuf->attributes.stride[0];
> + fb->fd = backend->drm.fd;
> + fb->dmabuf = dmabuf;
> +
> + return fb;
> +
> +failed:
> + for (i = 0; i < dmabuf->attributes.n_planes; i++) {
> + if (!handles[i])
> + continue;
> + close_drm_handle(backend->drm.fd, handles[i]);
> + }
> + return NULL;
> +}
> +
> +static void drm_fb_destroy_dmabuf(struct drm_fb *fb)
Needs line break after 'void'.
> +{
> + if (fb->fb_id)
> + drmModeRmFB(fb->fd, fb->fb_id);
> + weston_buffer_reference(&fb->buffer_ref, NULL);
> + free(fb);
> +}
The above function has nothing specific to dmabufs, it just copies what
drm_fb_destroy_callback() does. I think the latter should be refactored
to call the former. It would also be good to make that refactoring a
separate patch before this one, to introduce a common function for
cleaning up the common parts of a struct drm_fb. Also
drm_fb_destroy_dumb() should be refactored to call it. That would pave
way for properly sub-classing struct drm_fb for the different kinds.
But, it's not absolutely necessary for landing this patch, it can be
done as a follow-up, too. Actually I see a lot more one could clean up
here, if anyone had the time... so, nevermind.
> +
> static struct drm_fb *
> drm_fb_get_from_bo(struct gbm_bo *bo,
> struct drm_backend *backend, uint32_t format)
> @@ -426,6 +507,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->dmabuf) {
> + drm_fb_destroy_dmabuf(fb);
This is the only place that actually uses fb->dmabuf, right?
I would be inclined to change the field from 'struct
linux_dmabuf_buffer *dmabuf;' into 'bool is_dmabuf;', because
linux_dmabuf_buffer can disappear by client actions, which means that
you might be left with a stale pointer unless you explicitly subscribe
to its destruction. Someone else might later start using that pointer
and introduce a hard to see crash.
Currently we don't need any more destruction tracking than there is,
because once we have created a GBM bo or a DRM FB, those will keep
internal references and the linux_dmabuf_buffer is not strictly
required to keep using those. (Of course, once the client destroys the
wl_buffer, we need to stop using it, but that's handled elsewhere. I
think...)
> } else if (fb->bo) {
> if (fb->is_client_buffer)
> gbm_bo_destroy(fb->bo);
> @@ -437,13 +520,10 @@ 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);
> -
Just noting here, that we rely on the GBM format enum and DRM format
enum to be identical, which they are. This might be worth adding a
comment, so it doesn't confuse the reader.
> if (format == GBM_FORMAT_ARGB8888) {
> /* We can scanout an ARGB buffer if the surface's
> * opaque region covers the whole output, but we have
> @@ -473,12 +553,13 @@ drm_output_prepare_scanout_view(struct drm_output *output,
> (struct drm_backend *)output->base.compositor->backend;
> struct weston_buffer *buffer = ev->surface->buffer_ref.buffer;
> struct weston_buffer_viewport *viewport = &ev->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 ||
> @@ -488,22 +569,29 @@ 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,
> - buffer->resource, GBM_BO_USE_SCANOUT);
> -
> - /* Unable to use the buffer for scanout */
> - if (!bo)
> - return NULL;
> + if ((dmabuf = linux_dmabuf_buffer_get(buffer->resource))) {
> + if (((format = drm_output_check_scanout_format(
> + output, ev->surface,
> + dmabuf->attributes.format)) == 0) ||
> + (!(output->next = drm_fb_create_dmabuf(
> + dmabuf, b, format))))
> + return NULL;
I think this part would be much easier to read if the assignments were
statements of their own. Like:
dmabuf = ...;
if (dmabuf) {
format = ...;
if (format == 0)
return NULL;
output->next = ...;
if (!output->next)
return NULL;
} ...
> + } else if (b->gbm) {
> + bo = gbm_bo_import(b->gbm, GBM_BO_IMPORT_WL_BUFFER,
> + buffer->resource, GBM_BO_USE_SCANOUT);
>
> - format = drm_output_check_scanout_format(output, ev->surface, bo);
> - if (format == 0) {
> - gbm_bo_destroy(bo);
> - return NULL;
> - }
> + /* Unable to use the buffer for scanout */
> + if (!bo)
> + return NULL;
>
> - output->next = drm_fb_get_from_bo(bo, b, format);
> - if (!output->next) {
> - gbm_bo_destroy(bo);
> + if (((format = drm_output_check_scanout_format(
> + output, ev->surface,
> + gbm_bo_get_format(bo))) == 0) ||
> + (!(output->next = drm_fb_get_from_bo(bo, b, format)))) {
> + gbm_bo_destroy(bo);
> + return NULL;
> + }
The same here about readability.
> + } else {
> return NULL;
> }
>
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160615/63664a45/attachment.sig>
More information about the wayland-devel
mailing list