Bug 26686

Created attachment 33475 [details]
GTA3 screenshot

Can you bisect the commit that broke it for you?

http://kernel.osuosl.org/pub/software/scm/git/docs/git-bisect.html

(In reply to comment #2)
> Can you bisect the commit that broke it for you?

OK, i did it using WINE bisect tutorial, and this is the final output:

4f0f871730b76730ca58209181d16725b0c40184 is first bad commit
commit 4f0f871730b76730ca58209181d16725b0c40184
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Wed Feb 10 09:45:13 2010 +0000

    intel: Handle resetting of input params after EINTR during SET_TILING
    
    The SET_TILING is pernicious in that it overwrites the input arguments
    following an error in order to report the current tiling state of the
    buffer. This caught us by surprise as we then fed those arguments back
    into to the ioctl unmodified following an EINTR and so the kernel then
    reported success for the no-op. We interpreted this success as meaning
    that the tiling on the buffer had changed so updated our state and
    started using the buffer incorrectly in the new tiled/untiled manner.
    This lead to all sorts of random corruption and GPU hangs, even though
    the batch buffers would look sane (when the GPU had not wandered off
    into forbidden territory).
    
    References:
    
      Bug 25475 - [i915] Xorg crash / Execbuf while wedged
      http://bugs.freedesktop.org/show_bug.cgi?id=25475
    
      Bug 25554 - i830_uxa_prepare_access: gtt bo map failed: Input/output error
      http://bugs.freedesktop.org/show_bug.cgi?id=25554
    
    (And probably every other weird bug in the last few months.)
    
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

:040000 040000 0af4c96adb20514daff1251ec9fb5b542e8092f5 411ace5da42864017f0132efeb47c96778eca642 M      intel

Extremely unlikely to be a direct result of that commit.

I tried to comment out this line:

bo_gem->tiling_mode = set_tiling.tiling_mode;

and now things seem to work. I.e., this statement:

>it overwrites the input arguments following an error in
>order to report the current tiling state of the buffer.

should be false (or, at least, not quite true). This is proved by the fact that if i undo this part of commit:

-	if (ret != 0) {
-		*tiling_mode = bo_gem->tiling_mode;
-		return -errno;
-	}

, i.e. leave 'if' statement in place (and uncomment saving of current tiling_mode), then things also work, because tiling_mode is not saved on error now. So, i can conclude that sometimes the kernel doesn't report current tiling state correctly on error.

I added perror() before 'return -errno;' to see what error doesn't report tiling mode, and got "Invalid argument", i.e. EINVAL. Then i found this in i915_gem_set_tiling() in the kernel:

if (!i915_tiling_ok(dev, args->stride, obj->size, args->tiling_mode)) {
        mutex_lock(&dev->struct_mutex);
        drm_gem_object_unreference(obj);
        mutex_unlock(&dev->struct_mutex);
        return -EINVAL;
}

As you can see, it doesn't change tiling_mode.

So, there should be a check for error after ioctl(), and tiling_mode shouldn't be considered current.

Still present with libdrm 2.4.20.

commit fcf3e616eeeb289f96af1436d809f0a1a42bebb7
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Mon May 24 18:35:41 2010 +0100

    intel: Don't change tiling mode unless the kernel reports success.
    
    Fixes:
    
      Bug 26686 - Some textures are distorted with libdrm 2.4.18 in GTAVC&GTA3
      http://bugs.freedesktop.org/show_bug.cgi?id=26686
    
    This bug continues to haunt me. The kernel SET_TILING ioctl is
    inconsistent in its return values when reporting an error. If one of its
    sanity checks fail, then the input values are left unchanged. If the
    kernel later fails to change the tiling mode, then the input values are
    modified to match the current tiling on the object. In short, userspace
    cannot trust the return values upon error and so we must assume that
    upon error our current tiling mode matches reality and not update.

Ruslan, it would be invaluable if you could add an assert(ret == 0); and track down the source of the EINVAL.