Re: [PATCH v2 1/4] drm/sched: Enable signaling for finished fence

[Andrey Grodzovsky]

On 2022-09-07 02:37, Christian König wrote:
> Am 06.09.22 um 21:55 schrieb Andrey Grodzovsky:
> > On 2022-09-06 02:34, Christian König wrote:
> > > Am 05.09.22 um 18:34 schrieb Arvind Yadav:
> > > > Here's enabling software signaling for finished fence.
> > > >
> > > > Signed-off-by: Arvind Yadav <Arvind.Yadav@xxxxxxx>
> > > > ---
> > > >
> > > > Changes in v1 :
> > > > 1- Addressing Christian's comment to remove CONFIG_DEBUG_FS check from
> > > > this patch.
> > > > 2- The version of this patch is also changed and previously
> > > > it was [PATCH 2/4]
> > > >
> > > > ---
> > > >   drivers/gpu/drm/scheduler/sched_main.c | 2 ++
> > > >   1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
> > > > b/drivers/gpu/drm/scheduler/sched_main.c
> > > > index e0ab14e0fb6b..fe72de0e2911 100644
> > > > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > > > @@ -962,6 +962,8 @@ static int drm_sched_main(void *param)
> > > >               /* Drop for original kref_init of the fence */
> > > >               dma_fence_put(fence);
> > > >   + dma_fence_enable_sw_signaling(&s_fence->finished);
> > >
> > > Ok, this makes it a lot clearer. Previously I though that we have 
> > > some bug in dma_fence_add_callback().
> > >
> > > This is essentially the wrong place to call this, the finished fence 
> > > should be enabled by the caller and not here.
> > >
> > > There is also another problem in dma_fence_enable_sw_signaling(), it 
> > > returns early when the fence is already signaled:
> > >
> > >         if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> > >                 return;
> > >
> > > Please remove that one first.
> >
> >
> > Why we even need this explicit call if dma_fence_add_callback calls 
> > __dma_fence_enable_signaling anyway ?
>
> Two different fence objects.
>
> The dma_fence_add_callback() is done on the hw fence we get in return 
> of submitting the job.
>
> The dma_fence_enable_sw_signaling() here is done on the finished fence 
> we use to signal the completion externally.
>
> Key point is the finished fence should be used by the frontend drivers 
> which uses the scheduler and not by the scheduler itself.
>
> Christian.


Oh, so we need to explicitly call this because dma_fence_add_callback is 
not always used for finished fence right ?

Yea, then it makes sense that the client needs to manage this since each 
one has his own logic what to do with it.

Andrey


> > Andrey
> >
> >
> > > Thanks,
> > > Christian.
> > >
> > >
> > > > +
> > > >               r = dma_fence_add_callback(fence, &sched_job->cb,
> > > >                              drm_sched_job_done_cb);
> > > >               if (r == -ENOENT)