From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Siluvery, Arun" Subject: Re: [PATCH 4/4] drm/i915: Rework workaround data exporting to debugfs Date: Mon, 01 Sep 2014 15:45:29 +0100 Message-ID: <54048689.5090203@linux.intel.com> References: <1409578133-2800-1-git-send-email-arun.siluvery@linux.intel.com> <1409578133-2800-5-git-send-email-arun.siluvery@linux.intel.com> <20140901140657.GD1118@strange.amr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTP id A43A1892F3 for ; Mon, 1 Sep 2014 07:45:59 -0700 (PDT) In-Reply-To: <20140901140657.GD1118@strange.amr.corp.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Damien Lespiau Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On 01/09/2014 15:06, Damien Lespiau wrote: > On Mon, Sep 01, 2014 at 02:28:53PM +0100, Arun Siluvery wrote: >> Now w/a are organized in an array so we know exactly how many of them >> are applied; use the same array while exporting data to debugfs and >> remove the temporary array we currently have in driver priv structure. >> >> Signed-off-by: Arun Siluvery >> --- >> drivers/gpu/drm/i915/i915_debugfs.c | 41 +++++++++++++++++++++++---------- >> drivers/gpu/drm/i915/i915_drv.h | 14 ----------- >> drivers/gpu/drm/i915/intel_ringbuffer.c | 15 ++++++++++++ >> drivers/gpu/drm/i915/intel_ringbuffer.h | 8 +++++++ >> 4 files changed, 52 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c >> index 2727bda..bab0408 100644 >> --- a/drivers/gpu/drm/i915/i915_debugfs.c >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >> @@ -2465,6 +2465,14 @@ static int i915_wa_registers(struct seq_file *m, void *unused) >> struct drm_info_node *node = (struct drm_info_node *) m->private; >> struct drm_device *dev = node->minor->dev; >> struct drm_i915_private *dev_priv = dev->dev_private; >> + struct intel_ring_context_rodata ro_data; >> + >> + ret = ring_context_rodata(dev, &ro_data); >> + if (ret) { >> + seq_printf(m, "Workarounds applied: 0\n"); > > seq_puts() > >> + DRM_DEBUG_DRIVER("Workaround table not available !!\n"); >> + return -EINVAL; >> + } >> >> ret = mutex_lock_interruptible(&dev->struct_mutex); >> if (ret) >> @@ -2472,18 +2480,27 @@ static int i915_wa_registers(struct seq_file *m, void *unused) >> >> intel_runtime_pm_get(dev_priv); >> >> - seq_printf(m, "Workarounds applied: %d\n", dev_priv->num_wa_regs); >> - for (i = 0; i < dev_priv->num_wa_regs; ++i) { >> - u32 addr, mask; >> - >> - addr = dev_priv->intel_wa_regs[i].addr; >> - mask = dev_priv->intel_wa_regs[i].mask; >> - dev_priv->intel_wa_regs[i].value = I915_READ(addr) | mask; >> - if (dev_priv->intel_wa_regs[i].addr) >> - seq_printf(m, "0x%X: 0x%08X, mask: 0x%08X\n", >> - dev_priv->intel_wa_regs[i].addr, >> - dev_priv->intel_wa_regs[i].value, >> - dev_priv->intel_wa_regs[i].mask); >> + seq_printf(m, "Workarounds applied: %d\n", ro_data.num_items/2); >> + for (i = 0; i < ro_data.num_items; i += 2) { >> + u32 addr, mask, value; >> + >> + addr = ro_data.init_context[i]; >> + /* >> + * Most of workarounds are masked registers; >> + * to set a bit in lower 16-bits we set a mask bit in >> + * upper 16-bits so we can take either of them as mask but >> + * it doesn't work if the w/a is about clearing a bit so >> + * use upper 16-bits to cover both cases. >> + */ >> + mask = ro_data.init_context[i+1] >> 16; > > "Most" doesn't seem good here. Either it's "all" and we're happy, or we > need a generic way to describe the W/A (masked register or not). value + > mask is generic enough to code for both cases. > It seems some of them could be unmasked registers. We can use 'mask' itself to determine whether it is a masked/unmasked register. mask == 0 if it is an unmasked register. >> + >> + /* >> + * value represents the status of other bits in the >> + * register besides w/a bits >> + */ >> + value = I915_READ(addr) | mask; >> + seq_printf(m, "0x%X: 0x%08X, mask: 0x%08X\n", >> + addr, value, mask); >> } > > I still don't get it. 'value' is supposed to be the reference value for > the W/A, but you're or'ing the mask here, so you treat the mask as if it > were the reference value. This won't work if the W/A is about setting > multi-bits fields or about clearing a bit. > > The comment is still not clear enough. You're saying "other bits besides > the w/a bits", but or'ing the mask doesn't do that. > > Why do we care about the "other bits" in the reference value? they don't > matter. Why use something else than (ro_data.init_context[i+1] & 0xffff) > for the value here (as long we're talking about masked registers)? > I have always considered value as the register value (remaining bits of the register and w/a bits) and now I see your point. Yes lower 16-bits can be used as reference value, depending on whether it is a masked/unmasked we can use/not use the mask in conjunction with value in the test. regards Arun