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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
