Re: [PATCH 1/8] drm/i915: Add i915 perf infrastructure

[Robert Bragg <robert@sixbynine.org>]

[-- Attachment #1.1: Type: text/plain, Size: 4761 bytes --]

On Thu, Feb 4, 2016 at 1:42 AM, Emil Velikov <emil.l.velikov@gmail.com>
wrote:

> On 3 February 2016 at 18:39, Robert Bragg <robert@sixbynine.org> 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
>

[-- Attachment #1.2: Type: text/html, Size: 7024 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel