All of lore.kernel.org
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: Paulo Zanoni <przanoni@gmail.com>
Cc: intel-gfx@lists.freedesktop.org, Paulo Zanoni <paulo.r.zanoni@intel.com>
Subject: Re: [PATCH 10/15] drm/i915: check the power well when capturing error state
Date: Fri, 22 Mar 2013 09:45:38 +0100	[thread overview]
Message-ID: <CAKMK7uF4bMbk6isf253WoFNM1yDd3p9uoZANAYU1A5YudLM1HQ@mail.gmail.com> (raw)
In-Reply-To: <CA+gsUGSO9qz49xw7upg_9=aWgDhjJ-XSRezRRph1fsa8bqCdeg@mail.gmail.com>

On Thu, Mar 21, 2013 at 11:12 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> I don't understand your way of thinking: your argument is that "this
> is difficult to maintain", but then you're proposing the addition of a
> new i915_read, which IMHO is way much more complex and feels like a
> hackish solution to the problem. Every patch to the "normal"
> i915_read#x will have to also patch i915_read_nocheck#x. And on patch
> 0014, which you also suggested to use the "I915_READ_NOCHECK"
> function, our code will be relying on the _coincidence_ that when we
> read a register that does not exist, we read 0. I really don't think
> this is a sane approach.

I915_READ_NOCHECK already exists, it's called I915_READ_NOTRACE ;-)
We'd lose the tracepoint when just using that, but imo that's no too
horrible for debug checks.

Wrt the assert_pipe check relying on the fact that the hw returns 0,
I'm totally ok on relying on that. If that ever changes in a future
product, we can fix it.

> Our hardware is getting much more complex with respect to these reads,
> and I think we should never read ever read from a register that does
> not exist or is powered off. I'm open to more suggestions on how to
> write this patch, but I really think we shouldn't read registers that
> don't exist or are powered off.

The "much more complex" part is actually why I've looked into
different options for the _debug_ functions. I agree with you for all
the real display code. But debug code (especially the gpu hang stuff)
tends to not be run too often, hence I'm leaning more towards a "just
do it, I know what I'm doing" kind of approach. Currently it's fairly
simple, but with future hw we'll have more chances to screw up and not
dump a few regs we actually want. So I just want to avoid getting a
bug report where the first thing we have to do is patch up the reg
dumper (and we can't really test this any other way than by human
inspection of a dump).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

  reply	other threads:[~2013-03-22  8:45 UTC|newest]

Thread overview: 73+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-03-06 23:03 [PATCH 00/15] More "unclaimed register" fixes Paulo Zanoni
2013-03-06 23:03 ` [PATCH 01/15] drm/i915: only disable DDI sound if intel_crtc->eld_vld Paulo Zanoni
2013-03-07  9:31   ` Ville Syrjälä
2013-03-17 20:17     ` Daniel Vetter
2013-03-17 20:23     ` Daniel Vetter
2013-03-20 22:03       ` Paulo Zanoni
2013-03-20 22:24         ` Daniel Vetter
2013-03-22 17:11   ` [PATCH 2/7] " Paulo Zanoni
2013-03-22 18:11     ` Ville Syrjälä
2013-03-06 23:03 ` [PATCH 02/15] drm/i915: disable sound first on intel_disable_ddi Paulo Zanoni
2013-03-15 18:24   ` Ben Widawsky
2013-03-17 20:25     ` Daniel Vetter
2013-03-06 23:03 ` [PATCH 03/15] drm/i915: add intel_power_well_is_down Paulo Zanoni
2013-03-06 23:26   ` Daniel Vetter
2013-03-06 23:31     ` Daniel Vetter
2013-03-15 18:31     ` Ben Widawsky
2013-03-20 22:21       ` Paulo Zanoni
2013-03-22 17:14   ` [PATCH 3/7] drm/i915: add intel_using_power_well Paulo Zanoni
2013-04-17  9:04     ` Daniel Vetter
2013-04-17 12:27       ` Daniel Vetter
2013-03-06 23:03 ` [PATCH 04/15] drm/i915: don't touch the PF regs if the power well is down Paulo Zanoni
2013-03-06 23:28   ` Daniel Vetter
2013-03-15 18:41     ` Ben Widawsky
2013-03-22 17:16   ` [PATCH 4/7] " Paulo Zanoni
2013-04-12 20:23     ` Daniel Vetter
2013-03-06 23:03 ` [PATCH 05/15] drm/i915: capture the correct cursor registers on IVB Paulo Zanoni
2013-03-07  9:34   ` Ville Syrjälä
2013-03-15 18:45     ` Ben Widawsky
2013-03-17 20:26       ` Daniel Vetter
2013-03-06 23:03 ` [PATCH 06/15] drm/i915: there's no DSPSIZE register on gen4+ Paulo Zanoni
2013-03-07  9:38   ` Ville Syrjälä
2013-03-15 19:04     ` Ben Widawsky
2013-03-15 19:08       ` Ben Widawsky
2013-03-17 20:29         ` Daniel Vetter
2013-03-06 23:03 ` [PATCH 07/15] drm/i915: there's no DSPADDR register on Haswell Paulo Zanoni
2013-03-15 19:10   ` Ben Widawsky
2013-03-17 20:33     ` Daniel Vetter
2013-03-20 18:01       ` Paulo Zanoni
2013-03-20 21:44         ` Daniel Vetter
2013-03-22 17:19   ` [PATCH 5/7] drm/i915: fix DSPADDR Gen check Paulo Zanoni
2013-03-23 12:33     ` Daniel Vetter
2013-03-06 23:03 ` [PATCH 08/15] drm/i915: remove DSPPOS register Paulo Zanoni
2013-03-07  9:43   ` Ville Syrjälä
2013-03-17 20:39     ` Daniel Vetter
2013-03-15 19:13   ` Ben Widawsky
2013-03-22 17:20   ` [PATCH 6/7] drm/i915: there's no DSPPOS register on gen4+ Paulo Zanoni
2013-03-23 12:33     ` Daniel Vetter
2013-03-06 23:03 ` [PATCH 09/15] drm/i915: there's no PIPESTAT on Gen5+ Paulo Zanoni
2013-03-06 23:22   ` Daniel Vetter
2013-03-07  9:19     ` Ville Syrjälä
2013-03-22 17:24   ` [PATCH 7/7] drm/i915: there's no PIPESTAT on HAS_PCH_SPLIT platforms Paulo Zanoni
2013-03-23 12:33     ` Daniel Vetter
2013-03-06 23:03 ` [PATCH 10/15] drm/i915: check the power well when capturing error state Paulo Zanoni
2013-03-15 19:22   ` Ben Widawsky
2013-03-17 20:46   ` Daniel Vetter
2013-03-17 20:51     ` Daniel Vetter
2013-03-21 22:12     ` Paulo Zanoni
2013-03-22  8:45       ` Daniel Vetter [this message]
2013-03-06 23:03 ` [PATCH 11/15] drm/i915: add HAS_POWER_WELL Paulo Zanoni
2013-03-07 17:14   ` Jesse Barnes
2013-03-17 20:49     ` Daniel Vetter
2013-03-06 23:03 ` [PATCH 12/15] drm/i915: reorganize intel_lvds_supported Paulo Zanoni
2013-03-15 20:57   ` Ben Widawsky
2013-03-17 20:52     ` Daniel Vetter
2013-03-06 23:03 ` [PATCH 13/15] drm/i915: don't save/restore PCH_LVDS on LPT Paulo Zanoni
2013-03-15 21:04   ` Ben Widawsky
2013-03-17 20:54     ` Daniel Vetter
2013-03-06 23:03 ` [PATCH 14/15] drm/i915: check the power well on i915_pipe_enabled Paulo Zanoni
2013-03-15 21:06   ` Ben Widawsky
2013-03-17 20:55   ` Daniel Vetter
2013-03-06 23:03 ` [PATCH 15/15] drm/i915: add missing space in error message Paulo Zanoni
2013-03-15 21:10   ` Ben Widawsky
2013-03-17 20:57     ` Daniel Vetter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAKMK7uF4bMbk6isf253WoFNM1yDd3p9uoZANAYU1A5YudLM1HQ@mail.gmail.com \
    --to=daniel@ffwll.ch \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=paulo.r.zanoni@intel.com \
    --cc=przanoni@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.