diff mbox

drm/i915/oa: Check that OA is disabled before unpinning

Message ID 20180511135207.12880-1-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Chris Wilson May 11, 2018, 1:52 p.m. UTC
Before we unpin the buffer used for OA reports and return it to the
system, we need to be sure that the HW has finished writing into it.
For lack of a better idea, poll OACONTROL to check it is switched off.

References: https://bugs.freedesktop.org/show_bug.cgi?id=106379
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Cc: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/i915_perf.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Lionel Landwerlin May 11, 2018, 2:11 p.m. UTC | #1
On 11/05/18 14:52, Chris Wilson wrote:
> Before we unpin the buffer used for OA reports and return it to the
> system, we need to be sure that the HW has finished writing into it.
> For lack of a better idea, poll OACONTROL to check it is switched off.
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=106379
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>

Sounds fair :

Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>

> ---
>   drivers/gpu/drm/i915/i915_perf.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index d9341415df40..019bd2d073ad 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -1960,11 +1960,19 @@ static void i915_oa_stream_enable(struct i915_perf_stream *stream)
>   static void gen7_oa_disable(struct drm_i915_private *dev_priv)
>   {
>   	I915_WRITE(GEN7_OACONTROL, 0);
> +	if (intel_wait_for_register(dev_priv,
> +				    GEN7_OACONTROL, GEN7_OACONTROL_ENABLE, 0,
> +				    50))
> +		DRM_ERROR("wait for OA to be disabled timed out\n");
>   }
>   
>   static void gen8_oa_disable(struct drm_i915_private *dev_priv)
>   {
>   	I915_WRITE(GEN8_OACONTROL, 0);
> +	if (intel_wait_for_register(dev_priv,
> +				    GEN8_OACONTROL, GEN8_OA_COUNTER_ENABLE, 0,
> +				    50))
> +		DRM_ERROR("wait for OA to be disabled timed out\n");
>   }
>   
>   /**
Lionel Landwerlin May 11, 2018, 4:10 p.m. UTC | #2
On 11/05/18 15:11, Lionel Landwerlin wrote:
> On 11/05/18 14:52, Chris Wilson wrote:
>> Before we unpin the buffer used for OA reports and return it to the
>> system, we need to be sure that the HW has finished writing into it.
>> For lack of a better idea, poll OACONTROL to check it is switched off.
>>
>> References: https://bugs.freedesktop.org/show_bug.cgi?id=106379
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> Cc: Matthew Auld <matthew.auld@intel.com>
>
> Sounds fair :
>
> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>

Tested-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>

>
>> ---
>>   drivers/gpu/drm/i915/i915_perf.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_perf.c 
>> b/drivers/gpu/drm/i915/i915_perf.c
>> index d9341415df40..019bd2d073ad 100644
>> --- a/drivers/gpu/drm/i915/i915_perf.c
>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>> @@ -1960,11 +1960,19 @@ static void i915_oa_stream_enable(struct 
>> i915_perf_stream *stream)
>>   static void gen7_oa_disable(struct drm_i915_private *dev_priv)
>>   {
>>       I915_WRITE(GEN7_OACONTROL, 0);
>> +    if (intel_wait_for_register(dev_priv,
>> +                    GEN7_OACONTROL, GEN7_OACONTROL_ENABLE, 0,
>> +                    50))
>> +        DRM_ERROR("wait for OA to be disabled timed out\n");
>>   }
>>     static void gen8_oa_disable(struct drm_i915_private *dev_priv)
>>   {
>>       I915_WRITE(GEN8_OACONTROL, 0);
>> +    if (intel_wait_for_register(dev_priv,
>> +                    GEN8_OACONTROL, GEN8_OA_COUNTER_ENABLE, 0,
>> +                    50))
>> +        DRM_ERROR("wait for OA to be disabled timed out\n");
>>   }
>>     /**
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson May 11, 2018, 4:19 p.m. UTC | #3
Quoting Lionel Landwerlin (2018-05-11 17:10:49)
> On 11/05/18 15:11, Lionel Landwerlin wrote:
> > On 11/05/18 14:52, Chris Wilson wrote:
> >> Before we unpin the buffer used for OA reports and return it to the
> >> system, we need to be sure that the HW has finished writing into it.
> >> For lack of a better idea, poll OACONTROL to check it is switched off.
> >>
> >> References: https://bugs.freedesktop.org/show_bug.cgi?id=106379
> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> >> Cc: Matthew Auld <matthew.auld@intel.com>
> >
> > Sounds fair :
> >
> > Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> 
> Tested-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>

Thanks for the review and testing, with luck we will get a CI + KASAN
run over the w/e which will put the matter to rest. Pushed,
-Chris
Lionel Landwerlin May 17, 2018, 10:18 a.m. UTC | #4
This should be sent to stable right?

-
Lionel

On 11/05/18 14:52, Chris Wilson wrote:
> Before we unpin the buffer used for OA reports and return it to the
> system, we need to be sure that the HW has finished writing into it.
> For lack of a better idea, poll OACONTROL to check it is switched off.
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=106379
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_perf.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index d9341415df40..019bd2d073ad 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -1960,11 +1960,19 @@ static void i915_oa_stream_enable(struct i915_perf_stream *stream)
>   static void gen7_oa_disable(struct drm_i915_private *dev_priv)
>   {
>   	I915_WRITE(GEN7_OACONTROL, 0);
> +	if (intel_wait_for_register(dev_priv,
> +				    GEN7_OACONTROL, GEN7_OACONTROL_ENABLE, 0,
> +				    50))
> +		DRM_ERROR("wait for OA to be disabled timed out\n");
>   }
>   
>   static void gen8_oa_disable(struct drm_i915_private *dev_priv)
>   {
>   	I915_WRITE(GEN8_OACONTROL, 0);
> +	if (intel_wait_for_register(dev_priv,
> +				    GEN8_OACONTROL, GEN8_OA_COUNTER_ENABLE, 0,
> +				    50))
> +		DRM_ERROR("wait for OA to be disabled timed out\n");
>   }
>   
>   /**
Chris Wilson May 17, 2018, 10:21 a.m. UTC | #5
Quoting Lionel Landwerlin (2018-05-17 11:18:16)
> This should be sent to stable right?

Yeah, my bad for not digging out the relevant Fixes:

+cc Joonas for the next batch.
-Chris
Lionel Landwerlin May 17, 2018, 10:29 a.m. UTC | #6
On 17/05/18 11:21, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2018-05-17 11:18:16)
>> This should be sent to stable right?
> Yeah, my bad for not digging out the relevant Fixes: +cc Joonas for 
> the next batch. -Chris

I should have looked at it too. Was just in shock ;)

For Haswell:
Fixes: d79651522e89c4 ("drm/i915: Enable i915 perf stream for Haswell OA 
unit")

For Gen8+:
Fixes: 19f81df2859eb1 ("drm/i915/perf: Add OA unit support for Gen 8+")

-
Lionel
Joonas Lahtinen June 11, 2018, 12:44 p.m. UTC | #7
Quoting Lionel Landwerlin (2018-05-17 13:29:56)
> On 17/05/18 11:21, Chris Wilson wrote:
> > Quoting Lionel Landwerlin (2018-05-17 11:18:16)
> >> This should be sent to stable right?
> > Yeah, my bad for not digging out the relevant Fixes: +cc Joonas for 
> > the next batch. -Chris
> 
> I should have looked at it too. Was just in shock ;)
> 
> For Haswell:
> Fixes: d79651522e89c4 ("drm/i915: Enable i915 perf stream for Haswell OA 
> unit")
> 
> For Gen8+:
> Fixes: 19f81df2859eb1 ("drm/i915/perf: Add OA unit support for Gen 8+")

There's no PRs for stable, the stuff is supposed to be marked Cc: stable
based on the Fixes: tags...

So just amend with the information and send to stable mailing list.

Regards, Joonas
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index d9341415df40..019bd2d073ad 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -1960,11 +1960,19 @@  static void i915_oa_stream_enable(struct i915_perf_stream *stream)
 static void gen7_oa_disable(struct drm_i915_private *dev_priv)
 {
 	I915_WRITE(GEN7_OACONTROL, 0);
+	if (intel_wait_for_register(dev_priv,
+				    GEN7_OACONTROL, GEN7_OACONTROL_ENABLE, 0,
+				    50))
+		DRM_ERROR("wait for OA to be disabled timed out\n");
 }
 
 static void gen8_oa_disable(struct drm_i915_private *dev_priv)
 {
 	I915_WRITE(GEN8_OACONTROL, 0);
+	if (intel_wait_for_register(dev_priv,
+				    GEN8_OACONTROL, GEN8_OA_COUNTER_ENABLE, 0,
+				    50))
+		DRM_ERROR("wait for OA to be disabled timed out\n");
 }
 
 /**