[Mesa-dev] [PATCH 2/8] anv: Use library mtime for cache UUID.

[Emil Velikov]

On 27 November 2016 at 02:31, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On Thursday, November 24, 2016 8:30:39 PM PST Emil Velikov wrote:
>> From: Emil Velikov <emil.velikov at collabora.com>
>>
>> Inspired by a similar commit for radv.
>>
>> Rather than recomputing the timestamp on each make invocation, just
>> fetch it at runtime.
>>
>> Thus we no longer get the constant rebuild of anv_device.c and the
>> follow-up libvulkan_intel.so link, when nothing has changed.
>>
>> I.e. using make && make install is a little bit faster.
>>
>> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
>> ---
>>  src/intel/vulkan/anv_device.c | 37 +++++++++++++++++++++++++++++++++----
>>  1 file changed, 33 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
>> index 58c6b3f..4711501 100644
>> --- a/src/intel/vulkan/anv_device.c
>> +++ b/src/intel/vulkan/anv_device.c
>> @@ -21,15 +21,16 @@
>>   * IN THE SOFTWARE.
>>   */
>>
>> +#include <dlfcn.h>
>>  #include <assert.h>
>>  #include <stdbool.h>
>>  #include <string.h>
>>  #include <sys/mman.h>
>> +#include <sys/stat.h>
>>  #include <unistd.h>
>>  #include <fcntl.h>
>>
>>  #include "anv_private.h"
>> -#include "anv_timestamp.h"
>>  #include "util/strtod.h"
>>  #include "util/debug.h"
>>
>> @@ -53,11 +54,32 @@ compiler_perf_log(void *data, const char *fmt, ...)
>>     va_end(args);
>>  }
>>
>> -static void
>> +static int
>
> I'd prefer to have us return a bool on success - we don't tend to use
> the "0 means success" idiom much in Mesa.
>
Was hoping to have ~consistent code through the vulkan drivers. But I
don't mind that much so bool it is.

>> +anv_get_function_timestamp(void *ptr, uint32_t* timestamp)
>> +{
>> +   Dl_info info;
>> +   struct stat st;
>> +   if (!dladdr(ptr, &info) || !info.dli_fname)
>> +      return -1;
>> +
>> +   if (stat(info.dli_fname, &st))
>> +      return -1;
>> +
>> +   *timestamp = st.st_mtim.tv_sec;
>> +   return 0;
>> +}
>> +
>> +static int
>
> Ditto.
>
>>  anv_device_get_cache_uuid(void *uuid)
>>  {
>> +   uint32_t timestamp;
>> +
>>     memset(uuid, 0, VK_UUID_SIZE);
>> -   snprintf(uuid, VK_UUID_SIZE, "anv-%s", ANV_TIMESTAMP);
>> +   if (anv_get_function_timestamp(anv_device_get_cache_uuid, &timestamp))
>> +         return -1;
>> +
>> +   snprintf(uuid, VK_UUID_SIZE, "anv-%d", timestamp);
>> +   return 0;
>>  }
>>
>>  static VkResult
>> @@ -186,7 +208,14 @@ anv_physical_device_init(struct anv_physical_device *device,
>>     if (result != VK_SUCCESS)
>>         goto fail;
>>
>> -   anv_device_get_cache_uuid(device->uuid);
>> +   if (anv_device_get_cache_uuid(device->uuid)) {
>> +      anv_finish_wsi(device);
>> +      ralloc_free(device->compiler);
>> +      result = vk_errorf(VK_ERROR_INITIALIZATION_FAILED,
>> +                         "cannot generate UUID");
>> +      goto fail;
>> +   }
>> +
>
> I don't see why this error case is special - all the others just have
> "goto fail".  This stuff may need to get cleaned up, but shouldn't it
> happen for the other error paths too?
>
We need to set the error, since result is equal to VK_SUCCESS as we
can (barely) see in the diff above. If you want we can make
anv_device_get_cache_uuid() return VkResult and honour that ?
What do you mean with "all the others just have goto fail" ? Most
error paths have appropriate teardown with the incomplete ones being
addressed later in the series.
Are you thinking of more fine-grained goto labels or something else ?

> I wrote these same patches today, not realizing you'd already done it,
> so with those comments addressed, consider it a series-wide:
>
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>
> But please wait for Jason - it sounds like he has some concerns and
> I'm not clear what those are.
>
Ack, thanks !

Emil