[Mesa-stable] [PATCH] radv: Use build ID if available for cache UUID.

[Bas Nieuwenhuizen]

On Thu, Sep 20, 2018 at 1:29 PM Emil Velikov <emil.l.velikov at gmail.com> wrote:
>
> On 16 September 2018 at 01:58, Bas Nieuwenhuizen
> <bas at basnieuwenhuizen.nl> wrote:
> > To get an useful UUID for systems that have a non-useful mtime
> > for the binaries.
> >
> > I started using using SHA1 to ensure we get reasonable mixing
> > in the various possibilities and the various build id lengths.
> >
> > CC: <mesa-stable at lists.freedesktop.org>
> > ---
> >  src/amd/vulkan/radv_device.c | 43 +++++++++++++++++++++++++++++-------
> >  1 file changed, 35 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/amd/vulkan/radv_device.c b/src/amd/vulkan/radv_device.c
> > index 8989ec3553f..a2a73089f27 100644
> > --- a/src/amd/vulkan/radv_device.c
> > +++ b/src/amd/vulkan/radv_device.c
> > @@ -45,22 +45,49 @@
> >  #include "sid.h"
> >  #include "gfx9d.h"
> >  #include "addrlib/gfx9/chip/gfx9_enum.h"
> > +#include "util/build_id.h"
> >  #include "util/debug.h"
> > +#include "util/mesa-sha1.h"
> > +
> > +static bool
> > +radv_get_build_id(void *ptr, struct mesa_sha1 *ctx)
> > +{
> > +       uint32_t timestamp;
> > +
> > +#ifdef HAVE_DL_ITERATE_PHDR
> > +       const struct build_id_note *note = NULL;
> > +       if ((note = build_id_find_nhdr_for_addr(ptr))) {
> > +               _mesa_sha1_update(ctx, build_id_data(note), build_id_length(note));
> > +       } else
> > +#endif
> > +       if (disk_cache_get_function_timestamp(ptr, &timestamp)) {
> > +               if (!timestamp) {
> > +                       fprintf(stderr, "radv: The provided filesystem timestamp for the cache is bogus!\n");
> > +               }
> > +
> > +               _mesa_sha1_update(ctx, &timestamp, sizeof(timestamp));
> > +       } else
> > +               return false;
> > +       return true;
> > +}
> >
> >  static int
> >  radv_device_get_cache_uuid(enum radeon_family family, void *uuid)
> >  {
> > -       uint32_t mesa_timestamp, llvm_timestamp;
> > -       uint16_t f = family;
> > +       struct mesa_sha1 ctx;
> > +       unsigned char sha1[20];
> > +       unsigned ptr_size = sizeof(void*);
> >         memset(uuid, 0, VK_UUID_SIZE);
> > -       if (!disk_cache_get_function_timestamp(radv_device_get_cache_uuid, &mesa_timestamp) ||
> > -           !disk_cache_get_function_timestamp(LLVMInitializeAMDGPUTargetInfo, &llvm_timestamp))
> > +
> > +       if (!radv_get_build_id(radv_device_get_cache_uuid, &ctx) ||
> > +           !radv_get_build_id(LLVMInitializeAMDGPUTargetInfo, &ctx))
> >                 return -1;
> >
> > -       memcpy(uuid, &mesa_timestamp, 4);
> > -       memcpy((char*)uuid + 4, &llvm_timestamp, 4);
> > -       memcpy((char*)uuid + 8, &f, 2);
> > -       snprintf((char*)uuid + 10, VK_UUID_SIZE - 10, "radv%zd", sizeof(void *));
> > +       _mesa_sha1_update(&ctx, &family, sizeof(family));
> > +       _mesa_sha1_update(&ctx, &ptr_size, sizeof(ptr_size));
> > +       _mesa_sha1_final(&ctx, sha1);
> > +
> A slightly more important thing I did not notice:
> The _mesa_sha1_init() call is missing, so we're working on an
> uninitialized stack variable.

wow I missed that, thanks!

>
> Even then, skimming at radv wrt anv:
> - driver_uuid is never set, rather fortunately since
> ac_compute_driver_uuid() returns a fixed "AMD-MESA-DRV"
> ANV does sha1_update(build_id) + sha1_update(chipset_id)


>
> - device_uuid produces a "unique ID" of the bus location, not the device
> Ideally you'd want bus location + chipset_id + driver specific options.
> ANV only lacks the bus location, since the device is fixed ;-)

why would we need chipset id? Can there be multiple devices on a bus location?
>
> Aside: disk_cache_format_hex_id is a copy of _mesa_sha1_format()
>
> HTH
> Emil