Select patches for 2.6.40

Select patches for 2.6.40

[Chris Wilson]

Hi Keith,

These are the small patches above and beyond what you have already chosen
to include that I think will be useful for 2.6.40.
-Chris

[PATCH 01/16] drm/i915: Cache GT fifo count for SandyBridge

[Chris Wilson]

The read back of the available FIFO entries is vital for system
stability, but extremely costly. However, we only need a guide so as to
avoid eating into the reserved entries and since we are the only
consumer we can cache the read of the count from the last write.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_drv.c |   15 ++++++++++-----
 drivers/gpu/drm/i915/i915_drv.h |    1 +
 drivers/gpu/drm/i915/i915_reg.h |    1 +
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 52e52ce..50c6065 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -313,12 +313,17 @@ void gen6_gt_force_wake_put(struct drm_i915_private *dev_priv)
 
 void __gen6_gt_wait_for_fifo(struct drm_i915_private *dev_priv)
 {
-	int loop = 500;
-	u32 fifo = I915_READ_NOTRACE(GT_FIFO_FREE_ENTRIES);
-	while (fifo < 20 && loop--) {
-		udelay(10);
-		fifo = I915_READ_NOTRACE(GT_FIFO_FREE_ENTRIES);
+	if (dev_priv->gt_fifo_count < GT_FIFO_NUM_RESERVED_ENTRIES ) {
+		int loop = 500;
+		u32 fifo = I915_READ_NOTRACE(GT_FIFO_FREE_ENTRIES);
+		while (fifo <= GT_FIFO_NUM_RESERVED_ENTRIES && loop--) {
+			udelay(10);
+			fifo = I915_READ_NOTRACE(GT_FIFO_FREE_ENTRIES);
+		}
+		WARN_ON(loop < 0 && fifo <= GT_FIFO_NUM_RESERVED_ENTRIES);
+		dev_priv->gt_fifo_count = fifo;
 	}
+	dev_priv->gt_fifo_count--;
 }
 
 static int i915_drm_freeze(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e9d8243..d029476 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -268,6 +268,7 @@ typedef struct drm_i915_private {
 	int relative_constants_mode;
 
 	void __iomem *regs;
+	u32 gt_fifo_count;
 
 	struct intel_gmbus {
 		struct i2c_adapter adapter;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 289adaa..ef13f39 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3329,6 +3329,7 @@
 #define  FORCEWAKE_ACK				0x130090
 
 #define  GT_FIFO_FREE_ENTRIES			0x120008
+#define    GT_FIFO_NUM_RESERVED_ENTRIES		20
 
 #define GEN6_RPNSWREQ				0xA008
 #define   GEN6_TURBO_DISABLE			(1<<31)
-- 
1.7.5.1

[PATCH 02/16] drm/i915: Refactor pwrite/pread to use single copy of get_user_pages

[Chris Wilson]

Replace the three nearly identical copies of the code with a single
function. And take advantage of the opportunity to do some
micro-optimisation: avoid the vmalloc if at all possible and also avoid
dropping the lock unless we are forced to acquire the mm semaphore.

Measuring the impact of the optimisations, it turns out they are not so
micro after all. Running x11perf -aa10text on PineView:

  before 1.28 Mglyph/sec
  after  1.45 Mglyph/sec

(Glyphs, in general and on PineView in particular, are one of the very
few pwrite rate-limited workloads.)

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Reviewed-by: Paul Menzel <paulepanter@users.sourceforge.net>
---
 drivers/gpu/drm/i915/i915_gem.c |  149 ++++++++++++++++++++++++---------------
 1 files changed, 92 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index bf32527..89b751d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -257,6 +257,73 @@ static int i915_gem_object_needs_bit17_swizzle(struct drm_i915_gem_object *obj)
 		obj->tiling_mode != I915_TILING_NONE;
 }
 
+/**
+ * Magically retrieves the pages for the user addr whilst holding the
+ * dev->struct_mutex.
+ *
+ * Since we can not take the mm semaphore whilst holding our dev->struct_mutex,
+ * due to the pre-existing lock dependency established by i915_gem_fault(),
+ * we have to perform some sleight-of-hand.
+ *
+ * First, we try the lockless variant of get_user_pages(),
+ * __get_user_pages_fast(), whilst continuing to hold the mutex. If that
+ * fails to get all the user pages, then we no have choice but to acquire
+ * the mm semaphore, thus dropping the lock on dev->struct_mutex to do so.
+ * The dev->struct_mutex is then re-acquired before we return.
+ *
+ * Returns: an error code *and* the number of user pages acquired. Even
+ * on an error, you must iterate over the returned pages and release them.
+ */
+static int
+i915_gem_get_user_pages(struct drm_device *dev,
+			unsigned long addr,
+			bool write,
+			int *num_pages,
+			struct page ***pages_out)
+{
+	struct page **pages;
+	int pinned, ret;
+	int n = *num_pages;
+
+	pages = kmalloc(n*sizeof(struct page *),
+			GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN);
+	if (pages == NULL) {
+		pages = drm_malloc_ab(n, sizeof(struct page *));
+		if (pages == NULL) {
+			*pages_out = NULL;
+			*num_pages = 0;
+			return -ENOMEM;
+		}
+	}
+
+	pinned = __get_user_pages_fast(addr, n, write, pages);
+	if (pinned < n) {
+		struct mm_struct *mm = current->mm;
+
+		mutex_unlock(&dev->struct_mutex);
+		down_read(&mm->mmap_sem);
+		ret = get_user_pages(current, mm,
+				     addr + (pinned << PAGE_SHIFT),
+				     n - pinned,
+				     write, 0,
+				     pages + pinned,
+				     NULL);
+		up_read(&mm->mmap_sem);
+		mutex_lock(&dev->struct_mutex);
+		if (ret > 0)
+			pinned += ret;
+	}
+
+	ret = 0;
+	if (pinned < n)
+		ret = -EFAULT;
+
+	*num_pages = pinned;
+	*pages_out = pages;
+	return ret;
+}
+
+
 static inline void
 slow_shmem_copy(struct page *dst_page,
 		int dst_offset,
@@ -398,11 +465,11 @@ i915_gem_shmem_pread_slow(struct drm_device *dev,
 			  struct drm_file *file)
 {
 	struct address_space *mapping = obj->base.filp->f_path.dentry->d_inode->i_mapping;
-	struct mm_struct *mm = current->mm;
 	struct page **user_pages;
 	ssize_t remain;
-	loff_t offset, pinned_pages, i;
-	loff_t first_data_page, last_data_page, num_pages;
+	loff_t offset;
+	loff_t first_data_page, last_data_page;
+	int num_pages, i;
 	int shmem_page_offset;
 	int data_page_index, data_page_offset;
 	int page_length;
@@ -420,20 +487,10 @@ i915_gem_shmem_pread_slow(struct drm_device *dev,
 	last_data_page = (data_ptr + args->size - 1) / PAGE_SIZE;
 	num_pages = last_data_page - first_data_page + 1;
 
-	user_pages = drm_malloc_ab(num_pages, sizeof(struct page *));
-	if (user_pages == NULL)
-		return -ENOMEM;
-
-	mutex_unlock(&dev->struct_mutex);
-	down_read(&mm->mmap_sem);
-	pinned_pages = get_user_pages(current, mm, (uintptr_t)args->data_ptr,
-				      num_pages, 1, 0, user_pages, NULL);
-	up_read(&mm->mmap_sem);
-	mutex_lock(&dev->struct_mutex);
-	if (pinned_pages < num_pages) {
-		ret = -EFAULT;
+	ret = i915_gem_get_user_pages(dev, data_ptr, true,
+				      &num_pages, &user_pages);
+	if (ret)
 		goto out;
-	}
 
 	ret = i915_gem_object_set_cpu_read_domain_range(obj,
 							args->offset,
@@ -494,7 +551,7 @@ i915_gem_shmem_pread_slow(struct drm_device *dev,
 	}
 
 out:
-	for (i = 0; i < pinned_pages; i++) {
+	for (i = 0; i < num_pages; i++) {
 		SetPageDirty(user_pages[i]);
 		mark_page_accessed(user_pages[i]);
 		page_cache_release(user_pages[i]);
@@ -679,10 +736,9 @@ i915_gem_gtt_pwrite_slow(struct drm_device *dev,
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	ssize_t remain;
 	loff_t gtt_page_base, offset;
-	loff_t first_data_page, last_data_page, num_pages;
-	loff_t pinned_pages, i;
+	loff_t first_data_page, last_data_page;
+	int num_pages, i;
 	struct page **user_pages;
-	struct mm_struct *mm = current->mm;
 	int gtt_page_offset, data_page_offset, data_page_index, page_length;
 	int ret;
 	uint64_t data_ptr = args->data_ptr;
@@ -697,28 +753,18 @@ i915_gem_gtt_pwrite_slow(struct drm_device *dev,
 	last_data_page = (data_ptr + args->size - 1) / PAGE_SIZE;
 	num_pages = last_data_page - first_data_page + 1;
 
-	user_pages = drm_malloc_ab(num_pages, sizeof(struct page *));
-	if (user_pages == NULL)
-		return -ENOMEM;
-
-	mutex_unlock(&dev->struct_mutex);
-	down_read(&mm->mmap_sem);
-	pinned_pages = get_user_pages(current, mm, (uintptr_t)args->data_ptr,
-				      num_pages, 0, 0, user_pages, NULL);
-	up_read(&mm->mmap_sem);
-	mutex_lock(&dev->struct_mutex);
-	if (pinned_pages < num_pages) {
-		ret = -EFAULT;
-		goto out_unpin_pages;
-	}
+	ret = i915_gem_get_user_pages(dev, data_ptr, false,
+				      &num_pages, &user_pages);
+	if (ret)
+		goto out;
 
 	ret = i915_gem_object_set_to_gtt_domain(obj, true);
 	if (ret)
-		goto out_unpin_pages;
+		goto out;
 
 	ret = i915_gem_object_put_fence(obj);
 	if (ret)
-		goto out_unpin_pages;
+		goto out;
 
 	offset = obj->gtt_offset + args->offset;
 
@@ -753,8 +799,8 @@ i915_gem_gtt_pwrite_slow(struct drm_device *dev,
 		data_ptr += page_length;
 	}
 
-out_unpin_pages:
-	for (i = 0; i < pinned_pages; i++)
+out:
+	for (i = 0; i < num_pages; i++)
 		page_cache_release(user_pages[i]);
 	drm_free_large(user_pages);
 
@@ -803,11 +849,11 @@ i915_gem_shmem_pwrite_fast(struct drm_device *dev,
 		if (IS_ERR(page))
 			return PTR_ERR(page);
 
-		vaddr = kmap_atomic(page, KM_USER0);
+		vaddr = kmap_atomic(page);
 		ret = __copy_from_user_inatomic(vaddr + page_offset,
 						user_data,
 						page_length);
-		kunmap_atomic(vaddr, KM_USER0);
+		kunmap_atomic(vaddr);
 
 		set_page_dirty(page);
 		mark_page_accessed(page);
@@ -842,11 +888,10 @@ i915_gem_shmem_pwrite_slow(struct drm_device *dev,
 			   struct drm_file *file)
 {
 	struct address_space *mapping = obj->base.filp->f_path.dentry->d_inode->i_mapping;
-	struct mm_struct *mm = current->mm;
 	struct page **user_pages;
 	ssize_t remain;
-	loff_t offset, pinned_pages, i;
-	loff_t first_data_page, last_data_page, num_pages;
+	loff_t first_data_page, last_data_page, offset;
+	int num_pages, i;
 	int shmem_page_offset;
 	int data_page_index,  data_page_offset;
 	int page_length;
@@ -864,20 +909,10 @@ i915_gem_shmem_pwrite_slow(struct drm_device *dev,
 	last_data_page = (data_ptr + args->size - 1) / PAGE_SIZE;
 	num_pages = last_data_page - first_data_page + 1;
 
-	user_pages = drm_malloc_ab(num_pages, sizeof(struct page *));
-	if (user_pages == NULL)
-		return -ENOMEM;
-
-	mutex_unlock(&dev->struct_mutex);
-	down_read(&mm->mmap_sem);
-	pinned_pages = get_user_pages(current, mm, (uintptr_t)args->data_ptr,
-				      num_pages, 0, 0, user_pages, NULL);
-	up_read(&mm->mmap_sem);
-	mutex_lock(&dev->struct_mutex);
-	if (pinned_pages < num_pages) {
-		ret = -EFAULT;
+	ret = i915_gem_get_user_pages(dev, data_ptr, false,
+				      &num_pages, &user_pages);
+	if (ret)
 		goto out;
-	}
 
 	ret = i915_gem_object_set_to_cpu_domain(obj, 1);
 	if (ret)
@@ -940,7 +975,7 @@ i915_gem_shmem_pwrite_slow(struct drm_device *dev,
 	}
 
 out:
-	for (i = 0; i < pinned_pages; i++)
+	for (i = 0; i < num_pages; i++)
 		page_cache_release(user_pages[i]);
 	drm_free_large(user_pages);
 
-- 
1.7.5.1

Re: [PATCH 02/16] drm/i915: Refactor pwrite/pread to use single copy of get_user_pages

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 613 bytes --]

On Thu, 12 May 2011 22:17:10 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> +	pages = kmalloc(n*sizeof(struct page *),
> +			GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN);
> +	if (pages == NULL) {
> +		pages = drm_malloc_ab(n, sizeof(struct page *));
> +		if (pages == NULL) {
> +			*pages_out = NULL;
> +			*num_pages = 0;
> +			return -ENOMEM;
> +		}
> +	}

Please use drm_malloc_ab here unconditionally; you've got a potential
multiplication overflow, and drm_malloc_ab already uses kmalloc for
small amounts anyways.

Otherwise, this looks good to me.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 02/16] drm/i915: Refactor pwrite/pread to use single copy of get_user_pages

[Chris Wilson]

On Thu, 12 May 2011 17:21:50 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Thu, 12 May 2011 22:17:10 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > +	pages = kmalloc(n*sizeof(struct page *),
> > +			GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN);
> > +	if (pages == NULL) {
> > +		pages = drm_malloc_ab(n, sizeof(struct page *));
> > +		if (pages == NULL) {
> > +			*pages_out = NULL;
> > +			*num_pages = 0;
> > +			return -ENOMEM;
> > +		}
> > +	}
> 
> Please use drm_malloc_ab here unconditionally;

We're not performing the same trick as drm_malloc_ab() here though, since
this is only used for a temporary allocation we try to consume any
high-order pages, rather than building an array of order-0 pages, knowing
that they will be released shortly afterwards.

> you've got a potential multiplication overflow,

Now this is more serious. Should we not just E2BIG any bo_create that will
require num_pages > MAXINT/sizeof(struct page*)? [1TiB on 32bit]
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 02/16] drm/i915: Refactor pwrite/pread to use single copy of get_user_pages

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 657 bytes --]

On Sun, 15 May 2011 09:00:51 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> We're not performing the same trick as drm_malloc_ab() here though, since
> this is only used for a temporary allocation we try to consume any
> high-order pages, rather than building an array of order-0 pages, knowing
> that they will be released shortly afterwards.

If you want a 'large temporary allocator', then please feel free to
convince airlied that we need one in drm. For now, please just use the
existing drm function that provides the required interface and which
already has the necessary checks against overflow.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 03/16] drm/i915: s/addr & ~PAGE_MASK/offset_in_page(addr)/

[Chris Wilson]

Convert our open coded offset_in_page() to the common macro.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |   21 ++++++++++-----------
 1 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 89b751d..67ddda0 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -423,7 +423,7 @@ i915_gem_shmem_pread_fast(struct drm_device *dev,
 		 * page_offset = offset within page
 		 * page_length = bytes to copy for this page
 		 */
-		page_offset = offset & (PAGE_SIZE-1);
+		page_offset = offset_in_page(offset);
 		page_length = remain;
 		if ((page_offset + remain) > PAGE_SIZE)
 			page_length = PAGE_SIZE - page_offset;
@@ -512,9 +512,9 @@ i915_gem_shmem_pread_slow(struct drm_device *dev,
 		 * data_page_offset = offset with data_page_index page.
 		 * page_length = bytes to copy for this page
 		 */
-		shmem_page_offset = offset & ~PAGE_MASK;
+		shmem_page_offset = offset_in_page(offset);
 		data_page_index = data_ptr / PAGE_SIZE - first_data_page;
-		data_page_offset = data_ptr & ~PAGE_MASK;
+		data_page_offset = offset_in_page(data_ptr);
 
 		page_length = remain;
 		if ((shmem_page_offset + page_length) > PAGE_SIZE)
@@ -697,8 +697,8 @@ i915_gem_gtt_pwrite_fast(struct drm_device *dev,
 		 * page_offset = offset within page
 		 * page_length = bytes to copy for this page
 		 */
-		page_base = (offset & ~(PAGE_SIZE-1));
-		page_offset = offset & (PAGE_SIZE-1);
+		page_base = offset & PAGE_MASK;
+		page_offset = offset_in_page(offset);
 		page_length = remain;
 		if ((page_offset + remain) > PAGE_SIZE)
 			page_length = PAGE_SIZE - page_offset;
@@ -709,7 +709,6 @@ i915_gem_gtt_pwrite_fast(struct drm_device *dev,
 		 */
 		if (fast_user_write(dev_priv->mm.gtt_mapping, page_base,
 				    page_offset, user_data, page_length))
-
 			return -EFAULT;
 
 		remain -= page_length;
@@ -778,9 +777,9 @@ i915_gem_gtt_pwrite_slow(struct drm_device *dev,
 		 * page_length = bytes to copy for this page
 		 */
 		gtt_page_base = offset & PAGE_MASK;
-		gtt_page_offset = offset & ~PAGE_MASK;
+		gtt_page_offset = offset_in_page(offset);
 		data_page_index = data_ptr / PAGE_SIZE - first_data_page;
-		data_page_offset = data_ptr & ~PAGE_MASK;
+		data_page_offset = offset_in_page(data_ptr);
 
 		page_length = remain;
 		if ((gtt_page_offset + page_length) > PAGE_SIZE)
@@ -839,7 +838,7 @@ i915_gem_shmem_pwrite_fast(struct drm_device *dev,
 		 * page_offset = offset within page
 		 * page_length = bytes to copy for this page
 		 */
-		page_offset = offset & (PAGE_SIZE-1);
+		page_offset = offset_in_page(offset);
 		page_length = remain;
 		if ((page_offset + remain) > PAGE_SIZE)
 			page_length = PAGE_SIZE - page_offset;
@@ -933,9 +932,9 @@ i915_gem_shmem_pwrite_slow(struct drm_device *dev,
 		 * data_page_offset = offset with data_page_index page.
 		 * page_length = bytes to copy for this page
 		 */
-		shmem_page_offset = offset & ~PAGE_MASK;
+		shmem_page_offset = offset_in_page(offset);
 		data_page_index = data_ptr / PAGE_SIZE - first_data_page;
-		data_page_offset = data_ptr & ~PAGE_MASK;
+		data_page_offset = offset_in_page(data_ptr);
 
 		page_length = remain;
 		if ((shmem_page_offset + page_length) > PAGE_SIZE)
-- 
1.7.5.1

Re: [PATCH 03/16] drm/i915: s/addr & ~PAGE_MASK/offset_in_page(addr)/

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 292 bytes --]

On Thu, 12 May 2011 22:17:11 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Convert our open coded offset_in_page() to the common macro.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Keith Packard <keithp@keithp.com>

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 04/16] drm/i915: Only print out the actual number of fences for i915_error_state

[Chris Wilson]

From: Daniel Vetter <daniel.vetter@ffwll.ch>

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_debugfs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index c0ce5e4..48f7e257 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -776,7 +776,7 @@ static int i915_error_state(struct seq_file *m, void *unused)
 	seq_printf(m, "  INSTPM: 0x%08x\n", error->instpm);
 	seq_printf(m, "  seqno: 0x%08x\n", error->seqno);
 
-	for (i = 0; i < 16; i++)
+	for (i = 0; i < dev_priv->num_fence_regs; i++)
 		seq_printf(m, "  fence[%d] = %08llx\n", i, error->fence[i]);
 
 	if (error->active_bo)
-- 
1.7.5.1

Re: [PATCH 04/16] drm/i915: Only print out the actual number of fences for i915_error_state

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 520 bytes --]

On Thu, 12 May 2011 22:17:12 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> From: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

This change seems correct. Would be nice if this code printed out the
actual fence register contents (perhaps in addition to what the fence
management code thinks they should be...)

Reviewed-by: Keith Packard <keithp@keithp.com>

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 05/16] drm/i915/tv: Use a direct pointer for tv_mode

[Chris Wilson]

Instead of scanning through the static array using strcmp to find our
matching tv_mode, just keep a direct link around. The historical reason
for the strcmp was to match the connection property "tv format", but now
that property is translated for us into an index by the drm core.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mathew McKernan <matmckernan@rauland.com.au>
---
 drivers/gpu/drm/i915/intel_tv.c |  118 ++++++++++++++++-----------------------
 1 files changed, 48 insertions(+), 70 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 113e4e7..de214bb 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -43,12 +43,45 @@ enum tv_margin {
 	TV_MARGIN_RIGHT, TV_MARGIN_BOTTOM
 };
 
+struct tv_mode {
+	const char *name;
+	int clock;
+	int refresh; /* in millihertz (for precision) */
+	u32 oversample;
+	int hsync_end, hblank_start, hblank_end, htotal;
+	bool progressive, trilevel_sync, component_only;
+	int vsync_start_f1, vsync_start_f2, vsync_len;
+	bool veq_ena;
+	int veq_start_f1, veq_start_f2, veq_len;
+	int vi_end_f1, vi_end_f2, nbr_end;
+	bool burst_ena;
+	int hburst_start, hburst_len;
+	int vburst_start_f1, vburst_end_f1;
+	int vburst_start_f2, vburst_end_f2;
+	int vburst_start_f3, vburst_end_f3;
+	int vburst_start_f4, vburst_end_f4;
+	/*
+	 * subcarrier programming
+	 */
+	int dda2_size, dda3_size, dda1_inc, dda2_inc, dda3_inc;
+	u32 sc_reset;
+	bool pal_burst;
+	/*
+	 * blank/black levels
+	 */
+	const struct video_levels *composite_levels, *svideo_levels;
+	const struct color_conversion *composite_color, *svideo_color;
+	const u32 *filter_table;
+	int max_srcw;
+};
+
+
 /** Private structure for the integrated TV support */
 struct intel_tv {
 	struct intel_encoder base;
 
 	int type;
-	const char *tv_format;
+	const struct tv_mode *mode;
 	int margin[4];
 	u32 save_TV_H_CTL_1;
 	u32 save_TV_H_CTL_2;
@@ -349,39 +382,6 @@ static const struct video_levels component_levels = {
 };
 
 
-struct tv_mode {
-	const char *name;
-	int clock;
-	int refresh; /* in millihertz (for precision) */
-	u32 oversample;
-	int hsync_end, hblank_start, hblank_end, htotal;
-	bool progressive, trilevel_sync, component_only;
-	int vsync_start_f1, vsync_start_f2, vsync_len;
-	bool veq_ena;
-	int veq_start_f1, veq_start_f2, veq_len;
-	int vi_end_f1, vi_end_f2, nbr_end;
-	bool burst_ena;
-	int hburst_start, hburst_len;
-	int vburst_start_f1, vburst_end_f1;
-	int vburst_start_f2, vburst_end_f2;
-	int vburst_start_f3, vburst_end_f3;
-	int vburst_start_f4, vburst_end_f4;
-	/*
-	 * subcarrier programming
-	 */
-	int dda2_size, dda3_size, dda1_inc, dda2_inc, dda3_inc;
-	u32 sc_reset;
-	bool pal_burst;
-	/*
-	 * blank/black levels
-	 */
-	const struct video_levels *composite_levels, *svideo_levels;
-	const struct color_conversion *composite_color, *svideo_color;
-	const u32 *filter_table;
-	int max_srcw;
-};
-
-
 /*
  * Sub carrier DDA
  *
@@ -928,32 +928,12 @@ intel_tv_dpms(struct drm_encoder *encoder, int mode)
 	}
 }
 
-static const struct tv_mode *
-intel_tv_mode_lookup(const char *tv_format)
-{
-	int i;
-
-	for (i = 0; i < sizeof(tv_modes) / sizeof (tv_modes[0]); i++) {
-		const struct tv_mode *tv_mode = &tv_modes[i];
-
-		if (!strcmp(tv_format, tv_mode->name))
-			return tv_mode;
-	}
-	return NULL;
-}
-
-static const struct tv_mode *
-intel_tv_mode_find(struct intel_tv *intel_tv)
-{
-	return intel_tv_mode_lookup(intel_tv->tv_format);
-}
-
 static enum drm_mode_status
 intel_tv_mode_valid(struct drm_connector *connector,
 		    struct drm_display_mode *mode)
 {
 	struct intel_tv *intel_tv = intel_attached_tv(connector);
-	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
+	const struct tv_mode *tv_mode = intel_tv->mode;
 
 	/* Ensure TV refresh is close to desired refresh */
 	if (tv_mode && abs(tv_mode->refresh - drm_mode_vrefresh(mode) * 1000)
@@ -971,7 +951,7 @@ intel_tv_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *mode,
 	struct drm_device *dev = encoder->dev;
 	struct drm_mode_config *drm_config = &dev->mode_config;
 	struct intel_tv *intel_tv = enc_to_intel_tv(encoder);
-	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
+	const struct tv_mode *tv_mode = intel_tv->mode;
 	struct drm_encoder *other_encoder;
 
 	if (!tv_mode)
@@ -997,7 +977,7 @@ intel_tv_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
 	struct drm_crtc *crtc = encoder->crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	struct intel_tv *intel_tv = enc_to_intel_tv(encoder);
-	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
+	const struct tv_mode *tv_mode = intel_tv->mode;
 	u32 tv_ctl;
 	u32 hctl1, hctl2, hctl3;
 	u32 vctl1, vctl2, vctl3, vctl4, vctl5, vctl6, vctl7;
@@ -1321,23 +1301,21 @@ intel_tv_detect_type (struct intel_tv *intel_tv,
 static void intel_tv_find_better_format(struct drm_connector *connector)
 {
 	struct intel_tv *intel_tv = intel_attached_tv(connector);
-	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
+	const struct tv_mode *tv_mode = intel_tv->mode;
+	bool is_component_only = intel_tv->type == DRM_MODE_CONNECTOR_Component;
 	int i;
 
-	if ((intel_tv->type == DRM_MODE_CONNECTOR_Component) ==
-		tv_mode->component_only)
+	if (is_component_only == tv_mode->component_only)
 		return;
 
-
-	for (i = 0; i < sizeof(tv_modes) / sizeof(*tv_modes); i++) {
+	for (i = 0; i < ARRAY_SIZE(tv_modes); i++) {
 		tv_mode = tv_modes + i;
 
-		if ((intel_tv->type == DRM_MODE_CONNECTOR_Component) ==
-			tv_mode->component_only)
+		if (is_component_only == tv_mode->component_only)
 			break;
 	}
 
-	intel_tv->tv_format = tv_mode->name;
+	intel_tv->mode = tv_mode;
 	drm_connector_property_set_value(connector,
 		connector->dev->mode_config.tv_mode_property, i);
 }
@@ -1404,7 +1382,7 @@ intel_tv_chose_preferred_modes(struct drm_connector *connector,
 			       struct drm_display_mode *mode_ptr)
 {
 	struct intel_tv *intel_tv = intel_attached_tv(connector);
-	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
+	const struct tv_mode *tv_mode = intel_tv->mode;
 
 	if (tv_mode->nbr_end < 480 && mode_ptr->vdisplay == 480)
 		mode_ptr->type |= DRM_MODE_TYPE_PREFERRED;
@@ -1429,7 +1407,7 @@ intel_tv_get_modes(struct drm_connector *connector)
 {
 	struct drm_display_mode *mode_ptr;
 	struct intel_tv *intel_tv = intel_attached_tv(connector);
-	const struct tv_mode *tv_mode = intel_tv_mode_find(intel_tv);
+	const struct tv_mode *tv_mode = intel_tv->mode;
 	int j, count = 0;
 	u64 tmp;
 
@@ -1523,10 +1501,10 @@ intel_tv_set_property(struct drm_connector *connector, struct drm_property *prop
 			ret = -EINVAL;
 			goto out;
 		}
-		if (!strcmp(intel_tv->tv_format, tv_modes[val].name))
+		if (intel_tv->mode == tv_modes + val)
 			goto out;
 
-		intel_tv->tv_format = tv_modes[val].name;
+		intel_tv->mode = &tv_modes[val];
 		changed = true;
 	} else {
 		ret = -EINVAL;
@@ -1693,7 +1671,7 @@ intel_tv_init(struct drm_device *dev)
 	intel_tv->margin[TV_MARGIN_RIGHT] = 46;
 	intel_tv->margin[TV_MARGIN_BOTTOM] = 37;
 
-	intel_tv->tv_format = tv_modes[initial_mode].name;
+	intel_tv->mode = &tv_modes[initial_mode];
 
 	drm_encoder_helper_add(&intel_encoder->base, &intel_tv_helper_funcs);
 	drm_connector_helper_add(connector, &intel_tv_connector_helper_funcs);
-- 
1.7.5.1

Re: [PATCH 05/16] drm/i915/tv: Use a direct pointer for tv_mode

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 698 bytes --]

On Thu, 12 May 2011 22:17:13 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Instead of scanning through the static array using strcmp to find our
> matching tv_mode, just keep a direct link around. The historical reason
> for the strcmp was to match the connection property "tv format", but now
> that property is translated for us into an index by the drm core.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mathew McKernan <matmckernan@rauland.com.au>

Nice cleanup. Note that I cannot test this patch because TV out does not
work on the 965GM or GM45 machines that I have.

Reviewed-by: Keith Packard <keithp@keithp.com>

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 06/16] drm/i915: Replace ironlake_compute_wm0 with g4x_compute_wm0

[Chris Wilson]

The computation of the first-level watermarks for g4x and gen5+ are
based on the same algorithm, so we can refactor those code paths to
use a single function.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |   88 ++++++++--------------------------
 1 files changed, 20 insertions(+), 68 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 784e52c..4e1042b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3862,54 +3862,6 @@ static void i830_update_wm(struct drm_device *dev)
 #define ILK_LP0_PLANE_LATENCY		700
 #define ILK_LP0_CURSOR_LATENCY		1300
 
-static bool ironlake_compute_wm0(struct drm_device *dev,
-				 int pipe,
-				 const struct intel_watermark_params *display,
-				 int display_latency_ns,
-				 const struct intel_watermark_params *cursor,
-				 int cursor_latency_ns,
-				 int *plane_wm,
-				 int *cursor_wm)
-{
-	struct drm_crtc *crtc;
-	int htotal, hdisplay, clock, pixel_size;
-	int line_time_us, line_count;
-	int entries, tlb_miss;
-
-	crtc = intel_get_crtc_for_pipe(dev, pipe);
-	if (crtc->fb == NULL || !crtc->enabled)
-		return false;
-
-	htotal = crtc->mode.htotal;
-	hdisplay = crtc->mode.hdisplay;
-	clock = crtc->mode.clock;
-	pixel_size = crtc->fb->bits_per_pixel / 8;
-
-	/* Use the small buffer method to calculate plane watermark */
-	entries = ((clock * pixel_size / 1000) * display_latency_ns) / 1000;
-	tlb_miss = display->fifo_size*display->cacheline_size - hdisplay * 8;
-	if (tlb_miss > 0)
-		entries += tlb_miss;
-	entries = DIV_ROUND_UP(entries, display->cacheline_size);
-	*plane_wm = entries + display->guard_size;
-	if (*plane_wm > (int)display->max_wm)
-		*plane_wm = display->max_wm;
-
-	/* Use the large buffer method to calculate cursor watermark */
-	line_time_us = ((htotal * 1000) / clock);
-	line_count = (cursor_latency_ns / line_time_us + 1000) / 1000;
-	entries = line_count * 64 * pixel_size;
-	tlb_miss = cursor->fifo_size*cursor->cacheline_size - hdisplay * 8;
-	if (tlb_miss > 0)
-		entries += tlb_miss;
-	entries = DIV_ROUND_UP(entries, cursor->cacheline_size);
-	*cursor_wm = entries + cursor->guard_size;
-	if (*cursor_wm > (int)cursor->max_wm)
-		*cursor_wm = (int)cursor->max_wm;
-
-	return true;
-}
-
 /*
  * Check the wm result.
  *
@@ -4018,12 +3970,12 @@ static void ironlake_update_wm(struct drm_device *dev)
 	unsigned int enabled;
 
 	enabled = 0;
-	if (ironlake_compute_wm0(dev, 0,
-				 &ironlake_display_wm_info,
-				 ILK_LP0_PLANE_LATENCY,
-				 &ironlake_cursor_wm_info,
-				 ILK_LP0_CURSOR_LATENCY,
-				 &plane_wm, &cursor_wm)) {
+	if (g4x_compute_wm0(dev, 0,
+			    &ironlake_display_wm_info,
+			    ILK_LP0_PLANE_LATENCY,
+			    &ironlake_cursor_wm_info,
+			    ILK_LP0_CURSOR_LATENCY,
+			    &plane_wm, &cursor_wm)) {
 		I915_WRITE(WM0_PIPEA_ILK,
 			   (plane_wm << WM0_PIPE_PLANE_SHIFT) | cursor_wm);
 		DRM_DEBUG_KMS("FIFO watermarks For pipe A -"
@@ -4032,12 +3984,12 @@ static void ironlake_update_wm(struct drm_device *dev)
 		enabled |= 1;
 	}
 
-	if (ironlake_compute_wm0(dev, 1,
-				 &ironlake_display_wm_info,
-				 ILK_LP0_PLANE_LATENCY,
-				 &ironlake_cursor_wm_info,
-				 ILK_LP0_CURSOR_LATENCY,
-				 &plane_wm, &cursor_wm)) {
+	if (g4x_compute_wm0(dev, 1,
+			    &ironlake_display_wm_info,
+			    ILK_LP0_PLANE_LATENCY,
+			    &ironlake_cursor_wm_info,
+			    ILK_LP0_CURSOR_LATENCY,
+			    &plane_wm, &cursor_wm)) {
 		I915_WRITE(WM0_PIPEB_ILK,
 			   (plane_wm << WM0_PIPE_PLANE_SHIFT) | cursor_wm);
 		DRM_DEBUG_KMS("FIFO watermarks For pipe B -"
@@ -4102,10 +4054,10 @@ static void sandybridge_update_wm(struct drm_device *dev)
 	unsigned int enabled;
 
 	enabled = 0;
-	if (ironlake_compute_wm0(dev, 0,
-				 &sandybridge_display_wm_info, latency,
-				 &sandybridge_cursor_wm_info, latency,
-				 &plane_wm, &cursor_wm)) {
+	if (g4x_compute_wm0(dev, 0,
+			    &sandybridge_display_wm_info, latency,
+			    &sandybridge_cursor_wm_info, latency,
+			    &plane_wm, &cursor_wm)) {
 		I915_WRITE(WM0_PIPEA_ILK,
 			   (plane_wm << WM0_PIPE_PLANE_SHIFT) | cursor_wm);
 		DRM_DEBUG_KMS("FIFO watermarks For pipe A -"
@@ -4114,10 +4066,10 @@ static void sandybridge_update_wm(struct drm_device *dev)
 		enabled |= 1;
 	}
 
-	if (ironlake_compute_wm0(dev, 1,
-				 &sandybridge_display_wm_info, latency,
-				 &sandybridge_cursor_wm_info, latency,
-				 &plane_wm, &cursor_wm)) {
+	if (g4x_compute_wm0(dev, 1,
+			    &sandybridge_display_wm_info, latency,
+			    &sandybridge_cursor_wm_info, latency,
+			    &plane_wm, &cursor_wm)) {
 		I915_WRITE(WM0_PIPEB_ILK,
 			   (plane_wm << WM0_PIPE_PLANE_SHIFT) | cursor_wm);
 		DRM_DEBUG_KMS("FIFO watermarks For pipe B -"
-- 
1.7.5.1

Re: [PATCH 06/16] drm/i915: Replace ironlake_compute_wm0 with g4x_compute_wm0

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 432 bytes --]

On Thu, 12 May 2011 22:17:14 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> The computation of the first-level watermarks for g4x and gen5+ are
> based on the same algorithm, so we can refactor those code paths to
> use a single function.

g4x_compute_wm0 takes a plane. ironlake_compute_wm0 takes a pipe. The
change implicitly assumes that pipe == plane on ironlake. Please clarify?

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 06/16] drm/i915: Replace ironlake_compute_wm0 with g4x_compute_wm0

[Chris Wilson]

On Thu, 12 May 2011 17:34:49 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Thu, 12 May 2011 22:17:14 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > The computation of the first-level watermarks for g4x and gen5+ are
> > based on the same algorithm, so we can refactor those code paths to
> > use a single function.
> 
> g4x_compute_wm0 takes a plane. ironlake_compute_wm0 takes a pipe. The
> change implicitly assumes that pipe == plane on ironlake. Please clarify?

We lucked out. It should have been a plane on Ironlake as well.
Fortunately on ILK+ each pipe is slaved to one (identically numbered)
plane.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 06/16] drm/i915: Replace ironlake_compute_wm0 with g4x_compute_wm0

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 871 bytes --]

On Fri, 13 May 2011 10:19:52 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, 12 May 2011 17:34:49 -0700, Keith Packard <keithp@keithp.com> wrote:
> > On Thu, 12 May 2011 22:17:14 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > The computation of the first-level watermarks for g4x and gen5+ are
> > > based on the same algorithm, so we can refactor those code paths to
> > > use a single function.
> > 
> > g4x_compute_wm0 takes a plane. ironlake_compute_wm0 takes a pipe. The
> > change implicitly assumes that pipe == plane on ironlake. Please clarify?
> 
> We lucked out. It should have been a plane on Ironlake as well.
> Fortunately on ILK+ each pipe is slaved to one (identically numbered)
> plane.

Heh. Please make sure you mention stuff like that in commit messages
when you find them.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 07/16] drm/i915/crt: Explicitly return false if connected to a digital monitor

[Chris Wilson]

Rather than proceed on and silently return false by default, mention why
we rejected the presence of an EDID as implying the presence of a VGA
monitor. (The question arises whether there is a broken EDID which falsely
reports a digital connection when attached by VGA.)

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_crt.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index e93f93c..0979d88 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -288,6 +288,8 @@ static bool intel_crt_detect_ddc(struct drm_connector *connector)
 		 * This may be a DVI-I connector with a shared DDC
 		 * link between analog and digital outputs, so we
 		 * have to check the EDID input spec of the attached device.
+		 *
+		 * On the other hand, what should we do if it is a broken EDID?
 		 */
 		if (edid != NULL) {
 			is_digital = edid->input & DRM_EDID_INPUT_DIGITAL;
@@ -298,6 +300,8 @@ static bool intel_crt_detect_ddc(struct drm_connector *connector)
 		if (!is_digital) {
 			DRM_DEBUG_KMS("CRT detected via DDC:0x50 [EDID]\n");
 			return true;
+		} else {
+			DRM_DEBUG_KMS("CRT not detected via DDC:0x50 [EDID reports a digital panel]\n");
 		}
 	}
 
-- 
1.7.5.1

[PATCH 08/16] drm/i915/i2c: Convert from using GMBUS1 + reg_offset idiom to reg + 0

[Chris Wilson]

Keith complained that GMBUSx + reg_offset was ugly. An alternative
naming scheme which is more consistent with the reset of the code base
is to store the address of the GMBUS0 and then reference each of the
GMBUSx registers as an offset from GMBUS0.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_i2c.c |   51 +++++++++++++++++++++----------------
 1 files changed, 29 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index d3b903b..ed11523 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -51,6 +51,11 @@ struct intel_gpio {
 	u32 reg;
 };
 
+static int intel_gmbus_reg0(struct drm_device *dev)
+{
+	return HAS_PCH_SPLIT(dev) ? PCH_GMBUS0 : GMBUS0;
+}
+
 void
 intel_i2c_reset(struct drm_device *dev)
 {
@@ -232,36 +237,36 @@ gmbus_xfer(struct i2c_adapter *adapter,
 					       struct intel_gmbus,
 					       adapter);
 	struct drm_i915_private *dev_priv = adapter->algo_data;
-	int i, reg_offset;
+	int i, reg;
 
 	if (bus->force_bit)
 		return intel_i2c_quirk_xfer(dev_priv,
 					    bus->force_bit, msgs, num);
 
-	reg_offset = HAS_PCH_SPLIT(dev_priv->dev) ? PCH_GMBUS0 - GMBUS0 : 0;
+	reg = intel_gmbus_reg0(dev_priv->dev);
 
-	I915_WRITE(GMBUS0 + reg_offset, bus->reg0);
+	I915_WRITE(reg + 0, bus->reg0);
 
 	for (i = 0; i < num; i++) {
 		u16 len = msgs[i].len;
 		u8 *buf = msgs[i].buf;
 
 		if (msgs[i].flags & I2C_M_RD) {
-			I915_WRITE(GMBUS1 + reg_offset,
+			I915_WRITE(reg + 1,
 				   GMBUS_CYCLE_WAIT | (i + 1 == num ? GMBUS_CYCLE_STOP : 0) |
 				   (len << GMBUS_BYTE_COUNT_SHIFT) |
 				   (msgs[i].addr << GMBUS_SLAVE_ADDR_SHIFT) |
 				   GMBUS_SLAVE_READ | GMBUS_SW_RDY);
-			POSTING_READ(GMBUS2+reg_offset);
+			POSTING_READ(reg + 2);
 			do {
 				u32 val, loop = 0;
 
-				if (wait_for(I915_READ(GMBUS2 + reg_offset) & (GMBUS_SATOER | GMBUS_HW_RDY), 50))
+				if (wait_for(I915_READ(reg + 2) & (GMBUS_SATOER | GMBUS_HW_RDY), 50))
 					goto timeout;
-				if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
+				if (I915_READ(reg + 2) & GMBUS_SATOER)
 					goto clear_err;
 
-				val = I915_READ(GMBUS3 + reg_offset);
+				val = I915_READ(reg + 3);
 				do {
 					*buf++ = val & 0xff;
 					val >>= 8;
@@ -275,18 +280,18 @@ gmbus_xfer(struct i2c_adapter *adapter,
 				val |= *buf++ << (8 * loop);
 			} while (--len && ++loop < 4);
 
-			I915_WRITE(GMBUS3 + reg_offset, val);
-			I915_WRITE(GMBUS1 + reg_offset,
+			I915_WRITE(reg + 3, val);
+			I915_WRITE(reg + 1,
 				   (i + 1 == num ? GMBUS_CYCLE_STOP : GMBUS_CYCLE_WAIT) |
 				   (msgs[i].len << GMBUS_BYTE_COUNT_SHIFT) |
 				   (msgs[i].addr << GMBUS_SLAVE_ADDR_SHIFT) |
 				   GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
-			POSTING_READ(GMBUS2+reg_offset);
+			POSTING_READ(reg + 2);
 
 			while (len) {
-				if (wait_for(I915_READ(GMBUS2 + reg_offset) & (GMBUS_SATOER | GMBUS_HW_RDY), 50))
+				if (wait_for(I915_READ(reg + 2) & (GMBUS_SATOER | GMBUS_HW_RDY), 50))
 					goto timeout;
-				if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
+				if (I915_READ(reg + 2) & GMBUS_SATOER)
 					goto clear_err;
 
 				val = loop = 0;
@@ -294,14 +299,15 @@ gmbus_xfer(struct i2c_adapter *adapter,
 					val |= *buf++ << (8 * loop);
 				} while (--len && ++loop < 4);
 
-				I915_WRITE(GMBUS3 + reg_offset, val);
-				POSTING_READ(GMBUS2+reg_offset);
+				I915_WRITE(reg + 3, val);
+				POSTING_READ(reg + 2);
 			}
 		}
 
-		if (i + 1 < num && wait_for(I915_READ(GMBUS2 + reg_offset) & (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE), 50))
+		if (i + 1 < num &&
+		    wait_for(I915_READ(reg + 2) & (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE), 50))
 			goto timeout;
-		if (I915_READ(GMBUS2 + reg_offset) & GMBUS_SATOER)
+		if (I915_READ(reg + 2) & GMBUS_SATOER)
 			goto clear_err;
 	}
 
@@ -312,22 +318,23 @@ clear_err:
 	 * of resetting the GMBUS controller and so clearing the
 	 * BUS_ERROR raised by the slave's NAK.
 	 */
-	I915_WRITE(GMBUS1 + reg_offset, GMBUS_SW_CLR_INT);
-	I915_WRITE(GMBUS1 + reg_offset, 0);
+	I915_WRITE(reg + 1, GMBUS_SW_CLR_INT);
+	I915_WRITE(reg + 1, 0);
 
 done:
 	/* Mark the GMBUS interface as disabled. We will re-enable it at the
 	 * start of the next xfer, till then let it sleep.
 	 */
-	I915_WRITE(GMBUS0 + reg_offset, 0);
+	I915_WRITE(reg + 0, 0);
 	return i;
 
 timeout:
 	DRM_INFO("GMBUS timed out, falling back to bit banging on pin %d [%s]\n",
 		 bus->reg0 & 0xff, bus->adapter.name);
-	I915_WRITE(GMBUS0 + reg_offset, 0);
+	intel_i2c_reset(dev_priv->dev);
 
-	/* Hardware may not support GMBUS over these pins? Try GPIO bitbanging instead. */
+	/* Hardware may not support GMBUS over these pins?
+	 * Try GPIO bitbanging instead. */
 	bus->force_bit = intel_gpio_create(dev_priv, bus->reg0 & 0xff);
 	if (!bus->force_bit)
 		return -ENOMEM;
-- 
1.7.5.1

Re: [PATCH 08/16] drm/i915/i2c: Convert from using GMBUS1 + reg_offset idiom to reg + 0

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 259 bytes --]

On Thu, 12 May 2011 22:17:16 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> -	I915_WRITE(GMBUS0 + reg_offset, 0);
> +	intel_i2c_reset(dev_priv->dev);

This looks like an unrelated change. Please split this out.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 08/16] drm/i915/i2c: Convert from using GMBUS1 + reg_offset idiom to reg + 0

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 733 bytes --]

On Thu, 12 May 2011 22:17:16 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Keith complained that GMBUSx + reg_offset was ugly. An alternative
> naming scheme which is more consistent with the reset of the code base
> is to store the address of the GMBUS0 and then reference each of the
> GMBUSx registers as an offset from GMBUS0.

This looks completely wrong -- GMBUS1 is GMBUS0 + 4, not GMBUS0 + 1.

How about a simple function that computes the GMBUS register address
based on the device and a number? like:

static int intel_gmbus_reg(struct drm_device *dev, int reg) {
        int     base = HAS_PCH_SPLIT(dev) ? PCH_GMBUS0 : GMBUS0;

        return base + reg * 4;
}

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 08/16] drm/i915/i2c: Convert from using GMBUS1 + reg_offset idiom to reg + 0

[Chris Wilson]

On Thu, 12 May 2011 17:40:42 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Thu, 12 May 2011 22:17:16 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > Keith complained that GMBUSx + reg_offset was ugly. An alternative
> > naming scheme which is more consistent with the reset of the code base
> > is to store the address of the GMBUS0 and then reference each of the
> > GMBUSx registers as an offset from GMBUS0.
> 
> This looks completely wrong -- GMBUS1 is GMBUS0 + 4, not GMBUS0 + 1.

That was shameful.

> How about a simple function that computes the GMBUS register address
> based on the device and a number? like:
> 
> static int intel_gmbus_reg(struct drm_device *dev, int reg) {
>         int     base = HAS_PCH_SPLIT(dev) ? PCH_GMBUS0 : GMBUS0;
> 
>         return base + reg * 4;

And how about something like:

#define I915_GMBUS_WRITE(reg, val) \
   I915_WRITE(intel_gmbus_reg(dev_priv->dev, reg), val)

I915_GMBUS_WRITE(0, val);

For the body?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 08/16] drm/i915/i2c: Convert from using GMBUS1 + reg_offset idiom to reg + 0

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 350 bytes --]

On Fri, 13 May 2011 10:28:34 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> And how about something like:
> 
> #define I915_GMBUS_WRITE(reg, val) \
>    I915_WRITE(intel_gmbus_reg(dev_priv->dev, reg), val)
> 
> I915_GMBUS_WRITE(0, val);

Yes, that's very pretty! And do give it a try this time :-)

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 08/16] drm/i915/i2c: Convert from using GMBUS1 + reg_offset idiom to reg + 0

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 470 bytes --]

On Fri, 13 May 2011 08:00:40 -0700, Keith Packard <keithp@keithp.com> wrote:
Non-text part: multipart/signed
> On Fri, 13 May 2011 10:28:34 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > And how about something like:
> > 
> > #define I915_GMBUS_WRITE(reg, val) \
> >    I915_WRITE(intel_gmbus_reg(dev_priv->dev, reg), val)
> > 
> > I915_GMBUS_WRITE(0, val);

I haven't seen a follow-up patch with this in place.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 08/16] drm/i915/i2c: Convert from using GMBUS1 + reg_offset idiom to reg + 0

[Chris Wilson]

On Fri, 03 Jun 2011 13:55:10 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Fri, 13 May 2011 08:00:40 -0700, Keith Packard <keithp@keithp.com> wrote:
> Non-text part: multipart/signed
> > On Fri, 13 May 2011 10:28:34 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > 
> > > And how about something like:
> > > 
> > > #define I915_GMBUS_WRITE(reg, val) \
> > >    I915_WRITE(intel_gmbus_reg(dev_priv->dev, reg), val)
> > > 
> > > I915_GMBUS_WRITE(0, val);
> 
> I haven't seen a follow-up patch with this in place.

I need to address the broader concerns raised by Jean Delvare first.
Once I have our i2c adapter to his liking, I can then get the code to
yours.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 08/16] drm/i915/i2c: Convert from using GMBUS1 + reg_offset idiom to reg + 0

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 289 bytes --]

On Sat, 04 Jun 2011 00:09:29 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> I need to address the broader concerns raised by Jean Delvare first.
> Once I have our i2c adapter to his liking, I can then get the code to
> yours.

Sounds good.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 08/16] drm/i915/i2c: Convert from using GMBUS1 + reg_offset idiom to reg + 0

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 343 bytes --]

On Sat, 04 Jun 2011 00:09:29 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> I need to address the broader concerns raised by Jean Delvare first.
> Once I have our i2c adapter to his liking, I can then get the code to
> yours.

Are you still planning on cleaning up the i2c stuff at some point?

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 08/16] drm/i915/i2c: Convert from using GMBUS1 + reg_offset idiom to reg + 0

[Chris Wilson]

On Wed, 13 Jul 2011 11:33:22 -0700, Keith Packard <keithp@keithp.com> wrote:
Non-text part: multipart/signed
> On Sat, 04 Jun 2011 00:09:29 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > I need to address the broader concerns raised by Jean Delvare first.
> > Once I have our i2c adapter to his liking, I can then get the code to
> > yours.
> 
> Are you still planning on cleaning up the i2c stuff at some point?

Still planning on doing so, yes. The cost of reading the EDID every 10s is
irksome.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 09/16] drm/i915/gmbus: Reset the controller on initialisation

[Chris Wilson]

Toggle the Software Clear Interrupt bit which resets the controller to
clear any prior BUS_ERROR condition before we begin to use the
controller in earnest.

Suggested-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_i2c.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index ed11523..bf1c72c 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -60,10 +60,14 @@ void
 intel_i2c_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	if (HAS_PCH_SPLIT(dev))
-		I915_WRITE(PCH_GMBUS0, 0);
-	else
-		I915_WRITE(GMBUS0, 0);
+	int reg = intel_gmbus_reg0(dev);
+
+	/* First reset the controller by toggling the Sw Clear Interrupt. */
+	I915_WRITE(reg + 1, GMBUS_SW_CLR_INT);
+	I915_WRITE(reg + 1, 0);
+
+	/* Then mark the controller as disabled. */
+	I915_WRITE(reg + 0, 0);
 }
 
 static void intel_i2c_quirk_set(struct drm_i915_private *dev_priv, bool enable)
-- 
1.7.5.1

Re: [PATCH 09/16] drm/i915/gmbus: Reset the controller on initialisation

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 439 bytes --]

On Thu, 12 May 2011 22:17:17 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Toggle the Software Clear Interrupt bit which resets the controller to
> clear any prior BUS_ERROR condition before we begin to use the
> controller in earnest.

Looks reasonable, except for the bad register offsets (see reply to
previous patch). I fear this code wasn't tested at all; it shouldn't
have worked.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 09/16] drm/i915/gmbus: Reset the controller on initialisation

[Chris Wilson]

On Thu, 12 May 2011 17:41:36 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Thu, 12 May 2011 22:17:17 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > Toggle the Software Clear Interrupt bit which resets the controller to
> > clear any prior BUS_ERROR condition before we begin to use the
> > controller in earnest.
> 
> Looks reasonable, except for the bad register offsets (see reply to
> previous patch). I fear this code wasn't tested at all; it shouldn't
> have worked.

It has been booting daily on several machines for a month. I agree it
wouldn't have worked, but the since we automatically fallback to GPIO
should it go south, the failures didn't stop the external monitors from
being lit up.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 09/16] drm/i915/gmbus: Reset the controller on initialisation

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 556 bytes --]

On Fri, 13 May 2011 10:32:25 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> It has been booting daily on several machines for a month. I agree it
> wouldn't have worked, but the since we automatically fallback to GPIO
> should it go south, the failures didn't stop the external monitors from
> being lit up.

Yeah, nice that we fall back. Do we want a warning in the kernel log
when that happens (once, not multiple times)? At least some way to
verify that the new code is doing what we expect it to do.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 09/16] drm/i915/gmbus: Reset the controller on initialisation

[Chris Wilson]

On Fri, 13 May 2011 08:01:51 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Fri, 13 May 2011 10:32:25 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > It has been booting daily on several machines for a month. I agree it
> > wouldn't have worked, but the since we automatically fallback to GPIO
> > should it go south, the failures didn't stop the external monitors from
> > being lit up.
> 
> Yeah, nice that we fall back. Do we want a warning in the kernel log
> when that happens (once, not multiple times)? At least some way to
> verify that the new code is doing what we expect it to do.

We give a warning when we fail a GMBUS sequence and switch (permanently)
to using GPIO. I've been desensitised to those warnings, because there is
sometimes, depending upon the hardware and probes, one channel which fails.
The fact that I'd so utterly broken the code didn't occur to me.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 09/16] drm/i915/gmbus: Reset the controller on initialisation

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 355 bytes --]

On Thu, 12 May 2011 22:17:17 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Toggle the Software Clear Interrupt bit which resets the controller to
> clear any prior BUS_ERROR condition before we begin to use the
> controller in earnest.

I don't have a new patch with corrected register referencing yet...

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 10/16] drm/i915: Retire requests before disabling pagefaults

[Chris Wilson]

As we cannot wait upon an object to be released by the GPU once we have
disabled pagefaults, process any pending retirements first in the hope
that we move any potential relocations off the active list.

References: https://bugs.freedesktop.org/show_bug.cgi?id=35733
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 20a4cc5..3c639ba 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -446,6 +446,12 @@ i915_gem_execbuffer_relocate(struct drm_device *dev,
 	struct drm_i915_gem_object *obj;
 	int ret = 0;
 
+	/* Try to move as many of the relocation targets off the active list
+	 * to avoid unnecessary fallbacks to the slow path, as we cannot wait
+	 * for the retirement with pagefaults disabled.
+	 */
+	i915_gem_retire_requests(dev);
+
 	/* This is the fast path and we cannot handle a pagefault whilst
 	 * holding the struct mutex lest the user pass in the relocations
 	 * contained within a mmaped bo. For in such a case we, the page
-- 
1.7.5.1

Re: [PATCH 10/16] drm/i915: Retire requests before disabling pagefaults

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 715 bytes --]

On Thu, 12 May 2011 22:17:18 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> As we cannot wait upon an object to be released by the GPU once we have
> disabled pagefaults, process any pending retirements first in the hope
> that we move any potential relocations off the active list.

This seems to indicate a problem in user space. In any hot-path code,
user space should already have correctly computed the relocation
values and so the kernel should not have to map and re-write the value.

Kernel relocations are a requirement of the architecture, but not
something we should hit in practice once the application has gotten
itself running in a steady state.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 11/16] drm/i915: not finding a fence is a non-recoverable condition

[Chris Wilson]

From: Daniel Vetter <daniel.vetter@ffwll.ch>

This happens in two cases:
- userspace got its fence accounting wrong or
- the kernel got its fence accounting wrong.

In both cases there's absolutely no point in calling evict_everything,
that will not magically bring back the missing fence. So return a
different (hopefully somewhat sensible) error code.

This has the added benefit that out-of-gtt can be distinguish from
broken fence accounting by simply looking at the ioctl return code.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 67ddda0..dd0cfac 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2663,7 +2663,7 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj,
 
 	reg = i915_find_fence_reg(dev, pipelined);
 	if (reg == NULL)
-		return -ENOSPC;
+		return -EDEADLK;
 
 	ret = i915_gem_object_flush_fence(obj, pipelined);
 	if (ret)
-- 
1.7.5.1

Re: [PATCH 11/16] drm/i915: not finding a fence is a non-recoverable condition

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 310 bytes --]

On Thu, 12 May 2011 22:17:19 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> -		return -ENOSPC;
> +		return -EDEADLK;

Would be nice to have the kernel print out a debugging message at this
point -- the only way to hit this is from a bug in user or kernel code.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 11/16] drm/i915: not finding a fence is a non-recoverable condition

[Daniel Vetter]

On Thu, May 12, 2011 at 06:10:08PM -0700, Keith Packard wrote:
> On Thu, 12 May 2011 22:17:19 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > -		return -ENOSPC;
> > +		return -EDEADLK;
> 
> Would be nice to have the kernel print out a debugging message at this
> point -- the only way to hit this is from a bug in user or kernel code.

We've had that once and dropped it. The reason was that there should be no
user-triggerable kernel message (which might pointlessly fill the logs).
With the different return value I think it's good enough to catch.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 12/16] drm/915: fix relaxed tiling on gen2: tile height

[Chris Wilson]

From: Daniel Vetter <daniel.vetter@ffwll.ch>

A tile on gen2 has a size of 2kb, stride of 128 bytes and 16 rows.

Userspace was broken and assumed 8 rows. Chris Wilson noted that the
kernel unfortunately can't reliable check that because libdrm rounds
up the size to the next bucket.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index dd0cfac..afdbbd9 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1486,8 +1486,9 @@ i915_gem_get_unfenced_gtt_alignment(struct drm_i915_gem_object *obj)
 	 * edge of an even tile row (where tile rows are counted as if the bo is
 	 * placed in a fenced gtt region).
 	 */
-	if (IS_GEN2(dev) ||
-	    (obj->tiling_mode == I915_TILING_Y && HAS_128_BYTE_Y_TILING(dev)))
+	if (IS_GEN2(dev))
+		tile_height = 16;
+	else if (obj->tiling_mode == I915_TILING_Y && HAS_128_BYTE_Y_TILING(dev))
 		tile_height = 32;
 	else
 		tile_height = 8;
-- 
1.7.5.1

Re: [PATCH 12/16] drm/915: fix relaxed tiling on gen2: tile height

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 710 bytes --]

On Thu, 12 May 2011 22:17:20 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> From: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> A tile on gen2 has a size of 2kb, stride of 128 bytes and 16 rows.
> 
> Userspace was broken and assumed 8 rows. Chris Wilson noted that the
> kernel unfortunately can't reliable check that because libdrm rounds
> up the size to the next bucket.

Please explain (in the commit message) the impact on both new and old
user space. I remember (vaguely) that this patch will cause old user
space to have issues.

If so, we'll need a way to detect new user space and provide correct
behaviour there without impacting old user space.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 12/16] drm/915: fix relaxed tiling on gen2: tile height

[Daniel Vetter]

On Thu, May 12, 2011 at 06:13:32PM -0700, Keith Packard wrote:
> On Thu, 12 May 2011 22:17:20 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > From: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > A tile on gen2 has a size of 2kb, stride of 128 bytes and 16 rows.
> > 
> > Userspace was broken and assumed 8 rows. Chris Wilson noted that the
> > kernel unfortunately can't reliable check that because libdrm rounds
> > up the size to the next bucket.
> 
> Please explain (in the commit message) the impact on both new and old
> user space. I remember (vaguely) that this patch will cause old user
> space to have issues.

No problem for old userspace. This only changes the number of rows from 32
to 16. This value is used in the kernel to align buffers correctly, i.e.
it will save perhaps a tiny bit of gtt. Userspace on the other hand
assumed only 8 rows, which lead to underallocating it the last tile row.

So this patch is more documentation of actual hw behaviour than anything
else - hopefully getting rid of gen2 tiling confusion once and for all.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 12/16] drm/915: fix relaxed tiling on gen2: tile height

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 1031 bytes --]

On Sun, 15 May 2011 22:43:41 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:

> No problem for old userspace. This only changes the number of rows from 32
> to 16. This value is used in the kernel to align buffers correctly, i.e.
> it will save perhaps a tiny bit of gtt. Userspace on the other hand
> assumed only 8 rows, which lead to underallocating it the last tile
> row.

I have this vague memory of some problem in the past with tiling and old
user space for Gen2 hardware. I assume that old user space will just do
bad things, but that there's nothing the kernel can do to fix it, right?

The requirement here is that kernel changes not break user space, even
if the kernel was just masking a user-space bug. If that isn't true for
any old user space version, then this should be fine.

> So this patch is more documentation of actual hw behaviour than anything
> else - hopefully getting rid of gen2 tiling confusion once and for all.

Cool. Thanks for the clarification.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 12/16] drm/915: fix relaxed tiling on gen2: tile height

[Daniel Vetter]

On Sun, May 15, 2011 at 02:58:26PM -0700, Keith Packard wrote:
> I have this vague memory of some problem in the past with tiling and old
> user space for Gen2 hardware. I assume that old user space will just do
> bad things, but that there's nothing the kernel can do to fix it, right?

Ok, I'll try to fill you in with a quick summary on this - after all,
we've not only had just one issue with tiling, so it's rather easy to get
lost ;-) Very short answer is "yes".

The tiling height bug (aligning to 8 rows instead of 16) in userspace has
been around forever (as far as I bothered to dig into history, at least).
But that never really showed up because we were rounding up to the next
fence size, which is a pot and at least .5mb (on gen2). So ceil(rows/8)
was never odd, which is the only case this bug manifests. This also wastes
tons of memory, so Chris Wilson developed the relaxed fencing scheme to
avoid allocating unneeded backing storage (and only reserving the full gtt
space if there's a fence attached to the object). Only with support for
relaxed fencing both in the kernel and userspace portion of the driver
could we actually hit the bug. Matters were made worse by our rather large
impedance mismatch between userspace and kernel releases: Distros have
been shipping broken userspace in their stable releases before the kernel
part has hit Linus-mainline (which is when people started to complain, at
around .38-rc6, iirc).

We've tried to detect such underallocated last tile rows in the kernel and
reject such tiling attempts. But libdrm reuses buffers as long as they're
big enough, so the last tile row might intentionally not be complete, but
also never used. We can't tell these two cases apart in the kernel, which
is why we've had to back out that change again and just absorb the
resulting flak.

Aside: I think we need to improve our efforts to make the new patches in
-next testable by the community to decrease that turn-around time mismatch
and catch such issues earlier. I'm tossing around ideas, but nothing
concrete yet.

Yours, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 13/16] drm/i915: Remove unused enum "chip_family"

[Chris Wilson]

Superseded by the tracking the render generation in the chipset
capabiltiies struct.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d029476..ca11444 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -907,13 +907,6 @@ struct drm_i915_file_private {
 	} mm;
 };
 
-enum intel_chip_family {
-	CHIP_I8XX = 0x01,
-	CHIP_I9XX = 0x02,
-	CHIP_I915 = 0x04,
-	CHIP_I965 = 0x08,
-};
-
 #define INTEL_INFO(dev)	(((struct drm_i915_private *) (dev)->dev_private)->info)
 
 #define IS_I830(dev)		((dev)->pci_device == 0x3577)
-- 
1.7.5.1

Re: [PATCH 13/16] drm/i915: Remove unused enum "chip_family"

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 334 bytes --]

On Thu, 12 May 2011 22:17:21 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Superseded by the tracking the render generation in the chipset
> capabiltiies struct.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Yes please.

Reviewed-by: Keith Packard <keithp@keithp.com>

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 14/16] drm/i915: Use PCI-ID to identify Broadwater and Crestline

[Chris Wilson]

... as they only had a single PCI-ID each, and so using the pci-id is
easier than using a capability bit.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_debugfs.c |    2 --
 drivers/gpu/drm/i915/i915_drv.c     |    9 +++------
 drivers/gpu/drm/i915/i915_drv.h     |    6 ++----
 3 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 48f7e257..6612a61 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -70,8 +70,6 @@ static int i915_capabilities(struct seq_file *m, void *data)
 	B(need_gfx_hws);
 	B(is_g4x);
 	B(is_pineview);
-	B(is_broadwater);
-	B(is_crestline);
 	B(has_fbc);
 	B(has_pipe_cxsr);
 	B(has_hotplug);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 50c6065..9f42a57 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -122,15 +122,12 @@ static const struct intel_device_info intel_i945gm_info = {
 };
 
 static const struct intel_device_info intel_i965g_info = {
-	.gen = 4, .is_broadwater = 1,
-	.has_hotplug = 1,
-	.has_overlay = 1,
+	.gen = 4, .has_hotplug = 1, .has_overlay = 1,
 };
 
 static const struct intel_device_info intel_i965gm_info = {
-	.gen = 4, .is_crestline = 1,
-	.is_mobile = 1, .has_fbc = 1, .has_hotplug = 1,
-	.has_overlay = 1,
+	.gen = 4, .has_hotplug = 1, .has_overlay = 1,
+	.is_mobile = 1, .has_fbc = 1,
 	.supports_tv = 1,
 };
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ca11444..ae1ab1b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -227,8 +227,6 @@ struct intel_device_info {
 	u8 need_gfx_hws : 1;
 	u8 is_g4x : 1;
 	u8 is_pineview : 1;
-	u8 is_broadwater : 1;
-	u8 is_crestline : 1;
 	u8 has_fbc : 1;
 	u8 has_pipe_cxsr : 1;
 	u8 has_hotplug : 1;
@@ -917,8 +915,8 @@ struct drm_i915_file_private {
 #define IS_I915GM(dev)		((dev)->pci_device == 0x2592)
 #define IS_I945G(dev)		((dev)->pci_device == 0x2772)
 #define IS_I945GM(dev)		(INTEL_INFO(dev)->is_i945gm)
-#define IS_BROADWATER(dev)	(INTEL_INFO(dev)->is_broadwater)
-#define IS_CRESTLINE(dev)	(INTEL_INFO(dev)->is_crestline)
+#define IS_BROADWATER(dev)	((dev)->pci_device == 0x2972)
+#define IS_CRESTLINE(dev)	((dev)->pci_device == 0x2a02)
 #define IS_GM45(dev)		((dev)->pci_device == 0x2A42)
 #define IS_G4X(dev)		(INTEL_INFO(dev)->is_g4x)
 #define IS_PINEVIEW_G(dev)	((dev)->pci_device == 0xa001)
-- 
1.7.5.1

Re: [PATCH 14/16] drm/i915: Use PCI-ID to identify Broadwater and Crestline

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 397 bytes --]

On Thu, 12 May 2011 22:17:22 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> ... as they only had a single PCI-ID each, and so using the pci-id is
> easier than using a capability bit.

This doesn't seem useful to me; it only saves a couple of bits in the
struct and replaces that with compares. Meh.

Nacked-by: Keith Packard <keithp@keithp.com>

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 14/16] drm/i915: Use PCI-ID to identify Broadwater and Crestline

[Chris Wilson]

On Thu, 12 May 2011 18:16:00 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Thu, 12 May 2011 22:17:22 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > ... as they only had a single PCI-ID each, and so using the pci-id is
> > easier than using a capability bit.
> 
> This doesn't seem useful to me; it only saves a couple of bits in the
> struct and replaces that with compares. Meh.

The switch is from a comparatively expensive compare to a cheaper compare
*consistent* with the other individual chipset identifiers.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 14/16] drm/i915: Use PCI-ID to identify Broadwater and Crestline

[Daniel Vetter]

On Thu, May 12, 2011 at 06:16:00PM -0700, Keith Packard wrote:
> On Thu, 12 May 2011 22:17:22 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > ... as they only had a single PCI-ID each, and so using the pci-id is
> > easier than using a capability bit.
> 
> This doesn't seem useful to me; it only saves a couple of bits in the
> struct and replaces that with compares. Meh.
> 
> Nacked-by: Keith Packard <keithp@keithp.com>

I actually like this: I saves one needless indirection when reading
codepaths and trying to find out what code is run for a given pci id.
Also, these two bits seem to be the only ones that are used in only _one_
device type, which is a bit confusing.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 14/16] drm/i915: Use PCI-ID to identify Broadwater and Crestline

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 679 bytes --]

On Sun, 15 May 2011 22:49:02 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:

> I actually like this: I saves one needless indirection when reading
> codepaths and trying to find out what code is run for a given pci id.
> Also, these two bits seem to be the only ones that are used in only _one_
> device type, which is a bit confusing.

Yeah, we've got a bunch of single-bits per display generation; should we
create one of those for broadwater+crestline? That combination is used a
couple of times in the driver.

Thanks for your comments; I'll take this patch and we can consider
whether to add a bit for crestline+broadwater later.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH 14/16] drm/i915: Use PCI-ID to identify Broadwater and Crestline

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 289 bytes --]

On Thu, 12 May 2011 22:17:22 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> ... as they only had a single PCI-ID each, and so using the pci-id is
> easier than using a capability bit.

This patch no longer applies; do you want to update it?

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 15/16] drm/i915: Convert partial to full CPU read domain if we touch every page

[Chris Wilson]

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index afdbbd9..b92e8ea 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3206,7 +3206,9 @@ i915_gem_object_set_cpu_read_domain_range(struct drm_i915_gem_object *obj,
 	uint32_t old_read_domains;
 	int i, ret;
 
-	if (offset == 0 && size == obj->base.size)
+	/* If we touch all pages, just move the bo entirely to the CPU. */
+	if ((offset & PAGE_MASK) == 0 &&
+	    ALIGN(offset+size, PAGE_SIZE) == obj->base.size)
 		return i915_gem_object_set_to_cpu_domain(obj, 0);
 
 	ret = i915_gem_object_flush_gpu_write_domain(obj);
-- 
1.7.5.1

Re: [PATCH 15/16] drm/i915: Convert partial to full CPU read domain if we touch every page

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 230 bytes --]

On Thu, 12 May 2011 22:17:23 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

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

Can you justify this change with performance data?

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH 16/16] drm/i915: Share the common force-audio property between connectors

[Chris Wilson]

Make the audio property creation routine common and share the single
property between the connectors.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h    |    1 +
 drivers/gpu/drm/i915/intel_dp.c    |   15 ++-------------
 drivers/gpu/drm/i915/intel_drv.h   |    1 +
 drivers/gpu/drm/i915/intel_hdmi.c  |   14 ++------------
 drivers/gpu/drm/i915/intel_modes.c |   30 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_sdvo.c  |   14 ++------------
 6 files changed, 38 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ae1ab1b..d787a3d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -712,6 +712,7 @@ typedef struct drm_i915_private {
 	struct intel_fbdev *fbdev;
 
 	struct drm_property *broadcast_rgb_property;
+	struct drm_property *force_audio_property;
 
 	atomic_t forcewake_count;
 } drm_i915_private_t;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a4d8031..391b55f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -59,8 +59,6 @@ struct intel_dp {
 	bool is_pch_edp;
 	uint8_t	train_set[4];
 	uint8_t link_status[DP_LINK_STATUS_SIZE];
-
-	struct drm_property *force_audio_property;
 };
 
 /**
@@ -1702,7 +1700,7 @@ intel_dp_set_property(struct drm_connector *connector,
 	if (ret)
 		return ret;
 
-	if (property == intel_dp->force_audio_property) {
+	if (property == dev_priv->force_audio_property) {
 		int i = val;
 		bool has_audio;
 
@@ -1841,16 +1839,7 @@ bool intel_dpd_is_edp(struct drm_device *dev)
 static void
 intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *connector)
 {
-	struct drm_device *dev = connector->dev;
-
-	intel_dp->force_audio_property =
-		drm_property_create(dev, DRM_MODE_PROP_RANGE, "force_audio", 2);
-	if (intel_dp->force_audio_property) {
-		intel_dp->force_audio_property->values[0] = -1;
-		intel_dp->force_audio_property->values[1] = 1;
-		drm_connector_attach_property(connector, intel_dp->force_audio_property, 0);
-	}
-
+	intel_attach_force_audio_property(connector);
 	intel_attach_broadcast_rgb_property(connector);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 2e49b62..fed46b7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -236,6 +236,7 @@ struct intel_unpin_work {
 int intel_ddc_get_modes(struct drm_connector *c, struct i2c_adapter *adapter);
 extern bool intel_ddc_probe(struct intel_encoder *intel_encoder, int ddc_bus);
 
+extern void intel_attach_force_audio_property(struct drm_connector *connector);
 extern void intel_attach_broadcast_rgb_property(struct drm_connector *connector);
 
 extern void intel_crt_init(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index f289b86..70cde9e 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -45,7 +45,6 @@ struct intel_hdmi {
 	bool has_hdmi_sink;
 	bool has_audio;
 	int force_audio;
-	struct drm_property *force_audio_property;
 };
 
 static struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder)
@@ -287,7 +286,7 @@ intel_hdmi_set_property(struct drm_connector *connector,
 	if (ret)
 		return ret;
 
-	if (property == intel_hdmi->force_audio_property) {
+	if (property == dev_priv->force_audio_property) {
 		int i = val;
 		bool has_audio;
 
@@ -365,16 +364,7 @@ static const struct drm_encoder_funcs intel_hdmi_enc_funcs = {
 static void
 intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *connector)
 {
-	struct drm_device *dev = connector->dev;
-
-	intel_hdmi->force_audio_property =
-		drm_property_create(dev, DRM_MODE_PROP_RANGE, "force_audio", 2);
-	if (intel_hdmi->force_audio_property) {
-		intel_hdmi->force_audio_property->values[0] = -1;
-		intel_hdmi->force_audio_property->values[1] = 1;
-		drm_connector_attach_property(connector, intel_hdmi->force_audio_property, 0);
-	}
-
+	intel_attach_force_audio_property(connector);
 	intel_attach_broadcast_rgb_property(connector);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_modes.c b/drivers/gpu/drm/i915/intel_modes.c
index 9034dd8f..3b26a3b 100644
--- a/drivers/gpu/drm/i915/intel_modes.c
+++ b/drivers/gpu/drm/i915/intel_modes.c
@@ -81,6 +81,36 @@ int intel_ddc_get_modes(struct drm_connector *connector,
 	return ret;
 }
 
+static const char *force_audio_names[] = {
+	"off",
+	"auto",
+	"on",
+};
+
+void
+intel_attach_force_audio_property(struct drm_connector *connector)
+{
+	struct drm_device *dev = connector->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_property *prop;
+	int i;
+
+	prop = dev_priv->force_audio_property;
+	if (prop == NULL) {
+		prop = drm_property_create(dev, DRM_MODE_PROP_ENUM,
+					   "audio",
+					   ARRAY_SIZE(force_audio_names));
+		if (prop == NULL)
+			return;
+
+		for (i = 0; i < ARRAY_SIZE(force_audio_names); i++)
+			drm_property_add_enum(prop, i, i-1, force_audio_names[i]);
+
+		dev_priv->force_audio_property = prop;
+	}
+	drm_connector_attach_property(connector, prop, 0);
+}
+
 static const char *broadcast_rgb_names[] = {
 	"Full",
 	"Limited 16:235",
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 4324f33..d4cb369 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -148,8 +148,6 @@ struct intel_sdvo_connector {
 	int   format_supported_num;
 	struct drm_property *tv_format;
 
-	struct drm_property *force_audio_property;
-
 	/* add the property for the SDVO-TV */
 	struct drm_property *left;
 	struct drm_property *right;
@@ -1712,7 +1710,7 @@ intel_sdvo_set_property(struct drm_connector *connector,
 	if (ret)
 		return ret;
 
-	if (property == intel_sdvo_connector->force_audio_property) {
+	if (property == dev_priv->force_audio_property) {
 		int i = val;
 		bool has_audio;
 
@@ -2037,15 +2035,7 @@ intel_sdvo_add_hdmi_properties(struct intel_sdvo_connector *connector)
 {
 	struct drm_device *dev = connector->base.base.dev;
 
-	connector->force_audio_property =
-		drm_property_create(dev, DRM_MODE_PROP_RANGE, "force_audio", 2);
-	if (connector->force_audio_property) {
-		connector->force_audio_property->values[0] = -1;
-		connector->force_audio_property->values[1] = 1;
-		drm_connector_attach_property(&connector->base.base,
-					      connector->force_audio_property, 0);
-	}
-
+	intel_attach_force_audio_property(&connector->base.base);
 	if (INTEL_INFO(dev)->gen >= 4 && IS_MOBILE(dev))
 		intel_attach_broadcast_rgb_property(&connector->base.base);
 }
-- 
1.7.5.1

Re: [PATCH 16/16] drm/i915: Share the common force-audio property between connectors

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 336 bytes --]

On Thu, 12 May 2011 22:17:24 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Make the audio property creation routine common and share the single
> property between the connectors.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Keith Packard <keithp@keithp.com>

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: Select patches for 2.6.40

[Zou, Nanhai]

Hi Chris,
	How about Boqun's BSD irq fix patch?
That patch is important for smooth H.264 decoding on G45 and GM45.

Thanks
Zou Nanhai

>>-----Original Message-----
>>From: intel-gfx-bounces+nanhai.zou=intel.com@lists.freedesktop.org
>>[mailto:intel-gfx-bounces+nanhai.zou=intel.com@lists.freedesktop.org] On
>>Behalf Of Chris Wilson
>>Sent: 2011年5月13日 5:17
>>To: keithp@keithp.com
>>Cc: intel-gfx@lists.freedesktop.org
>>Subject: [Intel-gfx] Select patches for 2.6.40
>>
>>Hi Keith,
>>
>>These are the small patches above and beyond what you have already chosen
>>to include that I think will be useful for 2.6.40.
>>-Chris
>>
>>_______________________________________________
>>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