| Message ID | 20200512085944.222637-10-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | dma-fence lockdep annotations | expand |
Am 12.05.20 um 10:59 schrieb Daniel Vetter: > This is a bit tricky, since ->notifier_lock is held while calling > dma_fence_wait we must ensure that also the read side (i.e. > dma_fence_begin_signalling) is on the same side. If we mix this up > lockdep complaints, and that's again why we want to have these > annotations. > > A nice side effect of this is that because of the fs_reclaim priming > for dma_fence_enable lockdep now automatically checks for us that > nothing in here allocates memory, without even running any userptr > workloads. > > 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/gpu/drm/amd/amdgpu/amdgpu_cs.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > index 7653f62b1b2d..6db3f3c629b0 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > @@ -1213,6 +1213,7 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p, > struct amdgpu_job *job; > uint64_t seq; > int r; > + bool fence_cookie; > > job = p->job; > p->job = NULL; > @@ -1227,6 +1228,8 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p, > */ > mutex_lock(&p->adev->notifier_lock); > > + fence_cookie = dma_fence_begin_signalling(); > + > /* If userptr are invalidated after amdgpu_cs_parser_bos(), return > * -EAGAIN, drmIoctl in libdrm will restart the amdgpu_cs_ioctl. > */ > @@ -1264,12 +1267,14 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p, > amdgpu_vm_move_to_lru_tail(p->adev, &fpriv->vm); > > ttm_eu_fence_buffer_objects(&p->ticket, &p->validated, p->fence); > + dma_fence_end_signalling(fence_cookie); Mhm, this could come earlier in theory. E.g. after pushing the job to the scheduler. Christian. > mutex_unlock(&p->adev->notifier_lock); > > return 0; > > error_abort: > drm_sched_job_cleanup(&job->base); > + dma_fence_end_signalling(fence_cookie); > mutex_unlock(&p->adev->notifier_lock); > > error_unlock:
On Wed, May 13, 2020 at 9:02 AM Christian König <ckoenig.leichtzumerken@gmail.com> wrote: > > Am 12.05.20 um 10:59 schrieb Daniel Vetter: > > This is a bit tricky, since ->notifier_lock is held while calling > > dma_fence_wait we must ensure that also the read side (i.e. > > dma_fence_begin_signalling) is on the same side. If we mix this up > > lockdep complaints, and that's again why we want to have these > > annotations. > > > > A nice side effect of this is that because of the fs_reclaim priming > > for dma_fence_enable lockdep now automatically checks for us that > > nothing in here allocates memory, without even running any userptr > > workloads. > > > > 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/gpu/drm/amd/amdgpu/amdgpu_cs.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > > index 7653f62b1b2d..6db3f3c629b0 100644 > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > > @@ -1213,6 +1213,7 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p, > > struct amdgpu_job *job; > > uint64_t seq; > > int r; > > + bool fence_cookie; > > > > job = p->job; > > p->job = NULL; > > @@ -1227,6 +1228,8 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p, > > */ > > mutex_lock(&p->adev->notifier_lock); > > > > + fence_cookie = dma_fence_begin_signalling(); > > + > > /* If userptr are invalidated after amdgpu_cs_parser_bos(), return > > * -EAGAIN, drmIoctl in libdrm will restart the amdgpu_cs_ioctl. > > */ > > @@ -1264,12 +1267,14 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p, > > amdgpu_vm_move_to_lru_tail(p->adev, &fpriv->vm); > > > > ttm_eu_fence_buffer_objects(&p->ticket, &p->validated, p->fence); > > + dma_fence_end_signalling(fence_cookie); > > Mhm, this could come earlier in theory. E.g. after pushing the job to > the scheduler. Yeah, I have not much clue about how amdgpu works :-) In practice it doesn't matter much, since the enclosing adev->notifier_lock is a lot more strict about what it allows than the dma_fence signalling fake lock. -Daniel > > Christian. > > > mutex_unlock(&p->adev->notifier_lock); > > > > return 0; > > > > error_abort: > > drm_sched_job_cleanup(&job->base); > > + dma_fence_end_signalling(fence_cookie); > > mutex_unlock(&p->adev->notifier_lock); > > > > error_unlock: >
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index 7653f62b1b2d..6db3f3c629b0 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -1213,6 +1213,7 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p, struct amdgpu_job *job; uint64_t seq; int r; + bool fence_cookie; job = p->job; p->job = NULL; @@ -1227,6 +1228,8 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p, */ mutex_lock(&p->adev->notifier_lock); + fence_cookie = dma_fence_begin_signalling(); + /* If userptr are invalidated after amdgpu_cs_parser_bos(), return * -EAGAIN, drmIoctl in libdrm will restart the amdgpu_cs_ioctl. */ @@ -1264,12 +1267,14 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p, amdgpu_vm_move_to_lru_tail(p->adev, &fpriv->vm); ttm_eu_fence_buffer_objects(&p->ticket, &p->validated, p->fence); + dma_fence_end_signalling(fence_cookie); mutex_unlock(&p->adev->notifier_lock); return 0; error_abort: drm_sched_job_cleanup(&job->base); + dma_fence_end_signalling(fence_cookie); mutex_unlock(&p->adev->notifier_lock); error_unlock:
This is a bit tricky, since ->notifier_lock is held while calling dma_fence_wait we must ensure that also the read side (i.e. dma_fence_begin_signalling) is on the same side. If we mix this up lockdep complaints, and that's again why we want to have these annotations. A nice side effect of this is that because of the fs_reclaim priming for dma_fence_enable lockdep now automatically checks for us that nothing in here allocates memory, without even running any userptr workloads. 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/gpu/drm/amd/amdgpu/amdgpu_cs.c | 5 +++++ 1 file changed, 5 insertions(+)