<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 21 October 2017 at 02:54, Bas Nieuwenhuizen <<a href="mailto:bas@basnieuwenhuizen.nl" target="_blank">bas@basnieuwenhuizen.nl</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">For radv_create_shader_variants_from_pipeline_cache I'm not really sure why this would cause corruption. Yes we might create the variants a few times too much, but that should not cause any corruption. </blockquote><div> </div><div>Just had another look and figured out what the actual problem is - if there's a race between multiple threads to create the variants for a single entry, then they may not update p inside the loop properly (since they only do so when the variant isn't already created), so can end up using the wrong code. I'll send a patch - I don't think it's necessary now that we're locking around there (we'll only create all or none of the variants), but may as well fix it in case things change later.</div><div> </div><div>Alex</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Either way, it is a fix, so Reviewed-by: Bas Nieuwenhuizen <<a href="mailto:bas@basnieuwenhuizen.nl">bas@basnieuwenhuizen.nl</a>> and pushed. Thanks. <div class="HOEnZb"><div class="h5"> On Thu, Oct 19, 2017 at 12:49 PM, Alex Smith <<a href="mailto:asmith@feralinteractive.com">asmith@feralinteractive.com</a>> wrote: > Need to lock around the whole process of retrieving cached shaders, and > around GetPipelineCacheData. > > This fixes GPU hangs observed when creating multiple pipelines in > parallel, which appeared to be due to invalid shader code being pulled > from the cache. > > Signed-off-by: Alex Smith <<a href="mailto:asmith@feralinteractive.com">asmith@feralinteractive.com</a>> > --- > src/amd/vulkan/radv_pipeline_cache.c | 30 +++++++++++++++++++++++------- > 1 file changed, 23 insertions(+), 7 deletions(-) > > diff --git a/src/amd/vulkan/radv_pipeline_cache.c b/src/amd/vulkan/radv_pipeline_cache.c > index 034dc35af8..a75356b822 100644 > --- a/src/amd/vulkan/radv_pipeline_cache.c > +++ b/src/amd/vulkan/radv_pipeline_cache.c > @@ -177,15 +177,20 @@ radv_create_shader_variants_from_pipeline_cache(struct radv_device *device, > struct radv_shader_variant **variants) > { > struct cache_entry *entry; > - if (cache) > - entry = radv_pipeline_cache_search(cache, sha1); > - else > - entry = radv_pipeline_cache_search(device->mem_cache, sha1); > + > + if (!cache) > + cache = device->mem_cache; > + > + pthread_mutex_lock(&cache->mutex); > + > + entry = radv_pipeline_cache_search_unlocked(cache, sha1); > > if (!entry) { > if (!device->physical_device->disk_cache || > - (device->instance->debug_flags & RADV_DEBUG_NO_CACHE)) > + (device->instance->debug_flags & RADV_DEBUG_NO_CACHE)) { > + pthread_mutex_unlock(&cache->mutex); > return false; > + } > > uint8_t disk_sha1[20]; > disk_cache_compute_key(device->physical_device->disk_cache, > @@ -193,8 +198,10 @@ radv_create_shader_variants_from_pipeline_cache(struct radv_device *device, > entry = (struct cache_entry *) > disk_cache_get(device->physical_device->disk_cache, > disk_sha1, NULL); > - if (!entry) > + if (!entry) { > + pthread_mutex_unlock(&cache->mutex); > return false; > + } > } > > char *p = entry->code; > @@ -204,8 +211,10 @@ radv_create_shader_variants_from_pipeline_cache(struct radv_device *device, > struct cache_entry_variant_info info; > > variant = calloc(1, sizeof(struct radv_shader_variant)); > - if (!variant) > + if (!variant) { > + pthread_mutex_unlock(&cache->mutex); > return false; > + } > > memcpy(&info, p, sizeof(struct cache_entry_variant_info)); > p += sizeof(struct cache_entry_variant_info); > @@ -231,6 +240,7 @@ radv_create_shader_variants_from_pipeline_cache(struct radv_device *device, > p_atomic_inc(&entry->variants[i]->ref_count); > > memcpy(variants, entry->variants, sizeof(entry->variants)); > + pthread_mutex_unlock(&cache->mutex); > return true; > } > > @@ -509,12 +519,17 @@ VkResult radv_GetPipelineCacheData( > RADV_FROM_HANDLE(radv_pipeline_cache, cache, _cache); > struct cache_header *header; > VkResult result = VK_SUCCESS; > + > + pthread_mutex_lock(&cache->mutex); > + > const size_t size = sizeof(*header) + cache->total_size; > if (pData == NULL) { > + pthread_mutex_unlock(&cache->mutex); > *pDataSize = size; > return VK_SUCCESS; > } > if (*pDataSize < sizeof(*header)) { > + pthread_mutex_unlock(&cache->mutex); > *pDataSize = 0; > return VK_INCOMPLETE; > } > @@ -545,6 +560,7 @@ VkResult radv_GetPipelineCacheData( > } > *pDataSize = p - pData; > > + pthread_mutex_unlock(&cache->mutex); > return result; > } > > -- > 2.13.6 > </div></div></blockquote></div> </div></div>