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

[Chris Wilson]

On Thu, Apr 07, 2016 at 03:36:04PM +0100, Tvrtko Ursulin wrote:
> 
> 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));

See my other mail for a mockup of some code. Something along these
lines.

> >>+/**
> >>+ * 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?

It is the FORCEWAKE_RENDER to flush the write FIFO, it is just not clear
in the implementation (i.e. we have never done that explicitly - I do
remember at odd times counting register writes though...). Only in a few
places (over ring init) have we explicitly taken the fw domain across writes.

I'm leaning towards reporting that they do require the domain, with a
note saying about the fifo. It is a specialized interface after all that
is going to be using in fairly gen-specific paths (and erring on the
side of caution when used outside of that is wise).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre