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

[Kenneth Graunke]

On Sunday, November 27, 2016 12:17:52 PM PST Emil Velikov wrote:
> 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>
[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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20161127/2cf9630d/attachment.sig>