Re: [PATCH 3/3] drm/i915: Only grab correct forcewake for the engine with execlists

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

 




On 07/04/16 15:24, Chris Wilson wrote:
On Thu, Apr 07, 2016 at 03:05:40PM +0100, Tvrtko Ursulin wrote:
@@ -2099,6 +2101,12 @@ logical_ring_init(struct drm_device *dev, struct intel_engine_cs *engine)

  	logical_ring_init_platform_invariants(engine);

+	engine->fw_domains_elsp =
+		intel_reg_write_fw_domains(dev_priv, RING_ELSP(engine));
+	engine->fw_domains_csb =
+		intel_reg_write_fw_domains(dev_priv,
+					   RING_CONTEXT_STATUS_PTR(engine));

So is write a superset of fw? Tends to be reads that require fw more
than writes (gen6/7 fifo, gen8 write shadowing).

I think we need a READ | WRITE direction field.

Hm, yes embedding too much knowledge in the caller, how about just:

	engine->fw_domains_csb = intel_reg_read_fw_domains(dev_priv,
				 RING_CONTEXT_STATUS_PTR(engine));

	engine->fw_domains_csb |= intel_reg_write_fw_domains(dev_priv,
				  RING_CONTEXT_STATUS_PTR(engine));

?

+/**
+ * intel_reg_write_fw_domains - which forcewake domains are needed to write a register
+ * @dev_priv: pointer to struct drm_i915_private
+ * @reg: register in question
+ *
+ * Returns a set of forcewake domains required to be taken with for example
+ * intel_uncore_forcewake_get for the specified register to be writable with the
+ * raw mmio accessors.
+ */
+enum forcewake_domains
+intel_reg_write_fw_domains(struct drm_i915_private *dev_priv, i915_reg_t reg)
+{
+	enum forcewake_domains fw_domains;
+
+	if (intel_vgpu_active(dev_priv->dev))
+		return 0;
+
+	switch (INTEL_INFO(dev_priv)->gen) {
+	case 9:
+		fw_domains = __gen9_reg_write_fw_domains(i915_mmio_reg_offset(reg));
+		break;
+	case 8:
+		if (IS_CHERRYVIEW(dev_priv))
+			fw_domains = __chv_reg_write_fw_domains(i915_mmio_reg_offset(reg));
+		else
+			fw_domains = __gen8_reg_write_fw_domains(i915_mmio_reg_offset(reg));
+		break;
+	default:
+		MISSING_CASE(INTEL_INFO(dev_priv)->gen);
+	case 7:
+	case 6:

This is actually a tricky one. gen6/7 maintain a FIFO to store mmio
writes whilst it is powered down. If we fill that fifo we drop writes
(and that fifo is shared with functions on the device, i.e. it is not
ours to fill exclusively). So should we be saving that if you want to
make lots of writes you should take this forcewake domain. Yes. We should
report what domains they would require, it is still up to the caller as
to whether they risk the FIFO overflowing, but they should have the right
information to hand.

Missed that. But it isn't part of forcewake domains. So what would you return? Fake out a new domain just complicates things and adds cost for everyone. Maybe better just to limit the whole thing to gen8+ and leave olders platforms untouched?

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