Bug 74005 - [i965 Bisected]Piglit/glx_glx-make-glxdrawable-current fails
[i965 Bisected]Piglit/glx_glx-make-glxdrawable-current fails
Status: VERIFIED FIXED
Product: Mesa
Classification: Unclassified
Component: Drivers/DRI/i965
unspecified
All Linux (All)
: high major
Assigned To: Kristian Høgsberg
Intel 3D Bugs Mailing List
:
: 75797 77083 79448 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2014-01-24 02:08 UTC by lu hua
Modified: 2014-07-28 02:54 UTC (History)
11 users (show)

See Also:


Attachments
Patch (2.92 KB, patch)
2014-03-12 10:26 UTC, Iago Toral
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description lu hua 2014-01-24 02:08:02 UTC
System Environment:
--------------------------
Platform: Ironlake/Sandybridge/Ivybridge/Haswell/Baytrail
Libdrm:		(master)libdrm-2.4.52
Mesa:		(master)d8c7740ddabeb456243e40dc3cf0e86c7fca09d0
Xserver:	(master)xorg-server-1.15.0-1-ga92c6406e7f6c0b74fb1cb9361ad904facc0f722
Xf86_video_intel:		(master)2.99.907-48-g32010ed86bb8f28d3b02e1e54a592d79b92b2b98
Cairo:		(master)2a7f133639bed86a29dd9693cb78c0aa42eaff30
Libva:		(staging)79ddde6bab54618046f136471c020c08ff4fde50
Libva_intel_driver:		(staging)6736a39fce442a8e5abb2a2e99adf56e3965d35f
Kernel:	(drm-intel-nightly) 164a4cb4c1431a0689f85507868356fae24da638

Bug detailed description:
-----------------------------
It fails on Ironlake/Sandybridge/Ivybridge/Haswell/Baytrail with mesa master branch. It doesn't happen on 10.0 branch.

Bisect shows: 11baad35088dfd4bdabc1710df650dbfb413e7a3 is the first bad commit.
commit 11baad35088dfd4bdabc1710df650dbfb413e7a3
Author:     Kristian Høgsberg <krh@bitplanet.net>
AuthorDate: Tue Jan 21 12:17:03 2014 -0800
Commit:     Kristian Høgsberg <krh@bitplanet.net>
CommitDate: Wed Jan 22 12:30:59 2014 -0800

    intel: Fix initial MakeCurrent for single-buffer drawables

    Commit 05da4a7a5e7d5bd988cb31f94ed8e1f053d9ee39 attempts to eliminate the
    call to intel_update_renderbuffer() in the case where we already have a
    drawbuffer for the drawable.  Unfortunately this only checks the
    back left renderbuffer, which breaks in case of single buffer drawables.

    This means that the initial viewport will not be set in that case.  Instead,
    we now check whether the initial viewport has not been set, in which case
    we call out to intel_update_renderbuffer().

    https://bugs.freedesktop.org/show_bug.cgi?id=73862

    Signed-off-by: Kristian Høgsberg <krh@bitplanet.net>


output:
Probe color at (1,1)
  Expected: 0.000000 1.000000 0.000000
  Observed: 0.000000 0.000000 0.000000
PIGLIT: {'result': 'fail' }

Reproduce steps:
----------------------------
1. xinit
2. ./bin/glx-make-glxdrawable-current -auto
Comment 1 Ian Romanick 2014-02-12 18:49:26 UTC
If a regression bisects to a commit by an Intel employee, please assign the bug to that person.  Otherwise they may not notice the bug.

Does this problem still occur?  It should have been fixed by these commits:

commit f658150639c36eda351590e757247c56507f494f
Author: Kristian Høgsberg <krh@bitplanet.net>
Date:   Wed Feb 5 11:43:58 2014 -0800

    glx: Pass NULL DRI drawables into the DRI driver for None GLX drawables
    
    GLX_ARB_create_context allows making a GLX context current with None
    drawable and readables, but this was never implemented correctly in GLX.
    We would create a __DRIdrawable for the None GLX drawable and pass that
    to the DRI driver and that would somehow work.  Now it's somehow broken.
    
    The way this should have worked is that we pass a NULL DRI drawable
    to the DRI driver when the GLX user calls glXMakeContextCurrent()
    with None for drawable and readables.
    
    https://bugs.freedesktop.org/show_bug.cgi?id=74143
    Signed-off-by: Kristian Høgsberg <krh@bitplanet.net>

commit 44338cd826623ae0675861015a56c528261f3fd3
Author: Kristian Høgsberg <krh@bitplanet.net>
Date:   Wed Feb 5 10:59:02 2014 -0800

    i965: Move intel_prepare_render() above first buffer access
    
    The driver is supposed to ensure buffers before any drawing operation, but in
    do_blit_drawpixels() and do_blit_copypixels() we inspect the buffer format
    before calling intel_prepare_render().  That was covered up by the
    unconditional call to intel_prepare_render() in intelMakeCurrent(), but we
    now only do this on the initial intelMakeCurrent call for a context
    (to get the size for the initial viewport values).
    
    https://bugs.freedesktop.org/show_bug.cgi?id=74083
    
    Signed-off-by: Kristian Høgsberg <krh@bitplanet.net>
    Tested-by: Alexander Monakov <amonakov@gmail.com>
Comment 2 lu hua 2014-02-14 06:05:00 UTC
It still fails on Mesa master branch(commit 18caef953f2de134077bfa6e46f8616f68ff1b1b)
Comment 3 lu hua 2014-02-17 05:21:48 UTC
It also fails on Mesa 10.1 branch.
Comment 4 Ian Romanick 2014-02-26 22:59:28 UTC
I am unable to reproduce this failure using either master (with 18caef9) or with 10.1 on either the Fedora 18 or Fedora 20 xserver.

Are you able to reproduce this bug with a window manager running?
Comment 5 lu hua 2014-03-03 03:25:05 UTC
(In reply to comment #4)
> I am unable to reproduce this failure using either master (with 18caef9) or
> with 10.1 on either the Fedora 18 or Fedora 20 xserver.
> 
> Are you able to reproduce this bug with a window manager running?

It still fails on Mesa master branch(fc25956badb8e19) and 10.1 branch.
We run it on Fedora 19.
Comment 6 Iago Toral 2014-03-12 10:26:02 UTC
Created attachment 95650 [details] [review]
Patch

The problem seems to live in the fact that we check whether we have a viewport set to decide if we need to generate buffers for the drawable. This is not a valid solution for the scenario in which we switch drawables for the same context, since the viewport will be initialized the first time that we call MakeCurrent with one of the drawables. Thus, switching to a different drawable after the first MakeCurrent will not produce buffers for the new drawable, leading to the problem.

The patch reverts the behavior to the original solution with a small change to support single buffer drawables (which was the reason the bad commit was introduced). Basically, it checks if we have a render buffer for the drawable, but instead of checking the BACK_LEFT buffer which only works for double buffer drawables  we check the FRONT_LEFT buffer, which should work for both single  buffer and double buffer drawables.
Comment 7 lu hua 2014-03-13 07:19:43 UTC
(In reply to comment #6)
> Created attachment 95650 [details] [review] [review]
> Patch
> 

Fixed by this patch.
Comment 8 Kristian Høgsberg 2014-04-08 16:59:45 UTC
(In reply to comment #6)
> Created attachment 95650 [details] [review] [review]
> Patch
> 
> The problem seems to live in the fact that we check whether we have a
> viewport set to decide if we need to generate buffers for the drawable. This
> is not a valid solution for the scenario in which we switch drawables for
> the same context, since the viewport will be initialized the first time that
> we call MakeCurrent with one of the drawables. Thus, switching to a
> different drawable after the first MakeCurrent will not produce buffers for
> the new drawable, leading to the problem.
> 
> The patch reverts the behavior to the original solution with a small change
> to support single buffer drawables (which was the reason the bad commit was
> introduced). Basically, it checks if we have a render buffer for the
> drawable, but instead of checking the BACK_LEFT buffer which only works for
> double buffer drawables  we check the FRONT_LEFT buffer, which should work
> for both single  buffer and double buffer drawables.

We don't want to revert the behaviour.  The initial patch removed a call to intel_prepare_render() in intelMakeCurrent().  We're supposed to call intel_prepare_render() any time we're about to touch the buffers, but the up-front call to intel_prepare_render() in intelMakeCurrent covered up a few places where we forgot.  The fix now isn't to put back the up-front intel_prepare_render() call but to add it in the rendering paths that are missing it.

Also, for reference, we need the buffer size for the initial value of the context viewport.  So the first time a context is made current, we call intel_prepare_render() to get the buffers so we can see what size they are.  When the same context is later made current with a different drawable, we have a value for the viewport and we're not supposed to change it, so there's no point in getting buffers.
Comment 9 Kristian Høgsberg 2014-04-12 01:08:03 UTC
*** Bug 75797 has been marked as a duplicate of this bug. ***
Comment 10 Giulio Camuffo 2014-04-14 11:25:43 UTC
*** Bug 77083 has been marked as a duplicate of this bug. ***
Comment 11 Kristian Høgsberg 2014-04-22 06:20:47 UTC
*** Bug 76761 has been marked as a duplicate of this bug. ***
Comment 12 Kristian Høgsberg 2014-04-29 22:04:59 UTC
*** Bug 74689 has been marked as a duplicate of this bug. ***
Comment 13 nucrap 2014-06-15 23:52:24 UTC
Any news? It still happens and renders the Hawaii desktop environment unusable...
Comment 14 lu hua 2014-06-16 02:43:26 UTC
Test on (master)32c55448602f8ed764005e72682f5f3979763321, It still fails.
output:
Probe color at (1,1)
  Expected: 0.000000 1.000000 0.000000
  Observed: -9767039900831907594645915144552448.000000 0.000000 0.000000
PIGLIT: {'result': 'fail' }
Comment 15 Kristian Høgsberg 2014-06-18 17:19:59 UTC
*** Bug 79448 has been marked as a duplicate of this bug. ***
Comment 16 k3mist 2014-07-17 05:15:17 UTC
This bug is a pretty much a game-stopper for anyone on integrated Intel cards that would like to upgrade to Plasma 5 (myself included). 

Has any progress been made on this issue or is the latest comment from lu hua in June still the current state?
Comment 17 k3mist 2014-07-17 08:49:25 UTC
Please disregard my previous comment. I have received confirmation from else where the issue is resolved in Git.
Comment 18 Tapani Pälli 2014-07-24 08:13:35 UTC
this was fixed by commit 7928b94
Comment 19 lu hua 2014-07-28 02:54:53 UTC
Verified.Fixed.