<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Aug 14, 2017 at 12:36 AM, Christian König <span dir="ltr"><<a href="mailto:christian.koenig@amd.com" target="_blank">christian.koenig@amd.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">Am 14.08.2017 um 01:14 schrieb Jason Ekstrand:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On August 13, 2017 8:52:21 AM Christian König <<a href="mailto:christian.koenig@amd.com" target="_blank">christian.koenig@amd.com</a>> wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Am 13.08.2017 um 17:26 schrieb Jason Ekstrand:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On August 13, 2017 6:19:53 AM Christian König<br>
<<a href="mailto:christian.koenig@amd.com" target="_blank">christian.koenig@amd.com</a>> wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Patches #1-#4 are Acked-by: Christian König <<a href="mailto:christian.koenig@amd.com" target="_blank">christian.koenig@amd.com</a>>.<br>
<br>
Patch #5: NAK, that will break radeon.<br>
<br>
On radeon we need the non-default wait or otherwise we can run into a<br>
situation where we never signal a fence.<br>
<br>
The general question is why do you need this?<br>
</blockquote>
<br>
Because i915 sets a non-default wait function so calling wait_any just<br>
bails with fences from i915 immediately bails with -EINVAL. This makes<br>
it work even with non-default waits.<br>
</blockquote>
<br>
Ok well, let me refine the question: Why does i915 sets a non-default<br>
wait function?<br>
</blockquote>
<br>
I have no idea.<br>
</blockquote>
<br></span>
Can you figure that out? I'm not completely against removing that limitation, but it would be a lot cleaner if we can just fix i915 to not set a non-default wait function.<span class=""><br></span></blockquote><div><br></div><div>I asked on IRC how bad it would be and chris replied with "a lot of work". He didn't say it's impossible but, apparently, it is a pile of work.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
In radeon we have it because we need to handle 10+ years of different<br>
hardware generation, each which a bunch of separate bugs in their fence<br>
handling (and some even not solved by today).<br>
</blockquote>
<br>
So how does wait_any returning -EINVAL for non-default waits help radeon?<br>
</blockquote>
<br></span>
When the wait function detects a problem it reports -EDEADLK to the caller to signal that the hardware is stuck.<br>
<br>
The Caller then goes up the call chain and ultimately reports this to the IOCTL where the error is handled with a hardware reset. And yeah, I perfectly know that this design sucks badly.<br></blockquote><div><br></div><div>Could we use dma_fence::err for this? i.e., could the radeon driver set the error bit and then have wait_any check for errors on wakeup and report the first one it sees?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The point is we should try to prevent that the wait for any function is even used together with a radeon fence.<br></blockquote><div><br></div><div>Does radeon not support sync_file? Because this is the same mechanism it uses for poll.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Patch #6: Yes, please. Patch is Reviewed-by: Christian König<br>
<<a href="mailto:christian.koenig@amd.com" target="_blank">christian.koenig@amd.com</a>>.<br>
<br>
Patch #7: Already gave my rb on the patch Chris send out earlier.<br>
<br>
Patch #8: NAK to the whole approach.<br>
<br>
IIRC we discussed a very similar thing during the initial fence bringup<br>
and also during the fence_array development.<br>
<br>
The problem is that you can easily build ring dependencies and so<br>
deadlocks with it.<br>
<br>
I would really prefer an approach which is completely contained inside<br>
the syncobj code base.<br>
</blockquote>
<br>
Are you use to the approach of internally making a proxy so long as<br>
all the proxy code is inside syncobj?<br>
</blockquote>
Yes, that would be a start.<br>
<br>
In general if possible I would rather like to avoid the whole handling<br>
with the proxy/callback altogether, but that possible only works with<br>
wait for any if the waitqueue is global and that wouldn't be ideal either.<br>
</blockquote>
<br>
I'm happy to get rid of the proxies. They did work nicely but I'm not really attached to them<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Is also be happy to go back to the original approach with v3 of the<br>
last patch.<br>
</blockquote>
v3 looked like it should work as well, I would just drop abusing the<br>
fence callback structure for the signaling.<br>
<br>
Ideally we would finally come up with an interface to wait for multiple<br>
waitqueue at the same time, but that probably goes a bit to far.<br>
<br>
For now just use a single linked list to start all processes waiting for<br>
a fence to arrive or something like this.<br>
</blockquote>
<br>
I don't really know what you're suggesting. Patch v3 has a single waitqueue per process. Are you suggesting one per fence?<br>
</blockquote>
<br></div></div>
Yeah, more or less. What I'm suggesting is to use one wait_queue_head_t per drm_syncobj.<br>
<br>
See a wait_queue is a callback mechanism anyway, so you are wrapping a callback mechanism inside another callback mechanism and that makes not really much sense.<br></blockquote><div><br></div><div>Fair enough. There is one little snag though: We need to wait on sync objects and fences at the same time in order for WAIT_ANY | WAIT_FOR_SUBMIT to work. I see two options here:<br><br></div><div> 1) Convert dma-fence to use waitqueue instead of its callback mechanism and add a wait_queue_any. A quick grep for dma_fence_add_callback says that this would affect four drivers.<br><br></div><div> 2) Drop waitqueues and go back and fix patch v2 so that it does the wait correctly.<br><br></div><div>--Jason<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The problem is that we don't have a wait_event_* variant which can wait for multiple events. Somebody should really add something like that :)<br>
<br>
Regards,<br>
Christian.<div class="HOEnZb"><div class="h5"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
--Jason<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Regards,<br>
Christian.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Regards,<br>
Christian.<br>
<br>
Am 12.08.2017 um 00:39 schrieb Jason Ekstrand:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
This series does the same thing as my earlier series in that it adds<br>
a sync<br>
object wait interface complete with WAIT_FOR_SUBMIT flag. While the<br>
uapi<br>
remains unchanged, the guts look a bit different. Instead of adding a<br>
callback mechanism to drm_syncobj that fired whenever replace_fence was<br>
called, it's now using proxy fences. The drm_syncobj_fence_get still<br>
returns NULL whenever the sync object is in an unsubmitted state but<br>
there<br>
is a new drm_syncobj_fence_proxy_get which returns either the real<br>
fence or<br>
a proxy fence that will be triggered the next time replace_fence is<br>
called<br>
with a non-NULL replacement. This does make both<br>
drm_syncobj_fence_get and<br>
drm_syncobj_replace_fence a tiny bit more expensive, but it lets us<br>
do it<br>
all without locking.<br>
<br>
This series can be found as a branch here:<br>
<br>
<a href="https://cgit.freedesktop.org/~jekstrand/linux/log/?h=drm-syncobj-wait-submit-v4" rel="noreferrer" target="_blank">https://cgit.freedesktop.org/~<wbr>jekstrand/linux/log/?h=drm-syn<wbr>cobj-wait-submit-v4</a> <br>
<br>
<br>
IGT tests for DRM_IOCTL_SYNCOBJ_WAIT and DRM_IOCTL_SYNCOBJ_RESET can be<br>
found on patchwork here:<br>
<br>
<a href="https://patchwork.freedesktop.org/series/28666/" rel="noreferrer" target="_blank">https://patchwork.freedesktop.<wbr>org/series/28666/</a><br>
<br>
Patches to the Intel Vulkan driver to implement<br>
VK_KHR_external_fence on<br>
top of this kernel interface can be found here:<br>
<br>
<a href="https://cgit.freedesktop.org/~jekstrand/mesa/log/?h=wip/anv-external-fence" rel="noreferrer" target="_blank">https://cgit.freedesktop.org/~<wbr>jekstrand/mesa/log/?h=wip/anv-<wbr>external-fence</a> <br>
<br>
<br>
Cc: Dave Airlie <<a href="mailto:airlied@redhat.com" target="_blank">airlied@redhat.com</a>><br>
Cc: Chris Wilson <<a href="mailto:chris@chris-wilson.co.uk" target="_blank">chris@chris-wilson.co.uk</a>><br>
Cc: Christian König <<a href="mailto:christian.koenig@amd.com" target="_blank">christian.koenig@amd.com</a>><br>
<br>
Chris Wilson (2):<br>
dma-buf/dma-fence: Signal all callbacks from dma_fence_release()<br>
dma-buf/dma-fence: Add a mechanism for proxy fences<br>
<br>
Dave Airlie (1):<br>
drm/syncobj: add sync obj wait interface. (v8)<br>
<br>
Jason Ekstrand (6):<br>
drm/syncobj: Rename fence_get to find_fence<br>
drm/syncobj: Add a race-free drm_syncobj_fence_get helper<br>
i915: Add support for drm syncobjs<br>
dma-buf/dma-fence: Allow wait_any_timeout without default_wait (v2)<br>
drm/syncobj: Add a reset ioctl<br>
drm/syncobj: Allow wait for submit and signal behavior (v4)<br>
<br>
drivers/dma-buf/Makefile | 4 +-<br>
drivers/dma-buf/dma-fence-prox<wbr>y.c | 186 +++++++++++++++++++<br>
drivers/dma-buf/dma-fence.c | 34 ++--<br>
drivers/gpu/drm/amd/amdgpu/amd<wbr>gpu_cs.c | 2 +-<br>
drivers/gpu/drm/drm_internal.<wbr>h | 4 +<br>
drivers/gpu/drm/drm_ioctl.c | 4 +<br>
drivers/gpu/drm/drm_syncobj.c | 275<br>
+++++++++++++++++++++++++++--<br>
drivers/gpu/drm/i915/i915_drv.<wbr>c | 3 +-<br>
drivers/gpu/drm/i915/i915_gem_<wbr>execbuffer.c | 146 ++++++++++++++-<br>
include/drm/drm_syncobj.h | 15 +-<br>
include/linux/dma-fence-proxy.<wbr>h | 25 +++<br>
include/uapi/drm/drm.h | 19 ++<br>
include/uapi/drm/i915_drm.h | 30 +++-<br>
13 files changed, 710 insertions(+), 37 deletions(-)<br>
create mode 100644 drivers/dma-buf/dma-fence-prox<wbr>y.c<br>
create mode 100644 include/linux/dma-fence-proxy.<wbr>h<br>
<br>
</blockquote>
<br>
</blockquote>
<br>
<br>
</blockquote>
<br>
</blockquote>
<br>
<br>
</blockquote>
<br>
</div></div></blockquote></div><br></div></div>