[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 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.

- Robert

>
>> -Chris
>>
>> --
>> Chris Wilson, Intel Open Source Technology Centre
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch