Re: [PATCH 1/9] drm/i915: Move num_pipes to intel info

From: Ben Widawsky <ben@bwidawsk.net>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	intel-gfx@lists.freedesktop.org,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	"Mcallister, Jeffrey" <jeffrey.mcallister@intel.com>
Subject: Re: [PATCH 1/9] drm/i915: Move num_pipes to intel info
Date: Wed, 13 Mar 2013 11:36:22 -0700	[thread overview]
Message-ID: <20130313183620.GA21987@bwidawsk.net> (raw)
In-Reply-To: <20130313183116.GA17558@cantiga.alporthouse.com>

On Wed, Mar 13, 2013 at 06:31:16PM +0000, Chris Wilson wrote:
> On Wed, Mar 13, 2013 at 11:21:00AM -0700, Ben Widawsky wrote:
> > Requested by Daniel.
> > 
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c      |  9 +------
> >  drivers/gpu/drm/i915/i915_drv.c      | 48 ++++++++++++++++++------------------
> >  drivers/gpu/drm/i915/i915_drv.h      |  4 +--
> >  drivers/gpu/drm/i915/i915_irq.c      |  3 +--
> >  drivers/gpu/drm/i915/intel_display.c | 19 +++++++-------
> >  drivers/gpu/drm/i915/intel_fb.c      |  2 +-
> >  drivers/gpu/drm/i915/intel_panel.c   |  2 +-
> >  7 files changed, 40 insertions(+), 47 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index e16099b..ebcfe2e 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1630,14 +1630,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >  	mutex_init(&dev_priv->rps.hw_lock);
> >  	mutex_init(&dev_priv->modeset_restore_lock);
> >  
> > -	if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev))
> > -		dev_priv->num_pipe = 3;
> > -	else if (IS_MOBILE(dev) || !IS_GEN2(dev))
> > -		dev_priv->num_pipe = 2;
> > -	else
> > -		dev_priv->num_pipe = 1;
> > -
> > -	ret = drm_vblank_init(dev, dev_priv->num_pipe);
> > +	ret = drm_vblank_init(dev, INTEL_INFO(dev)->num_pipes);
> >  	if (ret)
> >  		goto out_gem_unload;
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 1ebed96..0849651 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -136,74 +136,74 @@ extern int intel_agp_enabled;
> >  	.driver_data = (unsigned long) info }
> >  
> >  static const struct intel_device_info intel_i830_info = {
> > -	.gen = 2, .is_mobile = 1, .cursor_needs_physical = 1,
> > +	.gen = 2, .is_mobile = 1, .cursor_needs_physical = 1, .num_pipes = 1,
> 2 pipes for 830gm
> 
> >  static const struct intel_device_info intel_i85x_info = {
> > -	.gen = 2, .is_i85x = 1, .is_mobile = 1,
> > +	.gen = 2, .is_i85x = 1, .is_mobile = 1, .num_pipes = 1,
> 2 pipes for 855gm
> 
> >  static const struct intel_device_info intel_i915g_info = {
> > -	.gen = 3, .is_i915g = 1, .cursor_needs_physical = 1,
> > +	.gen = 3, .is_i915g = 1, .cursor_needs_physical = 1, .num_pipes = 1,
> 2 pipes for 915g
> 
> >  static const struct intel_device_info intel_i945g_info = {
> > -	.gen = 3, .has_hotplug = 1, .cursor_needs_physical = 1,
> > +	.gen = 3, .has_hotplug = 1, .cursor_needs_physical = 1, .num_pipes = 1,
> 2 pipes for 945g
> 
> >  static const struct intel_device_info intel_i965g_info = {
> > -	.gen = 4, .is_broadwater = 1,
> > +	.gen = 4, .is_broadwater = 1, .num_pipes = 1,
> 2 pipes for 965g
> >  
> >  static const struct intel_device_info intel_g33_info = {
> > -	.gen = 3, .is_g33 = 1,
> > +	.gen = 3, .is_g33 = 1, .num_pipes = 1,
> 2 pipes for g33
> 
> >  	.need_gfx_hws = 1, .has_hotplug = 1,
> >  	.has_overlay = 1,
> >  };
> >  
> >  static const struct intel_device_info intel_g45_info = {
> > -	.gen = 4, .is_g4x = 1, .need_gfx_hws = 1,
> > +	.gen = 4, .is_g4x = 1, .need_gfx_hws = 1, .num_pipes = 1,
> 2 pipes for g45
> 
> >  static const struct intel_device_info intel_ironlake_d_info = {
> > -	.gen = 5,
> > +	.gen = 5, .num_pipes = 1,
> 2 pipes for ilk-d
> 
> >  static const struct intel_device_info intel_sandybridge_d_info = {
> > -	.gen = 6,
> > +	.gen = 6, .num_pipes = 1,
> 2 pipes for snb-d
> 
> >  static const struct intel_device_info intel_valleyview_d_info = {
> > -	.gen = 7,
> > +	.gen = 7, .num_pipes = 3,
> 2?
> 
> > @@ -341,6 +341,7 @@ struct drm_i915_gt_funcs {
> >  
> >  struct intel_device_info {
> >  	u32 display_mmio_offset;
> > +	u8 num_pipes:3;
> A hole!
> 
> >  	u8 gen;
> >  	u8 is_mobile:1;
> >  	u8 is_i85x:1;
> > @@ -912,7 +913,6 @@ typedef struct drm_i915_private {
> >  	struct work_struct hotplug_work;
> >  	bool enable_hotplug_processing;
> >  
> > -	int num_pipe;
> >  	int num_pch_pll;
> >  
> >  	unsigned long cfb_size;
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 2139714..b860f0b 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -254,10 +254,9 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe,
> >  			      struct timeval *vblank_time,
> >  			      unsigned flags)
> >  {
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_crtc *crtc;
> >  
> > -	if (pipe < 0 || pipe >= dev_priv->num_pipe) {
> > +	if (pipe < 0 || pipe >= INTEL_INFO(dev)->num_pipes) {
> num_pipes is unsigned so pipe<0 is a redundant compare
> 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 502cb28..23379e7 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2335,10 +2335,10 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
> >  		return 0;
> >  	}
> >  
> > -	if(intel_crtc->plane > dev_priv->num_pipe) {
> > +	if (intel_crtc->plane > INTEL_INFO(dev)->num_pipes) {
> 
>  >= ?
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

Thanks for detailed review. Sloppy code is a result of working while at
a conference. I'll apply these fixes locally presently.

-- 
Ben Widawsky, Intel Open Source Technology Center