Re: [PATCH v3 01/11] drm/i915: Add i915 perf infrastructure

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Robert Bragg <robert@sixbynine.org>
Cc: dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org,
	Sourab Gupta <sourab.gupta@intel.com>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH v3 01/11] drm/i915: Add i915 perf infrastructure
Date: Mon, 15 Aug 2016 15:57:57 +0100	[thread overview]
Message-ID: <20160815145757.GG3447@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <20160815144128.7847-2-robert@sixbynine.org>

On Mon, Aug 15, 2016 at 03:41:18PM +0100, Robert Bragg wrote:
> Adds base i915 perf infrastructure for Gen performance metrics.
> 
> This adds a DRM_IOCTL_I915_PERF_OPEN ioctl that takes an array of uint64
> properties to configure a stream of metrics and returns a new fd usable
> with standard VFS system calls including read() to read typed and sized
> records; ioctl() to enable or disable capture and poll() to wait for
> data.
> 
> A stream is opened something like:
> 
>   uint64_t properties[] = {
>       /* Single context sampling */
>       DRM_I915_PERF_PROP_CTX_HANDLE,        ctx_handle,
> 
>       /* Include OA reports in samples */
>       DRM_I915_PERF_PROP_SAMPLE_OA,         true,
> 
>       /* OA unit configuration */
>       DRM_I915_PERF_PROP_OA_METRICS_SET,    metrics_set_id,
>       DRM_I915_PERF_PROP_OA_FORMAT,         report_format,
>       DRM_I915_PERF_PROP_OA_EXPONENT,       period_exponent,
>    };
>    struct drm_i915_perf_open_param parm = {
>       .flags = I915_PERF_FLAG_FD_CLOEXEC |
>                I915_PERF_FLAG_FD_NONBLOCK |
>                I915_PERF_FLAG_DISABLED,
>       .properties_ptr = (uint64_t)properties,
>       .num_properties = sizeof(properties) / 16,
>    };
>    int fd = drmIoctl(drm_fd, DRM_IOCTL_I915_PERF_OPEN, &param);
> 
> Records read all start with a common { type, size } header with
> DRM_I915_PERF_RECORD_SAMPLE being of most interest. Sample records
> contain an extensible number of fields and it's the
> DRM_I915_PERF_PROP_SAMPLE_xyz properties given when opening that
> determine what's included in every sample.
> 
> No specific streams are supported yet so any attempt to open a stream
> will return an error.
> 
> Signed-off-by: Robert Bragg <robert@sixbynine.org>
> ---
>  drivers/gpu/drm/i915/Makefile    |   3 +
>  drivers/gpu/drm/i915/i915_drv.c  |   6 +
>  drivers/gpu/drm/i915/i915_drv.h  |  92 +++++++++
>  drivers/gpu/drm/i915/i915_perf.c | 430 +++++++++++++++++++++++++++++++++++++++
>  include/uapi/drm/i915_drm.h      |  67 ++++++
>  5 files changed, 598 insertions(+)
>  create mode 100644 drivers/gpu/drm/i915/i915_perf.c
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 6092f0e..9a2f1f9 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -106,6 +106,9 @@ i915-y += dvo_ch7017.o \
>  # virtual gpu code
>  i915-y += i915_vgpu.o
>  
> +# perf code
> +i915-y += i915_perf.o
> +
>  ifeq ($(CONFIG_DRM_I915_GVT),y)
>  i915-y += intel_gvt.o
>  include $(src)/gvt/Makefile
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 83afdd0..f5209ff 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1169,6 +1169,9 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
>  	 * cannot run before the connectors are registered.
>  	 */
>  	intel_fbdev_initial_config_async(dev);
> +
> +	/* Depends on sysfs having been initialized */
> +	i915_perf_init(dev_priv);

Then please call it i915_perf_register() and i915_perf_unregister()
respectively.

> +	struct {
> +		bool initialized;

This is bogus. We simply cannot allow userspace to access internals
before we set them up. As it stands you have no serialisation here, so
the protect is moot.

i915_perf_init() needs to be split up into the early initialisation
function to setup internals, and the i915_perf_register() function to
enable userspace (with however many steps you need in between).

> +		struct list_head streams;
> +	} perf;
> +
>  	/* Abstract the submission mechanism (legacy ringbuffer or execlists) away */
>  	struct {
>  		int (*execbuf_submit)(struct i915_execbuffer_params *params,
> @@ -3421,6 +3506,9 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
>  int i915_gem_context_reset_stats_ioctl(struct drm_device *dev, void *data,
>  				       struct drm_file *file);
>  
> +int i915_perf_open_ioctl(struct drm_device *dev, void *data,
> +			 struct drm_file *file);
> +
>  /* i915_gem_evict.c */
>  int __must_check i915_gem_evict_something(struct drm_device *dev,
>  					  struct i915_address_space *vm,
> @@ -3540,6 +3628,10 @@ int intel_engine_cmd_parser(struct intel_engine_cs *engine,
>  			    u32 batch_len,
>  			    bool is_master);
>  
> +/* i915_perf.c */
> +extern void i915_perf_init(struct drm_i915_private *dev_priv);
> +extern void i915_perf_fini(struct drm_i915_private *dev_priv);
> +
>  /* i915_suspend.c */
>  extern int i915_save_state(struct drm_device *dev);
>  extern int i915_restore_state(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> new file mode 100644
> index 0000000..d0ed645
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -0,0 +1,430 @@
> +/*
> + * Copyright © 2015-2016 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *   Robert Bragg <robert@sixbynine.org>
> + */
> +
> +#include <linux/anon_inodes.h>
> +#include <linux/sizes.h>
> +
> +#include "i915_drv.h"
> +
> +struct perf_open_properties {
> +	u32 sample_flags;
> +
> +	u64 single_context:1;
> +	u64 ctx_handle;
> +};
> +
> +static ssize_t i915_perf_read_locked(struct i915_perf_stream *stream,
> +				     struct file *file,
> +				     char __user *buf,
> +				     size_t count,
> +				     loff_t *ppos)
> +{
> +	struct i915_perf_read_state state = { count, 0, buf };
> +	int ret = stream->ops->read(stream, &state);
> +
> +	/* If we've successfully copied any data then reporting that
> +	 * takes precedence over any internal error status, so the
> +	 * data isn't lost
> +	 */
> +	return state.read ? state.read : (ret ? ret : -EAGAIN);

return state.read ?: ret ?: -EAGAIN;

Alternatively you could follow the standard pattern for read. Dare I ask
what is going to go into state that needs the obfuscation?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel