Re: [PATCH RFC 4/5] drm/amdgpu: Add accounting of command submission via DRM cgroup

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

Am 20.11.18 um 19:58 schrieb Kenny Ho:
> Account for the number of command submitted to amdgpu by type on a per
> cgroup basis, for the purpose of profiling/monitoring applications.
>
> x prefix in the control file name x.cmd_submitted.amd.stat signify
> experimental.
>
> Change-Id: Ibc22e5bda600f54fe820fe0af5400ca348691550
> Signed-off-by: Kenny Ho <Kenny.Ho@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c      |  5 ++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drmcgrp.c | 54 +++++++++++++++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drmcgrp.h |  5 ++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c    | 15 ++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h    |  5 +-
>   5 files changed, 83 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 663043c8f0f5..b448160aed89 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -33,6 +33,7 @@
>   #include "amdgpu_trace.h"
>   #include "amdgpu_gmc.h"
>   #include "amdgpu_gem.h"
> +#include "amdgpu_drmcgrp.h"
>   
>   static int amdgpu_cs_user_fence_chunk(struct amdgpu_cs_parser *p,
>   				      struct drm_amdgpu_cs_chunk_fence *data,
> @@ -1275,6 +1276,7 @@ int amdgpu_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>   	union drm_amdgpu_cs *cs = data;
>   	struct amdgpu_cs_parser parser = {};
>   	bool reserved_buffers = false;
> +	struct amdgpu_ring *ring;
>   	int i, r;
>   
>   	if (!adev->accel_working)
> @@ -1317,6 +1319,9 @@ int amdgpu_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>   	if (r)
>   		goto out;
>   
> +	ring = to_amdgpu_ring(parser.entity->rq->sched);
> +	amdgpu_drmcgrp_count_cs(current, dev, ring->funcs->type);
> +
>   	r = amdgpu_cs_submit(&parser, cs);
>   
>   out:
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drmcgrp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drmcgrp.c
> index ed8aac17769c..853b77532428 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drmcgrp.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drmcgrp.c
> @@ -1,11 +1,65 @@
>   // SPDX-License-Identifier: MIT
>   // Copyright 2018 Advanced Micro Devices, Inc.
>   #include <linux/slab.h>
> +#include <linux/mutex.h>
>   #include <linux/cgroup_drm.h>
>   #include <drm/drm_device.h>
> +#include "amdgpu_ring.h"
>   #include "amdgpu_drmcgrp.h"
>   
> +void amdgpu_drmcgrp_count_cs(struct task_struct *task, struct drm_device *dev,
> +		enum amdgpu_ring_type r_type)
> +{
> +	struct drmcgrp *drmcgrp = get_drmcgrp(task);
> +	struct drmcgrp_device_resource *ddr;
> +	struct drmcgrp *p;
> +	struct amd_drmcgrp_dev_resource *a_ddr;
> +
> +	if (drmcgrp == NULL)
> +		return;
> +
> +	ddr = drmcgrp->dev_resources[dev->primary->index];
> +
> +	mutex_lock(&ddr->ddev->mutex);
> +	for (p = drmcgrp; p != NULL; p = parent_drmcgrp(drmcgrp)) {
> +		a_ddr = ddr_amdddr(p->dev_resources[dev->primary->index]);
> +
> +		a_ddr->cs_count[r_type]++;
> +	}
> +	mutex_unlock(&ddr->ddev->mutex);
> +}
> +
> +int amd_drmcgrp_cmd_submit_accounting_read(struct seq_file *sf, void *v)
> +{
> +	struct drmcgrp *drmcgrp = css_drmcgrp(seq_css(sf));
> +	struct drmcgrp_device_resource *ddr = NULL;
> +	struct amd_drmcgrp_dev_resource *a_ddr = NULL;
> +	int i, j;
> +
> +	seq_puts(sf, "---\n");
> +	for (i = 0; i < MAX_DRM_DEV; i++) {
> +		ddr = drmcgrp->dev_resources[i];
> +
> +		if (ddr == NULL || ddr->ddev->vid != amd_drmcgrp_vendor_id)
> +			continue;
> +
> +		a_ddr = ddr_amdddr(ddr);
> +
> +		seq_printf(sf, "card%d:\n", i);
> +		for (j = 0; j < __MAX_AMDGPU_RING_TYPE; j++)
> +			seq_printf(sf, "  %s: %llu\n", amdgpu_ring_names[j], a_ddr->cs_count[j]);
> +	}
> +
> +	return 0;
> +}
> +
> +
>   struct cftype files[] = {
> +	{
> +		.name = "x.cmd_submitted.amd.stat",
> +		.seq_show = amd_drmcgrp_cmd_submit_accounting_read,
> +		.flags = CFTYPE_NOT_ON_ROOT,
> +	},
>   	{ } /* terminate */
>   };
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drmcgrp.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_drmcgrp.h
> index e2934b7a49f5..f894a9a1059f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drmcgrp.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drmcgrp.h
> @@ -5,12 +5,17 @@
>   #define _AMDGPU_DRMCGRP_H
>   
>   #include <linux/cgroup_drm.h>
> +#include "amdgpu_ring.h"
>   
>   /* for AMD specific DRM resources */
>   struct amd_drmcgrp_dev_resource {
>   	struct drmcgrp_device_resource ddr;
> +	u64 cs_count[__MAX_AMDGPU_RING_TYPE];
>   };
>   
> +void amdgpu_drmcgrp_count_cs(struct task_struct *task, struct drm_device *dev,
> +		enum amdgpu_ring_type r_type);
> +
>   static inline struct amd_drmcgrp_dev_resource *ddr_amdddr(struct drmcgrp_device_resource *ddr)
>   {
>   	return ddr ? container_of(ddr, struct amd_drmcgrp_dev_resource, ddr) : NULL;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> index b70e85ec147d..1606f84d2334 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> @@ -34,6 +34,21 @@
>   #include "amdgpu.h"
>   #include "atom.h"
>   
> +char const *amdgpu_ring_names[] = {
> +	[AMDGPU_RING_TYPE_GFX]		= "gfx",
> +	[AMDGPU_RING_TYPE_COMPUTE]	= "compute",
> +	[AMDGPU_RING_TYPE_SDMA]		= "sdma",
> +	[AMDGPU_RING_TYPE_UVD]		= "uvd",
> +	[AMDGPU_RING_TYPE_VCE]		= "vce",
> +	[AMDGPU_RING_TYPE_KIQ]		= "kiq",
> +	[AMDGPU_RING_TYPE_UVD_ENC]	= "uvd_enc",
> +	[AMDGPU_RING_TYPE_VCN_DEC]	= "vcn_dec",
> +	[AMDGPU_RING_TYPE_VCN_ENC]	= "vcn_enc",
> +	[AMDGPU_RING_TYPE_VCN_JPEG]	= "vcn_jpeg",
> +	[__MAX_AMDGPU_RING_TYPE]	= "_max"
> +
> +};
> +

Each ring already has a dedicated name, so that looks like a duplication 
to me.

What could be is that we need this for the ring type name, but then you 
need to rename it.

And please don't use something like "__MAX_AMDGPU....", just name that 
AMDGPU_RING_TYPE_LAST.

Christian.

>   /*
>    * Rings
>    * Most engines on the GPU are fed via ring buffers.  Ring
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> index 4caa301ce454..e292b7e89c6a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> @@ -56,9 +56,12 @@ enum amdgpu_ring_type {
>   	AMDGPU_RING_TYPE_UVD_ENC,
>   	AMDGPU_RING_TYPE_VCN_DEC,
>   	AMDGPU_RING_TYPE_VCN_ENC,
> -	AMDGPU_RING_TYPE_VCN_JPEG
> +	AMDGPU_RING_TYPE_VCN_JPEG,
> +	__MAX_AMDGPU_RING_TYPE
>   };
>   
> +extern char const *amdgpu_ring_names[];
> +
>   struct amdgpu_device;
>   struct amdgpu_ring;
>   struct amdgpu_ib;

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