Re: [Intel-gfx] [PATCH v3] drm/i915: Use two 32bit reads for select 64bit REG_READ ioctls

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jul 17, 2015 at 05:10:25PM +0200, Michał Winiarski wrote:
> On Thu, Jul 16, 2015 at 12:37:56PM +0100, Chris Wilson wrote:
> > Since the hardware sometimes mysteriously totally flummoxes the 64bit
> > read of a 64bit register when read using a single instruction, split the
> > read into two instructions. Since the read here is of automatically
> > incrementing timestamp counters, we also have to be very careful in
> > order to make sure that it does not increment between the two
> > instructions.
> > 
> > However, since userspace tried to workaround this issue and so enshrined
> > this ABI for a broken hardware read and in the process neglected that
> > the read only fails in some environments, we have to introduce a new
> > uABI flag for userspace to request the 2x32 bit accurate read of the
> > timestamp.
> > 
> > v2: Fix alignment check and include details of the workaround for
> > userspace.
> > 
> > Reported-by: Karol Herbst <freedesktop@karolherbst.de>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91317
> > Testcase: igt/gem_reg_read
> Tested-by: Michał Winiarski <michal.winiarski@intel.com>

Where are the mesa/beignet/libva patches for this?
-Daniel

> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Michał Winiarski <michal.winiarski@intel.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/gpu/drm/i915/intel_uncore.c | 26 +++++++++++++++++++-------
> >  include/uapi/drm/i915_drm.h         |  8 ++++++++
> >  2 files changed, 27 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index 2c477663d378..eb244b57b3fd 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -1310,10 +1310,12 @@ int i915_reg_read_ioctl(struct drm_device *dev,
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_i915_reg_read *reg = data;
> >  	struct register_whitelist const *entry = whitelist;
> > +	unsigned size;
> > +	u64 offset;
> >  	int i, ret = 0;
> >  
> >  	for (i = 0; i < ARRAY_SIZE(whitelist); i++, entry++) {
> > -		if (entry->offset == reg->offset &&
> > +		if (entry->offset == (reg->offset & -entry->size) &&
> >  		    (1 << INTEL_INFO(dev)->gen & entry->gen_bitmask))
> >  			break;
> >  	}
> > @@ -1321,23 +1323,33 @@ int i915_reg_read_ioctl(struct drm_device *dev,
> >  	if (i == ARRAY_SIZE(whitelist))
> >  		return -EINVAL;
> >  
> > +	/* We use the low bits to encode extra flags as the register should
> > +	 * be naturally aligned (and those that are not so aligned merely
> > +	 * limit the available flags for that register).
> > +	 */
> > +	offset = entry->offset;
> > +	size = entry->size;
> > +	size |= reg->offset ^ offset;
> > +
> >  	intel_runtime_pm_get(dev_priv);
> >  
> > -	switch (entry->size) {
> > +	switch (size) {
> > +	case 8 | 1:
> > +		reg->val = I915_READ64_2x32(offset, offset+4);
> > +		break;
> >  	case 8:
> > -		reg->val = I915_READ64(reg->offset);
> > +		reg->val = I915_READ64(offset);
> >  		break;
> >  	case 4:
> > -		reg->val = I915_READ(reg->offset);
> > +		reg->val = I915_READ(offset);
> >  		break;
> >  	case 2:
> > -		reg->val = I915_READ16(reg->offset);
> > +		reg->val = I915_READ16(offset);
> >  		break;
> >  	case 1:
> > -		reg->val = I915_READ8(reg->offset);
> > +		reg->val = I915_READ8(offset);
> >  		break;
> >  	default:
> > -		MISSING_CASE(entry->size);
> >  		ret = -EINVAL;
> >  		goto out;
> >  	}
> > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> > index b0f82ddab987..83f60f01dca2 100644
> > --- a/include/uapi/drm/i915_drm.h
> > +++ b/include/uapi/drm/i915_drm.h
> > @@ -1087,6 +1087,14 @@ struct drm_i915_reg_read {
> >  	__u64 offset;
> >  	__u64 val; /* Return value */
> >  };
> > +/* Known registers:
> > + *
> > + * Render engine timestamp - 0x2358 + 64bit - gen7+
> > + * - Note this register returns an invalid value if using the default
> > + *   single instruction 8byte read, in order to workaround that use
> > + *   offset (0x2538 | 1) instead.
> > + *
> > + */
> >  
> >  struct drm_i915_reset_stats {
> >  	__u32 ctx_id;
> > -- 
> > 2.1.4
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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