From: Ben Widawsky <ben@bwidawsk.net>
To: Eugeni Dodonov <eugeni.dodonov@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 01/10] drm/i915: Group the GT routines together in both code and vtable
Date: Mon, 2 Jul 2012 09:41:39 -0700 [thread overview]
Message-ID: <20120702094139.7e7919cb@bwidawsk.net> (raw)
In-Reply-To: <1341240671-5843-2-git-send-email-eugeni.dodonov@intel.com>
On Mon, 2 Jul 2012 11:51:02 -0300
Eugeni Dodonov <eugeni.dodonov@intel.com> wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
>
> Tidy up the routines for interacting with the GT (in particular the
> forcewake dance) which are scattered throughout the code in a single
> structure.
>
> v2: use wait_for_atomic for polling.
>
> v3: *really* use wait_for_atomic for polling.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
comments below, but still it's
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> ---
> drivers/gpu/drm/i915/i915_dma.c | 2 +-
> drivers/gpu/drm/i915/i915_drv.c | 101 ++++++++++++++++++++++-------------
> 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, 73 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 2166519..4dc7697 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);
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 79be879..928b667 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>
> @@ -432,36 +433,26 @@ 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;
> -
> - count = 0;
> - while (count++ < 50 && (I915_READ_NOTRACE(FORCEWAKE_ACK) & 1))
> - udelay(10);
> + if (wait_for_atomic_us((I915_READ_NOTRACE(FORCEWAKE_ACK) & 1) == 0, 500))
> + DRM_ERROR("Force wake wait timed out\n");
(Applies to all instances)
I'd really prefer a WARN_ON to get a backtrace for such cases. It just
dawned on me that since we moved to separate locking, does it make sense
to do a wedge check here also?
>
> I915_WRITE_NOTRACE(FORCEWAKE, 1);
> - POSTING_READ(FORCEWAKE);
This is an unnecessary optimization than only makes things a bit more
difficult to read IMO.
>
> - count = 0;
> - while (count++ < 50 && (I915_READ_NOTRACE(FORCEWAKE_ACK) & 1) == 0)
> - udelay(10);
> + if (wait_for_atomic_us((I915_READ_NOTRACE(FORCEWAKE_ACK) & 1), 500))
> + DRM_ERROR("Force wake wait timed out\n");
> }
>
> -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;
> -
> - count = 0;
> - while (count++ < 50 && (I915_READ_NOTRACE(FORCEWAKE_MT_ACK) & 1))
> - udelay(10);
> + if (wait_for_atomic_us((I915_READ_NOTRACE(FORCEWAKE_MT_ACK) & 1) == 0, 500))
> + DRM_ERROR("Force wake wait timed out\n");
>
> 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);
> + if (wait_for_atomic_us((I915_READ_NOTRACE(FORCEWAKE_MT_ACK) & 1), 500))
> + DRM_ERROR("Force wake wait timed out\n");
> }
>
> /*
> @@ -476,7 +467,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);
> }
>
> @@ -489,14 +480,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 */
> @@ -512,7 +503,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);
> }
>
> @@ -536,12 +527,8 @@ 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;
> -
> - count = 0;
> -
> /* Already awake? */
> if ((I915_READ(0x130094) & 0xa1) == 0xa1)
> return;
> @@ -549,18 +536,58 @@ void vlv_force_wake_get(struct drm_i915_private *dev_priv)
> I915_WRITE_NOTRACE(FORCEWAKE_VLV, 0xffffffff);
> POSTING_READ(FORCEWAKE_VLV);
>
> - count = 0;
> - while (count++ < 50 && (I915_READ_NOTRACE(FORCEWAKE_ACK_VLV) & 1) == 0)
> - udelay(10);
> + if (wait_for_atomic_us((I915_READ_NOTRACE(FORCEWAKE_ACK_VLV) & 1), 500))
> + DRM_ERROR("Force wake wait timed out\n");
> }
>
> -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;
> @@ -797,9 +824,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);
> @@ -1248,10 +1275,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); \
> 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 if (IS_VALLEYVIEW(dev_priv->dev) && IS_DISPLAYREG(reg)) { \
> val = read##y(dev_priv->regs + reg + 0x180000); \
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a0c15ab..60f6974 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -262,8 +262,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 */
> @@ -271,6 +269,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;
> @@ -362,6 +365,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;
> @@ -1200,6 +1205,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);
>
> @@ -1517,13 +1523,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 0438a10..3ca91e2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7121,9 +7121,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 a1d3da7..29720d2 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3763,34 +3763,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))
> @@ -3846,8 +3818,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-07-02 16:41 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-07-02 14:51 [PATCH 00/10] Haswell force wake/rps v3 Eugeni Dodonov
2012-07-02 14:51 ` [PATCH 01/10] drm/i915: Group the GT routines together in both code and vtable Eugeni Dodonov
2012-07-02 16:41 ` Ben Widawsky [this message]
2012-07-02 14:51 ` [PATCH 02/10] drm/i915: Implement w/a for sporadic read failures on waking from rc6 Eugeni Dodonov
2012-07-02 16:46 ` Ben Widawsky
2012-07-02 14:51 ` [PATCH 03/10] drm/i915: support Haswell force waking Eugeni Dodonov
2012-07-02 16:49 ` Ben Widawsky
2012-07-02 14:51 ` [PATCH 04/10] drm/i915: add RPS configuration for Haswell Eugeni Dodonov
2012-07-02 17:49 ` Ben Widawsky
2012-07-02 20:02 ` Eugeni Dodonov
2012-07-02 21:19 ` Ben Widawsky
2012-07-04 21:34 ` Chris Wilson
2012-07-02 14:51 ` [PATCH 05/10] drm/i915: slightly improve gt enable/disable routines Eugeni Dodonov
2012-07-02 17:51 ` Ben Widawsky
2012-07-02 14:51 ` [PATCH 06/10] drm/i915: enable RC6 by default on Haswell Eugeni Dodonov
2012-07-02 18:11 ` Ben Widawsky
2012-07-02 14:51 ` [PATCH 07/10] drm/i915: disable RC6 when disabling rps Eugeni Dodonov
2012-07-02 18:36 ` Ben Widawsky
2012-07-02 14:51 ` [PATCH 08/10] drm/i915: introduce haswell_init_clock_gating Eugeni Dodonov
2012-07-02 18:39 ` Ben Widawsky
2012-07-03 20:24 ` Daniel Vetter
2012-07-04 0:03 ` Eugeni Dodonov
2012-07-04 7:27 ` Daniel Vetter
2012-07-02 14:51 ` [PATCH 09/10] drm/i915: enable RC6 workaround on Haswell Eugeni Dodonov
2012-07-02 18:40 ` Ben Widawsky
2012-07-02 14:51 ` [PATCH 10/10] drm/i915: move force wake support into intel_pm Eugeni Dodonov
2012-07-02 18:41 ` Ben Widawsky
2012-07-04 7:34 ` Daniel Vetter
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=20120702094139.7e7919cb@bwidawsk.net \
--to=ben@bwidawsk.net \
--cc=eugeni.dodonov@intel.com \
--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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox