Re: [PATCH v2] drm/amdgpu: set start timestamp of fence in the right place

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Am 10.07.24 um 09:54 schrieb Zhu, Jiadong:
> [AMD Official Use Only - AMD Internal Distribution Only]
>
>> -----Original Message-----
>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>> Sent: Wednesday, July 10, 2024 3:17 PM
>> To: Zhu, Jiadong <Jiadong.Zhu@amd.com>; amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH v2] drm/amdgpu: set start timestamp of fence in the
>> right place
>>
>> Am 10.07.24 um 02:31 schrieb jiadong.zhu@amd.com:
>>> From: Jiadong Zhu <Jiadong.Zhu@amd.com>
>>>
>>> The job's embedded fence is dma_fence which shall not be conversed to
>>> amdgpu_fence.
>> Good catch.
>>
>>> The start timestamp shall be saved on job for hw_fence.
>> But NAK to that approach. Why do we need the start time here in the first
>> place?
>>
>> Regards,
>> Christian.
>>
> The start timestamp is used for ring mux to check if the fences are  unsignaled for a period of time under mcbp scenarios (by calling amdgpu_fence_last_unsignaled_time_us).

I can't find a reason for doing that in the first place. What is the 
background of this?

Regards,
Christian.


>
> Thanks,
> Jiadong
>>> v2: optimize get_fence_start_time.
>>> Signed-off-by: Jiadong Zhu <Jiadong.Zhu@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 31
>> ++++++++++++++++++++---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_job.h   |  3 +++
>>>    2 files changed, 31 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>> index 2f24a6aa13bf..72bb007e48c8 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
>>> @@ -88,6 +88,31 @@ static inline struct amdgpu_fence
>> *to_amdgpu_fence(struct dma_fence *f)
>>>      return NULL;
>>>    }
>>>
>>> +static inline void set_fence_start_time(struct dma_fence *f, ktime_t
>>> +start_timestamp) {
>>> +   if (f->ops == &amdgpu_fence_ops) {
>>> +           struct amdgpu_fence *__f = container_of(f, struct
>> amdgpu_fence,
>>> +base);
>>> +
>>> +           __f->start_timestamp = start_timestamp;
>>> +   } else if (f->ops == &amdgpu_job_fence_ops) {
>>> +           struct amdgpu_job *job = container_of(f, struct amdgpu_job,
>>> +hw_fence);
>>> +
>>> +           job->start_timestamp = start_timestamp;
>>> +   }
>>> +}
>>> +
>>> +static inline ktime_t get_fence_start_time(struct dma_fence *f) {
>>> +   if (unlikely(f->ops == &amdgpu_fence_ops)) {
>>> +           struct amdgpu_fence *__f = container_of(f, struct
>> amdgpu_fence,
>>> +base);
>>> +
>>> +           return __f->start_timestamp;
>>> +   }
>>> +   struct amdgpu_job *job = container_of(f, struct amdgpu_job,
>>> +hw_fence);
>>> +
>>> +   return job->start_timestamp;
>>> +}
>>> +
>>>    /**
>>>     * amdgpu_fence_write - write a fence value
>>>     *
>>> @@ -197,7 +222,7 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring,
>> struct dma_fence **f, struct amd
>>>              }
>>>      }
>>>
>>> -   to_amdgpu_fence(fence)->start_timestamp = ktime_get();
>>> +   set_fence_start_time(fence, ktime_get());
>>>
>>>      /* This function can't be called concurrently anyway, otherwise
>>>       * emitting the fence would mess up the hardware ring buffer.
>>> @@ -428,7 +453,7 @@ u64
>> amdgpu_fence_last_unsignaled_time_us(struct amdgpu_ring *ring)
>>>              return 0;
>>>
>>>      return ktime_us_delta(ktime_get(),
>>> -           to_amdgpu_fence(fence)->start_timestamp);
>>> +           get_fence_start_time(fence));
>>>    }
>>>
>>>    /**
>>> @@ -451,7 +476,7 @@ void
>> amdgpu_fence_update_start_timestamp(struct amdgpu_ring *ring,
>> uint32_t seq,
>>>      if (!fence)
>>>              return;
>>>
>>> -   to_amdgpu_fence(fence)->start_timestamp = timestamp;
>>> +   set_fence_start_time(fence, timestamp);
>>>    }
>>>
>>>    /**
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
>>> index a963a25ddd62..3a73fe11a1ce 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
>>> @@ -73,6 +73,9 @@ struct amdgpu_job {
>>>      uint64_t                gds_va;
>>>      bool                    init_shadow;
>>>
>>> +   /* start timestamp for hw_fence*/
>>> +   ktime_t                 start_timestamp;
>>> +
>>>      /* job_run_counter >= 1 means a resubmit job */
>>>      uint32_t                job_run_counter;
>>>