On to, 2016-06-09 at 12:29 +0100, Chris Wilson wrote: > There is an improbable, but not impossible, case that if we leave the > pages unpin as we operate on the object, then somebody may steal the > lock and change the cache domains after we have already inspected them. >
Which lock exactly? > (Whilst here, avail ourselves of the opportunity to take a couple of > steps to make the two functions look more similar.) > > Signed-off-by: Chris Wilson <[email protected]> > --- > drivers/gpu/drm/i915/i915_gem.c | 88 > ++++++++++++++++++++++++----------------- > 1 file changed, 51 insertions(+), 37 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index ffe3d3e9d69d..8c90b6a12d45 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -571,13 +571,22 @@ int i915_gem_obj_prepare_shmem_read(struct > drm_i915_gem_object *obj, > if ((obj->ops->flags & I915_GEM_OBJECT_HAS_STRUCT_PAGE) == 0) > return -EINVAL; > > + ret = i915_gem_object_get_pages(obj); > + if (ret) > + return ret; > + There's still time for the pages to disappear at this point if somebody is racing with us, and BUG_ON will be imminent. So I'm not sure which lock you mean. > + i915_gem_object_pin_pages(obj); > + > + if (obj->base.write_domain == I915_GEM_DOMAIN_CPU) > + goto out; > + > + ret = i915_gem_object_wait_rendering(obj, true); > + if (ret) > + goto err_unpin; > + > i915_gem_object_flush_gtt_write_domain(obj); > > if (!(obj->base.read_domains & I915_GEM_DOMAIN_CPU)) { > - ret = i915_gem_object_wait_rendering(obj, true); > - if (ret) > - return ret; > - > /* If we're not in the cpu read domain, set ourself into the gtt > * read domain and manually flush cachelines (if required). This > * optimizes for the case when the gpu will dirty the data > @@ -586,26 +595,25 @@ int i915_gem_obj_prepare_shmem_read(struct > drm_i915_gem_object *obj, > obj->cache_level); > } > > - ret = i915_gem_object_get_pages(obj); > - if (ret) > - return ret; > - > - i915_gem_object_pin_pages(obj); > - > if (*needs_clflush && !boot_cpu_has(X86_FEATURE_CLFLUSH)) { > ret = i915_gem_object_set_to_cpu_domain(obj, false); > - if (ret) { > - i915_gem_object_unpin_pages(obj); > - return ret; > - } > + if (ret) > + goto err_unpin; > + > *needs_clflush = 0; > } > > +out: > + /* return with the pages pinned */ > return 0; > + > +err_unpin: > + i915_gem_object_unpin_pages(obj); > + return ret; > } > > int i915_gem_obj_prepare_shmem_write(struct drm_i915_gem_object *obj, > - unsigned *needs_clflush) > + unsigned *needs_clflush) > { > int ret; > > @@ -613,20 +621,27 @@ int i915_gem_obj_prepare_shmem_write(struct > drm_i915_gem_object *obj, > if ((obj->ops->flags & I915_GEM_OBJECT_HAS_STRUCT_PAGE) == 0) > return -EINVAL; > > - i915_gem_object_flush_gtt_write_domain(obj); > + ret = i915_gem_object_get_pages(obj); > + if (ret) > + return ret; > > - if (obj->base.write_domain != I915_GEM_DOMAIN_CPU) { > - ret = i915_gem_object_wait_rendering(obj, false); > - if (ret) > - return ret; > + i915_gem_object_pin_pages(obj); > > - /* If we're not in the cpu write domain, set ourself into the > - * gtt write domain and manually flush cachelines (as required). > - * This optimizes for the case when the gpu will use the data > - * right away and we therefore have to clflush anyway. > - */ > - *needs_clflush |= cpu_write_needs_clflush(obj) << 1; > - } > + if (obj->base.write_domain == I915_GEM_DOMAIN_CPU) > + goto out; > + > + ret = i915_gem_object_wait_rendering(obj, false); > + if (ret) > + goto err_unpin; > + > + i915_gem_object_flush_gtt_write_domain(obj); > + > + /* If we're not in the cpu write domain, set ourself into the > + * gtt write domain and manually flush cachelines (as required). > + * This optimizes for the case when the gpu will use the data > + * right away and we therefore have to clflush anyway. > + */ > + *needs_clflush |= cpu_write_needs_clflush(obj) << 1; Use ?: to keep the code readable. > > /* Same trick applies to invalidate partially written cachelines read > * before writing. > @@ -635,27 +650,26 @@ int i915_gem_obj_prepare_shmem_write(struct > drm_i915_gem_object *obj, > *needs_clflush |= !cpu_cache_is_coherent(obj->base.dev, > obj->cache_level); > > - ret = i915_gem_object_get_pages(obj); > - if (ret) > - return ret; > - > - i915_gem_object_pin_pages(obj); > - > if (*needs_clflush && !boot_cpu_has(X86_FEATURE_CLFLUSH)) { > ret = i915_gem_object_set_to_cpu_domain(obj, true); > - if (ret) { > - i915_gem_object_unpin_pages(obj); > - return ret; > - } > + if (ret) > + goto err_unpin; > + > *needs_clflush = 0; > } > > if ((*needs_clflush & CLFLUSH_AFTER) == 0) > obj->cache_dirty = true; > > +out: > intel_fb_obj_invalidate(obj, ORIGIN_CPU); > obj->dirty = 1; > + /* return with the pages pinned */ > return 0; > + > +err_unpin: > + i915_gem_object_unpin_pages(obj); > + return ret; > } > > /* Per-page copy function for the shmem pread fastpath. -- Joonas Lahtinen Open Source Technology Center Intel Corporation _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
