[PATCH] drm/i915: fix comment referencing imaginary functions

[PATCH] drm/i915: fix comment referencing imaginary functions

[Matthew Auld]

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

✗ Fi.CI.BAT: failure for drm/i915: fix comment referencing imaginary functions

[Patchwork]

== 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

Re: [PATCH] drm/i915: fix comment referencing imaginary functions

[Arkadiusz Hiler]

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

Re: [PATCH] drm/i915: fix comment referencing imaginary functions

[Chris Wilson]

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

Re: [PATCH] drm/i915: fix comment referencing imaginary functions

[Mika Kuoppala]

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

Re: [PATCH] drm/i915: fix comment referencing imaginary functions

[Chris Wilson]

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

Re: [PATCH] drm/i915: fix comment referencing imaginary functions

[Arkadiusz Hiler]

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

Re: [PATCH] drm/i915: fix comment referencing imaginary functions

[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

Re: [PATCH] drm/i915: fix comment referencing imaginary functions

[Chris Wilson]

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