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 <chris@xxxxxxxxxxxxxxxxxx>
> ---
> 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
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
