[PATCH 00/14] drm-intel-collector - update

[PATCH 00/14] drm-intel-collector - update

[Rodrigo Vivi]

This is another drm-intel-collector updated notice:
http://cgit.freedesktop.org/~vivijim/drm-intel/log/?h=drm-intel-collector

Here goes the update list in order for better reviewers assignment:

Patch     drm/i915: Bring UP Power Wells before disabling RC6. - Reviewer:
Patch     drm/i915: Add support for stealing purgable stolen pages - Reviewer:
Patch     drm/i915: Do not allow a pending forcewake put to unbalance across reset - Reviewer:
Patch     drm/i915: Don't save/restore RS when not used - Reviewer:
Patch     drm/i915: add support for Z-order of planes - Reviewer:
Patch     drm/i915: Validate VBT header before trusting it - Reviewer:
Patch     drm/i915: Validate BDB section before reading - Reviewer:
Patch     drm/i915: Upgrade execbuffer fail after resume failure to EIO - Reviewer:
Patch     drm/i915: Add property to set HDMI aspect ratio - Reviewer:
Patch     drm/i915: Prevent context obj from being corrupted - Reviewer:
Patch     drm/i915/bdw: Add WT caching ability - Reviewer:
Patch     drm/i915/bdw: enable eDRAM. - Reviewer:
Patch     drm/i915/bdw: Disable idle DOP clock gating - Reviewer:
Patch     drm/i915: honour forced connector modes - Reviewer:

Overall Process:

drm-intel-collector - review request
 1. Daniel pushs drm-intel-testing (every 2 weeks)
 2. I rebase drm-intel-collector onto drm-intel-testing
 3. Add Reviewer: tag with voluntered reviewers. If you don't believe you should be assigned on a particular patch please don't get mad just tell you wont review or volunteer someone else.
 4. I resubmit remaining patches for review without picking any new (drm-intel-collector - review request)

drm-intel-collector - updated
 5. One week later I collect all simple (1-2) patches that wasn't yet reviewed and not queued by Daniel from one testing update until another.
 6. Request automated QA's PRTS automated i-g-t tests comparing drm-intel-testing x drm-intel-collector
 7. If tests are ok I send the update notification or the patches as a series to intel-gfx mailing list for better tracking and to be reviewed. (drm-intel-collector - updated)
 8. Let me know volunteers for review new patches and also let me know if I've picked any patch that I shouldn't.

There are some reasons that some patches can be left behind:
1. Your patch didn't applied cleanly and I couldn't easily solve the conflicts.
2. Kernel didn't compiled with your patch.
3. I simply missed it. If you believe this is the case please warn me.

Please help me to get these patches reviewed and queued by Daniel.

Also, please let me know if you have further ideas how to improve this process.

Thanks in advance,
Rodrigo.


Ben Widawsky (5):
  drm/i915: Don't save/restore RS when not used
  drm/i915: Prevent context obj from being corrupted
  drm/i915/bdw: Add WT caching ability
  drm/i915/bdw: enable eDRAM.
  drm/i915/bdw: Disable idle DOP clock gating

Chris Wilson (6):
  drm/i915: Add support for stealing purgable stolen pages
  drm/i915: Do not allow a pending forcewake put to unbalance across
    reset
  drm/i915: Validate VBT header before trusting it
  drm/i915: Validate BDB section before reading
  drm/i915: Upgrade execbuffer fail after resume failure to EIO
  drm/i915: honour forced connector modes

Deepak S (1):
  drm/i915: Bring UP Power Wells before disabling RC6.

Vandana Kannan (1):
  drm/i915: Add property to set HDMI aspect ratio

Yu(Alex) Dai (1):
  drm/i915: add support for Z-order of planes

 drivers/gpu/drm/i915/i915_drv.h            |  13 ++--
 drivers/gpu/drm/i915/i915_gem_context.c    |  68 +++++++++++++++--
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  14 +++-
 drivers/gpu/drm/i915/i915_gem_gtt.c        |  17 ++++-
 drivers/gpu/drm/i915/i915_gem_stolen.c     | 119 ++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/i915_reg.h            |   5 +-
 drivers/gpu/drm/i915/intel_bios.c          |  76 +++++++++++++-----
 drivers/gpu/drm/i915/intel_display.c       |  58 +++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h           |   8 ++
 drivers/gpu/drm/i915/intel_fbdev.c         |  15 +++-
 drivers/gpu/drm/i915/intel_hdmi.c          |  12 +++
 drivers/gpu/drm/i915/intel_modes.c         |  28 +++++++
 drivers/gpu/drm/i915/intel_pm.c            |  10 +++
 drivers/gpu/drm/i915/intel_sprite.c        | 119 ++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_uncore.c        |   7 +-
 include/uapi/drm/i915_drm.h                |   6 ++
 16 files changed, 517 insertions(+), 58 deletions(-)

-- 
1.8.3.1

[PATCH 01/14] drm/i915: Bring UP Power Wells before disabling RC6.

[Rodrigo Vivi]

From: Deepak S <deepak.s@intel.com>

We need do forcewake before Disabling RC6, This is what the BIOS
expects while going into suspend.

v2: updated commit message. (Daniel)

Signed-off-by: Deepak S <deepak.s@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 75c1c76..a66000c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3243,8 +3243,14 @@ static void valleyview_disable_rps(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	/* we're doing forcewake before Disabling RC6,
+	 * This what the BIOS expects when going into suspend */
+	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
+
 	I915_WRITE(GEN6_RC_CONTROL, 0);
 
+	gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
+
 	gen6_disable_rps_interrupts(dev);
 }
 
-- 
1.8.3.1

[PATCH 02/14] drm/i915: Add support for stealing purgable stolen pages

[Rodrigo Vivi]

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

If we run out of stolen memory when trying to allocate an object, see if
we can reap enough purgeable objects to free up enough contiguous free
space for the allocation. This is in principle very much like evicting
objects to free up enough contiguous space in the vma when binding
a new object - and you will be forgiven for thinking that the code looks
very similar.

At the moment, we do not allow userspace to allocate objects in stolen,
so there is neither the memory pressure to trigger stolen eviction nor
any purgeable objects inside the stolen arena. However, this will change
in the near future, and so better management and defragmentation of
stolen memory will become a real issue.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: "Gupta, Sourab" <sourab.gupta@intel.com>
Cc: "Goel, Akash" <akash.goel@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 119 ++++++++++++++++++++++++++++++---
 1 file changed, 108 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 62ef55b..1fc1986 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -329,18 +329,25 @@ cleanup:
 	return NULL;
 }
 
-struct drm_i915_gem_object *
-i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
+static bool mark_free(struct drm_i915_gem_object *obj, struct list_head *unwind)
+{
+	if (obj->stolen == NULL)
+		return false;
+
+	if (obj->madv != I915_MADV_DONTNEED)
+		return false;
+
+	list_add(&obj->obj_exec_link, unwind);
+	return drm_mm_scan_add_block(obj->stolen);
+}
+
+static struct drm_mm_node *stolen_alloc(struct drm_i915_private *dev_priv, u32 size)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_i915_gem_object *obj;
 	struct drm_mm_node *stolen;
+	struct drm_i915_gem_object *obj;
+	struct list_head unwind, evict;
 	int ret;
 
-	if (!drm_mm_initialized(&dev_priv->mm.stolen))
-		return NULL;
-
-	DRM_DEBUG_KMS("creating stolen object: size=%x\n", size);
 	if (size == 0)
 		return NULL;
 
@@ -350,11 +357,101 @@ i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
 
 	ret = drm_mm_insert_node(&dev_priv->mm.stolen, stolen, size,
 				 4096, DRM_MM_SEARCH_DEFAULT);
-	if (ret) {
-		kfree(stolen);
-		return NULL;
+	if (ret == 0)
+		return stolen;
+
+	/* No more stolen memory available, or too fragmented.
+	 * Try evicting purgeable objects and search again.
+	 */
+
+	drm_mm_init_scan(&dev_priv->mm.stolen, size, 4096, 0);
+	INIT_LIST_HEAD(&unwind);
+
+	list_for_each_entry(obj, &dev_priv->mm.unbound_list, global_list)
+		if (mark_free(obj, &unwind))
+			goto found;
+
+	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list)
+		if (mark_free(obj, &unwind))
+			goto found;
+
+found:
+	INIT_LIST_HEAD(&evict);
+	while (!list_empty(&unwind)) {
+		obj = list_first_entry(&unwind,
+				       struct drm_i915_gem_object,
+				       obj_exec_link);
+		list_del_init(&obj->obj_exec_link);
+
+		if (drm_mm_scan_remove_block(obj->stolen)) {
+			list_add(&obj->obj_exec_link, &evict);
+			drm_gem_object_reference(&obj->base);
+		}
 	}
 
+	ret = 0;
+	while (!list_empty(&evict)) {
+		obj = list_first_entry(&evict,
+				       struct drm_i915_gem_object,
+				       obj_exec_link);
+		list_del_init(&obj->obj_exec_link);
+
+		if (ret == 0) {
+			struct i915_vma *vma, *vma_next;
+
+			list_for_each_entry_safe(vma, vma_next,
+						 &obj->vma_list,
+						 vma_link)
+				if (i915_vma_unbind(vma))
+					break;
+
+			/* Stolen pins its pages to prevent the
+			 * normal shrinker from processing stolen
+			 * objects.
+			 */
+			i915_gem_object_unpin_pages(obj);
+
+			ret = i915_gem_object_put_pages(obj);
+			if (ret == 0) {
+				obj->madv = __I915_MADV_PURGED;
+
+				kfree(obj->stolen);
+				obj->stolen = NULL;
+			} else
+				i915_gem_object_pin_pages(obj);
+		}
+
+		drm_gem_object_unreference(&obj->base);
+	}
+
+	if (ret == 0)
+		ret = drm_mm_insert_node(&dev_priv->mm.stolen, stolen, size,
+					 4096, DRM_MM_SEARCH_DEFAULT);
+	if (ret == 0)
+		return stolen;
+
+	kfree(stolen);
+	return NULL;
+}
+
+struct drm_i915_gem_object *
+i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_gem_object *obj;
+	struct drm_mm_node *stolen;
+
+	lockdep_assert_held(&dev->struct_mutex);
+
+	if (!drm_mm_initialized(&dev_priv->mm.stolen))
+		return NULL;
+
+	DRM_DEBUG_KMS("creating stolen object: size=%x\n", size);
+
+	stolen = stolen_alloc(dev_priv, size);
+	if (stolen == NULL)
+		return NULL;
+
 	obj = _i915_gem_object_create_stolen(dev, stolen);
 	if (obj)
 		return obj;
-- 
1.8.3.1

[PATCH 03/14] drm/i915: Do not allow a pending forcewake put to unbalance across reset

[Rodrigo Vivi]

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

Across a device reset, we try to restore and user forcewake reference
counts. This is complicated by our deferred forcewake put adding an
extra reference, that may or may not be flushed when we call
del_timer_sync. So we have to take that pending reference into account
when restoring the forcewake.

Reported-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 2a72bab..c8969e3 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -317,8 +317,9 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	unsigned long irqflags;
+	int pending;
 
-	del_timer_sync(&dev_priv->uncore.force_wake_timer);
+	pending = del_timer_sync(&dev_priv->uncore.force_wake_timer);
 
 	/* Hold uncore.lock across reset to prevent any register access
 	 * with forcewake not set correctly
@@ -336,6 +337,8 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
 	if (restore) { /* If reset with a user forcewake, try to restore */
 		unsigned fw = 0;
 
+		dev_priv->uncore.forcewake_count -= pending;
+
 		if (IS_VALLEYVIEW(dev)) {
 			if (dev_priv->uncore.fw_rendercount)
 				fw |= FORCEWAKE_RENDER;
-- 
1.8.3.1

[PATCH 04/14] drm/i915: Don't save/restore RS when not used

[Rodrigo Vivi]

From: Ben Widawsky <benjamin.widawsky@intel.com>

Cc: Kenneth Graunke <kenneth@whitecape.org>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index f77b4c1..5474489 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -550,6 +550,7 @@ mi_set_context(struct intel_ring_buffer *ring,
 	       struct i915_hw_context *new_context,
 	       u32 hw_flags)
 {
+	u32 flags = hw_flags | MI_MM_SPACE_GTT;
 	int ret;
 
 	/* w/a: If Flush TLB Invalidation Mode is enabled, driver must do a TLB
@@ -563,6 +564,10 @@ mi_set_context(struct intel_ring_buffer *ring,
 			return ret;
 	}
 
+	/* These flags are for resource streamer on HSW+ */
+	if (!IS_HASWELL(ring->dev) && INTEL_INFO(ring->dev)->gen < 8)
+		flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
+
 	ret = intel_ring_begin(ring, 6);
 	if (ret)
 		return ret;
@@ -575,11 +580,8 @@ mi_set_context(struct intel_ring_buffer *ring,
 
 	intel_ring_emit(ring, MI_NOOP);
 	intel_ring_emit(ring, MI_SET_CONTEXT);
-	intel_ring_emit(ring, i915_gem_obj_ggtt_offset(new_context->obj) |
-			MI_MM_SPACE_GTT |
-			MI_SAVE_EXT_STATE_EN |
-			MI_RESTORE_EXT_STATE_EN |
-			hw_flags);
+	intel_ring_emit(ring,
+			i915_gem_obj_ggtt_offset(new_context->obj) | flags);
 	/*
 	 * w/a: MI_SET_CONTEXT must always be followed by MI_NOOP
 	 * WaMiSetContext_Hang:snb,ivb,vlv
-- 
1.8.3.1

[PATCH 05/14] drm/i915: add support for Z-order of planes

[Rodrigo Vivi]

From: "Yu(Alex) Dai" <yu.dai@intel.com>

Add "zorder" property to crtc to control Z-order of sprite and
primary planes. The plane tag is packed into the 64 bits value
in the order of from bottom (lower bits) to top (higher bits).
Four bits are used for each plane tag. It can support up to 16
planes.

The alpha channel of the plane is set during Z-order change. If
a plane is put to the bottom, its alpha channel is off. Otherwise,
its alpha channel is on.

Signed-off-by: Yu(Alex) Dai <yu.dai@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |   1 +
 drivers/gpu/drm/i915/i915_reg.h      |   3 +
 drivers/gpu/drm/i915/intel_display.c |  58 +++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h     |   6 ++
 drivers/gpu/drm/i915/intel_sprite.c  | 119 ++++++++++++++++++++++++++++++++++-
 include/uapi/drm/i915_drm.h          |   6 ++
 6 files changed, 186 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7d6acb4..6e14de9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2516,6 +2516,7 @@ int i915_reg_read_ioctl(struct drm_device *dev, void *data,
 			struct drm_file *file);
 int i915_get_reset_stats_ioctl(struct drm_device *dev, void *data,
 			       struct drm_file *file);
+extern int i915_set_plane_zorder(struct drm_crtc *crtc, uint64_t zorder);
 
 /* overlay */
 extern struct intel_overlay_error_state *intel_overlay_capture_error_state(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8f84555..8c382a5 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3881,6 +3881,9 @@ enum punit_power_well {
 #define   SPRITE_INT_GAMMA_ENABLE	(1<<13)
 #define   SPRITE_TILED			(1<<10)
 #define   SPRITE_DEST_KEY		(1<<2)
+#define   SPRITE_FORCE_BOTTOM		(1<<2)
+#define   SPRITE_ZORDER_ENABLE		(1<<0)
+
 #define _SPRA_LINOFF		0x70284
 #define _SPRA_STRIDE		0x70288
 #define _SPRA_POS		0x7028c
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index cd68a24..d245cca 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2169,18 +2169,27 @@ static int i9xx_update_primary_plane(struct drm_crtc *crtc,
 		break;
 	case DRM_FORMAT_XRGB1555:
 	case DRM_FORMAT_ARGB1555:
-		dspcntr |= DISPPLANE_BGRX555;
+		if (intel_crtc->primary_alpha)
+			dspcntr |= DISPPLANE_BGRA555;
+		else
+			dspcntr |= DISPPLANE_BGRX555;
 		break;
 	case DRM_FORMAT_RGB565:
 		dspcntr |= DISPPLANE_BGRX565;
 		break;
 	case DRM_FORMAT_XRGB8888:
 	case DRM_FORMAT_ARGB8888:
-		dspcntr |= DISPPLANE_BGRX888;
+		if (intel_crtc->primary_alpha)
+			dspcntr |= DISPPLANE_BGRA888;
+		else
+			dspcntr |= DISPPLANE_BGRX888;
 		break;
 	case DRM_FORMAT_XBGR8888:
 	case DRM_FORMAT_ABGR8888:
-		dspcntr |= DISPPLANE_RGBX888;
+		if (intel_crtc->primary_alpha)
+			dspcntr |= DISPPLANE_RGBA888;
+		else
+			dspcntr |= DISPPLANE_RGBX888;
 		break;
 	case DRM_FORMAT_XRGB2101010:
 	case DRM_FORMAT_ARGB2101010:
@@ -2188,7 +2197,10 @@ static int i9xx_update_primary_plane(struct drm_crtc *crtc,
 		break;
 	case DRM_FORMAT_XBGR2101010:
 	case DRM_FORMAT_ABGR2101010:
-		dspcntr |= DISPPLANE_RGBX101010;
+		if (intel_crtc->primary_alpha)
+			dspcntr |= DISPPLANE_RGBA101010;
+		else
+			dspcntr |= DISPPLANE_RGBX101010;
 		break;
 	default:
 		BUG();
@@ -8470,6 +8482,9 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
 
 	intel_crtc_cursor_set(crtc, NULL, 0, 0, 0);
 
+	if (intel_crtc->zorder_property)
+		drm_property_destroy(dev, intel_crtc->zorder_property);
+
 	drm_crtc_cleanup(crtc);
 
 	kfree(intel_crtc);
@@ -10422,6 +10437,27 @@ out_config:
 	return ret;
 }
 
+static int intel_crtc_set_property(struct drm_crtc *crtc,
+				   struct drm_property *prop,
+				   uint64_t val)
+{
+	struct intel_crtc *icrtc = to_intel_crtc(crtc);
+	int ret = -ENOENT;
+
+	if (prop && prop == icrtc->zorder_property) {
+		/* Simply return if no change in zorder */
+		if (icrtc->zorder == val)
+			return 0;
+
+		ret = i915_set_plane_zorder(crtc, val);
+
+		if (!ret)
+			icrtc->zorder = val;
+	}
+
+	return ret;
+}
+
 static const struct drm_crtc_funcs intel_crtc_funcs = {
 	.cursor_set = intel_crtc_cursor_set,
 	.cursor_move = intel_crtc_cursor_move,
@@ -10429,6 +10465,7 @@ static const struct drm_crtc_funcs intel_crtc_funcs = {
 	.set_config = intel_crtc_set_config,
 	.destroy = intel_crtc_destroy,
 	.page_flip = intel_crtc_page_flip,
+	.set_property = intel_crtc_set_property,
 };
 
 static void intel_cpu_pll_init(struct drm_device *dev)
@@ -10536,6 +10573,7 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc;
+	struct drm_property *prop = 0;
 	int i;
 
 	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
@@ -10568,6 +10606,18 @@ static void intel_crtc_init(struct drm_device *dev, int pipe)
 	dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base;
 
 	drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs);
+
+	intel_crtc->primary_alpha = false;
+	intel_crtc->sprite0_alpha = true;
+	intel_crtc->sprite1_alpha = true;
+
+	if (IS_VALLEYVIEW(dev))
+		prop = drm_property_create(dev, 0, "zorder", 1);
+
+	if (prop)
+		drm_object_attach_property(&intel_crtc->base.base, prop, 0);
+
+	intel_crtc->zorder_property = prop;
 }
 
 enum pipe intel_get_pipe_from_connector(struct intel_connector *connector)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c551472..c684427 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -387,6 +387,9 @@ struct intel_crtc {
 	bool new_enabled;
 
 	uint32_t ddi_pll_sel;
+	bool primary_alpha;
+	bool sprite0_alpha;
+	bool sprite1_alpha;
 
 	/* reset counter value when the last flip was submitted */
 	unsigned int reset_counter;
@@ -400,6 +403,9 @@ struct intel_crtc {
 		/* watermarks currently being used  */
 		struct intel_pipe_wm active;
 	} wm;
+
+	struct drm_property *zorder_property;
+	uint64_t zorder;
 };
 
 struct intel_plane_wm_parameters {
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 336ae6c..fb495d9 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -37,6 +37,101 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 
+static int vlv_set_plane_zorder(struct drm_crtc *crtc, uint64_t order)
+{
+	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
+	u32 val;
+	int s1_zorder = 0, s1_bottom = 0, s2_zorder = 0, s2_bottom = 0;
+	unsigned int i, z_order[I915_PLANE_MAX_NUM];
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	int pipe = intel_crtc->pipe;
+
+	/* Get plane order from bottom (lower bits) to top (higher bits) */
+	for (i = 0; i < I915_PLANE_MAX_NUM; i++)
+		z_order[i] = (order >> (i*4)) & 0xF;
+
+	/* The top layer must be cursor */
+	if (z_order[i-1] != I915_PLANE_CURSOR)
+		return -EINVAL;
+
+	/* No duplicated setting */
+	if (z_order[0] == z_order[1] ||
+		z_order[0] == z_order[2] ||
+		z_order[1] == z_order[2])
+		return -EINVAL;
+
+	switch (z_order[0]) {
+	case I915_PLANE_PRIMARY:
+		if (z_order[1] == I915_PLANE_SPRITE_B)
+			s1_zorder = 1;
+		break;
+	case I915_PLANE_SPRITE_A:
+		s1_bottom = 1;
+		if (z_order[1] == I915_PLANE_SPRITE_B)
+			s2_zorder = 1;
+		break;
+	case I915_PLANE_SPRITE_B:
+		s2_bottom = 1;
+		if (z_order[1] == I915_PLANE_SPRITE_A)
+			s1_zorder = 1;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* Clear the older Z-order */
+	val = I915_READ(SPCNTR(pipe, 0));
+	val &= ~(SPRITE_FORCE_BOTTOM | SPRITE_ZORDER_ENABLE);
+	I915_WRITE(SPCNTR(pipe, 0), val);
+
+	val = I915_READ(SPCNTR(pipe, 1));
+	val &= ~(SPRITE_FORCE_BOTTOM | SPRITE_ZORDER_ENABLE);
+	I915_WRITE(SPCNTR(pipe, 1), val);
+
+	/* Program new Z-order */
+	val = I915_READ(SPCNTR(pipe, 0));
+	if (s1_zorder)
+		val |= SPRITE_ZORDER_ENABLE;
+	if (s1_bottom)
+		val |= SPRITE_FORCE_BOTTOM;
+	I915_WRITE(SPCNTR(pipe, 0), val);
+
+	val = I915_READ(SPCNTR(pipe, 1));
+	if (s2_zorder)
+		val |= SPRITE_ZORDER_ENABLE;
+	if (s2_bottom)
+		val |= SPRITE_FORCE_BOTTOM;
+	I915_WRITE(SPCNTR(pipe, 1), val);
+
+	/* If a plane is not at bottom, the alpha channel should be on. */
+	if (z_order[0] != I915_PLANE_PRIMARY)
+		intel_crtc->primary_alpha = true;
+	else
+		intel_crtc->primary_alpha = false;
+
+	if (z_order[0] != I915_PLANE_SPRITE_A)
+		intel_crtc->sprite0_alpha = true;
+	else
+		intel_crtc->sprite0_alpha = false;
+
+	if (z_order[0] != I915_PLANE_SPRITE_B)
+		intel_crtc->sprite1_alpha = true;
+	else
+		intel_crtc->sprite1_alpha = false;
+
+	return 0;
+}
+
+int i915_set_plane_zorder(struct drm_crtc *crtc, uint64_t val)
+{
+	int ret = -ENOENT;
+
+	if (IS_VALLEYVIEW(crtc->dev))
+		ret = vlv_set_plane_zorder(crtc, val);
+
+	return ret;
+}
+
 static void
 vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 		 struct drm_framebuffer *fb,
@@ -50,10 +145,19 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 	struct intel_plane *intel_plane = to_intel_plane(dplane);
 	int pipe = intel_plane->pipe;
 	int plane = intel_plane->plane;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	u32 sprctl;
+	bool alpha = true;
 	unsigned long sprsurf_offset, linear_offset;
 	int pixel_size = drm_format_plane_cpp(fb->pixel_format, 0);
 
+	if (plane && intel_crtc->sprite1_alpha)
+		alpha = true;
+	else if (!plane && intel_crtc->sprite0_alpha)
+		alpha = true;
+	else
+		alpha = false;
+
 	sprctl = I915_READ(SPCNTR(pipe, plane));
 
 	/* Mask out pixel format bits in case we change it */
@@ -81,19 +185,28 @@ vlv_update_plane(struct drm_plane *dplane, struct drm_crtc *crtc,
 		sprctl |= SP_FORMAT_BGRX8888;
 		break;
 	case DRM_FORMAT_ARGB8888:
-		sprctl |= SP_FORMAT_BGRA8888;
+		if (alpha)
+			sprctl |= SP_FORMAT_BGRA8888;
+		else
+			sprctl |= SP_FORMAT_BGRX8888;
 		break;
 	case DRM_FORMAT_XBGR2101010:
 		sprctl |= SP_FORMAT_RGBX1010102;
 		break;
 	case DRM_FORMAT_ABGR2101010:
-		sprctl |= SP_FORMAT_RGBA1010102;
+		if (alpha)
+			sprctl |= SP_FORMAT_RGBA1010102;
+		else
+			sprctl |= SP_FORMAT_RGBX1010102;
 		break;
 	case DRM_FORMAT_XBGR8888:
 		sprctl |= SP_FORMAT_RGBX8888;
 		break;
 	case DRM_FORMAT_ABGR8888:
-		sprctl |= SP_FORMAT_RGBA8888;
+		if (alpha)
+			sprctl |= SP_FORMAT_RGBA8888;
+		else
+			sprctl |= SP_FORMAT_RGBX8888;
 		break;
 	default:
 		/*
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 8a3e4ef00..3f6eb90 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1050,4 +1050,10 @@ struct drm_i915_reset_stats {
 	__u32 pad;
 };
 
+#define I915_PLANE_PRIMARY		0
+#define I915_PLANE_SPRITE_A		1
+#define I915_PLANE_SPRITE_B		2
+#define I915_PLANE_CURSOR		3
+#define I915_PLANE_MAX_NUM		(I915_PLANE_CURSOR + 1)
+
 #endif /* _UAPI_I915_DRM_H_ */
-- 
1.8.3.1

[PATCH 06/14] drm/i915: Validate VBT header before trusting it

[Rodrigo Vivi]

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

Be we read and chase pointers from the VBT, it is prudent to make sure
that those accesses are wholly contained within the MMIO region, or else
we may cause a kernel panic during boot.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/intel_bios.c | 68 ++++++++++++++++++++++++++++-----------
 1 file changed, 50 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index fba9efd..fc9e806 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -1099,6 +1099,46 @@ static const struct dmi_system_id intel_no_opregion_vbt[] = {
 	{ }
 };
 
+static struct bdb_header *validate_vbt(char *base, size_t size,
+				       struct vbt_header *vbt,
+				       const char *source)
+{
+	size_t offset;
+	struct bdb_header *bdb;
+
+	if (vbt == NULL) {
+		DRM_DEBUG_DRIVER("VBT signature missing\n");
+		return NULL;
+	}
+
+	offset = (char *)vbt - base;
+	if (offset + sizeof(struct vbt_header) > size) {
+		DRM_DEBUG_DRIVER("VBT header incomplete\n");
+		return NULL;
+	}
+
+	if (memcmp(vbt->signature, "$VBT", 4)) {
+		DRM_DEBUG_DRIVER("VBT invalid signature\n");
+		return NULL;
+	}
+
+	offset += vbt->bdb_offset;
+	if (offset + sizeof(struct bdb_header) > size) {
+		DRM_DEBUG_DRIVER("BDB header incomplete\n");
+		return NULL;
+	}
+
+	bdb = (struct bdb_header *)(base + offset);
+	if (offset + bdb->bdb_size > size) {
+		DRM_DEBUG_DRIVER("BDB incomplete\n");
+		return NULL;
+	}
+
+	DRM_DEBUG_KMS("Using VBT from %s: %20s\n",
+		      source, vbt->signature);
+	return bdb;
+}
+
 /**
  * intel_parse_bios - find VBT and initialize settings from the BIOS
  * @dev: DRM device
@@ -1122,20 +1162,13 @@ intel_parse_bios(struct drm_device *dev)
 	init_vbt_defaults(dev_priv);
 
 	/* XXX Should this validation be moved to intel_opregion.c? */
-	if (!dmi_check_system(intel_no_opregion_vbt) && dev_priv->opregion.vbt) {
-		struct vbt_header *vbt = dev_priv->opregion.vbt;
-		if (memcmp(vbt->signature, "$VBT", 4) == 0) {
-			DRM_DEBUG_KMS("Using VBT from OpRegion: %20s\n",
-					 vbt->signature);
-			bdb = (struct bdb_header *)((char *)vbt + vbt->bdb_offset);
-		} else
-			dev_priv->opregion.vbt = NULL;
-	}
+	if (!dmi_check_system(intel_no_opregion_vbt) && dev_priv->opregion.vbt)
+		bdb = validate_vbt((char *)dev_priv->opregion.header, OPREGION_SIZE,
+				   (struct vbt_header *)dev_priv->opregion.vbt,
+				   "OpRegion");
 
 	if (bdb == NULL) {
-		struct vbt_header *vbt = NULL;
-		size_t size;
-		int i;
+		size_t i, size;
 
 		bios = pci_map_rom(pdev, &size);
 		if (!bios)
@@ -1143,19 +1176,18 @@ intel_parse_bios(struct drm_device *dev)
 
 		/* Scour memory looking for the VBT signature */
 		for (i = 0; i + 4 < size; i++) {
-			if (!memcmp(bios + i, "$VBT", 4)) {
-				vbt = (struct vbt_header *)(bios + i);
+			if (memcmp(bios + i, "$VBT", 4) == 0) {
+				bdb = validate_vbt(bios, size,
+						   (struct vbt_header *)(bios + i),
+						   "PCI ROM");
 				break;
 			}
 		}
 
-		if (!vbt) {
-			DRM_DEBUG_DRIVER("VBT signature missing\n");
+		if (!bdb) {
 			pci_unmap_rom(pdev, bios);
 			return -1;
 		}
-
-		bdb = (struct bdb_header *)(bios + i + vbt->bdb_offset);
 	}
 
 	/* Grab useful general definitions */
-- 
1.8.3.1

Re: [PATCH 06/14] drm/i915: Validate VBT header before trusting it

[Kumar, Shobhit]

On 4/19/2014 2:34 AM, Rodrigo Vivi wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
>
> Be we read and chase pointers from the VBT, it is prudent to make sure
> that those accesses are wholly contained within the MMIO region, or else
> we may cause a kernel panic during boot.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>   drivers/gpu/drm/i915/intel_bios.c | 68 ++++++++++++++++++++++++++++-----------
>   1 file changed, 50 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index fba9efd..fc9e806 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -1099,6 +1099,46 @@ static const struct dmi_system_id intel_no_opregion_vbt[] = {
>   	{ }
>   };
>
> +static struct bdb_header *validate_vbt(char *base, size_t size,
> +				       struct vbt_header *vbt,
> +				       const char *source)
> +{
> +	size_t offset;
> +	struct bdb_header *bdb;
> +
> +	if (vbt == NULL) {
> +		DRM_DEBUG_DRIVER("VBT signature missing\n");
> +		return NULL;
> +	}
> +
> +	offset = (char *)vbt - base;
> +	if (offset + sizeof(struct vbt_header) > size) {
> +		DRM_DEBUG_DRIVER("VBT header incomplete\n");
> +		return NULL;
> +	}
> +
> +	if (memcmp(vbt->signature, "$VBT", 4)) {
> +		DRM_DEBUG_DRIVER("VBT invalid signature\n");
> +		return NULL;
> +	}
> +
> +	offset += vbt->bdb_offset;
> +	if (offset + sizeof(struct bdb_header) > size) {
> +		DRM_DEBUG_DRIVER("BDB header incomplete\n");
> +		return NULL;
> +	}
> +
> +	bdb = (struct bdb_header *)(base + offset);
> +	if (offset + bdb->bdb_size > size) {
> +		DRM_DEBUG_DRIVER("BDB incomplete\n");
> +		return NULL;
> +	}

I know that BDB version check is really not enough and VBT should be 
forward compatible, but it would be good to have a version check in 
driver for the current BDB version the parser supports as well. 
Strictly speaking if we  put this check we should ideally reject any 
newer versions, but putting an error log indicating mismatch might be a 
  good idea for debug.


Regards
Shobhit

Re: [PATCH 06/14] drm/i915: Validate VBT header before trusting it

[Daniel Vetter]

On Thu, Apr 24, 2014 at 09:22:23PM +0530, Kumar, Shobhit wrote:
> On 4/19/2014 2:34 AM, Rodrigo Vivi wrote:
> >From: Chris Wilson <chris@chris-wilson.co.uk>
> >
> >Be we read and chase pointers from the VBT, it is prudent to make sure
> >that those accesses are wholly contained within the MMIO region, or else
> >we may cause a kernel panic during boot.
> >
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> >---
> >  drivers/gpu/drm/i915/intel_bios.c | 68 ++++++++++++++++++++++++++++-----------
> >  1 file changed, 50 insertions(+), 18 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> >index fba9efd..fc9e806 100644
> >--- a/drivers/gpu/drm/i915/intel_bios.c
> >+++ b/drivers/gpu/drm/i915/intel_bios.c
> >@@ -1099,6 +1099,46 @@ static const struct dmi_system_id intel_no_opregion_vbt[] = {
> >  	{ }
> >  };
> >
> >+static struct bdb_header *validate_vbt(char *base, size_t size,
> >+				       struct vbt_header *vbt,
> >+				       const char *source)
> >+{
> >+	size_t offset;
> >+	struct bdb_header *bdb;
> >+
> >+	if (vbt == NULL) {
> >+		DRM_DEBUG_DRIVER("VBT signature missing\n");
> >+		return NULL;
> >+	}
> >+
> >+	offset = (char *)vbt - base;
> >+	if (offset + sizeof(struct vbt_header) > size) {
> >+		DRM_DEBUG_DRIVER("VBT header incomplete\n");
> >+		return NULL;
> >+	}
> >+
> >+	if (memcmp(vbt->signature, "$VBT", 4)) {
> >+		DRM_DEBUG_DRIVER("VBT invalid signature\n");
> >+		return NULL;
> >+	}
> >+
> >+	offset += vbt->bdb_offset;
> >+	if (offset + sizeof(struct bdb_header) > size) {
> >+		DRM_DEBUG_DRIVER("BDB header incomplete\n");
> >+		return NULL;
> >+	}
> >+
> >+	bdb = (struct bdb_header *)(base + offset);
> >+	if (offset + bdb->bdb_size > size) {
> >+		DRM_DEBUG_DRIVER("BDB incomplete\n");
> >+		return NULL;
> >+	}
> 
> I know that BDB version check is really not enough and VBT should be forward
> compatible, but it would be good to have a version check in driver for the
> current BDB version the parser supports as well. Strictly speaking if we
> put this check we should ideally reject any newer versions, but putting an
> error log indicating mismatch might be a  good idea for debug.

Sounds more like an additional patch on top of this one here, so still r-b
from your side for these changes here?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 06/14] drm/i915: Validate VBT header before trusting it

[Kumar, Shobhit]

On 4/25/2014 1:32 PM, Daniel Vetter wrote:
> On Thu, Apr 24, 2014 at 09:22:23PM +0530, Kumar, Shobhit wrote:
>> On 4/19/2014 2:34 AM, Rodrigo Vivi wrote:
>>> From: Chris Wilson <chris@chris-wilson.co.uk>
>>>
>>> Be we read and chase pointers from the VBT, it is prudent to make sure
>>> that those accesses are wholly contained within the MMIO region, or else
>>> we may cause a kernel panic during boot.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_bios.c | 68 ++++++++++++++++++++++++++++-----------
>>>   1 file changed, 50 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
>>> index fba9efd..fc9e806 100644
>>> --- a/drivers/gpu/drm/i915/intel_bios.c
>>> +++ b/drivers/gpu/drm/i915/intel_bios.c
>>> @@ -1099,6 +1099,46 @@ static const struct dmi_system_id intel_no_opregion_vbt[] = {
>>>   	{ }
>>>   };
>>>
>>> +static struct bdb_header *validate_vbt(char *base, size_t size,
>>> +				       struct vbt_header *vbt,
>>> +				       const char *source)
>>> +{
>>> +	size_t offset;
>>> +	struct bdb_header *bdb;
>>> +
>>> +	if (vbt == NULL) {
>>> +		DRM_DEBUG_DRIVER("VBT signature missing\n");
>>> +		return NULL;
>>> +	}
>>> +
>>> +	offset = (char *)vbt - base;
>>> +	if (offset + sizeof(struct vbt_header) > size) {
>>> +		DRM_DEBUG_DRIVER("VBT header incomplete\n");
>>> +		return NULL;
>>> +	}
>>> +
>>> +	if (memcmp(vbt->signature, "$VBT", 4)) {
>>> +		DRM_DEBUG_DRIVER("VBT invalid signature\n");
>>> +		return NULL;
>>> +	}
>>> +
>>> +	offset += vbt->bdb_offset;
>>> +	if (offset + sizeof(struct bdb_header) > size) {
>>> +		DRM_DEBUG_DRIVER("BDB header incomplete\n");
>>> +		return NULL;
>>> +	}
>>> +
>>> +	bdb = (struct bdb_header *)(base + offset);
>>> +	if (offset + bdb->bdb_size > size) {
>>> +		DRM_DEBUG_DRIVER("BDB incomplete\n");
>>> +		return NULL;
>>> +	}
>>
>> I know that BDB version check is really not enough and VBT should be forward
>> compatible, but it would be good to have a version check in driver for the
>> current BDB version the parser supports as well. Strictly speaking if we
>> put this check we should ideally reject any newer versions, but putting an
>> error log indicating mismatch might be a  good idea for debug.
>
> Sounds more like an additional patch on top of this one here, so still r-b
> from your side for these changes here?
> -Daniel
>

Hmm, I felt it is something which is missed as part of this patch while 
validating VBT. But yes if you feel that it is okay to be a separate 
patch then for these changes -

Reviewed-by: Shobhit Kumar <shobhit.kumar@intel.com>

I will then push a new patch on version check.

Regards
Shobhit

Re: [PATCH 06/14] drm/i915: Validate VBT header before trusting it

[Daniel Vetter]

On Fri, Apr 25, 2014 at 01:54:06PM +0530, Kumar, Shobhit wrote:
> On 4/25/2014 1:32 PM, Daniel Vetter wrote:
> >On Thu, Apr 24, 2014 at 09:22:23PM +0530, Kumar, Shobhit wrote:
> >>On 4/19/2014 2:34 AM, Rodrigo Vivi wrote:
> >>>From: Chris Wilson <chris@chris-wilson.co.uk>
> >>>
> >>>Be we read and chase pointers from the VBT, it is prudent to make sure
> >>>that those accesses are wholly contained within the MMIO region, or else
> >>>we may cause a kernel panic during boot.
> >>>
> >>>Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>>Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> >>>---
> >>>  drivers/gpu/drm/i915/intel_bios.c | 68 ++++++++++++++++++++++++++++-----------
> >>>  1 file changed, 50 insertions(+), 18 deletions(-)
> >>>
> >>>diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> >>>index fba9efd..fc9e806 100644
> >>>--- a/drivers/gpu/drm/i915/intel_bios.c
> >>>+++ b/drivers/gpu/drm/i915/intel_bios.c
> >>>@@ -1099,6 +1099,46 @@ static const struct dmi_system_id intel_no_opregion_vbt[] = {
> >>>  	{ }
> >>>  };
> >>>
> >>>+static struct bdb_header *validate_vbt(char *base, size_t size,
> >>>+				       struct vbt_header *vbt,
> >>>+				       const char *source)
> >>>+{
> >>>+	size_t offset;
> >>>+	struct bdb_header *bdb;
> >>>+
> >>>+	if (vbt == NULL) {
> >>>+		DRM_DEBUG_DRIVER("VBT signature missing\n");
> >>>+		return NULL;
> >>>+	}
> >>>+
> >>>+	offset = (char *)vbt - base;
> >>>+	if (offset + sizeof(struct vbt_header) > size) {
> >>>+		DRM_DEBUG_DRIVER("VBT header incomplete\n");
> >>>+		return NULL;
> >>>+	}
> >>>+
> >>>+	if (memcmp(vbt->signature, "$VBT", 4)) {
> >>>+		DRM_DEBUG_DRIVER("VBT invalid signature\n");
> >>>+		return NULL;
> >>>+	}
> >>>+
> >>>+	offset += vbt->bdb_offset;
> >>>+	if (offset + sizeof(struct bdb_header) > size) {
> >>>+		DRM_DEBUG_DRIVER("BDB header incomplete\n");
> >>>+		return NULL;
> >>>+	}
> >>>+
> >>>+	bdb = (struct bdb_header *)(base + offset);
> >>>+	if (offset + bdb->bdb_size > size) {
> >>>+		DRM_DEBUG_DRIVER("BDB incomplete\n");
> >>>+		return NULL;
> >>>+	}
> >>
> >>I know that BDB version check is really not enough and VBT should be forward
> >>compatible, but it would be good to have a version check in driver for the
> >>current BDB version the parser supports as well. Strictly speaking if we
> >>put this check we should ideally reject any newer versions, but putting an
> >>error log indicating mismatch might be a  good idea for debug.
> >
> >Sounds more like an additional patch on top of this one here, so still r-b
> >from your side for these changes here?
> >-Daniel
> >
> 
> Hmm, I felt it is something which is missed as part of this patch while
> validating VBT. But yes if you feel that it is okay to be a separate patch
> then for these changes -
> 
> Reviewed-by: Shobhit Kumar <shobhit.kumar@intel.com>

Thanks, patch merged.

> I will then push a new patch on version check.

I've figured Chris could do it, but if you can take this over I don't
think Chris will complain ;-)

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 06/14] drm/i915: Validate VBT header before trusting it

[Chris Wilson]

On Fri, Apr 25, 2014 at 11:12:06AM +0200, Daniel Vetter wrote:
> On Fri, Apr 25, 2014 at 01:54:06PM +0530, Kumar, Shobhit wrote:
> > On 4/25/2014 1:32 PM, Daniel Vetter wrote:
> > I will then push a new patch on version check.
> 
> I've figured Chris could do it, but if you can take this over I don't
> think Chris will complain ;-)

I hope that our parser is forward-compatabile and the VBT updates
backwards-compatible... 
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 06/14] drm/i915: Validate VBT header before trusting it

[Kumar, Shobhit]

On 4/25/2014 2:58 PM, Chris Wilson wrote:
> On Fri, Apr 25, 2014 at 11:12:06AM +0200, Daniel Vetter wrote:
>> On Fri, Apr 25, 2014 at 01:54:06PM +0530, Kumar, Shobhit wrote:
>>> On 4/25/2014 1:32 PM, Daniel Vetter wrote:
>>> I will then push a new patch on version check.
>>
>> I've figured Chris could do it, but if you can take this over I don't
>> think Chris will complain ;-)
>
> I hope that our parser is forward-compatabile and the VBT updates
> backwards-compatible...

Yeah of-course, it should be like this.

Regards
Shobhit

[PATCH 07/14] drm/i915: Validate BDB section before reading

[Rodrigo Vivi]

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

Make sure that the whole BDB section is within the MMIO region prior to
accessing it contents. That we don't read outside of the secion is left
up to the individual section parsers.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/intel_bios.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index fc9e806..2945f57 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -49,13 +49,19 @@ find_section(struct bdb_header *bdb, int section_id)
 	total = bdb->bdb_size;
 
 	/* walk the sections looking for section_id */
-	while (index < total) {
+	while (index + 3 < total) {
 		current_id = *(base + index);
 		index++;
+
 		current_size = *((u16 *)(base + index));
 		index += 2;
+
+		if (index + current_size > total)
+			return NULL;
+
 		if (current_id == section_id)
 			return base + index;
+
 		index += current_size;
 	}
 
-- 
1.8.3.1

Re: [PATCH 07/14] drm/i915: Validate BDB section before reading

[Kumar, Shobhit]

On 4/19/2014 2:34 AM, Rodrigo Vivi wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
>
> Make sure that the whole BDB section is within the MMIO region prior to
> accessing it contents. That we don't read outside of the secion is left
> up to the individual section parsers.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>   drivers/gpu/drm/i915/intel_bios.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index fc9e806..2945f57 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -49,13 +49,19 @@ find_section(struct bdb_header *bdb, int section_id)
>   	total = bdb->bdb_size;
>
>   	/* walk the sections looking for section_id */
> -	while (index < total) {
> +	while (index + 3 < total) {
>   		current_id = *(base + index);
>   		index++;
> +
>   		current_size = *((u16 *)(base + index));
>   		index += 2;
> +
> +		if (index + current_size > total)
> +			return NULL;
> +
>   		if (current_id == section_id)
>   			return base + index;
> +
>   		index += current_size;
>   	}
>

Reviewed-by: Shobhit Kumar <shobhit.kumar@intel.com>

Re: [PATCH 07/14] drm/i915: Validate BDB section before reading

[Daniel Vetter]

On Thu, Apr 24, 2014 at 09:23:24PM +0530, Kumar, Shobhit wrote:
> On 4/19/2014 2:34 AM, Rodrigo Vivi wrote:
> >From: Chris Wilson <chris@chris-wilson.co.uk>
> >
> >Make sure that the whole BDB section is within the MMIO region prior to
> >accessing it contents. That we don't read outside of the secion is left
> >up to the individual section parsers.
> >
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> >---
> >  drivers/gpu/drm/i915/intel_bios.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> >index fc9e806..2945f57 100644
> >--- a/drivers/gpu/drm/i915/intel_bios.c
> >+++ b/drivers/gpu/drm/i915/intel_bios.c
> >@@ -49,13 +49,19 @@ find_section(struct bdb_header *bdb, int section_id)
> >  	total = bdb->bdb_size;
> >
> >  	/* walk the sections looking for section_id */
> >-	while (index < total) {
> >+	while (index + 3 < total) {
> >  		current_id = *(base + index);
> >  		index++;
> >+
> >  		current_size = *((u16 *)(base + index));
> >  		index += 2;
> >+
> >+		if (index + current_size > total)
> >+			return NULL;
> >+
> >  		if (current_id == section_id)
> >  			return base + index;
> >+
> >  		index += current_size;
> >  	}
> >
> 
> Reviewed-by: Shobhit Kumar <shobhit.kumar@intel.com>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 08/14] drm/i915: Upgrade execbuffer fail after resume failure to EIO

[Rodrigo Vivi]

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

If we try to execute on a known ring, but it has failed to be
initialised correctly, report that the GPU is hung rather than the
command invalid. This leaves us reporting EINVAL only if the user
requests execution on a ring that is not supported by the device.

This should prevent UXA from getting stuck in a null render loop after a
failed resume.

Reported-by: Jiri Kosina <jikos@jikos.cz>
References: https://bugs.freedesktop.org/show_bug.cgi?id=76554
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 0ec8621..d9a54a1 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -999,6 +999,18 @@ i915_reset_gen7_sol_offsets(struct drm_device *dev,
 	return 0;
 }
 
+static bool
+intel_ring_valid(struct intel_ring_buffer *ring)
+{
+	switch (ring->id) {
+	case RCS: return true;
+	case VCS: return HAS_BSD(ring->dev);
+	case BCS: return HAS_BLT(ring->dev);
+	case VECS: return HAS_VEBOX(ring->dev);
+	default: return false;
+	}
+}
+
 static int
 i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 		       struct drm_file *file,
@@ -1049,7 +1061,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 	if (!intel_ring_initialized(ring)) {
 		DRM_DEBUG("execbuf with invalid ring: %d\n",
 			  (int)(args->flags & I915_EXEC_RING_MASK));
-		return -EINVAL;
+		return intel_ring_valid(ring) ? -EIO : -EINVAL;
 	}
 
 	mode = args->flags & I915_EXEC_CONSTANTS_MASK;
-- 
1.8.3.1

[PATCH 09/14] drm/i915: Add property to set HDMI aspect ratio

[Rodrigo Vivi]

From: Vandana Kannan <vandana.kannan@intel.com>

Added a property to enable user space to set aspect ratio for HDMI displays.
If there is no user specified value, then PAR_NONE/Automatic option is set
by default. User can select aspect ratio 4:3 or 16:9. The aspect ratio
selected by user would come into effect with a mode set.

v2: Daniel's review comments incorporated.
Call for a mode set to update property.

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
Cc: Jesse Barnes <jesse.barnes@intel.com>
Cc: Vijay Purushothaman <vijay.a.purushothaman@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_drv.h    |  1 +
 drivers/gpu/drm/i915/intel_drv.h   |  2 ++
 drivers/gpu/drm/i915/intel_hdmi.c  | 12 ++++++++++++
 drivers/gpu/drm/i915/intel_modes.c | 28 ++++++++++++++++++++++++++++
 4 files changed, 43 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6e14de9..4e81ce1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1440,6 +1440,7 @@ struct drm_i915_private {
 
 	struct drm_property *broadcast_rgb_property;
 	struct drm_property *force_audio_property;
+	struct drm_property *aspect_ratio_property;
 
 	uint32_t hw_context_size;
 	struct list_head context_list;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c684427..5d3811c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -485,6 +485,7 @@ struct intel_hdmi {
 	bool has_audio;
 	enum hdmi_force_audio force_audio;
 	bool rgb_quant_range_selectable;
+	enum hdmi_picture_aspect aspect_ratio;
 	void (*write_infoframe)(struct drm_encoder *encoder,
 				enum hdmi_infoframe_type type,
 				const void *frame, ssize_t len);
@@ -863,6 +864,7 @@ int intel_connector_update_modes(struct drm_connector *connector,
 int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter *adapter);
 void intel_attach_force_audio_property(struct drm_connector *connector);
 void intel_attach_broadcast_rgb_property(struct drm_connector *connector);
+void intel_attach_aspect_ratio_property(struct drm_connector *connector);
 
 
 /* intel_overlay.c */
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index b606162..adc751d 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -367,6 +367,9 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
 	union hdmi_infoframe frame;
 	int ret;
 
+	/* Set user selected PAR to incoming mode's member */
+	adjusted_mode->picture_aspect_ratio = intel_hdmi->aspect_ratio;
+
 	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
 						       adjusted_mode);
 	if (ret < 0) {
@@ -1105,6 +1108,11 @@ intel_hdmi_set_property(struct drm_connector *connector,
 		goto done;
 	}
 
+	if (property == dev_priv->aspect_ratio_property) {
+		intel_hdmi->aspect_ratio = val;
+		goto done;
+	}
+
 	return -EINVAL;
 
 done:
@@ -1253,6 +1261,7 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c
 {
 	intel_attach_force_audio_property(connector);
 	intel_attach_broadcast_rgb_property(connector);
+	intel_attach_aspect_ratio_property(connector);
 	intel_hdmi->color_range_auto = true;
 }
 
@@ -1317,6 +1326,9 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 		intel_connector->get_hw_state = intel_connector_get_hw_state;
 	intel_connector->unregister = intel_connector_unregister;
 
+	/* Initialize aspect ratio member of intel_hdmi */
+	intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
+
 	intel_hdmi_add_properties(intel_hdmi, connector);
 
 	intel_connector_attach_encoder(intel_connector, intel_encoder);
diff --git a/drivers/gpu/drm/i915/intel_modes.c b/drivers/gpu/drm/i915/intel_modes.c
index 0e860f3..6f814da 100644
--- a/drivers/gpu/drm/i915/intel_modes.c
+++ b/drivers/gpu/drm/i915/intel_modes.c
@@ -126,3 +126,31 @@ intel_attach_broadcast_rgb_property(struct drm_connector *connector)
 
 	drm_object_attach_property(&connector->base, prop, 0);
 }
+
+static const struct drm_prop_enum_list aspect_ratio_names[] = {
+	{ HDMI_PICTURE_ASPECT_NONE, "Automatic" },
+	{ HDMI_PICTURE_ASPECT_4_3, "4:3" },
+	{ HDMI_PICTURE_ASPECT_16_9, "16:9" },
+};
+
+void
+intel_attach_aspect_ratio_property(struct drm_connector *connector)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_property *prop;
+
+	prop = dev_priv->aspect_ratio_property;
+	if (prop == NULL) {
+		prop = drm_property_create_enum(dev, DRM_MODE_PROP_ENUM,
+					   "HDMI aspect ratio",
+					   aspect_ratio_names,
+					   ARRAY_SIZE(aspect_ratio_names));
+		if (prop == NULL)
+			return;
+
+		dev_priv->aspect_ratio_property = prop;
+	}
+
+	drm_object_attach_property(&connector->base, prop, 0);
+}
-- 
1.8.3.1

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

[PATCH 10/14] drm/i915: Prevent context obj from being corrupted

[Rodrigo Vivi]

From: Ben Widawsky <benjamin.widawsky@linux.intel.com>

While the context is not being used, we can make the PTEs invalid, so
nothing can accidentally corrupt it. Systems tend to have a lot of
trouble when the context gets corrupted.

NOTE: This is a slightly different patch than what I posted to Bugzilla.

References: https://bugs.freedesktop.org/show_bug.cgi?id=75724
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 56 +++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h         |  2 +-
 2 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 5474489..b913ef6 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -545,6 +545,58 @@ i915_gem_context_get(struct drm_i915_file_private *file_priv, u32 id)
 	return ctx;
 }
 
+static void
+_ctx_ptes(struct intel_ring_buffer *ring,
+	  struct i915_hw_context *ctx,
+	  bool valid)
+{
+	const size_t ptes  = ctx->obj->base.size >> PAGE_SHIFT;
+	const u32 base = i915_gem_obj_ggtt_offset(ctx->obj);
+	struct sg_page_iter sg_iter;
+	struct i915_address_space *vm = ctx->vm;
+	int i = 0;
+
+	BUG_ON(!i915_gem_obj_is_pinned(ctx->obj));
+
+	if (intel_ring_begin(ring, round_up(ptes * 3, 2))) {
+		DRM_ERROR("Could not protect context object.\n");
+		return;
+	}
+
+	for_each_sg_page(ctx->obj->pages->sgl, &sg_iter, ctx->obj->pages->nents, 0) {
+		u32 pte = vm->pte_encode(sg_page_iter_dma_address(&sg_iter),
+					 ctx->obj->cache_level,
+					 valid);
+		intel_ring_emit(ring, MI_UPDATE_GTT | (1<<22));
+		/* The docs contradict themselves on the offset. They say dword
+		 * offset, yet the low 12 bits MBZ. */
+		intel_ring_emit(ring, (base & PAGE_MASK) + i);
+		intel_ring_emit(ring, pte);
+		i+=PAGE_SIZE;
+	}
+
+	if (i & PAGE_SHIFT)
+		intel_ring_emit(ring, MI_NOOP);
+
+	intel_ring_advance(ring);
+}
+
+static void
+enable_ctx_ptes(struct intel_ring_buffer *ring,
+		struct i915_hw_context *ctx)
+{
+	if (INTEL_INFO(ring->dev)->gen < 8)
+		_ctx_ptes(ring, ctx, true);
+}
+
+static void
+disable_ctx_ptes(struct intel_ring_buffer *ring,
+		 struct i915_hw_context *ctx)
+{
+	if (INTEL_INFO(ring->dev)->gen < 8)
+		_ctx_ptes(ring, ctx, false);
+}
+
 static inline int
 mi_set_context(struct intel_ring_buffer *ring,
 	       struct i915_hw_context *new_context,
@@ -568,6 +620,8 @@ mi_set_context(struct intel_ring_buffer *ring,
 	if (!IS_HASWELL(ring->dev) && INTEL_INFO(ring->dev)->gen < 8)
 		flags |= (MI_SAVE_EXT_STATE_EN | MI_RESTORE_EXT_STATE_EN);
 
+	enable_ctx_ptes(ring, new_context);
+
 	ret = intel_ring_begin(ring, 6);
 	if (ret)
 		return ret;
@@ -595,6 +649,8 @@ mi_set_context(struct intel_ring_buffer *ring,
 
 	intel_ring_advance(ring);
 
+	disable_ctx_ptes(ring, new_context);
+
 	return ret;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8c382a5..aa95e20 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -368,7 +368,7 @@
 #define MI_TOPOLOGY_FILTER      MI_INSTR(0x0D, 0)
 #define MI_LOAD_SCAN_LINES_EXCL MI_INSTR(0x13, 0)
 #define MI_URB_CLEAR            MI_INSTR(0x19, 0)
-#define MI_UPDATE_GTT           MI_INSTR(0x23, 0)
+#define MI_UPDATE_GTT           MI_INSTR(0x23, 1)
 #define MI_CLFLUSH              MI_INSTR(0x27, 0)
 #define MI_REPORT_PERF_COUNT    MI_INSTR(0x28, 0)
 #define   MI_REPORT_PERF_COUNT_GGTT (1<<0)
-- 
1.8.3.1

[PATCH 11/14] drm/i915/bdw: Add WT caching ability

[Rodrigo Vivi]

From: Ben Widawsky <benjamin.widawsky@intel.com>

I don't have any insight on what parts can do what. The docs do seem to
suggest WT caching works in at least the same manner as it doesn't on
Haswell.

The addr = 0  is to shut up GCC:
drivers/gpu/drm/i915/i915_gem_gtt.c:80:7: warning: 'addr' may be used
uninitialized in this function [-Wmaybe-uninitialized]

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_drv.h     | 11 ++++++-----
 drivers/gpu/drm/i915/i915_gem_gtt.c | 17 +++++++++++++----
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4e81ce1..2bc6745 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1835,11 +1835,12 @@ struct drm_i915_cmd_table {
 #define BSD_RING		(1<<VCS)
 #define BLT_RING		(1<<BCS)
 #define VEBOX_RING		(1<<VECS)
-#define HAS_BSD(dev)            (INTEL_INFO(dev)->ring_mask & BSD_RING)
-#define HAS_BLT(dev)            (INTEL_INFO(dev)->ring_mask & BLT_RING)
-#define HAS_VEBOX(dev)            (INTEL_INFO(dev)->ring_mask & VEBOX_RING)
-#define HAS_LLC(dev)            (INTEL_INFO(dev)->has_llc)
-#define HAS_WT(dev)            (IS_HASWELL(dev) && to_i915(dev)->ellc_size)
+#define HAS_BSD(dev)		(INTEL_INFO(dev)->ring_mask & BSD_RING)
+#define HAS_BLT(dev)		(INTEL_INFO(dev)->ring_mask & BLT_RING)
+#define HAS_VEBOX(dev)		(INTEL_INFO(dev)->ring_mask & VEBOX_RING)
+#define HAS_LLC(dev)		(INTEL_INFO(dev)->has_llc)
+#define HAS_WT(dev)		((IS_HASWELL(dev) || IS_BROADWELL(dev)) && \
+				 to_i915(dev)->ellc_size)
 #define I915_NEED_GFX_HWS(dev)	(INTEL_INFO(dev)->need_gfx_hws)
 
 #define HAS_HW_CONTEXTS(dev)	(INTEL_INFO(dev)->gen >= 6)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 0d514ff..4969162 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -68,10 +68,19 @@ static inline gen8_gtt_pte_t gen8_pte_encode(dma_addr_t addr,
 {
 	gen8_gtt_pte_t pte = valid ? _PAGE_PRESENT | _PAGE_RW : 0;
 	pte |= addr;
-	if (level != I915_CACHE_NONE)
-		pte |= PPAT_CACHED_INDEX;
-	else
+
+	switch (level) {
+	case I915_CACHE_NONE:
 		pte |= PPAT_UNCACHED_INDEX;
+		break;
+	case I915_CACHE_WT:
+		pte |= PPAT_DISPLAY_ELLC_INDEX;
+		break;
+	default:
+		pte |= PPAT_CACHED_INDEX;
+		break;
+	}
+
 	return pte;
 }
 
@@ -1368,7 +1377,7 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
 		(gen8_gtt_pte_t __iomem *)dev_priv->gtt.gsm + first_entry;
 	int i = 0;
 	struct sg_page_iter sg_iter;
-	dma_addr_t addr;
+	dma_addr_t addr = 0;
 
 	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0) {
 		addr = sg_dma_address(sg_iter.sg) +
-- 
1.8.3.1

Re: [PATCH 11/14] drm/i915/bdw: Add WT caching ability

[Volkin, Bradley D]

On Fri, Apr 18, 2014 at 02:04:27PM -0700, Rodrigo Vivi wrote:
> From: Ben Widawsky <benjamin.widawsky@intel.com>
> 
> I don't have any insight on what parts can do what. The docs do seem to
> suggest WT caching works in at least the same manner as it doesn't on
> Haswell.

As Ben previously mentioned, s/doesn't/does. Other than that, looks good

Reviewed-by: Brad Volkin <bradley.d.volkin@intel.com>

> 
> The addr = 0  is to shut up GCC:
> drivers/gpu/drm/i915/i915_gem_gtt.c:80:7: warning: 'addr' may be used
> uninitialized in this function [-Wmaybe-uninitialized]
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h     | 11 ++++++-----
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 17 +++++++++++++----
>  2 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4e81ce1..2bc6745 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1835,11 +1835,12 @@ struct drm_i915_cmd_table {
>  #define BSD_RING		(1<<VCS)
>  #define BLT_RING		(1<<BCS)
>  #define VEBOX_RING		(1<<VECS)
> -#define HAS_BSD(dev)            (INTEL_INFO(dev)->ring_mask & BSD_RING)
> -#define HAS_BLT(dev)            (INTEL_INFO(dev)->ring_mask & BLT_RING)
> -#define HAS_VEBOX(dev)            (INTEL_INFO(dev)->ring_mask & VEBOX_RING)
> -#define HAS_LLC(dev)            (INTEL_INFO(dev)->has_llc)
> -#define HAS_WT(dev)            (IS_HASWELL(dev) && to_i915(dev)->ellc_size)
> +#define HAS_BSD(dev)		(INTEL_INFO(dev)->ring_mask & BSD_RING)
> +#define HAS_BLT(dev)		(INTEL_INFO(dev)->ring_mask & BLT_RING)
> +#define HAS_VEBOX(dev)		(INTEL_INFO(dev)->ring_mask & VEBOX_RING)
> +#define HAS_LLC(dev)		(INTEL_INFO(dev)->has_llc)
> +#define HAS_WT(dev)		((IS_HASWELL(dev) || IS_BROADWELL(dev)) && \
> +				 to_i915(dev)->ellc_size)
>  #define I915_NEED_GFX_HWS(dev)	(INTEL_INFO(dev)->need_gfx_hws)
>  
>  #define HAS_HW_CONTEXTS(dev)	(INTEL_INFO(dev)->gen >= 6)
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 0d514ff..4969162 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -68,10 +68,19 @@ static inline gen8_gtt_pte_t gen8_pte_encode(dma_addr_t addr,
>  {
>  	gen8_gtt_pte_t pte = valid ? _PAGE_PRESENT | _PAGE_RW : 0;
>  	pte |= addr;
> -	if (level != I915_CACHE_NONE)
> -		pte |= PPAT_CACHED_INDEX;
> -	else
> +
> +	switch (level) {
> +	case I915_CACHE_NONE:
>  		pte |= PPAT_UNCACHED_INDEX;
> +		break;
> +	case I915_CACHE_WT:
> +		pte |= PPAT_DISPLAY_ELLC_INDEX;
> +		break;
> +	default:
> +		pte |= PPAT_CACHED_INDEX;
> +		break;
> +	}
> +
>  	return pte;
>  }
>  
> @@ -1368,7 +1377,7 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
>  		(gen8_gtt_pte_t __iomem *)dev_priv->gtt.gsm + first_entry;
>  	int i = 0;
>  	struct sg_page_iter sg_iter;
> -	dma_addr_t addr;
> +	dma_addr_t addr = 0;
>  
>  	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0) {
>  		addr = sg_dma_address(sg_iter.sg) +
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 12/14] drm/i915/bdw: enable eDRAM.

[Rodrigo Vivi]

From: Ben Widawsky <benjamin.widawsky@intel.com>

The same register exists for querying and programming eDRAM AKA eLLC. So
we can simply use it. For now, use all the same defaults as we had
for Haswell, since like Haswell, I have no further details.

I do not actually have a part with eDRAM, so I cannot test this.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/intel_uncore.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index c8969e3..0e6b502 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -373,7 +373,7 @@ void intel_uncore_early_sanitize(struct drm_device *dev)
 	if (HAS_FPGA_DBG_UNCLAIMED(dev))
 		__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
 
-	if (IS_HASWELL(dev) &&
+	if ((IS_HASWELL(dev) || IS_BROADWELL(dev)) &&
 	    (__raw_i915_read32(dev_priv, HSW_EDRAM_PRESENT) == 1)) {
 		/* The docs do not explain exactly how the calculation can be
 		 * made. It is somewhat guessable, but for now, it's always
-- 
1.8.3.1

Re: [PATCH 12/14] drm/i915/bdw: enable eDRAM.

[Volkin, Bradley D]

Reviewed-by: Brad Volkin <bradley.d.volkin@intel.com>

On Fri, Apr 18, 2014 at 02:04:28PM -0700, Rodrigo Vivi wrote:
> From: Ben Widawsky <benjamin.widawsky@intel.com>
> 
> The same register exists for querying and programming eDRAM AKA eLLC. So
> we can simply use it. For now, use all the same defaults as we had
> for Haswell, since like Haswell, I have no further details.
> 
> I do not actually have a part with eDRAM, so I cannot test this.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index c8969e3..0e6b502 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -373,7 +373,7 @@ void intel_uncore_early_sanitize(struct drm_device *dev)
>  	if (HAS_FPGA_DBG_UNCLAIMED(dev))
>  		__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
>  
> -	if (IS_HASWELL(dev) &&
> +	if ((IS_HASWELL(dev) || IS_BROADWELL(dev)) &&
>  	    (__raw_i915_read32(dev_priv, HSW_EDRAM_PRESENT) == 1)) {
>  		/* The docs do not explain exactly how the calculation can be
>  		 * made. It is somewhat guessable, but for now, it's always
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 13/14] drm/i915/bdw: Disable idle DOP clock gating

[Rodrigo Vivi]

From: Ben Widawsky <benjamin.widawsky@intel.com>

It seems we need this at least for the current platforms we have, but
probably not later. In any event, it should cause too much harm as we do
the same thing on several other platforms.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a66000c..8d40786 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4924,6 +4924,10 @@ static void gen8_init_clock_gating(struct drm_device *dev)
 	I915_WRITE(GEN7_HALF_SLICE_CHICKEN1,
 		   _MASKED_BIT_ENABLE(GEN7_SINGLE_SUBSCAN_DISPATCH_ENABLE));
 
+	/* WaDisableDopClockGating:bdw May not be needed for production */
+	I915_WRITE(GEN7_ROW_CHICKEN2,
+		   _MASKED_BIT_ENABLE(DOP_CLOCK_GATING_DISABLE));
+
 	/* WaSwitchSolVfFArbitrationPriority:bdw */
 	I915_WRITE(GAM_ECOCHK, I915_READ(GAM_ECOCHK) | HSW_ECOCHK_ARB_PRIO_SOL);
 
-- 
1.8.3.1

Re: [PATCH 13/14] drm/i915/bdw: Disable idle DOP clock gating

[Volkin, Bradley D]

Reviewed-by: Brad Volkin <bradley.d.volkin@intel.com>

On Fri, Apr 18, 2014 at 02:04:29PM -0700, Rodrigo Vivi wrote:
> From: Ben Widawsky <benjamin.widawsky@intel.com>
> 
> It seems we need this at least for the current platforms we have, but
> probably not later. In any event, it should cause too much harm as we do
> the same thing on several other platforms.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a66000c..8d40786 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4924,6 +4924,10 @@ static void gen8_init_clock_gating(struct drm_device *dev)
>  	I915_WRITE(GEN7_HALF_SLICE_CHICKEN1,
>  		   _MASKED_BIT_ENABLE(GEN7_SINGLE_SUBSCAN_DISPATCH_ENABLE));
>  
> +	/* WaDisableDopClockGating:bdw May not be needed for production */
> +	I915_WRITE(GEN7_ROW_CHICKEN2,
> +		   _MASKED_BIT_ENABLE(DOP_CLOCK_GATING_DISABLE));
> +
>  	/* WaSwitchSolVfFArbitrationPriority:bdw */
>  	I915_WRITE(GAM_ECOCHK, I915_READ(GAM_ECOCHK) | HSW_ECOCHK_ARB_PRIO_SOL);
>  
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 13/14] drm/i915/bdw: Disable idle DOP clock gating

[Daniel Vetter]

On Mon, Apr 28, 2014 at 09:37:36AM -0700, Volkin, Bradley D wrote:
> Reviewed-by: Brad Volkin <bradley.d.volkin@intel.com>
> 
> On Fri, Apr 18, 2014 at 02:04:29PM -0700, Rodrigo Vivi wrote:
> > From: Ben Widawsky <benjamin.widawsky@intel.com>
> > 
> > It seems we need this at least for the current platforms we have, but
> > probably not later. In any event, it should cause too much harm as we do
> > the same thing on several other platforms.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

All three patches from Ben merged, thanks.
-Daniel

> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index a66000c..8d40786 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4924,6 +4924,10 @@ static void gen8_init_clock_gating(struct drm_device *dev)
> >  	I915_WRITE(GEN7_HALF_SLICE_CHICKEN1,
> >  		   _MASKED_BIT_ENABLE(GEN7_SINGLE_SUBSCAN_DISPATCH_ENABLE));
> >  
> > +	/* WaDisableDopClockGating:bdw May not be needed for production */
> > +	I915_WRITE(GEN7_ROW_CHICKEN2,
> > +		   _MASKED_BIT_ENABLE(DOP_CLOCK_GATING_DISABLE));
> > +
> >  	/* WaSwitchSolVfFArbitrationPriority:bdw */
> >  	I915_WRITE(GAM_ECOCHK, I915_READ(GAM_ECOCHK) | HSW_ECOCHK_ARB_PRIO_SOL);
> >  
> > -- 
> > 1.8.3.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 14/14] drm/i915: honour forced connector modes

[Rodrigo Vivi]

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

In the move over to use BIOS connector configs, we lost the ability to
force a specific set of connectors on or off.  Try to remedy that by
dropping back to the old behavior if we detect a hard coded connector
config that tries to enable a connector (disabling is easy!).

Based on earlier patches by Jesse Barnes.

Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/intel_fbdev.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index b4d44e6..bea2261 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -338,8 +338,18 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
 			continue;
 		}
 
+		if (connector->force == DRM_FORCE_OFF) {
+			DRM_DEBUG_KMS("connector %d is disabled by user, skipping\n",
+				      connector->base.id);
+			enabled[i] = false;
+			continue;
+		}
+
 		encoder = connector->encoder;
 		if (!encoder || WARN_ON(!encoder->crtc)) {
+			if (connector->force > DRM_FORCE_OFF)
+				goto bail;
+
 			DRM_DEBUG_KMS("connector %d has no encoder or crtc, skipping\n",
 				      connector->base.id);
 			enabled[i] = false;
@@ -358,8 +368,7 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
 		for (j = 0; j < fb_helper->connector_count; j++) {
 			if (crtcs[j] == new_crtc) {
 				DRM_DEBUG_KMS("fallback: cloned configuration\n");
-				fallback = true;
-				goto out;
+				goto bail;
 			}
 		}
 
@@ -417,8 +426,8 @@ static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
 		fallback = true;
 	}
 
-out:
 	if (fallback) {
+bail:
 		DRM_DEBUG_KMS("Not using firmware configuration\n");
 		memcpy(enabled, save_enabled, dev->mode_config.num_connector);
 		kfree(save_enabled);
-- 
1.8.3.1

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

Re: [PATCH 00/14] drm-intel-collector - update

[Daniel Vetter]

Review assignments and drop suggestions below. Reviewers please
complain if you can't do it in a timely fashion. Rodrigo: Can you
please add an overall -collector item with the names of these people
to the review board so that mangers can track this? Maybe discuss the
overall approach with Paul first.

Thanks, Daniel

> Here goes the update list in order for better reviewers assignment:
>
> Patch     drm/i915: Bring UP Power Wells before disabling RC6. - Reviewer:

Paulo

> Patch     drm/i915: Add support for stealing purgable stolen pages - Reviewer:

Should be dropped and needs to be resend as part of the overall stolen
enabling.

> Patch     drm/i915: Do not allow a pending forcewake put to unbalance across reset - Reviewer:
> Patch     drm/i915: Don't save/restore RS when not used - Reviewer:

Abdiel (Helsinki team)

> Patch     drm/i915: add support for Z-order of planes - Reviewer:

Imo not -collector material, needs an overall approach. Also needs a
testcase. Should be dropped.

> Patch     drm/i915: Validate VBT header before trusting it - Reviewer:
> Patch     drm/i915: Validate BDB section before reading - Reviewer:

Shobhit is signe dup already.

> Patch     drm/i915: Upgrade execbuffer fail after resume failure to EIO - Reviewer:

Do we still need this on top of what I've merged. Chris?

> Patch     drm/i915: Add property to set HDMI aspect ratio - Reviewer:

Ville

> Patch     drm/i915: Prevent context obj from being corrupted - Reviewer:

Nacked, we simply need to fix up aliasing ppgtt

> Patch     drm/i915/bdw: Add WT caching ability - Reviewer:
> Patch     drm/i915/bdw: enable eDRAM. - Reviewer:
> Patch     drm/i915/bdw: Disable idle DOP clock gating - Reviewer:

Brad Volkin from vpg

> Patch     drm/i915: honour forced connector modes - Reviewer:

Jesse
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 00/14] drm-intel-collector - update

[Chris Wilson]

On Fri, Apr 25, 2014 at 11:04:22AM +0200, Daniel Vetter wrote:
> > Patch     drm/i915: Upgrade execbuffer fail after resume failure to EIO - Reviewer:
> 
> Do we still need this on top of what I've merged. Chris?

Yes. We can still start the device without initializing all of the
available rings and so hit this before we fail with an EIO.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 00/14] drm-intel-collector - update

[Daniel Vetter]

On Fri, Apr 25, 2014 at 10:24:45AM +0100, Chris Wilson wrote:
> On Fri, Apr 25, 2014 at 11:04:22AM +0200, Daniel Vetter wrote:
> > > Patch     drm/i915: Upgrade execbuffer fail after resume failure to EIO - Reviewer:
> > 
> > Do we still need this on top of what I've merged. Chris?
> 
> Yes. We can still start the device without initializing all of the
> available rings and so hit this before we fail with an EIO.

Hm, so we'd need to adjust the commit message a bit since with

commit 5582e8c3c49150c0e7398688b5ed167d6c3d44fd
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Wed Apr 9 09:19:41 2014 +0100

    drm/i915: Preserve ring buffers objects across resume

this is just a driver load issue and no longer a resume issue. Or do I
miss something else?

Or should we frob the ring init some more and simply leave the ring->obj
hanging around, eat the -EIO and wedge the gpu?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 00/14] drm-intel-collector - update

[Chris Wilson]

On Fri, Apr 25, 2014 at 12:07:48PM +0200, Daniel Vetter wrote:
> On Fri, Apr 25, 2014 at 10:24:45AM +0100, Chris Wilson wrote:
> > On Fri, Apr 25, 2014 at 11:04:22AM +0200, Daniel Vetter wrote:
> > > > Patch     drm/i915: Upgrade execbuffer fail after resume failure to EIO - Reviewer:
> > > 
> > > Do we still need this on top of what I've merged. Chris?
> > 
> > Yes. We can still start the device without initializing all of the
> > available rings and so hit this before we fail with an EIO.
> 
> Hm, so we'd need to adjust the commit message a bit since with
> 
> commit 5582e8c3c49150c0e7398688b5ed167d6c3d44fd
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Wed Apr 9 09:19:41 2014 +0100
> 
>     drm/i915: Preserve ring buffers objects across resume
> 
> this is just a driver load issue and no longer a resume issue. Or do I
> miss something else?
> 
> Or should we frob the ring init some more and simply leave the ring->obj
> hanging around, eat the -EIO and wedge the gpu?

No, because we abort i915_gem_init_rings(). This is an exceptional error
path and I think just fixing up the errno here is simplest. All other
paths should barf before they touch the ring thanks to
intel_ring_begin(). The only difficulty here is that we need to
differentiate between two different types of error. (Note that even UXA
should be checking for a dead GPU before it tries execbuffer, in this
case, at least since it started trying to not die randomly.) So I am
just arguing from an interface correctness pov.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 00/14] drm-intel-collector - update

[Daniel Vetter]

On Fri, Apr 25, 2014 at 12:19 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, Apr 25, 2014 at 12:07:48PM +0200, Daniel Vetter wrote:
>> On Fri, Apr 25, 2014 at 10:24:45AM +0100, Chris Wilson wrote:
>> > On Fri, Apr 25, 2014 at 11:04:22AM +0200, Daniel Vetter wrote:
>> > > > Patch     drm/i915: Upgrade execbuffer fail after resume failure to EIO - Reviewer:
>> > >
>> > > Do we still need this on top of what I've merged. Chris?
>> >
>> > Yes. We can still start the device without initializing all of the
>> > available rings and so hit this before we fail with an EIO.
>>
>> Hm, so we'd need to adjust the commit message a bit since with
>>
>> commit 5582e8c3c49150c0e7398688b5ed167d6c3d44fd
>> Author: Chris Wilson <chris@chris-wilson.co.uk>
>> Date:   Wed Apr 9 09:19:41 2014 +0100
>>
>>     drm/i915: Preserve ring buffers objects across resume
>>
>> this is just a driver load issue and no longer a resume issue. Or do I
>> miss something else?
>>
>> Or should we frob the ring init some more and simply leave the ring->obj
>> hanging around, eat the -EIO and wedge the gpu?
>
> No, because we abort i915_gem_init_rings(). This is an exceptional error
> path and I think just fixing up the errno here is simplest. All other
> paths should barf before they touch the ring thanks to
> intel_ring_begin(). The only difficulty here is that we need to
> differentiate between two different types of error. (Note that even UXA
> should be checking for a dead GPU before it tries execbuffer, in this
> case, at least since it started trying to not die randomly.) So I am
> just arguing from an interface correctness pov.

Yeah I've read around a bit in our gem_init_hw functions and this
seems to be the sanest option. Can you please update the patch's
commit message to reflect our discussion here? And maybe throw a t-b
in in case we have that.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch