On Tue, 2017-03-14 at 10:22 +0200, Pohjolainen, Topi wrote:
> On Fri, Mar 10, 2017 at 01:38:15PM +0100, Iago Toral Quiroga wrote:
> >
> > This situation can happen if we failed to allocate memory for the
> > shader.
> >
> > v2:
> > - We shouldn't see NULL shaders in anv_shader_bin_ref so we should
> > not check
> > for that (Jason). Make sure that callers don't attempt to call
> > this
> > function with a NULL shader and assert that this never happens
> > (Iago).
> > ---
> > src/intel/vulkan/anv_pipeline_cache.c | 5 ++++-
> > src/intel/vulkan/anv_private.h | 5 ++++-
> > 2 files changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/intel/vulkan/anv_pipeline_cache.c
> > b/src/intel/vulkan/anv_pipeline_cache.c
> > index a8ea80f..d6c8413 100644
> > --- a/src/intel/vulkan/anv_pipeline_cache.c
> > +++ b/src/intel/vulkan/anv_pipeline_cache.c
> > @@ -308,7 +308,8 @@ anv_pipeline_cache_upload_kernel(struct
> > anv_pipeline_cache *cache,
> > pthread_mutex_unlock(&cache->mutex);
> >
> > /* We increment refcount before handing it to the caller */
> > - anv_shader_bin_ref(bin);
> > + if (bin)
> > + anv_shader_bin_ref(bin);
> >
> > return bin;
> > } else {
> > @@ -546,6 +547,8 @@ VkResult anv_MergePipelineCaches(
> > struct hash_entry *entry;
> > hash_table_foreach(src->cache, entry) {
> > struct anv_shader_bin *bin = entry->data;
> > + assert(bin);
> > +
> > if (_mesa_hash_table_search(dst->cache, bin->key))
> > continue;
> >
> > diff --git a/src/intel/vulkan/anv_private.h
> > b/src/intel/vulkan/anv_private.h
> > index 27c1923..aa2b6d7 100644
> > --- a/src/intel/vulkan/anv_private.h
> > +++ b/src/intel/vulkan/anv_private.h
> > @@ -1543,13 +1543,16 @@ anv_shader_bin_destroy(struct anv_device
> > *device, struct anv_shader_bin *shader)
> > static inline void
> > anv_shader_bin_ref(struct anv_shader_bin *shader)
> > {
> > - assert(shader->ref_cnt >= 1);
> > + assert(shader && shader->ref_cnt >= 1);
> > __sync_fetch_and_add(&shader->ref_cnt, 1);
> > }
> >
> > static inline void
> > anv_shader_bin_unref(struct anv_device *device, struct
> > anv_shader_bin *shader)
> > {
> > + if (!shader)
> > + return;
> > +
> There are two callers in anv_pipeline.c: anv_DestroyPipeline and
> anv_pipeline_init. Both check against NULL before calling.
>
> In anv_pipeline_cache.c::anv_pipeline_cache_finish() this is only
> called if
> item is found in cache.
>
> Finally lookup_blorp_shader() and upload_blorp_shader() call this if
> and only
> if earlier reference is found in cache. Assert instead?
Sounds reasonable to me, I'll assert instead. Thanks!
Iago
> >
> > assert(shader->ref_cnt >= 1);
> > if (__sync_fetch_and_add(&shader->ref_cnt, -1) == 1)
> > anv_shader_bin_destroy(device, shader);
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
