[Intel-gfx] [PATCH v5 01/11] drm/i915: Add i915 perf infrastructure

[Emil Velikov]

Hi Robert,

I think I've spotted one interesting, yet trivial bit.

On 14 September 2016 at 15:19, Robert Bragg <robert at sixbynine.org> wrote:
> Adds base i915 perf infrastructure for Gen performance metrics.
>
> This adds a DRM_IOCTL_I915_PERF_OPEN ioctl that takes an array of uint64
> properties to configure a stream of metrics and returns a new fd usable
> with standard VFS system calls including read() to read typed and sized
> records; ioctl() to enable or disable capture and poll() to wait for
> data.
>
> A stream is opened something like:
>
>   uint64_t properties[] = {
>       /* Single context sampling */
>       DRM_I915_PERF_PROP_CTX_HANDLE,        ctx_handle,
>
>       /* Include OA reports in samples */
>       DRM_I915_PERF_PROP_SAMPLE_OA,         true,
>
>       /* OA unit configuration */
>       DRM_I915_PERF_PROP_OA_METRICS_SET,    metrics_set_id,
>       DRM_I915_PERF_PROP_OA_FORMAT,         report_format,
>       DRM_I915_PERF_PROP_OA_EXPONENT,       period_exponent,
>    };
>    struct drm_i915_perf_open_param parm = {
>       .flags = I915_PERF_FLAG_FD_CLOEXEC |
>                I915_PERF_FLAG_FD_NONBLOCK |
>                I915_PERF_FLAG_DISABLED,
>       .properties_ptr = (uint64_t)properties,
>       .num_properties = sizeof(properties) / 16,
>    };
>    int fd = drmIoctl(drm_fd, DRM_IOCTL_I915_PERF_OPEN, &param);
>
> Records read all start with a common { type, size } header with
> DRM_I915_PERF_RECORD_SAMPLE being of most interest. Sample records
> contain an extensible number of fields and it's the
> DRM_I915_PERF_PROP_SAMPLE_xyz properties given when opening that
> determine what's included in every sample.
>
If I'm understanding the above correctly the ioctl can only read user
data and does not write to params, correct ?

> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h

> +#define DRM_IOCTL_I915_PERF_OPEN       DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_PERF_OPEN, struct drm_i915_perf_open_param)

If so, we seems to have a one letter too much in DRM_IOWR - should one
use DRM_IOW/DRM_IOR ? Then again I'm not sure how many ioctls bother,
so please don't read too much into my suggestion :-)

Regards,
Emil