Re: [PATCH v4 1/6] drm: Add common fdinfo helper

[Christian König]

Am 13.04.23 um 00:42 schrieb Rob Clark:
> From: Rob Clark <robdclark@xxxxxxxxxxxx>
>
> Handle a bit of the boiler-plate in a single case, and make it easier to
> add some core tracked stats.
>
> v2: Update drm-usage-stats.rst, 64b client-id, rename drm_show_fdinfo
>
> Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Signed-off-by: Rob Clark <robdclark@xxxxxxxxxxxx>
> ---
>   Documentation/gpu/drm-usage-stats.rst | 10 +++++++-
>   drivers/gpu/drm/drm_file.c            | 35 +++++++++++++++++++++++++++
>   include/drm/drm_drv.h                 |  7 ++++++
>   include/drm/drm_file.h                |  4 +++
>   4 files changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
> index b46327356e80..2ab32c40e93c 100644
> --- a/Documentation/gpu/drm-usage-stats.rst
> +++ b/Documentation/gpu/drm-usage-stats.rst
> @@ -126,7 +126,15 @@ percentage utilization of the engine, whereas drm-engine-<str> only reflects
>   time active without considering what frequency the engine is operating as a
>   percentage of it's maximum frequency.
>   
> +Implementation Details
> +======================
> +
> +Drivers should use drm_show_fdinfo() in their `struct file_operations`, and
> +implement &drm_driver.show_fdinfo if they wish to provide any stats which
> +are not provided by drm_show_fdinfo().  But even driver specific stats should
> +be documented above and where possible, aligned with other drivers.

I'm really wondering if it wouldn't be less mid-layering if we let the 
drivers call the drm function to print the common values instead of the 
other way around?

Apart from that question the patch looks good to me.

Christian.

> +
>   Driver specific implementations
> -===============================
> +-------------------------------
>   
>   :ref:`i915-usage-stats`
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index a51ff8cee049..6d5bdd684ae2 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -148,6 +148,7 @@ bool drm_dev_needs_global_mutex(struct drm_device *dev)
>    */
>   struct drm_file *drm_file_alloc(struct drm_minor *minor)
>   {
> +	static atomic64_t ident = ATOMIC_INIT(0);
>   	struct drm_device *dev = minor->dev;
>   	struct drm_file *file;
>   	int ret;
> @@ -156,6 +157,8 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
>   	if (!file)
>   		return ERR_PTR(-ENOMEM);
>   
> +	/* Get a unique identifier for fdinfo: */
> +	file->client_id = atomic64_inc_return(&ident);
>   	file->pid = get_pid(task_pid(current));
>   	file->minor = minor;
>   
> @@ -868,6 +871,38 @@ void drm_send_event(struct drm_device *dev, struct drm_pending_event *e)
>   }
>   EXPORT_SYMBOL(drm_send_event);
>   
> +/**
> + * drm_show_fdinfo - helper for drm file fops
> + * @seq_file: output stream
> + * @f: the device file instance
> + *
> + * Helper to implement fdinfo, for userspace to query usage stats, etc, of a
> + * process using the GPU.  See also &drm_driver.show_fdinfo.
> + *
> + * For text output format description please see Documentation/gpu/drm-usage-stats.rst
> + */
> +void drm_show_fdinfo(struct seq_file *m, struct file *f)
> +{
> +	struct drm_file *file = f->private_data;
> +	struct drm_device *dev = file->minor->dev;
> +	struct drm_printer p = drm_seq_file_printer(m);
> +
> +	drm_printf(&p, "drm-driver:\t%s\n", dev->driver->name);
> +	drm_printf(&p, "drm-client-id:\t%llu\n", file->client_id);
> +
> +	if (dev_is_pci(dev->dev)) {
> +		struct pci_dev *pdev = to_pci_dev(dev->dev);
> +
> +		drm_printf(&p, "drm-pdev:\t%04x:%02x:%02x.%d\n",
> +			   pci_domain_nr(pdev->bus), pdev->bus->number,
> +			   PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> +	}
> +
> +	if (dev->driver->show_fdinfo)
> +		dev->driver->show_fdinfo(&p, file);
> +}
> +EXPORT_SYMBOL(drm_show_fdinfo);
> +
>   /**
>    * mock_drm_getfile - Create a new struct file for the drm device
>    * @minor: drm minor to wrap (e.g. #drm_device.primary)
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index 5b86bb7603e7..5edf2a13733b 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -401,6 +401,13 @@ struct drm_driver {
>   			       struct drm_device *dev, uint32_t handle,
>   			       uint64_t *offset);
>   
> +	/**
> +	 * @show_fdinfo:
> +	 *
> +	 * Print device specific fdinfo.  See Documentation/gpu/drm-usage-stats.rst.
> +	 */
> +	void (*show_fdinfo)(struct drm_printer *p, struct drm_file *f);
> +
>   	/** @major: driver major number */
>   	int major;
>   	/** @minor: driver minor number */
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index 0d1f853092ab..6de6d0e9c634 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -258,6 +258,9 @@ struct drm_file {
>   	/** @pid: Process that opened this file. */
>   	struct pid *pid;
>   
> +	/** @client_id: A unique id for fdinfo */
> +	u64 client_id;
> +
>   	/** @magic: Authentication magic, see @authenticated. */
>   	drm_magic_t magic;
>   
> @@ -437,6 +440,7 @@ void drm_send_event(struct drm_device *dev, struct drm_pending_event *e);
>   void drm_send_event_timestamp_locked(struct drm_device *dev,
>   				     struct drm_pending_event *e,
>   				     ktime_t timestamp);
> +void drm_show_fdinfo(struct seq_file *m, struct file *f);
>   
>   struct file *mock_drm_getfile(struct drm_minor *minor, unsigned int flags);
>