Hi Christian,
I'm glad to see Thierry's work revived. Hopefully this will soon be
the basis of many more drivers.
On 11 October 2015 at 16:09, Christian Gmeiner
<christian.gmei...@gmail.com> wrote:
> This commit adds a generic renderonly driver library, which fullfille
> the requirements for tegra and etnaviv. As a result it is possible to
> run unmodified egl software directly (without any compositor) on
> supported devices.
>
> In every use case we import a dumb buffer from scanout gpu into
> the renderonly gpu.
>
> If the scanout hardware does support the used tiling format from the
> renderonly gpu, a driver can define a function which is used to 'setup'
> the needed tiling on that imported buffer. This functions gets called
> during rendertarget resource creation.
>
> If the scanout hardware does not support the used tiling format we need
> to create an extra rendertarget resource for the renderonly gpu.
> During XXX we blit the renderonly rendertarget onto the imported dumb
> buffer.
>
I'd assume you meant to add something over the XXX here :-P
But seriously some people might not be too happy with the blit onto
dumb buffer. Personally I ok, esp. since we don't have anything better
atm.
That aside, there are a few minor nitpicks below. With those sorted I
believe the patch is good to land.
> We assume that the renderonly driver provides a blit function that is
> capable of resolving the tilied into untiled one.
>
> Signed-off-by: Christian Gmeiner <christian.gmei...@gmail.com>
> ---
> configure.ac | 1 +
> src/gallium/drivers/renderonly/Makefile.am | 11 +
> src/gallium/drivers/renderonly/Makefile.sources | 4 +
> .../drivers/renderonly/renderonly_context.c | 721
> +++++++++++++++++++++
> .../drivers/renderonly/renderonly_context.h | 80 +++
> .../drivers/renderonly/renderonly_resource.c | 296 +++++++++
> .../drivers/renderonly/renderonly_resource.h | 101 +++
> src/gallium/drivers/renderonly/renderonly_screen.c | 178 +++++
> src/gallium/drivers/renderonly/renderonly_screen.h | 55 ++
> 9 files changed, 1447 insertions(+)
> create mode 100644 src/gallium/drivers/renderonly/Makefile.am
> create mode 100644 src/gallium/drivers/renderonly/Makefile.sources
> create mode 100644 src/gallium/drivers/renderonly/renderonly_context.c
> create mode 100644 src/gallium/drivers/renderonly/renderonly_context.h
> create mode 100644 src/gallium/drivers/renderonly/renderonly_resource.c
> create mode 100644 src/gallium/drivers/renderonly/renderonly_resource.h
> create mode 100644 src/gallium/drivers/renderonly/renderonly_screen.c
> create mode 100644 src/gallium/drivers/renderonly/renderonly_screen.h
>
> diff --git a/configure.ac b/configure.ac
> index 217281f..ea485b1 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -2361,6 +2361,7 @@ AC_CONFIG_FILES([Makefile
> src/gallium/drivers/radeon/Makefile
> src/gallium/drivers/radeonsi/Makefile
> src/gallium/drivers/rbug/Makefile
> + src/gallium/drivers/renderonly/Makefile
> src/gallium/drivers/softpipe/Makefile
> src/gallium/drivers/svga/Makefile
> src/gallium/drivers/trace/Makefile
Don't recall of the top of my head but we might need the following
hunk. Otherwise the files won't end up in the tarball and configure
will scream at us.
--- a/src/gallium/Makefile.am
+++ b/src/gallium/Makefile.am
@@ -109,6 +109,7 @@ EXTRA_DIST = \
docs \
README.portability \
SConscript \
+ drivers/renderonly \
winsys/sw/gdi \
winsys/sw/hgl
> --- /dev/null
> +++ b/src/gallium/drivers/renderonly/Makefile.sources
> @@ -0,0 +1,4 @@
> +C_SOURCES := \
> + renderonly_context.c \
> + renderonly_resource.c \
> + renderonly_screen.c
Please list all the sources (including the headers) in here, sorted
alphabetically.
> --- /dev/null
> +++ b/src/gallium/drivers/renderonly/renderonly_context.c
[snip]
> +static void
> +renderonly_draw_vbo(struct pipe_context *pcontext,
> + const struct pipe_draw_info *pinfo)
> +{
> + struct renderonly_context *context = to_renderonly_context(pcontext);
> + struct pipe_draw_info info;
> +
> + if (pinfo && pinfo->indirect) {
Can pinfo really be null here ?
> + memcpy(&info, pinfo, sizeof(info));
> + info.indirect = renderonly_resource_unwrap(info.indirect);
During the unwrapping sometimes we're using the base object sometimes
the wrapped one. Can we use just the latter ? It should minimize the
(brief) 'wtf !?' moments.
[snip]
> +static void
> +renderonly_set_framebuffer_state(struct pipe_context *pcontext,
> + const struct pipe_framebuffer_state *fb)
> +{
> + struct renderonly_context *context = to_renderonly_context(pcontext);
> + struct pipe_framebuffer_state state;
> + unsigned i;
> +
> + if (fb) {
I doubt that fb can be NULL here.
[snip]
> +static void
> +renderonly_blit(struct pipe_context *pcontext,
> + const struct pipe_blit_info *pinfo)
> +{
> + struct renderonly_context *context = to_renderonly_context(pcontext);
> + struct pipe_blit_info info;
> +
> + if (pinfo) {
Again pinfo cannot/shouldn't be NULL here.
[snip]
> +static struct pipe_sampler_view *
> +renderonly_create_sampler_view(struct pipe_context *pcontext,
> + struct pipe_resource *ptexture,
> + const struct pipe_sampler_view *template)
> +{
> + struct renderonly_resource *texture =
> to_renderonly_resource(ptexture);
> + struct renderonly_context *context = to_renderonly_context(pcontext);
> + struct renderonly_sampler_view *view;
> +
> + view = calloc(1, sizeof(*view));
> + if (!view)
> + return NULL;
> +
> + view->gpu = context->gpu->create_sampler_view(context->gpu,
> + texture->gpu,
> + template);
Function can fail.
[snip]
> +static void *
> +renderonly_transfer_map(struct pipe_context *pcontext,
> + struct pipe_resource *presource,
> + unsigned level,
> + unsigned usage,
> + const struct pipe_box *box,
> + struct pipe_transfer **ptransfer)
> +{
> + struct renderonly_resource *resource =
> to_renderonly_resource(presource);
> + struct renderonly_context *context = to_renderonly_context(pcontext);
> + struct renderonly_transfer *transfer;
> +
> + transfer = calloc(1, sizeof(*transfer));
> + if (!transfer)
> + return NULL;
> +
> + transfer->map = context->gpu->transfer_map(context->gpu,
> + resource->gpu,
> + level,
> + usage,
> + box,
> + &transfer->gpu);
Ditto.
> --- /dev/null
> +++ b/src/gallium/drivers/renderonly/renderonly_resource.c
[snip]
> +static bool resource_import_scanout(struct renderonly_screen *screen,
> + struct renderonly_resource *resource,
> + const struct pipe_resource *template)
> +{
> + struct winsys_handle handle;
> + boolean status;
> + int fd, err;
> +
> + resource->gpu = screen->gpu->resource_create(screen->gpu,
> + template);
> + if (!resource->gpu)
> + return false;
> +
> + memset(&handle, 0, sizeof(handle));
> + handle.type = DRM_API_HANDLE_TYPE_FD;
> +
> + status = screen->gpu->resource_get_handle(screen->gpu,
> + resource->gpu,
> + &handle);
> + if (!status)
We're missing the cleanup in the error paths in this function.
> + return false;
> +
> + resource->stride = handle.stride;
> + fd = handle.handle;
> +
> + err = drmPrimeFDToHandle(screen->fd, fd, &resource->handle);
> + if (err < 0) {
> + fprintf(stderr, "drmPrimeFDToHandle() failed: %s\n",
> + strerror(errno));
> + close(fd);
Not 100% sure that we can/should close the fd, here and below.
> + return false;
> + }
> +
> + close(fd);
[snip]
> +static bool resource_dumb(struct renderonly_screen *screen,
> + struct renderonly_resource *resource,
> + const struct pipe_resource *template)
> +{
> + struct drm_mode_create_dumb create_dumb = { 0 };
> + struct winsys_handle handle;
> + int prime_fd, err;
> +
> + /* create dumb buffer at scanout GPU */
Use memset(&create_dumb...) like the rest of the patch ?
> + create_dumb.width = template->width0;
> + create_dumb.height = template->height0;
> + create_dumb.bpp = 32;
> + create_dumb.flags = 0;
> + create_dumb.pitch = 0;
> + create_dumb.size = 0;
> + create_dumb.handle = 0;
> +
> + err = ioctl(screen->fd, DRM_IOCTL_MODE_CREATE_DUMB, &create_dumb);
While I'm not objecting (esp. considering your usecase) there has been
concerns about doing any form of accelerated rendering + dumb fb. I'd
suspect that the relevant drivers and/or drm core might need to be
laxed for this to work ?
> + if (err < 0) {
> + fprintf(stderr, "DRM_IOCTL_MODE_CREATE_DUMB failed: %s\n",
> + strerror(errno));
> + return false;
> + }
> +
> + resource->handle = create_dumb.handle;
> + resource->stride = create_dumb.pitch;
> +
> + /* create resource at renderonly GPU */
> + resource->gpu = screen->gpu->resource_create(screen->gpu, template);
> + if (!resource->gpu)
Similar to resource_import_scanout() we're leaking various bits on error.
[snip]
> +struct pipe_resource *
> +renderonly_resource_create(struct pipe_screen *pscreen,
> + const struct pipe_resource *template)
> +{
[snip]
> +destroy:
> + if (resource->gpu)
> + screen->gpu->resource_destroy(screen->gpu, resource->gpu);
We don't directly create the resource* so we shouldn't destroy it.
[snip]
> --- /dev/null
> +++ b/src/gallium/drivers/renderonly/renderonly_screen.c
[snip]
> +static const char *
> +renderonly_get_name(struct pipe_screen *pscreen)
> +{
> + static char buffer[256];
> + struct renderonly_screen *screen = to_renderonly_screen(pscreen);
> +
> + util_snprintf(buffer, sizeof(buffer), "%s-%s",
> + drmGetDeviceNameFromFd(screen->fd),
drmGetDeviceNameFromFd() might not be the exact thing we want here,
but we can lots of time until the feature freeze to tweak it.
> + screen->gpu->get_name(screen->gpu));
> + return buffer;
> +}
> +
> +static const char *
> +renderonly_get_vendor(struct pipe_screen *pscreen)
> +{
> + return "renderonly";
This also looks a bit strange. Again can be fixed later on.
[snip]
> +static int renderonly_open_render_node(int fd)
> +{
> + return open("/dev/dri/renderD128", O_RDWR);
Doesn't drmGetRenderDeviceNameFromFd() provide the correct string here
? Might want to use loader_open_device() over open() as it also
handles O_CLOEXEC.
Cheers,
Emil
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
