diff mbox

[15/16] drm/radeon: implement ring commit tracking

Message ID 1341830523-30320-16-git-send-email-deathsimple@vodafone.de (mailing list archive)
State New, archived
Headers show

Commit Message

Christian König July 9, 2012, 10:42 a.m. UTC
Signed-off-by: Christian König <deathsimple@vodafone.de>
---
 drivers/gpu/drm/radeon/radeon.h      |    3 +++
 drivers/gpu/drm/radeon/radeon_ring.c |   39 ++++++++++++++++++++++++++++++++--
 2 files changed, 40 insertions(+), 2 deletions(-)

Comments

Jerome Glisse July 9, 2012, 3:36 p.m. UTC | #1
On Mon, Jul 9, 2012 at 6:42 AM, Christian König <deathsimple@vodafone.de> wrote:
> Signed-off-by: Christian König <deathsimple@vodafone.de>

Bit too complex to my taste, what about attached patch, it's lot
simpler. (Haven't tested
the patch but it should work)

Cheers,
Jerome

> ---
>  drivers/gpu/drm/radeon/radeon.h      |    3 +++
>  drivers/gpu/drm/radeon/radeon_ring.c |   39 ++++++++++++++++++++++++++++++++--
>  2 files changed, 40 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
> index fef4257..9c11be8 100644
> --- a/drivers/gpu/drm/radeon/radeon.h
> +++ b/drivers/gpu/drm/radeon/radeon.h
> @@ -637,6 +637,9 @@ struct radeon_ring {
>         u32                     ptr_reg_shift;
>         u32                     ptr_reg_mask;
>         u32                     nop;
> +       unsigned                *track_back;
> +       unsigned                track_ptr;
> +       unsigned                track_mask;
>  };
>
>  /*
> diff --git a/drivers/gpu/drm/radeon/radeon_ring.c b/drivers/gpu/drm/radeon/radeon_ring.c
> index d9b2e45..994c98c 100644
> --- a/drivers/gpu/drm/radeon/radeon_ring.c
> +++ b/drivers/gpu/drm/radeon/radeon_ring.c
> @@ -276,6 +276,8 @@ void radeon_ring_commit(struct radeon_device *rdev, struct radeon_ring *ring)
>         DRM_MEMORYBARRIER();
>         WREG32(ring->wptr_reg, (ring->wptr << ring->ptr_reg_shift) & ring->ptr_reg_mask);
>         (void)RREG32(ring->wptr_reg);
> +       ring->track_back[ring->track_ptr++] = ring->wptr_old;
> +       ring->track_ptr &= ring->track_mask;
>  }
>
>  void radeon_ring_unlock_commit(struct radeon_device *rdev, struct radeon_ring *ring)
> @@ -362,6 +364,27 @@ bool radeon_ring_test_lockup(struct radeon_device *rdev, struct radeon_ring *rin
>         return false;
>  }
>
> +static unsigned radeon_ring_first_valid_commit(struct radeon_ring *ring)
> +{
> +       unsigned i, c, result = ring->track_ptr;
> +       i = ring->track_ptr - 1;
> +       while (i != ring->track_ptr) {
> +               i &= ring->track_mask;
> +               c = ring->track_back[i];
> +
> +               if (ring->wptr >= ring->rptr) {
> +                       if (c < ring->rptr || c >= ring->wptr)
> +                               break;
> +               } else {
> +                       if (c < ring->rptr && c >= ring->wptr)
> +                               break;
> +               }
> +
> +               result = i--;
> +       }
> +       return result;
> +}
> +
>  int radeon_ring_init(struct radeon_device *rdev, struct radeon_ring *ring,
>                      unsigned ring_size, unsigned align,
>                      unsigned rptr_offs, unsigned rptr_reg, unsigned wptr_reg,
> @@ -403,6 +426,10 @@ int radeon_ring_init(struct radeon_device *rdev, struct radeon_ring *ring,
>                         dev_err(rdev->dev, "(%d) ring map failed\n", r);
>                         return r;
>                 }
> +               ring->track_back = kmalloc(ring_size / align, GFP_KERNEL);
> +               memset(ring->track_back, 0, ring_size / align);
> +               ring->track_ptr = 0;
> +               ring->track_mask = ((ring->ring_size / 4) / align) - 1;
>         }
>         ring->ptr_mask = (ring->ring_size / 4) - 1;
>         ring->ring_free_dw = ring->ring_size / 4;
> @@ -422,6 +449,7 @@ void radeon_ring_fini(struct radeon_device *rdev, struct radeon_ring *ring)
>         ring->ready = false;
>         ring->ring = NULL;
>         ring->ring_obj = NULL;
> +       kfree(ring->track_back);
>         mutex_unlock(&rdev->ring_lock);
>
>         if (ring_obj) {
> @@ -447,7 +475,7 @@ static int radeon_debugfs_ring_info(struct seq_file *m, void *data)
>         struct radeon_device *rdev = dev->dev_private;
>         int ridx = *(int*)node->info_ent->data;
>         struct radeon_ring *ring = &rdev->ring[ridx];
> -       unsigned count, i, j;
> +       unsigned count, i, j, commit;
>
>         radeon_ring_free_size(rdev, ring);
>         count = (ring->ring_size / 4) - ring->ring_free_dw;
> @@ -457,9 +485,16 @@ static int radeon_debugfs_ring_info(struct seq_file *m, void *data)
>         seq_printf(m, "driver's copy of the rptr: 0x%08x\n", ring->rptr);
>         seq_printf(m, "%u free dwords in ring\n", ring->ring_free_dw);
>         seq_printf(m, "%u dwords in ring\n", count);
> +       commit = radeon_ring_first_valid_commit(ring);
>         i = ring->rptr;
>         for (j = 0; j <= count; j++) {
> -               seq_printf(m, "r[%04d]=0x%08x\n", i, ring->ring[i]);
> +               seq_printf(m, "r[%04x]=0x%08x", i, ring->ring[i]);
> +               if (commit != ring->track_ptr && ring->track_back[commit] == i) {
> +                       seq_printf(m, " <-");
> +                       ++commit;
> +                       commit &= ring->track_mask;
> +               }
> +               seq_printf(m, "\n");
>                 i = (i + 1) & ring->ptr_mask;
>         }
>         return 0;
> --
> 1.7.9.5
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Christian König July 9, 2012, 3:48 p.m. UTC | #2
On 09.07.2012 17:36, Jerome Glisse wrote:
> On Mon, Jul 9, 2012 at 6:42 AM, Christian König <deathsimple@vodafone.de> wrote:
>> Signed-off-by: Christian König <deathsimple@vodafone.de>
> Bit too complex to my taste, what about attached patch, it's lot
> simpler. (Haven't tested
> the patch but it should work)
Cool idea! Depending on the writeback mechanism might not be the best 
part of it, but in general speaking all rings should have more than 
enough scratch registers for that!!!

Going to change it.

Thanks,
Christian.

>
> Cheers,
> Jerome
>
>> ---
>>   drivers/gpu/drm/radeon/radeon.h      |    3 +++
>>   drivers/gpu/drm/radeon/radeon_ring.c |   39 ++++++++++++++++++++++++++++++++--
>>   2 files changed, 40 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
>> index fef4257..9c11be8 100644
>> --- a/drivers/gpu/drm/radeon/radeon.h
>> +++ b/drivers/gpu/drm/radeon/radeon.h
>> @@ -637,6 +637,9 @@ struct radeon_ring {
>>          u32                     ptr_reg_shift;
>>          u32                     ptr_reg_mask;
>>          u32                     nop;
>> +       unsigned                *track_back;
>> +       unsigned                track_ptr;
>> +       unsigned                track_mask;
>>   };
>>
>>   /*
>> diff --git a/drivers/gpu/drm/radeon/radeon_ring.c b/drivers/gpu/drm/radeon/radeon_ring.c
>> index d9b2e45..994c98c 100644
>> --- a/drivers/gpu/drm/radeon/radeon_ring.c
>> +++ b/drivers/gpu/drm/radeon/radeon_ring.c
>> @@ -276,6 +276,8 @@ void radeon_ring_commit(struct radeon_device *rdev, struct radeon_ring *ring)
>>          DRM_MEMORYBARRIER();
>>          WREG32(ring->wptr_reg, (ring->wptr << ring->ptr_reg_shift) & ring->ptr_reg_mask);
>>          (void)RREG32(ring->wptr_reg);
>> +       ring->track_back[ring->track_ptr++] = ring->wptr_old;
>> +       ring->track_ptr &= ring->track_mask;
>>   }
>>
>>   void radeon_ring_unlock_commit(struct radeon_device *rdev, struct radeon_ring *ring)
>> @@ -362,6 +364,27 @@ bool radeon_ring_test_lockup(struct radeon_device *rdev, struct radeon_ring *rin
>>          return false;
>>   }
>>
>> +static unsigned radeon_ring_first_valid_commit(struct radeon_ring *ring)
>> +{
>> +       unsigned i, c, result = ring->track_ptr;
>> +       i = ring->track_ptr - 1;
>> +       while (i != ring->track_ptr) {
>> +               i &= ring->track_mask;
>> +               c = ring->track_back[i];
>> +
>> +               if (ring->wptr >= ring->rptr) {
>> +                       if (c < ring->rptr || c >= ring->wptr)
>> +                               break;
>> +               } else {
>> +                       if (c < ring->rptr && c >= ring->wptr)
>> +                               break;
>> +               }
>> +
>> +               result = i--;
>> +       }
>> +       return result;
>> +}
>> +
>>   int radeon_ring_init(struct radeon_device *rdev, struct radeon_ring *ring,
>>                       unsigned ring_size, unsigned align,
>>                       unsigned rptr_offs, unsigned rptr_reg, unsigned wptr_reg,
>> @@ -403,6 +426,10 @@ int radeon_ring_init(struct radeon_device *rdev, struct radeon_ring *ring,
>>                          dev_err(rdev->dev, "(%d) ring map failed\n", r);
>>                          return r;
>>                  }
>> +               ring->track_back = kmalloc(ring_size / align, GFP_KERNEL);
>> +               memset(ring->track_back, 0, ring_size / align);
>> +               ring->track_ptr = 0;
>> +               ring->track_mask = ((ring->ring_size / 4) / align) - 1;
>>          }
>>          ring->ptr_mask = (ring->ring_size / 4) - 1;
>>          ring->ring_free_dw = ring->ring_size / 4;
>> @@ -422,6 +449,7 @@ void radeon_ring_fini(struct radeon_device *rdev, struct radeon_ring *ring)
>>          ring->ready = false;
>>          ring->ring = NULL;
>>          ring->ring_obj = NULL;
>> +       kfree(ring->track_back);
>>          mutex_unlock(&rdev->ring_lock);
>>
>>          if (ring_obj) {
>> @@ -447,7 +475,7 @@ static int radeon_debugfs_ring_info(struct seq_file *m, void *data)
>>          struct radeon_device *rdev = dev->dev_private;
>>          int ridx = *(int*)node->info_ent->data;
>>          struct radeon_ring *ring = &rdev->ring[ridx];
>> -       unsigned count, i, j;
>> +       unsigned count, i, j, commit;
>>
>>          radeon_ring_free_size(rdev, ring);
>>          count = (ring->ring_size / 4) - ring->ring_free_dw;
>> @@ -457,9 +485,16 @@ static int radeon_debugfs_ring_info(struct seq_file *m, void *data)
>>          seq_printf(m, "driver's copy of the rptr: 0x%08x\n", ring->rptr);
>>          seq_printf(m, "%u free dwords in ring\n", ring->ring_free_dw);
>>          seq_printf(m, "%u dwords in ring\n", count);
>> +       commit = radeon_ring_first_valid_commit(ring);
>>          i = ring->rptr;
>>          for (j = 0; j <= count; j++) {
>> -               seq_printf(m, "r[%04d]=0x%08x\n", i, ring->ring[i]);
>> +               seq_printf(m, "r[%04x]=0x%08x", i, ring->ring[i]);
>> +               if (commit != ring->track_ptr && ring->track_back[commit] == i) {
>> +                       seq_printf(m, " <-");
>> +                       ++commit;
>> +                       commit &= ring->track_mask;
>> +               }
>> +               seq_printf(m, "\n");
>>                  i = (i + 1) & ring->ptr_mask;
>>          }
>>          return 0;
>> --
>> 1.7.9.5
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Jerome Glisse July 9, 2012, 4:12 p.m. UTC | #3
On Mon, Jul 9, 2012 at 11:48 AM, Christian König
<deathsimple@vodafone.de> wrote:
> On 09.07.2012 17:36, Jerome Glisse wrote:
>>
>> On Mon, Jul 9, 2012 at 6:42 AM, Christian König <deathsimple@vodafone.de>
>> wrote:
>>>
>>> Signed-off-by: Christian König <deathsimple@vodafone.de>
>>
>> Bit too complex to my taste, what about attached patch, it's lot
>> simpler. (Haven't tested
>> the patch but it should work)
>
> Cool idea! Depending on the writeback mechanism might not be the best part
> of it, but in general speaking all rings should have more than enough
> scratch registers for that!!!
>
> Going to change it.
>
> Thanks,
> Christian.
>

Note that i haven't included the fence emit intentionally idea being
if you resume ring you fence to emit the last fence as soon as
possible and just pretend that the faulty ib succeeded.

Cheers,
Jerome
diff mbox

Patch

diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index fef4257..9c11be8 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -637,6 +637,9 @@  struct radeon_ring {
 	u32			ptr_reg_shift;
 	u32			ptr_reg_mask;
 	u32			nop;
+	unsigned		*track_back;
+	unsigned		track_ptr;
+	unsigned		track_mask;
 };
 
 /*
diff --git a/drivers/gpu/drm/radeon/radeon_ring.c b/drivers/gpu/drm/radeon/radeon_ring.c
index d9b2e45..994c98c 100644
--- a/drivers/gpu/drm/radeon/radeon_ring.c
+++ b/drivers/gpu/drm/radeon/radeon_ring.c
@@ -276,6 +276,8 @@  void radeon_ring_commit(struct radeon_device *rdev, struct radeon_ring *ring)
 	DRM_MEMORYBARRIER();
 	WREG32(ring->wptr_reg, (ring->wptr << ring->ptr_reg_shift) & ring->ptr_reg_mask);
 	(void)RREG32(ring->wptr_reg);
+	ring->track_back[ring->track_ptr++] = ring->wptr_old;
+	ring->track_ptr &= ring->track_mask;
 }
 
 void radeon_ring_unlock_commit(struct radeon_device *rdev, struct radeon_ring *ring)
@@ -362,6 +364,27 @@  bool radeon_ring_test_lockup(struct radeon_device *rdev, struct radeon_ring *rin
 	return false;
 }
 
+static unsigned radeon_ring_first_valid_commit(struct radeon_ring *ring)
+{
+	unsigned i, c, result = ring->track_ptr;
+	i = ring->track_ptr - 1;
+	while (i != ring->track_ptr) {
+		i &= ring->track_mask;
+		c = ring->track_back[i];
+
+		if (ring->wptr >= ring->rptr) {
+			if (c < ring->rptr || c >= ring->wptr)
+				break;
+		} else {
+			if (c < ring->rptr && c >= ring->wptr)
+				break;
+		}
+
+		result = i--;
+	}
+	return result;
+}
+
 int radeon_ring_init(struct radeon_device *rdev, struct radeon_ring *ring,
 		     unsigned ring_size, unsigned align,
 		     unsigned rptr_offs, unsigned rptr_reg, unsigned wptr_reg,
@@ -403,6 +426,10 @@  int radeon_ring_init(struct radeon_device *rdev, struct radeon_ring *ring,
 			dev_err(rdev->dev, "(%d) ring map failed\n", r);
 			return r;
 		}
+		ring->track_back = kmalloc(ring_size / align, GFP_KERNEL);
+		memset(ring->track_back, 0, ring_size / align);
+		ring->track_ptr = 0;
+		ring->track_mask = ((ring->ring_size / 4) / align) - 1;
 	}
 	ring->ptr_mask = (ring->ring_size / 4) - 1;
 	ring->ring_free_dw = ring->ring_size / 4;
@@ -422,6 +449,7 @@  void radeon_ring_fini(struct radeon_device *rdev, struct radeon_ring *ring)
 	ring->ready = false;
 	ring->ring = NULL;
 	ring->ring_obj = NULL;
+	kfree(ring->track_back);
 	mutex_unlock(&rdev->ring_lock);
 
 	if (ring_obj) {
@@ -447,7 +475,7 @@  static int radeon_debugfs_ring_info(struct seq_file *m, void *data)
 	struct radeon_device *rdev = dev->dev_private;
 	int ridx = *(int*)node->info_ent->data;
 	struct radeon_ring *ring = &rdev->ring[ridx];
-	unsigned count, i, j;
+	unsigned count, i, j, commit;
 
 	radeon_ring_free_size(rdev, ring);
 	count = (ring->ring_size / 4) - ring->ring_free_dw;
@@ -457,9 +485,16 @@  static int radeon_debugfs_ring_info(struct seq_file *m, void *data)
 	seq_printf(m, "driver's copy of the rptr: 0x%08x\n", ring->rptr);
 	seq_printf(m, "%u free dwords in ring\n", ring->ring_free_dw);
 	seq_printf(m, "%u dwords in ring\n", count);
+	commit = radeon_ring_first_valid_commit(ring);
 	i = ring->rptr;
 	for (j = 0; j <= count; j++) {
-		seq_printf(m, "r[%04d]=0x%08x\n", i, ring->ring[i]);
+		seq_printf(m, "r[%04x]=0x%08x", i, ring->ring[i]);
+		if (commit != ring->track_ptr && ring->track_back[commit] == i) {
+			seq_printf(m, " <-");
+			++commit;
+			commit &= ring->track_mask;
+		}
+		seq_printf(m, "\n");
 		i = (i + 1) & ring->ptr_mask;
 	}
 	return 0;