diff mbox

[04/10] drm/i915: fix false positive "Unclaimed write" messages

Message ID 1353425264-3728-5-git-send-email-przanoni@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paulo Zanoni Nov. 20, 2012, 3:27 p.m. UTC
From: Paulo Zanoni <paulo.r.zanoni@intel.com>

We don't check if the "unclaimed register" bit is set before we call
writel, so if it was already set before, we might print a misleading
message about "unclaimed write" on the wrong register.

This patch makes us check the unclaimed bit before the writel, so we
can print a new "Unknown unclaimed register before writing to %x"
message.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Lespiau, Damien Nov. 20, 2012, 6:46 p.m. UTC | #1
> +       if (IS_HASWELL(dev_priv->dev) && (I915_READ_NOTRACE(GEN7_ERR_INT) & ERR_INT_MMIO_UNCLAIMED)) { \
> +               DRM_ERROR("Unknown unclaimed register before writing to %x\n", reg); \
> +               I915_WRITE_NOTRACE(GEN7_ERR_INT, ERR_INT_MMIO_UNCLAIMED); \
> +       } \

Do we really have to DRM_ERROR here? this bit being set seems to be
beyond things we can fix (BIOS leaving that behind?)
Paulo Zanoni Nov. 20, 2012, 7:10 p.m. UTC | #2
Hi

2012/11/20 Damien Lespiau <damien.lespiau@intel.com>:
>> +       if (IS_HASWELL(dev_priv->dev) && (I915_READ_NOTRACE(GEN7_ERR_INT) & ERR_INT_MMIO_UNCLAIMED)) { \
>> +               DRM_ERROR("Unknown unclaimed register before writing to %x\n", reg); \
>> +               I915_WRITE_NOTRACE(GEN7_ERR_INT, ERR_INT_MMIO_UNCLAIMED); \
>> +       } \
>
> Do we really have to DRM_ERROR here? this bit being set seems to be
> beyond things we can fix (BIOS leaving that behind?)

If we do I915_WRITE_NOTRACE or I915_READ_NOTRACE to a register that
does not exist I believe the unclaimed bit will be set.

>
> --
> Damien
Lespiau, Damien Nov. 20, 2012, 7:24 p.m. UTC | #3
On Tue, Nov 20, 2012 at 7:10 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> Hi
>
> 2012/11/20 Damien Lespiau <damien.lespiau@intel.com>:
>>> +       if (IS_HASWELL(dev_priv->dev) && (I915_READ_NOTRACE(GEN7_ERR_INT) & ERR_INT_MMIO_UNCLAIMED)) { \
>>> +               DRM_ERROR("Unknown unclaimed register before writing to %x\n", reg); \
>>> +               I915_WRITE_NOTRACE(GEN7_ERR_INT, ERR_INT_MMIO_UNCLAIMED); \
>>> +       } \
>>
>> Do we really have to DRM_ERROR here? this bit being set seems to be
>> beyond things we can fix (BIOS leaving that behind?)
>
> If we do I915_WRITE_NOTRACE or I915_READ_NOTRACE to a register that
> does not exist I believe the unclaimed bit will be set.

Yup, that looks true, I guess that if we're seeing that BIOSes leave
that behind and generate a false positive we can write that bit when
cleaning up after the BIOS anyway.

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
Paulo Zanoni Nov. 20, 2012, 7:34 p.m. UTC | #4
2012/11/20 Damien Lespiau <damien.lespiau@intel.com>:
> On Tue, Nov 20, 2012 at 7:10 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> Hi
>>
>> 2012/11/20 Damien Lespiau <damien.lespiau@intel.com>:
>>>> +       if (IS_HASWELL(dev_priv->dev) && (I915_READ_NOTRACE(GEN7_ERR_INT) & ERR_INT_MMIO_UNCLAIMED)) { \
>>>> +               DRM_ERROR("Unknown unclaimed register before writing to %x\n", reg); \
>>>> +               I915_WRITE_NOTRACE(GEN7_ERR_INT, ERR_INT_MMIO_UNCLAIMED); \
>>>> +       } \
>>>
>>> Do we really have to DRM_ERROR here? this bit being set seems to be
>>> beyond things we can fix (BIOS leaving that behind?)
>>
>> If we do I915_WRITE_NOTRACE or I915_READ_NOTRACE to a register that
>> does not exist I believe the unclaimed bit will be set.
>
> Yup, that looks true, I guess that if we're seeing that BIOSes leave
> that behind and generate a false positive we can write that bit when
> cleaning up after the BIOS anyway.

As far as I have investigated, we are also seeing leftovers from the
BIOS that you're talking about. We need to do an initial clear in this
bit at the driver initialization time (irq initialization?).

I forgot to say, but the error message introduced by this patch will
also be print when we I915_READ a register that does not exist (which
is the case when we hang the gpu, for example).

>
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
>
> --
> Damien
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 418d17c..88c44ad 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1251,6 +1251,10 @@  void i915_write##x(struct drm_i915_private *dev_priv, u32 reg, u##x val) { \
 	} \
 	if (IS_GEN5(dev_priv->dev)) \
 		ilk_dummy_write(dev_priv); \
+	if (IS_HASWELL(dev_priv->dev) && (I915_READ_NOTRACE(GEN7_ERR_INT) & ERR_INT_MMIO_UNCLAIMED)) { \
+		DRM_ERROR("Unknown unclaimed register before writing to %x\n", reg); \
+		I915_WRITE_NOTRACE(GEN7_ERR_INT, ERR_INT_MMIO_UNCLAIMED); \
+	} \
 	if (IS_VALLEYVIEW(dev_priv->dev) && IS_DISPLAYREG(reg)) { \
 		write##y(val, dev_priv->regs + reg + 0x180000);		\
 	} else {							\