[Intel-gfx] [PATCH 2/5] drm/i915: More assorted dev_priv cleanups

[Tvrtko Ursulin]

On 04/11/2016 15:32, Ville Syrjälä wrote:
> On Fri, Nov 04, 2016 at 02:42:45PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>
>> A small selection of macros which can only accept dev_priv from
>> now on and a resulting trickle of fixups.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h       | 27 ++++++++++++---------------
>>  drivers/gpu/drm/i915/i915_gpu_error.c |  2 +-
>>  drivers/gpu/drm/i915/i915_irq.c       |  6 +++---
>>  drivers/gpu/drm/i915/intel_crt.c      |  8 ++++----
>>  drivers/gpu/drm/i915/intel_display.c  |  4 ++--
>>  drivers/gpu/drm/i915/intel_dp.c       |  2 +-
>>  drivers/gpu/drm/i915/intel_hotplug.c  |  2 +-
>>  drivers/gpu/drm/i915/intel_psr.c      |  2 +-
>>  8 files changed, 25 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 45a30f730216..6060e41d25e5 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2901,28 +2901,25 @@ struct drm_i915_cmd_table {
>>  #define HAS_128_BYTE_Y_TILING(dev_priv) (!IS_GEN2(dev_priv) && \
>>  					 !(IS_I915G(dev_priv) || \
>>  					 IS_I915GM(dev_priv)))
>> -#define SUPPORTS_TV(dev)		(INTEL_INFO(dev)->supports_tv)
>> -#define I915_HAS_HOTPLUG(dev)		 (INTEL_INFO(dev)->has_hotplug)
>> -
>> -#define HAS_FW_BLC(dev_priv) (INTEL_GEN(dev_priv) > 2)
>> -#define HAS_PIPE_CXSR(dev) (INTEL_INFO(dev)->has_pipe_cxsr)
>> -#define HAS_FBC(dev) (INTEL_INFO(dev)->has_fbc)
>> +#define SUPPORTS_TV(dev_priv)		((dev_priv)->info.supports_tv)
>> +#define I915_HAS_HOTPLUG(dev_priv)	((dev_priv)->info.has_hotplug)
>>
>> +#define HAS_FW_BLC(dev_priv) 	(INTEL_GEN(dev_priv) > 2)
>> +#define HAS_PIPE_CXSR(dev_priv) ((dev_priv)->info.has_pipe_cxsr)
>> +#define HAS_FBC(dev_priv)	((dev_priv)->info.has_fbc)
>>  #define HAS_IPS(dev_priv)	(IS_HSW_ULT(dev_priv) || IS_BROADWELL(dev_priv))
>> -
>> -#define HAS_DP_MST(dev)	(INTEL_INFO(dev)->has_dp_mst)
>> -
>> +#define HAS_DP_MST(dev_priv)	((dev_priv)->info.has_dp_mst)
>>  #define HAS_DDI(dev_priv)	((dev_priv)->info.has_ddi)
>> -#define HAS_FPGA_DBG_UNCLAIMED(dev)	(INTEL_INFO(dev)->has_fpga_dbg)
>> -#define HAS_PSR(dev)		(INTEL_INFO(dev)->has_psr)
>> -#define HAS_RC6(dev)		(INTEL_INFO(dev)->has_rc6)
>> -#define HAS_RC6p(dev)		(INTEL_INFO(dev)->has_rc6p)
>> -
>> -#define HAS_CSR(dev)	(INTEL_INFO(dev)->has_csr)
>> +#define HAS_PSR(dev_priv)	((dev_priv)->info.has_psr)
>> +#define HAS_RC6(dev_priv)	((dev_priv)->info.has_rc6)
>> +#define HAS_RC6p(dev_priv)	((dev_priv)->info.has_rc6p)
>> +#define HAS_CSR(dev_priv)	((dev_priv)->info.has_csr)
>>
>>  #define HAS_RUNTIME_PM(dev_priv) ((dev_priv)->info.has_runtime_pm)
>>  #define HAS_64BIT_RELOC(dev_priv) ((dev_priv)->info.has_64bit_reloc)
>>
>> +#define HAS_FPGA_DBG_UNCLAIMED(dev_priv) ((dev_priv)->info.has_fpga_dbg)
>
> What's confusing me is this reordering of these macros. Was there a
> particular reason for doing that?

Just because of its long name, so I pulled it out and separated so the 
alignment is nicer in the blocks above it.

> Outside that it all looks pretty reasonable. Could got a bit further
> with passing around dev_priv in some cases, but I guess we can leave
> that to future work.

Yes, I mention that in the cover letter.

> One random idea that did pop into my head was this:
>
> static inline const struct ... *
> intel_info(struct drm_i915_private *dev_priv)
> {
> 	return &dev_priv->info;
> }
> #define HAS_WHATEVER(dev_priv) (intel_info(dev_priv)->whatever)
>
> for some extra type safety. Any thoughts?

Sounds like a good idea to me. And it would be really easy to do, 
localized to i915_drv.h, and then when the last INTEL_INFO(dev) gets 
converted we can make it use the inline as well.

Regards,

Tvrtko