Bug 25544

Created attachment 31941 [details] [review]
possible fix

does this patch help?
As you suspect there are differences due to the two physical texture units on r200. Well, in fact there shouldn't be any differences due to that, but there are hardware bugs so need to emit state for texture units 0/1 pairwise (these workarounds are insufficient btw at least when using ati_fragment_shader, but that's another story).
I think there's an issue in the current code if neither a texture is bound to a unit, nor is it really enabled, in this case it will try to subtract the dwords it doesn't need for relocation twice, which can only happen with r200 (on rv250 it would never submit the state for this texture unit in the first place).

Yes, that patch resolves the error on R200 with no problems on rv250.

It looks like the texture unit differences and hardware bugs cause other issues on R200 not present on rv250; dark and strobed textures with Compiz alt-tab and cube effects whilst running most games crash on a "drmRadeonCmdBuffer: -22" error with the following dmesg.

[drm:r100_cs_track_texture_check] *ERROR* No texture bound to unit 1
[drm:radeon_cs_ioctl] *ERROR* Invalid command stream !

I'll open another bug after a bit more testing as I've just started using KMS.

(In reply to comment #2)
> Yes, that patch resolves the error on R200 with no problems on rv250.
> 
> It looks like the texture unit differences and hardware bugs cause other issues
> on R200 not present on rv250; dark and strobed textures with Compiz alt-tab and
> cube effects whilst running most games crash on a "drmRadeonCmdBuffer: -22"
> error with the following dmesg.
> 
> [drm:r100_cs_track_texture_check] *ERROR* No texture bound to unit 1
> [drm:radeon_cs_ioctl] *ERROR* Invalid command stream !
> 
> I'll open another bug after a bit more testing as I've just started using KMS.

Looks like the command verifier isn't getting along with fake enable of texture unit 1. So in some cases when unit 0 is active and 1 is not, we need to enable unit 1 too due to hw bug, but set to LOOKUP_DISABLE. Since that'll not actually sample from the "enabled" unit, that's just fine and it's not necessary a texture is bound on that unit. But cs_track_texture_check can't deal with that since it only sees unit 1 is enabled but no texture is actually bound and hence complains... 

I don't know how to fix this though, could either make the track_texture_check more clever or maybe the userspace driver should just bind some valid texture (maybe just take the one from unit 0?) so it doesn't look special from the kernel side.

Roland, any reason not to apply the patch in comment 1?

(In reply to comment #4)
> Roland, any reason not to apply the patch in comment 1?

No that's just fine. It just doesn't help much since you'll still get the cs verification failures (I believe you could also get these with rv250 not using that r200 workaround code, by using ati_fragment_shader and "disabled lookups" - I think the verifier needs some work, it also won't catch invalid textures if they happen in the first pass of ati_fragment_shader anyway probably).

Created attachment 33643 [details] [review]
drm_r200_disable_texture_check.patch

Until R200 fake texture enable is supported by CS checker, R200 users can just disable texture checking with attached patch to actually have working 3D.

*** Bug 31138 has been marked as a duplicate of this bug. ***

Alan, can texture checking be disabled for R200 in production code? Building a patched kernel module isn't a feasible option for most users, and the effect of this bug is to make the 3D hardware basically unusable.

Was this same checking even done in the past? Before all the KMS upheaval, the R200 driver was rock-solid. (R200 was, I believe, the last Radeon release to have full 3D specs before the long dry spell that only recently ended.)

Not as far as I'm aware unfortunately. Texture checking came in with the KMS rewrite IIRC.

(Updated bug summary to match problem.)

I'm looking at r100_cs_track_texture_check() in linux-2.6.36/drivers/gpu/drm/radeon/r100.c (starting at line 3217). What if instead of an unconditional return, you were to do e.g.

	/* XXX: Disable texture check until R200 fake texture enable supported */
	if (rdev->family == CHIP_R200)
		return 0;

? Other parts of that function are already conditionalized based on chipset.

Created attachment 39799 [details] [review]
fix the drm CS checker

This patch should fix the issue.  Please test and I'll get it upstream and into stable.

I've tested the patch and it works. Thanks.

I've sent the patch to Dave for upstream inclusion.

Confirmed here too! gltron now runs perfectly, as does OpenArena. Thank you, Alex, for pouncing on this one.

There's now a GPU-lockup problem (that existed before this fix) that I've reported as bug #31150.

This fix has been released in kernel 2.6.37-rc1, and queued for stable.

I would like to know how I can get this resolved as well. I am having all the same issues with a R100, I have tried running the latest kernel from Ubuntu reps. 2.6.37 rc3. I do not really know how to incorporate the attached patch that you have said resolved the issue. Any help is greatly appreciated.

(In reply to comment #16)
> I would like to know how I can get this resolved as well. I am having all the
> same issues with a R100, I have tried running the latest kernel from Ubuntu
> reps. 2.6.37 rc3. I do not really know how to incorporate the attached patch
> that you have said resolved the issue. Any help is greatly appreciated.

The patch is not relevant to r100.  If you are seeing the same symptom on r1xx, you are seeing a different issue.  Please open a new bug and include the drm error messages and information about how to reproduce the issue.

(In reply to comment #17)
> (In reply to comment #16)
> > I would like to know how I can get this resolved as well. I am having all the
> > same issues with a R100, I have tried running the latest kernel from Ubuntu
> > reps. 2.6.37 rc3. I do not really know how to incorporate the attached patch
> > that you have said resolved the issue. Any help is greatly appreciated.
> 
> The patch is not relevant to r100.  If you are seeing the same symptom on r1xx,
> you are seeing a different issue.  Please open a new bug and include the drm
> error messages and information about how to reproduce the issue.

I think one issue the r100 texture tracking still has is that the tex_coord_type is not reset to 0 if a 2d texture is used after a cube map (only reset after a clear). Not sure if that's the issue here though...

(In reply to comment #17)
> 
> The patch is not relevant to r100.  If you are seeing the same symptom on r1xx,
> you are seeing a different issue.  Please open a new bug and include the drm
> error messages and information about how to reproduce the issue.

I would also point out that 2.6.37rc3 already has Alex's fix for this issue.

Thank you for your prompt comments. Since it looks like I am dealing with a persistent bug on the R100, even using the latest from the xorg-edgers PPA and the 2.6.37-rc3, I will submit a new bug ticket with hopefully the important details.