[PATCH 1/2] drm/i915: Give simulator a way to skip forcewake

[PATCH 1/2] drm/i915: Give simulator a way to skip forcewake

[Ben Widawsky]

Forcewake isn't implemented on the simulator, and doing the dance is
problematic. I haven't really root-caused it, but something in the chain
detects the polling of the ack register, and the system grinds to a
halt.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.h | 2 ++
 drivers/gpu/drm/i915/intel_pm.c | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index da1d33d..7a66f0a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -846,6 +846,8 @@ typedef struct drm_i915_private {
 	struct work_struct parity_error_work;
 	bool hw_contexts_disabled;
 	uint32_t hw_context_size;
+
+	bool is_simulator;
 } drm_i915_private_t;
 
 /* Iterate over initialised rings */
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 18c14d7..c436ea6 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4118,6 +4118,8 @@ static void vlv_force_wake_put(struct drm_i915_private *dev_priv)
 	POSTING_READ(FORCEWAKE_VLV);
 }
 
+static void null_wake(struct drm_i915_private *dev_priv) { }
+
 void intel_gt_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -4127,6 +4129,9 @@ void intel_gt_init(struct drm_device *dev)
 	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 (dev_priv->is_simulator) {
+		dev_priv->gt.force_wake_get = null_wake;
+		dev_priv->gt.force_wake_put = null_wake;
 	} 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;
-- 
1.7.11.2

[PATCH 2/2] drm/i915: Add simulator's host bridge

[Ben Widawsky]

Add the host bridge ID used by the simulator. This was added in a
previous patch for the agp layer, but wasn't preserved here.  It also
gives us an opportunity to let the rest of the driver know we're running
as the simulator for various workarounds.

We must always do this early as it's the only way we have to detect the
simulator.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 72e86a7..ebaaea1 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -387,6 +387,7 @@ MODULE_DEVICE_TABLE(pci, pciidlist);
 #define INTEL_PCH_CPT_DEVICE_ID_TYPE	0x1c00
 #define INTEL_PCH_PPT_DEVICE_ID_TYPE	0x1e00
 #define INTEL_PCH_LPT_DEVICE_ID_TYPE	0x8c00
+#define INTEL_PCH_HAS_DEVICE_ID_TYPE	0x7000
 
 void intel_detect_pch(struct drm_device *dev)
 {
@@ -422,6 +423,12 @@ void intel_detect_pch(struct drm_device *dev)
 				dev_priv->pch_type = PCH_LPT;
 				dev_priv->num_pch_pll = 0;
 				DRM_DEBUG_KMS("Found LynxPoint PCH\n");
+			} else if (id == INTEL_PCH_HAS_DEVICE_ID_TYPE) {
+				/* XXX it is important to do this early */
+				dev_priv->is_simulator = true;
+				dev_priv->pch_type = PCH_CPT;
+				dev_priv->num_pch_pll = 2;
+				DRM_DEBUG_KMS("Found HAS PCH\n");
 			}
 			BUG_ON(dev_priv->num_pch_pll > I915_NUM_PLLS);
 		}
-- 
1.7.11.2

Re: [PATCH 2/2] drm/i915: Add simulator's host bridge

[Chris Wilson]

On Fri, 20 Jul 2012 10:43:30 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> Add the host bridge ID used by the simulator. This was added in a
> previous patch for the agp layer, but wasn't preserved here.  It also
> gives us an opportunity to let the rest of the driver know we're running
> as the simulator for various workarounds.

I like how minimal this looks, though it does worry me that is a little
too easy... (Not least the question why the simulator doesn't work with
forcewake... Doesn't that strike you as odd that we have rc6 bugs and
the simulator dies... ;-)

I think I would rather see this as an intel_info so that it can be
expanded upon simply for future/past simulators.

However, the fundamental question is do we ever ship simulators? If we
try to avoid carrying pre-production w/a, shouldn't that mean that we
don't even contemplate pushing simulator interfaces.

Having said, carrying these patches centrally does mean that will be
reviewed and kept in mind for future iterations.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: Add simulator's host bridge

[Ben Widawsky]

On Sat, 21 Jul 2012 10:42:13 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Fri, 20 Jul 2012 10:43:30 -0700, Ben Widawsky <ben@bwidawsk.net>
> wrote:
> > Add the host bridge ID used by the simulator. This was added in a
> > previous patch for the agp layer, but wasn't preserved here.  It
> > also gives us an opportunity to let the rest of the driver know
> > we're running as the simulator for various workarounds.
> 
> I like how minimal this looks, though it does worry me that is a
> little too easy... (Not least the question why the simulator doesn't
> work with forcewake... Doesn't that strike you as odd that we have
> rc6 bugs and the simulator dies... ;-)

At some point I should go through the simulator code... Actually it's a
bit complicated, but the forcewake handshake doesn't normally even make
it to the simulator. I'll follow up with you on irc if you care
further than that. An added bonus though is register accesses are quite
slow, and removing these is in itself quite beneficial (not measured).

> 
> I think I would rather see this as an intel_info so that it can be
> expanded upon simply for future/past simulators.

I tried to go this path. The immediate hack, and problem is I can't
detect the simulator until we probe the host bridge. All the INTEL_INFO
stuff is const. I tried INTEL_INFO(dev)->is_simulator=true. Now the
issue with setting up real structures for simulators... Instead I
propose we want to treat the simulator transparently as much as
possible. The only entirely unavoidable bit I've found so far is the
host bridge PCI id, and in fact it was the case at least until
forcewake on Haswell. I'd like to model the simulator as a quirky
platform instead of a quirky GPU. If we go the INTEL_INFO path we end
up with a simulator variant of each generation (analogous to
is_mobile/desktop) which I see as just superflous typing.

> 
> However, the fundamental question is do we ever ship simulators? If we
> try to avoid carrying pre-production w/a, shouldn't that mean that we
> don't even contemplate pushing simulator interfaces.

I am certain nobody would say we will *never* ship a simulator. We have
internal customers though that benefit from this.

> 
> Having said, carrying these patches centrally does mean that will be
> reviewed and kept in mind for future iterations.

Yes, more importantly, we can clone a repo, build it, and it will
just run.

> -Chris
> 

Thank you for reviewing.


-- 
Ben Widawsky, Intel Open Source Technology Center