[Mesa-dev] [RFC] freedreno: import libdrm_freedreno + redesign submit

Eric Engestrom eric.engestrom at intel.com
Thu Oct 25 09:32:08 UTC 2018 

On Tuesday, 2018-10-23 10:49:26 -0400, mesa-dev-bounces at lists.freedesktop.org wrote:
> In the pursuit of lowering driver overhead, it became clear that some
> amount of redesign of how libdrm_freedreno constructs the submit ioctl
> would be needed.  In particular, as the gallium driver is starting to
> make heavier use of CP_SET_DRAW_STATE state groups/objects, the over-
> head of tracking cmd buffers and relocs becomes too much.  And for
> "streaming" state, which isn't ever reused (like uniform uploads) the
> overhead of allocating/freeing ringbuffer[1] objects is too high.
> 
> This redesign makes two main changes:
> 
>  1) Introduces a fd_submit object for tracking bos and cmds table
>     for the submit ioctl, making ringbuffer objects more light-
>     weight.  This was previously done in the ringbuffer.  But we
>     have many ringbuffer instances involved in a submit (gmem +
>     draw + potentially 1000's of state-group rbs), and only need
>     a single bos and cmds table.  (Reloc table is still per-rb)
> 
>     The submit is also a convenient place for a slab allocator for
>     ringbuffer objects.  Other options would have required locking
>     because, while we can guarantee allocations will only happen on
>     a single thread, free's could happen either on the application
>     thread or the flush_queue thread.  With the slab allocator in
>     the submit object, any frees that happen on the flush_queue
>     thread happen after we know that the application thread is done
>     with the submit.
> 
>  2) Introduce a new "softpin" msm_ringbuffer_sp implementation that
>     does not use relocs and only has cmds table entries for IB1 (ie.
>     the cmdstream buffers that kernel needs to CP_INDIRECT_BUFFER
>     to from the RB).  To do this properly will require some updates
>     on the kernel side, so whether you get the softpin or legacy
>     submit/ringbuffer implementation at runtime depends on your
>     kernel version.
> 
> To make all these changes in libdrm would basically require adding a
> libdrm_freedreno2, so this is a good point to just pull the libdrm code
> into mesa.  Plus it allows for using mesa's hashtable, slab allocator,
> etc.  And it lets us have asserts enabled for debug mesa buids but
> omitted for release builds.  And it makes life easier if further API
> changes become necessary.
> 
> At this point I haven't tried to pull in the kgsl backend.  Although
> I left the level of vfunc indirection which would make it possible
> to have other backends.  (And this was convenient to keep to allow
> for the "softpin" ringbuffer to coexist.)
> 
> NOTE: if bisecting a build error takes you hear, try a clean build.
> There are a bunch of ways things can go wrong if you still have
> libdrm_freedreno cflags.

Good note!
(and s/hear/here/)

> 
> [1] "ringbuffer" is probably a bad name, the only level of cmdstream
>     buffer that is actually a ring is RB managed by kernel.  User-
>     space cmdstream is all IB1/IB2 and state-groups.
> 
> Signed-off-by: Rob Clark <robdclark at gmail.com>
> ---
> Note one "benchmark" that I was at while working on this is webgl
> aquarium.  It isn't a terribly clever gl app, doing one draw per
> fish, uploading uniforms (and usually not making any other state
> changes) between each draw.  The "softpin" implementation drops
> CPU utilization by about 20%, and once you get CPU limited, it is
> worth ~50% fps boost.
> 
> Also note, android build probably needs some attention (mostly
> replacing libdrm_freedreno dependency with libdrm + valgrind).

Might be best to do that in the same commit, to avoid breaking bisects.

> 
>  configure.ac                                  |   2 -
>  meson.build                                   |   2 -
>  src/gallium/drivers/freedreno/Makefile.am     |   7 +-
>  .../drivers/freedreno/Makefile.sources        |  17 +
>  .../drivers/freedreno/a3xx/fd3_context.h      |   2 -
>  .../drivers/freedreno/a4xx/fd4_context.h      |   2 -
>  .../drivers/freedreno/a5xx/fd5_context.h      |   2 -
>  src/gallium/drivers/freedreno/a5xx/fd5_draw.c |   3 +-
>  .../drivers/freedreno/a6xx/fd6_context.h      |   2 -
>  src/gallium/drivers/freedreno/a6xx/fd6_draw.c |   3 +-
>  src/gallium/drivers/freedreno/a6xx/fd6_emit.c |  26 +-
>  .../drivers/freedreno/drm/freedreno_bo.c      | 361 +++++++++
>  .../freedreno/drm/freedreno_bo_cache.c        | 218 ++++++
>  .../drivers/freedreno/drm/freedreno_device.c  | 156 ++++
>  .../drivers/freedreno/drm/freedreno_drmif.h   | 126 +++
>  .../drivers/freedreno/drm/freedreno_pipe.c    | 100 +++
>  .../drivers/freedreno/drm/freedreno_priv.h    | 258 +++++++
>  .../freedreno/drm/freedreno_ringbuffer.c      | 114 +++
>  .../freedreno/drm/freedreno_ringbuffer.h      | 159 ++++
>  src/gallium/drivers/freedreno/drm/msm_bo.c    | 170 +++++
>  .../drivers/freedreno/drm/msm_device.c        |  61 ++
>  src/gallium/drivers/freedreno/drm/msm_drm.h   | 308 ++++++++
>  src/gallium/drivers/freedreno/drm/msm_pipe.c  | 223 ++++++
>  src/gallium/drivers/freedreno/drm/msm_priv.h  | 140 ++++
>  .../drivers/freedreno/drm/msm_ringbuffer.c    | 721 ++++++++++++++++++
>  .../drivers/freedreno/drm/msm_ringbuffer_sp.c | 551 +++++++++++++
>  .../drivers/freedreno/freedreno_batch.c       |  25 +-
>  .../drivers/freedreno/freedreno_batch.h       |   2 +
>  .../drivers/freedreno/freedreno_gmem.c        |   8 +-
>  .../drivers/freedreno/freedreno_screen.c      |   1 +
>  .../drivers/freedreno/freedreno_screen.h      |   4 +-
>  .../drivers/freedreno/freedreno_util.h        |  30 +-
>  .../drivers/freedreno/ir3/ir3_shader.c        |  11 +-
>  src/gallium/drivers/freedreno/meson.build     |  25 +-
>  src/gallium/winsys/freedreno/drm/meson.build  |   2 +-
>  35 files changed, 3765 insertions(+), 77 deletions(-)
>  create mode 100644 src/gallium/drivers/freedreno/drm/freedreno_bo.c
>  create mode 100644 src/gallium/drivers/freedreno/drm/freedreno_bo_cache.c
>  create mode 100644 src/gallium/drivers/freedreno/drm/freedreno_device.c
>  create mode 100644 src/gallium/drivers/freedreno/drm/freedreno_drmif.h
>  create mode 100644 src/gallium/drivers/freedreno/drm/freedreno_pipe.c
>  create mode 100644 src/gallium/drivers/freedreno/drm/freedreno_priv.h
>  create mode 100644 src/gallium/drivers/freedreno/drm/freedreno_ringbuffer.c
>  create mode 100644 src/gallium/drivers/freedreno/drm/freedreno_ringbuffer.h
>  create mode 100644 src/gallium/drivers/freedreno/drm/msm_bo.c
>  create mode 100644 src/gallium/drivers/freedreno/drm/msm_device.c
>  create mode 100644 src/gallium/drivers/freedreno/drm/msm_drm.h
>  create mode 100644 src/gallium/drivers/freedreno/drm/msm_pipe.c
>  create mode 100644 src/gallium/drivers/freedreno/drm/msm_priv.h
>  create mode 100644 src/gallium/drivers/freedreno/drm/msm_ringbuffer.c
>  create mode 100644 src/gallium/drivers/freedreno/drm/msm_ringbuffer_sp.c
> 
> diff --git a/configure.ac b/configure.ac
> index ab9bdce885f..15ac8b0d1b9 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -78,7 +78,6 @@ LIBDRM_AMDGPU_REQUIRED=2.4.93
>  LIBDRM_INTEL_REQUIRED=2.4.75
>  LIBDRM_NVVIEUX_REQUIRED=2.4.66
>  LIBDRM_NOUVEAU_REQUIRED=2.4.66
> -LIBDRM_FREEDRENO_REQUIRED=2.4.96
>  LIBDRM_ETNAVIV_REQUIRED=2.4.89
>  LIBDRM_VC4_REQUIRED=2.4.89
>  
> @@ -2722,7 +2721,6 @@ if test -n "$with_gallium_drivers"; then
>              ;;
>          xfreedreno)
>              HAVE_GALLIUM_FREEDRENO=yes
> -            PKG_CHECK_MODULES([FREEDRENO], [libdrm >= $LIBDRM_FREEDRENO_REQUIRED libdrm_freedreno >= $LIBDRM_FREEDRENO_REQUIRED])
>              require_libdrm "freedreno"
>              ;;
>          xetnaviv)
> diff --git a/meson.build b/meson.build
> index 505cc6c79bd..d483c61f5c9 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1112,7 +1112,6 @@ _drm_amdgpu_ver = '2.4.93'

Just above this:
----8<----
diff --git a/meson.build b/meson.build
index 6d274d9f4a404c29a8c8..4640a04ddbd899e42a69 100644
--- a/meson.build
+++ b/meson.build
@@ -1098,7 +1098,6 @@ dep_libdrm_amdgpu = null_dep
 dep_libdrm_radeon = null_dep
 dep_libdrm_nouveau = null_dep
 dep_libdrm_etnaviv = null_dep
-dep_libdrm_freedreno = null_dep
 dep_libdrm_intel = null_dep
 
 _drm_amdgpu_ver = '2.4.95'
---->8----

With that, the meson side of this patch is:
Reviewed-by: Eric Engestrom <eric.engestrom at intel.com>

>  _drm_radeon_ver = '2.4.71'
>  _drm_nouveau_ver = '2.4.66'
>  _drm_etnaviv_ver = '2.4.89'
> -_drm_freedreno_ver = '2.4.96'
>  _drm_intel_ver = '2.4.75'
>  _drm_ver = '2.4.75'
>  
> @@ -1123,7 +1122,6 @@ _libdrm_checks = [
>                with_gallium_r300 or with_gallium_r600)],
>    ['nouveau', (with_gallium_nouveau or with_dri_nouveau)],
>    ['etnaviv', with_gallium_etnaviv],
> -  ['freedreno', with_gallium_freedreno],
>  ]
>  
>  # VC4 only needs core libdrm support of this version, not a libdrm_vc4
[snip]
> diff --git a/src/gallium/drivers/freedreno/meson.build b/src/gallium/drivers/freedreno/meson.build
> index 9272602e547..d26aa0942e6 100644
> --- a/src/gallium/drivers/freedreno/meson.build
> +++ b/src/gallium/drivers/freedreno/meson.build
> @@ -71,6 +71,21 @@ files_libfreedreno = files(
>    'freedreno_texture.h',
>    'freedreno_util.c',
>    'freedreno_util.h',
> +  'drm/freedreno_bo.c',
> +  'drm/freedreno_bo_cache.c',
> +  'drm/freedreno_device.c',
> +  'drm/freedreno_drmif.h',
> +  'drm/freedreno_pipe.c',
> +  'drm/freedreno_priv.h',
> +  'drm/freedreno_ringbuffer.c',
> +  'drm/freedreno_ringbuffer.h',
> +  'drm/msm_bo.c',
> +  'drm/msm_device.c',
> +  'drm/msm_drm.h',
> +  'drm/msm_pipe.c',
> +  'drm/msm_priv.h',
> +  'drm/msm_ringbuffer.c',
> +  'drm/msm_ringbuffer_sp.c',
>    'a2xx/a2xx.xml.h',
>    'a2xx/disasm-a2xx.c',
>    'a2xx/fd2_blend.c',
> @@ -259,7 +274,12 @@ libfreedreno = static_library(
>    include_directories : freedreno_includes,
>    c_args : [freedreno_c_args, c_vis_args],
>    cpp_args : [freedreno_cpp_args, cpp_vis_args],
> -  dependencies : [dep_libdrm, dep_libdrm_freedreno, idep_nir_headers],
> +  dependencies : [
> +    dep_libdrm,
> +    dep_libdrm,

Harmless, but duplicate `dep_libdrm` here and in the next hunk below.

> +    dep_valgrind,
> +    idep_nir_headers
> +  ],
>  )
>  
>  driver_freedreno = declare_dependency(
> @@ -274,7 +294,8 @@ ir3_compiler = executable(
>    include_directories : freedreno_includes,
>    dependencies : [
>      dep_libdrm,
> -    dep_libdrm_freedreno,
> +    dep_libdrm,
> +    dep_valgrind,
>      dep_thread,
>      idep_nir,
>    ],
> diff --git a/src/gallium/winsys/freedreno/drm/meson.build b/src/gallium/winsys/freedreno/drm/meson.build
> index 34aff635dde..0fc02897ddd 100644
> --- a/src/gallium/winsys/freedreno/drm/meson.build
> +++ b/src/gallium/winsys/freedreno/drm/meson.build
> @@ -25,5 +25,5 @@ libfreedrenowinsys = static_library(
>      inc_src, inc_include, inc_gallium, inc_gallium_aux, inc_gallium_drivers,
>    ],
>    c_args : [c_vis_args],
> -  dependencies : [dep_libdrm_freedreno],
> +  dependencies : [dep_libdrm],
>  )
> -- 
> 2.17.2
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev

More information about the mesa-dev mailing list