<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Mon, Jul 7, 2014 at 2:34 PM, Daniel Vetter <<a href="mailto:daniel@ffwll.ch" target="_blank">daniel@ffwll.ch</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Fri, Jul 04, 2014 at 11:50:32AM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <<a href="mailto:paulo.r.zanoni@intel.com">paulo.r.zanoni@intel.com</a>> > > The current code only runs when we do an I915_WRITE operation. It > checks if the unclaimed register flag is set before we do the > operation, and then it checks it again after we do the operation. This > double check allows us to find out if the I915_WRITE operation in > question is the bad one, or if some previous code is the bad one. When > it finds a problem, our code uses DRM_ERROR to signal it. > > The good thing about the current code is that it detects the problem, > so at least we can know we did something wrong. The problem is that > even though we find the problem, we don't really have much information > to actually debug it. So whenever I see one of these DRM_ERROR > messages on my systems, the first thing I do is apply a patch to > change the DRM_ERROR to a WARN and also check for unclaimed registers > on I915_READ operations. This local patch makes things even slower, > but it usually helps a lot in finding the bad code. > > The first point here is that since the current code is only useful to > detect whether we have a problem or not, but it is not really good to > find the cause of the problem, I don't think we should be checking > both before and after every I915_WRITE operation: just doing the check > once should be enough for us to quickly detect problems. With this > change, the code that runs by default for every single user will only > do 1 read operation for every single I915_WRITE, instead of 2. This > patch does this change. > > The second point is that the local patch I have should be upstream, > but since it makes things slower it should be disabled by default. So > I added the i915.mmio_debug option to enable it. > > So after this patch, this is what will happen: > - By default, we will try to detect unclaimed registers once after > every I915_WRITE operation. Previously we tried twice for every > I915_WRITE. > - When we find an unclaimed register we will still print a DRM_ERROR > message, but we will now tell the user to try again with > i915.mmio_debug=1. > - When we use i915.mmio_debug=1 we will try to find unclaimed > registers both before and after every I915_READ and I915_WRITE > operation, and we will print stack traces in case we find them. > This should really help locating the exact point of the bad code > (or at least finding out that i915.ko is not the problem). > > This commit also opens space for really-slow register debugging > operations on other platforms. In theory we can now add lots and lots > of debug code behind i915.mmio_debug, enable this option on our tests, > and catch more problems. > > Signed-off-by: Paulo Zanoni <<a href="mailto:paulo.r.zanoni@intel.com">paulo.r.zanoni@intel.com</a>> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_params.c | 6 ++++ > drivers/gpu/drm/i915/intel_uncore.c | 68 +++++++++++++++++++++++++++++++++---- > 3 files changed, 68 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index ac06c0f..51d867f 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2092,6 +2092,7 @@ struct i915_params { > bool disable_display; > bool disable_vtd_wa; > int use_mmio_flip; > + bool mmio_debug; > }; > extern struct i915_params i915 __read_mostly; > > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c > index 8145729..7977872 100644 > --- a/drivers/gpu/drm/i915/i915_params.c > +++ b/drivers/gpu/drm/i915/i915_params.c > @@ -49,6 +49,7 @@ struct i915_params i915 __read_mostly = { > .enable_cmd_parser = 1, > .disable_vtd_wa = 0, > .use_mmio_flip = 0, > + .mmio_debug = 0, > }; > > module_param_named(modeset, i915.modeset, int, 0400); > @@ -161,3 +162,8 @@ MODULE_PARM_DESC(enable_cmd_parser, > module_param_named(use_mmio_flip, i915.use_mmio_flip, int, 0600); > MODULE_PARM_DESC(use_mmio_flip, > "use MMIO flips (-1=never, 0=driver discretion [default], 1=always)"); > + > +module_param_named(mmio_debug, i915.mmio_debug, bool, 0600); > +MODULE_PARM_DESC(disable_vtd_wa, </div></div></blockquote><div> </div><div>wrong parameter here!</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="HOEnZb"><div class="h5"> > + "Enable the MMIO debug code (default: false). This may negatively " > + "affect performance."); > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c > index 29145df..de5402f 100644 > --- a/drivers/gpu/drm/i915/intel_uncore.c > +++ b/drivers/gpu/drm/i915/intel_uncore.c > @@ -513,21 +513,72 @@ ilk_dummy_write(struct drm_i915_private *dev_priv) > __raw_i915_write32(dev_priv, MI_MODE, 0); > } > > +/** > + * hsw_unclaimed_reg_debug - tries to find unclaimed registers > + * @dev_priv: device private data > + * @reg: register we're about to touch or just have touched > + * @read: are we reading or writing a register now? > + * @before: are we running this before or after we touch the register? > + * > + * This function tries to detect unclaimed registers and provide as much > + * information as possible. It will only do something if the i915.mmio_debug > + * option is enabled. > + * > + * If we detect an unclaimed register when @before is true, it means some > + * unknown code wrote to an unclaimed register, and we're just detecting it now. > + * The bad code can be i915.ko, some other Kernel module (e.g., snd_hda_intel, > + * vgacon) or even user space. If we detect it when @before is false, then > + * there's a really good chance that the read/write operation we just did to > + * @reg is what triggered the unclaimed register message, but there's also the > + * possibility that after the operation we did, and before this function, > + * something else touched another unclaimed register, and we are detecting it > + * now. > + * > + * The unclaimed register bit can get set when we touch a register that does not > + * exist and when we touch a register that exists but is powered down. Please > + * also notice that the HW can only check some register ranges, so there's no > + * guarantee that all read and write operations to inexistent registers will be > + * caught by this function. > + * > + * Also please notice that we don't run this function on __raw operations and in > + * many other cases, so the case where @before is true is quite common. > + */ </div></div>Pet peeve of mine: Comments have a cost since they get stale really fast and then can be awfully misleading. We have way too many examples for that. My rule of interface kerneldoc for functions is that we should only have it for exported functions used in multiple different places in the driver. If there's only one caller (like for _init/fini) functions the comment should be very terse at most. For static inline helpers in the same file otoh code should simply be readable enough to not require comments. Imo this is the case here, so the kerneldoc should imo be dropped. If you want to keep some of it I guess we could document i915.mmio_debug with kerneldoc and then also pull it into our i915 section in the drm docbook. Maybe under a new "debug options" setting. We probably need a DOC: section to make this work. A few sentences should be more than enough. <div><div class="h5"> > static void > -hsw_unclaimed_reg_clear(struct drm_i915_private *dev_priv, u32 reg) > +hsw_unclaimed_reg_debug(struct drm_i915_private *dev_priv, u32 reg, bool read, > + bool before) > { > + const char *op = read ? "reading" : "writing to"; > + const char *when = before ? "before" : "after"; > + > + if (!i915.mmio_debug) > + return; > + > if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) { > - DRM_ERROR("Unknown unclaimed register before writing to %x\n", > - reg); > + WARN(1, "Unclaimed register detected %s %s register 0x%x\n", > + when, op, reg); > __raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM); > } > } > > +/** > + * hsw_unclaimed_reg_detect - tries to find unclaimed registers > + * @dev_priv: device private data > + * > + * This function will try to detect if something touched and unclaimed register, > + * triggering the FPGA_DBG bit. If this happens, we will print a message telling > + * the user to use i915.mmio_debug=1 so we can properly debug the problem. > + * > + * This way, we can still detect our bugs without giving the user the > + * performance impact of hsw_unclaimed_reg_debug(). > + */ > static void > -hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg) > +hsw_unclaimed_reg_detect(struct drm_i915_private *dev_priv) > { > + if (i915.mmio_debug) > + return; > + > if (__raw_i915_read32(dev_priv, FPGA_DBG) & FPGA_DBG_RM_NOCLAIM) { > - DRM_ERROR("Unclaimed write to %x\n", reg); > + DRM_ERROR("Unclaimed register detected. Please use the i915.mmio_debug=1 to debug this problem."); </div></div>But imo really the only piece that's required is this helpful message here into dmesg. Anyway, the concept looks solid. -Daniel <div class="HOEnZb"><div class="h5"> > __raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM); > } > } > @@ -564,6 +615,7 @@ gen5_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \ > static u##x \ > gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \ > REG_READ_HEADER(x); \ > + hsw_unclaimed_reg_debug(dev_priv, reg, true, true); \ > if (dev_priv->uncore.forcewake_count == 0 && \ > NEEDS_FORCE_WAKE((dev_priv), (reg))) { \ > dev_priv->uncore.funcs.force_wake_get(dev_priv, \ > @@ -574,6 +626,7 @@ gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \ > } else { \ > val = __raw_i915_read##x(dev_priv, reg); \ > } \ > + hsw_unclaimed_reg_debug(dev_priv, reg, true, false); \ > REG_READ_FOOTER; \ > } > > @@ -700,12 +753,13 @@ hsw_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace) > if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \ > __fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \ > } \ > - hsw_unclaimed_reg_clear(dev_priv, reg); \ > + hsw_unclaimed_reg_debug(dev_priv, reg, false, true); \ > __raw_i915_write##x(dev_priv, reg, val); \ > if (unlikely(__fifo_ret)) { \ > gen6_gt_check_fifodbg(dev_priv); \ > } \ > - hsw_unclaimed_reg_check(dev_priv, reg); \ > + hsw_unclaimed_reg_debug(dev_priv, reg, false, false); \ > + hsw_unclaimed_reg_detect(dev_priv); \ > REG_WRITE_FOOTER; \ > } > > -- > 2.0.0 > > _______________________________________________ > Intel-gfx mailing list > <a href="mailto:Intel-gfx@lists.freedesktop.org">Intel-gfx@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/intel-gfx" target="_blank">http://lists.freedesktop.org/mailman/listinfo/intel-gfx</a> </div></div>-- Daniel Vetter Software Engineer, Intel Corporation <a href="tel:%2B41%20%280%29%2079%20365%2057%2048" value="+41793655748">+41 (0) 79 365 57 48</a> - <a href="http://blog.ffwll.ch" target="_blank">http://blog.ffwll.ch</a> <div class="HOEnZb"><div class="h5">_______________________________________________ Intel-gfx mailing list <a href="mailto:Intel-gfx@lists.freedesktop.org">Intel-gfx@lists.freedesktop.org</a> <a href="http://lists.freedesktop.org/mailman/listinfo/intel-gfx" target="_blank">http://lists.freedesktop.org/mailman/listinfo/intel-gfx</a> </div></div></blockquote></div> The rest looks good. With that fixed and probably with comment removed as Daniel mentioned, feel free to use:</div><div class="gmail_extra">Reviewed-by: Rodrigo Vivi <<a href="mailto:rodrigo.vivi@gmail.com">rodrigo.vivi@gmail.com</a>> <div> </div>-- <div>Rodrigo Vivi</div><div>Blog: <a href="http://blog.vivi.eng.br" target="_blank">http://blog.vivi.eng.br</a></div><div> </div> </div></div>