On 14/05/2019 14:31, Rob Herring wrote:
> On Tue, May 14, 2019 at 5:48 AM Boris Brezillon
> <boris.brezillon@xxxxxxxxxxxxx> wrote:
>>
>> Add a way to dump perf counters through debugfs. The implementation is
>> kept simple and has a number of limitations:
>>
>> * it's not designed for multi-user usage as the counter values are
>> reset after each dump and there's no per-user context
>> * only accessible to root users
>> * no counters naming/position abstraction. Things are dumped in a raw
>> format that has to be parsed by the user who has to know where the
>> relevant values are and what they mean
>>
>> This implementation is intended to be used by mesa developers to help
>> debug perf-related issues while we work on a more generic approach that
>> would allow all GPU drivers to expose their counters in a consistent
>> way. As a result, this debugfs interface is considered unstable and
>> might be deprecated in the future.
>>
>> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
>> ---
>> Changes in v2:
>> * Expose counters through debugfs and keep things simple for now (we'll
>> work on a generic solution in parallel)
>> ---
>> drivers/gpu/drm/panfrost/Makefile | 3 +-
>> drivers/gpu/drm/panfrost/panfrost_device.c | 9 +
>> drivers/gpu/drm/panfrost/panfrost_device.h | 3 +
>> drivers/gpu/drm/panfrost/panfrost_drv.c | 7 +
>> drivers/gpu/drm/panfrost/panfrost_gpu.c | 7 +
>> drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 339 ++++++++++++++++++++
>> drivers/gpu/drm/panfrost/panfrost_perfcnt.h | 15 +
>> drivers/gpu/drm/panfrost/panfrost_regs.h | 19 ++
>> 8 files changed, 401 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/gpu/drm/panfrost/panfrost_perfcnt.c
>> create mode 100644 drivers/gpu/drm/panfrost/panfrost_perfcnt.h
>>
[...]
>> new file mode 100644
>> index 000000000000..8093783a5a26
>> --- /dev/null
>> +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
>> @@ -0,0 +1,339 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright 2019 Collabora Ltd */
>> +
>> +#include <drm/drm_file.h>
>> +#include <drm/drm_gem_shmem_helper.h>
>> +#include <drm/panfrost_drm.h>
>> +#include <linux/completion.h>
>> +#include <linux/debugfs.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/slab.h>
>> +#include <linux/uaccess.h>
>> +
>> +#include "panfrost_device.h"
>> +#include "panfrost_features.h"
>> +#include "panfrost_gem.h"
>> +#include "panfrost_issues.h"
>> +#include "panfrost_job.h"
>> +#include "panfrost_mmu.h"
>> +#include "panfrost_regs.h"
>> +
>> +#define COUNTERS_PER_BLOCK 64
>> +#define BYTES_PER_COUNTER 4
>> +#define BLOCKS_PER_COREGROUP 8
>> +#define V4_SHADERS_PER_COREGROUP 4
>> +
>> +struct panfrost_perfcnt {
>> + struct panfrost_gem_object *bo;
>> + size_t bosize;
>> + void *buf;
>> + bool enabled;
>> + struct mutex lock;
>> + struct completion dump_comp;
>> +};
>> +
>> +void panfrost_perfcnt_clean_cache_done(struct panfrost_device *pfdev)
>> +{
>> + complete(&pfdev->perfcnt->dump_comp);
>> +}
>> +
>> +void panfrost_perfcnt_sample_done(struct panfrost_device *pfdev)
>> +{
>> + gpu_write(pfdev, GPU_CMD, GPU_CMD_CLEAN_CACHES);
>> +}
>> +
>> +static void panfrost_perfcnt_setup(struct panfrost_device *pfdev)
>> +{
>> + u32 cfg;
>> +
>> + /*
>> + * Always use address space 0 for now.
>> + * FIXME: this needs to be updated when we start using different
>> + * address space.
>> + */
>> + cfg = GPU_PERFCNT_CFG_AS(0);
>> + if (panfrost_model_cmp(pfdev, 0x1000) >= 0)
>
> You've got a couple of these. Perhaps we should add either a
> model_is_bifrost() helper or an is_bifrost variable to use.
>
>> + cfg |= GPU_PERFCNT_CFG_SETSEL(1);
mali_kbase only sets this if CONFIG_MALI_PRFCNT_SET_SECONDARY is defined
- i.e. this selects a different selection of counters. At at the very
least this should be an optional flag that can be set, but I suspect the
primary set are the ones you are interested in.
>> +
>> + gpu_write(pfdev, GPU_PERFCNT_CFG,
>> + cfg | GPU_PERFCNT_CFG_MODE(GPU_PERFCNT_CFG_MODE_OFF));
>> +
>> + if (!pfdev->perfcnt->enabled)
>> + return;
>> +
>> + gpu_write(pfdev, GPU_PRFCNT_JM_EN, 0xffffffff);
>> + gpu_write(pfdev, GPU_PRFCNT_SHADER_EN, 0xffffffff);
>> + gpu_write(pfdev, GPU_PRFCNT_MMU_L2_EN, 0xffffffff);
>> +
>> + /*
>> + * Due to PRLAM-8186 we need to disable the Tiler before we enable HW
>> + * counters.
>
> Seems like you could just always apply the work-around? It doesn't
> look like it would be much different.
Technically the workaround causes the driver to perform the operation in
the wrong order below (write TILER_EN when the mode is MANUAL) - this is
a workaround for the hardware issue (and therefore works on that
hardware). But I wouldn't like to say it works on other hardware which
doesn't have the issue.
>> + */
>> + if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8186))
>> + gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0);
>> + else
>> + gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0xffffffff);
>> +
>> + gpu_write(pfdev, GPU_PERFCNT_CFG,
>> + cfg | GPU_PERFCNT_CFG_MODE(GPU_PERFCNT_CFG_MODE_MANUAL));
>> +
>> + if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8186))
>> + gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0xffffffff);
>> +}
>> +
>> +static int panfrost_perfcnt_dump(struct panfrost_device *pfdev)
>> +{
>> + u64 gpuva;
>> + int ret;
>> +
>> + reinit_completion(&pfdev->perfcnt->dump_comp);
>> + gpuva = pfdev->perfcnt->bo->node.start << PAGE_SHIFT;
>> + gpu_write(pfdev, GPU_PERFCNT_BASE_LO, gpuva);
>> + gpu_write(pfdev, GPU_PERFCNT_BASE_HI, gpuva >> 32);
>> + gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_SAMPLE);
>> + ret = wait_for_completion_interruptible_timeout(&pfdev->perfcnt->dump_comp,
>> + msecs_to_jiffies(1000));
>> + if (!ret)
>> + ret = -ETIMEDOUT;
>> + else if (ret > 0)
>> + ret = 0;
>> +
>> + return ret;
>> +}
>> +
>> +void panfrost_perfcnt_resume(struct panfrost_device *pfdev)
>
> No suspend handling needed?
>
>> +{
>> + if (pfdev->perfcnt)
>> + panfrost_perfcnt_setup(pfdev);
>> +}
>> +
>> +static ssize_t panfrost_perfcnt_enable_read(struct file *file,
>> + char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + struct panfrost_device *pfdev = file->private_data;
>> + ssize_t ret;
>> +
>> + mutex_lock(&pfdev->perfcnt->lock);
>> + ret = simple_read_from_buffer(user_buf, count, ppos,
>> + pfdev->perfcnt->enabled ? "Y\n" : "N\n",
>> + 2);
>> + mutex_unlock(&pfdev->perfcnt->lock);
>> +
>> + return ret;
>> +}
>> +
>> +static ssize_t panfrost_perfcnt_enable_write(struct file *file,
>> + const char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + struct panfrost_device *pfdev = file->private_data;
>> + bool enable;
>> + int ret;
>> +
>> + ret = kstrtobool_from_user(user_buf, count, &enable);
>> + if (ret)
>> + return ret;
>> +
>> + ret = pm_runtime_get_sync(pfdev->dev);
>> + if (ret < 0)
>> + return ret;
>> +
>> + mutex_lock(&pfdev->perfcnt->lock);
>> + if (enable != pfdev->perfcnt->enabled) {
>> + pfdev->perfcnt->enabled = enable;
>> + panfrost_perfcnt_setup(pfdev);
>> + }
>> + mutex_unlock(&pfdev->perfcnt->lock);
>> +
>> + pm_runtime_mark_last_busy(pfdev->dev);
>> + pm_runtime_put_autosuspend(pfdev->dev);
>> +
>> + return count;
>> +}
>> +
>> +static const struct file_operations panfrost_perfcnt_enable_fops = {
>> + .read = panfrost_perfcnt_enable_read,
>> + .write = panfrost_perfcnt_enable_write,
>> + .open = simple_open,
>> + .llseek = default_llseek,
>> +};
>> +
>> +static ssize_t panfrost_perfcnt_dump_read(struct file *file,
>> + char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + struct panfrost_device *pfdev = file->private_data;
>> + ssize_t ret;
>> +
>> + ret = pm_runtime_get_sync(pfdev->dev);
>> + if (ret < 0)
>> + return ret;
>> +
>> + mutex_lock(&pfdev->perfcnt->lock);
>> + if (!pfdev->perfcnt->enabled) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + ret = panfrost_perfcnt_dump(pfdev);
>> + if (ret)
>> + goto out;
>> +
>> + ret = simple_read_from_buffer(user_buf, count, ppos,
>> + pfdev->perfcnt->buf,
>> + pfdev->perfcnt->bosize);
>> +
>> +out:
>> + mutex_unlock(&pfdev->perfcnt->lock);
>> + pm_runtime_mark_last_busy(pfdev->dev);
>> + pm_runtime_put_autosuspend(pfdev->dev);
>> +
>> + return ret;
>> +}
>> +
>> +static const struct file_operations panfrost_perfcnt_dump_fops = {
>> + .read = panfrost_perfcnt_dump_read,
>> + .open = simple_open,
>> + .llseek = default_llseek,
>> +};
>> +
>> +int panfrost_perfcnt_debugfs_init(struct drm_minor *minor)
>> +{
>> + struct panfrost_device *pfdev = to_panfrost_device(minor->dev);
>> + struct dentry *file, *dir;
>> +
>> + dir = debugfs_create_dir("perfcnt", minor->debugfs_root);
>> + if (IS_ERR(dir))
>> + return PTR_ERR(dir);
>> +
>> + file = debugfs_create_file("dump", 0400, dir, pfdev,
>> + &panfrost_perfcnt_dump_fops);
>> + if (IS_ERR(file))
>> + return PTR_ERR(file);
>> +
>> + file = debugfs_create_file("enable", 0600, dir, pfdev,
>> + &panfrost_perfcnt_enable_fops);
>> + if (IS_ERR(file))
>> + return PTR_ERR(file);
>> +
>> + return 0;
>> +}
>> +
>> +int panfrost_perfcnt_init(struct panfrost_device *pfdev)
>> +{
>> + struct panfrost_perfcnt *perfcnt;
>> + struct drm_gem_shmem_object *bo;
>> + size_t size;
>> + u32 status;
>> + int ret;
>> +
>> + if (panfrost_has_hw_feature(pfdev, HW_FEATURE_V4)) {
>> + unsigned int ncoregroups;
>> +
>> + ncoregroups = hweight64(pfdev->features.l2_present);
>> + size = ncoregroups * BLOCKS_PER_COREGROUP *
>> + COUNTERS_PER_BLOCK * BYTES_PER_COUNTER;
>> + } else {
>> + unsigned int nl2c, ncores;
>> +
>> + /*
>> + * TODO: define a macro to extract the number of l2 caches from
>> + * mem_features.
>> + */
>> + nl2c = ((pfdev->features.mem_features >> 8) & GENMASK(3, 0)) + 1;
>> +
>> + /*
>> + * The ARM driver is grouping cores per core group and then
>> + * only using the number of cores in group 0 to calculate the
>> + * size. Not sure why this is done like that, but I guess
>> + * shader_present will only show cores in the first group
>> + * anyway.
>> + */
I'm not sure I understand this comment. I'm not sure which version of
the driver you are looking at, but shader_present should contain all the
cores (in every core group).
The kbase driver (in panfrost's git tree[1]), contains:
base_gpu_props *props = &kbdev->gpu_props.props;
u32 nr_l2 = props->l2_props.num_l2_slices;
u64 core_mask = props->coherency_info.group[0].core_mask;
u32 nr_blocks = fls64(core_mask);
/* JM and tiler counter blocks are always present */
dump_size = (2 + nr_l2 + nr_blocks) *
NR_CNT_PER_BLOCK *
NR_BYTES_PER_CNT;
[1]
https://gitlab.freedesktop.org/panfrost/mali_kbase/blob/master/driver/product/kernel/drivers/gpu/arm/midgard/mali_kbase_vinstr.c
i.e. the number of cores + number of L2 caches (plus 2 for JM/tiler)
multiplied by the block size.
Also another difference with the below is that fls64 and hweight64 don't
return the same numbers. If the core mask is non-contiguous this will
return different values.
>> + ncores = hweight64(pfdev->features.shader_present);
>> +
>> + /*
>> + * There's always one JM and one Tiler block, hence the '+ 2'
>> + * here.
>> + */
>> + size = (nl2c + ncores + 2) *
>> + COUNTERS_PER_BLOCK * BYTES_PER_COUNTER;
>> + }
>> +
>> + perfcnt = devm_kzalloc(pfdev->dev, sizeof(*perfcnt), GFP_KERNEL);
>> + if (!perfcnt)
>> + return -ENOMEM;
[...]
Steve
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
