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

[Christian König]

Am 14.12.21 um 10:44 schrieb 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?

Well yes and no. We can use driver private flags, it's just that for 
this use case it doesn't make much sense.

See what the functions do, for example amdgpu_fence_enable_signaling():

        if (test_bit(AMDGPU_FENCE_FLAG_EMBED_IN_JOB_BIT, &f->flags)) {
                 struct amdgpu_job *job = container_of(f, struct 
amdgpu_job, hw_fence);

                 ring = to_amdgpu_ring(job->base.sched);
         } else {
                 ring = to_amdgpu_fence(f)->ring;
         }

         if (!timer_pending(&ring->fence_drv.fallback_timer))
                 amdgpu_fence_schedule_fallback(ring);

         return true;

That can much cleaner be done as

static bool amdgpu_fence_enable_signaling(struct dma_fence *f)
{
         if (!timer_pending(&ring->fence_drv.fallback_timer))
amdgpu_fence_schedule_fallback(to_amdgpu_fence(f)->ring);
         return true;
}

static bool amdgpu_job_fence_enable_signaling(struct dma_fence *f)
{
....
}

If we want to avoid the duplication of logic we should just move the 
timer_pending() check into a separate function.

Regards,
Christian.

>
> 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:
>>>>>     	 *