[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
