[PATCH 2/2] drm/amdgpu: fix fence calculation

[Christian König]

And BTW amdgpu_ctx_fence_time() should probably be static.

Christian.

Am 12.05.21 um 08:55 schrieb Christian König:
> In this case amdgpu_ctx_fence_time should probably be changed to 
> initialize the variable itself.
>
> That is really bad coding style otherwise.
>
> Christian.
>
> Am 11.05.21 um 20:14 schrieb Nieto, David M:
>>
>> [AMD Official Use Only - Internal Distribution Only]
>>
>>
>> The local variables need to be initialized to zero, since 
>> amdgpu_ctx_fence_time accumulates and does not initialize
>>
>> David
>> ------------------------------------------------------------------------
>> *From:* Christian König <ckoenig.leichtzumerken at gmail.com>
>> *Sent:* Tuesday, May 11, 2021 12:53 AM
>> *To:* Nieto, David M <David.Nieto at amd.com>; 
>> amd-gfx at lists.freedesktop.org <amd-gfx at lists.freedesktop.org>
>> *Subject:* Re: [PATCH 2/2] drm/amdgpu: fix fence calculation
>> Am 10.05.21 um 22:29 schrieb David M Nieto:
>> > The proper metric for fence utilization over several
>> > contexts is an harmonic mean, but such calculation is
>> > prohibitive in kernel space, so the code approximates it.
>> >
>> > Because the approximation diverges when one context has a
>> > very small ratio compared with the other context, this change
>> > filter out ratios smaller that 0.01%
>> >
>> > Signed-off-by: David M Nieto <david.nieto at amd.com>
>> > Change-Id: I5b6e0ce5f489a5f55855d35354a6a3653e9d613b
>> > ---
>> >   drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 13 ++++++++++++-
>> >   drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h |  1 +
>> >   2 files changed, 13 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> > index 9036c93b4a0c..89ee464b9424 100644
>> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
>> > @@ -698,16 +698,27 @@ ktime_t amdgpu_ctx_mgr_fence_usage(struct 
>> amdgpu_ctx_mgr *mgr, uint32_t hwip,
>> >        struct amdgpu_ctx_entity *centity;
>> >        ktime_t total = 0, max = 0;
>> >
>> > +
>>
>> Unrelated white space change.
>>
>> >        if (idx >= AMDGPU_MAX_ENTITY_NUM)
>> >                return 0;
>> >        idp = &mgr->ctx_handles;
>> >        mutex_lock(&mgr->lock);
>> >        idr_for_each_entry(idp, ctx, id) {
>> > +             ktime_t ttotal = tmax = ktime_set(0, 0);
>>
>> There should be a blank line between decleration and code and please
>> don't initialize local variables if it isn't necessary.
>>
>> Christian.
>>
>> >                if (!ctx->entities[hwip][idx])
>> >                        continue;
>> >
>> >                centity = ctx->entities[hwip][idx];
>> > -             amdgpu_ctx_fence_time(ctx, centity, &total, &max);
>> > +             amdgpu_ctx_fence_time(ctx, centity, &ttotal, &tmax);
>> > +
>> > +             /* Harmonic mean approximation diverges for very small
>> > +              * values. If ratio < 0.01% ignore
>> > +              */
>> > +             if (AMDGPU_CTX_FENCE_USAGE_MIN_RATIO(tmax, ttotal))
>> > +                     continue;
>> > +
>> > +             total = ktime_add(total, ttotal);
>> > +             max = ktime_after(tmax, max) ? tmax : max;
>> >        }
>> >
>> >        mutex_unlock(&mgr->lock);
>> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
>> > index 10dcf59a5c6b..3541dfb059ec 100644
>> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
>> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
>> > @@ -30,6 +30,7 @@ struct drm_file;
>> >   struct amdgpu_fpriv;
>> >
>> >   #define AMDGPU_MAX_ENTITY_NUM 4
>> > +#define AMDGPU_CTX_FENCE_USAGE_MIN_RATIO(max, total) (max > 
>> 16384ULL*total)
>> >
>> >   struct amdgpu_ctx_entity {
>> >        uint64_t                sequence;
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20210512/15a81628/attachment-0001.htm>