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

[Daniele Ceraolo Spurio]

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. In most of those cases the BO handle is not 
currently closed, but it seemed dirty to me to keep that behavior and 
have library functions that didn't clean up after themselves. Would it 
be ok for you to keep both unplug and close, with the first doing just 
the signaling of the fence and the second doing both the signaling (if 
not already done) and the bo handle closure? this would basically be 
what is in the current version of the patches but with a different name. 
Alternatively, we could explicitly return the handle from the plug() 
call to make it clearer that it needs closing:

uint32_t igt_plug(int fd, struct igt_cork *cork);
void igt_unplug(struct igt_cork *cork);

Could also keep the typedef in this case since the handle is returned 
and we don't need to peek inside the cork struct.

Thanks,
Daniele