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

["Nieto, David M" <David.Nieto@amd.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4143 bytes --]

[AMD Official Use Only - Internal Distribution Only]

yep, you are right...

I'll make those changes.

David
________________________________
From: Christian König <ckoenig.leichtzumerken@gmail.com>
Sent: Tuesday, May 11, 2021 11:56 PM
To: Nieto, David M <David.Nieto@amd.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 2/2] drm/amdgpu: fix fence calculation

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@gmail.com><mailto:ckoenig.leichtzumerken@gmail.com>
Sent: Tuesday, May 11, 2021 12:53 AM
To: Nieto, David M <David.Nieto@amd.com><mailto:David.Nieto@amd.com>; amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org><mailto:amd-gfx@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@amd.com><mailto:david.nieto@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;




[-- Attachment #1.2: Type: text/html, Size: 9193 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx