[Mesa-dev] [PATCH 3/2] r600g, compute: provide local copy of ac_binary.{h, c}

Jan Vesely jan.vesely at rutgers.edu
Fri Jun 9 19:16:08 UTC 2017


On Fri, 2017-06-09 at 13:44 -0500, Aaron Watry wrote:
> On Fri, Jun 9, 2017 at 12:36 PM, Jan Vesely <jan.vesely at rutgers.edu> wrote:
> > On Fri, 2017-06-09 at 10:12 -0500, Aaron Watry wrote:
> > > On Fri, Jun 9, 2017 at 8:20 AM, Jan Vesely <jan.vesely at rutgers.edu> wrote:
> > > > This is a verbatim copy of the code. The functions can be cleaned up since
> > > > r600 does not use all the stuff that gcn does.
> > > > The symbol names have been changed since we still use ac_binary.h header
> > > > (for struct definition)
> > > > 
> > > > Signed-off-by: Jan Vesely <jan.vesely at rutgers.edu>
> > > > ---
> > > > Emil, Aaron,
> > > > 
> > > > this is the last patch to get rid of libamd_common dependency (and thus libdrm_amdgpu). I have only remote access to the machine atm, so it's compile tested only.
> > > > 
> > > > Jan
> > > > 
> > > >  configure.ac                                       |   5 +-
> > > >  src/gallium/drivers/r600/Automake.inc              |  10 +-
> > > >  src/gallium/drivers/r600/Makefile.am               |   2 +
> > > >  src/gallium/drivers/r600/evergreen_compute.c       | 197 ++++++++++++++++++++-
> > > >  .../drivers/r600/evergreen_compute_internal.h      |   2 +-
> > > >  src/gallium/drivers/radeon/r600_pipe_common.c      |  21 ---
> > > >  src/gallium/drivers/radeon/r600_pipe_common.h      |   5 -
> > > >  src/gallium/targets/pipe-loader/Makefile.am        |  10 +-
> > > >  8 files changed, 200 insertions(+), 52 deletions(-)
> > > > 
> > > > diff --git a/configure.ac b/configure.ac
> > > > index 9433e3c..fc4a58f 100644
> > > > --- a/configure.ac
> > > > +++ b/configure.ac
> > > > @@ -2631,10 +2631,7 @@ AM_CONDITIONAL(HAVE_SWRAST_DRI, test x$HAVE_SWRAST_DRI = xyes)
> > > >  AM_CONDITIONAL(HAVE_RADEON_VULKAN, test "x$HAVE_RADEON_VULKAN" = xyes)
> > > >  AM_CONDITIONAL(HAVE_INTEL_VULKAN, test "x$HAVE_INTEL_VULKAN" = xyes)
> > > > 
> > > > -# FIXME: r600g still depends and amd_common (ac_binary*) when building OpenCL
> > > > -AM_CONDITIONAL(HAVE_AMD_DRIVERS, test \( "x$HAVE_GALLIUM_R600" = xyes -a \
> > > > -                                      "x$enable_opencl" = xyes \) -o \
> > > > -                                      "x$HAVE_GALLIUM_RADEONSI" = xyes -o \
> > > > +AM_CONDITIONAL(HAVE_AMD_DRIVERS, test "x$HAVE_GALLIUM_RADEONSI" = xyes -o \
> > > >                                        "x$HAVE_RADEON_VULKAN" = xyes)
> > > > 
> > > >  AM_CONDITIONAL(HAVE_INTEL_DRIVERS, test "x$HAVE_INTEL_VULKAN" = xyes -o \
> > > > diff --git a/src/gallium/drivers/r600/Automake.inc b/src/gallium/drivers/r600/Automake.inc
> > > > index 642d527..bb9f6ec 100644
> > > > --- a/src/gallium/drivers/r600/Automake.inc
> > > > +++ b/src/gallium/drivers/r600/Automake.inc
> > > > @@ -5,18 +5,12 @@ TARGET_CPPFLAGS += -DGALLIUM_R600
> > > >  TARGET_LIB_DEPS += \
> > > >         $(top_builddir)/src/gallium/drivers/r600/libr600.la \
> > > >         $(RADEON_LIBS) \
> > > > -       $(LIBDRM_LIBS)
> > > > +       $(LIBDRM_LIBS) \
> > > > +       $(LIBELF_LIBS)
> > > > 
> > > >  TARGET_RADEON_WINSYS = \
> > > >         $(top_builddir)/src/gallium/winsys/radeon/drm/libradeonwinsys.la
> > > > 
> > > >  TARGET_RADEON_COMMON = \
> > > >         $(top_builddir)/src/gallium/drivers/radeon/libradeon.la
> > > > -
> > > > -# TODO: drop this dependency. libamd_common requires libdrm_amdgpu.
> > > > -if HAVE_AMD_DRIVERS
> > > > -TARGET_RADEON_COMMON += \
> > > > -       $(top_builddir)/src/amd/common/libamd_common.la
> > > > -endif
> > > > -
> > > >  endif
> > > > diff --git a/src/gallium/drivers/r600/Makefile.am b/src/gallium/drivers/r600/Makefile.am
> > > > index 44fd51d..fbfb6e6 100644
> > > > --- a/src/gallium/drivers/r600/Makefile.am
> > > > +++ b/src/gallium/drivers/r600/Makefile.am
> > > > @@ -9,11 +9,13 @@ BUILT_SOURCES = $(R600_GENERATED_FILES)
> > > >  AM_CFLAGS = \
> > > >         $(GALLIUM_DRIVER_CFLAGS) \
> > > >         $(RADEON_CFLAGS) \
> > > > +       $(LIBELF_CFLAGS) \
> > > >         -I$(top_srcdir)/src/amd/common
> > > > 
> > > >  AM_CXXFLAGS = \
> > > >         $(GALLIUM_DRIVER_CXXFLAGS) \
> > > >         $(RADEON_CFLAGS) \
> > > > +       $(LIBELF_CFLAGS) \
> > > >         -I$(top_srcdir)/src/amd/common
> > > > 
> > > >  noinst_LTLIBRARIES = libr600.la
> > > > diff --git a/src/gallium/drivers/r600/evergreen_compute.c b/src/gallium/drivers/r600/evergreen_compute.c
> > > > index d30024d..69a6d8b 100644
> > > > --- a/src/gallium/drivers/r600/evergreen_compute.c
> > > > +++ b/src/gallium/drivers/r600/evergreen_compute.c
> > > > @@ -24,9 +24,10 @@
> > > >   *      Adam Rak <adam.rak at streamnovation.com>
> > > >   */
> > > > 
> > > > +#include <gelf.h>
> > > > +#include <libelf.h>
> > > >  #include <stdio.h>
> > > >  #include <errno.h>
> > > > -#include "ac_binary.h"
> > > >  #include "pipe/p_defines.h"
> > > >  #include "pipe/p_state.h"
> > > >  #include "pipe/p_context.h"
> > > > @@ -179,6 +180,192 @@ static void evergreen_cs_set_constant_buffer(struct r600_context *rctx,
> > > >  #define R_028850_SQ_PGM_RESOURCES_PS                 0x028850
> > > > 
> > > >  #ifdef HAVE_OPENCL
> > > > +/*
> > > > + * shader binary helpers.
> > > > + */
> > > > +static void r600_shader_binary_init(struct ac_shader_binary *b)
> > > > +{
> > > > +       memset(b, 0, sizeof(*b));
> > > > +}
> > > > +
> > > > +static void r600_shader_binary_clean(struct ac_shader_binary *b)
> > > > +{
> > > > +       if (!b)
> > > > +               return;
> > > > +       FREE(b->code);
> > > > +       FREE(b->config);
> > > > +       FREE(b->rodata);
> > > > +       FREE(b->global_symbol_offsets);
> > > > +       FREE(b->relocs);
> > > > +       FREE(b->disasm_string);
> > > > +       FREE(b->llvm_ir_string);
> > > > +}
> > > > +
> > > > +static void parse_symbol_table(Elf_Data *symbol_table_data,
> > > > +                               const GElf_Shdr *symbol_table_header,
> > > > +                               struct ac_shader_binary *binary)
> > > > +{
> > > > +       GElf_Sym symbol;
> > > > +       unsigned i = 0;
> > > > +       unsigned symbol_count =
> > > > +               symbol_table_header->sh_size / symbol_table_header->sh_entsize;
> > > > +
> > > > +       /* We are over allocating this list, because symbol_count gives the
> > > > +        * total number of symbols, and we will only be filling the list
> > > > +        * with offsets of global symbols.  The memory savings from
> > > > +        * allocating the correct size of this list will be small, and
> > > > +        * I don't think it is worth the cost of pre-computing the number
> > > > +        * of global symbols.
> > > > +        */
> > > > +       binary->global_symbol_offsets = CALLOC(symbol_count, sizeof(uint64_t));
> > > > +
> > > > +       while (gelf_getsym(symbol_table_data, i++, &symbol)) {
> > > > +               unsigned i;
> > > > +               if (GELF_ST_BIND(symbol.st_info) != STB_GLOBAL ||
> > > > +                   symbol.st_shndx == 0 /* Undefined symbol */) {
> > > > +                       continue;
> > > > +               }
> > > > +
> > > > +               binary->global_symbol_offsets[binary->global_symbol_count] =
> > > > +                                       symbol.st_value;
> > > > +
> > > > +               /* Sort the list using bubble sort.  This list will usually
> > > > +                * be small. */
> > > > +               for (i = binary->global_symbol_count; i > 0; --i) {
> > > > +                       uint64_t lhs = binary->global_symbol_offsets[i - 1];
> > > > +                       uint64_t rhs = binary->global_symbol_offsets[i];
> > > > +                       if (lhs < rhs) {
> > > > +                               break;
> > > > +                       }
> > > > +                       binary->global_symbol_offsets[i] = lhs;
> > > > +                       binary->global_symbol_offsets[i - 1] = rhs;
> > > > +               }
> > > > +               ++binary->global_symbol_count;
> > > > +       }
> > > > +}
> > > > +
> > > > +
> > > > +static void parse_relocs(Elf *elf, Elf_Data *relocs, Elf_Data *symbols,
> > > > +                       unsigned symbol_sh_link,
> > > > +                       struct ac_shader_binary *binary)
> > > > +{
> > > > +       unsigned i;
> > > > +
> > > > +       if (!relocs || !symbols || !binary->reloc_count) {
> > > > +               return;
> > > > +       }
> > > > +       binary->relocs = CALLOC(binary->reloc_count,
> > > > +                       sizeof(struct ac_shader_reloc));
> > > > +       for (i = 0; i < binary->reloc_count; i++) {
> > > > +               GElf_Sym symbol;
> > > > +               GElf_Rel rel;
> > > > +               char *symbol_name;
> > > > +               struct ac_shader_reloc *reloc = &binary->relocs[i];
> > > > +
> > > > +               gelf_getrel(relocs, i, &rel);
> > > > +               gelf_getsym(symbols, GELF_R_SYM(rel.r_info), &symbol);
> > > > +               symbol_name = elf_strptr(elf, symbol_sh_link, symbol.st_name);
> > > > +
> > > > +               reloc->offset = rel.r_offset;
> > > > +               strncpy(reloc->name, symbol_name, sizeof(reloc->name)-1);
> > > > +               reloc->name[sizeof(reloc->name)-1] = 0;
> > > > +       }
> > > > +}
> > > > +
> > > > +//ac_elf_read
> > > > +
> > > > +static void r600_elf_read(const char *elf_data, unsigned elf_size,
> > > > +                struct ac_shader_binary *binary)
> > > > +{
> > > > +       char *elf_buffer;
> > > > +       Elf *elf;
> > > > +       Elf_Scn *section = NULL;
> > > > +       Elf_Data *symbols = NULL, *relocs = NULL;
> > > > +       size_t section_str_index;
> > > > +       unsigned symbol_sh_link = 0;
> > > > +
> > > > +       /* One of the libelf implementations
> > > > +        * (http://www.mr511.de/software/english.htm) requires calling
> > > > +        * elf_version() before elf_memory().
> > > > +        */
> > > > +       elf_version(EV_CURRENT);
> > > > +       elf_buffer = MALLOC(elf_size);
> > > > +       memcpy(elf_buffer, elf_data, elf_size);
> > > > +
> > > > +       elf = elf_memory(elf_buffer, elf_size);
> > > > +
> > > > +       elf_getshdrstrndx(elf, &section_str_index);
> > > > +
> > > > +       while ((section = elf_nextscn(elf, section))) {
> > > > +               const char *name;
> > > > +               Elf_Data *section_data = NULL;
> > > > +               GElf_Shdr section_header;
> > > > +               if (gelf_getshdr(section, &section_header) != &section_header) {
> > > > +                       fprintf(stderr, "Failed to read ELF section header\n");
> > > > +                       return;
> > > > +               }
> > > > +               name = elf_strptr(elf, section_str_index, section_header.sh_name);
> > > > +               if (!strcmp(name, ".text")) {
> > > > +                       section_data = elf_getdata(section, section_data);
> > > > +                       binary->code_size = section_data->d_size;
> > > > +                       binary->code = MALLOC(binary->code_size * sizeof(unsigned char));
> > > > +                       memcpy(binary->code, section_data->d_buf, binary->code_size);
> > > > +               } else if (!strcmp(name, ".AMDGPU.config")) {
> > > > +                       section_data = elf_getdata(section, section_data);
> > > > +                       binary->config_size = section_data->d_size;
> > > > +                       binary->config = MALLOC(binary->config_size * sizeof(unsigned char));
> > > > +                       memcpy(binary->config, section_data->d_buf, binary->config_size);
> > > > +               } else if (!strcmp(name, ".AMDGPU.disasm")) {
> > > > +                       /* Always read disassembly if it's available. */
> > > > +                       section_data = elf_getdata(section, section_data);
> > > > +                       binary->disasm_string = strndup(section_data->d_buf,
> > > > +                                                       section_data->d_size);
> > > > +               } else if (!strncmp(name, ".rodata", 7)) {
> > > > +                       section_data = elf_getdata(section, section_data);
> > > > +                       binary->rodata_size = section_data->d_size;
> > > > +                       binary->rodata = MALLOC(binary->rodata_size * sizeof(unsigned char));
> > > > +                       memcpy(binary->rodata, section_data->d_buf, binary->rodata_size);
> > > > +               } else if (!strncmp(name, ".symtab", 7)) {
> > > > +                       symbols = elf_getdata(section, section_data);
> > > > +                       symbol_sh_link = section_header.sh_link;
> > > > +                       parse_symbol_table(symbols, &section_header, binary);
> > > > +               } else if (!strcmp(name, ".rel.text")) {
> > > > +                       relocs = elf_getdata(section, section_data);
> > > > +                       binary->reloc_count = section_header.sh_size /
> > > > +                                       section_header.sh_entsize;
> > > > +               }
> > > > +       }
> > > > +
> > > > +       parse_relocs(elf, relocs, symbols, symbol_sh_link, binary);
> > > > +
> > > > +       if (elf){
> > > > +               elf_end(elf);
> > > > +       }
> > > > +       FREE(elf_buffer);
> > > > +
> > > > +       /* Cache the config size per symbol */
> > > > +       if (binary->global_symbol_count) {
> > > > +               binary->config_size_per_symbol =
> > > > +                       binary->config_size / binary->global_symbol_count;
> > > > +       } else {
> > > > +               binary->global_symbol_count = 1;
> > > > +               binary->config_size_per_symbol = binary->config_size;
> > > > +       }
> > > > +}
> > > > +
> > > > +static const unsigned char *r600_shader_binary_config_start(
> > > > +       const struct ac_shader_binary *binary,
> > > > +       uint64_t symbol_offset)
> > > > +{
> > > > +       unsigned i;
> > > > +       for (i = 0; i < binary->global_symbol_count; ++i) {
> > > > +               if (binary->global_symbol_offsets[i] == symbol_offset) {
> > > > +                       unsigned offset = i * binary->config_size_per_symbol;
> > > > +                       return binary->config + offset;
> > > > +               }
> > > > +       }
> > > > +       return binary->config;
> > > > +}
> > > > 
> > > >  static void r600_shader_binary_read_config(const struct ac_shader_binary *binary,
> > > >                                            struct r600_bytecode *bc,
> > > > @@ -187,7 +374,7 @@ static void r600_shader_binary_read_config(const struct ac_shader_binary *binary
> > > >  {
> > > >         unsigned i;
> > > >         const unsigned char *config =
> > > > -               ac_shader_binary_config_start(binary, symbol_offset);
> > > > +               r600_shader_binary_config_start(binary, symbol_offset);
> > > > 
> > > >         for (i = 0; i < binary->config_size_per_symbol; i+= 8) {
> > > >                 unsigned reg =
> > > > @@ -250,8 +437,8 @@ static void *evergreen_create_compute_state(struct pipe_context *ctx,
> > > >         COMPUTE_DBG(rctx->screen, "*** evergreen_create_compute_state\n");
> > > >         header = cso->prog;
> > > >         code = cso->prog + sizeof(struct pipe_llvm_program_header);
> > > > -       radeon_shader_binary_init(&shader->binary);
> > > > -       ac_elf_read(code, header->num_bytes, &shader->binary);
> > > > +       r600_shader_binary_init(&shader->binary);
> > > > +       r600_elf_read(code, header->num_bytes, &shader->binary);
> > > >         r600_create_shader(&shader->bc, &shader->binary, &use_kill);
> > > > 
> > > >         /* Upload code + ROdata */
> > > > @@ -281,7 +468,7 @@ static void evergreen_delete_compute_state(struct pipe_context *ctx, void *state
> > > >         if (!shader)
> > > >                 return;
> > > > 
> > > > -       radeon_shader_binary_clean(&shader->binary);
> > > > +       r600_shader_binary_clean(&shader->binary);
> > > 
> > > This call to r600_shader_binary_clean needs an #ifdef HAVE_OPENCL
> > > guard around it.
> > > 
> > > That being said, we still have a dependency on amdgpu.h in
> > > amd/common/ac_gpu_info.h here, which breaks the build if the amdgpu.h
> > > header isn't installed. Emil's patch 4/5 fixes that issue, so I think
> > > we'll have to include that somewhere in this series when we commit it.
> > 
> > right, thanks for clarification. Yours or Emil's patches address that
> > so I did not include them. I don't have a problem if it's two different
> > series.
> > 
> > > 
> > > With the aforementioned HAVE_OPENCL guard and Emil's 4/5, I've got
> > > the following builds results on my BARTS/6850 system:
> > > 
> > > r600g, no CL, no amdgpu.h: Succeeds
> > > r600g, with CL, no amdgpu.h: Succeeds, clinfo works
> > > r600g, no CL, with amdgpu.h: Succeeds
> > > r600g, with CL, with amdgpu.h: Succeeds, clinfo works
> > > 
> > > With that #ifdef change from above added, this is:
> > > Tested-By: Aaron Watry <awatry at gmail.com>
> > 
> > fixed in v2. It might be worth testing at least one piglit, since
> > clinfo does not hit dispatch codepaths.
> 
> I retested both CL builds (with/without amdgpu.h present), and both
> work fine when I run:
> $ bin/cl-program-tester tests/cl/program/execute/builtin/math/tan.cl

thanks, I think one test is enough. if I broke something it would have
showed up early.

> So I think we're ok here. I can run a full piglit run if we feel it's
> necessary, but it wouldn't be until Sunday/Monday, as I'm headed out
> of town for the weekend.

there's no need. I'll try to add the patches into my daily regression
run over the weekend.

regards,
Jan

> 
> --Aaron
> 
> 
> > 
> > thanks,
> > Jan
> > 
> > > 
> > > --Aaron
> > > 
> > > >         r600_destroy_shader(&shader->bc);
> > > > 
> > > >         /* TODO destroy shader->code_bo, shader->const_bo
> > > > diff --git a/src/gallium/drivers/r600/evergreen_compute_internal.h b/src/gallium/drivers/r600/evergreen_compute_internal.h
> > > > index 6f4be3e..32f53ad 100644
> > > > --- a/src/gallium/drivers/r600/evergreen_compute_internal.h
> > > > +++ b/src/gallium/drivers/r600/evergreen_compute_internal.h
> > > > @@ -21,10 +21,10 @@
> > > >   * Authors:
> > > >   *      Adam Rak <adam.rak at streamnovation.com>
> > > >   */
> > > > -
> > > >  #ifndef EVERGREEN_COMPUTE_INTERNAL_H
> > > >  #define EVERGREEN_COMPUTE_INTERNAL_H
> > > > 
> > > > +#include "ac_binary.h"
> > > >  #include "r600_asm.h"
> > > >  #ifdef HAVE_OPENCL
> > > >  #include <llvm-c/Core.h>
> > > > diff --git a/src/gallium/drivers/radeon/r600_pipe_common.c b/src/gallium/drivers/radeon/r600_pipe_common.c
> > > > index 48d136a..3939623 100644
> > > > --- a/src/gallium/drivers/radeon/r600_pipe_common.c
> > > > +++ b/src/gallium/drivers/radeon/r600_pipe_common.c
> > > > @@ -64,27 +64,6 @@ struct r600_multi_fence {
> > > >  };
> > > > 
> > > >  /*
> > > > - * shader binary helpers.
> > > > - */
> > > > -void radeon_shader_binary_init(struct ac_shader_binary *b)
> > > > -{
> > > > -       memset(b, 0, sizeof(*b));
> > > > -}
> > > > -
> > > > -void radeon_shader_binary_clean(struct ac_shader_binary *b)
> > > > -{
> > > > -       if (!b)
> > > > -               return;
> > > > -       FREE(b->code);
> > > > -       FREE(b->config);
> > > > -       FREE(b->rodata);
> > > > -       FREE(b->global_symbol_offsets);
> > > > -       FREE(b->relocs);
> > > > -       FREE(b->disasm_string);
> > > > -       FREE(b->llvm_ir_string);
> > > > -}
> > > > -
> > > > -/*
> > > >   * pipe_context
> > > >   */
> > > > 
> > > > diff --git a/src/gallium/drivers/radeon/r600_pipe_common.h b/src/gallium/drivers/radeon/r600_pipe_common.h
> > > > index 84d38fb..2d2a05f 100644
> > > > --- a/src/gallium/drivers/radeon/r600_pipe_common.h
> > > > +++ b/src/gallium/drivers/radeon/r600_pipe_common.h
> > > > @@ -34,8 +34,6 @@
> > > > 
> > > >  #include <stdio.h>
> > > > 
> > > > -#include "amd/common/ac_binary.h"
> > > > -
> > > >  #include "radeon/radeon_winsys.h"
> > > > 
> > > >  #include "util/disk_cache.h"
> > > > @@ -134,9 +132,6 @@ struct r600_perfcounters;
> > > >  struct tgsi_shader_info;
> > > >  struct r600_qbo_state;
> > > > 
> > > > -void radeon_shader_binary_init(struct ac_shader_binary *b);
> > > > -void radeon_shader_binary_clean(struct ac_shader_binary *b);
> > > > -
> > > >  /* Only 32-bit buffer allocations are supported, gallium doesn't support more
> > > >   * at the moment.
> > > >   */
> > > > diff --git a/src/gallium/targets/pipe-loader/Makefile.am b/src/gallium/targets/pipe-loader/Makefile.am
> > > > index b1ef07e..e06d9321 100644
> > > > --- a/src/gallium/targets/pipe-loader/Makefile.am
> > > > +++ b/src/gallium/targets/pipe-loader/Makefile.am
> > > > @@ -129,14 +129,8 @@ pipe_r600_la_LIBADD = \
> > > >         $(top_builddir)/src/gallium/drivers/radeon/libradeon.la \
> > > >         $(top_builddir)/src/gallium/drivers/r600/libr600.la \
> > > >         $(LIBDRM_LIBS) \
> > > > -       $(RADEON_LIBS)
> > > > -
> > > > -# TODO: drop this dependency. libamd_common requires libdrm_amdgpu.
> > > > -if HAVE_AMD_DRIVERS
> > > > -pipe_r600_la_LIBADD += \
> > > > -       $(top_builddir)/src/amd/common/libamd_common.la
> > > > -endif
> > > > -
> > > > +       $(RADEON_LIBS) \
> > > > +       $(LIBELF_LIBS)
> > > >  endif
> > > > 
> > > >  if HAVE_GALLIUM_RADEONSI
> > > > --
> > > > 2.9.4
> > > > 
> > 
> > --
> > Jan Vesely <jan.vesely at rutgers.edu>

-- 
Jan Vesely <jan.vesely at rutgers.edu>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170609/faf7f8ce/attachment.sig>


More information about the mesa-dev mailing list