* [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.