[PATCH 0/5] A few fixes on top of the wa_regs patches

[PATCH 0/5] A few fixes on top of the wa_regs patches

[Damien Lespiau]

Hi Arun,

I've compiled a few patches that I think solve some small-ish issues around
your wa_regs series. Could you please have a look at them and comment/give your
r-b tag if you judge appropriate?

On top of those patches, I'd love some comments on the issues I raised in the
other mail and possible follow up patches to address them.

  http://lists.freedesktop.org/archives/intel-gfx/2014-August/051514.html

At some point, we'll also need a bit of coherence with what Mika has been doing:

  http://lists.freedesktop.org/archives/intel-gfx/2014-August/050000.html

-- 
Damien

Damien Lespiau (5):
  drm/i915: Rename intel_wa_registers with a i915_ prefix
  drm/i915: Don't overrun the intel_wa_regs array
  drm/i915: Don't silently discard workarounds
  drm/i915: Remove unneeded brackets
  drm/i915: Don't restrict i915_wa_registers to BDW

 drivers/gpu/drm/i915/i915_debugfs.c     | 9 ++-------
 drivers/gpu/drm/i915/intel_ringbuffer.c | 4 ++--
 2 files changed, 4 insertions(+), 9 deletions(-)

-- 
1.8.3.1

[PATCH 1/5] drm/i915: Rename intel_wa_registers with a i915_ prefix

[Damien Lespiau]

Those debugfs files are prefixed by i915, the name of the kernel module,
presumably to make the difference with files exposed by core DRM.

Also, add a ',' at the end of the last entry. This is to ease the
conflict resolution when rebasing internal patches that add a member at
the end of the array. Without it, wiggle can't do its job as we need to
modify an existing line (appending the ',').

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 1467cc1..fc3d582a 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2628,7 +2628,7 @@ static int i915_shared_dplls_info(struct seq_file *m, void *unused)
 	return 0;
 }
 
-static int intel_wa_registers(struct seq_file *m, void *unused)
+static int i915_wa_registers(struct seq_file *m, void *unused)
 {
 	int i;
 	int ret;
@@ -4198,7 +4198,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_semaphore_status", i915_semaphore_status, 0},
 	{"i915_shared_dplls_info", i915_shared_dplls_info, 0},
 	{"i915_dp_mst_info", i915_dp_mst_info, 0},
-	{"intel_wa_registers", intel_wa_registers, 0}
+	{"i915_wa_registers", i915_wa_registers, 0},
 };
 #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
 
-- 
1.8.3.1

[PATCH 2/5] drm/i915: Don't overrun the intel_wa_regs array

[Damien Lespiau]

When entering intel_ring_emit_wa() with num_wa_regs equal to
I915_MAX_WA_REGS, we end up indexing the intel_wa_regs array beyond its
allocation.

Fix the check then.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 12135ef..96618c0 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -663,7 +663,7 @@ static inline void intel_ring_emit_wa(struct intel_engine_cs *ring,
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (dev_priv->num_wa_regs > I915_MAX_WA_REGS)
+	if (dev_priv->num_wa_regs >= I915_MAX_WA_REGS)
 		return;
 
 	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
-- 
1.8.3.1

[PATCH 3/5] drm/i915: Don't silently discard workarounds

[Damien Lespiau]

If we happen to emit more than I915_MAX_WA_REGS workarounds, we will
currently discard them, not even emit the LRI. Not really what we want,
so warn loudly.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 96618c0..5ef0f99 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -663,7 +663,7 @@ static inline void intel_ring_emit_wa(struct intel_engine_cs *ring,
 	struct drm_device *dev = ring->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (dev_priv->num_wa_regs >= I915_MAX_WA_REGS)
+	if (WARN_ON(dev_priv->num_wa_regs >= I915_MAX_WA_REGS))
 		return;
 
 	intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
-- 
1.8.3.1

[PATCH 4/5] drm/i915: Remove unneeded brackets

[Damien Lespiau]

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 5ef0f99..f8aadc3 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -671,7 +671,7 @@ static inline void intel_ring_emit_wa(struct intel_engine_cs *ring,
 	intel_ring_emit(ring, value);
 
 	dev_priv->intel_wa_regs[dev_priv->num_wa_regs].addr = addr;
-	dev_priv->intel_wa_regs[dev_priv->num_wa_regs].mask = (value) & 0xFFFF;
+	dev_priv->intel_wa_regs[dev_priv->num_wa_regs].mask = value & 0xFFFF;
 	/* value is updated with the status of remaining bits of this
 	 * register when it is read from debugfs file
 	 */
-- 
1.8.3.1

[PATCH 5/5] drm/i915: Don't restrict i915_wa_registers to BDW

[Damien Lespiau]

We have CHV code that already makes the test obsolete. Besides, when
num_wa_regs is 0 (platforms not gathering that W/A data), we expose
something sensible already.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index fc3d582a..cd4f045 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2636,11 +2636,6 @@ static int i915_wa_registers(struct seq_file *m, void *unused)
 	struct drm_device *dev = node->minor->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (!IS_BROADWELL(dev)) {
-		DRM_DEBUG_DRIVER("Workaround table not available !!\n");
-		return -EINVAL;
-	}
-
 	ret = mutex_lock_interruptible(&dev->struct_mutex);
 	if (ret)
 		return ret;
-- 
1.8.3.1

Re: [PATCH 0/5] A few fixes on top of the wa_regs patches

[Siluvery, Arun]

On 30/08/2014 16:50, Damien Lespiau wrote:
> Hi Arun,
>
> I've compiled a few patches that I think solve some small-ish issues around
> your wa_regs series. Could you please have a look at them and comment/give your
> r-b tag if you judge appropriate?
>
> On top of those patches, I'd love some comments on the issues I raised in the
> other mail and possible follow up patches to address them.
>
>    http://lists.freedesktop.org/archives/intel-gfx/2014-August/051514.html

Hi Damien,

I really appreciate you taking time to not just give review comments but 
also sending patches to fix those issues.

Chris suggested a way of emitting all LRIs using a simple function and I 
really wanted to rework everything based on that suggestion.

The LRIs are now organized in an array as opposed to sending them 
individually also debugfs patch can make use of it. I have removed the 
temporary array included in driver private structure.
I think now it looks clean and we can easily add new w/a with minimal 
changes.

Since all of the patches are modified I think it is better to squash 
them with the merged ones rather than updating them with new patches so 
I have folded your patches during rework and will send them after 
testing, please review them and give your comments.

regards
Arun

>
> At some point, we'll also need a bit of coherence with what Mika has been doing:
>
>    http://lists.freedesktop.org/archives/intel-gfx/2014-August/050000.html
>

Re: [PATCH 0/5] A few fixes on top of the wa_regs patches

[Daniel Vetter]

On Sun, Aug 31, 2014 at 08:32:55PM +0100, Siluvery, Arun wrote:
> On 30/08/2014 16:50, Damien Lespiau wrote:
> >Hi Arun,
> >
> >I've compiled a few patches that I think solve some small-ish issues around
> >your wa_regs series. Could you please have a look at them and comment/give your
> >r-b tag if you judge appropriate?
> >
> >On top of those patches, I'd love some comments on the issues I raised in the
> >other mail and possible follow up patches to address them.
> >
> >   http://lists.freedesktop.org/archives/intel-gfx/2014-August/051514.html
> 
> Hi Damien,
> 
> I really appreciate you taking time to not just give review comments but
> also sending patches to fix those issues.
> 
> Chris suggested a way of emitting all LRIs using a simple function and I
> really wanted to rework everything based on that suggestion.
> 
> The LRIs are now organized in an array as opposed to sending them
> individually also debugfs patch can make use of it. I have removed the
> temporary array included in driver private structure.
> I think now it looks clean and we can easily add new w/a with minimal
> changes.
> 
> Since all of the patches are modified I think it is better to squash them
> with the merged ones rather than updating them with new patches so I have
> folded your patches during rework and will send them after testing, please
> review them and give your comments.

Please don't squash fixup patches when I've merged your patch already -
usually I only drop patches when they're terminally broken, so if you send
me a new version I have to fiddle things to make it all apply. But
squashing in a fixup patch is simpler. And imo also easier to review.

And once we deal in fixup patches it's ok to have a bunch of them imo,
too.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 0/5] A few fixes on top of the wa_regs patches

[Siluvery, Arun]

On 01/09/2014 10:08, Daniel Vetter wrote:
> On Sun, Aug 31, 2014 at 08:32:55PM +0100, Siluvery, Arun wrote:
>> On 30/08/2014 16:50, Damien Lespiau wrote:
>>> Hi Arun,
>>>
>>> I've compiled a few patches that I think solve some small-ish issues around
>>> your wa_regs series. Could you please have a look at them and comment/give your
>>> r-b tag if you judge appropriate?
>>>
>>> On top of those patches, I'd love some comments on the issues I raised in the
>>> other mail and possible follow up patches to address them.
>>>
>>>    http://lists.freedesktop.org/archives/intel-gfx/2014-August/051514.html
>>
>> Hi Damien,
>>
>> I really appreciate you taking time to not just give review comments but
>> also sending patches to fix those issues.
>>
>> Chris suggested a way of emitting all LRIs using a simple function and I
>> really wanted to rework everything based on that suggestion.
>>
>> The LRIs are now organized in an array as opposed to sending them
>> individually also debugfs patch can make use of it. I have removed the
>> temporary array included in driver private structure.
>> I think now it looks clean and we can easily add new w/a with minimal
>> changes.
>>
>> Since all of the patches are modified I think it is better to squash them
>> with the merged ones rather than updating them with new patches so I have
>> folded your patches during rework and will send them after testing, please
>> review them and give your comments.
>
> Please don't squash fixup patches when I've merged your patch already -
> usually I only drop patches when they're terminally broken, so if you send
> me a new version I have to fiddle things to make it all apply. But
> squashing in a fixup patch is simpler. And imo also easier to review.

In this case all of the code is newly added so most of it should apply 
cleanly but if the preference is to not squash them I can send fix-up 
patches accordingly.

regards
Arun

>
> And once we deal in fixup patches it's ok to have a bunch of them imo,
> too.
> -Daniel
>

Re: [PATCH 1/5] drm/i915: Rename intel_wa_registers with a i915_ prefix

[Siluvery, Arun]

On 30/08/2014 16:50, Damien Lespiau wrote:
> Those debugfs files are prefixed by i915, the name of the kernel module,
> presumably to make the difference with files exposed by core DRM.
>
> Also, add a ',' at the end of the last entry. This is to ease the
> conflict resolution when rebasing internal patches that add a member at
> the end of the array. Without it, wiggle can't do its job as we need to
> modify an existing line (appending the ',').
>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 1467cc1..fc3d582a 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2628,7 +2628,7 @@ static int i915_shared_dplls_info(struct seq_file *m, void *unused)
>   	return 0;
>   }
>
> -static int intel_wa_registers(struct seq_file *m, void *unused)
> +static int i915_wa_registers(struct seq_file *m, void *unused)
>   {
>   	int i;
>   	int ret;
> @@ -4198,7 +4198,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
>   	{"i915_semaphore_status", i915_semaphore_status, 0},
>   	{"i915_shared_dplls_info", i915_shared_dplls_info, 0},
>   	{"i915_dp_mst_info", i915_dp_mst_info, 0},
> -	{"intel_wa_registers", intel_wa_registers, 0}
> +	{"i915_wa_registers", i915_wa_registers, 0},
>   };
>   #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
>
>
Reviewed-by: Arun Siluvery <arun.siluvery@linux.intel.com>
This is only for this patch, remaining patches are not required in the 
rework.

regards
Arun

Re: [PATCH 5/5] drm/i915: Don't restrict i915_wa_registers to BDW

[Siluvery, Arun]

On 30/08/2014 16:51, Damien Lespiau wrote:
> We have CHV code that already makes the test obsolete. Besides, when
> num_wa_regs is 0 (platforms not gathering that W/A data), we expose
> something sensible already.
>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c | 5 -----
>   1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index fc3d582a..cd4f045 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2636,11 +2636,6 @@ static int i915_wa_registers(struct seq_file *m, void *unused)
>   	struct drm_device *dev = node->minor->dev;
>   	struct drm_i915_private *dev_priv = dev->dev_private;
>
> -	if (!IS_BROADWELL(dev)) {
> -		DRM_DEBUG_DRIVER("Workaround table not available !!\n");
> -		return -EINVAL;
> -	}
> -
>   	ret = mutex_lock_interruptible(&dev->struct_mutex);
>   	if (ret)
>   		return ret;
>
This can also be taken, so patches 1 and 5 in this series.
Reviewed-by: Arun Siluvery <arun.siluvery@linux.intel.com>

regards
Arun

Re: [PATCH 0/5] A few fixes on top of the wa_regs patches

[Siluvery, Arun]

On 30/08/2014 16:50, Damien Lespiau wrote:
> Hi Arun,
>
> I've compiled a few patches that I think solve some small-ish issues around
> your wa_regs series. Could you please have a look at them and comment/give your
> r-b tag if you judge appropriate?
>
> On top of those patches, I'd love some comments on the issues I raised in the
> other mail and possible follow up patches to address them.
>
>    http://lists.freedesktop.org/archives/intel-gfx/2014-August/051514.html
>
> At some point, we'll also need a bit of coherence with what Mika has been doing:
>
>    http://lists.freedesktop.org/archives/intel-gfx/2014-August/050000.html
>
Hi Daniel,

Since the new workaround design/implementation takes time could you 
please pull the patches in this series to fix the issues and also the 
patch to change filename in igt.

for the series,
Reviewed-by: Arun Siluvery <arun.siluvery@linux.intel.com>

regards
Arun

Re: [PATCH 0/5] A few fixes on top of the wa_regs patches

[Daniel Vetter]

On Wed, Sep 03, 2014 at 10:18:47AM +0100, Siluvery, Arun wrote:
> On 30/08/2014 16:50, Damien Lespiau wrote:
> >Hi Arun,
> >
> >I've compiled a few patches that I think solve some small-ish issues around
> >your wa_regs series. Could you please have a look at them and comment/give your
> >r-b tag if you judge appropriate?
> >
> >On top of those patches, I'd love some comments on the issues I raised in the
> >other mail and possible follow up patches to address them.
> >
> >   http://lists.freedesktop.org/archives/intel-gfx/2014-August/051514.html
> >
> >At some point, we'll also need a bit of coherence with what Mika has been doing:
> >
> >   http://lists.freedesktop.org/archives/intel-gfx/2014-August/050000.html
> >
> Hi Daniel,
> 
> Since the new workaround design/implementation takes time could you please
> pull the patches in this series to fix the issues and also the patch to
> change filename in igt.

Yes this is the best approach since clarifying what exactly the full blown
w/a infrastructure should do will take a bit of time.

> for the series,
> Reviewed-by: Arun Siluvery <arun.siluvery@linux.intel.com>

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch