[PATCH 1/3] drm/i915/pmu: avoid an maybe-uninitialized warning

[PATCH 1/3] drm/i915/pmu: avoid an maybe-uninitialized warning

[Arnd Bergmann]

Conditional spinlocks make it hard for gcc and for lockdep to
follow the code flow. This one causes a warning with at least
gcc-9 and higher:

In file included from include/linux/irq.h:14,
                 from drivers/gpu/drm/i915/i915_pmu.c:7:
drivers/gpu/drm/i915/i915_pmu.c: In function 'i915_sample':
include/linux/spinlock.h:289:3: error: 'flags' may be used uninitialized in this function [-Werror=maybe-uninitialized]
  289 |   _raw_spin_unlock_irqrestore(lock, flags); \
      |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/gpu/drm/i915/i915_pmu.c:288:17: note: 'flags' was declared here
  288 |   unsigned long flags;
      |                 ^~~~~

Split out the part between the locks into a separate function
for readability and to let the compiler figure out what the
logic actually is.

Fixes: d79e1bd676f0 ("drm/i915/pmu: Only use exclusive mmio access for gen7")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
I have no idea why I see three separate issues like this pop up in i915,
there are not a lot of them elsewhere.

 drivers/gpu/drm/i915/i915_pmu.c | 84 ++++++++++++++++-----------------
 1 file changed, 42 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index e991a707bdb7..962ded9ce73f 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -269,12 +269,48 @@ static bool exclusive_mmio_access(const struct drm_i915_private *i915)
 	return IS_GEN(i915, 7);
 }
 
+static void engine_sample(struct intel_engine_cs *engine, unsigned int period_ns)
+{
+	struct intel_engine_pmu *pmu = &engine->pmu;
+	bool busy;
+	u32 val;
+
+	val = ENGINE_READ_FW(engine, RING_CTL);
+	if (val == 0) /* powerwell off => engine idle */
+		return;
+
+	if (val & RING_WAIT)
+		add_sample(&pmu->sample[I915_SAMPLE_WAIT], period_ns);
+	if (val & RING_WAIT_SEMAPHORE)
+		add_sample(&pmu->sample[I915_SAMPLE_SEMA], period_ns);
+
+	/* No need to sample when busy stats are supported. */
+	if (intel_engine_supports_stats(engine))
+		return;
+
+	/*
+	 * While waiting on a semaphore or event, MI_MODE reports the
+	 * ring as idle. However, previously using the seqno, and with
+	 * execlists sampling, we account for the ring waiting as the
+	 * engine being busy. Therefore, we record the sample as being
+	 * busy if either waiting or !idle.
+	 */
+	busy = val & (RING_WAIT_SEMAPHORE | RING_WAIT);
+	if (!busy) {
+		val = ENGINE_READ_FW(engine, RING_MI_MODE);
+		busy = !(val & MODE_IDLE);
+	}
+	if (busy)
+		add_sample(&pmu->sample[I915_SAMPLE_BUSY], period_ns);
+}
+
 static void
 engines_sample(struct intel_gt *gt, unsigned int period_ns)
 {
 	struct drm_i915_private *i915 = gt->i915;
 	struct intel_engine_cs *engine;
 	enum intel_engine_id id;
+	unsigned long flags;
 
 	if ((i915->pmu.enable & ENGINE_SAMPLE_MASK) == 0)
 		return;
@@ -283,53 +319,17 @@ engines_sample(struct intel_gt *gt, unsigned int period_ns)
 		return;
 
 	for_each_engine(engine, gt, id) {
-		struct intel_engine_pmu *pmu = &engine->pmu;
-		spinlock_t *mmio_lock;
-		unsigned long flags;
-		bool busy;
-		u32 val;
-
 		if (!intel_engine_pm_get_if_awake(engine))
 			continue;
 
-		mmio_lock = NULL;
-		if (exclusive_mmio_access(i915))
-			mmio_lock = &engine->uncore->lock;
-
-		if (unlikely(mmio_lock))
-			spin_lock_irqsave(mmio_lock, flags);
-
-		val = ENGINE_READ_FW(engine, RING_CTL);
-		if (val == 0) /* powerwell off => engine idle */
-			goto skip;
-
-		if (val & RING_WAIT)
-			add_sample(&pmu->sample[I915_SAMPLE_WAIT], period_ns);
-		if (val & RING_WAIT_SEMAPHORE)
-			add_sample(&pmu->sample[I915_SAMPLE_SEMA], period_ns);
-
-		/* No need to sample when busy stats are supported. */
-		if (intel_engine_supports_stats(engine))
-			goto skip;
-
-		/*
-		 * While waiting on a semaphore or event, MI_MODE reports the
-		 * ring as idle. However, previously using the seqno, and with
-		 * execlists sampling, we account for the ring waiting as the
-		 * engine being busy. Therefore, we record the sample as being
-		 * busy if either waiting or !idle.
-		 */
-		busy = val & (RING_WAIT_SEMAPHORE | RING_WAIT);
-		if (!busy) {
-			val = ENGINE_READ_FW(engine, RING_MI_MODE);
-			busy = !(val & MODE_IDLE);
+		if (exclusive_mmio_access(i915)) {
+			spin_lock_irqsave(&engine->uncore->lock, flags);
+			engine_sample(engine, period_ns);
+			spin_unlock_irqrestore(&engine->uncore->lock, flags);
+		} else {
+			engine_sample(engine, period_ns);
 		}
-		if (busy)
-			add_sample(&pmu->sample[I915_SAMPLE_BUSY], period_ns);
 
-skip:
-		if (unlikely(mmio_lock))
-			spin_unlock_irqrestore(mmio_lock, flags);
 		intel_engine_pm_put_async(engine);
 	}
 }
-- 
2.26.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm/i915: work around false-positive maybe-uninitialized warning

[Arnd Bergmann]

gcc-9 gets confused by the code flow in check_dirty_whitelist:

drivers/gpu/drm/i915/gt/selftest_workarounds.c: In function 'check_dirty_whitelist':
drivers/gpu/drm/i915/gt/selftest_workarounds.c:492:17: error: 'rsvd' may be used uninitialized in this function [-Werror=maybe-uninitialized]

I could not figure out a good way to do this in a way that gcc
understands better, so initialize the variable to zero, as last
resort.

Fixes: aee20aaed887 ("drm/i915: Implement read-only support in whitelist selftest")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/i915/gt/selftest_workarounds.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
index 5ed323254ee1..32785463ec9e 100644
--- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
+++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
@@ -623,6 +623,8 @@ static int check_dirty_whitelist(struct intel_context *ce)
 				err = -EINVAL;
 				goto out_unpin;
 			}
+		} else {
+			rsvd = 0;
 		}
 
 		expect = results[0];
-- 
2.26.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/i915/selftests: avoid bogus maybe-uninitialized warning

[Arnd Bergmann]

gcc has a problem following values through IS_ERR/PTR_ERR macros,
leading to a false-positive warning in allmodconfig, with any
compiler version:

In file included from drivers/gpu/drm/i915/gt/intel_lrc.c:5892:
drivers/gpu/drm/i915/gt/selftest_lrc.c: In function 'create_gpr_client.isra.0':
drivers/gpu/drm/i915/gt/selftest_lrc.c:2902:23: error: 'rq' may be used uninitialized in this function [-Werror=maybe-uninitialized]

This one is hard to avoid without impairing readability or adding a
bugus NULL pointer.

Fixes: c92724de6db1 ("drm/i915/selftests: Try to detect rollback during batchbuffer preemption")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/gpu/drm/i915/gt/selftest_lrc.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c
index 824f99c4cc7c..933c3f5adf0a 100644
--- a/drivers/gpu/drm/i915/gt/selftest_lrc.c
+++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c
@@ -2908,23 +2908,25 @@ create_gpr_client(struct intel_engine_cs *engine,
 
 	vma = i915_vma_instance(global->obj, ce->vm, NULL);
 	if (IS_ERR(vma)) {
-		err = PTR_ERR(vma);
+		rq = ERR_CAST(vma);
 		goto out_ce;
 	}
 
 	err = i915_vma_pin(vma, 0, 0, PIN_USER);
-	if (err)
+	if (err) {
+		rq = ERR_PTR(err);
 		goto out_ce;
+	}
 
 	batch = create_gpr_user(engine, vma, offset);
 	if (IS_ERR(batch)) {
-		err = PTR_ERR(batch);
+		rq = ERR_CAST(batch);
 		goto out_vma;
 	}
 
 	rq = intel_context_create_request(ce);
 	if (IS_ERR(rq)) {
-		err = PTR_ERR(rq);
+		rq = ERR_CAST(rq);
 		goto out_batch;
 	}
 
@@ -2946,17 +2948,20 @@ create_gpr_client(struct intel_engine_cs *engine,
 	i915_vma_unlock(batch);
 	i915_vma_unpin(batch);
 
-	if (!err)
+	if (!err) {
 		i915_request_get(rq);
-	i915_request_add(rq);
-
+		i915_request_add(rq);
+	} else {
+		i915_request_add(rq);
+		rq = ERR_PTR(err);
+	}
 out_batch:
 	i915_vma_put(batch);
 out_vma:
 	i915_vma_unpin(vma);
 out_ce:
 	intel_context_put(ce);
-	return err ? ERR_PTR(err) : rq;
+	return rq;
 }
 
 static int preempt_user(struct intel_engine_cs *engine,
-- 
2.26.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/i915/pmu: avoid an maybe-uninitialized warning

[Chris Wilson]

Quoting Arnd Bergmann (2020-05-27 15:05:08)
> Conditional spinlocks make it hard for gcc and for lockdep to
> follow the code flow. This one causes a warning with at least
> gcc-9 and higher:
> 
> In file included from include/linux/irq.h:14,
>                  from drivers/gpu/drm/i915/i915_pmu.c:7:
> drivers/gpu/drm/i915/i915_pmu.c: In function 'i915_sample':
> include/linux/spinlock.h:289:3: error: 'flags' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>   289 |   _raw_spin_unlock_irqrestore(lock, flags); \
>       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/gpu/drm/i915/i915_pmu.c:288:17: note: 'flags' was declared here
>   288 |   unsigned long flags;
>       |                 ^~~~~
> 
> Split out the part between the locks into a separate function
> for readability and to let the compiler figure out what the
> logic actually is.
> 
> Fixes: d79e1bd676f0 ("drm/i915/pmu: Only use exclusive mmio access for gen7")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> I have no idea why I see three separate issues like this pop up in i915,
> there are not a lot of them elsewhere.

gcc v8:
add/remove: 1/0 grow/shrink: 0/1 up/down: 99/-164 (-65)
Function                                     old     new   delta
engine_sample                                  -      99     +99
i915_sample                                  871     707    -164

Which is compelling. Is gcc 9 simliar?

Given the above reduction, I find it hard to argue with.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/i915: work around false-positive maybe-uninitialized warning

[Chris Wilson]

Quoting Arnd Bergmann (2020-05-27 15:05:09)
> gcc-9 gets confused by the code flow in check_dirty_whitelist:
> 
> drivers/gpu/drm/i915/gt/selftest_workarounds.c: In function 'check_dirty_whitelist':
> drivers/gpu/drm/i915/gt/selftest_workarounds.c:492:17: error: 'rsvd' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> 
> I could not figure out a good way to do this in a way that gcc
> understands better, so initialize the variable to zero, as last
> resort.

Does it look neater if we initialise it as a local? No.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/i915/selftests: avoid bogus maybe-uninitialized warning

[Chris Wilson]

Quoting Arnd Bergmann (2020-05-27 15:05:10)
> gcc has a problem following values through IS_ERR/PTR_ERR macros,
> leading to a false-positive warning in allmodconfig, with any
> compiler version:
> 
> In file included from drivers/gpu/drm/i915/gt/intel_lrc.c:5892:
> drivers/gpu/drm/i915/gt/selftest_lrc.c: In function 'create_gpr_client.isra.0':
> drivers/gpu/drm/i915/gt/selftest_lrc.c:2902:23: error: 'rq' may be used uninitialized in this function [-Werror=maybe-uninitialized]
> 
> This one is hard to avoid without impairing readability or adding a
> bugus NULL pointer.
> 
> Fixes: c92724de6db1 ("drm/i915/selftests: Try to detect rollback during batchbuffer preemption")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/gpu/drm/i915/gt/selftest_lrc.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c
> index 824f99c4cc7c..933c3f5adf0a 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c
> @@ -2908,23 +2908,25 @@ create_gpr_client(struct intel_engine_cs *engine,
>  
>         vma = i915_vma_instance(global->obj, ce->vm, NULL);
>         if (IS_ERR(vma)) {
> -               err = PTR_ERR(vma);
> +               rq = ERR_CAST(vma);
>                 goto out_ce;
>         }
>  
>         err = i915_vma_pin(vma, 0, 0, PIN_USER);
> -       if (err)
> +       if (err) {
> +               rq = ERR_PTR(err);
>                 goto out_ce;
> +       }
>  
>         batch = create_gpr_user(engine, vma, offset);
>         if (IS_ERR(batch)) {
> -               err = PTR_ERR(batch);
> +               rq = ERR_CAST(batch);
>                 goto out_vma;
>         }
>  
>         rq = intel_context_create_request(ce);
>         if (IS_ERR(rq)) {
> -               err = PTR_ERR(rq);
> +               rq = ERR_CAST(rq);
>                 goto out_batch;
>         }
>  
> @@ -2946,17 +2948,20 @@ create_gpr_client(struct intel_engine_cs *engine,
>         i915_vma_unlock(batch);
>         i915_vma_unpin(batch);
>  
> -       if (!err)
> +       if (!err) {
>                 i915_request_get(rq);
> -       i915_request_add(rq);
> -
> +               i915_request_add(rq);
> +       } else {
> +               i915_request_add(rq);
> +               rq = ERR_PTR(err);
> +       }
>  out_batch:
>         i915_vma_put(batch);
>  out_vma:
>         i915_vma_unpin(vma);
>  out_ce:
>         intel_context_put(ce);
> -       return err ? ERR_PTR(err) : rq;
> +       return rq;

Hmm, I've used this style a few times, so could do with some syntactic
refinement.

drivers/gpu/drm/i915/gem/i915_gem_userptr.c:    return err ? ERR_PTR(err) : mm->mn;
drivers/gpu/drm/i915/gt/selftest_hangcheck.c:   return err ? ERR_PTR(err) : rq;
drivers/gpu/drm/i915/gt/selftest_lrc.c: return err ? ERR_PTR(err) : rq;
drivers/gpu/drm/i915/selftests/i915_gem_gtt.c:  return err ? ERR_PTR(err) : rq;
drivers/gpu/drm/i915/selftests/i915_request.c:  return err ? ERR_PTR(err) : request;
drivers/gpu/drm/i915/selftests/igt_spinner.c:   return err ? ERR_PTR(err) : rq;
-Chris

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel