<div dir="ltr"><br><div><div class="gmail_extra"><div class="gmail_quote">On Wed, Sep 14, 2016 at 3:42 PM, Emil Velikov <span dir="ltr"><<a target="_blank" href="mailto:emil.l.velikov@gmail.com">emil.l.velikov@gmail.com</a>></span> wrote:<br><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote">Hi Robert,<br>
<br>
I think I've spotted one interesting, yet trivial bit.<br>
<div><div><br>
On 14 September 2016 at 15:19, Robert Bragg <<a target="_blank" href="mailto:robert@sixbynine.org">robert@sixbynine.org</a>> wrote:<br>
> Adds base i915 perf infrastructure for Gen performance metrics.<br>
><br>
> This adds a DRM_IOCTL_I915_PERF_OPEN ioctl that takes an array of uint64<br>
> properties to configure a stream of metrics and returns a new fd usable<br>
> with standard VFS system calls including read() to read typed and sized<br>
> records; ioctl() to enable or disable capture and poll() to wait for<br>
> data.<br>
><br>
> A stream is opened something like:<br>
><br>
>   uint64_t properties[] = {<br>
>       /* Single context sampling */<br>
>       DRM_I915_PERF_PROP_CTX_<wbr>HANDLE,        ctx_handle,<br>
><br>
>       /* Include OA reports in samples */<br>
>       DRM_I915_PERF_PROP_SAMPLE_OA,<wbr>         true,<br>
><br>
>       /* OA unit configuration */<br>
>       DRM_I915_PERF_PROP_OA_<wbr>METRICS_SET,    metrics_set_id,<br>
>       DRM_I915_PERF_PROP_OA_FORMAT,<wbr>         report_format,<br>
>       DRM_I915_PERF_PROP_OA_EXPONEN<wbr>T,       period_exponent,<br>
>    };<br>
>    struct drm_i915_perf_open_param parm = {<br>
>       .flags = I915_PERF_FLAG_FD_CLOEXEC |<br>
>                I915_PERF_FLAG_FD_NONBLOCK |<br>
>                I915_PERF_FLAG_DISABLED,<br>
>       .properties_ptr = (uint64_t)properties,<br>
>       .num_properties = sizeof(properties) / 16,<br>
>    };<br>
>    int fd = drmIoctl(drm_fd, DRM_IOCTL_I915_PERF_OPEN, &param);<br>
><br>
> Records read all start with a common { type, size } header with<br>
> DRM_I915_PERF_RECORD_SAMPLE being of most interest. Sample records<br>
> contain an extensible number of fields and it's the<br>
> DRM_I915_PERF_PROP_SAMPLE_xyz properties given when opening that<br>
> determine what's included in every sample.<br>
><br>
</div></div>If I'm understanding the above correctly the ioctl can only read user<br>
data and does not write to params, correct ?<br>
<br>
> --- a/include/uapi/drm/i915_drm.h<br>
> +++ b/include/uapi/drm/i915_drm.h<br>
<span><br>
> +#define DRM_IOCTL_I915_PERF_OPEN       DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_PERF_OPEN, struct drm_i915_perf_open_param)<br>
<br>
</span>If so, we seems to have a one letter too much in DRM_IOWR - should one<br>
use DRM_IOW/DRM_IOR ? Then again I'm not sure how many ioctls bother,<br>
so please don't read too much into my suggestion :-)<br></blockquote><div><br><div><div><div><div>Ah, yep, good catch, I don't write back to the param struct any more.<br><br></div>The
 first iteration of this interface was even more closely modeled on the core 
linux perf interface where the param struct starts with a size member 
and in a case where userspace passes a structure that's smaller than 
expected the kernel returns an error but also writes back the expected 
size to inform userspace.<br><br></div>i915 perf moved to taking an array of u64 properties and no longer writes back a size member in the param struct like perf.<br><br></div>Thanks,<br></div>- Robert<br><br> </div><blockquote style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex" class="gmail_quote">
<br>
Regards,<br>
Emil<br>
</blockquote></div><br></div></div></div>