Re: [PATCH] drm/i915: Flush the existing fence before GGTT read/write

From: Matthew Auld <matthew.auld@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Flush the existing fence before GGTT read/write
Date: Fri, 23 Aug 2019 16:28:58 +0100	[thread overview]
Message-ID: <eb7a7a60-6110-733b-5aa4-2c13d4d29b7d@intel.com> (raw)
In-Reply-To: <cb9a9a4e-d4b1-daf8-258d-2335a6dd4b16@intel.com>

On 23/08/2019 16:26, Matthew Auld wrote:
> On 23/08/2019 16:03, Chris Wilson wrote:
>> Our fence management is lazy, very lazy. If the user marks an object as
>> untiled, we do not immediately flush the fence but merely mark it as
>> dirty. On the use we have to remember to check and remove the fence, by
>> which time we hope it is idle and we do not have to wait.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111468
>> Fixes: 1f7fd484fff1 ("drm/i915: Replace i915_vma_put_fence()")
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Matthew Auld <matthew.auld@intel.com>
> 
> Well that sucks, should we also try to be more solid on the reloc path?

Also not really a blocker so,
Reviewed-by: Matthew Auld <matthew.auld@intel.com>

> 
>> ---
>>   drivers/gpu/drm/i915/i915_gem.c | 30 ++++++++++++++++++++++++++++--
>>   1 file changed, 28 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c 
>> b/drivers/gpu/drm/i915/i915_gem.c
>> index eb31b69a316a..41b28f6d8620 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -324,6 +324,26 @@ gtt_user_read(struct io_mapping *mapping,
>>       return unwritten;
>>   }
>> +static int linear_ggtt_offset(struct i915_vma *vma, u64 *offset)
>> +{
>> +    int ret;
>> +
>> +    GEM_BUG_ON(!i915_vma_is_map_and_fenceable(vma));
>> +
>> +    /* Open-coded i915_vma_pin_iomap() */
>> +
>> +    if (vma->fence) {
>> +        mutex_lock(&vma->vm->mutex);
>> +        ret = i915_vma_revoke_fence(vma);
>> +        mutex_unlock(&vma->vm->mutex);
>> +        if (ret)
>> +            return ret;
>> +    }
>> +
>> +    *offset = i915_ggtt_offset(vma);
>> +    return 0;
>> +}
>> +
>>   static int
>>   i915_gem_gtt_pread(struct drm_i915_gem_object *obj,
>>              const struct drm_i915_gem_pread *args)
>> @@ -350,7 +370,10 @@ i915_gem_gtt_pread(struct drm_i915_gem_object *obj,
>>                              PIN_NONBLOCK /* NOWARN */ |
>>                              PIN_NOEVICT);
>>       if (!IS_ERR(vma)) {
>> -        node.start = i915_ggtt_offset(vma);
>> +        ret = linear_ggtt_offset(vma, &node.start);
>> +        if (ret)
>> +            goto out_unpin;
>> +
>>           node.allocated = false;
> 
> node.allocated = false should go before the jump.
> 
>>       } else {
>>           ret = insert_mappable_node(ggtt, &node, PAGE_SIZE);
>> @@ -560,7 +583,10 @@ i915_gem_gtt_pwrite_fast(struct 
>> drm_i915_gem_object *obj,
>>                              PIN_NONBLOCK /* NOWARN */ |
>>                              PIN_NOEVICT);
>>       if (!IS_ERR(vma)) {
>> -        node.start = i915_ggtt_offset(vma);
>> +        ret = linear_ggtt_offset(vma, &node.start);
>> +        if (ret)
>> +            goto out_unpin;
>> +
>>           node.allocated = false;
>>       } else {
>>           ret = insert_mappable_node(ggtt, &node, PAGE_SIZE);
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx