[PATCH] drm/radeon/kms: r600 CS parser fixes

[PATCH] drm/radeon/kms: r600 CS parser fixes

[Alex Deucher]

- buffer offsets in the base regs are 256b aligned so
shift properly when comparing, fixed by Andre Maasikas
- mipmap size was calculated wrong when nlevel=0
- texture bo offsets were used after the bo base address was added
- vertex resource size register is size - 1, not size

Signed-off-by: Alex Deucher <alexdeucher@gmail.com>
Cc: Andre Maasikas <amaasikas@gmail.com>
---
 drivers/gpu/drm/radeon/r600_cs.c |   27 +++++++++++++++------------
 1 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/radeon/r600_cs.c b/drivers/gpu/drm/radeon/r600_cs.c
index c3ea212..52b5252 100644
--- a/drivers/gpu/drm/radeon/r600_cs.c
+++ b/drivers/gpu/drm/radeon/r600_cs.c
@@ -174,7 +174,7 @@ static inline int r600_cs_track_validate_cb(struct radeon_cs_parser *p, int i)
 		dev_warn(p->dev, "FMASK or CMASK buffer are not supported by this kernel\n");
 		return -EINVAL;
 	}
-	size = radeon_bo_size(track->cb_color_bo[i]);
+	size = radeon_bo_size(track->cb_color_bo[i]) - track->cb_color_bo_offset[i];
 	if (r600_bpe_from_format(&bpe, G_0280A0_FORMAT(track->cb_color_info[i]))) {
 		dev_warn(p->dev, "%s:%d cb invalid format %d for %d (0x%08X)\n",
 			 __func__, __LINE__, G_0280A0_FORMAT(track->cb_color_info[i]),
@@ -938,7 +938,7 @@ static inline int r600_cs_check_reg(struct radeon_cs_parser *p, u32 reg, u32 idx
 			return -EINVAL;
 		}
 		tmp = (reg - CB_COLOR0_BASE) / 4;
-		track->cb_color_bo_offset[tmp] = radeon_get_ib_value(p, idx);
+		track->cb_color_bo_offset[tmp] = radeon_get_ib_value(p, idx) << 8;
 		ib[idx] += (u32)((reloc->lobj.gpu_offset >> 8) & 0xffffffff);
 		track->cb_color_base_last[tmp] = ib[idx];
 		track->cb_color_bo[tmp] = reloc->robj;
@@ -950,7 +950,7 @@ static inline int r600_cs_check_reg(struct radeon_cs_parser *p, u32 reg, u32 idx
 					"0x%04X\n", reg);
 			return -EINVAL;
 		}
-		track->db_offset = radeon_get_ib_value(p, idx);
+		track->db_offset = radeon_get_ib_value(p, idx) << 8;
 		ib[idx] += (u32)((reloc->lobj.gpu_offset >> 8) & 0xffffffff);
 		track->db_bo = reloc->robj;
 		break;
@@ -1055,10 +1055,10 @@ static void r600_texture_size(unsigned nfaces, unsigned blevel, unsigned nlevels
 	}
 	*l0_size = ALIGN((w0 * bpe), pitch_align) * h0 * d0;
 	*mipmap_size = offset;
-	if (!blevel)
-		*mipmap_size -= *l0_size;
 	if (!nlevels)
 		*mipmap_size = *l0_size;
+	if (!blevel)
+		*mipmap_size -= *l0_size;
 }
 
 /**
@@ -1165,14 +1165,14 @@ static inline int r600_check_texture_resource(struct radeon_cs_parser *p,  u32 i
 			  (pitch_align * bpe),
 			  &l0_size, &mipmap_size);
 	/* using get ib will give us the offset into the texture bo */
-	word0 = radeon_get_ib_value(p, idx + 2);
+	word0 = radeon_get_ib_value(p, idx + 2) << 8;
 	if ((l0_size + word0) > radeon_bo_size(texture)) {
 		dev_warn(p->dev, "texture bo too small (%d %d %d %d -> %d have %ld)\n",
 			w0, h0, bpe, word0, l0_size, radeon_bo_size(texture));
 		return -EINVAL;
 	}
 	/* using get ib will give us the offset into the mipmap bo */
-	word0 = radeon_get_ib_value(p, idx + 3);
+	word0 = radeon_get_ib_value(p, idx + 3) << 8;
 	if ((mipmap_size + word0) > radeon_bo_size(mipmap)) {
 		dev_warn(p->dev, "mipmap bo too small (%d %d %d %d %d %d -> %d have %ld)\n",
 			w0, h0, bpe, blevel, nlevels, word0, mipmap_size, radeon_bo_size(texture));
@@ -1366,7 +1366,7 @@ static int r600_packet3_check(struct radeon_cs_parser *p,
 		}
 		for (i = 0; i < (pkt->count / 7); i++) {
 			struct radeon_bo *texture, *mipmap;
-			u32 size, offset;
+			u32 size, offset, base_offset, mip_offset;
 
 			switch (G__SQ_VTX_CONSTANT_TYPE(radeon_get_ib_value(p, idx+(i*7)+6+1))) {
 			case SQ_TEX_VTX_VALID_TEXTURE:
@@ -1376,7 +1376,7 @@ static int r600_packet3_check(struct radeon_cs_parser *p,
 					DRM_ERROR("bad SET_RESOURCE\n");
 					return -EINVAL;
 				}
-				ib[idx+1+(i*7)+2] += (u32)((reloc->lobj.gpu_offset >> 8) & 0xffffffff);
+				base_offset = (u32)((reloc->lobj.gpu_offset >> 8) & 0xffffffff);
 				if (reloc->lobj.tiling_flags & RADEON_TILING_MACRO)
 					ib[idx+1+(i*7)+0] |= S_038000_TILE_MODE(V_038000_ARRAY_2D_TILED_THIN1);
 				else if (reloc->lobj.tiling_flags & RADEON_TILING_MICRO)
@@ -1388,12 +1388,14 @@ static int r600_packet3_check(struct radeon_cs_parser *p,
 					DRM_ERROR("bad SET_RESOURCE\n");
 					return -EINVAL;
 				}
-				ib[idx+1+(i*7)+3] += (u32)((reloc->lobj.gpu_offset >> 8) & 0xffffffff);
+				mip_offset = (u32)((reloc->lobj.gpu_offset >> 8) & 0xffffffff);
 				mipmap = reloc->robj;
 				r = r600_check_texture_resource(p,  idx+(i*7)+1,
 								texture, mipmap, reloc->lobj.tiling_flags);
 				if (r)
 					return r;
+				ib[idx+1+(i*7)+2] += base_offset;
+				ib[idx+1+(i*7)+3] += mip_offset;
 				break;
 			case SQ_TEX_VTX_VALID_BUFFER:
 				/* vtx base */
@@ -1403,10 +1405,11 @@ static int r600_packet3_check(struct radeon_cs_parser *p,
 					return -EINVAL;
 				}
 				offset = radeon_get_ib_value(p, idx+1+(i*7)+0);
-				size = radeon_get_ib_value(p, idx+1+(i*7)+1);
+				size = radeon_get_ib_value(p, idx+1+(i*7)+1) + 1;
 				if (p->rdev && (size + offset) > radeon_bo_size(reloc->robj)) {
 					/* force size to size of the buffer */
-					dev_warn(p->dev, "vbo resource seems too big for the bo\n");
+					dev_warn(p->dev, "vbo resource seems too big (%d) for the bo (%ld)\n",
+						 size + offset, radeon_bo_size(reloc->robj));
 					ib[idx+1+(i*7)+1] = radeon_bo_size(reloc->robj);
 				}
 				ib[idx+1+(i*7)+0] += (u32)((reloc->lobj.gpu_offset) & 0xffffffff);
-- 
1.7.1.1

Re: [PATCH] drm/radeon/kms: r600 CS parser fixes

[Andy Furniss]

Alex Deucher wrote:
> - buffer offsets in the base regs are 256b aligned so
> shift properly when comparing, fixed by Andre Maasikas
> - mipmap size was calculated wrong when nlevel=0
> - texture bo offsets were used after the bo base address was added
> - vertex resource size register is size - 1, not size

I tried this patch on a d-r-t from 100805 (todays d-r-t seems to have 
reverted to a month ago and doesn't work well with tiling)

The patch, while fixing mesa demos lodbias and cubemap, seems to regress 
ut2004 and doom3 demos -

radeon 0000:01:00.0: mipmap bo too small (128 128 4 0 7 393216 -> 458816 
have 655360)

Re: [PATCH] drm/radeon/kms: r600 CS parser fixes

[Jon Sturm]

On Tue, Aug 10, 2010 at 10:14 AM, Andy Furniss <andyqos@ukfsn.org> wrote:
> Alex Deucher wrote:
>>
>> - buffer offsets in the base regs are 256b aligned so
>> shift properly when comparing, fixed by Andre Maasikas
>> - mipmap size was calculated wrong when nlevel=0
>> - texture bo offsets were used after the bo base address was added
>> - vertex resource size register is size - 1, not size
>
> I tried this patch on a d-r-t from 100805 (todays d-r-t seems to have
> reverted to a month ago and doesn't work well with tiling)
>
> The patch, while fixing mesa demos lodbias and cubemap, seems to regress
> ut2004 and doom3 demos -

ut2004 has been having issues for a while so I wouldn't blame this
patch 100%, Then again my issues seem to be similar to
https://bugs.freedesktop.org/show_bug.cgi?id=27443 which may or may
not be related.

>
> radeon 0000:01:00.0: mipmap bo too small (128 128 4 0 7 393216 -> 458816
> have 655360)
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Re: [PATCH] drm/radeon/kms: r600 CS parser fixes

[Andy Furniss]

Jon Sturm wrote:

> ut2004 has been having issues for a while so I wouldn't blame this
> patch 100%, Then again my issues seem to be similar to
> https://bugs.freedesktop.org/show_bug.cgi?id=27443 which may or may
> not be related.

Only having the demo and not seriously playing all levels (or much at 
all) I haven't hit that one.

This one does seem to be down to the patch. If you have the full game it 
could be things are different, as it's actually the nvidia ad that the 
demo starts with the provokes this. Doom3 demo I can get into a saved 
game, but it dies with the same error after about 10 secs.

Both work with the same d-r-t without the patch.

Re: [PATCH] drm/radeon/kms: r600 CS parser fixes

[Alex Deucher]

On Fri, Aug 13, 2010 at 6:38 AM, Andy Furniss <andyqos@ukfsn.org> wrote:
> Jon Sturm wrote:
>
>> ut2004 has been having issues for a while so I wouldn't blame this
>> patch 100%, Then again my issues seem to be similar to
>> https://bugs.freedesktop.org/show_bug.cgi?id=27443 which may or may
>> not be related.
>
> Only having the demo and not seriously playing all levels (or much at all) I
> haven't hit that one.
>
> This one does seem to be down to the patch. If you have the full game it
> could be things are different, as it's actually the nvidia ad that the demo
> starts with the provokes this. Doom3 demo I can get into a saved game, but
> it dies with the same error after about 10 secs.
>
> Both work with the same d-r-t without the patch.

Does reverting this part of the patch fix it?

@@ -1055,10 +1055,10 @@ static void r600_texture_size(unsigned nfaces,
unsigned blevel, unsigned nlevels
       }
       *l0_size = ALIGN((w0 * bpe), pitch_align) * h0 * d0;
       *mipmap_size = offset;
-       if (!blevel)
-               *mipmap_size -= *l0_size;
       if (!nlevels)
               *mipmap_size = *l0_size;
+       if (!blevel)
+               *mipmap_size -= *l0_size;
 }


> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Re: [PATCH] drm/radeon/kms: r600 CS parser fixes

[Andy Furniss]

Alex Deucher wrote:

> Does reverting this part of the patch fix it?
>
> @@ -1055,10 +1055,10 @@ static void r600_texture_size(unsigned nfaces,
> unsigned blevel, unsigned nlevels
>         }
>         *l0_size = ALIGN((w0 * bpe), pitch_align) * h0 * d0;
>         *mipmap_size = offset;
> -       if (!blevel)
> -               *mipmap_size -= *l0_size;
>         if (!nlevels)
>                 *mipmap_size = *l0_size;
> +       if (!blevel)
> +               *mipmap_size -= *l0_size;
>   }

No, it does make the nunbers bigger, though -

radeon 0000:01:00.0: mipmap bo too small (512 512 4 0 0 1048576 -> 
1048576 have 1409024)

Just as a double check this is my diff against current d-r-t for this test.

diff --git a/drivers/gpu/drm/radeon/r600_cs.c 
b/drivers/gpu/drm/radeon/r600_cs.c
index d886494..f6580ca 100644
--- a/drivers/gpu/drm/radeon/r600_cs.c
+++ b/drivers/gpu/drm/radeon/r600_cs.c
@@ -1051,10 +1051,10 @@ static void r600_texture_size(unsigned nfaces, 
unsigned blevel, unsigned nlevels
         }
         *l0_size = ALIGN((w0 * bpe), pitch_align) * h0 * d0;
         *mipmap_size = offset;
-       if (!nlevels)
-               *mipmap_size = *l0_size;
         if (!blevel)
                 *mipmap_size -= *l0_size;
+       if (!nlevels)
+               *mipmap_size = *l0_size;
  }

  /**

Re: [PATCH] drm/radeon/kms: r600 CS parser fixes

[Andy Furniss]

Andy Furniss wrote:

> No, it does make the nunbers bigger, though -
>
> radeon 0000:01:00.0: mipmap bo too small (512 512 4 0 0 1048576 ->
> 1048576 have 1409024)

Further (non exhaustive) testing shows it regresses some mesa demos as 
well - lodbias,reflect & shadowtex -

  mipmap bo too small (256 256 4 0 0 262144 -> 262144 have 360448)

Re: [PATCH] drm/radeon/kms: r600 CS parser fixes

[Alex Deucher]

On Wed, Aug 18, 2010 at 7:45 AM, Andy Furniss <andyqos@ukfsn.org> wrote:
> Andy Furniss wrote:
>
>> No, it does make the nunbers bigger, though -
>>
>> radeon 0000:01:00.0: mipmap bo too small (512 512 4 0 0 1048576 ->
>> 1048576 have 1409024)
>
> Further (non exhaustive) testing shows it regresses some mesa demos as well
> - lodbias,reflect & shadowtex -
>
>  mipmap bo too small (256 256 4 0 0 262144 -> 262144 have 360448)
>

The initial patch or the partial revert I just had you try?

Alex

Re: [PATCH] drm/radeon/kms: r600 CS parser fixes

[Andy Furniss]

Alex Deucher wrote:
> On Wed, Aug 18, 2010 at 7:45 AM, Andy Furniss<andyqos@ukfsn.org>  wrote:
>> Andy Furniss wrote:
>>
>>> No, it does make the nunbers bigger, though -
>>>
>>> radeon 0000:01:00.0: mipmap bo too small (512 512 4 0 0 1048576 ->
>>> 1048576 have 1409024)
>>
>> Further (non exhaustive) testing shows it regresses some mesa demos as well
>> - lodbias,reflect&  shadowtex -
>>
>>   mipmap bo too small (256 256 4 0 0 262144 ->  262144 have 360448)
>>
>
> The initial patch or the partial revert I just had you try?
>
> Alex
>
The partial revert.

Re: [PATCH] drm/radeon/kms: r600 CS parser fixes

[Alex Deucher]

On Fri, Aug 13, 2010 at 6:38 AM, Andy Furniss <andyqos@ukfsn.org> wrote:
> Jon Sturm wrote:
>
>> ut2004 has been having issues for a while so I wouldn't blame this
>> patch 100%, Then again my issues seem to be similar to
>> https://bugs.freedesktop.org/show_bug.cgi?id=27443 which may or may
>> not be related.
>
> Only having the demo and not seriously playing all levels (or much at all) I
> haven't hit that one.
>
> This one does seem to be down to the patch. If you have the full game it
> could be things are different, as it's actually the nvidia ad that the demo
> starts with the provokes this. Doom3 demo I can get into a saved game, but
> it dies with the same error after about 10 secs.
>
> Both work with the same d-r-t without the patch.

Does this patch help?

diff --git a/drivers/gpu/drm/radeon/r600_cs.c b/drivers/gpu/drm/radeon/r600_cs.c
index d886494..e83fc88 100644
--- a/drivers/gpu/drm/radeon/r600_cs.c
+++ b/drivers/gpu/drm/radeon/r600_cs.c
@@ -1046,7 +1046,6 @@ static void r600_texture_size(unsigned nfaces,
unsigned blevel, unsigned nlevels
                        rowstride = ALIGN((width * bpe), pitch_align);
                        size = height * rowstride * depth;
                        offset += size;
-                       offset = (offset + 0x1f) & ~0x1f;
                }
        }
        *l0_size = ALIGN((w0 * bpe), pitch_align) * h0 * d0;


Alex

> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Re: [PATCH] drm/radeon/kms: r600 CS parser fixes

[Andy Furniss]

Alex Deucher wrote:

> Does this patch help?
>
> diff --git a/drivers/gpu/drm/radeon/r600_cs.c b/drivers/gpu/drm/radeon/r600_cs.c
> index d886494..e83fc88 100644
> --- a/drivers/gpu/drm/radeon/r600_cs.c
> +++ b/drivers/gpu/drm/radeon/r600_cs.c
> @@ -1046,7 +1046,6 @@ static void r600_texture_size(unsigned nfaces,
> unsigned blevel, unsigned nlevels
>                          rowstride = ALIGN((width * bpe), pitch_align);
>                          size = height * rowstride * depth;
>                          offset += size;
> -                       offset = (offset + 0x1f)&  ~0x1f;
>                  }
>          }
>          *l0_size = ALIGN((w0 * bpe), pitch_align) * h0 * d0;

No. it's still the same with this.