[Mesa-dev] [RFC 1/2] gallium: add renderonly driver
Rob Clark
robdclark at gmail.com
Thu Oct 15 16:40:06 PDT 2015
On Thu, Oct 15, 2015 at 7:09 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> 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.gmeiner at 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.
imho, it is ok if driver specific (or maybe in this case we should
call it "semi driver-specific") code is blitting (or otherwise using
gpu) on dumb buffers.. the main point about dumb buffers is some
particular gpu may not support operating on dumb buffers, so truly
generic code outside of drivers should not make assumptions..
I guess this code counts as "helper" code, so some hw that could not
support dumb buffer access simply doesn't use it and implements their
own version instead..
BR,
-R
> 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.gmeiner at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list