[Mesa-dev] [PATCH 1/3] gallium: add renderonly library

[Thierry Reding]

On Wed, Nov 30, 2016 at 02:44:34PM +0100, Christian Gmeiner wrote:
> This a very lightweight library to add basic support for
> renderonly GPUs. It does all the magic regarding in/exporting
> buffers etc. This library will likely break android support and
> hopefully will get replaced with a better solution based on gbm2.
> 
> Signed-off-by: Christian Gmeiner <christian.gmeiner at gmail.com>
> ---
>  src/gallium/Automake.inc                      |   5 +
>  src/gallium/auxiliary/Makefile.am             |  10 ++
>  src/gallium/auxiliary/Makefile.sources        |   4 +
>  src/gallium/auxiliary/renderonly/renderonly.c | 199 ++++++++++++++++++++++++++
>  src/gallium/auxiliary/renderonly/renderonly.h |  81 +++++++++++
>  5 files changed, 299 insertions(+)
>  create mode 100644 src/gallium/auxiliary/renderonly/renderonly.c
>  create mode 100644 src/gallium/auxiliary/renderonly/renderonly.h

Hi Christian,

sorry for being late to the party. I only ran across this by accident.
Would you mind keeping me in Cc for subsequent versions of this? It's
been more than two years since I wrote the original proposal for this,
but I'm still interested in seeing a solution emerge.

Anyway, thanks for picking this up.

From a diff point of view this is certainly much more terse than the
original proposal. I find it slightly unfortunate that the drivers for
the render GPU have to be changed. But it's hard to argue with the
reduction in size.

I still think that Tegra will eventually require more than the stub that
you have in this series because the same device that exposes the scanout
engine also contains units that can do video compositing, decoding and
encoding. There's in fact recently been discussions on how best to
provide that functionality and Mesa looks like the best long-term target
currently. Anyway, that's a bridge I think we can cross when we get
there, this concept looks fine to get started.

> diff --git a/src/gallium/Automake.inc b/src/gallium/Automake.inc
> index 6fe2e22..6aadcb9 100644
> --- a/src/gallium/Automake.inc
> +++ b/src/gallium/Automake.inc
> @@ -50,6 +50,11 @@ GALLIUM_COMMON_LIB_DEPS = \
>  	$(PTHREAD_LIBS) \
>  	$(DLOPEN_LIBS)
>  
> +if HAVE_LIBDRM
> +GALLIUM_COMMON_LIB_DEPS += \
> +	$(LIBDRM_LIBS)
> +endif

Nit: Is the conditional necessary here? LIBDRM_LIBS would be empty if
libdrm wasn't found, right? So no harm in just appending it to the
GALLIUM_COMMON_LIB_DEPS variable unconditonally?

>  GALLIUM_WINSYS_CFLAGS = \
>  	-I$(top_srcdir)/src \
>  	-I$(top_srcdir)/include \
> diff --git a/src/gallium/auxiliary/Makefile.am b/src/gallium/auxiliary/Makefile.am
> index 4a4a4fb..6b63cf1 100644
> --- a/src/gallium/auxiliary/Makefile.am
> +++ b/src/gallium/auxiliary/Makefile.am
> @@ -20,6 +20,16 @@ libgallium_la_SOURCES = \
>  	$(NIR_SOURCES) \
>  	$(GENERATED_SOURCES)
>  
> +if HAVE_LIBDRM
> +
> +AM_CFLAGS += \
> +	$(LIBDRM_CFLAGS)

Same here.

> diff --git a/src/gallium/auxiliary/Makefile.sources b/src/gallium/auxiliary/Makefile.sources
> index 5d4fe30..8d3e4a9 100644
> --- a/src/gallium/auxiliary/Makefile.sources
> +++ b/src/gallium/auxiliary/Makefile.sources
> @@ -435,3 +435,7 @@ GALLIVM_SOURCES := \
>  	draw/draw_llvm_sample.c \
>  	draw/draw_pt_fetch_shade_pipeline_llvm.c \
>  	draw/draw_vs_llvm.c
> +
> +RENDERONLY_SOURCES := \
> +	renderonly/renderonly.c \
> +	renderonly/renderonly.h
> diff --git a/src/gallium/auxiliary/renderonly/renderonly.c b/src/gallium/auxiliary/renderonly/renderonly.c
> new file mode 100644
> index 0000000..c4ea784
> --- /dev/null
> +++ b/src/gallium/auxiliary/renderonly/renderonly.c
> @@ -0,0 +1,199 @@
> +/*
> + * Copyright (C) 2016 Christian Gmeiner <christian.gmeiner at gmail.com>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + *
> + * Authors:
> + *    Christian Gmeiner <christian.gmeiner at gmail.com>
> + */
> +
> +#include "renderonly/renderonly.h"

This include is oddly placed. Should it not go below all other includes?

> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <sys/ioctl.h>
> +#include <xf86drm.h>
> +
> +#include "state_tracker/drm_driver.h"
> +#include "pipe/p_screen.h"
> +#include "util/u_memory.h"
> +
> +struct pipe_screen *
> +renderonly_screen_create(int fd, const struct renderonly_ops *ops, void *priv)

This is slightly nit-picky, but I found it confusing when first reading
through the patches: I know this started out as an effort to support the
various render-only devices out there, but what you're creating here is
really an abstraction for the scanout engine. Reading renderonly_* the
first thing I think of is that this creates a render-only device, where
in fact is creates the scanout engine.

Perhaps renaming this to struct scanout, struct scanout_engine or any
other number of possibilities would remove that confusion.

> +{
> +   struct renderonly *ro;
> +
> +   ro = CALLOC_STRUCT(renderonly);
> +   if (!ro)
> +      return NULL;
> +
> +   ro->kms_fd = fd;

The kms_ prefix seems rather redundant to me, given that struct
renderonly represents the KMS device.

> +   ro->ops = ops;
> +   ro->priv = priv;
> +
> +   ro->screen = ops->create(ro);

It's somewhat odd that drivers need to jump through this hoop in order
to create their screens. I think it would be easier if they just
embedded the struct renderonly and initialized it in their create
function.

I think that's probably what Nicolai and Emil were saying as well.

> +   if (!ro->screen)
> +      goto cleanup;
> +
> +   return ro->screen;
> +
> +cleanup:
> +   FREE(ro);
> +
> +   return NULL;
> +}
> +
> +static bool
> +use_kms_bumb_buffer(struct renderonly_scanout *scanout,

You probably meant "dumb" here. Somehow you managed to get it right in
the variables, but the function name and the callsite consistently have
the same typo. =)

> +      struct pipe_resource *rsc, struct renderonly *ro)
> +{
> +   struct winsys_handle handle;
> +   int prime_fd, err;
> +   struct drm_mode_create_dumb create_dumb = {
> +      .width = rsc->width0,
> +      .height = rsc->height0,
> +      .bpp = 32,

This seems like maybe too strong an assumption. Or at the very least a
waste of memory. What if somebody wanted to use 16- or 24-bit buffers?

> +static bool
> +import_gpu_scanout(struct renderonly_scanout *scanout,
> +      struct pipe_resource *rsc, struct renderonly *ro)
> +{
> +   struct pipe_screen *screen = ro->screen;
> +   boolean status;
> +   int fd, err;
> +   struct winsys_handle handle = {
> +      .type = DRM_API_HANDLE_TYPE_FD
> +   };
> +
> +   status = screen->resource_get_handle(screen, NULL, rsc, &handle,
> +         PIPE_HANDLE_USAGE_READ_WRITE);
> +   if (!status)
> +      return false;
> +
> +   scanout->stride = handle.stride;
> +   fd = handle.handle;
> +
> +   err = drmPrimeFDToHandle(ro->kms_fd, fd, &scanout->handle);
> +   close(fd);
> +
> +   if (err < 0) {
> +      fprintf(stderr, "drmPrimeFDToHandle() failed: %s\n", strerror(errno));
> +      return false;
> +   }
> +
> +   if (ro->ops->tiling) {

tiling is maybe not the best name for this, since arguable the scanout
driver might want to do any number of other things while importing.
Perhaps ->import() would be generic enough?

> +      err = ro->ops->tiling(ro->kms_fd, scanout->handle);
> +      if (err < 0) {
> +         fprintf(stderr, "failed to set tiling parameters: %s\n", strerror(errno));
> +         close(scanout->handle);

I don't think scanout->handle is something you should be passing to
close().

> +         return false;
> +      }
> +   }
> +
> +   return true;
> +}
> +
> +struct renderonly_scanout *
> +renderonly_scanout_for_resource(struct pipe_resource *rsc, struct renderonly *ro)
> +{
> +   struct renderonly_scanout *scanout;
> +   bool ret;
> +
> +   scanout = CALLOC_STRUCT(renderonly_scanout);
> +   if (!scanout)
> +      return NULL;
> +
> +   if (ro->ops->intermediate_rendering)
> +       ret = use_kms_bumb_buffer(scanout, rsc, ro);
> +   else
> +       ret = import_gpu_scanout(scanout, rsc, ro);

Given my kernel background I've grown allergic to this kind of
construct. What you're creating here is a midlayer that's very likely to
cause headaches down the road. I think a better alternative would be to
add a callback to the struct renderonly_ops that drivers have to set to
whatever the implementation is that they want.

If you want to provide standard helpers, that's fine. Turn both the
use_kms_bumb_buffer() and import_gpu_scanout() functions into public
functions that drivers can reference. Then drivers can simply assign
them to the new operation, or implement their own if they find that the
defaults aren't flexible enough.

On of the problem, the most immediate in this case, with this kind of
midlayer is that if intermediate_rendering == {false,true} is not enough
for a given use-case, we end up having to add yet another flag that
specifies behaviour.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20161219/21708580/attachment-0001.sig>