Re: [PATCH 2/5] drm/i915: More assorted dev_priv cleanups

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Nov 04, 2016 at 04:03:55PM +0000, Tvrtko Ursulin wrote:
> 
> 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@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@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.

The original grouping looked more based on functionality, so made a bit
more sense to me.

> 
> > 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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx