| Message ID | 20200512085944.222637-2-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dma-fence lockdep annotations | expand |
Quoting Daniel Vetter (2020-05-12 09:59:28) > But only for non-zero timeout, to avoid false positives. > > One question here is whether the might_sleep should be unconditional, > or only for real timeouts. I'm not sure, so went with the more > defensive option. But in the interest of locking down the cross-driver > dma_fence rules we might want to be more aggressive. You can argue for enforcing might_sleep() as internal queries should be using dma_fence_is_signaled() and not dma_fence_wait_timeout(0). > Cc: linux-media@vger.kernel.org > Cc: linaro-mm-sig@lists.linaro.org > Cc: linux-rdma@vger.kernel.org > Cc: amd-gfx@lists.freedesktop.org > Cc: intel-gfx@lists.freedesktop.org > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Christian König <christian.koenig@amd.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/dma-buf/dma-fence.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > index 052a41e2451c..6802125349fb 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -208,6 +208,9 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout) > if (WARN_ON(timeout < 0)) > return -EINVAL; > > + if (timeout > 0) > + might_sleep(); might_sleep_if(timeout > 0); -Chris
Am 12.05.20 um 10:59 schrieb Daniel Vetter: > But only for non-zero timeout, to avoid false positives. > > One question here is whether the might_sleep should be unconditional, > or only for real timeouts. I'm not sure, so went with the more > defensive option. But in the interest of locking down the cross-driver > dma_fence rules we might want to be more aggressive. > > Cc: linux-media@vger.kernel.org > Cc: linaro-mm-sig@lists.linaro.org > Cc: linux-rdma@vger.kernel.org > Cc: amd-gfx@lists.freedesktop.org > Cc: intel-gfx@lists.freedesktop.org > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Christian König <christian.koenig@amd.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/dma-buf/dma-fence.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > index 052a41e2451c..6802125349fb 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -208,6 +208,9 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout) > if (WARN_ON(timeout < 0)) > return -EINVAL; > > + if (timeout > 0) > + might_sleep(); > + I would rather like to see might_sleep() called here all the time even with timeout==0. IIRC I removed the code in TTM abusing this in atomic context quite a while ago, but could be that some leaked in again or it is called in atomic context elsewhere as well. Christian. > trace_dma_fence_wait_start(fence); > if (fence->ops->wait) > ret = fence->ops->wait(fence, intr, timeout);
Op 12-05-2020 om 11:08 schreef Christian König: > Am 12.05.20 um 10:59 schrieb Daniel Vetter: >> But only for non-zero timeout, to avoid false positives. >> >> One question here is whether the might_sleep should be unconditional, >> or only for real timeouts. I'm not sure, so went with the more >> defensive option. But in the interest of locking down the cross-driver >> dma_fence rules we might want to be more aggressive. >> >> Cc: linux-media@vger.kernel.org >> Cc: linaro-mm-sig@lists.linaro.org >> Cc: linux-rdma@vger.kernel.org >> Cc: amd-gfx@lists.freedesktop.org >> Cc: intel-gfx@lists.freedesktop.org >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Cc: Christian König <christian.koenig@amd.com> >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> >> --- >> drivers/dma-buf/dma-fence.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c >> index 052a41e2451c..6802125349fb 100644 >> --- a/drivers/dma-buf/dma-fence.c >> +++ b/drivers/dma-buf/dma-fence.c >> @@ -208,6 +208,9 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout) >> if (WARN_ON(timeout < 0)) >> return -EINVAL; >> + if (timeout > 0) >> + might_sleep(); >> + > > I would rather like to see might_sleep() called here all the time even with timeout==0. > > IIRC I removed the code in TTM abusing this in atomic context quite a while ago, but could be that some leaked in again or it is called in atomic context elsewhere as well. Same, glad I'm not the only one who wants it. :) ~Maarten
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 052a41e2451c..6802125349fb 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -208,6 +208,9 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout) if (WARN_ON(timeout < 0)) return -EINVAL; + if (timeout > 0) + might_sleep(); + trace_dma_fence_wait_start(fence); if (fence->ops->wait) ret = fence->ops->wait(fence, intr, timeout);
But only for non-zero timeout, to avoid false positives. One question here is whether the might_sleep should be unconditional, or only for real timeouts. I'm not sure, so went with the more defensive option. But in the interest of locking down the cross-driver dma_fence rules we might want to be more aggressive. Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org Cc: linux-rdma@vger.kernel.org Cc: amd-gfx@lists.freedesktop.org Cc: intel-gfx@lists.freedesktop.org Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Christian König <christian.koenig@amd.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/dma-buf/dma-fence.c | 3 +++ 1 file changed, 3 insertions(+)