[Mesa-dev] [PATCH 2/8] anv: Use library mtime for cache UUID.
From: Emil Velikov 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 --- 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 #include #include #include #include +#include #include #include #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 +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 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; + } + /* XXX: Actually detect bit6 swizzling */ isl_device_init(&device->isl_dev, &device->info, swizzled); -- 2.10.2 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/8] anv: Use library mtime for cache UUID.
I'm not sure what I think of this... It has some interesting implications. I'll think about it and write something more detailed on Monday. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/8] anv: Use library mtime for cache UUID.
On 24 November 2016 at 20:58, Jason Ekstrand wrote: > I'm not sure what I think of this... It has some interesting implications. > I'll think about it and write something more detailed on Monday. But but... it helps up get rid of the lovely timestamp experience Thanks for having a look - how go back and enjoy Thanks Giving ;-) Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/8] anv: Use library mtime for cache UUID.
On Thursday, November 24, 2016 8:30:39 PM PST Emil Velikov wrote: > From: Emil Velikov > > 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 > --- > 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 > #include > #include > #include > #include > +#include > #include > #include > > #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. > +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? 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 But please wait for Jason - it sounds like he has some concerns and I'm not clear what those are. > /* XXX: Actually detect bit6 swizzling */ > isl_device_init(&device->isl_dev, &device->info, swizzled); > signature.asc Description: This is a digitally signed message part. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/8] anv: Use library mtime for cache UUID.
On Sat, Nov 26, 2016 at 6:31 PM, Kenneth Graunke wrote: > On Thursday, November 24, 2016 8:30:39 PM PST Emil Velikov wrote: > > From: Emil Velikov > > > > 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 > > --- > > 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 > > #include > > #include > > #include > > #include > > +#include > > #include > > #include > > > > #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. > > > +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? > > 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 > > But please wait for Jason - it sounds like he has some concerns and > I'm not clear what those are. > Nah. I'm fine with it. I'd just like to eventually come up with something more static. This seems to work fine. > > /* XXX: Actually detect bit6 swizzling */ > > isl_device_init(&device->isl_dev, &device->info, swizzled); > > > > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/8] anv: Use library mtime for cache UUID.
On 27 November 2016 at 02:31, Kenneth Graunke wrote: > On Thursday, November 24, 2016 8:30:39 PM PST Emil Velikov wrote: >> From: Emil Velikov >> >> 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 >> --- >> 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 >> #include >> #include >> #include >> #include >> +#include >> #include >> #include >> >> #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 > > 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
Re: [Mesa-dev] [PATCH 2/8] anv: Use library mtime for cache UUID.
On Sunday, November 27, 2016 12:17:52 PM PST Emil Velikov wrote: > On 27 November 2016 at 02:31, Kenneth Graunke wrote: > > On Thursday, November 24, 2016 8:30:39 PM PST Emil Velikov wrote: > >> From: Emil Velikov [snip] > >> @@ -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 was looking at the previous two. device->compiler == NULL apparently doesn't need to perform any teardown. But anv_init_wsi failing ought to be freeing device->compiler, and it isn't. That's a bug I suppose. Rather than adding anv_finish_wsi/ralloc_free calls here, why not move the cache UUID initialization earlier? (Say, after the MMAP_VERSION check). Then we can just do if (!anv_device_get_cache_uuid(device->uuid, device->chipset_id)) { result = vkerrorf(VK_ERROR_INITIALIZATION_FAILED, "failed to get cache UUID"); goto fail; } Or make anv_device_get_cache_uuid return VkSuccess/vkerrorf. I like that idea too...it's clearer than a boolean or an integer. I'm fine with whatever you come up with. signature.asc Description: This is a digitally signed message part. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev