From: Ben Widawsky <ben@bwidawsk.net>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 1/2] drm/i915: Group the GT routines together in both code and vtable
Date: Wed, 13 Jun 2012 12:07:19 -0700 [thread overview]
Message-ID: <20120613120719.79caed44@bwidawsk.net> (raw)
In-Reply-To: <1339608592-27360-1-git-send-email-chris@chris-wilson.co.uk>
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
>
> 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?
> @@ -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
next prev parent reply other threads:[~2012-06-13 19:07 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-06-13 17:29 [PATCH 1/2] drm/i915: Group the GT routines together in both code and vtable Chris Wilson
2012-06-13 17:29 ` [PATCH 2/2] drm/i915: Implement w/a for sporadic read failures on waking from rc6 Chris Wilson
2012-06-15 18:31 ` Eugeni Dodonov
2012-06-13 19:07 ` Ben Widawsky [this message]
2012-06-13 19:47 ` [PATCH 1/2] drm/i915: Group the GT routines together in both code and vtable Chris Wilson
2012-06-15 15:11 ` Eugeni Dodonov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120613120719.79caed44@bwidawsk.net \
--to=ben@bwidawsk.net \
--cc=chris@chris-wilson.co.uk \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.