[Mesa-dev] [PATCH 1/3] gallium: add renderonly library
Thierry Reding
thierry.reding at gmail.com
Mon Dec 19 21:50:47 UTC 2016
On Mon, Dec 19, 2016 at 08:54:04PM +0100, Christian Gmeiner wrote:
> 2016-12-19 14:08 GMT+01:00 Thierry Reding <thierry.reding at gmail.com>:
> > On Wed, Nov 30, 2016 at 02:44:34PM +0100, Christian Gmeiner wrote:
[...]
> >> 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.
>
> I just redone what other have done in that file. There are some if's
> in that file and I am unsure
> what to do here.
Maybe Emil has a strong opinion here, I was really just nit-picking, so
feel free to leave this.
> >> 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?
> >
>
> I think thats a matter of style but I can puit it above #include
> "state_tracker/drm_driver.h"
I prefer having these in hierarchical order. That is, anything from the
C runtime goes first, then other system includes, followed by project
core includes and finally driver-local includes. By that ordering
renderonly.h would be very last in line. Not sure if anyone in Mesa is
pedantic enough to insist on a common style, so feel free to leave this
as-is if you prefer.
> >> +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.
> >
>
> In my current version I have only one struct left:
>
> struct renderonly {
> int (*tiling)(int fd, uint32_t handle); /**< for !intermediate_rendering */
> bool intermediate_rendering;
> int kms_fd;
> int gpu_fd;
> };
>
> And here is how the only function in imx-drm winsys looks like:
>
> struct pipe_screen *imx_drm_screen_create(int fd)
> {
> struct renderonly ro = {
> .intermediate_rendering = true,
> .kms_fd = fd,
> .gpu_fd = open("/dev/dri/renderD128", O_RDWR | O_CLOEXEC)
> };
>
> if (ro.gpu_fd < 0)
> return NULL;
>
> struct pipe_screen *screen = etna_drm_screen_create_renderonly(&ro);
Erm... hopefully etna_drm_screen_create_renderonly() will make a copy of
everything it needs, otherwise you'd be referencing stack that's gone
out of scope.
The above is about as thin as it can get, but it should also be flexible
enough to allow embedding within a driver-private pipe_screen wrapper,
so I think that's fine. I'll try and port over the Tegra patches when
you've posted the new version, and report how far I get.
> if (!screen)
> close(ro.gpu_fd);
>
> return screen;
> }
>
> So everything got more explicit regarding file handles and the struct represents
> the combination of the kms and a gpu device now. I am not sure if the rename
> is still needed but if it is maybe something like renderonly_ctx would work.
I can't think of a really good name off the top of my head, so let's
keep it as-is for now. If I ever think of anything better I can propose
a patch to change it.
> >> +{
> >> + 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.
> >
>
> Was true for V1 but in V2 everything looks quite different.
True. But then, looking at the new struct renderonly, do we really need
both file descriptors? It looks to me like the renderonly driver will
only need its fd and will likely store that internally. And if drivers
create a slightly thicker wrapper than what you do in i.MX they'd need
to store the KMS fd somewhere else, too.
In the end, that's something we can always redo incrementally. I think
what you've come up with is certainly good enough for now.
> > 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.
> >
>
> So the struct renderonly could look like:
>
> struct renderonly {
> struct renderonly_scanout *(*create_for_resource)(struct pipe_resource *rsc);
> int kms_fd;
> int gpu_fd;
> };
>
> Now the kms winsys part can define what needs to be done in a generic way. I
> am not sure if it would make lot of sense to provide a use_kms_bumb_buffer()
> or import_gpu_scanout() public functions at all as every driver may need a small
> variation if them. This also solves the 16 vs. 24 vs 32 bit buffer
> problem by pushing
> it directly to the kms winsys part - nice!
Yeah, this is a lot less midlayer. I think it should work at least for
all our initial cases. Given the simplicity it would also be easy to
extend if anyone ever needed anything else.
I think there would probably still be some use in a standard helper for
the dumb buffer use-case, but maybe that can be deferred until there's a
second driver that needs it. Currently we seem to have only the dumb
buffer case for i.MX and the GPU import case for Tegra.
If you go for the above, you might want to make sure to document what
exactly create_for_resource() is supposed to do. Taking a pipe_resource
indicates that it would take an existing resource and wrap it, but then
for the dumb buffer case it will actually go and allocate it. So if rsc
is actually only a template, perhaps it can be made const and renamed to
template?
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/227166ca/attachment.sig>
More information about the mesa-dev
mailing list