On 27 November 2016 at 02:31, Kenneth Graunke <kenn...@whitecape.org> wrote:
> On Thursday, November 24, 2016 8:30:39 PM PST Emil Velikov wrote:
>> From: Emil Velikov <emil.veli...@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.veli...@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, ×tamp))
>> + 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 <kenn...@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
