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 <arun.siluvery@xxxxxxxxxxxxxxx>
> ---
> 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.
> +
> + /*
> + * 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)?
--
Damien
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
