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

[Emil Velikov]

On 20 September 2018 at 18:24, Bas Nieuwenhuizen
<bas at basnieuwenhuizen.nl> wrote:
> 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!
>
I'm surprised there was no compiler warning :-\

>>
>> Even then, skimming at radv wrt anv:
>> - driver_uuid is never set, rather fortunately sincep
>> 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?

>From the Vulkan spec:
deviceUUID must be immutable for a given device across instances,
processes, driver APIs, driver versions, and system reboots

If you place another card in the came slow (say old one has died) the
UUID will stay the same.
I'm suggesting both bus and chipset since you can have varying
restrictions on the different slots, resulting in perf. or feature
changes.

-Emil