<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Feb 4, 2016 at 1:42 AM, Emil Velikov <span dir="ltr"><<a href="mailto:emil.l.velikov@gmail.com" target="_blank">emil.l.velikov@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 3 February 2016 at 18:39, Robert Bragg <<a href="mailto:robert@sixbynine.org">robert@sixbynine.org</a>> wrote:<br>
<br>
> index a5524cc..68ca26e 100644<br>
> --- a/include/uapi/drm/i915_drm.h<br>
> +++ b/include/uapi/drm/i915_drm.h<br>
<br>
</span><span class="">> @@ -1170,4 +1172,71 @@ struct drm_i915_gem_context_param {<br>
> __u64 value;<br>
> };<br>
><br>
> +#define I915_PERF_FLAG_FD_CLOEXEC (1<<0)<br>
> +#define I915_PERF_FLAG_FD_NONBLOCK (1<<1)<br>
> +#define I915_PERF_FLAG_DISABLED (1<<2)<br>
> +<br>
> +enum drm_i915_perf_property_id {<br>
> + /**<br>
> + * Open the stream for a specific context handle (as used with<br>
> + * execbuffer2). A stream opened for a specific context this way<br>
> + * won't typically require root privileges.<br>
> + */<br>
> + DRM_I915_PERF_CTX_HANDLE_PROP = 1,<br>
> +<br>
</span>Wouldn't DRM_I915_PERF_PROP_CTX_HANDLE be a better name ? It's more<br>
obvious at least :-P<br></blockquote><div><br></div><div>Yeah would be more consistent to keep the common namespacing at the front.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> + DRM_I915_PERF_PROP_MAX /* non-ABI */<br>
</span>Isn't the use of enums (in drm UAPI) discouraged ? Afaics only the old<br>
DRI1 i915 code has them.<br>
Same question applies throughout the patch/series.<br></blockquote><div><br></div><div>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.<br></div><div><br></div><div>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)<br><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> +};<br>
> +<br>
> +struct drm_i915_perf_open_param {<br>
> + /** CLOEXEC, NONBLOCK... */<br>
</span>Some other places in i915 define the macros just after the __u32<br>
flags. This will allow you to drop the comment ;-)<br>
<br>
> + __u32 flags;<br>
> +<br>
And ... we broke 32 bit userspare on 64 bit kernels. Please check for<br>
holes and other issues as described in Daniel Vetter's<br>
article/documentation [1]<br>
<br>
[1] <a href="https://www.kernel.org/doc/Documentation/ioctl/botching-up-ioctls.txt" rel="noreferrer" target="_blank">https://www.kernel.org/doc/Documentation/ioctl/botching-up-ioctls.txt</a></blockquote><div><br></div><div>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.<br><br></div><div>I think I'll bump the flags to be 64bit.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<span class=""><br>
> + /**<br>
> + * Pointer to array of u64 (id, value) pairs configuring the stream<br>
> + * to open.<br>
> + */<br>
> + __u64 __user properties;<br>
> +<br>
> + /** The number of u64 (id, value) pairs */<br>
> + __u32 n_properties;<br>
</span>The rest of the file uses num_foo or foo_count. Can we opt for one of those ?<br></blockquote><div><br></div><div>Ah yep, num_properties looks good to me.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> +};<br>
> +<br>
> +#define I915_PERF_IOCTL_ENABLE _IO('i', 0x0)<br>
> +#define I915_PERF_IOCTL_DISABLE _IO('i', 0x1)<br>
> +<br>
> +/**<br>
> + * Common to all i915 perf records<br>
> + */<br>
> +struct drm_i915_perf_record_header {<br>
> + __u32 type;<br>
> + __u16 pad;<br>
</span>Move pad at the end ? This way we can (maybe?) reuse it in the future.<br></blockquote><div><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
> + __u16 size;<br>
> +};<br>
> +<br>
<br>
</span>Daniel, is there a consensus about zeroing the pad from userspace and<br>
checking it for garbage in the ioctl ? The documentation says "do it",<br>
yet I have a hunch that we're missing a few. Also what error should<br>
the ioctl return in such a case - EINVAL or EFAULT ? Worth<br>
documenting, just in case ?<br></blockquote><div><br></div><div>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.<br><br></div><div>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.<br><br><br></div><div>Thanks for looking through this,<br></div><div>- Robert<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
-Emil<br>
</font></span></blockquote></div><br></div></div>