* [PATCH] drm/i915: Initialize HWS page address after GPU reset
@ 2015-06-02 19:06 Arun Siluvery
2015-06-03 3:13 ` shuang.he
2015-06-03 16:14 ` Ville Syrjälä
0 siblings, 2 replies; 7+ messages in thread
From: Arun Siluvery @ 2015-06-02 19:06 UTC (permalink / raw)
To: intel-gfx
After GPU reset, HW is losing the address of HWS page in the register.
The page itself is valid except that HW is not aware of its location.
[ 64.368623] [drm:gen8_init_common_ring [i915]] *ERROR* HWS Page address = 0x00000000
[ 64.368655] [drm:gen8_init_common_ring [i915]] *ERROR* HWS Page address = 0x00000000
[ 64.368681] [drm:gen8_init_common_ring [i915]] *ERROR* HWS Page address = 0x00000000
[ 64.368704] [drm:gen8_init_common_ring [i915]] *ERROR* HWS Page address = 0x00000000
This patch reloads this value into the register during ring init.
Change-Id: Ibdd1e4645921b4deb02dfc8d0d8e6ba993ce7371
Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
---
drivers/gpu/drm/i915/intel_lrc.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 0413b8f..73033f5 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1085,6 +1085,12 @@ static int gen8_init_common_ring(struct intel_engine_cs *ring)
I915_WRITE_IMR(ring, ~(ring->irq_enable_mask | ring->irq_keep_mask));
I915_WRITE(RING_HWSTAM(ring->mmio_base), 0xffffffff);
+ if (ring->status_page.obj) {
+ I915_WRITE(RING_HWS_PGA(ring->mmio_base),
+ (u32)ring->status_page.gfx_addr);
+ POSTING_READ(RING_HWS_PGA(ring->mmio_base));
+ }
+
I915_WRITE(RING_MODE_GEN7(ring),
_MASKED_BIT_DISABLE(GFX_REPLAY_MODE) |
_MASKED_BIT_ENABLE(GFX_RUN_LIST_ENABLE));
--
2.3.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Initialize HWS page address after GPU reset
2015-06-02 19:06 [PATCH] drm/i915: Initialize HWS page address after GPU reset Arun Siluvery
@ 2015-06-03 3:13 ` shuang.he
2015-06-03 16:14 ` Ville Syrjälä
1 sibling, 0 replies; 7+ messages in thread
From: shuang.he @ 2015-06-03 3:13 UTC (permalink / raw)
To: shuang.he, lei.a.liu, intel-gfx, arun.siluvery
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6526
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV 276/276 276/276
ILK 303/303 303/303
SNB -1 315/315 314/315
IVB 343/343 343/343
BYT 287/287 287/287
BDW 321/321 321/321
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*SNB igt@pm_rpm@dpms-mode-unset-non-lpsp PASS(1) DMESG_WARN(1)
(dmesg patch applied)WARNING:at_drivers/gpu/drm/i915/intel_uncore.c:#assert_device_not_suspended[i915]()@WARNING:.* at .* assert_device_not_suspended+0x
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Initialize HWS page address after GPU reset
2015-06-02 19:06 [PATCH] drm/i915: Initialize HWS page address after GPU reset Arun Siluvery
2015-06-03 3:13 ` shuang.he
@ 2015-06-03 16:14 ` Ville Syrjälä
2015-06-04 8:18 ` Jani Nikula
2015-06-15 5:20 ` Daniel Vetter
1 sibling, 2 replies; 7+ messages in thread
From: Ville Syrjälä @ 2015-06-03 16:14 UTC (permalink / raw)
To: Arun Siluvery; +Cc: intel-gfx
On Tue, Jun 02, 2015 at 08:06:59PM +0100, Arun Siluvery wrote:
> After GPU reset, HW is losing the address of HWS page in the register.
> The page itself is valid except that HW is not aware of its location.
>
> [ 64.368623] [drm:gen8_init_common_ring [i915]] *ERROR* HWS Page address = 0x00000000
> [ 64.368655] [drm:gen8_init_common_ring [i915]] *ERROR* HWS Page address = 0x00000000
> [ 64.368681] [drm:gen8_init_common_ring [i915]] *ERROR* HWS Page address = 0x00000000
> [ 64.368704] [drm:gen8_init_common_ring [i915]] *ERROR* HWS Page address = 0x00000000
>
> This patch reloads this value into the register during ring init.
>
> Change-Id: Ibdd1e4645921b4deb02dfc8d0d8e6ba993ce7371
> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_lrc.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 0413b8f..73033f5 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1085,6 +1085,12 @@ static int gen8_init_common_ring(struct intel_engine_cs *ring)
> I915_WRITE_IMR(ring, ~(ring->irq_enable_mask | ring->irq_keep_mask));
> I915_WRITE(RING_HWSTAM(ring->mmio_base), 0xffffffff);
>
> + if (ring->status_page.obj) {
> + I915_WRITE(RING_HWS_PGA(ring->mmio_base),
> + (u32)ring->status_page.gfx_addr);
> + POSTING_READ(RING_HWS_PGA(ring->mmio_base));
> + }
I was going to suggest removing the same thing from the
lrc_setup_hardware_status_page(), but after another look it seems we
sometimes call .init_hw() before the context setup. Would be nice to
have a more consistent sequence for init and reset. But anyway the patch
looks OK to me. I verified that we indeed lose this register on GPU
reset.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> +
> I915_WRITE(RING_MODE_GEN7(ring),
> _MASKED_BIT_DISABLE(GFX_REPLAY_MODE) |
> _MASKED_BIT_ENABLE(GFX_RUN_LIST_ENABLE));
> --
> 2.3.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Initialize HWS page address after GPU reset
2015-06-03 16:14 ` Ville Syrjälä
@ 2015-06-04 8:18 ` Jani Nikula
2015-06-15 5:20 ` Daniel Vetter
1 sibling, 0 replies; 7+ messages in thread
From: Jani Nikula @ 2015-06-04 8:18 UTC (permalink / raw)
To: Ville Syrjälä, Arun Siluvery; +Cc: intel-gfx
On Wed, 03 Jun 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Tue, Jun 02, 2015 at 08:06:59PM +0100, Arun Siluvery wrote:
>> After GPU reset, HW is losing the address of HWS page in the register.
>> The page itself is valid except that HW is not aware of its location.
>>
>> [ 64.368623] [drm:gen8_init_common_ring [i915]] *ERROR* HWS Page address = 0x00000000
>> [ 64.368655] [drm:gen8_init_common_ring [i915]] *ERROR* HWS Page address = 0x00000000
>> [ 64.368681] [drm:gen8_init_common_ring [i915]] *ERROR* HWS Page address = 0x00000000
>> [ 64.368704] [drm:gen8_init_common_ring [i915]] *ERROR* HWS Page address = 0x00000000
>>
>> This patch reloads this value into the register during ring init.
>>
>> Change-Id: Ibdd1e4645921b4deb02dfc8d0d8e6ba993ce7371
>> Signed-off-by: Arun Siluvery <arun.siluvery@linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_lrc.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index 0413b8f..73033f5 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -1085,6 +1085,12 @@ static int gen8_init_common_ring(struct intel_engine_cs *ring)
>> I915_WRITE_IMR(ring, ~(ring->irq_enable_mask | ring->irq_keep_mask));
>> I915_WRITE(RING_HWSTAM(ring->mmio_base), 0xffffffff);
>>
>> + if (ring->status_page.obj) {
>> + I915_WRITE(RING_HWS_PGA(ring->mmio_base),
>> + (u32)ring->status_page.gfx_addr);
>> + POSTING_READ(RING_HWS_PGA(ring->mmio_base));
>> + }
>
> I was going to suggest removing the same thing from the
> lrc_setup_hardware_status_page(), but after another look it seems we
> sometimes call .init_hw() before the context setup. Would be nice to
> have a more consistent sequence for init and reset. But anyway the patch
> looks OK to me. I verified that we indeed lose this register on GPU
> reset.
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Pushed to drm-intel-fixes, thanks for the patch and review.
BR,
Jani.
>
>> +
>> I915_WRITE(RING_MODE_GEN7(ring),
>> _MASKED_BIT_DISABLE(GFX_REPLAY_MODE) |
>> _MASKED_BIT_ENABLE(GFX_RUN_LIST_ENABLE));
>> --
>> 2.3.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Initialize HWS page address after GPU reset
2015-06-03 16:14 ` Ville Syrjälä
2015-06-04 8:18 ` Jani Nikula
@ 2015-06-15 5:20 ` Daniel Vetter
2015-06-18 14:05 ` Siluvery, Arun
1 sibling, 1 reply; 7+ messages in thread
From: Daniel Vetter @ 2015-06-15 5:20 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
On Wed, Jun 3, 2015 at 6:14 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> I was going to suggest removing the same thing from the
> lrc_setup_hardware_status_page(), but after another look it seems we
> sometimes call .init_hw() before the context setup. Would be nice to
> have a more consistent sequence for init and reset. But anyway the patch
> looks OK to me. I verified that we indeed lose this register on GPU
> reset.
Yep, this is a mess. And historically _any_ difference between driver
load and gpu reset (or resume fwiw) has lead to hilarious bugs, so
this difference is really troubling to me. Arun, can you please work
on a patch to unify the setup sequence here, so that both driver load
gpu resets work the same way? By the time we're calling gem_init_hw
the default context should have been created already, and hence we
should be able to write to HWS_PGA in ring->init_hw only.
Also I wonder about resume, where's the HWS_PGA restore for that case?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Initialize HWS page address after GPU reset
2015-06-15 5:20 ` Daniel Vetter
@ 2015-06-18 14:05 ` Siluvery, Arun
2015-06-18 14:53 ` Daniel Vetter
0 siblings, 1 reply; 7+ messages in thread
From: Siluvery, Arun @ 2015-06-18 14:05 UTC (permalink / raw)
To: Daniel Vetter, Ville Syrjälä; +Cc: intel-gfx
On 15/06/2015 06:20, Daniel Vetter wrote:
> On Wed, Jun 3, 2015 at 6:14 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
>> I was going to suggest removing the same thing from the
>> lrc_setup_hardware_status_page(), but after another look it seems we
>> sometimes call .init_hw() before the context setup. Would be nice to
>> have a more consistent sequence for init and reset. But anyway the patch
>> looks OK to me. I verified that we indeed lose this register on GPU
>> reset.
>
> Yep, this is a mess. And historically _any_ difference between driver
> load and gpu reset (or resume fwiw) has lead to hilarious bugs, so
> this difference is really troubling to me. Arun, can you please work
> on a patch to unify the setup sequence here, so that both driver load
> gpu resets work the same way? By the time we're calling gem_init_hw
> the default context should have been created already, and hence we
> should be able to write to HWS_PGA in ring->init_hw only.
>
Hi Daniel,
I think the problem in this case was the code to init HWS page after
reset was missing for Gen8+. For Gen7 we are doing this as part of
ring->init_hw.
Gen7:
i915_reset()
+--> i915_gem_init_hw()
+------> ring->init_hw() which is init_render_ring()
+----------> init_ring_common()
+------------> intel_ring_setup_status_page()
Gen8:
i915_reset()
+--> i915_gem_init_hw()
+------> ring->init_hw() which is gen8_init_render_ring()
+--------> gen8_init_common_ring() - I added changes in this function.
We could probably use intel_ring_setup_status_page() for both cases,
does it have to be Gen7 specific?
> Also I wonder about resume, where's the HWS_PGA restore for that case?
It is covered.
i915_drm_resume()
+-->i915_gem_init_hw
regards
Arun
> -Daniel
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Initialize HWS page address after GPU reset
2015-06-18 14:05 ` Siluvery, Arun
@ 2015-06-18 14:53 ` Daniel Vetter
0 siblings, 0 replies; 7+ messages in thread
From: Daniel Vetter @ 2015-06-18 14:53 UTC (permalink / raw)
To: Siluvery, Arun; +Cc: intel-gfx
On Thu, Jun 18, 2015 at 03:05:12PM +0100, Siluvery, Arun wrote:
> On 15/06/2015 06:20, Daniel Vetter wrote:
> >On Wed, Jun 3, 2015 at 6:14 PM, Ville Syrjälä
> ><ville.syrjala@linux.intel.com> wrote:
> >>I was going to suggest removing the same thing from the
> >>lrc_setup_hardware_status_page(), but after another look it seems we
> >>sometimes call .init_hw() before the context setup. Would be nice to
> >>have a more consistent sequence for init and reset. But anyway the patch
> >>looks OK to me. I verified that we indeed lose this register on GPU
> >>reset.
> >
> >Yep, this is a mess. And historically _any_ difference between driver
> >load and gpu reset (or resume fwiw) has lead to hilarious bugs, so
> >this difference is really troubling to me. Arun, can you please work
> >on a patch to unify the setup sequence here, so that both driver load
> >gpu resets work the same way? By the time we're calling gem_init_hw
> >the default context should have been created already, and hence we
> >should be able to write to HWS_PGA in ring->init_hw only.
> >
>
> Hi Daniel,
>
> I think the problem in this case was the code to init HWS page after reset
> was missing for Gen8+. For Gen7 we are doing this as part of ring->init_hw.
>
> Gen7:
> i915_reset()
> +--> i915_gem_init_hw()
> +------> ring->init_hw() which is init_render_ring()
> +----------> init_ring_common()
> +------------> intel_ring_setup_status_page()
>
> Gen8:
> i915_reset()
> +--> i915_gem_init_hw()
> +------> ring->init_hw() which is gen8_init_render_ring()
> +--------> gen8_init_common_ring() - I added changes in this function.
>
> We could probably use intel_ring_setup_status_page() for both cases, does it
> have to be Gen7 specific?
My concern isn't that we have two functions doing hws setup. My concern is
that we now have 2 callsites for execlist mode doing hws setup, with
slight differences between reset/driver load and resume.
I want one, unconditional call to set up the hws page at exactly the right
place in the setup sequence. That might require some refactoring, I
haven't looked that closely at intel_lrc.c
The usual approach is that gem_init does exclusively software setup, and
gem_init_hw does all the register writes an actual enabling. I think the
hws setup in the driver load code is currently called from gem_init(),
which is the wrong place.
> >Also I wonder about resume, where's the HWS_PGA restore for that case?
> It is covered.
>
> i915_drm_resume()
> +-->i915_gem_init_hw
Ok, so should be covered with whatever fix we have for gpu reset.
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-06-18 14:50 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-02 19:06 [PATCH] drm/i915: Initialize HWS page address after GPU reset Arun Siluvery
2015-06-03 3:13 ` shuang.he
2015-06-03 16:14 ` Ville Syrjälä
2015-06-04 8:18 ` Jani Nikula
2015-06-15 5:20 ` Daniel Vetter
2015-06-18 14:05 ` Siluvery, Arun
2015-06-18 14:53 ` Daniel Vetter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.