Re: [Intel-gfx] [PATCH 13/26] drm/i915: use the new iterator in i915_gem_busy_ioctl

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: "Christian König" <ckoenig.leichtzumerken@gmail.com>,
	linaro-mm-sig@lists.linaro.org, dri-devel@lists.freedesktop.org,
	linux-media@vger.kernel.org, intel-gfx@lists.freedesktop.org
Cc: daniel@ffwll.ch
Subject: Re: [Intel-gfx] [PATCH 13/26] drm/i915: use the new iterator in i915_gem_busy_ioctl
Date: Tue, 21 Sep 2021 14:10:26 +0100	[thread overview]
Message-ID: <5705c186-1c17-cd5d-c11d-8b5337204c8a@linux.intel.com> (raw)
In-Reply-To: <64b6a924-be38-0ed0-da92-86296702f71c@gmail.com>

On 21/09/2021 10:41, Christian König wrote:
> Am 20.09.21 um 12:33 schrieb Tvrtko Ursulin:
>> On 20/09/2021 11:13, Christian König wrote:
>>> Am 20.09.21 um 10:45 schrieb Tvrtko Ursulin:
>>>>
>>>> On 17/09/2021 13:35, Christian König wrote:
>>>>> This makes the function much simpler since the complex
>>>>> retry logic is now handled else where.
>>>>>
>>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>>> ---
>>>>>   drivers/gpu/drm/i915/gem/i915_gem_busy.c | 32 
>>>>> ++++++++----------------
>>>>>   1 file changed, 11 insertions(+), 21 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_busy.c 
>>>>> b/drivers/gpu/drm/i915/gem/i915_gem_busy.c
>>>>> index 6234e17259c1..b1cb7ba688da 100644
>>>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_busy.c
>>>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_busy.c
>>>>> @@ -82,8 +82,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void 
>>>>> *data,
>>>>>   {
>>>>>       struct drm_i915_gem_busy *args = data;
>>>>>       struct drm_i915_gem_object *obj;
>>>>> -    struct dma_resv_list *list;
>>>>> -    unsigned int seq;
>>>>> +    struct dma_resv_iter cursor;
>>>>> +    struct dma_fence *fence;
>>>>>       int err;
>>>>>         err = -ENOENT;
>>>>> @@ -109,27 +109,17 @@ i915_gem_busy_ioctl(struct drm_device *dev, 
>>>>> void *data,
>>>>>        * to report the overall busyness. This is what the 
>>>>> wait-ioctl does.
>>>>>        *
>>>>>        */
>>>>> -retry:
>>>>> -    seq = raw_read_seqcount(&obj->base.resv->seq);
>>>>> -
>>>>> -    /* Translate the exclusive fence to the READ *and* WRITE 
>>>>> engine */
>>>>> -    args->busy = 
>>>>> busy_check_writer(dma_resv_excl_fence(obj->base.resv));
>>>>> -
>>>>> -    /* Translate shared fences to READ set of engines */
>>>>> -    list = dma_resv_shared_list(obj->base.resv);
>>>>> -    if (list) {
>>>>> -        unsigned int shared_count = list->shared_count, i;
>>>>> -
>>>>> -        for (i = 0; i < shared_count; ++i) {
>>>>> -            struct dma_fence *fence =
>>>>> -                rcu_dereference(list->shared[i]);
>>>>> -
>>>>> +    args->busy = false;
>>>>> +    dma_resv_iter_begin(&cursor, obj->base.resv, true);
>>>>> +    dma_resv_for_each_fence_unlocked(&cursor, fence) {
>>>>
>>>> You did not agree with my suggestion to reset args->busy on restart 
>>>> and so preserve current behaviour?
>>>
>>> No, I want to keep the restart behavior internally to the dma_resv 
>>> object and as far as I can see it should not make a difference here.
>>
>> To be clear, on paper difference between old and new implementation is 
>> if the restart happens while processing the shared fences.
>>
>> Old implementation unconditionally goes to "args->busy =
>> >>> busy_check_writer(dma_resv_excl_fence(obj->base.resv));" and so 
>> overwrites the set of flags returned to userspace.
>>
>> New implementation can merge new read flags to the old set of flags 
>> and so return a composition of past and current fences.
>>
>> Maybe it does not matter hugely in this case, depends if userspace 
>> typically just restarts until flags are clear. But I am not sure.
>>
>> On the higher level - what do you mean with wanting to keep the 
>> restart behaviour internal? Not providing iterators users means of 
>> detecting it? I think it has to be provided.
> 
> Ok I will adjust that for now to get the patch set upstream. But in 
> general when somebody outside of the dma_resv code base depends on the 
> restart behavior then that's a bug inside the design of that code.

Thanks, no change in behaviour makes for an easy r-b. :)

> The callers should only care about what unsignaled fences are inside the 
> dma_resv container and it shouldn't matter if those fences are presented 
> once or multiple times because of a reset..
> 
> When this makes a difference we have a bug in the handling and should 
> probably consider taking the dma_resv.lock instead.

I agree, which is why I was mentioning earlier how it would be good to 
completely sort locked from unlocked iterators and avoid situations 
where unlocked one is called from a path where object is locked.

Unfortunately for the display code path I cannot easily help with the 
audit of call paths. And I think there are at least two patches in your 
series which need KMS expertise.

Regards,

Tvrtko