[Intel-gfx] [PATCH i-g-t 1/8] lib/igt_dummyload: add igt_cork

[Daniele Ceraolo Spurio]

On 19/10/17 11:12, Chris Wilson wrote:
> Quoting Daniele Ceraolo Spurio (2017-10-18 17:50:33)
>>
>>
>> On 18/10/17 09:04, Chris Wilson wrote:
>>> Quoting Daniele Ceraolo Spurio (2017-10-18 16:49:24)
>>>>
>>>>
>>>> On 13/10/17 09:37, Daniele Ceraolo Spurio wrote:
>>>>>
>>>>>
>>>>> On 13/10/17 01:31, Chris Wilson wrote:
>>>>>> Quoting Chris Wilson (2017-10-12 23:57:38)
>>>>>>> Quoting Daniele Ceraolo Spurio (2017-10-12 23:27:27)
>>>>>>>> +igt_cork_t *igt_cork_new(int fd);
>>>>>>>
>>>>>>> _new does not imply plugged.
>>>>>>>
>>>>>>>> +void igt_cork_signal(igt_cork_t *cork);
>>>>>>>
>>>>>>> When have you signaled a cork?
>>>>>>>
>>>>>>>> +void igt_cork_free(int fd, igt_cork_t *cork);
>>>>>>>
>>>>>>> _free does not imply unplug.
>>>>>>
>>>>>> To be clear the verbs are to plug and unplug a queue/schedule. Cork is a
>>>>>> reference to TCP_CORK which does the same thing, but plug/unplug are
>>>>>> more commonplace (at least in kernel code).
>>>>>>
>>>>>> I don't see any reason why we need a malloc here.
>>>>>> -Chris
>>>>>>
>>>>>
>>>>> I added the malloc just to use the same approach as the spin_batch, I'll
>>>>> get rid of it.
>>>>> My concern with the existing plug/unplug scheme was that the plug()
>>>>> function in the various tests didn't really plug anything but just
>>>>> created the bo and that was slightly confusing.
>>>
>>> It created a bo with an unsignaled fence, that's enough to plug anything
>>> attached to it. Since we can't just say plug(device) we have to say
>>> execbuf(device, plug()).
>>>
>>>>> What do you think of going with:
>>>>>
>>>>>        struct igt_cork {
>>>>>            int device;
>>>>>            uint32_t handle;
>>>>>            uint32_t fence;
>>>>>        };
>>>>>
>>>>>        struct igt_cork igt_cork_create(int fd);
>>>>>        void igt_cork_unplug(struct igt_cork *cork);
>>>>>        void igt_cork_close(int fd, struct igt_cork *cork);
>>>>>        void igt_cork_unplug_and_close(int fd, struct igt_cork *cork);
>>>
>>> close will always be unplug; there's no differentiation, in both APIs we
>>> ensure that any fence associated with the device or timeline fd is
>>> signaled upon release. We could lose the fence and still work, but for
>>> us it gives us the means by which we can do a test-and-set and report an
>>> issue where the fence was signaled too early (due to slow test setup).
>>> Similarly once unplugged, there is no use for the struct anymore, you
>>> could release the device/timeline, but we've embedded it because in
>>> terms of overhead, so far it has been insignificant.
>>>
>>> Leaving a fence dangling by separating unplug/close is a good way to
>>> leave lots of timeouts and GPU resets behind.
>>> -Chris
>>>
>>
>> What I wanted to separate is the unplugging from the closing of the BO
>> handle, because in some case we keep the BO around for a while after
>> unblocking the execution.
> 
> Where? What value could it possibly have? You know the state of its
> fence, so presumably you want the contents. In such a situation you don't
> need a dummy cork to plug the queue, you have a real object with which
> you want interact.
> 
>> In most of those cases the BO handle is not
>> currently closed,
> 
> Every single unplug() function is closing the device; which closes the
> handle; gem_close() is superfluous. Early on I kept the vgem fd around
> and just needed to close the handle, but it looks like all of those
> functions have now been converted to own their device.
> -Chris
> 

Apologies for being unclear, what I was referring to was the imported BO 
handle, not the one from the vgem device. In a couple of tests 
(gem_wait, gem_exec_latency) we still use it after the unplug. My 
understanding (which could be wrong, since I haven't looked at this area 
a lot) is that the imported handle will stay valid even if the original 
device is closed and that's why I wanted to explicitly clean it up. 
It'll still be closed when the fd is closed so we're not doing something 
inherently bad, but I felt it would've been cleaner to have that in the 
common function. I'll add a note about it in the function description 
instead and keep the existing plug/unplug scheme.

Daniele