Re: [PATCH 1/2] drm/i915: Group the GT routines together in both code and vtable

[Chris Wilson <chris@chris-wilson.co.uk>]

On Wed, 13 Jun 2012 12:07:19 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Wed, 13 Jun 2012 18:29:51 +0100
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > Tidy up the routines for interacting with the GT (in particular the
> > forcewake dance) which are scattered throughout the code in a single
> > structure.
> 
> A few comments inline. First though, the bikeshed:
> 
> I'd really rather the structure not be named, "gt" unless you have
> further reaching plans for it. GT is way to generic. Also, I think it
> makes a lot of sense to move the forcewake dancing into intel_pm.c

This patch predated the intel_pm split. I toyed with the idea of
updating it, but preferred to get feedback first. Shall we call it
grantsdale instead? Or uncore? My opinion is that this more core
functionality than power-management, but first and foremost it
should not be scattered across multiple files.
 
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c      |    2 +-
> >  drivers/gpu/drm/i915/i915_drv.c      |  100 ++++++++++++++++++++++++----------
> >  drivers/gpu/drm/i915/i915_drv.h      |   17 +++---
> >  drivers/gpu/drm/i915/intel_display.c |    3 -
> >  drivers/gpu/drm/i915/intel_pm.c      |   30 ----------
> >  5 files changed, 81 insertions(+), 71 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index fa8f269..a605928 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -1548,6 +1548,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >  	}
> >  
> >  	intel_irq_init(dev);
> > +	intel_gt_init(dev);
> >  
> >  	/* Try to make sure MCHBAR is enabled before poking at it */
> >  	intel_setup_mchbar(dev);
> > @@ -1580,7 +1581,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >  	if (!IS_I945G(dev) && !IS_I945GM(dev))
> >  		pci_enable_msi(dev->pdev);
> >  
> > -	spin_lock_init(&dev_priv->gt_lock);
> >  	spin_lock_init(&dev_priv->irq_lock);
> >  	spin_lock_init(&dev_priv->error_lock);
> >  	spin_lock_init(&dev_priv->rps_lock);
> 
> By this logic, we should also move the irq_lock init to intel_irq_init,
> and the rps_lock init to enable_rps or something.
> 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 238a521..2058316 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -32,6 +32,7 @@
> >  #include "drm.h"
> >  #include "i915_drm.h"
> >  #include "i915_drv.h"
> > +#include "i915_trace.h"
> >  #include "intel_drv.h"
> >  
> >  #include <linux/console.h>
> 
> What is this doing here?

An earlier version was more funky with grander purpose.
 
> > @@ -423,36 +424,38 @@ bool i915_semaphore_is_enabled(struct drm_device *dev)
> >  	return 1;
> >  }
> >  
> > -void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
> > +static void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
> >  {
> > -	int count;
> > +	unsigned long timeout;
> >  
> > -	count = 0;
> > -	while (count++ < 50 && (I915_READ_NOTRACE(FORCEWAKE_ACK) & 1))
> > -		udelay(10);
> > +	timeout = jiffies + msecs_to_jiffies(1);
> > +	while (I915_READ_NOTRACE(FORCEWAKE_ACK) & 1 &&
> > +	       time_before(jiffies, timeout))
> > +		;
> > 
> 
> Can't we just use the wait_for macros here?
Yes.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre