Re: [PATCH v5 01/11] drm/i915: Add i915 perf infrastructure

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On Wed, Sep 14, 2016 at 3:42 PM, Emil Velikov <emil.l.velikov@xxxxxxxxx> wrote:
Hi Robert,

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

On 14 September 2016 at 15:19, Robert Bragg <robert@xxxxxxxxxxxxx> 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 :-)

Ah, yep, good catch, I don't write back to the param struct any more.

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.

i915 perf moved to taking an array of u64 properties and no longer writes back a size member in the param struct like perf.

Thanks,
- Robert

 

Regards,
Emil

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux