Re: [PATCH 6/8] drm/i915: Introduce execlist_port_* accessors

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2017-09-20 15:37:03)
>> Instead of trusting that first available port is at index 0,
>> use accessor to hide this. This is a preparation for a
>> following patches where head can be at arbitrary location
>> in the port array.
>> 
>> v2: improved commit message, elsp_ready readability (Chris)
>> 
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c        | 16 +++++++----
>>  drivers/gpu/drm/i915/i915_gpu_error.c      |  4 +--
>>  drivers/gpu/drm/i915/i915_guc_submission.c | 17 ++++++-----
>>  drivers/gpu/drm/i915/i915_irq.c            |  2 +-
>>  drivers/gpu/drm/i915/intel_engine_cs.c     |  2 +-
>>  drivers/gpu/drm/i915/intel_lrc.c           | 42 +++++++++++++++------------
>>  drivers/gpu/drm/i915/intel_ringbuffer.h    | 46 ++++++++++++++++++++++++++----
>>  7 files changed, 87 insertions(+), 42 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index dbeb6f08ab79..af8cc2eab1b1 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -3348,16 +3348,20 @@ static int i915_engine_info(struct seq_file *m, void *unused)
>>  
>>                         rcu_read_lock();
>>                         for (idx = 0; idx < execlist_num_ports(el); idx++) {
>> -                               unsigned int count;
>> +                               const struct execlist_port *port;
>> +                               unsigned int count, n;
>>  
>> -                               rq = port_unpack(&el->port[idx], &count);
>> +                               port = execlist_port_index(el, idx);
>> +                               n = port_index(port, el);
>
> Bah, execlist_port_index() implies to me that it should return the
> index, not the port. I would just call it execlist_port(). How does that
> look?

It looks much better.

>
>> -static inline void
>> +#define __port_idx(start, index, mask) (((start) + (index)) & (mask))
>> +
>> +static inline struct execlist_port *
>> +execlist_port_head(struct intel_engine_execlist * const el)
>> +{
>> +       return &el->port[el->port_head];
>> +}
>> +
>> +/* Index starting from port_head */
>> +static inline struct execlist_port *
>> +execlist_port_index(struct intel_engine_execlist * const el,
>> +                   const unsigned int n)
>> +{
>> +       return &el->port[__port_idx(el->port_head, n, el->port_mask)];
>> +}
>> +
>> +static inline struct execlist_port *
>> +execlist_port_tail(struct intel_engine_execlist * const el)
>> +{
>> +       return &el->port[__port_idx(el->port_head, -1, el->port_mask)];
>> +}
>
> Hmm, I was expecting
>
> execlist_port_head() { return execlist_port(el, 0); }
> execlist_port_tail() { return execlist_port(el, -1); }

Seems that I did these on the next patch, moved to here.

>
> What's the impact on object size? (As a quick guide to how much the
> compiler can keep the code in check.)

I can't say what would constitute as a still in check, but things
grow:

add/remove: 0/0 grow/shrink: 6/1 up/down: 277/-26 (251)
function                                     old     new   delta
intel_lrc_irq_handler                       1591    1700    +109
i915_guc_irq_handler                        1041    1110     +69
i915_engine_info                            1983    2031     +48
insert_request                               127     152     +25
intel_engine_is_idle                         304     317     +13
gen8_cs_irq_handler                          113     126     +13
capture                                     5454    5428     -26
Total: Before=1144612, After=1144863, chg +0.02%

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