On Fri, Aug 05, 2016 at 09:11:14PM +0100, Chris Wilson wrote:
> In the debate as to whether the second read of active->request is
> ordered after the dependent reads of the first read of active->request,
> just give in and throw a smp_rmb() in there so that ordering of loads is
> assured.
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
Oh now I get it, you pushed more patches meanwhile ;-)
> ---
> drivers/gpu/drm/i915/i915_gem.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 28c34cd7f489..c6d85ae35ba6 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3709,7 +3709,7 @@ i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
> return vma;
> }
>
> -static __always_inline unsigned __busy_read_flag(unsigned int id)
> +static __always_inline unsigned int __busy_read_flag(unsigned int id)
> {
> /* Note that we could alias engines in the execbuf API, but
> * that would be very unwise as it prevents userspace from
> @@ -3727,7 +3727,7 @@ static __always_inline unsigned int __busy_write_id(unsigned int id)
> return id;
> }
>
> -static __always_inline unsigned
> +static __always_inline unsigned int
> __busy_set_if_active(const struct i915_gem_active *active,
> unsigned int (*flag)(unsigned int id))
> {
> @@ -3745,18 +3745,19 @@ __busy_set_if_active(const struct i915_gem_active *active,
> id = request->engine->exec_id;
>
> /* Check that the pointer wasn't reassigned and overwritten. */
Needs a comment, e.g.
In __i915_gem_active_get_rcu we enforce ordering between
the lockless access and the 2nd rcu pointer dereference
through the barrier in fence_get_rcu/kref_get_unless_zero
for the success case. Here there is no thing and we need
an explicit read barrier. The corresponding write barrier
is part of rcu_assign_pointer.
Hm reminds me, might be good to mention in active_get_rcu where the write
barrier is, too.
With the above (or similar) added:
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> + smp_rmb();
> if (request == rcu_access_pointer(active->request))
> return flag(id);
> } while (1);
> }
>
> -static inline unsigned
> +static __always_inline unsigned int
> busy_check_reader(const struct i915_gem_active *active)
> {
> return __busy_set_if_active(active, __busy_read_flag);
> }
>
> -static inline unsigned
> +static __always_inline unsigned int
> busy_check_writer(const struct i915_gem_active *active)
> {
> return __busy_set_if_active(active, __busy_write_id);
> --
> 2.8.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
