From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
Intel-gfx@lists.freedesktop.org,
Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Subject: Re: [PATCH 3/3] drm/i915: Only grab correct forcewake for the engine with execlists
Date: Thu, 7 Apr 2016 15:36:04 +0100 [thread overview]
Message-ID: <57067054.6090103@linux.intel.com> (raw)
In-Reply-To: <20160407142431.GE18061@nuc-i3427.alporthouse.com>
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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2016-04-07 14:36 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-07 14:05 [PATCH 1/3] drm/i915: Extract knowledge of register forcewake domains Tvrtko Ursulin
2016-04-07 14:05 ` [PATCH 2/3] drm/i915: Remove forcewake request registers from the shadowed table Tvrtko Ursulin
2016-04-07 14:41 ` Chris Wilson
2016-04-07 14:05 ` [PATCH 3/3] drm/i915: Only grab correct forcewake for the engine with execlists Tvrtko Ursulin
2016-04-07 14:24 ` Chris Wilson
2016-04-07 14:36 ` Tvrtko Ursulin [this message]
2016-04-07 14:59 ` Chris Wilson
2016-04-07 14:35 ` Chris Wilson
2016-04-07 14:52 ` Tvrtko Ursulin
2016-04-07 15:33 ` Chris Wilson
2016-04-07 15:56 ` [PATCH v3 " Tvrtko Ursulin
2016-04-12 13:18 ` Chris Wilson
2016-04-07 14:40 ` [PATCH 1/3] drm/i915: Extract knowledge of register forcewake domains Chris Wilson
2016-04-07 16:32 ` ✗ Fi.CI.BAT: failure for series starting with [1/3] " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=57067054.6090103@linux.intel.com \
--to=tvrtko.ursulin@linux.intel.com \
--cc=Intel-gfx@lists.freedesktop.org \
--cc=chris@chris-wilson.co.uk \
--cc=tvrtko.ursulin@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.