[PATCH 00/12] Misc locking fixes and GEM debugging

[PATCH 00/12] Misc locking fixes and GEM debugging

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

A collection of patches addressing locking inversions, missing
locking, un-needed atomic waiting, more precision for the latter
and adding some infrastructure to catch some of these during
driver development.

Tvrtko Ursulin (12):
  drm/i915: Add wait_for_us
  drm/i915: Do not wait atomically for display clocks
  drm/i915/guc: Do not wait for firmware load atomically
  drm/i915/lrc: Do not wait atomically when stopping engines
  drm/i915: Kconfig for extra driver debugging
  drm/i915: Do not lie about atomic wait granularity
  drm/i915: GEM operations need to be done under the big lock
  drm/i915: Fix struct mutex vs. RPS lock inversion
  drm/i915/ilk: Move register read under spinlock
  drm/i915: Introduce dedicated object VMA iterator
  drm/i915: Introduce dedicated safe object VMA iterator
  drm/i915: Add BKL asserts to get page helpers

 drivers/gpu/drm/i915/Kconfig             |  6 ++++++
 drivers/gpu/drm/i915/Kconfig.debug       | 12 +++++++++++
 drivers/gpu/drm/i915/i915_debugfs.c      |  8 +++----
 drivers/gpu/drm/i915/i915_drv.h          | 24 +++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem.c          | 30 +++++++++++++-------------
 drivers/gpu/drm/i915/i915_gem_gtt.c      |  2 +-
 drivers/gpu/drm/i915/i915_gem_shrinker.c |  5 ++---
 drivers/gpu/drm/i915/i915_gem_stolen.c   |  3 +++
 drivers/gpu/drm/i915/i915_gem_userptr.c  |  2 +-
 drivers/gpu/drm/i915/intel_display.c     | 20 +++++++++---------
 drivers/gpu/drm/i915/intel_dp.c          |  3 +--
 drivers/gpu/drm/i915/intel_drv.h         | 36 +++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_guc_loader.c  |  6 +++---
 drivers/gpu/drm/i915/intel_lrc.c         |  2 +-
 drivers/gpu/drm/i915/intel_pm.c          | 15 +++++++------
 drivers/gpu/drm/i915/intel_psr.c         |  2 +-
 16 files changed, 122 insertions(+), 54 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/Kconfig.debug

-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 01/12] drm/i915: Add wait_for_us

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

This is for callers who want micro-second precision but are not
waiting from the atomic context.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 3 +--
 drivers/gpu/drm/i915/intel_drv.h | 9 +++++----
 drivers/gpu/drm/i915/intel_psr.c | 2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e2bea710614f..fb8a76ec6ade 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1788,11 +1788,10 @@ static void wait_panel_status(struct intel_dp *intel_dp,
 			I915_READ(pp_stat_reg),
 			I915_READ(pp_ctrl_reg));
 
-	if (_wait_for((I915_READ(pp_stat_reg) & mask) == value, 5000, 10)) {
+	if (_wait_for((I915_READ(pp_stat_reg) & mask) == value, 5000000, 10000))
 		DRM_ERROR("Panel status timeout: status %08x control %08x\n",
 				I915_READ(pp_stat_reg),
 				I915_READ(pp_ctrl_reg));
-	}
 
 	DRM_DEBUG_KMS("Wait complete\n");
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f620023ed134..779d17a9fcce 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -45,8 +45,8 @@
  * having timed out, since the timeout could be due to preemption or similar and
  * we've never had a chance to check the condition before the timeout.
  */
-#define _wait_for(COND, MS, W) ({ \
-	unsigned long timeout__ = jiffies + msecs_to_jiffies(MS) + 1;	\
+#define _wait_for(COND, US, W) ({ \
+	unsigned long timeout__ = jiffies + usecs_to_jiffies(US) + 1;	\
 	int ret__ = 0;							\
 	while (!(COND)) {						\
 		if (time_after(jiffies, timeout__)) {			\
@@ -55,7 +55,7 @@
 			break;						\
 		}							\
 		if ((W) && drm_can_sleep()) {				\
-			usleep_range((W)*1000, (W)*2000);		\
+			usleep_range((W), (W)*2);			\
 		} else {						\
 			cpu_relax();					\
 		}							\
@@ -63,7 +63,8 @@
 	ret__;								\
 })
 
-#define wait_for(COND, MS) _wait_for(COND, MS, 1)
+#define wait_for(COND, MS) _wait_for(COND, (MS) * 1000, 1000)
+#define wait_for_us(COND, US) _wait_for(COND, US, 1)
 #define wait_for_atomic(COND, MS) _wait_for(COND, MS, 0)
 #define wait_for_atomic_us(COND, US) _wait_for((COND), \
 					       DIV_ROUND_UP((US), 1000), 0)
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 9ccff3011523..e12377963839 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -492,7 +492,7 @@ static void hsw_psr_disable(struct intel_dp *intel_dp)
 
 		/* Wait till PSR is idle */
 		if (_wait_for((I915_READ(EDP_PSR_STATUS_CTL) &
-			       EDP_PSR_STATUS_STATE_MASK) == 0, 2000, 10))
+			       EDP_PSR_STATUS_STATE_MASK) == 0, 2000000, 10000))
 			DRM_ERROR("Timed out waiting for PSR Idle State\n");
 
 		dev_priv->psr.active = false;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 02/12] drm/i915: Do not wait atomically for display clocks

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Looks like this code does not need to wait atomically since it
otherwise takes the mutex.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 304fc9637026..a7530cf612d7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9753,8 +9753,8 @@ static void broadwell_set_cdclk(struct drm_device *dev, int cdclk)
 	val |= LCPLL_CD_SOURCE_FCLK;
 	I915_WRITE(LCPLL_CTL, val);
 
-	if (wait_for_atomic_us(I915_READ(LCPLL_CTL) &
-			       LCPLL_CD_SOURCE_FCLK_DONE, 1))
+	if (wait_for_us(I915_READ(LCPLL_CTL) &
+			LCPLL_CD_SOURCE_FCLK_DONE, 1))
 		DRM_ERROR("Switching to FCLK failed\n");
 
 	val = I915_READ(LCPLL_CTL);
@@ -9788,8 +9788,8 @@ static void broadwell_set_cdclk(struct drm_device *dev, int cdclk)
 	val &= ~LCPLL_CD_SOURCE_FCLK;
 	I915_WRITE(LCPLL_CTL, val);
 
-	if (wait_for_atomic_us((I915_READ(LCPLL_CTL) &
-				LCPLL_CD_SOURCE_FCLK_DONE) == 0, 1))
+	if (wait_for_us((I915_READ(LCPLL_CTL) &
+			LCPLL_CD_SOURCE_FCLK_DONE) == 0, 1))
 		DRM_ERROR("Switching back to LCPLL failed\n");
 
 	mutex_lock(&dev_priv->rps.hw_lock);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 03/12] drm/i915/guc: Do not wait for firmware load atomically

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

It does not look like this code needs to wait atomically?

Higher in the call chain it calls the GEM API and I do
not see that the section is under any spin locks or such.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Alex Dai <yu.dai@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_loader.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 3accd914490f..82a3c03fbc0e 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -199,7 +199,7 @@ static void set_guc_init_params(struct drm_i915_private *dev_priv)
  * the value matches either of two values representing completion
  * of the GuC boot process.
  *
- * This is used for polling the GuC status in a wait_for_atomic()
+ * This is used for polling the GuC status in a wait_for()
  * loop below.
  */
 static inline bool guc_ucode_response(struct drm_i915_private *dev_priv,
@@ -259,14 +259,14 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv)
 	I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(UOS_MOVE | START_DMA));
 
 	/*
-	 * Spin-wait for the DMA to complete & the GuC to start up.
+	 * Wait for the DMA to complete & the GuC to start up.
 	 * NB: Docs recommend not using the interrupt for completion.
 	 * Measurements indicate this should take no more than 20ms, so a
 	 * timeout here indicates that the GuC has failed and is unusable.
 	 * (Higher levels of the driver will attempt to fall back to
 	 * execlist mode if this happens.)
 	 */
-	ret = wait_for_atomic(guc_ucode_response(dev_priv, &status), 100);
+	ret = wait_for(guc_ucode_response(dev_priv, &status), 100);
 
 	DRM_DEBUG_DRIVER("DMA status 0x%x, GuC status 0x%x\n",
 			I915_READ(DMA_CTRL), status);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 04/12] drm/i915/lrc: Do not wait atomically when stopping engines

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

I do not see that this needs to be done atomically and up to
one second is quite a long time to busy loop.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 3a03646e343d..3ca7f48a418b 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1049,7 +1049,7 @@ void intel_logical_ring_stop(struct intel_engine_cs *ring)
 
 	/* TODO: Is this correct with Execlists enabled? */
 	I915_WRITE_MODE(ring, _MASKED_BIT_ENABLE(STOP_RING));
-	if (wait_for_atomic((I915_READ_MODE(ring) & MODE_IDLE) != 0, 1000)) {
+	if (wait_for((I915_READ_MODE(ring) & MODE_IDLE) != 0, 1000)) {
 		DRM_ERROR("%s :timed out trying to stop ring\n", ring->name);
 		return;
 	}
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 05/12] drm/i915: Kconfig for extra driver debugging

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

v2: Added a submenu based on an idea by Chris Wilson.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/Kconfig       |  6 ++++++
 drivers/gpu/drm/i915/Kconfig.debug | 12 ++++++++++++
 2 files changed, 18 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/Kconfig.debug

diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index 051eab33e4c7..98a178565492 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -47,3 +47,9 @@ config DRM_I915_PRELIMINARY_HW_SUPPORT
 	  option changes the default for that module option.
 
 	  If in doubt, say "N".
+
+menu "drm/i915 Debugging"
+depends on DRM_I915
+depends on EXPERT
+source drivers/gpu/drm/i915/Kconfig.debug
+endmenu
diff --git a/drivers/gpu/drm/i915/Kconfig.debug b/drivers/gpu/drm/i915/Kconfig.debug
new file mode 100644
index 000000000000..649a562ddf17
--- /dev/null
+++ b/drivers/gpu/drm/i915/Kconfig.debug
@@ -0,0 +1,12 @@
+config DRM_I915_DEBUG
+        bool "Enable additional driver debugging"
+        depends on DRM_I915
+        default n
+        help
+          Choose this option to turn on extra driver debugging that may affect
+          performance but will catch some internal issues.
+
+          Recommended for driver developers only.
+
+          If in doubt, say "N".
+
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 06/12] drm/i915: Do not lie about atomic wait granularity

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Currently the wait_for_atomic_us only allows for a millisecond
granularity which is not nice towards callers requesting small
micro-second waits.

Re-implement it so micro-second granularity is really supported
and not just in the name of the macro.

v2:
  * Warn when used from non-atomic context (if possible).
  * Warn on too long atomic waits.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_drv.h | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 779d17a9fcce..af6c1539106f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -63,11 +63,32 @@
 	ret__;								\
 })
 
+#if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT)
+  #define _WAIT_FOR_ATOMIC_CHECK WARN_ON_ONCE(!in_atomic())
+#else
+  #define _WAIT_FOR_ATOMIC_CHECK do { } while(0)
+#endif
+
+#define _wait_for_atomic(COND, US) ({ \
+	unsigned long end__; \
+	int ret__ = 0; \
+	_WAIT_FOR_ATOMIC_CHECK; \
+	BUILD_BUG_ON((US) > 50000); \
+	end__ = (local_clock() >> 10) + (US) + 1; \
+	while (!(COND)) { \
+		if (time_after((unsigned long)(local_clock() >> 10), end__)) { \
+			ret__ = -ETIMEDOUT; \
+			break; \
+		} \
+		cpu_relax(); \
+	} \
+	ret__; \
+})
+
 #define wait_for(COND, MS) _wait_for(COND, (MS) * 1000, 1000)
 #define wait_for_us(COND, US) _wait_for(COND, US, 1)
-#define wait_for_atomic(COND, MS) _wait_for(COND, MS, 0)
-#define wait_for_atomic_us(COND, US) _wait_for((COND), \
-					       DIV_ROUND_UP((US), 1000), 0)
+#define wait_for_atomic(COND, MS) _wait_for_atomic(COND, (MS) * 1000)
+#define wait_for_atomic_us(COND, US) _wait_for_atomic((COND), (US))
 
 #define KHz(x) (1000 * (x))
 #define MHz(x) KHz(1000 * (x))
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 07/12] drm/i915: GEM operations need to be done under the big lock

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

VMA creation and GEM list management need the big lock.

v2:

Mutex unlock ended on the wrong path somehow. (0-day, Julia Lawall)

Not to mention drm_gem_object_unreference was there in existing
code with no mutex held.

v3:

Some callers of i915_gem_object_create_stolen_for_preallocated
already hold the lock so move the mutex into the other caller
as well.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 3 +++
 drivers/gpu/drm/i915/intel_display.c   | 8 ++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index c384dc9c8a63..a97a5e762c0f 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -635,6 +635,9 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
 		return NULL;
 
+	if (WARN_ON_ONCE(!mutex_is_locked(&dev->struct_mutex)))
+		return NULL;
+
 	DRM_DEBUG_KMS("creating preallocated stolen object: stolen_offset=%x, gtt_offset=%x, size=%x\n",
 			stolen_offset, gtt_offset, size);
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a7530cf612d7..5018295cd92b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2551,12 +2551,16 @@ intel_alloc_initial_plane_obj(struct intel_crtc *crtc,
 	if (size_aligned * 2 > dev_priv->gtt.stolen_usable_size)
 		return false;
 
+	mutex_lock(&dev->struct_mutex);
+
 	obj = i915_gem_object_create_stolen_for_preallocated(dev,
 							     base_aligned,
 							     base_aligned,
 							     size_aligned);
-	if (!obj)
+	if (!obj) {
+		mutex_unlock(&dev->struct_mutex);
 		return false;
+	}
 
 	obj->tiling_mode = plane_config->tiling;
 	if (obj->tiling_mode == I915_TILING_X)
@@ -2569,12 +2573,12 @@ intel_alloc_initial_plane_obj(struct intel_crtc *crtc,
 	mode_cmd.modifier[0] = fb->modifier[0];
 	mode_cmd.flags = DRM_MODE_FB_MODIFIERS;
 
-	mutex_lock(&dev->struct_mutex);
 	if (intel_framebuffer_init(dev, to_intel_framebuffer(fb),
 				   &mode_cmd, obj)) {
 		DRM_DEBUG_KMS("intel fb init failed\n");
 		goto out_unref_obj;
 	}
+
 	mutex_unlock(&dev->struct_mutex);
 
 	DRM_DEBUG_KMS("initial plane fb obj %p\n", obj);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/12] drm/i915: Fix struct mutex vs. RPS lock inversion

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

RPS lock must be taken before the struct_mutex to avoid
locking inversion. So stop grabbing it for the whole
powersave initialization and instead only take it during
the sections which need it.

Also, struct_mutex is not needed any more since dedicated
RPS lock was added in:

   commit 4fc688ce79772496503d22263d61b071a8fb596e
   Author: Jesse Barnes <jbarnes@virtuousgeek.org>
   Date:   Fri Nov 2 11:14:01 2012 -0700

       drm/i915: protect RPS/RC6 related accesses (including PCU) with a new mutex

Based on prototype patch by Chris Wilson and a subsequent
mailing list discussion involving Ville, Imre, Chris and
Daniel.

v2: More details in the commit.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 4 ----
 drivers/gpu/drm/i915/intel_pm.c      | 9 +++++----
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5018295cd92b..af0d33a3697a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15995,9 +15995,7 @@ void intel_modeset_gem_init(struct drm_device *dev)
 	struct drm_i915_gem_object *obj;
 	int ret;
 
-	mutex_lock(&dev->struct_mutex);
 	intel_init_gt_powersave(dev);
-	mutex_unlock(&dev->struct_mutex);
 
 	intel_modeset_init_hw(dev);
 
@@ -16077,9 +16075,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
 
 	intel_cleanup_overlay(dev);
 
-	mutex_lock(&dev->struct_mutex);
 	intel_cleanup_gt_powersave(dev);
-	mutex_unlock(&dev->struct_mutex);
 
 	intel_teardown_gmbus(dev);
 }
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 31bc4ea395ac..7f137a70b6ae 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5175,8 +5175,6 @@ static void cherryview_setup_pctx(struct drm_device *dev)
 	u32 pcbr;
 	int pctx_size = 32*1024;
 
-	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
-
 	pcbr = I915_READ(VLV_PCBR);
 	if ((pcbr >> VLV_PCBR_ADDR_SHIFT) == 0) {
 		DRM_DEBUG_DRIVER("BIOS didn't set up PCBR, fixing up\n");
@@ -5198,7 +5196,7 @@ static void valleyview_setup_pctx(struct drm_device *dev)
 	u32 pcbr;
 	int pctx_size = 24*1024;
 
-	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+	mutex_lock(&dev->struct_mutex);
 
 	pcbr = I915_READ(VLV_PCBR);
 	if (pcbr) {
@@ -5226,7 +5224,7 @@ static void valleyview_setup_pctx(struct drm_device *dev)
 	pctx = i915_gem_object_create_stolen(dev, pctx_size);
 	if (!pctx) {
 		DRM_DEBUG("not enough stolen space for PCTX, disabling\n");
-		return;
+		goto out;
 	}
 
 	pctx_paddr = dev_priv->mm.stolen_base + pctx->stolen->start;
@@ -5235,6 +5233,7 @@ static void valleyview_setup_pctx(struct drm_device *dev)
 out:
 	DRM_DEBUG_DRIVER("PCBR: 0x%08x\n", I915_READ(VLV_PCBR));
 	dev_priv->vlv_pctx = pctx;
+	mutex_unlock(&dev->struct_mutex);
 }
 
 static void valleyview_cleanup_pctx(struct drm_device *dev)
@@ -5244,8 +5243,10 @@ static void valleyview_cleanup_pctx(struct drm_device *dev)
 	if (WARN_ON(!dev_priv->vlv_pctx))
 		return;
 
+	mutex_lock(&dev->struct_mutex);
 	drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
 	dev_priv->vlv_pctx = NULL;
+	mutex_unlock(&dev->struct_mutex);
 }
 
 static void valleyview_init_gt_powersave(struct drm_device *dev)
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 09/12] drm/i915/ilk: Move register read under spinlock

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Code does read-modify-write but the read was outside the lock.

It is fine since the caller holds struct mutex, but if we
correct this we open up the opportunity for decreasing the
mutex duration time since the call to ironlake_enable_drps
does not need it any longer since it is covered by the
mchdev_lock lock.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_pm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 7f137a70b6ae..78271d17caa0 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4112,11 +4112,13 @@ bool ironlake_set_drps(struct drm_device *dev, u8 val)
 static void ironlake_enable_drps(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 rgvmodectl = I915_READ(MEMMODECTL);
+	u32 rgvmodectl;
 	u8 fmax, fmin, fstart, vstart;
 
 	spin_lock_irq(&mchdev_lock);
 
+	rgvmodectl = I915_READ(MEMMODECTL);
+
 	/* Enable temp reporting */
 	I915_WRITE16(PMMISC, I915_READ(PMMISC) | MCPPCE_EN);
 	I915_WRITE16(TSC1, I915_READ(TSC1) | TSE);
@@ -6189,8 +6191,8 @@ void intel_enable_gt_powersave(struct drm_device *dev)
 		return;
 
 	if (IS_IRONLAKE_M(dev)) {
-		mutex_lock(&dev->struct_mutex);
 		ironlake_enable_drps(dev);
+		mutex_lock(&dev->struct_mutex);
 		intel_init_emon(dev);
 		mutex_unlock(&dev->struct_mutex);
 	} else if (INTEL_INFO(dev)->gen >= 6) {
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/12] drm/i915: Introduce dedicated object VMA iterator

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Purpose is to catch places which iterate the object VMA list
without holding the big lock.

Implemented by open coding list_for_each_entry to make the
macro compatible with existing call sites.

v2: Error capture runs without the mutex so iterate directly from there.
v3: Replace WARN_ON with lockdep_assert_held. (Chris Wilson, Daniel Vetter)
v4: Moved under dedicated CONFIG_DRM_I915_DEBUG and back to WARN_ON.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_debugfs.c      |  8 ++++----
 drivers/gpu/drm/i915/i915_drv.h          | 11 +++++++++++
 drivers/gpu/drm/i915/i915_gem.c          | 24 ++++++++++++------------
 drivers/gpu/drm/i915/i915_gem_gtt.c      |  2 +-
 drivers/gpu/drm/i915/i915_gem_shrinker.c |  2 +-
 5 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 863012a2602e..ff444f09ea98 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -117,7 +117,7 @@ static u64 i915_gem_obj_total_ggtt_size(struct drm_i915_gem_object *obj)
 	u64 size = 0;
 	struct i915_vma *vma;
 
-	list_for_each_entry(vma, &obj->vma_list, vma_link) {
+	i915_gem_obj_for_each_vma(vma, obj) {
 		if (i915_is_ggtt(vma->vm) &&
 		    drm_mm_node_allocated(&vma->node))
 			size += vma->node.size;
@@ -155,7 +155,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
 		   obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
 	if (obj->base.name)
 		seq_printf(m, " (name: %d)", obj->base.name);
-	list_for_each_entry(vma, &obj->vma_list, vma_link) {
+	i915_gem_obj_for_each_vma(vma, obj) {
 		if (vma->pin_count > 0)
 			pin_count++;
 	}
@@ -164,7 +164,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
 		seq_printf(m, " (display)");
 	if (obj->fence_reg != I915_FENCE_REG_NONE)
 		seq_printf(m, " (fence: %d)", obj->fence_reg);
-	list_for_each_entry(vma, &obj->vma_list, vma_link) {
+	i915_gem_obj_for_each_vma(vma, obj) {
 		seq_printf(m, " (%sgtt offset: %08llx, size: %08llx",
 			   i915_is_ggtt(vma->vm) ? "g" : "pp",
 			   vma->node.start, vma->node.size);
@@ -342,7 +342,7 @@ static int per_file_stats(int id, void *ptr, void *data)
 		stats->shared += obj->base.size;
 
 	if (USES_FULL_PPGTT(obj->base.dev)) {
-		list_for_each_entry(vma, &obj->vma_list, vma_link) {
+		i915_gem_obj_for_each_vma(vma, obj) {
 			struct i915_hw_ppgtt *ppgtt;
 
 			if (!drm_mm_node_allocated(&vma->node))
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 905e90f25957..05ef750386df 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2861,6 +2861,17 @@ struct drm_i915_gem_object *i915_gem_object_create_from_data(
 void i915_gem_free_object(struct drm_gem_object *obj);
 void i915_gem_vma_destroy(struct i915_vma *vma);
 
+#ifdef CONFIG_DRM_I915_DEBUG
+  #define i915_gem_obj_for_each_vma(vma, obj) \
+	for (WARN_ON_ONCE(!mutex_is_locked(&(obj)->base.dev->struct_mutex)), \
+	     vma = list_first_entry(&(obj)->vma_list, typeof(*vma), vma_link);\
+	     &vma->vma_link != (&(obj)->vma_list); \
+	     vma = list_next_entry(vma, vma_link))
+#else
+  #define i915_gem_obj_for_each_vma(vma, obj) \
+		list_for_each_entry((vma), &(obj)->vma_list, vma_link)
+#endif
+
 /* Flags used by pin/bind&friends. */
 #define PIN_MAPPABLE	(1<<0)
 #define PIN_NONBLOCK	(1<<1)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c558887b2084..ce9d0544b42c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2454,7 +2454,7 @@ i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring)
 	list_move_tail(&obj->global_list,
 		       &to_i915(obj->base.dev)->mm.bound_list);
 
-	list_for_each_entry(vma, &obj->vma_list, vma_link) {
+	i915_gem_obj_for_each_vma(vma, obj) {
 		if (!list_empty(&vma->mm_list))
 			list_move_tail(&vma->mm_list, &vma->vm->inactive_list);
 	}
@@ -3873,7 +3873,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 			 */
 		}
 
-		list_for_each_entry(vma, &obj->vma_list, vma_link) {
+		i915_gem_obj_for_each_vma(vma, obj) {
 			if (!drm_mm_node_allocated(&vma->node))
 				continue;
 
@@ -3883,7 +3883,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 		}
 	}
 
-	list_for_each_entry(vma, &obj->vma_list, vma_link)
+	i915_gem_obj_for_each_vma(vma, obj)
 		vma->node.color = cache_level;
 	obj->cache_level = cache_level;
 
@@ -4613,7 +4613,7 @@ struct i915_vma *i915_gem_obj_to_vma(struct drm_i915_gem_object *obj,
 				     struct i915_address_space *vm)
 {
 	struct i915_vma *vma;
-	list_for_each_entry(vma, &obj->vma_list, vma_link) {
+	i915_gem_obj_for_each_vma(vma, obj) {
 		if (vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL &&
 		    vma->vm == vm)
 			return vma;
@@ -4630,7 +4630,7 @@ struct i915_vma *i915_gem_obj_to_ggtt_view(struct drm_i915_gem_object *obj,
 	if (WARN_ONCE(!view, "no view specified"))
 		return ERR_PTR(-EINVAL);
 
-	list_for_each_entry(vma, &obj->vma_list, vma_link)
+	i915_gem_obj_for_each_vma(vma, obj)
 		if (vma->vm == ggtt &&
 		    i915_ggtt_view_equal(&vma->ggtt_view, view))
 			return vma;
@@ -5201,7 +5201,7 @@ u64 i915_gem_obj_offset(struct drm_i915_gem_object *o,
 
 	WARN_ON(vm == &dev_priv->mm.aliasing_ppgtt->base);
 
-	list_for_each_entry(vma, &o->vma_list, vma_link) {
+	i915_gem_obj_for_each_vma(vma, o) {
 		if (i915_is_ggtt(vma->vm) &&
 		    vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
 			continue;
@@ -5220,7 +5220,7 @@ u64 i915_gem_obj_ggtt_offset_view(struct drm_i915_gem_object *o,
 	struct i915_address_space *ggtt = i915_obj_to_ggtt(o);
 	struct i915_vma *vma;
 
-	list_for_each_entry(vma, &o->vma_list, vma_link)
+	i915_gem_obj_for_each_vma(vma, o)
 		if (vma->vm == ggtt &&
 		    i915_ggtt_view_equal(&vma->ggtt_view, view))
 			return vma->node.start;
@@ -5234,7 +5234,7 @@ bool i915_gem_obj_bound(struct drm_i915_gem_object *o,
 {
 	struct i915_vma *vma;
 
-	list_for_each_entry(vma, &o->vma_list, vma_link) {
+	i915_gem_obj_for_each_vma(vma, o) {
 		if (i915_is_ggtt(vma->vm) &&
 		    vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
 			continue;
@@ -5251,7 +5251,7 @@ bool i915_gem_obj_ggtt_bound_view(struct drm_i915_gem_object *o,
 	struct i915_address_space *ggtt = i915_obj_to_ggtt(o);
 	struct i915_vma *vma;
 
-	list_for_each_entry(vma, &o->vma_list, vma_link)
+	i915_gem_obj_for_each_vma(vma, o)
 		if (vma->vm == ggtt &&
 		    i915_ggtt_view_equal(&vma->ggtt_view, view) &&
 		    drm_mm_node_allocated(&vma->node))
@@ -5264,7 +5264,7 @@ bool i915_gem_obj_bound_any(struct drm_i915_gem_object *o)
 {
 	struct i915_vma *vma;
 
-	list_for_each_entry(vma, &o->vma_list, vma_link)
+	i915_gem_obj_for_each_vma(vma, o)
 		if (drm_mm_node_allocated(&vma->node))
 			return true;
 
@@ -5281,7 +5281,7 @@ unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
 
 	BUG_ON(list_empty(&o->vma_list));
 
-	list_for_each_entry(vma, &o->vma_list, vma_link) {
+	i915_gem_obj_for_each_vma(vma, o) {
 		if (i915_is_ggtt(vma->vm) &&
 		    vma->ggtt_view.type != I915_GGTT_VIEW_NORMAL)
 			continue;
@@ -5294,7 +5294,7 @@ unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
 bool i915_gem_obj_is_pinned(struct drm_i915_gem_object *obj)
 {
 	struct i915_vma *vma;
-	list_for_each_entry(vma, &obj->vma_list, vma_link)
+	i915_gem_obj_for_each_vma(vma, obj)
 		if (vma->pin_count > 0)
 			return true;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 715a771f0b31..ab00b2a3c035 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3237,7 +3237,7 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
 	vm = &dev_priv->gtt.base;
 	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
 		flush = false;
-		list_for_each_entry(vma, &obj->vma_list, vma_link) {
+		i915_gem_obj_for_each_vma(vma, obj) {
 			if (vma->vm != vm)
 				continue;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index 58c1e592bbdb..4c89bd45dbde 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -52,7 +52,7 @@ static int num_vma_bound(struct drm_i915_gem_object *obj)
 	struct i915_vma *vma;
 	int count = 0;
 
-	list_for_each_entry(vma, &obj->vma_list, vma_link) {
+	i915_gem_obj_for_each_vma(vma, obj) {
 		if (drm_mm_node_allocated(&vma->node))
 			count++;
 		if (vma->pin_count)
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 11/12] drm/i915: Introduce dedicated safe object VMA iterator

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Purpose is to catch places which iterate the object VMA list
without holding the big lock.

Implemented by open coding list_for_each_entry_safe to make
the macro compatible with existing call sites.

v2: Replace WARN_ON with lockdep_assert_held. (Chris Wilson, Daniel Vetter)
v3: Moved under dedicated CONFIG_DRM_I915_DEBUG and back to WARN_ON.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h          | 10 ++++++++++
 drivers/gpu/drm/i915/i915_gem.c          |  6 +++---
 drivers/gpu/drm/i915/i915_gem_shrinker.c |  3 +--
 drivers/gpu/drm/i915/i915_gem_userptr.c  |  2 +-
 4 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 05ef750386df..4ad025210416 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2867,9 +2867,19 @@ void i915_gem_vma_destroy(struct i915_vma *vma);
 	     vma = list_first_entry(&(obj)->vma_list, typeof(*vma), vma_link);\
 	     &vma->vma_link != (&(obj)->vma_list); \
 	     vma = list_next_entry(vma, vma_link))
+
+  #define i915_gem_obj_for_each_vma_safe(vma, next, obj) \
+	for (WARN_ON_ONCE(!mutex_is_locked(&(obj)->base.dev->struct_mutex)), \
+	     vma = list_first_entry(&(obj)->vma_list, typeof(*vma), vma_link), \
+	     next = list_next_entry(vma, vma_link); \
+	     &vma->vma_link != (&(obj)->vma_list); \
+	     vma = next, next = list_next_entry(next, vma_link))
 #else
   #define i915_gem_obj_for_each_vma(vma, obj) \
 		list_for_each_entry((vma), &(obj)->vma_list, vma_link)
+
+  #define i915_gem_obj_for_each_vma_safe(vma, next, obj) \
+	    list_for_each_entry_safe((vma), (next), &(obj)->vma_list, vma_link)
 #endif
 
 /* Flags used by pin/bind&friends. */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ce9d0544b42c..d75062d7aa6a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -272,7 +272,7 @@ drop_pages(struct drm_i915_gem_object *obj)
 	int ret;
 
 	drm_gem_object_reference(&obj->base);
-	list_for_each_entry_safe(vma, next, &obj->vma_list, vma_link)
+	i915_gem_obj_for_each_vma_safe(vma, next, obj)
 		if (i915_vma_unbind(vma))
 			break;
 
@@ -3810,7 +3810,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj,
 	 * catch the issue of the CS prefetch crossing page boundaries and
 	 * reading an invalid PTE on older architectures.
 	 */
-	list_for_each_entry_safe(vma, next, &obj->vma_list, vma_link) {
+	i915_gem_obj_for_each_vma_safe(vma, next, obj) {
 		if (!drm_mm_node_allocated(&vma->node))
 			continue;
 
@@ -4556,7 +4556,7 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
 
 	trace_i915_gem_object_destroy(obj);
 
-	list_for_each_entry_safe(vma, next, &obj->vma_list, vma_link) {
+	i915_gem_obj_for_each_vma_safe(vma, next, obj) {
 		int ret;
 
 		vma->pin_count = 0;
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index 4c89bd45dbde..ae13a9f636fa 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -175,8 +175,7 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
 			drm_gem_object_reference(&obj->base);
 
 			/* For the unbound phase, this should be a no-op! */
-			list_for_each_entry_safe(vma, v,
-						 &obj->vma_list, vma_link)
+			i915_gem_obj_for_each_vma_safe(vma, v, obj)
 				if (i915_vma_unbind(vma))
 					break;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 74a4d1714879..a2430b8c4aba 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -78,7 +78,7 @@ static void cancel_userptr(struct work_struct *work)
 		was_interruptible = dev_priv->mm.interruptible;
 		dev_priv->mm.interruptible = false;
 
-		list_for_each_entry_safe(vma, tmp, &obj->vma_list, vma_link) {
+		i915_gem_obj_for_each_vma_safe(vma, tmp, obj) {
 			int ret = i915_vma_unbind(vma);
 			WARN_ON(ret && ret != -EIO);
 		}
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 12/12] drm/i915: Add BKL asserts to get page helpers

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Purpose is catching illegal callers.

v2: Replace WARN_ON with lockdep_assert_held. (Chris Wilson, Daniel Vetter)
v3: Moved under dedicated CONFIG_DRM_I915_DEBUG and back to WARN_ON.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4ad025210416..6d34f74a5919 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2933,6 +2933,9 @@ i915_gem_object_get_dirty_page(struct drm_i915_gem_object *obj, int n);
 static inline struct page *
 i915_gem_object_get_page(struct drm_i915_gem_object *obj, int n)
 {
+#ifdef CONFIG_DRM_I915_DEBUG
+	WARN_ON_ONCE(!mutex_is_locked(&obj->base.dev->struct_mutex));
+#endif
 	if (WARN_ON(n >= obj->base.size >> PAGE_SHIFT))
 		return NULL;
 
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for Misc locking fixes and GEM debugging

[Patchwork]

== Summary ==

Series 3006v1 Misc locking fixes and GEM debugging
http://patchwork.freedesktop.org/api/1.0/series/3006/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-b-frame-sequence:
                dmesg-warn -> PASS       (skl-i5k-2)
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> PASS       (byt-nuc)

bdw-nuci7        total:156  pass:147  dwarn:0   dfail:0   fail:0   skip:9  
bdw-ultra        total:159  pass:147  dwarn:0   dfail:0   fail:0   skip:12 
bsw-nuc-2        total:159  pass:129  dwarn:0   dfail:0   fail:0   skip:30 
byt-nuc          total:159  pass:136  dwarn:0   dfail:0   fail:0   skip:23 
hsw-brixbox      total:159  pass:146  dwarn:0   dfail:0   fail:0   skip:13 
hsw-gt2          total:159  pass:149  dwarn:0   dfail:0   fail:0   skip:10 
hsw-xps12        total:156  pass:146  dwarn:0   dfail:0   fail:0   skip:10 
ilk-hp8440p      total:159  pass:111  dwarn:0   dfail:0   fail:0   skip:48 
ivb-t430s        total:159  pass:145  dwarn:0   dfail:0   fail:0   skip:14 
skl-i5k-2        total:159  pass:144  dwarn:1   dfail:0   fail:0   skip:14 
snb-dellxps      total:159  pass:137  dwarn:0   dfail:0   fail:0   skip:22 
snb-x220t        total:159  pass:137  dwarn:0   dfail:0   fail:1   skip:21 

Results at /archive/results/CI_IGT_test/Patchwork_1340/

5d3deb0902a962218ad9b0e583e4d1bbdec29f9a drm-intel-nightly: 2016y-02m-01d-20h-05m-03s UTC integration manifest
bb47195bb06eab046c89691d0a527ea47d5d8e3d drm/i915: Add BKL asserts to get page helpers
c828ee3e795e318601e4b773b376a90e8f9fff30 drm/i915: Introduce dedicated safe object VMA iterator
e2a01dfe5c15750e306f99c7a7eb74a3ede4e4a1 drm/i915: Introduce dedicated object VMA iterator
cee676163abdbeae5b22ac87e99a3640c57ed717 drm/i915/ilk: Move register read under spinlock
a94d4b0b39db5fb4eeadeaeb953f8bc76acece31 drm/i915: Fix struct mutex vs. RPS lock inversion
fb1d051d6427b5536a2ad7022181dd5f87ef3f8e drm/i915: GEM operations need to be done under the big lock
3bf2524d60496461a3918c283f9f473f6330197d drm/i915: Do not lie about atomic wait granularity
e9c67ede8f4a9063b6bf350f0084aa5d3fa3f41f drm/i915: Kconfig for extra driver debugging
9f3db6eb03d2ebc1266de2d998f2e194d2b9dd90 drm/i915/lrc: Do not wait atomically when stopping engines
ca58ad5e9250cee3d0e3d1aa52e005bd5950407b drm/i915/guc: Do not wait for firmware load atomically
7de26a409b7911a17c7d2e231313b91a683c49cb drm/i915: Do not wait atomically for display clocks
666b46edb2e312b0afffca09b0d986b44f3b8775 drm/i915: Add wait_for_us

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/12] drm/i915: Introduce dedicated object VMA iterator

[Chris Wilson]

On Tue, Feb 02, 2016 at 11:06:28AM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Purpose is to catch places which iterate the object VMA list
> without holding the big lock.
> 
> Implemented by open coding list_for_each_entry to make the
> macro compatible with existing call sites.
> 
> v2: Error capture runs without the mutex so iterate directly from there.
> v3: Replace WARN_ON with lockdep_assert_held. (Chris Wilson, Daniel Vetter)
> v4: Moved under dedicated CONFIG_DRM_I915_DEBUG and back to WARN_ON.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c      |  8 ++++----
>  drivers/gpu/drm/i915/i915_drv.h          | 11 +++++++++++
>  drivers/gpu/drm/i915/i915_gem.c          | 24 ++++++++++++------------
>  drivers/gpu/drm/i915/i915_gem_gtt.c      |  2 +-
>  drivers/gpu/drm/i915/i915_gem_shrinker.c |  2 +-
>  5 files changed, 29 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 863012a2602e..ff444f09ea98 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -117,7 +117,7 @@ static u64 i915_gem_obj_total_ggtt_size(struct drm_i915_gem_object *obj)
>  	u64 size = 0;
>  	struct i915_vma *vma;
>  
> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> +	i915_gem_obj_for_each_vma(vma, obj) {
>  		if (i915_is_ggtt(vma->vm) &&
>  		    drm_mm_node_allocated(&vma->node))
>  			size += vma->node.size;
> @@ -155,7 +155,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>  		   obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
>  	if (obj->base.name)
>  		seq_printf(m, " (name: %d)", obj->base.name);
> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> +	i915_gem_obj_for_each_vma(vma, obj) {
>  		if (vma->pin_count > 0)
>  			pin_count++;
>  	}
> @@ -164,7 +164,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>  		seq_printf(m, " (display)");
>  	if (obj->fence_reg != I915_FENCE_REG_NONE)
>  		seq_printf(m, " (fence: %d)", obj->fence_reg);
> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> +	i915_gem_obj_for_each_vma(vma, obj) {
>  		seq_printf(m, " (%sgtt offset: %08llx, size: %08llx",
>  			   i915_is_ggtt(vma->vm) ? "g" : "pp",
>  			   vma->node.start, vma->node.size);
> @@ -342,7 +342,7 @@ static int per_file_stats(int id, void *ptr, void *data)
>  		stats->shared += obj->base.size;
>  
>  	if (USES_FULL_PPGTT(obj->base.dev)) {
> -		list_for_each_entry(vma, &obj->vma_list, vma_link) {
> +		i915_gem_obj_for_each_vma(vma, obj) {
>  			struct i915_hw_ppgtt *ppgtt;
>  
>  			if (!drm_mm_node_allocated(&vma->node))
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 905e90f25957..05ef750386df 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2861,6 +2861,17 @@ struct drm_i915_gem_object *i915_gem_object_create_from_data(
>  void i915_gem_free_object(struct drm_gem_object *obj);
>  void i915_gem_vma_destroy(struct i915_vma *vma);
>  
> +#ifdef CONFIG_DRM_I915_DEBUG
> +  #define i915_gem_obj_for_each_vma(vma, obj) \

This is the wrong name, i915_gem_object is the object on which we are
iterating over, so i915_gem_object_.

> +	for (WARN_ON_ONCE(!mutex_is_locked(&(obj)->base.dev->struct_mutex)), \

I disagree that we should be doing i915-specific lockdep debugging and
*ignoring* the core facilities. If we locked at RCU, they introduced a
new set of _protected/_check interfaces, an idea that could also be used
here to add this or something like this to the core.
Something like

#define list_for_each_entry_check(pos, list, member, lock) \
for (typeof(*lock) == typeof(struct mutex) ? assert_lockdep_held(lock) : assert_spin_locked(lock); \
     pos = list_first_entry(head, typeof(*pos), member); \
     &pos->member != (head); \
     pos = list_next_entry(pos, member))

#define i915_gem_object_for_each_vma(vma, obj) \
	list_for_each_entry_check(vma, &(obj)->vma_list, vma_link, &(obj)->base.dev->struct_mutex)
	
could be lifted easily, and makes i915_gem_object_for_each_vma() easier
to understand (i.e. serves better as documentation).

> +	     vma = list_first_entry(&(obj)->vma_list, typeof(*vma), vma_link);\
> +	     &vma->vma_link != (&(obj)->vma_list); \
> +	     vma = list_next_entry(vma, vma_link))
> +#else
> +  #define i915_gem_obj_for_each_vma(vma, obj) \
> +		list_for_each_entry((vma), &(obj)->vma_list, vma_link)
> +#endif
> +
>  /* Flags used by pin/bind&friends. */
>  #define PIN_MAPPABLE	(1<<0)
>  #define PIN_NONBLOCK	(1<<1)
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index c558887b2084..ce9d0544b42c 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2454,7 +2454,7 @@ i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring)
>  	list_move_tail(&obj->global_list,
>  		       &to_i915(obj->base.dev)->mm.bound_list);
>  
> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> +	i915_gem_obj_for_each_vma(vma, obj) {

This and the majority of the conversions simply should not exist and
only do so because of what I consider to be bad API. After they are
removed, there are only a few list iterators left. That said, there is
value in documenting the current locking.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/12] drm/i915: Add BKL asserts to get page helpers

[Chris Wilson]

On Tue, Feb 02, 2016 at 11:06:30AM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Purpose is catching illegal callers.
> 
> v2: Replace WARN_ON with lockdep_assert_held. (Chris Wilson, Daniel Vetter)
> v3: Moved under dedicated CONFIG_DRM_I915_DEBUG and back to WARN_ON.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_drv.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4ad025210416..6d34f74a5919 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2933,6 +2933,9 @@ i915_gem_object_get_dirty_page(struct drm_i915_gem_object *obj, int n);
>  static inline struct page *
>  i915_gem_object_get_page(struct drm_i915_gem_object *obj, int n)
>  {
> +#ifdef CONFIG_DRM_I915_DEBUG
> +	WARN_ON_ONCE(!mutex_is_locked(&obj->base.dev->struct_mutex));
> +#endif

But lockdep gives us useful debug information!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/12] drm/i915: Add wait_for_us

[Chris Wilson]

On Tue, Feb 02, 2016 at 11:06:19AM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> This is for callers who want micro-second precision but are not
> waiting from the atomic context.

linux/time.h provides us with USEC_PER_MSEC that would help to break up
these large numbers better for human consumption.

2000 -> 2*USEC_PER_SEC
10 -> 10*USEC_PER_MSEC

Maybe:

#define wait_for_seconds(x) ((x)*USEC_PER_SEC)
#define wait_for_milliseconds(x) ((x)*USEC_PER_MSEC)

if (_wait_for((I915_READ(pp_stat_reg) & mask) == value,
	      wait_for_seconds(5) /* timeout */,
	      wait_for_millseconds(10) /* interval */))

> @@ -55,7 +55,7 @@
>  			break;						\
>  		}							\
>  		if ((W) && drm_can_sleep()) {				\

Note after the atomic conversion, we can also do the !atomic assert here
and kill the drm_can_sleep() check
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/12] drm/i915: Do not wait atomically for display clocks

[Chris Wilson]

On Tue, Feb 02, 2016 at 11:06:20AM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Looks like this code does not need to wait atomically since it
> otherwise takes the mutex.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 304fc9637026..a7530cf612d7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9753,8 +9753,8 @@ static void broadwell_set_cdclk(struct drm_device *dev, int cdclk)
>  	val |= LCPLL_CD_SOURCE_FCLK;
>  	I915_WRITE(LCPLL_CTL, val);
>  
> -	if (wait_for_atomic_us(I915_READ(LCPLL_CTL) &
> -			       LCPLL_CD_SOURCE_FCLK_DONE, 1))
> +	if (wait_for_us(I915_READ(LCPLL_CTL) &
> +			LCPLL_CD_SOURCE_FCLK_DONE, 1))

Thinking about wait_for_seconds and friends from before, does this read
better as

if (wait_for(I915_READ(LCPLL_CTL) & LCPLL_CD_SOURCE_FCLK_DONE,
	     wait_for_microseconds(1))
?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/12] drm/i915/ilk: Move register read under spinlock

[Chris Wilson]

On Tue, Feb 02, 2016 at 11:06:27AM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Code does read-modify-write but the read was outside the lock.
> 
> It is fine since the caller holds struct mutex, but if we
> correct this we open up the opportunity for decreasing the
> mutex duration time since the call to ironlake_enable_drps
> does not need it any longer since it is covered by the
> mchdev_lock lock.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>

Makes sense,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/12] drm/i915: Add BKL asserts to get page helpers

[Tvrtko Ursulin]

On 02/02/16 11:39, Chris Wilson wrote:
> On Tue, Feb 02, 2016 at 11:06:30AM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Purpose is catching illegal callers.
>>
>> v2: Replace WARN_ON with lockdep_assert_held. (Chris Wilson, Daniel Vetter)
>> v3: Moved under dedicated CONFIG_DRM_I915_DEBUG and back to WARN_ON.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 4ad025210416..6d34f74a5919 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2933,6 +2933,9 @@ i915_gem_object_get_dirty_page(struct drm_i915_gem_object *obj, int n);
>>   static inline struct page *
>>   i915_gem_object_get_page(struct drm_i915_gem_object *obj, int n)
>>   {
>> +#ifdef CONFIG_DRM_I915_DEBUG
>> +	WARN_ON_ONCE(!mutex_is_locked(&obj->base.dev->struct_mutex));
>> +#endif
>
> But lockdep gives us useful debug information!

But you can't stick in a for loop. That applies to the previous patch so 
this one is consistent with what CONFIG_DRM_I915_DEBUG does.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/12] drm/i915: GEM operations need to be done under the big lock

[Chris Wilson]

On Tue, Feb 02, 2016 at 11:06:25AM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> VMA creation and GEM list management need the big lock.
> 
> v2:
> 
> Mutex unlock ended on the wrong path somehow. (0-day, Julia Lawall)
> 
> Not to mention drm_gem_object_unreference was there in existing
> code with no mutex held.
> 
> v3:
> 
> Some callers of i915_gem_object_create_stolen_for_preallocated
> already hold the lock so move the mutex into the other caller
> as well.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 3 +++
>  drivers/gpu/drm/i915/intel_display.c   | 8 ++++++--
>  2 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index c384dc9c8a63..a97a5e762c0f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -635,6 +635,9 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
>  	if (!drm_mm_initialized(&dev_priv->mm.stolen))
>  		return NULL;
>  
> +	if (WARN_ON_ONCE(!mutex_is_locked(&dev->struct_mutex)))
> +		return NULL;

I'm not keen on !mutex_is_locked(), since it can also just be another
thread holding the struct_mutex that grants us protection from this
check. At least lockdep checks that the caller holds the lock.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/12] drm/i915: Introduce dedicated object VMA iterator

[Tvrtko Ursulin]

On 02/02/16 11:36, Chris Wilson wrote:
> On Tue, Feb 02, 2016 at 11:06:28AM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Purpose is to catch places which iterate the object VMA list
>> without holding the big lock.
>>
>> Implemented by open coding list_for_each_entry to make the
>> macro compatible with existing call sites.
>>
>> v2: Error capture runs without the mutex so iterate directly from there.
>> v3: Replace WARN_ON with lockdep_assert_held. (Chris Wilson, Daniel Vetter)
>> v4: Moved under dedicated CONFIG_DRM_I915_DEBUG and back to WARN_ON.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c      |  8 ++++----
>>   drivers/gpu/drm/i915/i915_drv.h          | 11 +++++++++++
>>   drivers/gpu/drm/i915/i915_gem.c          | 24 ++++++++++++------------
>>   drivers/gpu/drm/i915/i915_gem_gtt.c      |  2 +-
>>   drivers/gpu/drm/i915/i915_gem_shrinker.c |  2 +-
>>   5 files changed, 29 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 863012a2602e..ff444f09ea98 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -117,7 +117,7 @@ static u64 i915_gem_obj_total_ggtt_size(struct drm_i915_gem_object *obj)
>>   	u64 size = 0;
>>   	struct i915_vma *vma;
>>
>> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
>> +	i915_gem_obj_for_each_vma(vma, obj) {
>>   		if (i915_is_ggtt(vma->vm) &&
>>   		    drm_mm_node_allocated(&vma->node))
>>   			size += vma->node.size;
>> @@ -155,7 +155,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>>   		   obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
>>   	if (obj->base.name)
>>   		seq_printf(m, " (name: %d)", obj->base.name);
>> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
>> +	i915_gem_obj_for_each_vma(vma, obj) {
>>   		if (vma->pin_count > 0)
>>   			pin_count++;
>>   	}
>> @@ -164,7 +164,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>>   		seq_printf(m, " (display)");
>>   	if (obj->fence_reg != I915_FENCE_REG_NONE)
>>   		seq_printf(m, " (fence: %d)", obj->fence_reg);
>> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
>> +	i915_gem_obj_for_each_vma(vma, obj) {
>>   		seq_printf(m, " (%sgtt offset: %08llx, size: %08llx",
>>   			   i915_is_ggtt(vma->vm) ? "g" : "pp",
>>   			   vma->node.start, vma->node.size);
>> @@ -342,7 +342,7 @@ static int per_file_stats(int id, void *ptr, void *data)
>>   		stats->shared += obj->base.size;
>>
>>   	if (USES_FULL_PPGTT(obj->base.dev)) {
>> -		list_for_each_entry(vma, &obj->vma_list, vma_link) {
>> +		i915_gem_obj_for_each_vma(vma, obj) {
>>   			struct i915_hw_ppgtt *ppgtt;
>>
>>   			if (!drm_mm_node_allocated(&vma->node))
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 905e90f25957..05ef750386df 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2861,6 +2861,17 @@ struct drm_i915_gem_object *i915_gem_object_create_from_data(
>>   void i915_gem_free_object(struct drm_gem_object *obj);
>>   void i915_gem_vma_destroy(struct i915_vma *vma);
>>
>> +#ifdef CONFIG_DRM_I915_DEBUG
>> +  #define i915_gem_obj_for_each_vma(vma, obj) \
>
> This is the wrong name, i915_gem_object is the object on which we are
> iterating over, so i915_gem_object_.

To me obj is just a shorthand for object, sorry. :) It sounds like bad 
naming if obj and object mean something functionally different. Imagine 
if request, req and rq meant something different. :D

>> +	for (WARN_ON_ONCE(!mutex_is_locked(&(obj)->base.dev->struct_mutex)), \
>
> I disagree that we should be doing i915-specific lockdep debugging and
> *ignoring* the core facilities. If we locked at RCU, they introduced a
> new set of _protected/_check interfaces, an idea that could also be used
> here to add this or something like this to the core.
> Something like
>
> #define list_for_each_entry_check(pos, list, member, lock) \
> for (typeof(*lock) == typeof(struct mutex) ? assert_lockdep_held(lock) : assert_spin_locked(lock); \
>       pos = list_first_entry(head, typeof(*pos), member); \
>       &pos->member != (head); \
>       pos = list_next_entry(pos, member))
>
> #define i915_gem_object_for_each_vma(vma, obj) \
> 	list_for_each_entry_check(vma, &(obj)->vma_list, vma_link, &(obj)->base.dev->struct_mutex)
> 	
> could be lifted easily, and makes i915_gem_object_for_each_vma() easier
> to understand (i.e. serves better as documentation).

Don't know, needs buy-in from the relevant people, and depends on how 
useful to outside of i915 it would be. And if you can make 
lockdep_assert_held work in this context.

But I am also not sure all i915 debugging should be tied to lockdep 
debugging so to me it is not so clear-cut.

>> +	     vma = list_first_entry(&(obj)->vma_list, typeof(*vma), vma_link);\
>> +	     &vma->vma_link != (&(obj)->vma_list); \
>> +	     vma = list_next_entry(vma, vma_link))
>> +#else
>> +  #define i915_gem_obj_for_each_vma(vma, obj) \
>> +		list_for_each_entry((vma), &(obj)->vma_list, vma_link)
>> +#endif
>> +
>>   /* Flags used by pin/bind&friends. */
>>   #define PIN_MAPPABLE	(1<<0)
>>   #define PIN_NONBLOCK	(1<<1)
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index c558887b2084..ce9d0544b42c 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2454,7 +2454,7 @@ i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring)
>>   	list_move_tail(&obj->global_list,
>>   		       &to_i915(obj->base.dev)->mm.bound_list);
>>
>> -	list_for_each_entry(vma, &obj->vma_list, vma_link) {
>> +	i915_gem_obj_for_each_vma(vma, obj) {
>
> This and the majority of the conversions simply should not exist and
> only do so because of what I consider to be bad API. After they are

Bad API or what you really meant was GEM internals should not do it? 
What is the harm anyway? More use, if it is painless, means less 
likelyhood for copy&paste errors in the future.

> removed, there are only a few list iterators left. That said, there is
> value in documenting the current locking.

The last bit meaning exactly?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/12] drm/i915: Do not lie about atomic wait granularity

[Chris Wilson]

On Tue, Feb 02, 2016 at 11:06:24AM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Currently the wait_for_atomic_us only allows for a millisecond
> granularity which is not nice towards callers requesting small
> micro-second waits.
> 
> Re-implement it so micro-second granularity is really supported
> and not just in the name of the macro.
> 
> v2:
>   * Warn when used from non-atomic context (if possible).
>   * Warn on too long atomic waits.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>

This also warns if we attempt to use a variable timeout as well, which
is a bonus.

>  drivers/gpu/drm/i915/intel_drv.h | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 779d17a9fcce..af6c1539106f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -63,11 +63,32 @@
>  	ret__;								\
>  })
>  
> +#if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT)

Ok, that took a little bit of digging to verify.

/* If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false. */

> +  #define _WAIT_FOR_ATOMIC_CHECK WARN_ON_ONCE(!in_atomic())
> +#else
> +  #define _WAIT_FOR_ATOMIC_CHECK do { } while(0)
> +#endif

Hmm, not often that we do indent inside #if, but when it is, it is
more typically '# define'

> +#define _wait_for_atomic(COND, US) ({ \
> +	unsigned long end__; \
> +	int ret__ = 0; \
> +	_WAIT_FOR_ATOMIC_CHECK; \
> +	BUILD_BUG_ON((US) > 50000); \
> +	end__ = (local_clock() >> 10) + (US) + 1; \
> +	while (!(COND)) { \
> +		if (time_after((unsigned long)(local_clock() >> 10), end__)) { \
> +			ret__ = -ETIMEDOUT; \
> +			break; \
> +		} \
> +		cpu_relax(); \
> +	} \
> +	ret__; \
> +})

Ok,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/12] drm/i915: Introduce dedicated object VMA iterator

[Chris Wilson]

On Tue, Feb 02, 2016 at 12:10:19PM +0000, Tvrtko Ursulin wrote:
> On 02/02/16 11:36, Chris Wilson wrote:
> >#define list_for_each_entry_check(pos, list, member, lock) \
> >for (typeof(*lock) == typeof(struct mutex) ? assert_lockdep_held(lock) : assert_spin_locked(lock); \
> >      pos = list_first_entry(head, typeof(*pos), member); \
> >      &pos->member != (head); \
> >      pos = list_next_entry(pos, member))
> >
> >#define i915_gem_object_for_each_vma(vma, obj) \
> >	list_for_each_entry_check(vma, &(obj)->vma_list, vma_link, &(obj)->base.dev->struct_mutex)
> >	
> >could be lifted easily, and makes i915_gem_object_for_each_vma() easier
> >to understand (i.e. serves better as documentation).
> 
> Don't know, needs buy-in from the relevant people, and depends on
> how useful to outside of i915 it would be. And if you can make
> lockdep_assert_held work in this context.

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 4004a7cf8db4..931684a74533 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -140,7 +140,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
                   obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
        if (obj->base.name)
                seq_printf(m, " (name: %d)", obj->base.name);
-       list_for_each_entry(vma, &obj->vma_list, obj_link) {
+       i915_gem_object_for_each_vma(vma, obj) {
                if (vma->pin_count > 0)
                        pin_count++;
        }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 31487aa11977..574e45ab43cb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3565,4 +3565,36 @@ int remap_io_mapping(struct vm_area_struct *vma,
                     unsigned long addr, unsigned long pfn, unsigned long size,
                     struct io_mapping *iomap);
 
+static __always_inline void __list_check_mutex(struct mutex *lock)
+{
+       lockdep_assert_held(lock);
+}
+
+static __always_inline void __list_check_spin(struct spinlock *lock)
+{
+        assert_spin_locked(lock);
+}
+
+static __always_inline void __list_check_bug(void *_lock)
+{
+       BUILD_BUG_ON("unknown lock type");
+}
+
+#define __list_check(lock) \
+       ({ __builtin_types_compatible_p(typeof(*lock), struct mutex) ? \
+        __list_check_mutex((struct mutex *)lock) : \
+        __builtin_types_compatible_p(typeof(*lock), struct spinlock) ? \
+        __list_check_spin((struct spinlock *)lock) : \
+        __list_check_bug(lock); \
+        0; })
+
+#define list_for_each_entry_check(pos, head, member, lock) \
+       for (__list_check(lock), \
+            pos = list_first_entry(head, typeof(*pos), member); \
+            &pos->member != (head); \
+            pos = list_next_entry(pos, member))
+
+#define i915_gem_object_for_each_vma(vma, obj) \
+       list_for_each_entry_check(vma, &(obj)->vma_list, obj_link, &(obj)->base.dev->struct_mutex)
+
 #endif

> But I am also not sure all i915 debugging should be tied to lockdep
> debugging so to me it is not so clear-cut.

> >>+	     vma = list_first_entry(&(obj)->vma_list, typeof(*vma), vma_link);\
> >>+	     &vma->vma_link != (&(obj)->vma_list); \
> >>+	     vma = list_next_entry(vma, vma_link))
> >>+#else
> >>+  #define i915_gem_obj_for_each_vma(vma, obj) \
> >>+		list_for_each_entry((vma), &(obj)->vma_list, vma_link)
> >>+#endif
> >>+
> >>  /* Flags used by pin/bind&friends. */
> >>  #define PIN_MAPPABLE	(1<<0)
> >>  #define PIN_NONBLOCK	(1<<1)
> >>diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >>index c558887b2084..ce9d0544b42c 100644
> >>--- a/drivers/gpu/drm/i915/i915_gem.c
> >>+++ b/drivers/gpu/drm/i915/i915_gem.c
> >>@@ -2454,7 +2454,7 @@ i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring)
> >>  	list_move_tail(&obj->global_list,
> >>  		       &to_i915(obj->base.dev)->mm.bound_list);
> >>
> >>-	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> >>+	i915_gem_obj_for_each_vma(vma, obj) {
> >
> >This and the majority of the conversions simply should not exist and
> >only do so because of what I consider to be bad API. After they are
> 
> Bad API or what you really meant was GEM internals should not do it?
> What is the harm anyway? More use, if it is painless, means less
> likelyhood for copy&paste errors in the future.

I mean we have many functions that walk the vma_list that have no
purpose.
 
> >removed, there are only a few list iterators left. That said, there is
> >value in documenting the current locking.
> 
> The last bit meaning exactly?

This patch (its precusor) did not find a bug, it generated a
false-positive warning. It does have some usefulness for providing
locking annotation. And in the future when the locking changes, it will
be useful in verifying the callsites remain correct.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/12] drm/i915: Fix struct mutex vs. RPS lock inversion

[Chris Wilson]

On Tue, Feb 02, 2016 at 11:06:26AM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> RPS lock must be taken before the struct_mutex to avoid
> locking inversion. So stop grabbing it for the whole
> powersave initialization and instead only take it during
> the sections which need it.
> 
> Also, struct_mutex is not needed any more since dedicated
> RPS lock was added in:
> 
>    commit 4fc688ce79772496503d22263d61b071a8fb596e
>    Author: Jesse Barnes <jbarnes@virtuousgeek.org>
>    Date:   Fri Nov 2 11:14:01 2012 -0700
> 
>        drm/i915: protect RPS/RC6 related accesses (including PCU) with a new mutex
> 
> Based on prototype patch by Chris Wilson and a subsequent
> mailing list discussion involving Ville, Imre, Chris and
> Daniel.
> 
> v2: More details in the commit.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 4 ----
>  drivers/gpu/drm/i915/intel_pm.c      | 9 +++++----
>  2 files changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5018295cd92b..af0d33a3697a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15995,9 +15995,7 @@ void intel_modeset_gem_init(struct drm_device *dev)
>  	struct drm_i915_gem_object *obj;
>  	int ret;
>  
> -	mutex_lock(&dev->struct_mutex);
>  	intel_init_gt_powersave(dev);
> -	mutex_unlock(&dev->struct_mutex);
>  
>  	intel_modeset_init_hw(dev);
>  
> @@ -16077,9 +16075,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
>  
>  	intel_cleanup_overlay(dev);
>  
> -	mutex_lock(&dev->struct_mutex);
>  	intel_cleanup_gt_powersave(dev);
> -	mutex_unlock(&dev->struct_mutex);

The whitespace no longer conveys meaning, it used to be to clearly mark
the mutex section.

> @@ -5235,6 +5233,7 @@ static void valleyview_setup_pctx(struct drm_device *dev)
>  out:
>  	DRM_DEBUG_DRIVER("PCBR: 0x%08x\n", I915_READ(VLV_PCBR));
>  	dev_priv->vlv_pctx = pctx;
> +	mutex_unlock(&dev->struct_mutex);
>  }
>  
>  static void valleyview_cleanup_pctx(struct drm_device *dev)
> @@ -5244,8 +5243,10 @@ static void valleyview_cleanup_pctx(struct drm_device *dev)
>  	if (WARN_ON(!dev_priv->vlv_pctx))
>  		return;
>  
> +	mutex_lock(&dev->struct_mutex);
>  	drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
>  	dev_priv->vlv_pctx = NULL;
> +	mutex_unlock(&dev->struct_mutex);

This made me smile.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/12] drm/i915: Add wait_for_us

[Dave Gordon]

On 02/02/16 11:06, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> This is for callers who want micro-second precision but are not
> waiting from the atomic context.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_dp.c  | 3 +--
>   drivers/gpu/drm/i915/intel_drv.h | 9 +++++----
>   drivers/gpu/drm/i915/intel_psr.c | 2 +-
>   3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e2bea710614f..fb8a76ec6ade 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1788,11 +1788,10 @@ static void wait_panel_status(struct intel_dp *intel_dp,
>   			I915_READ(pp_stat_reg),
>   			I915_READ(pp_ctrl_reg));
>
> -	if (_wait_for((I915_READ(pp_stat_reg) & mask) == value, 5000, 10)) {
> +	if (_wait_for((I915_READ(pp_stat_reg) & mask) == value, 5000000, 10000))
>   		DRM_ERROR("Panel status timeout: status %08x control %08x\n",
>   				I915_READ(pp_stat_reg),
>   				I915_READ(pp_ctrl_reg));
> -	}
>
>   	DRM_DEBUG_KMS("Wait complete\n");
>   }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f620023ed134..779d17a9fcce 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -45,8 +45,8 @@
>    * having timed out, since the timeout could be due to preemption or similar and
>    * we've never had a chance to check the condition before the timeout.
>    */
> -#define _wait_for(COND, MS, W) ({ \
> -	unsigned long timeout__ = jiffies + msecs_to_jiffies(MS) + 1;	\
> +#define _wait_for(COND, US, W) ({ \
> +	unsigned long timeout__ = jiffies + usecs_to_jiffies(US) + 1;	\
>   	int ret__ = 0;							\
>   	while (!(COND)) {						\
>   		if (time_after(jiffies, timeout__)) {			\
> @@ -55,7 +55,7 @@
>   			break;						\
>   		}							\
>   		if ((W) && drm_can_sleep()) {				\
> -			usleep_range((W)*1000, (W)*2000);		\
> +			usleep_range((W), (W)*2);			\
>   		} else {						\
>   			cpu_relax();					\
>   		}							\
> @@ -63,7 +63,8 @@
>   	ret__;								\
>   })
>
> -#define wait_for(COND, MS) _wait_for(COND, MS, 1)
> +#define wait_for(COND, MS) _wait_for(COND, (MS) * 1000, 1000)
> +#define wait_for_us(COND, US) _wait_for(COND, US, 1)
>   #define wait_for_atomic(COND, MS) _wait_for(COND, MS, 0)
>   #define wait_for_atomic_us(COND, US) _wait_for((COND), \
>   					       DIV_ROUND_UP((US), 1000), 0)

Don't these latter two macros need to be altered since the underlying 
_wait_for() now takes times in us rather than ms?

.Dave.

> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 9ccff3011523..e12377963839 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -492,7 +492,7 @@ static void hsw_psr_disable(struct intel_dp *intel_dp)
>
>   		/* Wait till PSR is idle */
>   		if (_wait_for((I915_READ(EDP_PSR_STATUS_CTL) &
> -			       EDP_PSR_STATUS_STATE_MASK) == 0, 2000, 10))
> +			       EDP_PSR_STATUS_STATE_MASK) == 0, 2000000, 10000))
>   			DRM_ERROR("Timed out waiting for PSR Idle State\n");
>
>   		dev_priv->psr.active = false;
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 10/12] drm/i915: Introduce dedicated object VMA iterator

[Tvrtko Ursulin]

On 02/02/16 12:58, Chris Wilson wrote:
> On Tue, Feb 02, 2016 at 12:10:19PM +0000, Tvrtko Ursulin wrote:
>> On 02/02/16 11:36, Chris Wilson wrote:
>>> #define list_for_each_entry_check(pos, list, member, lock) \
>>> for (typeof(*lock) == typeof(struct mutex) ? assert_lockdep_held(lock) : assert_spin_locked(lock); \
>>>       pos = list_first_entry(head, typeof(*pos), member); \
>>>       &pos->member != (head); \
>>>       pos = list_next_entry(pos, member))
>>>
>>> #define i915_gem_object_for_each_vma(vma, obj) \
>>> 	list_for_each_entry_check(vma, &(obj)->vma_list, vma_link, &(obj)->base.dev->struct_mutex)
>>> 	
>>> could be lifted easily, and makes i915_gem_object_for_each_vma() easier
>>> to understand (i.e. serves better as documentation).
>>
>> Don't know, needs buy-in from the relevant people, and depends on
>> how useful to outside of i915 it would be. And if you can make
>> lockdep_assert_held work in this context.
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 4004a7cf8db4..931684a74533 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -140,7 +140,7 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>                     obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
>          if (obj->base.name)
>                  seq_printf(m, " (name: %d)", obj->base.name);
> -       list_for_each_entry(vma, &obj->vma_list, obj_link) {
> +       i915_gem_object_for_each_vma(vma, obj) {
>                  if (vma->pin_count > 0)
>                          pin_count++;
>          }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 31487aa11977..574e45ab43cb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3565,4 +3565,36 @@ int remap_io_mapping(struct vm_area_struct *vma,
>                       unsigned long addr, unsigned long pfn, unsigned long size,
>                       struct io_mapping *iomap);
>
> +static __always_inline void __list_check_mutex(struct mutex *lock)
> +{
> +       lockdep_assert_held(lock);
> +}
> +
> +static __always_inline void __list_check_spin(struct spinlock *lock)
> +{
> +        assert_spin_locked(lock);
> +}
> +
> +static __always_inline void __list_check_bug(void *_lock)
> +{
> +       BUILD_BUG_ON("unknown lock type");
> +}
> +
> +#define __list_check(lock) \
> +       ({ __builtin_types_compatible_p(typeof(*lock), struct mutex) ? \
> +        __list_check_mutex((struct mutex *)lock) : \
> +        __builtin_types_compatible_p(typeof(*lock), struct spinlock) ? \
> +        __list_check_spin((struct spinlock *)lock) : \
> +        __list_check_bug(lock); \
> +        0; })
> +
> +#define list_for_each_entry_check(pos, head, member, lock) \
> +       for (__list_check(lock), \
> +            pos = list_first_entry(head, typeof(*pos), member); \
> +            &pos->member != (head); \
> +            pos = list_next_entry(pos, member))
> +
> +#define i915_gem_object_for_each_vma(vma, obj) \
> +       list_for_each_entry_check(vma, &(obj)->vma_list, obj_link, &(obj)->base.dev->struct_mutex)
> +
>   #endif

Nice! Want to take it over? ;)

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/12] drm/i915: Add wait_for_us

[Tvrtko Ursulin]

On 02/02/16 13:35, Dave Gordon wrote:
> On 02/02/16 11:06, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> This is for callers who want micro-second precision but are not
>> waiting from the atomic context.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c  | 3 +--
>>   drivers/gpu/drm/i915/intel_drv.h | 9 +++++----
>>   drivers/gpu/drm/i915/intel_psr.c | 2 +-
>>   3 files changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> b/drivers/gpu/drm/i915/intel_dp.c
>> index e2bea710614f..fb8a76ec6ade 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1788,11 +1788,10 @@ static void wait_panel_status(struct intel_dp
>> *intel_dp,
>>               I915_READ(pp_stat_reg),
>>               I915_READ(pp_ctrl_reg));
>>
>> -    if (_wait_for((I915_READ(pp_stat_reg) & mask) == value, 5000, 10)) {
>> +    if (_wait_for((I915_READ(pp_stat_reg) & mask) == value, 5000000,
>> 10000))
>>           DRM_ERROR("Panel status timeout: status %08x control %08x\n",
>>                   I915_READ(pp_stat_reg),
>>                   I915_READ(pp_ctrl_reg));
>> -    }
>>
>>       DRM_DEBUG_KMS("Wait complete\n");
>>   }
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index f620023ed134..779d17a9fcce 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -45,8 +45,8 @@
>>    * having timed out, since the timeout could be due to preemption or
>> similar and
>>    * we've never had a chance to check the condition before the timeout.
>>    */
>> -#define _wait_for(COND, MS, W) ({ \
>> -    unsigned long timeout__ = jiffies + msecs_to_jiffies(MS) + 1;    \
>> +#define _wait_for(COND, US, W) ({ \
>> +    unsigned long timeout__ = jiffies + usecs_to_jiffies(US) + 1;    \
>>       int ret__ = 0;                            \
>>       while (!(COND)) {                        \
>>           if (time_after(jiffies, timeout__)) {            \
>> @@ -55,7 +55,7 @@
>>               break;                        \
>>           }                            \
>>           if ((W) && drm_can_sleep()) {                \
>> -            usleep_range((W)*1000, (W)*2000);        \
>> +            usleep_range((W), (W)*2);            \
>>           } else {                        \
>>               cpu_relax();                    \
>>           }                            \
>> @@ -63,7 +63,8 @@
>>       ret__;                                \
>>   })
>>
>> -#define wait_for(COND, MS) _wait_for(COND, MS, 1)
>> +#define wait_for(COND, MS) _wait_for(COND, (MS) * 1000, 1000)
>> +#define wait_for_us(COND, US) _wait_for(COND, US, 1)
>>   #define wait_for_atomic(COND, MS) _wait_for(COND, MS, 0)
>>   #define wait_for_atomic_us(COND, US) _wait_for((COND), \
>>                              DIV_ROUND_UP((US), 1000), 0)
>
> Don't these latter two macros need to be altered since the underlying
> _wait_for() now takes times in us rather than ms?

Yes you are right, thanks! (Consequence of re-ordering and re-basing.)

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/12] drm/i915: Add wait_for_us

[Tvrtko Ursulin]

On 02/02/16 11:57, Chris Wilson wrote:
> On Tue, Feb 02, 2016 at 11:06:19AM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> This is for callers who want micro-second precision but are not
>> waiting from the atomic context.
>
> linux/time.h provides us with USEC_PER_MSEC that would help to break up
> these large numbers better for human consumption.
>
> 2000 -> 2*USEC_PER_SEC
> 10 -> 10*USEC_PER_MSEC
>
> Maybe:
>
> #define wait_for_seconds(x) ((x)*USEC_PER_SEC)
> #define wait_for_milliseconds(x) ((x)*USEC_PER_MSEC)
>
> if (_wait_for((I915_READ(pp_stat_reg) & mask) == value,
> 	      wait_for_seconds(5) /* timeout */,
> 	      wait_for_millseconds(10) /* interval */))

There are only two callers where it would be a bit interesting so it 
just feels like needless change to me at the moment. Better to keep the 
established conventions for these two macros.

>> @@ -55,7 +55,7 @@
>>   			break;						\
>>   		}							\
>>   		if ((W) && drm_can_sleep()) {				\
>
> Note after the atomic conversion, we can also do the !atomic assert here
> and kill the drm_can_sleep() check

Noted. Maybe I'll put a comment somewhere.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/12] drm/i915: Do not wait atomically for display clocks

[Dave Gordon]

On 02/02/16 12:00, Chris Wilson wrote:
> On Tue, Feb 02, 2016 at 11:06:20AM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Looks like this code does not need to wait atomically since it
>> otherwise takes the mutex.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 304fc9637026..a7530cf612d7 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -9753,8 +9753,8 @@ static void broadwell_set_cdclk(struct drm_device *dev, int cdclk)
>>   	val |= LCPLL_CD_SOURCE_FCLK;
>>   	I915_WRITE(LCPLL_CTL, val);
>>
>> -	if (wait_for_atomic_us(I915_READ(LCPLL_CTL) &
>> -			       LCPLL_CD_SOURCE_FCLK_DONE, 1))
>> +	if (wait_for_us(I915_READ(LCPLL_CTL) &
>> +			LCPLL_CD_SOURCE_FCLK_DONE, 1))
>
> Thinking about wait_for_seconds and friends from before, does this read
> better as
>
> if (wait_for(I915_READ(LCPLL_CTL) & LCPLL_CD_SOURCE_FCLK_DONE,
> 	     wait_for_microseconds(1))
> ?
> -Chris

No, not really, because it confuses a function (or macro) that tests for 
a condition with one that converts between different units of time, yet 
they both have names beginning wait_for...

And people might expect a function called wait_for_microseconds() to 
actually wait for some number of microseconds!

There's an ambiguity in English anyway e.g. "wait for a bus" (event) vs 
"wait for 10 minutes" (duration). But there's no need to propagate 
natural-language foolishness into code.

What we're really trying to express is

     ({  while (!condition && !timedout)
             delay(interval)
	resultis timedout; })

but there's still a question about, why should the granularity of the 
delay be related to the precision of the timespec? With these patches, 
we've got a situation where if the timeout is specified in us, the delay 
between rechecking the condition is 1us, but if the timeout is in ms, 
there's a 1ms recheck interval.

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 03/12] drm/i915/guc: Do not wait for firmware load atomically

[Dave Gordon]

On 02/02/16 11:06, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> It does not look like this code needs to wait atomically?
>
> Higher in the call chain it calls the GEM API and I do
> not see that the section is under any spin locks or such.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Alex Dai <yu.dai@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_guc_loader.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 3accd914490f..82a3c03fbc0e 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -199,7 +199,7 @@ static void set_guc_init_params(struct drm_i915_private *dev_priv)
>    * the value matches either of two values representing completion
>    * of the GuC boot process.
>    *
> - * This is used for polling the GuC status in a wait_for_atomic()
> + * This is used for polling the GuC status in a wait_for()
>    * loop below.
>    */
>   static inline bool guc_ucode_response(struct drm_i915_private *dev_priv,
> @@ -259,14 +259,14 @@ static int guc_ucode_xfer_dma(struct drm_i915_private *dev_priv)
>   	I915_WRITE(DMA_CTRL, _MASKED_BIT_ENABLE(UOS_MOVE | START_DMA));
>
>   	/*
> -	 * Spin-wait for the DMA to complete & the GuC to start up.
> +	 * Wait for the DMA to complete & the GuC to start up.
>   	 * NB: Docs recommend not using the interrupt for completion.
>   	 * Measurements indicate this should take no more than 20ms, so a
>   	 * timeout here indicates that the GuC has failed and is unusable.
>   	 * (Higher levels of the driver will attempt to fall back to
>   	 * execlist mode if this happens.)
>   	 */
> -	ret = wait_for_atomic(guc_ucode_response(dev_priv, &status), 100);
> +	ret = wait_for(guc_ucode_response(dev_priv, &status), 100);
>
>   	DRM_DEBUG_DRIVER("DMA status 0x%x, GuC status 0x%x\n",
>   			I915_READ(DMA_CTRL), status);

LGTM.

Reviewed-by: Dave Gordon <david.s.gordon@intel.com>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/12] drm/i915: Fix struct mutex vs. RPS lock inversion

[Tvrtko Ursulin]

On 02/02/16 13:16, Chris Wilson wrote:
> On Tue, Feb 02, 2016 at 11:06:26AM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> RPS lock must be taken before the struct_mutex to avoid
>> locking inversion. So stop grabbing it for the whole
>> powersave initialization and instead only take it during
>> the sections which need it.
>>
>> Also, struct_mutex is not needed any more since dedicated
>> RPS lock was added in:
>>
>>     commit 4fc688ce79772496503d22263d61b071a8fb596e
>>     Author: Jesse Barnes <jbarnes@virtuousgeek.org>
>>     Date:   Fri Nov 2 11:14:01 2012 -0700
>>
>>         drm/i915: protect RPS/RC6 related accesses (including PCU) with a new mutex
>>
>> Based on prototype patch by Chris Wilson and a subsequent
>> mailing list discussion involving Ville, Imre, Chris and
>> Daniel.
>>
>> v2: More details in the commit.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Imre Deak <imre.deak@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c | 4 ----
>>   drivers/gpu/drm/i915/intel_pm.c      | 9 +++++----
>>   2 files changed, 5 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 5018295cd92b..af0d33a3697a 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -15995,9 +15995,7 @@ void intel_modeset_gem_init(struct drm_device *dev)
>>   	struct drm_i915_gem_object *obj;
>>   	int ret;
>>
>> -	mutex_lock(&dev->struct_mutex);
>>   	intel_init_gt_powersave(dev);
>> -	mutex_unlock(&dev->struct_mutex);
>>
>>   	intel_modeset_init_hw(dev);
>>
>> @@ -16077,9 +16075,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
>>
>>   	intel_cleanup_overlay(dev);
>>
>> -	mutex_lock(&dev->struct_mutex);
>>   	intel_cleanup_gt_powersave(dev);
>> -	mutex_unlock(&dev->struct_mutex);
>
> The whitespace no longer conveys meaning, it used to be to clearly mark
> the mutex section.

Guess I can remove more lines of code and get credits for that. :D

>> @@ -5235,6 +5233,7 @@ static void valleyview_setup_pctx(struct drm_device *dev)
>>   out:
>>   	DRM_DEBUG_DRIVER("PCBR: 0x%08x\n", I915_READ(VLV_PCBR));
>>   	dev_priv->vlv_pctx = pctx;
>> +	mutex_unlock(&dev->struct_mutex);
>>   }
>>
>>   static void valleyview_cleanup_pctx(struct drm_device *dev)
>> @@ -5244,8 +5243,10 @@ static void valleyview_cleanup_pctx(struct drm_device *dev)
>>   	if (WARN_ON(!dev_priv->vlv_pctx))
>>   		return;
>>
>> +	mutex_lock(&dev->struct_mutex);
>>   	drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
>>   	dev_priv->vlv_pctx = NULL;
>> +	mutex_unlock(&dev->struct_mutex);
>
> This made me smile.

Yeah mechanical- want unreference_unlocked instead?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 01/12] drm/i915: Add wait_for_us

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

This is for callers who want micro-second precision but are not
waiting from the atomic context.

v2:
  * Fix atomic waits. (Dave Gordon)
  * Use USEC_PER_SEC and USEC_PER_MSEC. (Chris Wilson)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Dave Gordon <david.s.gordon@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_dp.c  |  4 ++--
 drivers/gpu/drm/i915/intel_drv.h | 15 ++++++++-------
 drivers/gpu/drm/i915/intel_psr.c |  3 ++-
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f44aba1019b8..d8a86e065ae9 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1788,11 +1788,11 @@ static void wait_panel_status(struct intel_dp *intel_dp,
 			I915_READ(pp_stat_reg),
 			I915_READ(pp_ctrl_reg));
 
-	if (_wait_for((I915_READ(pp_stat_reg) & mask) == value, 5000, 10)) {
+	if (_wait_for((I915_READ(pp_stat_reg) & mask) == value,
+		      5 * USEC_PER_SEC, 10 * USEC_PER_MSEC))
 		DRM_ERROR("Panel status timeout: status %08x control %08x\n",
 				I915_READ(pp_stat_reg),
 				I915_READ(pp_ctrl_reg));
-	}
 
 	DRM_DEBUG_KMS("Wait complete\n");
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 93ba14a3bb76..93da5ee2b09e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -45,8 +45,8 @@
  * having timed out, since the timeout could be due to preemption or similar and
  * we've never had a chance to check the condition before the timeout.
  */
-#define _wait_for(COND, MS, W) ({ \
-	unsigned long timeout__ = jiffies + msecs_to_jiffies(MS) + 1;	\
+#define _wait_for(COND, US, W) ({ \
+	unsigned long timeout__ = jiffies + usecs_to_jiffies(US) + 1;	\
 	int ret__ = 0;							\
 	while (!(COND)) {						\
 		if (time_after(jiffies, timeout__)) {			\
@@ -55,7 +55,7 @@
 			break;						\
 		}							\
 		if ((W) && drm_can_sleep()) {				\
-			usleep_range((W)*1000, (W)*2000);		\
+			usleep_range((W), (W)*2);			\
 		} else {						\
 			cpu_relax();					\
 		}							\
@@ -63,10 +63,11 @@
 	ret__;								\
 })
 
-#define wait_for(COND, MS) _wait_for(COND, MS, 1)
-#define wait_for_atomic(COND, MS) _wait_for(COND, MS, 0)
-#define wait_for_atomic_us(COND, US) _wait_for((COND), \
-					       DIV_ROUND_UP((US), 1000), 0)
+#define wait_for(COND, MS)	  	_wait_for((COND), (MS) * 1000, 1000)
+#define wait_for_us(COND, US)	  	_wait_for((COND), (US), 1)
+
+#define wait_for_atomic(COND, MS) 	_wait_for((COND), (MS) * 1000, 0)
+#define wait_for_atomic_us(COND, US)	_wait_for((COND), (US), 0)
 
 #define KHz(x) (1000 * (x))
 #define MHz(x) KHz(1000 * (x))
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 4ab757947f15..366b24b14fc1 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -507,7 +507,8 @@ static void hsw_psr_disable(struct intel_dp *intel_dp)
 
 		/* Wait till PSR is idle */
 		if (_wait_for((I915_READ(EDP_PSR_STATUS_CTL) &
-			       EDP_PSR_STATUS_STATE_MASK) == 0, 2000, 10))
+			       EDP_PSR_STATUS_STATE_MASK) == 0,
+			       2 * USEC_PER_SEC, 10 * USEC_PER_MSEC))
 			DRM_ERROR("Timed out waiting for PSR Idle State\n");
 
 		dev_priv->psr.active = false;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 06/12] drm/i915: Do not lie about atomic wait granularity

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Currently the wait_for_atomic_us only allows for a millisecond
granularity which is not nice towards callers requesting small
micro-second waits.

Re-implement it so micro-second granularity is really supported
and not just in the name of the macro.

v2:
  * Warn when used from non-atomic context (if possible).
  * Warn on too long atomic waits.

v3:
  * Added comment explaining CONFIG_PREEMPT_COUNT.
  * Fixed pre-processor indentation.
  (Chris Wilson)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_drv.h | 31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 93da5ee2b09e..90e14bfdded6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -44,6 +44,10 @@
  * contexts. Note that it's important that we check the condition again after
  * having timed out, since the timeout could be due to preemption or similar and
  * we've never had a chance to check the condition before the timeout.
+ * 
+ * TODO: When modesetting has fully transitioned to atomic, the below
+ * drm_can_sleep() can be removed and in_atomic()/!in_atomic() asserts
+ * added.
  */
 #define _wait_for(COND, US, W) ({ \
 	unsigned long timeout__ = jiffies + usecs_to_jiffies(US) + 1;	\
@@ -66,8 +70,31 @@
 #define wait_for(COND, MS)	  	_wait_for((COND), (MS) * 1000, 1000)
 #define wait_for_us(COND, US)	  	_wait_for((COND), (US), 1)
 
-#define wait_for_atomic(COND, MS) 	_wait_for((COND), (MS) * 1000, 0)
-#define wait_for_atomic_us(COND, US)	_wait_for((COND), (US), 0)
+/* If CONFIG_PREEMPT_COUNT is disabled, in_atomic() always reports false. */
+#if defined(CONFIG_DRM_I915_DEBUG) && defined(CONFIG_PREEMPT_COUNT)
+# define _WAIT_FOR_ATOMIC_CHECK WARN_ON_ONCE(!in_atomic())
+#else
+# define _WAIT_FOR_ATOMIC_CHECK do { } while(0)
+#endif
+
+#define _wait_for_atomic(COND, US) ({ \
+	unsigned long end__; \
+	int ret__ = 0; \
+	_WAIT_FOR_ATOMIC_CHECK; \
+	BUILD_BUG_ON((US) > 50000); \
+	end__ = (local_clock() >> 10) + (US) + 1; \
+	while (!(COND)) { \
+		if (time_after((unsigned long)(local_clock() >> 10), end__)) { \
+			ret__ = -ETIMEDOUT; \
+			break; \
+		} \
+		cpu_relax(); \
+	} \
+	ret__; \
+})
+
+#define wait_for_atomic(COND, MS)	_wait_for_atomic((COND), (MS) * 1000)
+#define wait_for_atomic_us(COND, US)	_wait_for_atomic((COND), (US))
 
 #define KHz(x) (1000 * (x))
 #define MHz(x) KHz(1000 * (x))
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v4 07/12] drm/i915: GEM operations need to be done under the big lock

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

VMA creation and GEM list management need the big lock.

v2:

Mutex unlock ended on the wrong path somehow. (0-day, Julia Lawall)

Not to mention drm_gem_object_unreference was there in existing
code with no mutex held.

v3:

Some callers of i915_gem_object_create_stolen_for_preallocated
already hold the lock so move the mutex into the other caller
as well.

v4:

Changed to lockdep_assert_held. (Chris Wilson)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 2 ++
 drivers/gpu/drm/i915/intel_display.c   | 8 ++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index c384dc9c8a63..8cb0371673c5 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -635,6 +635,8 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
 	if (!drm_mm_initialized(&dev_priv->mm.stolen))
 		return NULL;
 
+	lockdep_assert_held(&dev->struct_mutex);
+
 	DRM_DEBUG_KMS("creating preallocated stolen object: stolen_offset=%x, gtt_offset=%x, size=%x\n",
 			stolen_offset, gtt_offset, size);
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f3c036d98b6c..b7ee2f3af856 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2551,12 +2551,16 @@ intel_alloc_initial_plane_obj(struct intel_crtc *crtc,
 	if (size_aligned * 2 > dev_priv->gtt.stolen_usable_size)
 		return false;
 
+	mutex_lock(&dev->struct_mutex);
+
 	obj = i915_gem_object_create_stolen_for_preallocated(dev,
 							     base_aligned,
 							     base_aligned,
 							     size_aligned);
-	if (!obj)
+	if (!obj) {
+		mutex_unlock(&dev->struct_mutex);
 		return false;
+	}
 
 	obj->tiling_mode = plane_config->tiling;
 	if (obj->tiling_mode == I915_TILING_X)
@@ -2569,12 +2573,12 @@ intel_alloc_initial_plane_obj(struct intel_crtc *crtc,
 	mode_cmd.modifier[0] = fb->modifier[0];
 	mode_cmd.flags = DRM_MODE_FB_MODIFIERS;
 
-	mutex_lock(&dev->struct_mutex);
 	if (intel_framebuffer_init(dev, to_intel_framebuffer(fb),
 				   &mode_cmd, obj)) {
 		DRM_DEBUG_KMS("intel fb init failed\n");
 		goto out_unref_obj;
 	}
+
 	mutex_unlock(&dev->struct_mutex);
 
 	DRM_DEBUG_KMS("initial plane fb obj %p\n", obj);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3 08/12] drm/i915: Fix struct mutex vs. RPS lock inversion

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

RPS lock must be taken before the struct_mutex to avoid
locking inversion. So stop grabbing it for the whole
powersave initialization and instead only take it during
the sections which need it.

Also, struct_mutex is not needed any more since dedicated
RPS lock was added in:

   commit 4fc688ce79772496503d22263d61b071a8fb596e
   Author: Jesse Barnes <jbarnes@virtuousgeek.org>
   Date:   Fri Nov 2 11:14:01 2012 -0700

       drm/i915: protect RPS/RC6 related accesses (including PCU) with a new mutex

Based on prototype patch by Chris Wilson and a subsequent
mailing list discussion involving Ville, Imre, Chris and
Daniel.

v2: More details in the commit.

v3: Use drm_gem_object_unreference_unlocked. (Chris Wilson)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 4 ----
 drivers/gpu/drm/i915/intel_pm.c      | 9 ++++-----
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b7ee2f3af856..7dc23a034c0e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15973,9 +15973,7 @@ void intel_modeset_gem_init(struct drm_device *dev)
 	struct drm_i915_gem_object *obj;
 	int ret;
 
-	mutex_lock(&dev->struct_mutex);
 	intel_init_gt_powersave(dev);
-	mutex_unlock(&dev->struct_mutex);
 
 	intel_modeset_init_hw(dev);
 
@@ -16055,9 +16053,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
 
 	intel_cleanup_overlay(dev);
 
-	mutex_lock(&dev->struct_mutex);
 	intel_cleanup_gt_powersave(dev);
-	mutex_unlock(&dev->struct_mutex);
 
 	intel_teardown_gmbus(dev);
 }
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 31bc4ea395ac..e077a091a962 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5175,8 +5175,6 @@ static void cherryview_setup_pctx(struct drm_device *dev)
 	u32 pcbr;
 	int pctx_size = 32*1024;
 
-	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
-
 	pcbr = I915_READ(VLV_PCBR);
 	if ((pcbr >> VLV_PCBR_ADDR_SHIFT) == 0) {
 		DRM_DEBUG_DRIVER("BIOS didn't set up PCBR, fixing up\n");
@@ -5198,7 +5196,7 @@ static void valleyview_setup_pctx(struct drm_device *dev)
 	u32 pcbr;
 	int pctx_size = 24*1024;
 
-	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+	mutex_lock(&dev->struct_mutex);
 
 	pcbr = I915_READ(VLV_PCBR);
 	if (pcbr) {
@@ -5226,7 +5224,7 @@ static void valleyview_setup_pctx(struct drm_device *dev)
 	pctx = i915_gem_object_create_stolen(dev, pctx_size);
 	if (!pctx) {
 		DRM_DEBUG("not enough stolen space for PCTX, disabling\n");
-		return;
+		goto out;
 	}
 
 	pctx_paddr = dev_priv->mm.stolen_base + pctx->stolen->start;
@@ -5235,6 +5233,7 @@ static void valleyview_setup_pctx(struct drm_device *dev)
 out:
 	DRM_DEBUG_DRIVER("PCBR: 0x%08x\n", I915_READ(VLV_PCBR));
 	dev_priv->vlv_pctx = pctx;
+	mutex_unlock(&dev->struct_mutex);
 }
 
 static void valleyview_cleanup_pctx(struct drm_device *dev)
@@ -5244,7 +5243,7 @@ static void valleyview_cleanup_pctx(struct drm_device *dev)
 	if (WARN_ON(!dev_priv->vlv_pctx))
 		return;
 
-	drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
+	drm_gem_object_unreference_unlocked(&dev_priv->vlv_pctx->base);
 	dev_priv->vlv_pctx = NULL;
 }
 
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/12] drm/i915: Fix struct mutex vs. RPS lock inversion

[Chris Wilson]

On Tue, Feb 02, 2016 at 02:13:43PM +0000, Tvrtko Ursulin wrote:
> 
> On 02/02/16 13:16, Chris Wilson wrote:
> >On Tue, Feb 02, 2016 at 11:06:26AM +0000, Tvrtko Ursulin wrote:
> >>@@ -5244,8 +5243,10 @@ static void valleyview_cleanup_pctx(struct drm_device *dev)
> >>  	if (WARN_ON(!dev_priv->vlv_pctx))
> >>  		return;
> >>
> >>+	mutex_lock(&dev->struct_mutex);
> >>  	drm_gem_object_unreference(&dev_priv->vlv_pctx->base);
> >>  	dev_priv->vlv_pctx = NULL;
> >>+	mutex_unlock(&dev->struct_mutex);
> >
> >This made me smile.
> 
> Yeah mechanical- want unreference_unlocked instead?

_unlocked() has the advantage of not suggesting that dev_priv->vlv_pctx
needs to be part of the locked transaction (that's the bit that caused a
double take).

But it really doesn't matter.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/12] drm/i915: Do not wait atomically for display clocks

[Chris Wilson]

On Tue, Feb 02, 2016 at 02:08:44PM +0000, Dave Gordon wrote:
> On 02/02/16 12:00, Chris Wilson wrote:
> >On Tue, Feb 02, 2016 at 11:06:20AM +0000, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >>Looks like this code does not need to wait atomically since it
> >>otherwise takes the mutex.
> >>
> >>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>---
> >>  drivers/gpu/drm/i915/intel_display.c | 8 ++++----
> >>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>index 304fc9637026..a7530cf612d7 100644
> >>--- a/drivers/gpu/drm/i915/intel_display.c
> >>+++ b/drivers/gpu/drm/i915/intel_display.c
> >>@@ -9753,8 +9753,8 @@ static void broadwell_set_cdclk(struct drm_device *dev, int cdclk)
> >>  	val |= LCPLL_CD_SOURCE_FCLK;
> >>  	I915_WRITE(LCPLL_CTL, val);
> >>
> >>-	if (wait_for_atomic_us(I915_READ(LCPLL_CTL) &
> >>-			       LCPLL_CD_SOURCE_FCLK_DONE, 1))
> >>+	if (wait_for_us(I915_READ(LCPLL_CTL) &
> >>+			LCPLL_CD_SOURCE_FCLK_DONE, 1))
> >
> >Thinking about wait_for_seconds and friends from before, does this read
> >better as
> >
> >if (wait_for(I915_READ(LCPLL_CTL) & LCPLL_CD_SOURCE_FCLK_DONE,
> >	     wait_for_microseconds(1))
> >?
> >-Chris
> 
> No, not really, because it confuses a function (or macro) that tests
> for a condition with one that converts between different units of
> time, yet they both have names beginning wait_for...
>
> And people might expect a function called wait_for_microseconds() to
> actually wait for some number of microseconds!

So, suggest an improvement. ms_to_us, msec_to_usec, MSEC_TO_USEC,
SEC_TO_USEC, timeout_in_microseconds, timeout_in_millseconds,
timeout_in_seconds. But since this is specific to the wait_for
interface, they need to reflect that, so wait_timeout_in_seconds, but
that is too general (now we are treading on wait_event territory), so
back to wait_for_seconds_timeout()?

On the other hand, msecs_to_jiffies().

My question is whether having a universal wait_for() macro is
preferrable over having both wait_for() and wait_for_us() and passing in
million+ numbers.

> There's an ambiguity in English anyway e.g. "wait for a bus" (event)
> vs "wait for 10 minutes" (duration). But there's no need to
> propagate natural-language foolishness into code.

Oh, you are misinterpreting the name. It was meant to be wait_for (the
interface name) + _units.

> What we're really trying to express is
> 
>     ({  while (!condition && !timedout)
>             delay(interval)
> 	resultis timedout; })
> 
> but there's still a question about, why should the granularity of
> the delay be related to the precision of the timespec? With these
> patches, we've got a situation where if the timeout is specified in
> us, the delay between rechecking the condition is 1us, but if the
> timeout is in ms, there's a 1ms recheck interval.

Why not? The original intent was for checking status changes across tens
of microseconds where we wanted a coarse granularity and there was no
impetus for high accuracy. Then this was copy-pasted into a new routine
for microsecond resoluation, and again there was no demand for fine
control over the interval. Nor has anyone really looked at whether 1us
is a good figure for the "high resolution" waits - except for a couple
of instances where a different value has been seemingly arbitrarily
chosen. We have talked about using timeout/N or an exponential interval.

If I had chosen wait_until() as the name, that would have saved a few
brain cells. But I can't see a way to avoid the name proliferation for
_atomic, _with_interval like wait_event. At the least, we can trim that
down by avoiding encoding the units into the macro name. We also
probably don't yet need the _with_interval, so the only question is
whether it is possible to have a single macro suitable for waits from
1us to 1s (and a second macro wait_for_atomic that operates in
microseconds for good reason).

ret = wait_for(I915_READ(LCPLL_CTL) & LCPLL_CD_SOURCE_FCLK_DONE, 1);

and

ret = wait_for((I915_READ(dpll_reg) & port_mask) == expected_mask,
	       secs_to_usecs(1));

Is it possible to have a single dtrt macro to cover such a range?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/12] drm/i915: Add wait_for_us

[Chris Wilson]

On Tue, Feb 02, 2016 at 02:04:55PM +0000, Tvrtko Ursulin wrote:
> 
> 
> On 02/02/16 11:57, Chris Wilson wrote:
> >On Tue, Feb 02, 2016 at 11:06:19AM +0000, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >>This is for callers who want micro-second precision but are not
> >>waiting from the atomic context.
> >
> >linux/time.h provides us with USEC_PER_MSEC that would help to break up
> >these large numbers better for human consumption.
> >
> >2000 -> 2*USEC_PER_SEC
> >10 -> 10*USEC_PER_MSEC
> >
> >Maybe:
> >
> >#define wait_for_seconds(x) ((x)*USEC_PER_SEC)
> >#define wait_for_milliseconds(x) ((x)*USEC_PER_MSEC)
> >
> >if (_wait_for((I915_READ(pp_stat_reg) & mask) == value,
> >	      wait_for_seconds(5) /* timeout */,
> >	      wait_for_millseconds(10) /* interval */))
> 
> There are only two callers where it would be a bit interesting so it
> just feels like needless change to me at the moment. Better to keep
> the established conventions for these two macros.

I am a bit more concerned that there are any users of _wait_for()
outside of the header.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 07/12] drm/i915: GEM operations need to be done under the big lock

[Chris Wilson]

On Tue, Feb 02, 2016 at 02:46:19PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> VMA creation and GEM list management need the big lock.
> 
> v2:
> 
> Mutex unlock ended on the wrong path somehow. (0-day, Julia Lawall)
> 
> Not to mention drm_gem_object_unreference was there in existing
> code with no mutex held.
> 
> v3:
> 
> Some callers of i915_gem_object_create_stolen_for_preallocated
> already hold the lock so move the mutex into the other caller
> as well.
> 
> v4:
> 
> Changed to lockdep_assert_held. (Chris Wilson)

To appease Daniel, I was thinking could we smarten up

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index c57e424d914b..e10da10e0033 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -422,8 +422,19 @@ struct lock_class_key { };
 
 #define lockdep_depth(tsk)     (0)
 
+#ifdef CONFIG_POOR_MANS_LOCKDEP
+#define __lockdep_check(l) \
+       ({ __builtin_types_compatible_p(typeof(*l), struct mutex) ? \
+        mutex_is_locked((struct mutex *)l) : \
+        __builtin_types_compatible_p(typeof(*l), spinlock_t) ? \
+        spin_is_locked((spinlock_t *)l) : \
+        0; })
+#define lockdep_assert_held(l) do { WARN_ON(!__lockdep_check(l)); } while (0)
+#define lockdep_assert_held_once(l) do { WARN_ON_ONCE(!__lockdep_check(l)); } while (0)
+#else
 #define lockdep_assert_held(l)                 do { (void)(l); } while (0)
 #define lockdep_assert_held_once(l)            do { (void)(l); } while (0)
+#endif
 
 #define lockdep_recursing(tsk)                 (0)
 
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 8c15b29d5adc..9f96341f8773 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1025,6 +1025,17 @@ config LOCKDEP
        select KALLSYMS
        select KALLSYMS_ALL
 
+config POOR_MANS_LOCKDEP
+       bool "Lock debugging: cheap warning upon forgotten locks"
+       depends on !LOCKDEP
+       default n
+       help
+        This feature enables the kernel to WARN if it detects an inconsistency
+        in the locking. It is vastly less capable than PROVE_LOCKING but only
+        has a small runtime penalty. It may be useful for situations where
+        the overhead of lockdep is too great, but information can be gleaned
+        from the noise of enabling a WARN widescale.
+
 config LOCK_STAT
        bool "Lock usage statistics"
        depends on DEBUG_KERNEL && TRACE_IRQFLAGS_SUPPORT && STACKTRACE_SUPPORT && LOCKDEP_SUPPORT


into something that may fly upstream? Then we do get accurate lockdep
warnings, and we could even enable the WARNs by distro-default (instead of
lockdep).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 08/12] drm/i915: Fix struct mutex vs. RPS lock inversion

[Chris Wilson]

On Tue, Feb 02, 2016 at 02:46:58PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> RPS lock must be taken before the struct_mutex to avoid
> locking inversion. So stop grabbing it for the whole
> powersave initialization and instead only take it during
> the sections which need it.
> 
> Also, struct_mutex is not needed any more since dedicated
> RPS lock was added in:
> 
>    commit 4fc688ce79772496503d22263d61b071a8fb596e
>    Author: Jesse Barnes <jbarnes@virtuousgeek.org>
>    Date:   Fri Nov 2 11:14:01 2012 -0700
> 
>        drm/i915: protect RPS/RC6 related accesses (including PCU) with a new mutex
> 
> Based on prototype patch by Chris Wilson and a subsequent
> mailing list discussion involving Ville, Imre, Chris and
> Daniel.
> 
> v2: More details in the commit.
> 
> v3: Use drm_gem_object_unreference_unlocked. (Chris Wilson)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 07/12] drm/i915: GEM operations need to be done under the big lock

[Chris Wilson]

On Tue, Feb 02, 2016 at 11:06:25AM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> VMA creation and GEM list management need the big lock.
> 
> v2:
> 
> Mutex unlock ended on the wrong path somehow. (0-day, Julia Lawall)
> 
> Not to mention drm_gem_object_unreference was there in existing
> code with no mutex held.
> 
> v3:
> 
> Some callers of i915_gem_object_create_stolen_for_preallocated
> already hold the lock so move the mutex into the other caller
> as well.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 07/12] drm/i915: GEM operations need to be done under the big lock

[Chris Wilson]

On Tue, Feb 02, 2016 at 02:46:19PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> VMA creation and GEM list management need the big lock.
> 
> v2:
> 
> Mutex unlock ended on the wrong path somehow. (0-day, Julia Lawall)
> 
> Not to mention drm_gem_object_unreference was there in existing
> code with no mutex held.
> 
> v3:
> 
> Some callers of i915_gem_object_create_stolen_for_preallocated
> already hold the lock so move the mutex into the other caller
> as well.
> 
> v4:
> 
> Changed to lockdep_assert_held. (Chris Wilson)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>

Also
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
:)

Daniel, what did you think of having a config option to turn
lockdep_assert_held into WARN_ON(!mutex_is_locked()) when lockdep is not
fully-enabled?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 07/12] drm/i915: GEM operations need to be done under the big lock

[Daniel Vetter]

On Thu, Feb 11, 2016 at 10:13:30AM +0000, Chris Wilson wrote:
> On Tue, Feb 02, 2016 at 02:46:19PM +0000, Tvrtko Ursulin wrote:
> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> > VMA creation and GEM list management need the big lock.
> > 
> > v2:
> > 
> > Mutex unlock ended on the wrong path somehow. (0-day, Julia Lawall)
> > 
> > Not to mention drm_gem_object_unreference was there in existing
> > code with no mutex held.
> > 
> > v3:
> > 
> > Some callers of i915_gem_object_create_stolen_for_preallocated
> > already hold the lock so move the mutex into the other caller
> > as well.
> > 
> > v4:
> > 
> > Changed to lockdep_assert_held. (Chris Wilson)
> > 
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Also
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> :)
> 
> Daniel, what did you think of having a config option to turn
> lockdep_assert_held into WARN_ON(!mutex_is_locked()) when lockdep is not
> fully-enabled?

I like, and kinda would even go as far as making it the default. But that
might upset some performance concious folks, so Kconfig seems fine. Cany
you please submit that to a wider audience? Then we'd never have to have
this discussion again ;-)

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx