drm/i915: Check idle to active before processing CSQ
diff mbox

Message ID 1438870157-18032-1-git-send-email-mika.kuoppala@intel.com
State New
Headers show

Commit Message

Mika Kuoppala Aug. 6, 2015, 2:09 p.m. UTC
If idle to active bit is set, the rest of the fields
in CSQ are not valid.

Bail out early if this is the case in order to prevent
rest of the loop inspecting stale values.

Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Daniel Vetter Aug. 6, 2015, 4:04 p.m. UTC | #1
On Thu, Aug 06, 2015 at 05:09:17PM +0300, Mika Kuoppala wrote:
> If idle to active bit is set, the rest of the fields
> in CSQ are not valid.
> 
> Bail out early if this is the case in order to prevent
> rest of the loop inspecting stale values.
> 
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>

Same questions here too, what's the impact. E.g. if you only found this by
bspec/code inspection then it's for -next, but if it's to fix some known
breakage then it's for -fixes + cc: stable.

Thanks, Daniel

> ---
>  drivers/gpu/drm/i915/intel_lrc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 99bba8e..96218bf 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -497,6 +497,9 @@ void intel_lrc_irq_handler(struct intel_engine_cs *ring)
>  		status_id = I915_READ(RING_CONTEXT_STATUS_BUF(ring) +
>  				(read_pointer % 6) * 8 + 4);
>  
> +		if (status & GEN8_CTX_STATUS_IDLE_ACTIVE)
> +			continue;
> +
>  		if (status & GEN8_CTX_STATUS_PREEMPTED) {
>  			if (status & GEN8_CTX_STATUS_LITE_RESTORE) {
>  				if (execlists_check_remove_request(ring, status_id))
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Mika Kuoppala Aug. 7, 2015, 8:15 a.m. UTC | #2
Daniel Vetter <daniel@ffwll.ch> writes:

> On Thu, Aug 06, 2015 at 05:09:17PM +0300, Mika Kuoppala wrote:
>> If idle to active bit is set, the rest of the fields
>> in CSQ are not valid.
>> 
>> Bail out early if this is the case in order to prevent
>> rest of the loop inspecting stale values.
>> 
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>
> Same questions here too, what's the impact. E.g. if you only found this by
> bspec/code inspection then it's for -next, but if it's to fix some known
> breakage then it's for -fixes + cc: stable.
>

To this and the masked write one: Both of these were found
when I was trying to find out root cause for skl hangs.

They are both for -next. Both are in the correctness
department vrt bspec and I haven't observed any other
impact.

Point taken on being more verbose.

-Mika

> Thanks, Daniel
>
>> ---
>>  drivers/gpu/drm/i915/intel_lrc.c | 3 +++
>>  1 file changed, 3 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index 99bba8e..96218bf 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -497,6 +497,9 @@ void intel_lrc_irq_handler(struct intel_engine_cs *ring)
>>  		status_id = I915_READ(RING_CONTEXT_STATUS_BUF(ring) +
>>  				(read_pointer % 6) * 8 + 4);
>>  
>> +		if (status & GEN8_CTX_STATUS_IDLE_ACTIVE)
>> +			continue;
>> +
>>  		if (status & GEN8_CTX_STATUS_PREEMPTED) {
>>  			if (status & GEN8_CTX_STATUS_LITE_RESTORE) {
>>  				if (execlists_check_remove_request(ring, status_id))
>> -- 
>> 2.1.4
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Daniel Vetter Aug. 7, 2015, 11:52 a.m. UTC | #3
On Fri, Aug 07, 2015 at 11:15:56AM +0300, Mika Kuoppala wrote:
> Daniel Vetter <daniel@ffwll.ch> writes:
> 
> > On Thu, Aug 06, 2015 at 05:09:17PM +0300, Mika Kuoppala wrote:
> >> If idle to active bit is set, the rest of the fields
> >> in CSQ are not valid.
> >> 
> >> Bail out early if this is the case in order to prevent
> >> rest of the loop inspecting stale values.
> >> 
> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> >
> > Same questions here too, what's the impact. E.g. if you only found this by
> > bspec/code inspection then it's for -next, but if it's to fix some known
> > breakage then it's for -fixes + cc: stable.
> >
> 
> To this and the masked write one: Both of these were found
> when I was trying to find out root cause for skl hangs.
> 
> They are both for -next. Both are in the correctness
> department vrt bspec and I haven't observed any other
> impact.
> 
> Point taken on being more verbose.

Thanks I added a note about this to the first patch and merged it. This
one here still seems to miss an r-b.
-Daniel
arun.siluvery@linux.intel.com Aug. 7, 2015, 2:30 p.m. UTC | #4
On 07/08/2015 12:52, Daniel Vetter wrote:
> On Fri, Aug 07, 2015 at 11:15:56AM +0300, Mika Kuoppala wrote:
>> Daniel Vetter <daniel@ffwll.ch> writes:
>>
>>> On Thu, Aug 06, 2015 at 05:09:17PM +0300, Mika Kuoppala wrote:
>>>> If idle to active bit is set, the rest of the fields
>>>> in CSQ are not valid.
>>>>
>>>> Bail out early if this is the case in order to prevent
>>>> rest of the loop inspecting stale values.
>>>>
>>>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>>>

looks good to me, didn't observe any impact with this patch.
Reviewed-by: Arun Siluvery <arun.siluvery@linux.intel.com>

regards
Arun

>>> Same questions here too, what's the impact. E.g. if you only found this by
>>> bspec/code inspection then it's for -next, but if it's to fix some known
>>> breakage then it's for -fixes + cc: stable.
>>>
>>
>> To this and the masked write one: Both of these were found
>> when I was trying to find out root cause for skl hangs.
>>
>> They are both for -next. Both are in the correctness
>> department vrt bspec and I haven't observed any other
>> impact.
>>
>> Point taken on being more verbose.
>
> Thanks I added a note about this to the first patch and merged it. This
> one here still seems to miss an r-b.
> -Daniel
>
Daniel Vetter Aug. 11, 2015, 10:01 a.m. UTC | #5
On Fri, Aug 07, 2015 at 03:30:12PM +0100, Siluvery, Arun wrote:
> On 07/08/2015 12:52, Daniel Vetter wrote:
> >On Fri, Aug 07, 2015 at 11:15:56AM +0300, Mika Kuoppala wrote:
> >>Daniel Vetter <daniel@ffwll.ch> writes:
> >>
> >>>On Thu, Aug 06, 2015 at 05:09:17PM +0300, Mika Kuoppala wrote:
> >>>>If idle to active bit is set, the rest of the fields
> >>>>in CSQ are not valid.
> >>>>
> >>>>Bail out early if this is the case in order to prevent
> >>>>rest of the loop inspecting stale values.
> >>>>
> >>>>Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> >>>
> 
> looks good to me, didn't observe any impact with this patch.
> Reviewed-by: Arun Siluvery <arun.siluvery@linux.intel.com>

Queued for -next, thanks for the patch.
-Daniel

> 
> regards
> Arun
> 
> >>>Same questions here too, what's the impact. E.g. if you only found this by
> >>>bspec/code inspection then it's for -next, but if it's to fix some known
> >>>breakage then it's for -fixes + cc: stable.
> >>>
> >>
> >>To this and the masked write one: Both of these were found
> >>when I was trying to find out root cause for skl hangs.
> >>
> >>They are both for -next. Both are in the correctness
> >>department vrt bspec and I haven't observed any other
> >>impact.
> >>
> >>Point taken on being more verbose.
> >
> >Thanks I added a note about this to the first patch and merged it. This
> >one here still seems to miss an r-b.
> >-Daniel
> >
>
Shuang He Aug. 12, 2015, 8:52 p.m. UTC | #6
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 7109
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  302/302              302/302
SNB                                  315/315              315/315
IVB                                  336/336              336/336
BYT                                  283/283              283/283
HSW                                  378/378              378/378
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
Note: You need to pay more attention to line start with '*'

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 99bba8e..96218bf 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -497,6 +497,9 @@  void intel_lrc_irq_handler(struct intel_engine_cs *ring)
 		status_id = I915_READ(RING_CONTEXT_STATUS_BUF(ring) +
 				(read_pointer % 6) * 8 + 4);
 
+		if (status & GEN8_CTX_STATUS_IDLE_ACTIVE)
+			continue;
+
 		if (status & GEN8_CTX_STATUS_PREEMPTED) {
 			if (status & GEN8_CTX_STATUS_LITE_RESTORE) {
 				if (execlists_check_remove_request(ring, status_id))