Re: [CI 2/2] drm/i915: Initialize legacy semaphores from engine hw id indexed array

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

On 16/08/16 17:18, Chris Wilson wrote:
> On Tue, Aug 16, 2016 at 05:04:21PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Build the legacy semaphore initialisation array using the engine
>> hardware ids instead of driver internal ones. This makes the
>> static array size dependent only on the number of gen6 semaphore
>> engines.
>>
>> Also makes the per-engine semaphore wait and signal tables
>> hardware id indexed saving some more space.
>>
>> v2: Refactor I915_GEN6_NUM_ENGINES to GEN6_SEMAPHORE_LAST. (Chris Wilson)
>> v3: More polish. (Chris Wilson)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>   drivers/gpu/drm/i915/intel_ringbuffer.c | 55 +++++++++++++++++----------------
>>   drivers/gpu/drm/i915/intel_ringbuffer.h |  7 +++--
>>   2 files changed, 34 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> index fa22bd87bab0..12703ea27259 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> @@ -1337,8 +1337,7 @@ static int gen6_signal(struct drm_i915_gem_request *req)
>>   {
>>   	struct intel_ring *ring = req->ring;
>>   	struct drm_i915_private *dev_priv = req->i915;
>> -	struct intel_engine_cs *useless;
>> -	enum intel_engine_id id;
>> +	struct intel_engine_cs *engine;
>>   	int ret, num_rings;
>>
>>   	num_rings = INTEL_INFO(dev_priv)->num_rings;
>> @@ -1346,9 +1345,13 @@ static int gen6_signal(struct drm_i915_gem_request *req)
>>   	if (ret)
>>   		return ret;
>>
>> -	for_each_engine_id(useless, dev_priv, id) {
>> -		i915_reg_t mbox_reg = req->engine->semaphore.mbox.signal[id];
>> +	for_each_engine(engine, dev_priv) {
>> +		i915_reg_t mbox_reg;
>> +
>> +		if (!(BIT(engine->hw_id) & GEN6_SEMAPHORES_MASK))
>> +			continue;
>
> Later on, it would be nice if this used
> 	for_each_engine_masked()
> instead (presupposing we have an efficient iterator for a sparse mask).
> The issue is in defining GEN6_SEMAPHORES_MASK
>
> #define GEN6_SEMAPHORES_MASK \
> 	(RENDER_RING |
> 	 BSD_RING |
> 	 BSD2_RING |
> 	 BLT_RING |
> 	 VECS_RING)
>
> Defnitely pencil that in for when otherwise we would iterate over 10
> empty slots in dev_priv->engines[].
>
> I'm thinking that we should make for_each_engine_mask() the default, say
> 	for_each_engine(engine, dev_priv, ALL_ENGINES, id)

Or add an initialized engine array to dev_priv, in addition to the 
existing map for best of both worlds.

That would prevent churn in for_each_engine users.

Or just give up and add something smarter, like a typdef 
intel_engine_it, to each of them and be done with churn once for all.

But we said there aren't any iterators on fast paths anyway so I would 
rather we chose to do nothing. :)

Regards,

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