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

[Daniel Vetter]

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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch