[Intel-gfx] [PATCH 1/8] drm/i915: Add i915 perf infrastructure

[Emil Velikov]

On 3 February 2016 at 18:39, Robert Bragg <robert at sixbynine.org> wrote:

> index a5524cc..68ca26e 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h

> @@ -1170,4 +1172,71 @@ struct drm_i915_gem_context_param {
>         __u64 value;
>  };
>
> +#define I915_PERF_FLAG_FD_CLOEXEC      (1<<0)
> +#define I915_PERF_FLAG_FD_NONBLOCK     (1<<1)
> +#define I915_PERF_FLAG_DISABLED                (1<<2)
> +
> +enum drm_i915_perf_property_id {
> +       /**
> +        * Open the stream for a specific context handle (as used with
> +        * execbuffer2). A stream opened for a specific context this way
> +        * won't typically require root privileges.
> +        */
> +       DRM_I915_PERF_CTX_HANDLE_PROP = 1,
> +
Wouldn't DRM_I915_PERF_PROP_CTX_HANDLE be a better name ? It's more
obvious at least :-P

> +       DRM_I915_PERF_PROP_MAX /* non-ABI */
Isn't the use of enums (in drm UAPI) discouraged ? Afaics only the old
DRI1 i915 code has them.
Same question applies throughout the patch/series.

> +};
> +
> +struct drm_i915_perf_open_param {
> +       /** CLOEXEC, NONBLOCK... */
Some other places in i915 define the macros just after the __u32
flags. This will allow you to drop the comment ;-)

> +       __u32 flags;
> +
And ... we broke 32 bit userspare on 64 bit kernels. Please check for
holes and other issues as described in Daniel Vetter's
article/documentation [1]

[1] https://www.kernel.org/doc/Documentation/ioctl/botching-up-ioctls.txt

> +       /**
> +        * Pointer to array of u64 (id, value) pairs configuring the stream
> +        * to open.
> +        */
> +       __u64 __user properties;
> +
> +       /** The number of u64 (id, value) pairs */
> +       __u32 n_properties;
The rest of the file uses num_foo or foo_count. Can we opt for one of those ?

> +};
> +
> +#define I915_PERF_IOCTL_ENABLE _IO('i', 0x0)
> +#define I915_PERF_IOCTL_DISABLE        _IO('i', 0x1)
> +
> +/**
> + * Common to all i915 perf records
> + */
> +struct drm_i915_perf_record_header {
> +       __u32 type;
> +       __u16 pad;
Move pad at the end ? This way we can (maybe?) reuse it in the future.

> +       __u16 size;
> +};
> +

Daniel, is there a consensus about zeroing the pad from userspace and
checking it for garbage in the ioctl ? The documentation says "do it",
yet I have a hunch that we're missing a few. Also what error should
the ioctl return in such a case - EINVAL or EFAULT ? Worth
documenting, just in case ?

-Emil