[Intel-gfx] [RFC 7/8] drm/i915: Add support for forwarding execbuffer tags in timestamp sample metadata

[Robert Bragg]

On Wed, Aug 5, 2015 at 4:25 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Wed, Aug 05, 2015 at 02:59:03PM +0100, Robert Bragg wrote:
>> On Wed, Aug 5, 2015 at 10:29 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
>> > On Wed, Aug 05, 2015 at 10:17:55AM +0100, Chris Wilson wrote:
>> >> On Wed, Aug 05, 2015 at 11:25:43AM +0530, sourab.gupta at intel.com wrote:
>> >> > @@ -555,10 +558,12 @@ static void forward_one_gen_pmu_sample(struct drm_i915_private *dev_priv,
>> >> >     struct drm_i915_ts_node_ctx_id *ctx_info;
>> >> >     struct drm_i915_ts_node_ring_id *ring_info;
>> >> >     struct drm_i915_ts_node_pid *pid_info;
>> >> > +   struct drm_i915_ts_node_tag *tag_info;
>> >> >     struct perf_raw_record raw;
>> >> >
>> >> >     BUILD_BUG_ON((TS_DATA_SIZE != 8) || (CTX_INFO_SIZE != 8) ||
>> >> > -                   (RING_INFO_SIZE != 8) || (PID_INFO_SIZE != 8));
>> >> > +                   (RING_INFO_SIZE != 8) || (PID_INFO_SIZE != 8) ||
>> >> > +                   (TAG_INFO_SIZE != 8));
>> >>
>> >> This is much more useful if each clause is independent. The error
>> >> message is then unambiguous and it looks neater.
>> >>
>> >> >     snapshot = dev_priv->gen_pmu.buffer.addr + node->offset;
>> >> >     snapshot_size = TS_DATA_SIZE + CTX_INFO_SIZE;
>> >>
>> >> > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
>> >> > index 3dcc862..db91098 100644
>> >> > --- a/include/uapi/drm/i915_drm.h
>> >> > +++ b/include/uapi/drm/i915_drm.h
>> >> > @@ -104,7 +104,8 @@ struct drm_i915_gen_pmu_attr {
>> >> >     __u32 size;
>> >> >     __u32 sample_ring:1,
>> >> >             sample_pid:1,
>> >> > -           __reserved_1:30;
>> >> > +           sample_tag:1,
>> >> > +           __reserved_1:29;
>> >>
>> >> Start each bitfield entry on its own line with __u32;
>> >
>> > also no bitfields in uapi headers.
>> > -Daniel
>>
>> Ah, I had previously asked Sourab to pack the bitfields into the same
>> u64. I think we only get into undefined ABI territory if we have
>> multiple sequential bitfields in the structure where the compiler can
>> choose to combine them in some undefined way?
>>
>> This follows the same pattern for bitfields seen in struct perf_event_attr.
>>
>> I'm not sure we'll need lots of flags in our case though so perhaps it
>> would be fine to avoid the use of bitfields altogether here.
>
> It might be uapi cargo culting, but I'm just not sure ;-) The other
> problem with bitfields is that it's fickle properly size the reserved
> fields, and we need those to correctly reject unused flags. Otherwise
> userspace might but garbage in there and extendability is out the window.

In my latest branch (sorry I haven't sent out a recent RFC myself as
I'm hoping to update public Gen Observability docs before I do that) I
ended up slightly generalizing and exporting perf_copy_attr() in
kernel/events/core.c to use the same tested code to help with this.
Core perf's approach to versioning + extending the attributes
structure seems pretty decent.

That said though regarding unused/reserved fields I realise now I did
miss an important check within the i915_oa code that core perf has
which is to explicitly return -EINVAL if __reserved_1 != 0.

Maybe that should be taken as a case in point.

- Robert

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch