drm/i915: Use two 32bit reads for all 64bit REG_READ ioctls
diff mbox

Message ID 1436946644-583-1-git-send-email-chris@chris-wilson.co.uk
State New
Headers show

Commit Message

Chris Wilson July 15, 2015, 7:50 a.m. UTC
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.

The phenomen was first observed on a 32bit system which offset the value
by 32bits, but recently even 64bit Haswell systems have been
demonstrated to return complete garbage instead.

Reported-by: Karol Herbst <freedesktop@karolherbst.de>
Tested-by: Karol Herbst <freedesktop@karolherbst.de>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91317
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/intel_uncore.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Chris Wilson July 15, 2015, 7:59 a.m. UTC | #1
On Wed, Jul 15, 2015 at 08:50:44AM +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.
> 
> The phenomen was first observed on a 32bit system which offset the value
> by 32bits, but recently even 64bit Haswell systems have been
> demonstrated to return complete garbage instead.
> 
> Reported-by: Karol Herbst <freedesktop@karolherbst.de>
> Tested-by: Karol Herbst <freedesktop@karolherbst.de>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91317
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: stable@vger.kernel.org

It should be noted that earlier attempts tried to workaround broken
userspace code in beignet that tried to utilise the broken reads. However,
as this aptly demonstrates the result can be truly random and it was
silly to even try and workaround such damage.
-Chris
Chris Wilson July 15, 2015, 8:32 a.m. UTC | #2
On Wed, Jul 15, 2015 at 08:59:52AM +0100, Chris Wilson wrote:
> On Wed, Jul 15, 2015 at 08:50:44AM +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.
> > 
> > The phenomen was first observed on a 32bit system which offset the value
> > by 32bits, but recently even 64bit Haswell systems have been
> > demonstrated to return complete garbage instead.
> > 
> > Reported-by: Karol Herbst <freedesktop@karolherbst.de>
> > Tested-by: Karol Herbst <freedesktop@karolherbst.de>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91317
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: stable@vger.kernel.org
> 
> It should be noted that earlier attempts tried to workaround broken
> userspace code in beignet that tried to utilise the broken reads. However,
> as this aptly demonstrates the result can be truly random and it was
> silly to even try and workaround such damage.

I thought mesa used the timestamp correctly. It doesn't. Userspace is
just complete and utter garbage. 
-Chris
Shuang He July 19, 2015, 3:25 a.m. UTC | #3
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6802
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  302/302              302/302
SNB                                  315/315              315/315
IVB                                  342/342              342/342
BYT                 -3              285/285              282/285
HSW                                  378/378              378/378
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_partial_pwrite_pread@reads-display      PASS(1)      FAIL(1)
*BYT  igt@gem_partial_pwrite_pread@reads-uncached      PASS(1)      FAIL(1)
*BYT  igt@gem_tiled_partial_pwrite_pread@reads      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 2c477663d378..66fdc85697f6 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -1325,7 +1325,7 @@  int i915_reg_read_ioctl(struct drm_device *dev,
 
 	switch (entry->size) {
 	case 8:
-		reg->val = I915_READ64(reg->offset);
+		reg->val = I915_READ64_2x32(reg->offset, reg->offset+4);
 		break;
 	case 4:
 		reg->val = I915_READ(reg->offset);