Hi Thierry,
2016-12-19 14:08 GMT+01:00 Thierry Reding <thierry.red...@gmail.com>:
> On Wed, Nov 30, 2016 at 02:44:34PM +0100, Christian Gmeiner wrote:
>> This a very lightweight library to add basic support for
>> renderonly GPUs. It does all the magic regarding in/exporting
>> buffers etc. This library will likely break android support and
>> hopefully will get replaced with a better solution based on gbm2.
>>
>> Signed-off-by: Christian Gmeiner <christian.gmei...@gmail.com>
>> ---
>> src/gallium/Automake.inc | 5 +
>> src/gallium/auxiliary/Makefile.am | 10 ++
>> src/gallium/auxiliary/Makefile.sources | 4 +
>> src/gallium/auxiliary/renderonly/renderonly.c | 199
>> ++++++++++++++++++++++++++
>> src/gallium/auxiliary/renderonly/renderonly.h | 81 +++++++++++
>> 5 files changed, 299 insertions(+)
>> create mode 100644 src/gallium/auxiliary/renderonly/renderonly.c
>> create mode 100644 src/gallium/auxiliary/renderonly/renderonly.h
>
> Hi Christian,
>
> sorry for being late to the party. I only ran across this by accident.
> Would you mind keeping me in Cc for subsequent versions of this? It's
> been more than two years since I wrote the original proposal for this,
> but I'm still interested in seeing a solution emerge.
>
Sure will add you to CC list. Btw. your timing could not be better I
wanted to send out
v2 today. But I will hold it back and incooperate your feedback.
> Anyway, thanks for picking this up.
>
> From a diff point of view this is certainly much more terse than the
> original proposal. I find it slightly unfortunate that the drivers for
> the render GPU have to be changed. But it's hard to argue with the
> reduction in size.
>
> I still think that Tegra will eventually require more than the stub that
> you have in this series because the same device that exposes the scanout
> engine also contains units that can do video compositing, decoding and
> encoding. There's in fact recently been discussions on how best to
> provide that functionality and Mesa looks like the best long-term target
> currently. Anyway, that's a bridge I think we can cross when we get
> there, this concept looks fine to get started.
>
>> diff --git a/src/gallium/Automake.inc b/src/gallium/Automake.inc
>> index 6fe2e22..6aadcb9 100644
>> --- a/src/gallium/Automake.inc
>> +++ b/src/gallium/Automake.inc
>> @@ -50,6 +50,11 @@ GALLIUM_COMMON_LIB_DEPS = \
>> $(PTHREAD_LIBS) \
>> $(DLOPEN_LIBS)
>>
>> +if HAVE_LIBDRM
>> +GALLIUM_COMMON_LIB_DEPS += \
>> + $(LIBDRM_LIBS)
>> +endif
>
> Nit: Is the conditional necessary here? LIBDRM_LIBS would be empty if
> libdrm wasn't found, right? So no harm in just appending it to the
> GALLIUM_COMMON_LIB_DEPS variable unconditonally?
>
Done.
>> 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.
>
>> 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.gmei...@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.gmei...@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"
>> +
>> +#include <errno.h>
>> +#include <fcntl.h>
>> +#include <stdio.h>
>> +#include <sys/ioctl.h>
>> +#include <xf86drm.h>
>> +
>> +#include "state_tracker/drm_driver.h"
>> +#include "pipe/p_screen.h"
>> +#include "util/u_memory.h"
>> +
>> +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);
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.
>> +{
>> + 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.
>> + ro->ops = ops;
>> + ro->priv = priv;
>> +
>> + ro->screen = ops->create(ro);
>
> It's somewhat odd that drivers need to jump through this hoop in order
> to create their screens. I think it would be easier if they just
> embedded the struct renderonly and initialized it in their create
> function.
>
> I think that's probably what Nicolai and Emil were saying as well.
Yeah.. with V2 everything feels much easier to understand and cleaner.
>
>> + if (!ro->screen)
>> + goto cleanup;
>> +
>> + return ro->screen;
>> +
>> +cleanup:
>> + FREE(ro);
>> +
>> + return NULL;
>> +}
>> +
>> +static bool
>> +use_kms_bumb_buffer(struct renderonly_scanout *scanout,
>
> You probably meant "dumb" here. Somehow you managed to get it right in
> the variables, but the function name and the callsite consistently have
> the same typo. =)
>
Upps.. :)
>> + struct pipe_resource *rsc, struct renderonly *ro)
>> +{
>> + struct winsys_handle handle;
>> + int prime_fd, err;
>> + struct drm_mode_create_dumb create_dumb = {
>> + .width = rsc->width0,
>> + .height = rsc->height0,
>> + .bpp = 32,
>
> This seems like maybe too strong an assumption. Or at the very least a
> waste of memory. What if somebody wanted to use 16- or 24-bit buffers?
>
Good point... have an answer for this later in the mail.
>> +static bool
>> +import_gpu_scanout(struct renderonly_scanout *scanout,
>> + struct pipe_resource *rsc, struct renderonly *ro)
>> +{
>> + struct pipe_screen *screen = ro->screen;
>> + boolean status;
>> + int fd, err;
>> + struct winsys_handle handle = {
>> + .type = DRM_API_HANDLE_TYPE_FD
>> + };
>> +
>> + status = screen->resource_get_handle(screen, NULL, rsc, &handle,
>> + PIPE_HANDLE_USAGE_READ_WRITE);
>> + if (!status)
>> + return false;
>> +
>> + scanout->stride = handle.stride;
>> + fd = handle.handle;
>> +
>> + err = drmPrimeFDToHandle(ro->kms_fd, fd, &scanout->handle);
>> + close(fd);
>> +
>> + if (err < 0) {
>> + fprintf(stderr, "drmPrimeFDToHandle() failed: %s\n", strerror(errno));
>> + return false;
>> + }
>> +
>> + if (ro->ops->tiling) {
>
> tiling is maybe not the best name for this, since arguable the scanout
> driver might want to do any number of other things while importing.
> Perhaps ->import() would be generic enough?
>
import() sounds fine to me but maybe we can drop it.
>> + err = ro->ops->tiling(ro->kms_fd, scanout->handle);
>> + if (err < 0) {
>> + fprintf(stderr, "failed to set tiling parameters: %s\n",
>> strerror(errno));
>> + close(scanout->handle);
>
> I don't think scanout->handle is something you should be passing to
> close().
>
Yeah.. looks you are right.. fill fix that in V2.
>> + return false;
>> + }
>> + }
>> +
>> + return true;
>> +}
>> +
>> +struct renderonly_scanout *
>> +renderonly_scanout_for_resource(struct pipe_resource *rsc, struct
>> renderonly *ro)
>> +{
>> + struct renderonly_scanout *scanout;
>> + bool ret;
>> +
>> + scanout = CALLOC_STRUCT(renderonly_scanout);
>> + if (!scanout)
>> + return NULL;
>> +
>> + if (ro->ops->intermediate_rendering)
>> + ret = use_kms_bumb_buffer(scanout, rsc, ro);
>> + else
>> + ret = import_gpu_scanout(scanout, rsc, ro);
>
> Given my kernel background I've grown allergic to this kind of
> construct. What you're creating here is a midlayer that's very likely to
> cause headaches down the road. I think a better alternative would be to
> add a callback to the struct renderonly_ops that drivers have to set to
> whatever the implementation is that they want.
This is a very good point and I thought about it some days ago.
> 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!
> On of the problem, the most immediate in this case, with this kind of
> midlayer is that if intermediate_rendering == {false,true} is not enough
> for a given use-case, we end up having to add yet another flag that
> specifies behaviour.
>
Thanks for your review!
greets
--
Christian Gmeiner, MSc
https://soundcloud.com/christian-gmeiner
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
