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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, 13 Jun 2012 18:29:51 +0100
Chris Wilson <chris at 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

> 
> Signed-off-by: Chris Wilson <chris at 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?

> @@ -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?
 
>  	I915_WRITE_NOTRACE(FORCEWAKE, 1);
> -	POSTING_READ(FORCEWAKE);
>  
> -	count = 0;
> -	while (count++ < 50 && (I915_READ_NOTRACE(FORCEWAKE_ACK) & 1) == 0)
> -		udelay(10);
> +	timeout = jiffies + msecs_to_jiffies(1);
> +	while ((I915_READ_NOTRACE(FORCEWAKE_ACK) & 1) == 0 &&
> +	       time_before(jiffies, timeout))
> +		;
>  }

Same as above.

>  
> -void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv)
> +static void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv)
>  {
> -	int count;
> +	unsigned long timeout;
>  
> -	count = 0;
> -	while (count++ < 50 && (I915_READ_NOTRACE(FORCEWAKE_MT_ACK) & 1))
> -		udelay(10);
> +	timeout = jiffies + msecs_to_jiffies(1);
> +	while (I915_READ_NOTRACE(FORCEWAKE_MT_ACK) & 1 &&
> +	       time_before(jiffies, timeout))
> +		;
>  

Again

>  	I915_WRITE_NOTRACE(FORCEWAKE_MT, _MASKED_BIT_ENABLE(1));
> -	POSTING_READ(FORCEWAKE_MT);
>  
> -	count = 0;
> -	while (count++ < 50 && (I915_READ_NOTRACE(FORCEWAKE_MT_ACK) & 1) == 0)
> -		udelay(10);
> +	timeout = jiffies + msecs_to_jiffies(1);
> +	while ((I915_READ_NOTRACE(FORCEWAKE_MT_ACK) & 1) == 0 &&
> +	       time_before(jiffies, timeout))
> +		;
>  }
>  

You get the idea

>  /*
> @@ -467,7 +470,7 @@ void gen6_gt_force_wake_get(struct drm_i915_private *dev_priv)
>  
>  	spin_lock_irqsave(&dev_priv->gt_lock, irqflags);
>  	if (dev_priv->forcewake_count++ == 0)
> -		dev_priv->display.force_wake_get(dev_priv);
> +		dev_priv->gt.force_wake_get(dev_priv);
>  	spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags);
>  }
>  
> @@ -480,14 +483,14 @@ static void gen6_gt_check_fifodbg(struct drm_i915_private *dev_priv)
>  		I915_WRITE_NOTRACE(GTFIFODBG, GT_FIFO_CPU_ERROR_MASK);
>  }
>  
> -void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
> +static void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
>  {
>  	I915_WRITE_NOTRACE(FORCEWAKE, 0);
>  	/* The below doubles as a POSTING_READ */
>  	gen6_gt_check_fifodbg(dev_priv);
>  }
>  
> -void __gen6_gt_force_wake_mt_put(struct drm_i915_private *dev_priv)
> +static void __gen6_gt_force_wake_mt_put(struct drm_i915_private *dev_priv)
>  {
>  	I915_WRITE_NOTRACE(FORCEWAKE_MT, _MASKED_BIT_DISABLE(1));
>  	/* The below doubles as a POSTING_READ */
> @@ -503,7 +506,7 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
>  
>  	spin_lock_irqsave(&dev_priv->gt_lock, irqflags);
>  	if (--dev_priv->forcewake_count == 0)
> -		dev_priv->display.force_wake_put(dev_priv);
> +		dev_priv->gt.force_wake_put(dev_priv);
>  	spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags);
>  }
>  
> @@ -527,7 +530,7 @@ int __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
>  	return ret;
>  }
>  
> -void vlv_force_wake_get(struct drm_i915_private *dev_priv)
> +static void vlv_force_wake_get(struct drm_i915_private *dev_priv)
>  {
>  	int count;
>  
> @@ -545,13 +548,54 @@ void vlv_force_wake_get(struct drm_i915_private *dev_priv)
>  		udelay(10);
>  }
>  
> -void vlv_force_wake_put(struct drm_i915_private *dev_priv)
> +static void vlv_force_wake_put(struct drm_i915_private *dev_priv)
>  {
>  	I915_WRITE_NOTRACE(FORCEWAKE_VLV, 0xffff0000);
>  	/* FIXME: confirm VLV behavior with Punit folks */
>  	POSTING_READ(FORCEWAKE_VLV);
>  }
>  
> +void intel_gt_init(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	spin_lock_init(&dev_priv->gt_lock);
> +
> +	if (IS_VALLEYVIEW(dev)) {
> +		dev_priv->gt.force_wake_get = vlv_force_wake_get;
> +		dev_priv->gt.force_wake_put = vlv_force_wake_put;
> +	} else if (INTEL_INFO(dev)->gen >= 6) {
> +		dev_priv->gt.force_wake_get = __gen6_gt_force_wake_get;
> +		dev_priv->gt.force_wake_put = __gen6_gt_force_wake_put;
> +
> +		/* IVB configs may use multi-threaded forcewake */
> +		if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) {
> +			u32 ecobus;
> +
> +			/* A small trick here - if the bios hasn't configured
> +			 * MT forcewake, and if the device is in RC6, then
> +			 * force_wake_mt_get will not wake the device and the
> +			 * ECOBUS read will return zero. Which will be
> +			 * (correctly) interpreted by the test below as MT
> +			 * forcewake being disabled.
> +			 */
> +			mutex_lock(&dev->struct_mutex);
> +			__gen6_gt_force_wake_mt_get(dev_priv);
> +			ecobus = I915_READ_NOTRACE(ECOBUS);
> +			__gen6_gt_force_wake_mt_put(dev_priv);
> +			mutex_unlock(&dev->struct_mutex);
> +
> +			if (ecobus & FORCEWAKE_MT_ENABLE) {
> +				DRM_DEBUG_KMS("Using MT version of forcewake\n");
> +				dev_priv->gt.force_wake_get =
> +					__gen6_gt_force_wake_mt_get;
> +				dev_priv->gt.force_wake_put =
> +					__gen6_gt_force_wake_mt_put;
> +			}
> +		}
> +	}
> +}
> +
>  static int i915_drm_freeze(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -788,9 +832,9 @@ static int gen6_do_reset(struct drm_device *dev)
>  
>  	/* If reset with a user forcewake, try to restore, otherwise turn it off */
>  	if (dev_priv->forcewake_count)
> -		dev_priv->display.force_wake_get(dev_priv);
> +		dev_priv->gt.force_wake_get(dev_priv);
>  	else
> -		dev_priv->display.force_wake_put(dev_priv);
> +		dev_priv->gt.force_wake_put(dev_priv);
>  
>  	/* Restore fifo count */
>  	dev_priv->gt_fifo_count = I915_READ_NOTRACE(GT_FIFO_FREE_ENTRIES);
> @@ -1151,10 +1195,10 @@ u##x i915_read##x(struct drm_i915_private *dev_priv, u32 reg) { \
>  		unsigned long irqflags; \
>  		spin_lock_irqsave(&dev_priv->gt_lock, irqflags); \
>  		if (dev_priv->forcewake_count == 0) \
> -			dev_priv->display.force_wake_get(dev_priv); \
> +			dev_priv->gt.force_wake_get(dev_priv); \
gg>  		val = read##y(dev_priv->regs + reg); \
>  		if (dev_priv->forcewake_count == 0) \
> -			dev_priv->display.force_wake_put(dev_priv); \
> +			dev_priv->gt.force_wake_put(dev_priv); \
>  		spin_unlock_irqrestore(&dev_priv->gt_lock, irqflags); \
>  	} else { \
>  		val = read##y(dev_priv->regs + reg); \
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6f0dff9..b00119a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -261,8 +261,6 @@ struct drm_i915_display_funcs {
>  			  struct drm_i915_gem_object *obj);
>  	int (*update_plane)(struct drm_crtc *crtc, struct drm_framebuffer *fb,
>  			    int x, int y);
> -	void (*force_wake_get)(struct drm_i915_private *dev_priv);
> -	void (*force_wake_put)(struct drm_i915_private *dev_priv);
>  	/* clock updates for mode set */
>  	/* cursor updates */
>  	/* render clock increase/decrease */
> @@ -270,6 +268,11 @@ struct drm_i915_display_funcs {
>  	/* pll clock increase/decrease */
>  };
>  
> +struct drm_i915_gt_funcs {
> +	void (*force_wake_get)(struct drm_i915_private *dev_priv);
> +	void (*force_wake_put)(struct drm_i915_private *dev_priv);
> +};
> +
>  struct intel_device_info {
>  	u8 gen;
>  	u8 is_mobile:1;
> @@ -349,6 +352,8 @@ typedef struct drm_i915_private {
>  	int relative_constants_mode;
>  
>  	void __iomem *regs;
> +
> +	struct drm_i915_gt_funcs gt;
>  	/** gt_fifo_count and the subsequent register write are synchronized
>  	 * with dev->struct_mutex. */
>  	unsigned gt_fifo_count;
> @@ -1177,6 +1182,7 @@ void i915_hangcheck_elapsed(unsigned long data);
>  void i915_handle_error(struct drm_device *dev, bool wedged);
>  
>  extern void intel_irq_init(struct drm_device *dev);
> +extern void intel_gt_init(struct drm_device *dev);
>  
>  void i915_error_state_free(struct kref *error_ref);
>  
> @@ -1484,13 +1490,6 @@ extern int intel_trans_dp_port_sel(struct drm_crtc *crtc);
>  extern int intel_enable_rc6(const struct drm_device *dev);
>  
>  extern bool i915_semaphore_is_enabled(struct drm_device *dev);
> -extern void __gen6_gt_force_wake_get(struct drm_i915_private *dev_priv);
> -extern void __gen6_gt_force_wake_mt_get(struct drm_i915_private *dev_priv);
> -extern void __gen6_gt_force_wake_put(struct drm_i915_private *dev_priv);
> -extern void __gen6_gt_force_wake_mt_put(struct drm_i915_private *dev_priv);
> -
> -extern void vlv_force_wake_get(struct drm_i915_private *dev_priv);
> -extern void vlv_force_wake_put(struct drm_i915_private *dev_priv);
>  
>  /* overlay */
>  #ifdef CONFIG_DEBUG_FS
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index acbfe9e..45d700d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6656,9 +6656,6 @@ static void intel_init_display(struct drm_device *dev)
>  			dev_priv->display.write_eld = ironlake_write_eld;
>  		} else
>  			dev_priv->display.update_wm = NULL;
> -	} else if (IS_VALLEYVIEW(dev)) {
> -		dev_priv->display.force_wake_get = vlv_force_wake_get;
> -		dev_priv->display.force_wake_put = vlv_force_wake_put;
>  	} else if (IS_G4X(dev)) {
>  		dev_priv->display.write_eld = g4x_write_eld;
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a62c673..82fcbb4 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3686,34 +3686,6 @@ void intel_init_pm(struct drm_device *dev)
>  
>  	/* For FIFO watermark updates */
>  	if (HAS_PCH_SPLIT(dev)) {
> -		dev_priv->display.force_wake_get = __gen6_gt_force_wake_get;
> -		dev_priv->display.force_wake_put = __gen6_gt_force_wake_put;
> -
> -		/* IVB configs may use multi-threaded forcewake */
> -		if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) {
> -			u32	ecobus;
> -
> -			/* A small trick here - if the bios hasn't configured MT forcewake,
> -			 * and if the device is in RC6, then force_wake_mt_get will not wake
> -			 * the device and the ECOBUS read will return zero. Which will be
> -			 * (correctly) interpreted by the test below as MT forcewake being
> -			 * disabled.
> -			 */
> -			mutex_lock(&dev->struct_mutex);
> -			__gen6_gt_force_wake_mt_get(dev_priv);
> -			ecobus = I915_READ_NOTRACE(ECOBUS);
> -			__gen6_gt_force_wake_mt_put(dev_priv);
> -			mutex_unlock(&dev->struct_mutex);
> -
> -			if (ecobus & FORCEWAKE_MT_ENABLE) {
> -				DRM_DEBUG_KMS("Using MT version of forcewake\n");
> -				dev_priv->display.force_wake_get =
> -					__gen6_gt_force_wake_mt_get;
> -				dev_priv->display.force_wake_put =
> -					__gen6_gt_force_wake_mt_put;
> -			}
> -		}
> -
>  		if (HAS_PCH_IBX(dev))
>  			dev_priv->display.init_pch_clock_gating = ibx_init_clock_gating;
>  		else if (HAS_PCH_CPT(dev))
> @@ -3769,8 +3741,6 @@ void intel_init_pm(struct drm_device *dev)
>  		dev_priv->display.update_wm = valleyview_update_wm;
>  		dev_priv->display.init_clock_gating =
>  			valleyview_init_clock_gating;
> -		dev_priv->display.force_wake_get = vlv_force_wake_get;
> -		dev_priv->display.force_wake_put = vlv_force_wake_put;
>  	} else if (IS_PINEVIEW(dev)) {
>  		if (!intel_get_cxsr_latency(IS_PINEVIEW_G(dev),
>  					    dev_priv->is_ddr3,



-- 
Ben Widawsky, Intel Open Source Technology Center


[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux