Re: [Mesa-dev] [PATCH 1/3] gallium: add renderonly library

2016-12-24 Thread Emil Velikov
On 19 December 2016 at 21:50, Thierry Reding  wrote:
> On Mon, Dec 19, 2016 at 08:54:04PM +0100, Christian Gmeiner wrote:
>> 2016-12-19 14:08 GMT+01:00 Thierry Reding :
>> > 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.
>
There's no strong policy for these, plus we have a mix of the two atm.
I'm fine either way on the include/cflags, but if anyone has strong
preference sure go ahead :-)

Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev


Re: [Mesa-dev] [PATCH 1/3] gallium: add renderonly library

2016-12-19 Thread Christian Gmeiner
2016-12-19 22:50 GMT+01:00 Thierry Reding :
> On Mon, Dec 19, 2016 at 08:54:04PM +0100, Christian Gmeiner wrote:
>> 2016-12-19 14:08 GMT+01:00 Thierry Reding :
>> > 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 000..c4ea784
>> >> --- /dev/null
>> >> +++ b/src/gallium/auxiliary/renderonly/renderonly.c
>> >> @@ -0,0 +1,199 @@
>> >> +/*
>> >> + * Copyright (C) 2016 Christian Gmeiner 
>> >> + *
>> >> + * 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 
>> >> + */
>> >> +
>> >> +#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;
>>

Re: [Mesa-dev] [PATCH 1/3] gallium: add renderonly library

2016-12-19 Thread Thierry Reding
On Mon, Dec 19, 2016 at 08:54:04PM +0100, Christian Gmeiner wrote:
> 2016-12-19 14:08 GMT+01:00 Thierry Reding :
> > 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 000..c4ea784
> >> --- /dev/null
> >> +++ b/src/gallium/auxiliary/renderonly/renderonly.c
> >> @@ -0,0 +1,199 @@
> >> +/*
> >> + * Copyright (C) 2016 Christian Gmeiner 
> >> + *
> >> + * 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 
> >> + */
> >> +
> >> +#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 

Re: [Mesa-dev] [PATCH 1/3] gallium: add renderonly library

2016-12-19 Thread Christian Gmeiner
Hi Thierry,

2016-12-19 14:08 GMT+01:00 Thierry Reding :
> 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 
>> ---
>>  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 000..c4ea784
>> --- /dev/null
>> +++ b/src/gallium/auxiliary/renderonly/renderonly.c
>> @@ -0,0 +1,199 @@
>> +/*
>> + * Copyright (C) 2016 Christian Gmeiner 
>> + *
>> + * 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, 

Re: [Mesa-dev] [PATCH 1/3] gallium: add renderonly library

2016-12-19 Thread Thierry Reding
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 
> ---
>  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.

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?

>  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.

> 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 000..c4ea784
> --- /dev/null
> +++ b/src/gallium/auxiliary/renderonly/renderonly.c
> @@ -0,0 +1,199 @@
> +/*
> + * Copyright (C) 2016 Christian Gmeiner 
> + *
> + * 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 
> + */
> 

Re: [Mesa-dev] [PATCH 1/3] gallium: add renderonly library

2016-12-09 Thread Alexandre Courbot
Hi Emil,

On 12/09/2016 11:20 PM, Emil Velikov wrote:
> On 9 December 2016 at 13:20, Alexandre Courbot  wrote:
>> On 12/08/2016 04:16 PM, Alexandre Courbot wrote:
>>> On 11/30/2016 10:44 PM, 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.
>>>
>>> Since we have no idea when said better solution will be available, and
>>> the situation of render-only GPUs has been unsustainable for way too
>>> long, I really hope a solution like this one can be merged in the meantime.
>>>
>>> I have tried it after porting support for Tegra
>>> (https://github.com/austriancoder/mesa/commit/2c7354701ee21ca28f69f5d7588f1d497553b4bf)
>>> to this latest version. Here are a few issues I have met:
>>>
>>> First, setting the tiling works indeed just fine if we are using an
>>> ioctl for this. However my impression was that the preferred way of
>>> doing it was through FB modifiers, and we started moving Tegra to this
>>> scheme. Problem: the FB modifier is passed through a call to
>>> drmModeAddFB2WithModifiers(), which is called by the client program, not
>>> Mesa - which in this case leaves the program with the burden of figuring
>>> out what the modifier should be. So with FB modifiers the problem is
>>> still here.
>>>
>>> Another issue I have seen is that GLX does not seem to work with this.
>>> X/modesetting starts just fine, and GLamor also seems to initialize.
>>> However glxinfo freezes on a xshmfence_await() call, and all GLX
>>> programs fail as follow:
>>
>> Solved that issue by forcing is_different_gpu to true in
>> loader_dri3_drawable_init() (pretty hackish, looking for a better way).
>>
>> Also I had another issue with Wayland where EGL windows would be
>> displayed all black. I traced this to the fact that Wayland was trying
>> to share the buffer by calling the old FLINK ioctl on the rendernode
>> device, which is forbidden. Opening card1 instead of renderD128 did the
>> trick as a workaround, but I am surprised as I thought Wayland was using
>> DRI3 exclusively? I am not very familiar with neither Mesa nor Wayland
>> though, so my assumption may very well be incorrect.
>>
> Some of these issues is due to the hardcoded nature of the card/render
> node. I've had drmDevice API which could/should be extended and
> utilised here.
> Earlier versions were quite buggy, so make sure to use
> 677cd97dc4a930af508388713f5016baf664ed18 or later.

My libdrm was 2.4.74, so I think this patch is there.

> 
> Since from kernel there is no relation between the KMS and GPU device,
> one will need to apply some heuristics locally. At some point we might
> want to make things more systematic/configurable, but let's get it
> working first ;-)
> 
> Thus, please propose/add anything to drmDevice that will you think is
> enough to build some heuristics on.
> 
> With that sorted, the Wayland FLINK issues should go away.

Thanks for the hint. I will look into that.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev


Re: [Mesa-dev] [PATCH 1/3] gallium: add renderonly library

2016-12-09 Thread Alexandre Courbot
Hi Daniel,

On 12/09/2016 11:13 PM, Daniel Stone wrote:
> Hi Alexandre,
> 
> On 9 December 2016 at 13:20, Alexandre Courbot  wrote:
>> On 12/08/2016 04:16 PM, Alexandre Courbot wrote:
>>> First, setting the tiling works indeed just fine if we are using an
>>> ioctl for this. However my impression was that the preferred way of
>>> doing it was through FB modifiers, and we started moving Tegra to this
>>> scheme. Problem: the FB modifier is passed through a call to
>>> drmModeAddFB2WithModifiers(), which is called by the client program, not
>>> Mesa - which in this case leaves the program with the burden of figuring
>>> out what the modifier should be. So with FB modifiers the problem is
>>> still here.
>>>
>>> Another issue I have seen is that GLX does not seem to work with this.
>>> X/modesetting starts just fine, and GLamor also seems to initialize.
>>> However glxinfo freezes on a xshmfence_await() call, and all GLX
>>> programs fail as follow:
>>
>> Solved that issue by forcing is_different_gpu to true in
>> loader_dri3_drawable_init() (pretty hackish, looking for a better way).
>>
>> Also I had another issue with Wayland where EGL windows would be
>> displayed all black. I traced this to the fact that Wayland was trying
>> to share the buffer by calling the old FLINK ioctl on the rendernode
>> device, which is forbidden. Opening card1 instead of renderD128 did the
>> trick as a workaround, but I am surprised as I thought Wayland was using
>> DRI3 exclusively? I am not very familiar with neither Mesa nor Wayland
>> though, so my assumption may very well be incorrect.
> 
> Wayland doesn't use DRI-anything; Mesa has its own interface for
> Wayland. I'm really surprised that you're seeing this behaviour
> though: if you search for WL_DRM_CAPABILITY_PRIME (i.e. send dmabufs
> rather than flink names) in src/egl/drivers/dri2/platform_wayland.c,
> you'll see that a) we always use it when available, and b) we refuse
> to initialise when the device is a rendernode and we don't have PRIME.
> So I'm not sure how this could ever happen ...

Interesting. I will try to get to the bottom of this as a way to improve
my (weak) understanding of Mesa.

> 
>> Anyway, with this patch and the corresponding Tegra support, I have a
>> working solution that can run unmodified Mesa applications using KMS,
>> EGL/Wayland and GLX backends on TK1 and TX1 platforms. Neat!
> 
> Cool! I assume this will work on Tegra124 more generally then - do you
> have a branch somewhere?

Yes, anything using Tegra124 or Tegra210 with working display drivers (a
few Chromebooks so far, and probably the Pixel C sometime soon) should
directly benefit from it.

I have pushed a branch (just Christian's initial branch + a port of his
first Tegra patch and my hacks to make Wayland and GLX work) here:
https://github.com/Gnurou/mesa/tree/renderonly

> 
>> Considering that we have been ressorting to hacking all the KMS
>> applications of interest to connect the render and display nodes
>> together with the right tiling settings for the last two years, I regard
>> this patch as a huge improvement for mobile graphics and would like to
>> strongly support it.
>>
>> My only remaining concern is that this scheme cannot support the case
>> where the tiling format is specified using FB modifiers, since this
>> requires drmModeAddFB2WithModifiers() to be called from the application.
>> So for Tegra we have to resort to a staging, not enabled by default
>> SET_TILING ioctl. Not ideal, but recompiling your kernel with an
>> additional config option is much less a hassle than patching every KMS
>> app under the sun.
>>
>> So while thoughts about how this last issue can be addressed are
>> welcome, I think this little lib can improve the life of many SoC users.
> 
> Check out Ben Widawsky's 'Renderbuffer Decompression (and GBM
> modifiers)' patchset. With this, as well as krh's pending GETPLANE2
> ioctl that will allow us to get a list of acceptable modifiers for
> display from KMS, we can trivially implement this in clients without
> the need for a backchannel ioctl:
> https://git.collabora.com/cgit/user/daniels/weston.git/commit/?h=wip/2016-11/gbm-planes-modifiers

That should make a good reading for the weekend. :) Thanks!

Alex.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev


Re: [Mesa-dev] [PATCH 1/3] gallium: add renderonly library

2016-12-09 Thread Emil Velikov
On 9 December 2016 at 13:20, Alexandre Courbot  wrote:
> On 12/08/2016 04:16 PM, Alexandre Courbot wrote:
>> On 11/30/2016 10:44 PM, 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.
>>
>> Since we have no idea when said better solution will be available, and
>> the situation of render-only GPUs has been unsustainable for way too
>> long, I really hope a solution like this one can be merged in the meantime.
>>
>> I have tried it after porting support for Tegra
>> (https://github.com/austriancoder/mesa/commit/2c7354701ee21ca28f69f5d7588f1d497553b4bf)
>> to this latest version. Here are a few issues I have met:
>>
>> First, setting the tiling works indeed just fine if we are using an
>> ioctl for this. However my impression was that the preferred way of
>> doing it was through FB modifiers, and we started moving Tegra to this
>> scheme. Problem: the FB modifier is passed through a call to
>> drmModeAddFB2WithModifiers(), which is called by the client program, not
>> Mesa - which in this case leaves the program with the burden of figuring
>> out what the modifier should be. So with FB modifiers the problem is
>> still here.
>>
>> Another issue I have seen is that GLX does not seem to work with this.
>> X/modesetting starts just fine, and GLamor also seems to initialize.
>> However glxinfo freezes on a xshmfence_await() call, and all GLX
>> programs fail as follow:
>
> Solved that issue by forcing is_different_gpu to true in
> loader_dri3_drawable_init() (pretty hackish, looking for a better way).
>
> Also I had another issue with Wayland where EGL windows would be
> displayed all black. I traced this to the fact that Wayland was trying
> to share the buffer by calling the old FLINK ioctl on the rendernode
> device, which is forbidden. Opening card1 instead of renderD128 did the
> trick as a workaround, but I am surprised as I thought Wayland was using
> DRI3 exclusively? I am not very familiar with neither Mesa nor Wayland
> though, so my assumption may very well be incorrect.
>
Some of these issues is due to the hardcoded nature of the card/render
node. I've had drmDevice API which could/should be extended and
utilised here.
Earlier versions were quite buggy, so make sure to use
677cd97dc4a930af508388713f5016baf664ed18 or later.

Since from kernel there is no relation between the KMS and GPU device,
one will need to apply some heuristics locally. At some point we might
want to make things more systematic/configurable, but let's get it
working first ;-)

Thus, please propose/add anything to drmDevice that will you think is
enough to build some heuristics on.

With that sorted, the Wayland FLINK issues should go away.

> Anyway, with this patch and the corresponding Tegra support, I have a
> working solution that can run unmodified Mesa applications using KMS,
> EGL/Wayland and GLX backends on TK1 and TX1 platforms. Neat!
>
> Considering that we have been ressorting to hacking all the KMS
> applications of interest to connect the render and display nodes
> together with the right tiling settings for the last two years, I regard
> this patch as a huge improvement for mobile graphics and would like to
> strongly support it.
>
> My only remaining concern is that this scheme cannot support the case
> where the tiling format is specified using FB modifiers, since this
> requires drmModeAddFB2WithModifiers() to be called from the application.
> So for Tegra we have to resort to a staging, not enabled by default
> SET_TILING ioctl. Not ideal, but recompiling your kernel with an
> additional config option is much less a hassle than patching every KMS
> app under the sun.
>
> So while thoughts about how this last issue can be addressed are
> welcome, I think this little lib can improve the life of many SoC users.
Agreed - let's have things as-is. One can "polish" the backend side of
things, once we have something in place.

Thanks
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev


Re: [Mesa-dev] [PATCH 1/3] gallium: add renderonly library

2016-12-09 Thread Daniel Stone
Hi Alexandre,

On 9 December 2016 at 13:20, Alexandre Courbot  wrote:
> On 12/08/2016 04:16 PM, Alexandre Courbot wrote:
>> First, setting the tiling works indeed just fine if we are using an
>> ioctl for this. However my impression was that the preferred way of
>> doing it was through FB modifiers, and we started moving Tegra to this
>> scheme. Problem: the FB modifier is passed through a call to
>> drmModeAddFB2WithModifiers(), which is called by the client program, not
>> Mesa - which in this case leaves the program with the burden of figuring
>> out what the modifier should be. So with FB modifiers the problem is
>> still here.
>>
>> Another issue I have seen is that GLX does not seem to work with this.
>> X/modesetting starts just fine, and GLamor also seems to initialize.
>> However glxinfo freezes on a xshmfence_await() call, and all GLX
>> programs fail as follow:
>
> Solved that issue by forcing is_different_gpu to true in
> loader_dri3_drawable_init() (pretty hackish, looking for a better way).
>
> Also I had another issue with Wayland where EGL windows would be
> displayed all black. I traced this to the fact that Wayland was trying
> to share the buffer by calling the old FLINK ioctl on the rendernode
> device, which is forbidden. Opening card1 instead of renderD128 did the
> trick as a workaround, but I am surprised as I thought Wayland was using
> DRI3 exclusively? I am not very familiar with neither Mesa nor Wayland
> though, so my assumption may very well be incorrect.

Wayland doesn't use DRI-anything; Mesa has its own interface for
Wayland. I'm really surprised that you're seeing this behaviour
though: if you search for WL_DRM_CAPABILITY_PRIME (i.e. send dmabufs
rather than flink names) in src/egl/drivers/dri2/platform_wayland.c,
you'll see that a) we always use it when available, and b) we refuse
to initialise when the device is a rendernode and we don't have PRIME.
So I'm not sure how this could ever happen ...

> Anyway, with this patch and the corresponding Tegra support, I have a
> working solution that can run unmodified Mesa applications using KMS,
> EGL/Wayland and GLX backends on TK1 and TX1 platforms. Neat!

Cool! I assume this will work on Tegra124 more generally then - do you
have a branch somewhere?

> Considering that we have been ressorting to hacking all the KMS
> applications of interest to connect the render and display nodes
> together with the right tiling settings for the last two years, I regard
> this patch as a huge improvement for mobile graphics and would like to
> strongly support it.
>
> My only remaining concern is that this scheme cannot support the case
> where the tiling format is specified using FB modifiers, since this
> requires drmModeAddFB2WithModifiers() to be called from the application.
> So for Tegra we have to resort to a staging, not enabled by default
> SET_TILING ioctl. Not ideal, but recompiling your kernel with an
> additional config option is much less a hassle than patching every KMS
> app under the sun.
>
> So while thoughts about how this last issue can be addressed are
> welcome, I think this little lib can improve the life of many SoC users.

Check out Ben Widawsky's 'Renderbuffer Decompression (and GBM
modifiers)' patchset. With this, as well as krh's pending GETPLANE2
ioctl that will allow us to get a list of acceptable modifiers for
display from KMS, we can trivially implement this in clients without
the need for a backchannel ioctl:
https://git.collabora.com/cgit/user/daniels/weston.git/commit/?h=wip/2016-11/gbm-planes-modifiers

Cheers,
Daniel
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev


Re: [Mesa-dev] [PATCH 1/3] gallium: add renderonly library

2016-12-09 Thread Alexandre Courbot
On 12/08/2016 04:16 PM, Alexandre Courbot wrote:
> On 11/30/2016 10:44 PM, 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.
> 
> Since we have no idea when said better solution will be available, and
> the situation of render-only GPUs has been unsustainable for way too
> long, I really hope a solution like this one can be merged in the meantime.
> 
> I have tried it after porting support for Tegra
> (https://github.com/austriancoder/mesa/commit/2c7354701ee21ca28f69f5d7588f1d497553b4bf)
> to this latest version. Here are a few issues I have met:
> 
> First, setting the tiling works indeed just fine if we are using an
> ioctl for this. However my impression was that the preferred way of
> doing it was through FB modifiers, and we started moving Tegra to this
> scheme. Problem: the FB modifier is passed through a call to
> drmModeAddFB2WithModifiers(), which is called by the client program, not
> Mesa - which in this case leaves the program with the burden of figuring
> out what the modifier should be. So with FB modifiers the problem is
> still here.
> 
> Another issue I have seen is that GLX does not seem to work with this.
> X/modesetting starts just fine, and GLamor also seems to initialize.
> However glxinfo freezes on a xshmfence_await() call, and all GLX
> programs fail as follow:

Solved that issue by forcing is_different_gpu to true in
loader_dri3_drawable_init() (pretty hackish, looking for a better way).

Also I had another issue with Wayland where EGL windows would be
displayed all black. I traced this to the fact that Wayland was trying
to share the buffer by calling the old FLINK ioctl on the rendernode
device, which is forbidden. Opening card1 instead of renderD128 did the
trick as a workaround, but I am surprised as I thought Wayland was using
DRI3 exclusively? I am not very familiar with neither Mesa nor Wayland
though, so my assumption may very well be incorrect.

Anyway, with this patch and the corresponding Tegra support, I have a
working solution that can run unmodified Mesa applications using KMS,
EGL/Wayland and GLX backends on TK1 and TX1 platforms. Neat!

Considering that we have been ressorting to hacking all the KMS
applications of interest to connect the render and display nodes
together with the right tiling settings for the last two years, I regard
this patch as a huge improvement for mobile graphics and would like to
strongly support it.

My only remaining concern is that this scheme cannot support the case
where the tiling format is specified using FB modifiers, since this
requires drmModeAddFB2WithModifiers() to be called from the application.
So for Tegra we have to resort to a staging, not enabled by default
SET_TILING ioctl. Not ideal, but recompiling your kernel with an
additional config option is much less a hassle than patching every KMS
app under the sun.

So while thoughts about how this last issue can be addressed are
welcome, I think this little lib can improve the life of many SoC users.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev


Re: [Mesa-dev] [PATCH 1/3] gallium: add renderonly library

2016-12-07 Thread Alexandre Courbot
On 11/30/2016 10:44 PM, 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.

Since we have no idea when said better solution will be available, and
the situation of render-only GPUs has been unsustainable for way too
long, I really hope a solution like this one can be merged in the meantime.

I have tried it after porting support for Tegra
(https://github.com/austriancoder/mesa/commit/2c7354701ee21ca28f69f5d7588f1d497553b4bf)
to this latest version. Here are a few issues I have met:

First, setting the tiling works indeed just fine if we are using an
ioctl for this. However my impression was that the preferred way of
doing it was through FB modifiers, and we started moving Tegra to this
scheme. Problem: the FB modifier is passed through a call to
drmModeAddFB2WithModifiers(), which is called by the client program, not
Mesa - which in this case leaves the program with the burden of figuring
out what the modifier should be. So with FB modifiers the problem is
still here.

Another issue I have seen is that GLX does not seem to work with this.
X/modesetting starts just fine, and GLamor also seems to initialize.
However glxinfo freezes on a xshmfence_await() call, and all GLX
programs fail as follow:

# glxgears
libGL error: MESA-LOADER: failed to retrieve device information
MESA-LOADER: failed to retrieve device information
MESA-LOADER: failed to retrieve device information
X Error of failed request:  BadAlloc (insufficient resources for operation)
  Major opcode of failed request:  149 ()
  Minor opcode of failed request:  2
  Serial number of failed request:  37
  Current serial number in output stream:  38

Not sure if you are having these issues with Vivante as well?
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev


Re: [Mesa-dev] [PATCH 1/3] gallium: add renderonly library

2016-12-05 Thread Christian Gmeiner
Hi Emil,


2016-12-01 15:42 GMT+01:00 Emil Velikov :
> On 1 December 2016 at 12:00, Nicolai Hähnle  wrote:
>> Congratulations on a huge amount of work! Obviously I can't say much about
>> the driver itself. Some things that I noticed for the renderonly library.
>>
>> On 30.11.2016 14:44, 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.
>>
>>
>> Some more comments would be _really_ helpful. What is the purpose of a
>> "scanout" object? What does for_prime vs. for_resource mean? What does
>> intermediate_rendering mean and what is it good for?
>>
>> The lifecycle of the renderonly object itself looks wrong to me:
>> renderonly_screen_create calls the actual driver's screen_create, but the
>> actual driver's screen_create frees the renderonly struct? Please make it
>> consistent: Either have a proper wrapper or (better, since this is so
>> lightweight) have the driver's screen_create call the
>> renderonly_screen_create.
>>
> I think Nicolai has it spot on here - both on the comment and wrapping side.
> Please include some documentation (even the GBM2 snippet from the
> cover letter) here. Be that in the summary and/or code.
>

Will come up with some docs.

thanks for your review!
--
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


Re: [Mesa-dev] [PATCH 1/3] gallium: add renderonly library

2016-12-05 Thread Christian Gmeiner
Hi Nicolai,


2016-12-01 13:00 GMT+01:00 Nicolai Hähnle :
> Congratulations on a huge amount of work! Obviously I can't say much about
> the driver itself. Some things that I noticed for the renderonly library.
>
> On 30.11.2016 14:44, 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.
>
>
> Some more comments would be _really_ helpful. What is the purpose of a
> "scanout" object? What does for_prime vs. for_resource mean? What does
> intermediate_rendering mean and what is it good for?
>

Makes sense and I will try to come up with a nice way to document it.

>
> The lifecycle of the renderonly object itself looks wrong to me:
> renderonly_screen_create calls the actual driver's screen_create, but the
> actual driver's screen_create frees the renderonly struct? Please make it
> consistent: Either have a proper wrapper or (better, since this is so
> lightweight) have the driver's screen_create call the
> renderonly_screen_create.
>

I will rework this part for v2.

thanks a lot for your comments!
--
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


Re: [Mesa-dev] [PATCH 1/3] gallium: add renderonly library

2016-12-01 Thread Emil Velikov
On 1 December 2016 at 12:00, Nicolai Hähnle  wrote:
> Congratulations on a huge amount of work! Obviously I can't say much about
> the driver itself. Some things that I noticed for the renderonly library.
>
> On 30.11.2016 14:44, 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.
>
>
> Some more comments would be _really_ helpful. What is the purpose of a
> "scanout" object? What does for_prime vs. for_resource mean? What does
> intermediate_rendering mean and what is it good for?
>
> The lifecycle of the renderonly object itself looks wrong to me:
> renderonly_screen_create calls the actual driver's screen_create, but the
> actual driver's screen_create frees the renderonly struct? Please make it
> consistent: Either have a proper wrapper or (better, since this is so
> lightweight) have the driver's screen_create call the
> renderonly_screen_create.
>
I think Nicolai has it spot on here - both on the comment and wrapping side.
Please include some documentation (even the GBM2 snippet from the
cover letter) here. Be that in the summary and/or code.

-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev


Re: [Mesa-dev] [PATCH 1/3] gallium: add renderonly library

2016-12-01 Thread Nicolai Hähnle
Congratulations on a huge amount of work! Obviously I can't say much 
about the driver itself. Some things that I noticed for the renderonly 
library.


On 30.11.2016 14:44, 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.


Some more comments would be _really_ helpful. What is the purpose of a 
"scanout" object? What does for_prime vs. for_resource mean? What does 
intermediate_rendering mean and what is it good for?


The lifecycle of the renderonly object itself looks wrong to me: 
renderonly_screen_create calls the actual driver's screen_create, but 
the actual driver's screen_create frees the renderonly struct? Please 
make it consistent: Either have a proper wrapper or (better, since this 
is so lightweight) have the driver's screen_create call the 
renderonly_screen_create.


Cheers,
Nicolai


Signed-off-by: Christian Gmeiner 
---
 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

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
+
 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)
+
+libgallium_la_SOURCES += \
+   $(RENDERONLY_SOURCES)
+
+endif
+
 if HAVE_MESA_LLVM

 AM_CFLAGS += \
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 000..c4ea784
--- /dev/null
+++ b/src/gallium/auxiliary/renderonly/renderonly.c
@@ -0,0 +1,199 @@
+/*
+ * Copyright (C) 2016 Christian Gmeiner 
+ *
+ * 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 
+ */
+
+#include "renderonly/renderonly.h"
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#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)
+{
+   struct renderonly *ro;
+
+   ro = CALLOC_STRUCT(renderonly);
+   if (!ro)
+  return NULL;
+
+   ro->kms_fd = fd;
+   ro->ops = ops;
+   ro->priv = priv;
+
+   ro->screen = ops->create(ro);
+   if (!ro->screen)
+  goto cleanup;
+
+   return