[Mesa-dev] [PATCH 1/3] gallium: add renderonly library
Christian Gmeiner
christian.gmeiner at gmail.com
Tue Dec 20 07:07:21 UTC 2016
2016-12-19 22:50 GMT+01:00 Thierry Reding <thierry.reding at gmail.com>:
> 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.
>
Okay.. I think this could be fixed after it went in.
>> >> +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.
>
It does it via renderonly_dup(..).
> 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.
>
Wonderful.
>> >> +{
>> >> + 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.
>
Ok.
>> > 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?
>
Thanks for you feedback again.
greets
--
Christian Gmeiner, MSc
https://soundcloud.com/christian-gmeiner
More information about the mesa-dev
mailing list