Re: [PATCH] drm/amdgpu: Get rid of amdgpu_job->external_hw_fence

[Andrey Grodzovsky <andrey.grodzovsky@xxxxxxx>]

On 2022-07-13 13:33, Christian König wrote:
> Am 13.07.22 um 19:13 schrieb Andrey Grodzovsky:
> > This is a follow-up cleanup to [1]. See bellow refcount balancing
> > for calling amdgpu_job_submit_direct after this cleanup as far
> > as I calculated.
> >
> > amdgpu_fence_emit
> >     dma_fence_init 1
> >     dma_fence_get(fence) 2
> >     rcu_assign_pointer(*ptr, dma_fence_get(fence) 3
> >
> > ---> amdgpu_job_submit_direct completes before fence signaled
> >             amdgpu_sa_bo_free
> >                 (*sa_bo)->fence = dma_fence_get(fence) 4
> >
> >             amdgpu_job_free
> >                 dma_fence_put 3
> >
> >             amdgpu_vcn_enc_get_destroy_msg
> >                 *fence = dma_fence_get(f) 4
> >                 dma_fence_put(f); 3
> >
> >             amdgpu_vcn_enc_ring_test_ib
> >                 dma_fence_put(fence) 2
> >
> >             amdgpu_fence_process
> >                 dma_fence_put 1
> >
> >             amdgpu_sa_bo_remove_locked
> >                 dma_fence_put 0
> >
> > ---> amdgpu_job_submit_direct completes after fence signaled
> >             amdgpu_fence_process
> >                 dma_fence_put 2
> >
> >             amdgpu_job_free
> >                 dma_fence_put 1
> >
> >             amdgpu_vcn_enc_get_destroy_msg
> >                 *fence = dma_fence_get(f) 2
> >                 dma_fence_put(f); 1
> >
> >             amdgpu_vcn_enc_ring_test_ib
> >                 dma_fence_put(fence) 0
> >
> > [1] - 
> > https://patchwork.kernel.org/project/dri-devel/cover/20220624180955.485440-1-andrey.grodzovsky@xxxxxxx/
> >
> > Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky@xxxxxxx>
> > Suggested-by: Christian König <christian.koenig@xxxxxxx>
>
> Of hand that looks correct to me, but could be that I'm missing 
> something as well.
>
> Anyway I think I can give an Reviewed-by: Christian König 
> <christian.koenig@xxxxxxx> for this.
>
> Thanks,
> Christian.


Pushed, thanks.

Andrey


> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  3 +--
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c    | 27 ++++------------------
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_job.h    |  1 -
> >   3 files changed, 6 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index 16faea7ed1cd..b79ee4ffb879 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -5229,8 +5229,7 @@ int amdgpu_device_gpu_recover(struct 
> > amdgpu_device *adev,
> >        *
> >        * job->base holds a reference to parent fence
> >        */
> > -    if (job && (job->hw_fence.ops != NULL) &&
> > -        dma_fence_is_signaled(&job->hw_fence)) {
> > +    if (job && dma_fence_is_signaled(&job->hw_fence)) {
> >           job_signaled = true;
> >           dev_info(adev->dev, "Guilty job already signaled, skipping 
> > HW reset");
> >           goto skip_hw_reset;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > index 6fa381ee5fa0..10fdd12cf853 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > @@ -134,16 +134,10 @@ void amdgpu_job_free_resources(struct 
> > amdgpu_job *job)
> >   {
> >       struct amdgpu_ring *ring = to_amdgpu_ring(job->base.sched);
> >       struct dma_fence *f;
> > -    struct dma_fence *hw_fence;
> >       unsigned i;
> >   -    if (job->hw_fence.ops == NULL)
> > -        hw_fence = job->external_hw_fence;
> > -    else
> > -        hw_fence = &job->hw_fence;
> > -
> >       /* use sched fence if available */
> > -    f = job->base.s_fence ? &job->base.s_fence->finished : hw_fence;
> > +    f = job->base.s_fence ? &job->base.s_fence->finished :  
> > &job->hw_fence;
> >       for (i = 0; i < job->num_ibs; ++i)
> >           amdgpu_ib_free(ring->adev, &job->ibs[i], f);
> >   }
> > @@ -157,11 +151,7 @@ static void amdgpu_job_free_cb(struct 
> > drm_sched_job *s_job)
> >       amdgpu_sync_free(&job->sync);
> >       amdgpu_sync_free(&job->sched_sync);
> >   -    /* only put the hw fence if has embedded fence */
> > -    if (job->hw_fence.ops != NULL)
> > -        dma_fence_put(&job->hw_fence);
> > -    else
> > -        kfree(job);
> > +    dma_fence_put(&job->hw_fence);
> >   }
> >     void amdgpu_job_free(struct amdgpu_job *job)
> > @@ -170,11 +160,7 @@ void amdgpu_job_free(struct amdgpu_job *job)
> >       amdgpu_sync_free(&job->sync);
> >       amdgpu_sync_free(&job->sched_sync);
> >   -    /* only put the hw fence if has embedded fence */
> > -    if (job->hw_fence.ops != NULL)
> > -        dma_fence_put(&job->hw_fence);
> > -    else
> > -        kfree(job);
> > +    dma_fence_put(&job->hw_fence);
> >   }
> >     int amdgpu_job_submit(struct amdgpu_job *job, struct 
> > drm_sched_entity *entity,
> > @@ -204,15 +190,12 @@ int amdgpu_job_submit_direct(struct amdgpu_job 
> > *job, struct amdgpu_ring *ring,
> >       int r;
> >         job->base.sched = &ring->sched;
> > -    r = amdgpu_ib_schedule(ring, job->num_ibs, job->ibs, NULL, fence);
> > -    /* record external_hw_fence for direct submit */
> > -    job->external_hw_fence = dma_fence_get(*fence);
> > +    r = amdgpu_ib_schedule(ring, job->num_ibs, job->ibs, job, fence);
> > +
> >       if (r)
> >           return r;
> >         amdgpu_job_free(job);
> > -    dma_fence_put(*fence);
> > -
> >       return 0;
> >   }
> >   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> > index d599c0540b46..babc0af751c2 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
> > @@ -50,7 +50,6 @@ struct amdgpu_job {
> >       struct amdgpu_sync    sync;
> >       struct amdgpu_sync    sched_sync;
> >       struct dma_fence    hw_fence;
> > -    struct dma_fence    *external_hw_fence;
> >       uint32_t        preamble_status;
> >       uint32_t                preemption_status;
> >       bool                    vm_needs_flush;