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

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

On 17/08/16 10:41, Chris Wilson wrote:
> On Wed, Aug 17, 2016 at 10:34:18AM +0100, Tvrtko Ursulin wrote:
>> 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.
>
> We have the ring_mask which already tells us that mapping, so I think
> the second array is overkill.

Yes, I said "in addition to the existing map". In addition we could have 
an array of only initialized engines to avoid any skipping at runtime. 
Since iterators end with intel_engine_initialized anyway.

>> 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. :)
>
> gen6_semaphore_signal is called uncomfortably often, I'd be tempted to
> keep it trim. We should also contemplate delayed semaphore signaling
> again.

The list/array idea would solve that AFAICT.

Regards,

Tvrtko

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