Re: [PATCH v2] drm/i915/execlists: Refactor common engine setup

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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




_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux