[PATCH 1/3] dma-buf: make the flags can be configured during dma fence init

[Huang, Ray]

[AMD Official Use Only]

> -----Original Message-----
> From: Koenig, Christian <Christian.Koenig at amd.com>
> Sent: Tuesday, December 14, 2021 5:24 PM
> To: Huang, Ray <Ray.Huang at amd.com>; dri-devel at lists.freedesktop.org;
> Daniel Vetter <daniel.vetter at ffwll.ch>; Sumit Semwal
> <sumit.semwal at linaro.org>
> Cc: amd-gfx at lists.freedesktop.org; linux-media at vger.kernel.org; Deucher,
> Alexander <Alexander.Deucher at amd.com>; Liu, Monk
> <Monk.Liu at amd.com>
> Subject: Re: [PATCH 1/3] dma-buf: make the flags can be configured during
> dma fence init
> 
> Am 14.12.21 um 10:19 schrieb Huang, Ray:
> > [AMD Official Use Only]
> >
> >> -----Original Message-----
> >> From: Koenig, Christian <Christian.Koenig at amd.com>
> >> Sent: Tuesday, December 14, 2021 4:00 PM
> >> To: Huang, Ray <Ray.Huang at amd.com>; dri-devel at lists.freedesktop.org;
> >> Daniel Vetter <daniel.vetter at ffwll.ch>; Sumit Semwal
> >> <sumit.semwal at linaro.org>
> >> Cc: amd-gfx at lists.freedesktop.org; linux-media at vger.kernel.org;
> >> Deucher, Alexander <Alexander.Deucher at amd.com>; Liu, Monk
> >> <Monk.Liu at amd.com>
> >> Subject: Re: [PATCH 1/3] dma-buf: make the flags can be configured
> >> during dma fence init
> >>
> >> Am 13.12.21 um 07:34 schrieb Huang Rui:
> >>> In some user scenarios, the get_timeline_name callback uses the
> >>> flags to decide which way to return the timeline string name. Once
> >>> the trace_dma_fence_init event is enabled, it will call
> >>> get_timeline_name callback to dump the fence structure. However, at
> >>> this moment, the flags are always 0, and it might trigger some
> >>> issues in get_timeline_name callback implementation of different gpu
> >>> driver. So make a member to initialize the flags in dma_fence_init().
> >> Well that doesn't make much sense to me.
> >>
> >> None of the dma_fence callbacks is called from the dma_fence_init
> >> function (or at least shouldn't). So drivers always have the
> >> opportunity to to adjust the flags.
> >>
> >> So please explain the rational again?
> > Once we enable trace_dma_fence_init event, we will call get_driver_name
> and get_timeline_name callback function to dump the names in
> dma_fence_init().
> > At that time, the flags are always 0. However, in
> amdgpu_fence_get_timeline_name(), it will check the flags
> (AMDGPU_FENCE_FLAG_EMBED_IN_JOB_BIT) to select which way to get
> the ring.
> > If the fence should be actually embedded in the job (will be set after that),
> it still will trigger a kernel panic (please see patch2) because it go with a
> wrong way. Because we cannot set the flags at the start of dma_fence_init.
> That is the problem.
> 
> Well then I think we should fix the whole approach instead because what
> you try to do here is utterly nonsense. You can't modify the ops structure on
> the fly because that is used by all the fences.
> 
> Instead please just duplicate the amdgpu_fence_ops() and separate them
> into two structure, one for each case.
> 
> This way we should also be able to completely drop the
> AMDGPU_FENCE_FLAG_EMBED_IN_JOB_BIT flag.
> 

OK, you mean this flag is not one of them in standard dma_fence_flag_bits and it AMD specific, so we would better to drop this to align the dam_fence structure design?

Thanks,
Ray

> Regards,
> Christian.
> 
> >
> > Thanks,
> > Ray
> >
> >> Christian.
> >>
> >>> Signed-off-by: Huang Rui <ray.huang at amd.com>
> >>> ---
> >>>    drivers/dma-buf/dma-fence.c | 2 +-
> >>>    include/linux/dma-fence.h   | 7 +++++++
> >>>    2 files changed, 8 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/dma-buf/dma-fence.c
> >>> b/drivers/dma-buf/dma-fence.c index 066400ed8841..3e0622bf385f
> >>> 100644
> >>> --- a/drivers/dma-buf/dma-fence.c
> >>> +++ b/drivers/dma-buf/dma-fence.c
> >>> @@ -952,7 +952,7 @@ dma_fence_init(struct dma_fence *fence, const
> >> struct dma_fence_ops *ops,
> >>>    	fence->lock = lock;
> >>>    	fence->context = context;
> >>>    	fence->seqno = seqno;
> >>> -	fence->flags = 0UL;
> >>> +	fence->flags = ops->init_flags;
> >>>    	fence->error = 0;
> >>>
> >>>    	trace_dma_fence_init(fence);
> >>> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> >>> index 1ea691753bd3..f9270c5bc07a 100644
> >>> --- a/include/linux/dma-fence.h
> >>> +++ b/include/linux/dma-fence.h
> >>> @@ -131,6 +131,13 @@ struct dma_fence_ops {
> >>>    	 */
> >>>    	bool use_64bit_seqno;
> >>>
> >>> +	/**
> >>> +	 * @init_flags:
> >>> +	 *
> >>> +	 * The initial value of fence flags (A mask of DMA_FENCE_FLAG_*
> >> defined).
> >>> +	 */
> >>> +	unsigned long init_flags;
> >>> +
> >>>    	/**
> >>>    	 * @get_driver_name:
> >>>    	 *