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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
