* [PATCH] drm/i915: fix comment referencing imaginary functions @ 2016-08-24 16:03 Matthew Auld 2016-08-24 16:50 ` ✗ Fi.CI.BAT: failure for " Patchwork 2016-10-21 12:16 ` [PATCH] " Arkadiusz Hiler 0 siblings, 2 replies; 9+ messages in thread From: Matthew Auld @ 2016-08-24 16:03 UTC (permalink / raw) To: intel-gfx The comment which documents the proper usage of the *_FW family of macros makes reference to intel_uncore_forcewake_irq{unlock, lock}, which is just confusing, seeing as such a set of functions don't even exist and never have for that matter(according to git). Let's fix that by replacing them with intel_uncore_forcewake_{get, put}. Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Matthew Auld <matthew.auld@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index ff96b7a..c285d61 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3792,8 +3792,8 @@ __raw_write(64, q) * critical sections inside IRQ handlers where forcewake is explicitly * controlled. * Think twice, and think again, before using these. - * Note: Should only be used between intel_uncore_forcewake_irqlock() and - * intel_uncore_forcewake_irqunlock(). + * Note: Should only be used between intel_uncore_forcewake_get and + * intel_uncore_forcewake_put. */ #define I915_READ_FW(reg__) __raw_i915_read32(dev_priv, (reg__)) #define I915_WRITE_FW(reg__, val__) __raw_i915_write32(dev_priv, (reg__), (val__)) -- 2.7.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 9+ messages in thread
* ✗ Fi.CI.BAT: failure for drm/i915: fix comment referencing imaginary functions 2016-08-24 16:03 [PATCH] drm/i915: fix comment referencing imaginary functions Matthew Auld @ 2016-08-24 16:50 ` Patchwork 2016-10-21 12:16 ` [PATCH] " Arkadiusz Hiler 1 sibling, 0 replies; 9+ messages in thread From: Patchwork @ 2016-08-24 16:50 UTC (permalink / raw) To: Matthew Auld; +Cc: intel-gfx == Series Details == Series: drm/i915: fix comment referencing imaginary functions URL : https://patchwork.freedesktop.org/series/11527/ State : failure == Summary == Series 11527v1 drm/i915: fix comment referencing imaginary functions http://patchwork.freedesktop.org/api/1.0/series/11527/revisions/1/mbox/ Test kms_cursor_legacy: Subgroup basic-cursor-vs-flip-varying-size: pass -> FAIL (fi-bsw-n3050) Subgroup basic-flip-vs-cursor-legacy: fail -> PASS (fi-hsw-4770k) Test kms_pipe_crc_basic: Subgroup nonblocking-crc-pipe-b: pass -> SKIP (fi-hsw-4770r) Subgroup nonblocking-crc-pipe-c: skip -> PASS (fi-hsw-4770r) Subgroup read-crc-pipe-c: pass -> SKIP (fi-hsw-4770r) fi-bdw-5557u total:252 pass:235 dwarn:0 dfail:0 fail:2 skip:15 fi-bsw-n3050 total:252 pass:202 dwarn:0 dfail:0 fail:4 skip:46 fi-hsw-4770k total:252 pass:222 dwarn:6 dfail:1 fail:1 skip:22 fi-hsw-4770r total:252 pass:222 dwarn:0 dfail:0 fail:2 skip:28 fi-ivb-3520m total:252 pass:220 dwarn:0 dfail:0 fail:1 skip:31 fi-skl-6260u total:252 pass:236 dwarn:0 dfail:0 fail:2 skip:14 fi-skl-6700k total:252 pass:216 dwarn:4 dfail:0 fail:4 skip:28 fi-snb-2520m total:252 pass:203 dwarn:4 dfail:0 fail:2 skip:43 fi-snb-2600 total:252 pass:203 dwarn:4 dfail:0 fail:2 skip:43 Results at /archive/results/CI_IGT_test/Patchwork_2422/ 6737eeadd55aa09ac998698461138309ab623dbb drm-intel-nightly: 2016y-08m-24d-15h-53m-53s UTC integration manifest a58a4c2 drm/i915: fix comment referencing imaginary functions _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: fix comment referencing imaginary functions 2016-08-24 16:03 [PATCH] drm/i915: fix comment referencing imaginary functions Matthew Auld 2016-08-24 16:50 ` ✗ Fi.CI.BAT: failure for " Patchwork @ 2016-10-21 12:16 ` Arkadiusz Hiler 2016-10-21 12:28 ` Chris Wilson 1 sibling, 1 reply; 9+ messages in thread From: Arkadiusz Hiler @ 2016-10-21 12:16 UTC (permalink / raw) To: Matthew Auld; +Cc: intel-gfx On Wed, Aug 24, 2016 at 05:03:11PM +0100, Matthew Auld wrote: > The comment which documents the proper usage of the *_FW family of macros makes > reference to intel_uncore_forcewake_irq{unlock, lock}, which is just > confusing, seeing as such a set of functions don't even exist and never have > for that matter(according to git). Let's fix that by replacing them with > intel_uncore_forcewake_{get, put}. > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Matthew Auld <matthew.auld@intel.com> Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index ff96b7a..c285d61 100644 > --- a/drivers/gpu/drm/i916/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -3792,8 +3792,8 @@ __raw_write(64, q) > * critical sections inside IRQ handlers where forcewake is explicitly > * controlled. > * Think twice, and think again, before using these. > - * Note: Should only be used between intel_uncore_forcewake_irqlock() and > - * intel_uncore_forcewake_irqunlock(). > + * Note: Should only be used between intel_uncore_forcewake_get and > + * intel_uncore_forcewake_put. > */ > #define I915_READ_FW(reg__) __raw_i915_read32(dev_priv, (reg__)) > #define I915_WRITE_FW(reg__, val__) __raw_i915_write32(dev_priv, (reg__), (val__)) > -- > 2.7.4 -- Cheers, Arek _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: fix comment referencing imaginary functions 2016-10-21 12:16 ` [PATCH] " Arkadiusz Hiler @ 2016-10-21 12:28 ` Chris Wilson 2016-10-21 13:00 ` Mika Kuoppala 0 siblings, 1 reply; 9+ messages in thread From: Chris Wilson @ 2016-10-21 12:28 UTC (permalink / raw) To: Arkadiusz Hiler; +Cc: intel-gfx, Matthew Auld On Fri, Oct 21, 2016 at 02:16:46PM +0200, Arkadiusz Hiler wrote: > On Wed, Aug 24, 2016 at 05:03:11PM +0100, Matthew Auld wrote: > > The comment which documents the proper usage of the *_FW family of macros makes > > reference to intel_uncore_forcewake_irq{unlock, lock}, which is just > > confusing, seeing as such a set of functions don't even exist and never have > > for that matter(according to git). Let's fix that by replacing them with > > intel_uncore_forcewake_{get, put}. > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> The downside is that this now doesn't mention the locking required to prevent machine hangs on some platforms. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: fix comment referencing imaginary functions 2016-10-21 12:28 ` Chris Wilson @ 2016-10-21 13:00 ` Mika Kuoppala 2016-10-21 13:57 ` Chris Wilson 0 siblings, 1 reply; 9+ messages in thread From: Mika Kuoppala @ 2016-10-21 13:00 UTC (permalink / raw) To: Chris Wilson, Arkadiusz Hiler; +Cc: intel-gfx, Matthew Auld Chris Wilson <chris@chris-wilson.co.uk> writes: > On Fri, Oct 21, 2016 at 02:16:46PM +0200, Arkadiusz Hiler wrote: >> On Wed, Aug 24, 2016 at 05:03:11PM +0100, Matthew Auld wrote: >> > The comment which documents the proper usage of the *_FW family of macros makes >> > reference to intel_uncore_forcewake_irq{unlock, lock}, which is just >> > confusing, seeing as such a set of functions don't even exist and never have >> > for that matter(according to git). Let's fix that by replacing them with >> > intel_uncore_forcewake_{get, put}. >> > >> > Cc: Chris Wilson <chris@chris-wilson.co.uk> >> > Signed-off-by: Matthew Auld <matthew.auld@intel.com> >> Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > > The downside is that this now doesn't mention the locking required to > prevent machine hangs on some platforms. "intel_uncore_forcewake_get will acquire forcewake reference and also take a uncore.lock to guarantee explicit access by one thread only. As some registers don't need forcewake held, intel_uncore_forcewake_{get,put} can be omitted. If you do so, be warned that on some gens (gen7), concurrent access to the same cacheline by multiple cpu threads with the gpu can risk a system hang. You need to grab uncore spinlock explicitly to guard against this." Would that be accurate addition? -Mika > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: fix comment referencing imaginary functions 2016-10-21 13:00 ` Mika Kuoppala @ 2016-10-21 13:57 ` Chris Wilson 2016-10-24 11:23 ` Arkadiusz Hiler 0 siblings, 1 reply; 9+ messages in thread From: Chris Wilson @ 2016-10-21 13:57 UTC (permalink / raw) To: Mika Kuoppala; +Cc: intel-gfx, Matthew Auld On Fri, Oct 21, 2016 at 04:00:10PM +0300, Mika Kuoppala wrote: > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > On Fri, Oct 21, 2016 at 02:16:46PM +0200, Arkadiusz Hiler wrote: > >> On Wed, Aug 24, 2016 at 05:03:11PM +0100, Matthew Auld wrote: > >> > The comment which documents the proper usage of the *_FW family of macros makes > >> > reference to intel_uncore_forcewake_irq{unlock, lock}, which is just > >> > confusing, seeing as such a set of functions don't even exist and never have > >> > for that matter(according to git). Let's fix that by replacing them with > >> > intel_uncore_forcewake_{get, put}. > >> > > >> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > >> > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > >> Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > > > > The downside is that this now doesn't mention the locking required to > > prevent machine hangs on some platforms. > > "intel_uncore_forcewake_get will acquire forcewake reference and also > take a uncore.lock to guarantee explicit access by one thread only. As > some registers don't need forcewake held, intel_uncore_forcewake_{get,put} > can be omitted. If you do so, be warned that on some gens (gen7), > concurrent access to the same cacheline by multiple cpu threads with the gpu > can risk a system hang. You need to grab uncore spinlock explicitly to > guard against this." > > Would that be accurate addition? intel_uncore_forcewake_get() doesn't acquire the spinlock for you, just for itself. The full sequence would be spin_lock_irq(&dev_priv->uncore.lock); intel_uncore_forcewake_get__locked() ... intel_uncore_forcewake_put__locked() spin_unlock_irq(&dev_priv->uncore.lock); We very rarely do that either (a) presuming that we are serialised by some other lock, (b) don't care because it is safe or (c) completely forgotten about the risks. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: fix comment referencing imaginary functions 2016-10-21 13:57 ` Chris Wilson @ 2016-10-24 11:23 ` Arkadiusz Hiler 2016-10-25 11:29 ` Matthew Auld 0 siblings, 1 reply; 9+ messages in thread From: Arkadiusz Hiler @ 2016-10-24 11:23 UTC (permalink / raw) To: Chris Wilson, Mika Kuoppala, intel-gfx, Matthew Auld On Fri, Oct 21, 2016 at 02:57:28PM +0100, Chris Wilson wrote: > On Fri, Oct 21, 2016 at 04:00:10PM +0300, Mika Kuoppala wrote: > > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > > > On Fri, Oct 21, 2016 at 02:16:46PM +0200, Arkadiusz Hiler wrote: > > >> On Wed, Aug 24, 2016 at 05:03:11PM +0100, Matthew Auld wrote: > > >> > The comment which documents the proper usage of the *_FW family of macros makes > > >> > reference to intel_uncore_forcewake_irq{unlock, lock}, which is just > > >> > confusing, seeing as such a set of functions don't even exist and never have > > >> > for that matter(according to git). Let's fix that by replacing them with > > >> > intel_uncore_forcewake_{get, put}. > > >> > > > >> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > >> > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > > >> Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > > > > > > The downside is that this now doesn't mention the locking required to > > > prevent machine hangs on some platforms. Previous version neither mentioned that clearly. Imaginary functions with irq in name is more confusing than helpful in my opinion. The assumption that those were mistaken for {get,put} is easy enough to make. > > > > "intel_uncore_forcewake_get will acquire forcewake reference and also > > take a uncore.lock to guarantee explicit access by one thread only. As > > some registers don't need forcewake held, intel_uncore_forcewake_{get,put} > > can be omitted. If you do so, be warned that on some gens (gen7), > > concurrent access to the same cacheline by multiple cpu threads with the gpu > > can risk a system hang. You need to grab uncore spinlock explicitly to > > guard against this." > > > > Would that be accurate addition? > > intel_uncore_forcewake_get() doesn't acquire the spinlock for you, just > for itself. > > The full sequence would be > > spin_lock_irq(&dev_priv->uncore.lock); > intel_uncore_forcewake_get() > ... > intel_uncore_forcewake_put() > spin_unlock_irq(&dev_priv->uncore.lock); > > We very rarely do that either (a) presuming that we are serialised by > some other lock, (b) don't care because it is safe or (c) completely > forgotten about the risks. > -Chris Then all that should be mentioned? My take on it: These are untraced mmio-accessors that are only valid to be used inside critical sections inside IRQ handlers where forcewake is explicitly controlled. Think twice, and think again, before using these. Those possibly should be used between: spin_lock_irq(&dev_priv->uncore.lock); intel_uncore_forcewake_get(); and intel_uncore_forcewake_put(); spin_unlock_irq(&dev_priv->uncore.lock); Note: some registers may not need forcewake held, so intel_uncore_forcewake_{get,put} can be omitted. Code may be serialised by different lock, so immediate spin_{lock,unlock}_irq() may not be necessary. -- Cheers, Arek _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: fix comment referencing imaginary functions 2016-10-24 11:23 ` Arkadiusz Hiler @ 2016-10-25 11:29 ` Matthew Auld 2016-10-25 11:39 ` Chris Wilson 0 siblings, 1 reply; 9+ messages in thread From: Matthew Auld @ 2016-10-25 11:29 UTC (permalink / raw) To: Arkadiusz Hiler; +Cc: Intel Graphics Development, Matthew Auld > These are untraced mmio-accessors that are only valid to be used inside > critical sections inside IRQ handlers where forcewake is explicitly > controlled. > > Think twice, and think again, before using these. > > Those possibly should be used between: > > spin_lock_irq(&dev_priv->uncore.lock); > intel_uncore_forcewake_get(); > > and > > intel_uncore_forcewake_put(); > spin_unlock_irq(&dev_priv->uncore.lock); > > > Note: some registers may not need forcewake held, so > intel_uncore_forcewake_{get,put} can be omitted. > > Code may be serialised by different lock, so immediate > spin_{lock,unlock}_irq() may not be necessary. Maybe roll that up into a new patch? Assuming Chris is happy... _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: fix comment referencing imaginary functions 2016-10-25 11:29 ` Matthew Auld @ 2016-10-25 11:39 ` Chris Wilson 0 siblings, 0 replies; 9+ messages in thread From: Chris Wilson @ 2016-10-25 11:39 UTC (permalink / raw) To: Matthew Auld; +Cc: Intel Graphics Development, Matthew Auld On Tue, Oct 25, 2016 at 12:29:41PM +0100, Matthew Auld wrote: > > These are untraced mmio-accessors that are only valid to be used inside > > critical sections inside IRQ handlers where forcewake is explicitly > > controlled. > > > > Think twice, and think again, before using these. > > > > Those possibly should be used between: > > > > spin_lock_irq(&dev_priv->uncore.lock); > > intel_uncore_forcewake_get(); > > > > and > > > > intel_uncore_forcewake_put(); > > spin_unlock_irq(&dev_priv->uncore.lock); > > > > > > Note: some registers may not need forcewake held, so > > intel_uncore_forcewake_{get,put} can be omitted. > > > > Code may be serialised by different lock, so immediate > > spin_{lock,unlock}_irq() may not be necessary. > Maybe roll that up into a new patch? Assuming Chris is happy... > s/inside IRQ handlers/, such as inside IRQ handlers,/ As an example, these accessors can possibly be used between: can be omitted, see intel_uncore_forcewake_for_reg(). Certain architectures will die if the same cacheline is concurrently accessed by different clients (e.g. Ivybridge). Access to registers should therefore generally be serialised, by either the dev_priv->uncore.lock or a more localised lock guarding all access to that bank of registers. -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-10-25 11:39 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-08-24 16:03 [PATCH] drm/i915: fix comment referencing imaginary functions Matthew Auld 2016-08-24 16:50 ` ✗ Fi.CI.BAT: failure for " Patchwork 2016-10-21 12:16 ` [PATCH] " Arkadiusz Hiler 2016-10-21 12:28 ` Chris Wilson 2016-10-21 13:00 ` Mika Kuoppala 2016-10-21 13:57 ` Chris Wilson 2016-10-24 11:23 ` Arkadiusz Hiler 2016-10-25 11:29 ` Matthew Auld 2016-10-25 11:39 ` Chris Wilson
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.