On Fri, Apr 29, 2016 at 11:11:20AM +0100, Tvrtko Ursulin wrote: > > On 29/04/16 11:00, Chris Wilson wrote: > >On Fri, Apr 29, 2016 at 10:50:02AM +0100, Tvrtko Ursulin wrote: > >> > >>On 29/04/16 10:39, Chris Wilson wrote: > >>>On Fri, Apr 29, 2016 at 10:25:41AM +0100, Tvrtko Ursulin wrote: > >>>>On 29/04/16 10:15, Chris Wilson wrote: > >>>>>diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > >>>>>index 2e0eaa9fa240..2c94072ab085 100644 > >>>>>--- a/drivers/gpu/drm/i915/intel_lrc.c > >>>>>+++ b/drivers/gpu/drm/i915/intel_lrc.c > >>>>>@@ -2016,14 +2016,17 @@ logical_ring_setup(struct drm_device *dev, enum intel_engine_id id) > >>>>> struct intel_engine_cs *engine = &dev_priv->engine[id]; > >>>>> enum forcewake_domains fw_domains; > >>>>> > >>>>>- engine->dev = dev; > >>>>>- > >>>>> engine->id = id; > >>>>> engine->name = info->name; > >>>>> engine->exec_id = info->exec_id; > >>>>> engine->guc_id = info->guc_id; > >>>>> engine->mmio_base = info->mmio_base; > >>>>> > >>>>>+ /* disable interrupts to this engine before we install ourselves*/ > >>>>>+ I915_WRITE_IMR(engine, ~0); > >>>>>+ > >>>>>+ engine->dev = dev; > >>>>>+ > >>>>> /* Intentionally left blank. */ > >>>>> engine->buffer = NULL; > >>>>> > >>>>>Make sense? > >>>> > >>>>Not the most elegant because all the hw access we have so far is in > >>>>engine->init_hw. Why can't we just make intel_engine_initialized > >>>>return false until the very last thing in engine constructors? > >>> > >>>In my defence sanitizing the hw before we are ready is common practice > >>>across the driver. The unfun part is that irq install is before gem_init > >>>(because modeset init wants irq enabled for GMBUS/dp-aux). gem init > >>>itself could be split up and moved around so that the setup and init_hw > >>>phases are separate (which would be next on the init ordering hitlist I > >>>hope). > >>> > >>>I want engine->dev/engine->i915 set early so we can use it during setup > >>>and init-hw and so that for_each_engine() works as expected in that > >>>time. > >> > >>Why wouldn't an explicit engine->initialized flag solve that? You > >>could keep setting engine->dev early (as it should be) and then set > >>engine->initialized at the end of per-engine constructors. > > > >Because it becomes irrelevant very shortly. The only interesting > >question remaining is whether or not we should be sanitizing the IMR > >first. It has been suggested elsewhere (in Ville's review of the GT > >interrupt handling) that doing the sanitization would be useful. > > How come it becomes irrelevant? Will there not be > intel_engine_initialized? Because as long as there is, imho it makes > sense not to use engine->dev for it. The only argument here is how to handle an interrupt left over from before the driver loads. At all other times engine->dev means precisely that. I do not agree that you need to duplicate the state. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
