| Message ID | 1422449006-4028-2-git-send-email-mika.kuoppala@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jan 28, 2015 at 02:43:25PM +0200, Mika Kuoppala wrote: > commit 05a2fb157e44a53c79133805d30eaada43911941 > Author: Mika Kuoppala <mika.kuoppala@linux.intel.com> > Date: Mon Jan 19 16:20:43 2015 +0200 > > drm/i915: Consolidate forcewake code > > introduced domain handling where each domain has it's own posting > read registers. This changed the forcewake sequence on 'put' side when > there is multiple domains as there would be extra read between the domain > puts. Any posting read should be enough to flush all the changes. > > Do a posting read only once, at the end of the sequence and for > the first domain. Like it was before. > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> fwiw, I would argue that the posting read in _get() is superfluous as we will serialise the fw with not only the ack, but any subsequent mmio. On the _put() side we do want to flush the write so that the hw can power down as early as possible. So just kill the posting read from _get and otherwise drop the patch. :) -Chris
Chris Wilson <chris@chris-wilson.co.uk> writes: > On Wed, Jan 28, 2015 at 02:43:25PM +0200, Mika Kuoppala wrote: >> commit 05a2fb157e44a53c79133805d30eaada43911941 >> Author: Mika Kuoppala <mika.kuoppala@linux.intel.com> >> Date: Mon Jan 19 16:20:43 2015 +0200 >> >> drm/i915: Consolidate forcewake code >> >> introduced domain handling where each domain has it's own posting >> read registers. This changed the forcewake sequence on 'put' side when >> there is multiple domains as there would be extra read between the domain >> puts. Any posting read should be enough to flush all the changes. >> >> Do a posting read only once, at the end of the sequence and for >> the first domain. Like it was before. >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > > fwiw, I would argue that the posting read in _get() is superfluous as we > will serialise the fw with not only the ack, but any subsequent mmio. > > On the _put() side we do want to flush the write so that the hw can > power down as early as possible. So just kill the posting read from _get > and otherwise drop the patch. :) Yes, both put/get patches should be dropped. I posted a patch removing the posting read on get side and with your explanations in commit message. This all starts to make so much sense that some gen is bound to break ;) -Mika > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Wed, Jan 28, 2015 at 03:28:56PM +0200, Mika Kuoppala wrote: > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > On Wed, Jan 28, 2015 at 02:43:25PM +0200, Mika Kuoppala wrote: > >> commit 05a2fb157e44a53c79133805d30eaada43911941 > >> Author: Mika Kuoppala <mika.kuoppala@linux.intel.com> > >> Date: Mon Jan 19 16:20:43 2015 +0200 > >> > >> drm/i915: Consolidate forcewake code > >> > >> introduced domain handling where each domain has it's own posting > >> read registers. This changed the forcewake sequence on 'put' side when > >> there is multiple domains as there would be extra read between the domain > >> puts. Any posting read should be enough to flush all the changes. > >> > >> Do a posting read only once, at the end of the sequence and for > >> the first domain. Like it was before. > >> > >> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > > > > fwiw, I would argue that the posting read in _get() is superfluous as we > > will serialise the fw with not only the ack, but any subsequent mmio. > > > > On the _put() side we do want to flush the write so that the hw can > > power down as early as possible. So just kill the posting read from _get > > and otherwise drop the patch. :) > > Yes, both put/get patches should be dropped. I posted a patch removing > the posting read on get side and with your explanations in commit message. > > This all starts to make so much sense that some gen is bound to break ;) IIRC the posting read from same cache line actually fixed real bugs. So I'm a bit worried about dropping them. But I suppose it's possible only the _put side was important for those bugs.
On Wed, Jan 28, 2015 at 03:25:05PM +0200, Mika Kuoppala wrote: > The checking for ack and also any subsequent mmio access > will serialize with setting the forcewake bit. Drop the > posting read as superfluous. > > Note that in the put side we still want to keep the posting read > as it will ensure that the hw sees our forcewake release in a > timely manner and doesn't keep the hw powered up. > > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
Ville Syrjälä <ville.syrjala@linux.intel.com> writes: > On Wed, Jan 28, 2015 at 03:28:56PM +0200, Mika Kuoppala wrote: >> Chris Wilson <chris@chris-wilson.co.uk> writes: >> >> > On Wed, Jan 28, 2015 at 02:43:25PM +0200, Mika Kuoppala wrote: >> >> commit 05a2fb157e44a53c79133805d30eaada43911941 >> >> Author: Mika Kuoppala <mika.kuoppala@linux.intel.com> >> >> Date: Mon Jan 19 16:20:43 2015 +0200 >> >> >> >> drm/i915: Consolidate forcewake code >> >> >> >> introduced domain handling where each domain has it's own posting >> >> read registers. This changed the forcewake sequence on 'put' side when >> >> there is multiple domains as there would be extra read between the domain >> >> puts. Any posting read should be enough to flush all the changes. >> >> >> >> Do a posting read only once, at the end of the sequence and for >> >> the first domain. Like it was before. >> >> >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> >> > >> > fwiw, I would argue that the posting read in _get() is superfluous as we >> > will serialise the fw with not only the ack, but any subsequent mmio. >> > >> > On the _put() side we do want to flush the write so that the hw can >> > power down as early as possible. So just kill the posting read from _get >> > and otherwise drop the patch. :) >> >> Yes, both put/get patches should be dropped. I posted a patch removing >> the posting read on get side and with your explanations in commit message. >> >> This all starts to make so much sense that some gen is bound to break ;) > > IIRC the posting read from same cache line actually fixed real bugs. So > I'm a bit worried about dropping them. But I suppose it's possible only > the _put side was important for those bugs. I found these: commit 6af2d180f82151cf3d58952e35a4f96e45bc453a Author: Daniel Vetter <daniel.vetter@ffwll.ch> Date: Thu Jul 26 16:24:50 2012 +0200 drm/i915: fix forcewake related hangs on snb commit 8dee3eea3ccd3b6c00a8d3a08dd715d6adf737dd Author: Ben Widawsky <ben@bwidawsk.net> Date: Sat Sep 1 22:59:50 2012 -0700 drm/i915: Never read FORCEWAKE https://bugs.freedesktop.org/show_bug.cgi?id=51738 https://bugs.freedesktop.org/show_bug.cgi?id=52424 The snb here seems to survive gem_dummy_reloc_loop and gem_ring_sync_loop in here with the get side posting removed. -Mika > > -- > Ville Syrjälä > Intel OTC
On Wed, Jan 28, 2015 at 05:54:14PM +0200, Mika Kuoppala wrote: > Ville Syrjälä <ville.syrjala@linux.intel.com> writes: > > > On Wed, Jan 28, 2015 at 03:28:56PM +0200, Mika Kuoppala wrote: > >> Chris Wilson <chris@chris-wilson.co.uk> writes: > >> > >> > On Wed, Jan 28, 2015 at 02:43:25PM +0200, Mika Kuoppala wrote: > >> >> commit 05a2fb157e44a53c79133805d30eaada43911941 > >> >> Author: Mika Kuoppala <mika.kuoppala@linux.intel.com> > >> >> Date: Mon Jan 19 16:20:43 2015 +0200 > >> >> > >> >> drm/i915: Consolidate forcewake code > >> >> > >> >> introduced domain handling where each domain has it's own posting > >> >> read registers. This changed the forcewake sequence on 'put' side when > >> >> there is multiple domains as there would be extra read between the domain > >> >> puts. Any posting read should be enough to flush all the changes. > >> >> > >> >> Do a posting read only once, at the end of the sequence and for > >> >> the first domain. Like it was before. > >> >> > >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > >> > > >> > fwiw, I would argue that the posting read in _get() is superfluous as we > >> > will serialise the fw with not only the ack, but any subsequent mmio. > >> > > >> > On the _put() side we do want to flush the write so that the hw can > >> > power down as early as possible. So just kill the posting read from _get > >> > and otherwise drop the patch. :) > >> > >> Yes, both put/get patches should be dropped. I posted a patch removing > >> the posting read on get side and with your explanations in commit message. > >> > >> This all starts to make so much sense that some gen is bound to break ;) > > > > IIRC the posting read from same cache line actually fixed real bugs. So > > I'm a bit worried about dropping them. But I suppose it's possible only > > the _put side was important for those bugs. > > I found these: > > commit 6af2d180f82151cf3d58952e35a4f96e45bc453a > Author: Daniel Vetter <daniel.vetter@ffwll.ch> > Date: Thu Jul 26 16:24:50 2012 +0200 > > drm/i915: fix forcewake related hangs on snb > > commit 8dee3eea3ccd3b6c00a8d3a08dd715d6adf737dd > Author: Ben Widawsky <ben@bwidawsk.net> > Date: Sat Sep 1 22:59:50 2012 -0700 > > drm/i915: Never read FORCEWAKE > > https://bugs.freedesktop.org/show_bug.cgi?id=51738 > https://bugs.freedesktop.org/show_bug.cgi?id=52424 > > The snb here seems to survive gem_dummy_reloc_loop and > gem_ring_sync_loop in here with the get side posting removed. Note that we kept the once associated with #52424, but judging by my comments in #51738 the posting read is just a band aid anyway as a full mb() itself was not adequate. -Chris
On Wed, Jan 28, 2015 at 02:04:27PM +0000, Chris Wilson wrote: > On Wed, Jan 28, 2015 at 03:25:05PM +0200, Mika Kuoppala wrote: > > The checking for ack and also any subsequent mmio access > > will serialize with setting the forcewake bit. Drop the > > posting read as superfluous. > > > > Note that in the put side we still want to keep the posting read > > as it will ensure that the hw sees our forcewake release in a > > timely manner and doesn't keep the hw powered up. > > > > Suggested-by: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> This and 1/3 merged, thansk for patches&review. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c index be2c7fc..ebd9068 100644 --- a/drivers/gpu/drm/i915/intel_uncore.c +++ b/drivers/gpu/drm/i915/intel_uncore.c @@ -123,42 +123,42 @@ fw_domain_posting_read(const struct intel_uncore_forcewake_domain *d) } static void -fw_domains_get(struct drm_i915_private *dev_priv, enum forcewake_domains fw_domains) +fw_domains_posting_read(struct drm_i915_private *dev_priv) { struct intel_uncore_forcewake_domain *d; enum forcewake_domain_id id; - for_each_fw_domain_mask(d, fw_domains, dev_priv, id) { - fw_domain_wait_ack_clear(d); - fw_domain_get(d); + /* No need to do for all, just do for first found */ + for_each_fw_domain(d, dev_priv, id) { fw_domain_posting_read(d); - fw_domain_wait_ack(d); + break; } } static void -fw_domains_put(struct drm_i915_private *dev_priv, enum forcewake_domains fw_domains) +fw_domains_get(struct drm_i915_private *dev_priv, enum forcewake_domains fw_domains) { struct intel_uncore_forcewake_domain *d; enum forcewake_domain_id id; for_each_fw_domain_mask(d, fw_domains, dev_priv, id) { - fw_domain_put(d); + fw_domain_wait_ack_clear(d); + fw_domain_get(d); fw_domain_posting_read(d); + fw_domain_wait_ack(d); } } static void -fw_domains_posting_read(struct drm_i915_private *dev_priv) +fw_domains_put(struct drm_i915_private *dev_priv, enum forcewake_domains fw_domains) { struct intel_uncore_forcewake_domain *d; enum forcewake_domain_id id; - /* No need to do for all, just do for first found */ - for_each_fw_domain(d, dev_priv, id) { - fw_domain_posting_read(d); - break; - } + for_each_fw_domain_mask(d, fw_domains, dev_priv, id) + fw_domain_put(d); + + fw_domains_posting_read(dev_priv); } static void
commit 05a2fb157e44a53c79133805d30eaada43911941 Author: Mika Kuoppala <mika.kuoppala@linux.intel.com> Date: Mon Jan 19 16:20:43 2015 +0200 drm/i915: Consolidate forcewake code introduced domain handling where each domain has it's own posting read registers. This changed the forcewake sequence on 'put' side when there is multiple domains as there would be extra read between the domain puts. Any posting read should be enough to flush all the changes. Do a posting read only once, at the end of the sequence and for the first domain. Like it was before. Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> --- drivers/gpu/drm/i915/intel_uncore.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)