Re: [PATCH 2/6] drm/i915: Allow passing any known pointer to for_each_engine()

From: Dave Gordon <david.s.gordon@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 2/6] drm/i915: Allow passing any known pointer to for_each_engine()
Date: Mon, 21 Mar 2016 15:44:27 +0000	[thread overview]
Message-ID: <56F016DB.8010907@intel.com> (raw)
In-Reply-To: <1458335784-1773-2-git-send-email-chris@chris-wilson.co.uk>

On 18/03/16 21:16, Chris Wilson wrote:
> Rather than require the user to grab a drm_i915_private, allow them to
> pass anything that we know how to derive such a pointer user to_i915()
>
> Note this fixes a macro bug in for_each_engine_masked() which was not
> using its dev_priv__ parameter.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h         | 8 ++++----
>   drivers/gpu/drm/i915/i915_gem_context.c | 4 ++--
>   drivers/gpu/drm/i915/intel_mocs.c       | 3 +--
>   3 files changed, 7 insertions(+), 8 deletions(-)

Hmm .. generally I'm quite keen on enhancing to_i915(), but I'm not sure 
about this iterator. The thing is, that the array of engines are 
associated with the device (or dev_priv/i915) as a whole, and not with 
an object (etc). In particular, some objects can have associations with 
one or more specific engines, either permanently (e.g. the kernel 
context subobjects) or transiently (objects being used by workloads). It 
therefore seems counterintuitive to use such an object as the basis for 
iterating over all (initialised) engines; I might just as reasonably 
expect the macro to iterate over all (but only) the engines associated 
with the object (whatever that might mean in a particular case).

So I think for_each_engine() should continue to require the caller to 
provide a (struct drm_i915_private *) so that it's clear that we're 
operating on the whole device. But the partially simplification of 
allowing to_i915(engine) rather than to_i915(engine->dev) is fine. And 
of course we still want the bug fix!

> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8606e2c7db04..0c9fe00d3e83 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1988,12 +1988,12 @@ static inline struct drm_i915_private *guc_to_i915(struct intel_guc *guc)
>   }
>
>   /* Iterate over initialised rings */
> -#define for_each_engine(ring__, dev_priv__, i__) \
> +#define for_each_engine(ring__, ptr__, i__) \
>   	for ((i__) = 0; (i__) < I915_NUM_ENGINES; (i__)++) \
> -		for_each_if ((((ring__) = &(dev_priv__)->engine[(i__)]), intel_engine_initialized((ring__))))
> +		for_each_if ((((ring__) = &to_i915(ptr__)->engine[(i__)]), intel_engine_initialized((ring__))))
>
> -#define for_each_engine_masked(engine__, dev_priv__, mask__) \
> -	for ((engine__) = &dev_priv->engine[0]; (engine__) < &dev_priv->engine[I915_NUM_ENGINES]; (engine__)++) \
> +#define for_each_engine_masked(engine__, ptr__, mask__) \
> +	for ((engine__) = &to_i915(ptr__)->engine[0]; (engine__) < &to_i915(ptr__)->engine[I915_NUM_ENGINES]; (engine__)++) \
>   		for_each_if (intel_engine_flag((engine__)) & (mask__) && intel_engine_initialized((engine__)))
>
>   enum hdmi_force_audio {
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 394e525e55f1..a8afd0cee7f7 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -553,7 +553,7 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
>
>   			intel_ring_emit(engine,
>   					MI_LOAD_REGISTER_IMM(num_rings));
> -			for_each_engine(signaller, to_i915(engine->dev), i) {
> +			for_each_engine(signaller, engine->dev, i) {

for_each_engine(signaller, to_i915(engine), i) ?

>   				if (signaller == engine)
>   					continue;
>
> @@ -582,7 +582,7 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
>
>   			intel_ring_emit(engine,
>   					MI_LOAD_REGISTER_IMM(num_rings));
> -			for_each_engine(signaller, to_i915(engine->dev), i) {
> +			for_each_engine(signaller, engine->dev, i) {

Also for_each_engine(signaller, to_i915(engine), i)

>   				if (signaller == engine)
>   					continue;
>
> diff --git a/drivers/gpu/drm/i915/intel_mocs.c b/drivers/gpu/drm/i915/intel_mocs.c
> index 3c725dde16ed..45200b93e9bb 100644
> --- a/drivers/gpu/drm/i915/intel_mocs.c
> +++ b/drivers/gpu/drm/i915/intel_mocs.c
> @@ -323,12 +323,11 @@ int intel_rcs_context_init_mocs(struct drm_i915_gem_request *req)
>   	int ret;
>
>   	if (get_mocs_settings(req->engine->dev, &t)) {
> -		struct drm_i915_private *dev_priv = req->i915;
>   		struct intel_engine_cs *engine;
>   		enum intel_engine_id ring_id;
>
>   		/* Program the control registers */
> -		for_each_engine(engine, dev_priv, ring_id) {
> +		for_each_engine(engine, req->i915, ring_id) {

I'm quite happy with this one :)

.Dave.

>   			ret = emit_mocs_control_table(req, &t, ring_id);
>   			if (ret)
>   				return ret;
>

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