Re: [PATCH] drm/i915: Initialize HWS page address after GPU reset

From: Daniel Vetter <daniel@ffwll.ch>
To: "Siluvery, Arun" <arun.siluvery@linux.intel.com>
Cc: intel-gfx <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/i915: Initialize HWS page address after GPU reset
Date: Thu, 18 Jun 2015 16:53:42 +0200	[thread overview]
Message-ID: <20150618145342.GE7752@phenom.ffwll.local> (raw)
In-Reply-To: <5582D018.7080302@linux.intel.com>

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