| Message ID | 1409578133-2800-5-git-send-email-arun.siluvery@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@linux.intel.com> > --- > 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)?
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 <arun.siluvery@linux.intel.com> >> --- >> 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
On Mon, Sep 01, 2014 at 03:45:29PM +0100, Siluvery, Arun wrote: > >>+ /* > >>+ * 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. I don't think we can use mask == 0 when it's an unmasked register. In this case, we still need to be able to have a mask that tells us which are the interesting bits in the value. If we want something generic that can be applied to masked and unmasked register, we'll something like: struct reg_wa { unsigned int masked_register : 1; u32 addr; u32 mask; u32 value; }; So: 1. masked register case: - masked_register = 1 - addr - mask (selects the lower 16 bits that are coding for the W/A value) - value (it only makes sense to some of the lower 16 bits set here) 2. 'normal' register case - masked_register = 0 - addr - mask - value From that generic description you can cover all cases, eg. 1. the write for masked registers becomes: mask << 16 | value 2. the write for normal registers becomes: (read(addr) & ~mask) | value 2. check a W/A has been applied (both normal and masked register): read(addr) & mask == value We seem to have only masked registers atm, so it's fine to handle just that case, but if you envision that we'll need more than masked registers, we need a better W/A definition.
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"); + 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; + + /* + * 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); } intel_runtime_pm_put(dev_priv); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 49b7be7..bcf79f0 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1553,20 +1553,6 @@ struct drm_i915_private { struct intel_shared_dpll shared_dplls[I915_NUM_PLLS]; int dpio_phy_iosf_port[I915_NUM_PHYS_VLV]; - /* - * workarounds are currently applied at different places and - * changes are being done to consolidate them so exact count is - * not clear at this point, use a max value for now. - */ -#define I915_MAX_WA_REGS 16 - struct { - u32 addr; - u32 value; - /* bitmask representing WA bits */ - u32 mask; - } intel_wa_regs[I915_MAX_WA_REGS]; - u32 num_wa_regs; - /* Reclocking support */ bool render_reclock_avail; bool lvds_downclock_avail; diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index bae1527..6c07e69 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -760,6 +760,21 @@ static int ring_init_context(struct intel_engine_cs *ring) return ret; } +int ring_context_rodata(struct drm_device *dev, + struct intel_ring_context_rodata *ro_data) +{ + if (IS_CHERRYVIEW(dev)) { + ro_data->init_context = chv_ring_init_context; + ro_data->num_items = ARRAY_SIZE(chv_ring_init_context); + } else if (IS_BROADWELL(dev)) { + ro_data->init_context = bdw_ring_init_context; + ro_data->num_items = ARRAY_SIZE(bdw_ring_init_context); + } else + return -EINVAL; + + return 0; +} + static int init_render_ring(struct intel_engine_cs *ring) { struct drm_device *dev = ring->dev; diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 96479c8..f555505 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -317,6 +317,14 @@ struct intel_engine_cs { u32 (*get_cmd_length_mask)(u32 cmd_header); }; +struct intel_ring_context_rodata { + u32 num_items; + const u32 *init_context; +}; + +int ring_context_rodata(struct drm_device *dev, + struct intel_ring_context_rodata *ro_data); + bool intel_ring_initialized(struct intel_engine_cs *ring); static inline unsigned
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@linux.intel.com> --- 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(-)