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

From: Robert Bragg <robert@sixbynine.org>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	Robert Bragg <robert@sixbynine.org>,
	intel-gfx@lists.freedesktop.org,
	Daniel Vetter <daniel.vetter@intel.com>,
	Jani Nikula <jani.nikula@linux.intel.com>,
	David Airlie <airlied@linux.ie>,
	Zhenyu Wang <zhenyuw@linux.intel.com>,
	Sourab Gupta <sourab.gupta@intel.com>,
	ML dri-devel <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH v3 01/11] drm/i915: Add i915 perf infrastructure
Date: Tue, 16 Aug 2016 15:59:24 +0100	[thread overview]
Message-ID: <CAMou1-0H+F85002wrXfTcHqh0bqRqs_yxSx=RnTyEjpN+3fFWA@mail.gmail.com> (raw)
In-Reply-To: <20160815145757.GG3447@nuc-i3427.alporthouse.com>

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

On Mon, Aug 15, 2016 at 3:57 PM, Chris Wilson <chris@chris-wilson.co.uk>
wrote:

> 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.
>

Ah, previously I was initializing in i915_driver_load() and I think it
should have been synchronized by requiring a drm fd to interact with the
driver. I'd need to dig in to understand why, but it was previously ok to
init before i915_setup_sysfs(), so this looks like I messed up the rebase,
not properly appreciating I was now initializing after being visible to
userspace.

> 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).
>

I think it's probably just the sysfs bits that may need to be deferred
until after i915_sysfs_init(), after drm_dev_register() where we're visible
to userspace.

>
> > +             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;
>

Ah, I hadn't heard of the Elvis operator before.

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

I had dug around a bit when I was trying to decide how to handle the corner
cases here and found some precedent for prioritize reporting any data
copied over an error for a read().

I've changed the comment to give an example and a reference:

/* If we've successfully copied any data then reporting that
 * takes precedence over any internal error status, so the
 * data isn't lost.
 *
 * For example ret will be -ENOSPC whenever there is more
 * buffered data than can be copied to userspace, but that's
 * only interesting if we weren't able to copy some data
 * because it implies the userspace buffer is too small to
 * receive a single record (and we never split records).
 *
 * Another case with ret == -EFAULT is more of a grey area
 * since it would seem like bad form for userspace to ask us
 * to overrun its buffer, but the user knows best:
 *
 *   http://yarchive.net/comp/linux/partial_reads_writes.html
 */
return state.read ?: (ret ?: -EAGAIN);

Searching for another reference, I also just found the linux device drivers
book talks about the same kind of convention:

http://www.makelinux.net/ldd3/chp-3-sect-7

"Both the read and write methods return a negative value if an error
occurs. A return value greater than or equal to 0, instead, tells the
calling program how many bytes have been successfully transferred. If some
data is transferred correctly and then an error happens, the return value
must be the count of bytes successfully transferred, and the error does not
get reported until the next time the function is called. Implementing this
convention requires, of course, that your driver remember that the error
has occurred so that it can return the error status in the future."

- Robert

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
>

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

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel