Re: [PATCH 25/46] drm/i915: Store the BIT(engine->id) as the engine's mask

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 06/02/2019 13:03, Chris Wilson wrote:
> In the next patch, we are introducing a broad virtual engine to encompass
> multiple physical engines, losing the 1:1 nature of BIT(engine->id). To
> reflect the broader set of engines implied by the virtual instance, lets
> store the full bitmask.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_reset.c                | 4 ++--
>   drivers/gpu/drm/i915/intel_engine_cs.c           | 3 +++
>   drivers/gpu/drm/i915/intel_hangcheck.c           | 8 ++++----
>   drivers/gpu/drm/i915/intel_ringbuffer.c          | 4 ++--
>   drivers/gpu/drm/i915/intel_ringbuffer.h          | 7 +------
>   drivers/gpu/drm/i915/selftests/intel_hangcheck.c | 2 +-
>   drivers/gpu/drm/i915/selftests/mock_engine.c     | 1 +
>   7 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reset.c b/drivers/gpu/drm/i915/i915_reset.c
> index 7051c0a43941..78c9689629a0 100644
> --- a/drivers/gpu/drm/i915/i915_reset.c
> +++ b/drivers/gpu/drm/i915/i915_reset.c
> @@ -1053,7 +1053,7 @@ void i915_reset(struct drm_i915_private *i915,
>   static inline int intel_gt_reset_engine(struct drm_i915_private *i915,
>   					struct intel_engine_cs *engine)
>   {
> -	return intel_gpu_reset(i915, intel_engine_flag(engine));
> +	return intel_gpu_reset(i915, engine->mask);
>   }
>   
>   /**
> @@ -1253,7 +1253,7 @@ void i915_handle_error(struct drm_i915_private *i915,
>   				continue;
>   
>   			if (i915_reset_engine(engine, msg) == 0)
> -				engine_mask &= ~intel_engine_flag(engine);
> +				engine_mask &= ~engine->mask;
>   
>   			clear_bit(I915_RESET_ENGINE + engine->id,
>   				  &error->flags);
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index ce7c19f2ae49..45e38877ab17 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -313,7 +313,10 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
>   	if (!engine)
>   		return -ENOMEM;
>   
> +	BUILD_BUG_ON(BITS_PER_TYPE(engine->mask) < I915_NUM_ENGINES);
> +
>   	engine->id = id;
> +	engine->mask = BIT(id);
>   	engine->i915 = dev_priv;
>   	__sprint_engine_name(engine->name, info);
>   	engine->hw_id = engine->guc_id = info->hw_id;
> diff --git a/drivers/gpu/drm/i915/intel_hangcheck.c b/drivers/gpu/drm/i915/intel_hangcheck.c
> index e04b2560369e..58b6ff8453dc 100644
> --- a/drivers/gpu/drm/i915/intel_hangcheck.c
> +++ b/drivers/gpu/drm/i915/intel_hangcheck.c
> @@ -120,7 +120,7 @@ engine_stuck(struct intel_engine_cs *engine, u64 acthd)
>   	 */
>   	tmp = I915_READ_CTL(engine);
>   	if (tmp & RING_WAIT) {
> -		i915_handle_error(dev_priv, BIT(engine->id), 0,
> +		i915_handle_error(dev_priv, engine->mask, 0,
>   				  "stuck wait on %s", engine->name);
>   		I915_WRITE_CTL(engine, tmp);
>   		return ENGINE_WAIT_KICK;
> @@ -282,13 +282,13 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
>   		hangcheck_store_sample(engine, &hc);
>   
>   		if (hc.stalled) {
> -			hung |= intel_engine_flag(engine);
> +			hung |= engine->mask;
>   			if (hc.action != ENGINE_DEAD)
> -				stuck |= intel_engine_flag(engine);
> +				stuck |= engine->mask;
>   		}
>   
>   		if (hc.wedged)
> -			wedged |= intel_engine_flag(engine);
> +			wedged |= engine->mask;
>   	}
>   
>   	if (GEM_SHOW_DEBUG() && (hung | stuck)) {
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 1b96b0960adc..91c49f644898 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1859,8 +1859,8 @@ static int switch_context(struct i915_request *rq)
>   				goto err;
>   		} while (--loops);
>   
> -		if (intel_engine_flag(engine) & ppgtt->pd_dirty_rings) {
> -			unwind_mm = intel_engine_flag(engine);
> +		if (ppgtt->pd_dirty_rings & engine->mask) {
> +			unwind_mm = engine->mask;
>   			ppgtt->pd_dirty_rings &= ~unwind_mm;
>   			hw_flags = MI_FORCE_RESTORE;
>   		}
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 39a9ee7b61e2..7777d46784f9 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -334,6 +334,7 @@ struct intel_engine_cs {
>   	enum intel_engine_id id;
>   	unsigned int hw_id;
>   	unsigned int guc_id;
> +	unsigned long mask;

Could use intel_ring_mask_t - if we renamed it to intel_engine_mask_t - 
which is already checked with a BUILD_BUG_ON.

>   
>   	u8 uabi_id;
>   	u8 uabi_class;
> @@ -668,12 +669,6 @@ execlists_port_complete(struct intel_engine_execlists * const execlists,
>   	return port;
>   }
>   
> -static inline unsigned int
> -intel_engine_flag(const struct intel_engine_cs *engine)
> -{
> -	return BIT(engine->id);
> -}
> -
>   static inline u32
>   intel_read_status_page(const struct intel_engine_cs *engine, int reg)
>   {
> diff --git a/drivers/gpu/drm/i915/selftests/intel_hangcheck.c b/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
> index 4aa57d0d1b92..50a7f57a00a4 100644
> --- a/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
> +++ b/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
> @@ -1142,7 +1142,7 @@ static int __igt_reset_evict_vma(struct drm_i915_private *i915,
>   
>   out_reset:
>   	igt_global_reset_lock(i915);
> -	fake_hangcheck(rq->i915, intel_engine_flag(rq->engine));
> +	fake_hangcheck(rq->i915, rq->engine->mask);
>   	igt_global_reset_unlock(i915);
>   
>   	if (tsk) {
> diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c b/drivers/gpu/drm/i915/selftests/mock_engine.c
> index ec1ae948954c..c2c954f64226 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_engine.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_engine.c
> @@ -223,6 +223,7 @@ struct intel_engine_cs *mock_engine(struct drm_i915_private *i915,
>   	engine->base.i915 = i915;
>   	snprintf(engine->base.name, sizeof(engine->base.name), "%s", name);
>   	engine->base.id = id;
> +	engine->base.mask = BIT(id);
>   	engine->base.status_page.addr = (void *)(engine + 1);
>   
>   	engine->base.context_pin = mock_context_pin;
> 

No other suggestions.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx