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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
