Re: [Mesa-dev] [PATCH v2 3/3] imx: gallium driver for imx-drm scanout driver

[Christian Gmeiner]

Hi Emil,

thanks for your review!

2017-01-10 14:22 GMT+01:00 Emil Velikov :
> Hi Christian,
>
> Similar to 2/3 there's a few trivial nitpicks which can be addressed
> at a later stage.
>

I fixed them and will squash them.

> On 23 December 2016 at 22:04, Christian Gmeiner
>  wrote:
>> Changes from V1 -> V2:
>>  - updated Copyright
>>  - added $(top_srcdir)/src/gallium/winsys to include path (suggested by Emil)
>>  - adapted driver to new renderonly API
>>
>> Signed-off-by: Christian Gmeiner 
>> ---
>>  configure.ac   | 12 ++
>>  src/gallium/Makefile.am|  4 ++
>>  .../auxiliary/pipe-loader/pipe_loader_drm.c|  5 +++
>>  src/gallium/auxiliary/target-helpers/drm_helper.h  | 24 +++
>>  .../auxiliary/target-helpers/drm_helper_public.h   |  3 ++
>>  src/gallium/drivers/imx/Automake.inc   |  9 
>>  src/gallium/drivers/imx/Makefile.am|  9 
>>  src/gallium/targets/dri/Makefile.am|  1 +
>>  src/gallium/targets/dri/target.c   |  8 
>>  src/gallium/winsys/imx/drm/Makefile.am | 34 +++
>>  src/gallium/winsys/imx/drm/Makefile.sources|  3 ++
>>  src/gallium/winsys/imx/drm/imx_drm_public.h| 34 +++
>>  src/gallium/winsys/imx/drm/imx_drm_winsys.c| 50 
>> ++
>>  13 files changed, 196 insertions(+)
>>  create mode 100644 src/gallium/drivers/imx/Automake.inc
>>  create mode 100644 src/gallium/drivers/imx/Makefile.am
>>  create mode 100644 src/gallium/winsys/imx/drm/Makefile.am
>>  create mode 100644 src/gallium/winsys/imx/drm/Makefile.sources
>>  create mode 100644 src/gallium/winsys/imx/drm/imx_drm_public.h
>>  create mode 100644 src/gallium/winsys/imx/drm/imx_drm_winsys.c
>>
>> diff --git a/configure.ac b/configure.ac
>> index 0b98ce8..59c4064 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -2512,6 +2512,9 @@ if test -n "$with_gallium_drivers"; then
>>  PKG_CHECK_MODULES([ETNAVIV], [libdrm_etnaviv >= 
>> $LIBDRM_ETNAVIV_REQUIRED])
>>  require_libdrm "etnaviv"
>>  ;;
>> +   ximx)
>> +HAVE_GALLIUM_IMX=yes
>> +;;
> Update help string for --with-gallium-drivers.
>

Fixed in separate patch which gets squashed before pushing.

>
>> --- /dev/null
>> +++ b/src/gallium/drivers/imx/Makefile.am
>> @@ -0,0 +1,9 @@
>> +include $(top_srcdir)/src/gallium/Automake.inc
>> +
>> +AM_CPPFLAGS = \
>> +   $(GALLIUM_CFLAGS) \
>> +   $(IMX_CFLAGS)
> IMX_CFLAGS is empty, please remove.
>

Fixed in separate patch which gets squashed before pushing.

>
>> new file mode 100644
>> index 000..d155b2e
>> --- /dev/null
>> +++ b/src/gallium/winsys/imx/drm/Makefile.am
>
>
>> +include Makefile.sources
>> +include $(top_srcdir)/src/gallium/Automake.inc
>> +
>> +AM_CFLAGS = \
>> +   -I$(top_srcdir)/src/gallium/drivers \
>> +   -I$(top_srcdir)/src/gallium/winsys \
>> +   $(GALLIUM_WINSYS_CFLAGS) \
>> +   $(IMX_CFLAGS)
> Ditto.
>

Fixed in separate patch which gets squashed before pushing.

>> +
>> +noinst_LTLIBRARIES = libimxdrm.la
>> +
>> +libimxdrm_la_SOURCES = $(C_SOURCES)
>> \ No newline at end of file
> Add newline ?
>

Fixed in separate patch which gets squashed before pushing.

>> diff --git a/src/gallium/winsys/imx/drm/Makefile.sources 
>> b/src/gallium/winsys/imx/drm/Makefile.sources
>> new file mode 100644
>> index 000..3c0d6fb
>> --- /dev/null
>> +++ b/src/gallium/winsys/imx/drm/Makefile.sources
>> @@ -0,0 +1,3 @@
>> +C_SOURCES := \
>> +   imx_drm_public.h \
>> +   imx_drm_winsys.c
>> \ No newline at end of file
> Ditto.
>

Fixed in separate patch which gets squashed before pushing.

>> --- /dev/null
>> +++ b/src/gallium/winsys/imx/drm/imx_drm_winsys.c
>
>> +struct pipe_screen *imx_drm_screen_create(int fd)
>> +{
>> +   struct renderonly ro = {
>> +  .create_for_resource = renderonly_create_kms_dumb_buffer_for_resource,
>> +  .kms_fd = fd,
>> +  .gpu_fd = open("/dev/dri/renderD128", O_RDWR | O_CLOEXEC)
> As we get Thierry's libdrm work we can polish the heuristics. But this
> will be fine for now.
>

Great!

greets
--
Christian Gmeiner, MSc

https://www.youtube.com/user/AloryOFFICIAL
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 v2 3/3] imx: gallium driver for imx-drm scanout driver

[Emil Velikov]

Hi Christian,

Similar to 2/3 there's a few trivial nitpicks which can be addressed
at a later stage.

On 23 December 2016 at 22:04, Christian Gmeiner
 wrote:
> Changes from V1 -> V2:
>  - updated Copyright
>  - added $(top_srcdir)/src/gallium/winsys to include path (suggested by Emil)
>  - adapted driver to new renderonly API
>
> Signed-off-by: Christian Gmeiner 
> ---
>  configure.ac   | 12 ++
>  src/gallium/Makefile.am|  4 ++
>  .../auxiliary/pipe-loader/pipe_loader_drm.c|  5 +++
>  src/gallium/auxiliary/target-helpers/drm_helper.h  | 24 +++
>  .../auxiliary/target-helpers/drm_helper_public.h   |  3 ++
>  src/gallium/drivers/imx/Automake.inc   |  9 
>  src/gallium/drivers/imx/Makefile.am|  9 
>  src/gallium/targets/dri/Makefile.am|  1 +
>  src/gallium/targets/dri/target.c   |  8 
>  src/gallium/winsys/imx/drm/Makefile.am | 34 +++
>  src/gallium/winsys/imx/drm/Makefile.sources|  3 ++
>  src/gallium/winsys/imx/drm/imx_drm_public.h| 34 +++
>  src/gallium/winsys/imx/drm/imx_drm_winsys.c| 50 
> ++
>  13 files changed, 196 insertions(+)
>  create mode 100644 src/gallium/drivers/imx/Automake.inc
>  create mode 100644 src/gallium/drivers/imx/Makefile.am
>  create mode 100644 src/gallium/winsys/imx/drm/Makefile.am
>  create mode 100644 src/gallium/winsys/imx/drm/Makefile.sources
>  create mode 100644 src/gallium/winsys/imx/drm/imx_drm_public.h
>  create mode 100644 src/gallium/winsys/imx/drm/imx_drm_winsys.c
>
> diff --git a/configure.ac b/configure.ac
> index 0b98ce8..59c4064 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -2512,6 +2512,9 @@ if test -n "$with_gallium_drivers"; then
>  PKG_CHECK_MODULES([ETNAVIV], [libdrm_etnaviv >= 
> $LIBDRM_ETNAVIV_REQUIRED])
>  require_libdrm "etnaviv"
>  ;;
> +   ximx)
> +HAVE_GALLIUM_IMX=yes
> +;;
Update help string for --with-gallium-drivers.


> --- /dev/null
> +++ b/src/gallium/drivers/imx/Makefile.am
> @@ -0,0 +1,9 @@
> +include $(top_srcdir)/src/gallium/Automake.inc
> +
> +AM_CPPFLAGS = \
> +   $(GALLIUM_CFLAGS) \
> +   $(IMX_CFLAGS)
IMX_CFLAGS is empty, please remove.


> new file mode 100644
> index 000..d155b2e
> --- /dev/null
> +++ b/src/gallium/winsys/imx/drm/Makefile.am


> +include Makefile.sources
> +include $(top_srcdir)/src/gallium/Automake.inc
> +
> +AM_CFLAGS = \
> +   -I$(top_srcdir)/src/gallium/drivers \
> +   -I$(top_srcdir)/src/gallium/winsys \
> +   $(GALLIUM_WINSYS_CFLAGS) \
> +   $(IMX_CFLAGS)
Ditto.

> +
> +noinst_LTLIBRARIES = libimxdrm.la
> +
> +libimxdrm_la_SOURCES = $(C_SOURCES)
> \ No newline at end of file
Add newline ?

> diff --git a/src/gallium/winsys/imx/drm/Makefile.sources 
> b/src/gallium/winsys/imx/drm/Makefile.sources
> new file mode 100644
> index 000..3c0d6fb
> --- /dev/null
> +++ b/src/gallium/winsys/imx/drm/Makefile.sources
> @@ -0,0 +1,3 @@
> +C_SOURCES := \
> +   imx_drm_public.h \
> +   imx_drm_winsys.c
> \ No newline at end of file
Ditto.

> --- /dev/null
> +++ b/src/gallium/winsys/imx/drm/imx_drm_winsys.c

> +struct pipe_screen *imx_drm_screen_create(int fd)
> +{
> +   struct renderonly ro = {
> +  .create_for_resource = renderonly_create_kms_dumb_buffer_for_resource,
> +  .kms_fd = fd,
> +  .gpu_fd = open("/dev/dri/renderD128", O_RDWR | O_CLOEXEC)
As we get Thierry's libdrm work we can polish the heuristics. But this
will be fine for now.

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

Re: [Mesa-dev] [PATCH v2 3/3] imx: gallium driver for imx-drm scanout driver

[Emil Velikov]

On 4 January 2017 at 09:10, Thierry Reding  wrote:
> On Fri, Dec 23, 2016 at 11:04:51PM +0100, Christian Gmeiner wrote:
> [...]
>> +struct pipe_screen *imx_drm_screen_create(int fd)
>> +{
>> +   struct renderonly ro = {
>> +  .create_for_resource = renderonly_create_kms_dumb_buffer_for_resource,
>> +  .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();
>> +   if (!screen)
>> +  close(ro.gpu_fd);
>> +
>> +   return screen;
>> +}
>
> So while trying to port Tegra to this renderonly library, I realized
> that we're not going to be able to use this for Tegra after all. It
> should work, and I'm going to go through with the port just for the
> sake of testing, for the specific use-case of hooking up the GPU and
> the scanout device.
>
> However there's some work being done to implement functionality for
> other Tegra units within Mesa, which is going to make this a bit of
> a showstopper.
>
> To be more specific: in the above, the imx-drm driver returns the screen
> associated with the etnaviv render node, rather than a full wrapper for
> the KMS file descriptor. That's effectively what allows you to have this
> work without jumping through extra hoops since it will cause DRI3 and
> Wayland to properly set up the driver for clients.
>
> For Tegra, however, we're going to need a beefier wrapper in order to
> multiplex between the GPU and other units, such as the VIC, NVENC or
> NVDEC.
>
> Emil, my recollection is that you had fairly strong objections to the
> initial proposal of a full-blown wrapper for Tegra. In light of the
> above I don't think we can avoid it forever, though.
>
Re-reading my reply - there seems to be some
confusion/misunderstanding. Perhaps my inline nitpicks diverted the
whole thing.

Ether way:
I was in favour of the approach since it gives us a nice quick
solution, which can be reworked [at a later stage] as better
solution(s) come to mind.

> I guess a good way forward would be to add a very thin driver for Tegra,
> similar to what Christian has done for i.MX and move to a more flexible
> solution later on when work for VIC/NVENC/NVDEC becomes ready. The most
> obvious advantage is that it would get us the long-wanted support fairly
> easily. A disadvantage, albeit a small one, is that the changes we need
> to make to Nouveau will become obsolete.
>
Fully agree with going for this solution for now. One will only need
200-300 loc so nuking those at a later stage should be quite OK.

> Furthermore I think most of the work I did on PRIME support becomes
> unnecessary with the above workaround. Given that Mesa will only ever
> see the Nouveau pipe_screen, at least Weston should work properly out of
> the box. DRI3 might still be problematic according to Alex's earlier
> tests of the Tegra renderonly port. I have a couple more ideas that I'd
> like to try out to see if they can solve this issue.
>
I think that your work on PRIME/libdrm is a requirement for proper
coupling and thus opening the correct kms/render node.
Atm. we're going a bit of a guessing with the hardcoded nodename.

I've not looked at the Wayland code recently so we might want to
double-check that alongside EGL.

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

Re: [Mesa-dev] [PATCH v2 3/3] imx: gallium driver for imx-drm scanout driver

[Thierry Reding]

On Fri, Dec 23, 2016 at 11:04:51PM +0100, Christian Gmeiner wrote:
[...]
> +struct pipe_screen *imx_drm_screen_create(int fd)
> +{
> +   struct renderonly ro = {
> +  .create_for_resource = renderonly_create_kms_dumb_buffer_for_resource,
> +  .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();
> +   if (!screen)
> +  close(ro.gpu_fd);
> +
> +   return screen;
> +}

So while trying to port Tegra to this renderonly library, I realized
that we're not going to be able to use this for Tegra after all. It
should work, and I'm going to go through with the port just for the
sake of testing, for the specific use-case of hooking up the GPU and
the scanout device.

However there's some work being done to implement functionality for
other Tegra units within Mesa, which is going to make this a bit of
a showstopper.

To be more specific: in the above, the imx-drm driver returns the screen
associated with the etnaviv render node, rather than a full wrapper for
the KMS file descriptor. That's effectively what allows you to have this
work without jumping through extra hoops since it will cause DRI3 and
Wayland to properly set up the driver for clients.

For Tegra, however, we're going to need a beefier wrapper in order to
multiplex between the GPU and other units, such as the VIC, NVENC or
NVDEC.

Emil, my recollection is that you had fairly strong objections to the
initial proposal of a full-blown wrapper for Tegra. In light of the
above I don't think we can avoid it forever, though.

I guess a good way forward would be to add a very thin driver for Tegra,
similar to what Christian has done for i.MX and move to a more flexible
solution later on when work for VIC/NVENC/NVDEC becomes ready. The most
obvious advantage is that it would get us the long-wanted support fairly
easily. A disadvantage, albeit a small one, is that the changes we need
to make to Nouveau will become obsolete.

Furthermore I think most of the work I did on PRIME support becomes
unnecessary with the above workaround. Given that Mesa will only ever
see the Nouveau pipe_screen, at least Weston should work properly out of
the box. DRI3 might still be problematic according to Alex's earlier
tests of the Tegra renderonly port. I have a couple more ideas that I'd
like to try out to see if they can solve this issue.

Thierry


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev