On 3 February 2016 at 18:39, Robert Bragg <robert@xxxxxxxxxxxxx> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
