| Message ID | 20160429091504.GE30680@nuc-i3427.alporthouse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 29/04/16 10:15, Chris Wilson wrote: > On Fri, Apr 29, 2016 at 10:04:35AM +0100, Tvrtko Ursulin wrote: >> >> On 28/04/16 18:35, Chris Wilson wrote: >>> Move all of the constant assignments up front and into a common >>> function. This is primarily to ensure the backpointers are set as early >>> as possible for later use during initialisation. >>> >>> v2: Use a constant struct so that all the similar values are set >>> together. >>> >>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> >>> Cc: Dave Gordon <david.s.gordon@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_lrc.c | 176 +++++++++++++++++++++------------------ >>> 1 file changed, 93 insertions(+), 83 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c >>> index 874c2515f9d4..2e0eaa9fa240 100644 >>> --- a/drivers/gpu/drm/i915/intel_lrc.c >>> +++ b/drivers/gpu/drm/i915/intel_lrc.c >>> @@ -1921,8 +1921,7 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine) >>> } >>> >>> static void >>> -logical_ring_default_vfuncs(struct drm_device *dev, >>> - struct intel_engine_cs *engine) >>> +logical_ring_default_vfuncs(struct intel_engine_cs *engine) >>> { >>> /* Default vfuncs which can be overriden by each engine. */ >>> engine->init_hw = gen8_init_common_ring; >>> @@ -1933,7 +1932,7 @@ logical_ring_default_vfuncs(struct drm_device *dev, >>> engine->emit_bb_start = gen8_emit_bb_start; >>> engine->get_seqno = gen8_get_seqno; >>> engine->set_seqno = gen8_set_seqno; >>> - if (IS_BXT_REVID(dev, 0, BXT_REVID_A1)) { >>> + if (IS_BXT_REVID(engine->dev, 0, BXT_REVID_A1)) { >>> engine->irq_seqno_barrier = bxt_a_seqno_barrier; >>> engine->set_seqno = bxt_a_set_seqno; >>> } >>> @@ -1944,6 +1943,7 @@ logical_ring_default_irqs(struct intel_engine_cs *engine, unsigned shift) >>> { >>> engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT << shift; >>> engine->irq_keep_mask = GT_CONTEXT_SWITCH_INTERRUPT << shift; >>> + init_waitqueue_head(&engine->irq_queue); >>> } >>> >>> static int >>> @@ -1964,31 +1964,68 @@ lrc_setup_hws(struct intel_engine_cs *engine, >>> return 0; >>> } >>> >>> -static int >>> -logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine) >>> +static const struct logical_ring_info { >>> + const char *name; >>> + unsigned exec_id; >>> + unsigned guc_id; >>> + u32 mmio_base; >>> + unsigned irq_shift; >>> +} logical_rings[] = { >>> + [RCS] = { >>> + .name = "render ring", >>> + .exec_id = I915_EXEC_RENDER, >>> + .guc_id = GUC_RENDER_ENGINE, >>> + .mmio_base = RENDER_RING_BASE, >>> + .irq_shift = GEN8_RCS_IRQ_SHIFT, >>> + }, >>> + [BCS] = { >>> + .name = "blitter ring", >>> + .exec_id = I915_EXEC_BLT, >>> + .guc_id = GUC_BLITTER_ENGINE, >>> + .mmio_base = BLT_RING_BASE, >>> + .irq_shift = GEN8_BCS_IRQ_SHIFT, >>> + }, >>> + [VCS] = { >>> + .name = "bsd ring", >>> + .exec_id = I915_EXEC_BSD, >>> + .guc_id = GUC_VIDEO_ENGINE, >>> + .mmio_base = GEN6_BSD_RING_BASE, >>> + .irq_shift = GEN8_VCS1_IRQ_SHIFT, >>> + }, >>> + [VCS2] = { >>> + .name = "bsd2 ring", >>> + .exec_id = I915_EXEC_BSD, >>> + .guc_id = GUC_VIDEO_ENGINE2, >>> + .mmio_base = GEN8_BSD2_RING_BASE, >>> + .irq_shift = GEN8_VCS2_IRQ_SHIFT, >>> + }, >>> + [VECS] = { >>> + .name = "video enhancement ring", >>> + .exec_id = I915_EXEC_VEBOX, >>> + .guc_id = GUC_VIDEOENHANCE_ENGINE, >>> + .mmio_base = VEBOX_RING_BASE, >>> + .irq_shift = GEN8_VECS_IRQ_SHIFT, >>> + }, >>> +}; >>> + >>> +static struct intel_engine_cs * >>> +logical_ring_setup(struct drm_device *dev, enum intel_engine_id id) >>> { >> >> Would dev_priv be better? Just to gradually move towards the correct >> state of things. > > I have a patch queued up to do engine->i915 (1 KiB in object code > reduction) next. > >>> + const struct logical_ring_info *info = &logical_rings[id]; >>> struct drm_i915_private *dev_priv = to_i915(dev); >>> - struct intel_context *dctx = dev_priv->kernel_context; >>> + struct intel_engine_cs *engine = &dev_priv->engine[id]; >>> enum forcewake_domains fw_domains; >>> - int ret; >>> - >>> - /* Intentionally left blank. */ >>> - engine->buffer = NULL; >>> >>> engine->dev = dev; >> >> Looking at usages of intel_engine_initialized... one potential >> danger scenario would be interrupt noise during driver load end in >> notify ring and explodes. Sounds very unlikely but theoretically it >> is a regression compared to where engine->dev initialization was >> before. >> >> We should really move away from engine->dev for this and just add an >> explicit flag. > > Hmm. not that but I think we really should be sanitizing the irq here > and enabling them last. > > Like: > > 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? Regards, Tvrtko
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. -Chris
On Fri, Apr 29, 2016 at 10:25:41AM +0100, Tvrtko Ursulin wrote: > 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? The other thing I've been mulling over is just making for_each_engine iterate over the ring_mask. Means the reinstatement of temporary variables (at least for the macros I've sketched) :( -Chris
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. Engine setup would then work fine - I don't see it needed for_each_engine or intel_engine_initialized ? And problem of hw sanitation could then be left out of this patch/discussion, no? Regards, Tvrtko
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. -Chris
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. Regards, Tvrtko
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
On Fri, Apr 29, 2016 at 11:22:59AM +0100, Chris Wilson wrote: > 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. Imo the low-level irq clearing should all be done in the relevant irq setup code in i915_irq.c. Atm we just forgot to do that. I guess you can have a bikeshed whether the enginer IMR enable/disable functions should be together with the clearing or not, placing them in either file is fine. But since we already clear the higher-level IMR registers in i915_irq.c we might as well clear the low-level ones too in i915_irq.c. -Daniel
On Mon, May 02, 2016 at 10:51:31AM +0200, Daniel Vetter wrote: > Imo the low-level irq clearing should all be done in the relevant irq > setup code in i915_irq.c. Atm we just forgot to do that. I guess you can > have a bikeshed whether the enginer IMR enable/disable functions should be > together with the clearing or not, placing them in either file is fine. > But since we already clear the higher-level IMR registers in i915_irq.c we > might as well clear the low-level ones too in i915_irq.c. That's tricky since that is done before the engines - so how do we get the various bases to i915_irq.c without duplication? Enabling the irq for the engines is part of init_hw, so correspondingly putting the early disable into init looks fine to me. -Chris
On Mon, May 02, 2016 at 11:58:38AM +0100, Chris Wilson wrote: > On Mon, May 02, 2016 at 10:51:31AM +0200, Daniel Vetter wrote: > > Imo the low-level irq clearing should all be done in the relevant irq > > setup code in i915_irq.c. Atm we just forgot to do that. I guess you can > > have a bikeshed whether the enginer IMR enable/disable functions should be > > together with the clearing or not, placing them in either file is fine. > > But since we already clear the higher-level IMR registers in i915_irq.c we > > might as well clear the low-level ones too in i915_irq.c. > > That's tricky since that is done before the engines - so how do we get > the various bases to i915_irq.c without duplication? Enabling the irq > for the engines is part of init_hw, so correspondingly putting the > early disable into init looks fine to me. I just don't particularly like that we have hw access in a function that thus far (and at a glance still after this patch) only sets up software state. I'll probably lead to some ugly surprise. That's why I'd like to move this either to engine->init_hw or to i915_irq.c. -Daniel
On Mon, May 09, 2016 at 09:02:33AM +0200, Daniel Vetter wrote: > On Mon, May 02, 2016 at 11:58:38AM +0100, Chris Wilson wrote: > > On Mon, May 02, 2016 at 10:51:31AM +0200, Daniel Vetter wrote: > > > Imo the low-level irq clearing should all be done in the relevant irq > > > setup code in i915_irq.c. Atm we just forgot to do that. I guess you can > > > have a bikeshed whether the enginer IMR enable/disable functions should be > > > together with the clearing or not, placing them in either file is fine. > > > But since we already clear the higher-level IMR registers in i915_irq.c we > > > might as well clear the low-level ones too in i915_irq.c. > > > > That's tricky since that is done before the engines - so how do we get > > the various bases to i915_irq.c without duplication? Enabling the irq > > for the engines is part of init_hw, so correspondingly putting the > > early disable into init looks fine to me. > > I just don't particularly like that we have hw access in a function that > thus far (and at a glance still after this patch) only sets up software > state. I'll probably lead to some ugly surprise. That's why I'd like to > move this either to engine->init_hw or to i915_irq.c. This is sanitize. We do enable it in engine->init_hw(), but the point raised by Ville earlier in his review of GT irq handling is that nobody currently disables the ring IMR before use. Here we have a chicken-and-egg problem, do we duplicate knowledge of available engines (and their mmio_base) in irq preinstall/sanitize or do we do the engine specific mmio in engine initialisation? The problem Turslin was raising was that on future enabling, somebody had enabled the engine IRQ before the engines were initialised (i.e. had completely disregarded the current init_hw sequence). Plonking it in i915_irq.c is not foolproof either! -Chris
On Mon, May 09, 2016 at 08:45:16AM +0100, Chris Wilson wrote: > On Mon, May 09, 2016 at 09:02:33AM +0200, Daniel Vetter wrote: > > On Mon, May 02, 2016 at 11:58:38AM +0100, Chris Wilson wrote: > > > On Mon, May 02, 2016 at 10:51:31AM +0200, Daniel Vetter wrote: > > > > Imo the low-level irq clearing should all be done in the relevant irq > > > > setup code in i915_irq.c. Atm we just forgot to do that. I guess you can > > > > have a bikeshed whether the enginer IMR enable/disable functions should be > > > > together with the clearing or not, placing them in either file is fine. > > > > But since we already clear the higher-level IMR registers in i915_irq.c we > > > > might as well clear the low-level ones too in i915_irq.c. > > > > > > That's tricky since that is done before the engines - so how do we get > > > the various bases to i915_irq.c without duplication? Enabling the irq > > > for the engines is part of init_hw, so correspondingly putting the > > > early disable into init looks fine to me. > > > > I just don't particularly like that we have hw access in a function that > > thus far (and at a glance still after this patch) only sets up software > > state. I'll probably lead to some ugly surprise. That's why I'd like to > > move this either to engine->init_hw or to i915_irq.c. > > This is sanitize. We do enable it in engine->init_hw(), but the point > raised by Ville earlier in his review of GT irq handling is that nobody > currently disables the ring IMR before use. Here we have a > chicken-and-egg problem, do we duplicate knowledge of available engines > (and their mmio_base) in irq preinstall/sanitize or do we do the engine > specific mmio in engine initialisation? The problem Turslin was raising > was that on future enabling, somebody had enabled the engine IRQ before > the engines were initialised (i.e. had completely disregarded the > current init_hw sequence). Plonking it in i915_irq.c is not foolproof > either! Hm, couldn't we put it into init_hw? i915_irq.c sets up the top-level interrupts, but for GT stuff all masked. In init_hw we could clear that then, and before init_hw no one should call ring->get_irq to enable it and potentially cause havoc. Or still too fragile in your opinion? Indeed putting it into i915_irq.c seems to not be great since it splits the gt per-engine mask reg handling too far. -Daniel
On Mon, May 09, 2016 at 09:58:20AM +0200, Daniel Vetter wrote: > On Mon, May 09, 2016 at 08:45:16AM +0100, Chris Wilson wrote: > > This is sanitize. We do enable it in engine->init_hw(), but the point > > raised by Ville earlier in his review of GT irq handling is that nobody > > currently disables the ring IMR before use. Here we have a > > chicken-and-egg problem, do we duplicate knowledge of available engines > > (and their mmio_base) in irq preinstall/sanitize or do we do the engine > > specific mmio in engine initialisation? The problem Turslin was raising > > was that on future enabling, somebody had enabled the engine IRQ before > > the engines were initialised (i.e. had completely disregarded the > > current init_hw sequence). Plonking it in i915_irq.c is not foolproof > > either! > > Hm, couldn't we put it into init_hw? i915_irq.c sets up the top-level > interrupts, but for GT stuff all masked. In init_hw we could clear that > then, and before init_hw no one should call ring->get_irq to enable it and > potentially cause havoc. Or still too fragile in your opinion? The race is if we get an interrupt inside init_engine, after we set engine->dev but before we setup the state for the irq handler. (Note the race isn't strictly just dev, everything we touch inside the irq handler gives arise to a potential ordering issue.) > Indeed putting it into i915_irq.c seems to not be great since it splits > the gt per-engine mask reg handling too far. Yeah. -Chris
On Mon, May 09, 2016 at 11:41:41AM +0100, Chris Wilson wrote: > On Mon, May 09, 2016 at 09:58:20AM +0200, Daniel Vetter wrote: > > On Mon, May 09, 2016 at 08:45:16AM +0100, Chris Wilson wrote: > > > This is sanitize. We do enable it in engine->init_hw(), but the point > > > raised by Ville earlier in his review of GT irq handling is that nobody > > > currently disables the ring IMR before use. Here we have a > > > chicken-and-egg problem, do we duplicate knowledge of available engines > > > (and their mmio_base) in irq preinstall/sanitize or do we do the engine > > > specific mmio in engine initialisation? The problem Turslin was raising > > > was that on future enabling, somebody had enabled the engine IRQ before > > > the engines were initialised (i.e. had completely disregarded the > > > current init_hw sequence). Plonking it in i915_irq.c is not foolproof > > > either! > > > > Hm, couldn't we put it into init_hw? i915_irq.c sets up the top-level > > interrupts, but for GT stuff all masked. In init_hw we could clear that > > then, and before init_hw no one should call ring->get_irq to enable it and > > potentially cause havoc. Or still too fragile in your opinion? > > The race is if we get an interrupt inside init_engine, after we set > engine->dev but before we setup the state for the irq handler. (Note the > race isn't strictly just dev, everything we touch inside the irq handler > gives arise to a potential ordering issue.) But how does this happen? Assuming we did mask all the higher bits correctly beforehand ... Is this just theoretical (in which case I think cleanup in init_hw is totally fine), or did it go kaboom already? -Daniel
On Tue, May 10, 2016 at 09:46:05AM +0200, Daniel Vetter wrote: > On Mon, May 09, 2016 at 11:41:41AM +0100, Chris Wilson wrote: > > On Mon, May 09, 2016 at 09:58:20AM +0200, Daniel Vetter wrote: > > > On Mon, May 09, 2016 at 08:45:16AM +0100, Chris Wilson wrote: > > > > This is sanitize. We do enable it in engine->init_hw(), but the point > > > > raised by Ville earlier in his review of GT irq handling is that nobody > > > > currently disables the ring IMR before use. Here we have a > > > > chicken-and-egg problem, do we duplicate knowledge of available engines > > > > (and their mmio_base) in irq preinstall/sanitize or do we do the engine > > > > specific mmio in engine initialisation? The problem Turslin was raising > > > > was that on future enabling, somebody had enabled the engine IRQ before > > > > the engines were initialised (i.e. had completely disregarded the > > > > current init_hw sequence). Plonking it in i915_irq.c is not foolproof > > > > either! > > > > > > Hm, couldn't we put it into init_hw? i915_irq.c sets up the top-level > > > interrupts, but for GT stuff all masked. In init_hw we could clear that > > > then, and before init_hw no one should call ring->get_irq to enable it and > > > potentially cause havoc. Or still too fragile in your opinion? > > > > The race is if we get an interrupt inside init_engine, after we set > > engine->dev but before we setup the state for the irq handler. (Note the > > race isn't strictly just dev, everything we touch inside the irq handler > > gives arise to a potential ordering issue.) > > But how does this happen? Assuming we did mask all the higher bits > correctly beforehand ... Is this just theoretical (in which case I think > cleanup in init_hw is totally fine), or did it go kaboom already? We haven't masked all the bits correctly beforehand. We don't today, and tomorrow someone thought it would be fun to enable the GT interrupts before the engines were initialised. Still, it can only go bang if the interrupt was asserted - so a particularly interesting bios or kexec / hibernation scenario. -Chris
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;