| Message ID | 1357634037-32542-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 8 Jan 2013 09:33:57 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > This could very well be caused by dirt left behind by the BIOS, so > tune it down to the debug level. If the issue is solely the one described, then it should be silence when we takeover, just like all the other error/debug status registers. DRM_DEBUG is a little too noisy for this, maybe we should look at trace_print or one of the other dynamic debugs for this class of info? -Chris
On Tue, Jan 8, 2013 at 10:32 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Tue, 8 Jan 2013 09:33:57 +0100, Daniel Vetter <daniel.vetter@ffwll.ch> wrote: >> This could very well be caused by dirt left behind by the BIOS, so >> tune it down to the debug level. > > If the issue is solely the one described, then it should be silence when > we takeover, just like all the other error/debug status registers. Good point, I'll check whether pimping gt_reset is good enough. > DRM_DEBUG is a little too noisy for this, maybe we should look at > trace_print or one of the other dynamic debugs for this class of info? The downside of trace_printk is that it doesn't pop up in dmesg. If all that reg checking starts to hurt, we might want to exclude the GT range from it - iirc this seemed most useful for display enabling. -Daniel
On Tue, 8 Jan 2013 11:07:03 +0100 Daniel Vetter <daniel.vetter@ffwll.ch> wrote: > On Tue, Jan 8, 2013 at 10:32 AM, Chris Wilson > <chris@chris-wilson.co.uk> wrote: > > On Tue, 8 Jan 2013 09:33:57 +0100, Daniel Vetter > > <daniel.vetter@ffwll.ch> wrote: > >> This could very well be caused by dirt left behind by the BIOS, so > >> tune it down to the debug level. > > > > If the issue is solely the one described, then it should be silence > > when we takeover, just like all the other error/debug status > > registers. > > Good point, I'll check whether pimping gt_reset is good enough. I agree with Chris FWIW. Changing this for the sake of the one instance during takeover isn't good enough. > > > DRM_DEBUG is a little too noisy for this, maybe we should look at > > trace_print or one of the other dynamic debugs for this class of > > info? > > The downside of trace_printk is that it doesn't pop up in dmesg. If > all that reg checking starts to hurt, we might want to exclude the GT > range from it - iirc this seemed most useful for display enabling. > -Daniel It was useful for display enabling because that's where the biggest delta was for IVB->HSW. I don't think that it will be the case for all platforms. The reason I found it interesting initially is it's also capable of catching when we try to write to powered down things.
Hi 2013/1/8 Daniel Vetter <daniel.vetter@ffwll.ch>: > This could very well be caused by dirt left behind by the BIOS, so > tune it down to the debug level. This message can also be triggered by "I915_READ(REGISTER_THAT_DOES_NOT_EXIST)". Still, I also suspect that the very first message we see is due to something left behind by the BIOS. During the very first I915_WRITE that we do in our driver we are hitting one of these messages. My initial thought was "let's zero GEN7_ERR_INT inside ivybridge_irq_postinstall", but then I discovered the error message is so early in our driver initialization that the correct place to "zero GEN7_ERR_INT" would be inside intel_irq_init (because it seems this is before the very first I915_WRITE we do). It seems to me that it's not good to have all those checks in our I915_{READ/WRITE} macros, so my plans are: Step 1 - Fix all the messages we still have (3 small patches, I already have them locally) Step 2 - Remove the checking code inside I915_WRITE and handle GEN7_ERR_INT the same way we treat the other interrupts. With this, we will lose the ability to check exactly which register is triggering the problem, but no one is supposed to be triggering that problem since we fixed everything in step 1. If we ever see the error message triggered by the interrupt, we will at least be able to bisect or to re-add the messages in a debugging session. IMHO, knowing which register causes the problem is very useful to the developer, but the tradeoff between "knowing the register" and "polluting i915_write and i915_read" should be considered. It's not hard to add the messages back for a debugging session (and today, whenever I have to debug these messages, I also add dump_stack() to my local code). Opinions? If no one objects I'll submit patches for this soon. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=58897 > Tested-by: shui yangwei <yangweix.shui@intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_drv.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 530db83..08dbcc3 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1258,7 +1258,7 @@ 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); \ > + DRM_DEBUG("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)) { \ > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 530db83..08dbcc3 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1258,7 +1258,7 @@ 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); \ + DRM_DEBUG("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)) { \