Hi Emil,
Thanks for the review!
2015-10-16 1:09 GMT+02:00 Emil Velikov <emil.l.veli...@gmail.com>:
> Hi Christian,
>
> I'm glad to see Thierry's work revived. Hopefully this will soon be
> the basis of many more drivers.
I need to look into egl on wayland issue and hope to gain deeper knowledge
about the problem.
>
> 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
>
For sure :)
> 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.
What is the problem with doing such a blit?
>
> That aside, there are a few minor nitpicks below. With those sorted I
> believe the patch is good to land.
Good to hear.
>
>> 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.
I will check that. For the moment I did not look into that area.
>
> --- 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
>
Ok.
>> --- /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.
Ok.
>
>> --- /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.
>
Ok.
> [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.
Ok. I did not had a deep look at the call graph but did some kind of
defense programming 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.
>
Ok. Should I simply trust that it never can be NULL or can I add an assert?
> [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.
>
Correct. Will fix it.
> [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.
Correct. Will fix it.
>
>> --- /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.
>
Correct. Will fix it.
>> + 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.
>
Me neither. I will have a look at the docs.
>> + 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 ?
>
Yes - that makes it more consistent.
>> + 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 ?
Whats the meaning of laxed? Can tell me more about that concerns?
>
>> + 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.
>
Will have a look at it.
> [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.
>
Ok.
> [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.
>
Fine.
>> + 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.
>
We could return a mixture of scanout and renderonly driver vendor.
> [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.
>
I will have a look at it.
Thanks
--
Christian Gmeiner, MSc
https://soundcloud.com/christian-gmeiner
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
