Bug 61012

Summary: alloc_layout_array tx * ty assertion failure when making pbuffer current
Product: Mesa Reporter: Brian Crowell <freedesktop>
Component: GLXAssignee: mesa-dev
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: medium    
Version: 9.0   
Hardware: x86 (IA32)   
OS: Linux (All)   
Whiteboard:
i915 platform: i915 features:
Attachments: Test case
llvmpipe texture size patch
Initialize the buffer's size in create_xmesa_buffer()
another attempt to fix pbuffer initialization

Description Brian Crowell 2013-02-17 17:44:18 UTC
The following(ish) code produces an assertion failure using llvmpipe libGL from Mesa 9.0.2:

    GLXContext new_context = glXCreateNewContext(
        __display, configs[0], GLX_RGBA_TYPE, NULL, True );

    if( !new_context )
        g_error( "Failed to create context." );

    int pbuf_attrs[] = { GLX_PBUFFER_WIDTH, 8, GLX_PBUFFER_HEIGHT, 8, GLX_PRESERVED_CONTENTS, False, None };

    GLXPbuffer pbuf = glXCreatePbuffer( __display, configs[0], pbuf_attrs );

    if( pbuf == None )
        g_error( "Failed to create XGL pixel buffer." );

    if( !glXMakeContextCurrent( __display,
        pbuf,
        pbuf,
        new_context ) ) {
        g_error( "Failed to set context current." );
    }

src/gallium/drivers/llvmpipe/lp_texture.c:93:alloc_layout_array: Assertion `num_slices * tx * ty > 0' failed.

In alloc_layout_array, tx and ty are zero. These values seem to come from state stored with the pbuffer; Mesa is trying to realize the drawable during the glXMakeContextCurrent call.

Near as I can tell, the call responsible for storing that state with the pbuffer is this code:

  http://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/state_trackers/glx/xlib/xm_api.c?id=369e46888904c6d379b8b477d9242cff1608e30e#n460

A call to get_drawable_size retrieves the size of the pbuffer drawable into local variables width and height, but these values never make it into the XMesaBuffer structure.




Backtrace:
#0  0xb696717a in _debug_assert_fail (expr=0xb725b888 "num_slices * tx * ty > 0", file=
    0xb725b85c "src/gallium/drivers/llvmpipe/lp_texture.c", line=93, function=0xb725c140 "alloc_layout_array")
    at src/gallium/auxiliary/util/u_debug.c:278
#1  0xb6641058 in alloc_layout_array (num_slices=1, width=0, height=0) at src/gallium/drivers/llvmpipe/lp_texture.c:93
#2  0xb66413f4 in llvmpipe_displaytarget_layout (screen=0x8714b98, lpr=0x8b43460) at src/gallium/drivers/llvmpipe/lp_texture.c:216
#3  0xb66415c5 in llvmpipe_resource_create (_screen=0x8714b98, templat=0xbfffdf44) at src/gallium/drivers/llvmpipe/lp_texture.c:263
#4  0xb66c29db in xmesa_st_framebuffer_validate_textures (stfbi=0x87040c8, width=0, height=0, mask=1)
    at src/gallium/state_trackers/glx/xlib/xm_st.c:171
#5  0xb66c2b2e in xmesa_st_framebuffer_validate (stfbi=0x87040c8, statts=0x8b433c4, count=1, out=0xbfffe028)
    at src/gallium/state_trackers/glx/xlib/xm_st.c:224
#6  0xb66e122c in st_framebuffer_validate (stfb=0x8b43088, st=0x8b3d7f0) at src/mesa/state_tracker/st_manager.c:195
#7  0xb66e2226 in st_api_make_current (stapi=0xb75d2700, stctxi=0x8b3d7f0, stdrawi=0x87040c8, streadi=0x87040c8)
    at src/mesa/state_tracker/st_manager.c:734
#8  0xb66c1dec in XMesaMakeCurrent2 (c=0x8a60e00, drawBuffer=0x8767aa8, readBuffer=0x8767aa8)
    at src/gallium/state_trackers/glx/xlib/xm_api.c:1189
#9  0xb66c4b36 in glXMakeContextCurrent (dpy=0x87261b8, draw=54525953, read=54525953, ctx=0x8a56290)
    at src/gallium/state_trackers/glx/xlib/glx_api.c:1161
#10 0xb7af9807 in gl_set_current_context (context=0x8b43078) at src/cprocess/gl.c:152
#11 0xb7af9bcf in gl_ensure_context () at src/cprocess/gl.c:185
...and so on...
Comment 1 Brian Crowell 2013-02-17 21:40:06 UTC
Created attachment 75010 [details]
Test case

Hopefully minimal test case. Use the llvmpipe libGL.
Comment 2 Brian Paul 2013-02-18 16:49:01 UTC
Created attachment 75059 [details] [review]
llvmpipe texture size patch

Can you test the attached patch?
Comment 3 Roland Scheidegger 2013-02-18 17:06:19 UTC
(In reply to comment #0)
> The following(ish) code produces an assertion failure using llvmpipe libGL
> from Mesa 9.0.2:
> Near as I can tell, the call responsible for storing that state with the
> pbuffer is this code:
> 
>  
> http://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/state_trackers/glx/
> xlib/xm_api.c?id=369e46888904c6d379b8b477d9242cff1608e30e#n460
> 
> A call to get_drawable_size retrieves the size of the pbuffer drawable into
> local variables width and height, but these values never make it into the
> XMesaBuffer structure.

This isn't llvmpipe specific right?
That call you mention indeed looks a bit odd. The function comment explicitly states Width/Height of the new buffer will be 0 so I don't know why it calls get_drawable_size() there in the first place (probably some leftover from older code).
It looks like for pixmaps/windows that information will be filled out later at MakeCurrent time, which will eventually call xmesa_check_buffer_size() which will fill it in. However, for pbuffers this is a noop. My guess is it should be filled out in XMesaCreatePBuffer() instead since pbuffers have fixed size (?).
Comment 4 Brian Paul 2013-02-18 17:58:52 UTC
Created attachment 75065 [details] [review]
Initialize the buffer's size in create_xmesa_buffer()

Here's another patch to try.  It uses the results of get_drawable_size() to initialize the buffer's dimensions.  I've run other test programs and it seems to be OK.

I think my previous patch for llvmpipe is also needed since it's possible to create a pbuffer of size 0 x 0.
Comment 5 Roland Scheidegger 2013-02-18 18:41:57 UTC
Created attachment 75068 [details] [review]
another attempt to fix pbuffer initialization

Hmm is it legal to use XGetGeometry() with pbuffers?
I think something like this patch would be better.
Not sure if guarding against zero-sized buffers in drivers is needed. Might be but there are other instances where we hack up such windows to have width/height of 1 for that reason so we don't have to do it in drivers.
Comment 6 Brian Paul 2013-02-18 19:12:29 UTC
(In reply to comment #5)
> Created attachment 75068 [details] [review] [review]
> another attempt to fix pbuffer initialization
> 
> Hmm is it legal to use XGetGeometry() with pbuffers?

Not normally, but in the glx/xlib code we create a dummy pixmap for each pbuffer so that we have an XID that we can pass around.


> I think something like this patch would be better.

That would be fine too.  It's what I first tried.

> Not sure if guarding against zero-sized buffers in drivers is needed. Might
> be but there are other instances where we hack up such windows to have
> width/height of 1 for that reason so we don't have to do it in drivers.

I hacked up a test for a 0x0 surface.  Softpipe worked but the llvmpipe assertion failed.  I guess I'd consider the llvmpipe change to be a defensive coding check.  One less way for llvmpipe to fail is good thing.
Comment 7 Roland Scheidegger 2013-02-18 20:02:33 UTC
(In reply to comment #6)
> (In reply to comment #5)
> > Created attachment 75068 [details] [review] [review] [review]
> > another attempt to fix pbuffer initialization
> > 
> > Hmm is it legal to use XGetGeometry() with pbuffers?
> 
> Not normally, but in the glx/xlib code we create a dummy pixmap for each
> pbuffer so that we have an XID that we can pass around.
Ah ok then that should be fine too.
I think though in this case the function comment should be updated too (which is why I was thinking this function shouldn't really set up size for whatever reason).

> 
> 
> > I think something like this patch would be better.
> 
> That would be fine too.  It's what I first tried.
> 
> > Not sure if guarding against zero-sized buffers in drivers is needed. Might
> > be but there are other instances where we hack up such windows to have
> > width/height of 1 for that reason so we don't have to do it in drivers.
> 
> I hacked up a test for a 0x0 surface.  Softpipe worked but the llvmpipe
> assertion failed.  I guess I'd consider the llvmpipe change to be a
> defensive coding check.  One less way for llvmpipe to fail is good thing.
Yeah probably. Though zero-sized resources are a pretty nasty thing.
Comment 8 Brian Crowell 2013-02-18 20:23:55 UTC
On Mon, Feb 18, 2013 at 11:58 AM,  <bugzilla-daemon@freedesktop.org> wrote:
> Here's another patch to try.  It uses the results of get_drawable_size() to
> initialize the buffer's dimensions.  I've run other test programs and it
> seems
> to be OK.

This was essentially the fix I did to get past it on my own. I was
able to make the context current, but I can't verify anything past
that because the pbuffer is essentially a dummy so I can do
direct-to-texture rendering with framebuffers (which is failing for
more LLVM-related reasons). I read somewhere that with OpenGL 3.0
compliance, I can specify None for the drawable in
glXMakeContextCurrent instead of making a dummy pbuffer.

Anyhow, I know this lets me make the context current, and the program
used to work in Mesa 8.0.5, so I assume the fix is enough. Is there
anything specific you'd like me to try?
Comment 9 Brian Paul 2013-02-19 14:30:36 UTC
Fixed by commit e2091f64cb9ea79f3b51c353ed9facc03ec5690a

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.