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

[Chris Wilson]

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