On Thu, Feb 4, 2016 at 1:42 AM, Emil Velikov <emil.l.velikov@xxxxxxxxx> wrote:
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
Yeah would be more consistent to keep the common namespacing at the front.
> + 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.
An enum here seems like a pretty good technical fit. I think the potential for different enums to have different sizes is a likely reason for being cautious about using them as part of a kernel ABI (so probably unwise to have enum based members in an ioctl structure) but in this case these IDs are always passed to the kernel as a u64. We benefit from the compiler reminding us to handle all properties in the driver if we switch on this enum which I like, as well as having the _MAX value to refer to in the driver which is perhaps a little more reliable at the end of an enum vs a #define which needs to be explicitly updated for each addition.
For reference, the first iteration of this driver was based on the core pref infrastructure and in this case the enum + _MAX /* non-ABI */ approach is borrowed from there (ref: include/uapi/linux/perf_event.h)
> +};
> +
> +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
Ah yeah, don't think this would break 32bit userspace, but still would be good to remove that hole, this has been through a few iterations and there used to be a __u32 type here, well spotted thanks.
I think I'll bump the flags to be 64bit.
> + /**
> + * 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 ?
Ah yep, num_properties looks good to me.
> +};
> +
> +#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.
This header was originally based on struct perf_event_header which is layed out with the padding in the middle too. I can't currently think of a strong reason to maintain a record header that's ABI compatible with perf, but /maybe/ it could be convenient in some case to have a common layout for profiling tools that deal with both. I think we can consider it reserved for future use either way.
> + __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 ?
In case you're wondering about the padding in the header above, note that this header never gets passed in from userspace, it's a header for data read by userspace and the driver zeros the padding.
For the hole in the ioctl I'll probably fill that by extending the flags member and the driver currently returns -EINVAL if any unknown flag bits are set by userspace.
Thanks for looking through this,
- Robert
-Emil
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
