Re: [PATCH] drm/i915: Small display interrupt handlers tidy

Dave Gordon
2016-05-06T13:53:56Z

On 06/05/16 14:27, Tvrtko Ursulin wrote:
>
> On 06/05/16 12:27, Chris Wilson wrote:
>> On Fri, May 06, 2016 at 12:07:04PM +0100, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> I have noticed some of our interrupt handlers use both dev and
>>> dev_priv while they could get away with only dev_priv in the
>>> huge majority of cases.
>>>
>>> Tidying that up had a cascading effect on changing functions
>>> prototypes, so relatively big churn factor, but I think it is
>>> for the better.
>>>
>>> For example even where changes cascade out of i915_irq.c, for
>>> functions prefixed with intel_, genX_ or <plat>_, it makes more
>>> sense to take dev_priv directly anyway.
>>>
>>> This allows us to eliminate local variables and intermixed usage
>>> of dev and dev_priv where only one is good enough.
>>>
>>> End result is shrinkage of both source and the resulting binary.
>>>
>>> i915.ko:
>>>
>>> - .text 000b0899
>>> + .text 000b0619
>>>
>>> Or if we look at the Gen8 display irq chain:
>>>
>>> -00000000000006ad t gen8_irq_handler
>>> +0000000000000663 t gen8_irq_handler
>>> -0000000000000028 T intel_opregion_asle_intr
>>> +0000000000000024 T intel_opregion_asle_intr
>>> -000000000000008c t ilk_hpd_irq_handler
>>> +000000000000007f t ilk_hpd_irq_handler
>>> -0000000000000116 T intel_check_page_flip
>>> +0000000000000112 T intel_check_page_flip
>>> -000000000000011a T intel_prepare_page_flip
>>> +0000000000000119 T intel_prepare_page_flip
>>> -0000000000000014 T intel_finish_page_flip_plane
>>> +0000000000000013 T intel_finish_page_flip_plane
>>> -0000000000000053 t hsw_pipe_crc_irq_handler
>>> +000000000000004c t hsw_pipe_crc_irq_handler
>>> -000000000000022e t cpt_irq_handler
>>> +0000000000000213 t cpt_irq_handler
>>>
>>> So small shrinkage but it is all fast paths so doesn't harm.
>>>
>>> Situation is similar in other interrupt handlers as well.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> ---
>>> drivers/gpu/drm/i915/i915_drv.c | 2 +-
>>> drivers/gpu/drm/i915/i915_drv.h | 13 +-
>>> drivers/gpu/drm/i915/i915_irq.c | 386
>>> ++++++++++++++++------------------
>>> drivers/gpu/drm/i915/intel_display.c | 33 ++-
>>> drivers/gpu/drm/i915/intel_drv.h | 12 +-
>>> drivers/gpu/drm/i915/intel_hotplug.c | 13 +-
>>> drivers/gpu/drm/i915/intel_opregion.c | 4 +-
>>> drivers/gpu/drm/i915/intel_pm.c | 33 ++-
>>> 8 files changed, 232 insertions(+), 264 deletions(-)
>>
>> I have a similar series with almost identical impact, so from that
>> perspective we should be close. I found we could measure an improvement
>> in IRQ waiter wakeup latency (but only about ~2% so right on the noise
>> level) for gen6/gen7.
>>
>> Patch looks good, spot checking against mine:
>>
>> @@ -1617,8 +1617,7 @@ void gen6_rps_idle(struct drm_i915_private
>> *dev_priv);
>> void gen6_rps_boost(struct drm_i915_private *dev_priv,
>> struct intel_rps_client *rps,
>> unsigned long submitted);
>> -void intel_queue_rps_boost_for_request(struct drm_device *dev,
>> - struct drm_i915_gem_request *req);
>> +void intel_queue_rps_boost_for_request(struct drm_i915_gem_request
>> *req);
>
> Against your tree or your patch? If latter that would be quite amazing!
>
>> Follow up whilst waiting for CI?
>
> Sure.
>
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> Thanks!
>
> Regards,
> Tvrtko

I've got some Cocci scripts that do the whole kaboodle, including
INTEL_INFO()->gen => INTEL_GEN(), and passing dev_priv to INTEL_INFO()
and all the IS_X() macros wherever possible. Saves ~1.5Kb, but lots and
lots of churn. Perhaps we should pick-n-choose single files or groups of
files at a time? intel_display.c and intel_pm.c are the biggest targets,
whereas intel_lrc.c has only a couple remaining.

.Dave.