[Mesa-dev] [PATCH 4/7] mesa: remove check_compatible() in make_current
Miklós Máté
mtmkls at gmail.com
Thu Feb 11 20:57:08 UTC 2016
On 02/09/2016 05:21 AM, Ian Romanick wrote:
> On 02/05/2016 01:11 PM, Miklós Máté wrote:
>> this was marked for removal since 2007
>> ctx::Visual is also removed, since this was its only legit user
>> ---
>> .../drivers/dri/radeon/radeon_common_context.c | 2 +-
>> src/mesa/main/blend.c | 4 +-
>> src/mesa/main/blend.h | 4 +-
>> src/mesa/main/context.c | 89 ++--------------------
>> src/mesa/main/mtypes.h | 7 --
>> src/mesa/main/pixel.c | 4 +-
>> src/mesa/main/pixel.h | 4 +-
>> 7 files changed, 15 insertions(+), 99 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/radeon/radeon_common_context.c b/src/mesa/drivers/dri/radeon/radeon_common_context.c
>> index 4660d98..2989f63 100644
>> --- a/src/mesa/drivers/dri/radeon/radeon_common_context.c
>> +++ b/src/mesa/drivers/dri/radeon/radeon_common_context.c
>> @@ -604,7 +604,7 @@ GLboolean radeonMakeCurrent(__DRIcontext * driContextPriv,
>> }
>>
>> if(driDrawPriv == NULL && driReadPriv == NULL) {
>> - drfb = _mesa_create_framebuffer(&radeon->glCtx.Visual);
>> + drfb = _mesa_get_incomplete_framebuffer();
>> readfb = drfb;
>> }
>> else {
>> diff --git a/src/mesa/main/blend.c b/src/mesa/main/blend.c
>> index 2ae22e9..28e2dbf 100644
>> --- a/src/mesa/main/blend.c
>> +++ b/src/mesa/main/blend.c
>> @@ -921,7 +921,7 @@ _mesa_get_render_format(const struct gl_context *ctx, mesa_format format)
>> * Initializes the related fields in the context color attribute group,
>> * __struct gl_contextRec::Color.
>> */
>> -void _mesa_init_color( struct gl_context * ctx )
>> +void _mesa_init_color( struct gl_context * ctx, GLuint doubleBufferMode )
> Mesa has changed it's style (years ago, at this point) to not have the
> spaces after the ( or before the ). Since the prototype is being
> updated anyway, this is a good time to fix the style.
>
> I think I'd also prefer doubleBufferMode to be bool.
>
> I have a bunch of comments below about the other changes, but I think
> explicitly passing the double buffer mode around is a reasonable change.
>> {
>> GLuint i;
>>
>> @@ -951,7 +951,7 @@ void _mesa_init_color( struct gl_context * ctx )
>>
>> /* GL_FRONT is not possible on GLES. Instead GL_BACK will render to either
>> * the front or the back buffer depending on the config */
>> - if (ctx->Visual.doubleBufferMode || _mesa_is_gles(ctx)) {
>> + if (doubleBufferMode || _mesa_is_gles(ctx)) {
>> ctx->Color.DrawBuffer[0] = GL_BACK;
>> }
>> else {
>> diff --git a/src/mesa/main/blend.h b/src/mesa/main/blend.h
>> index 8ab9e02..f4854a6 100644
>> --- a/src/mesa/main/blend.h
>> +++ b/src/mesa/main/blend.h
>> @@ -124,7 +124,7 @@ _mesa_update_clamp_vertex_color(struct gl_context *ctx,
>> extern mesa_format
>> _mesa_get_render_format(const struct gl_context *ctx, mesa_format format);
>>
>> -extern void
>> -_mesa_init_color( struct gl_context * ctx );
>> +extern void
>> +_mesa_init_color( struct gl_context * ctx, GLuint doubleBufferMode );
> Same comment here about fixing the spacing while changing the prototype.
>
>>
>> #endif
>> diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
>> index 8b415ed..2a512c6 100644
>> --- a/src/mesa/main/context.c
>> +++ b/src/mesa/main/context.c
>> @@ -796,7 +796,7 @@ check_context_limits(struct gl_context *ctx)
>> * functions for the more complex data structures.
>> */
>> static GLboolean
>> -init_attrib_groups(struct gl_context *ctx)
>> +init_attrib_groups(struct gl_context *ctx, GLuint doubleBufferMode)
> Same comment here about s/GLuint/bool/
>
>> {
>> assert(ctx);
>>
>> @@ -810,7 +810,7 @@ init_attrib_groups(struct gl_context *ctx)
>> _mesa_init_accum( ctx );
>> _mesa_init_attrib( ctx );
>> _mesa_init_buffer_objects( ctx );
>> - _mesa_init_color( ctx );
>> + _mesa_init_color( ctx, doubleBufferMode );
>> _mesa_init_current( ctx );
>> _mesa_init_depth( ctx );
>> _mesa_init_debug( ctx );
>> @@ -828,7 +828,7 @@ init_attrib_groups(struct gl_context *ctx)
>> _mesa_init_multisample( ctx );
>> _mesa_init_performance_monitors( ctx );
>> _mesa_init_pipeline( ctx );
>> - _mesa_init_pixel( ctx );
>> + _mesa_init_pixel( ctx, doubleBufferMode );
> Spaces.
>
>> _mesa_init_pixelstore( ctx );
>> _mesa_init_point( ctx );
>> _mesa_init_polygon( ctx );
>> @@ -1159,15 +1159,6 @@ _mesa_initialize_context(struct gl_context *ctx,
>> ctx->WinSysDrawBuffer = NULL;
>> ctx->WinSysReadBuffer = NULL;
>>
>> - if (visual) {
>> - ctx->Visual = *visual;
>> - ctx->HasConfig = GL_TRUE;
>> - }
>> - else {
>> - memset(&ctx->Visual, 0, sizeof ctx->Visual);
>> - ctx->HasConfig = GL_FALSE;
>> - }
>> -
>> _mesa_override_gl_version(ctx);
>>
>> /* misc one-time initializations */
>> @@ -1193,7 +1184,7 @@ _mesa_initialize_context(struct gl_context *ctx,
>>
>> _mesa_reference_shared_state(ctx, &ctx->Shared, shared);
>>
>> - if (!init_attrib_groups( ctx ))
>> + if (!init_attrib_groups( ctx, visual->doubleBufferMode ))
> Spaces.
>
>> goto fail;
>>
>> /* setup the API dispatch tables with all nop functions */
>> @@ -1521,57 +1512,6 @@ _mesa_copy_context( const struct gl_context *src, struct gl_context *dst,
>>
>>
>> /**
>> - * Check if the given context can render into the given framebuffer
>> - * by checking visual attributes.
>> - *
>> - * Most of these tests could go away because Mesa is now pretty flexible
>> - * in terms of mixing rendering contexts with framebuffers. As long
>> - * as RGB vs. CI mode agree, we're probably good.
>> - *
>> - * \return GL_TRUE if compatible, GL_FALSE otherwise.
>> - */
> This is an area where we have woefully few test cases. The existing
> code is wrong, but removing all of it is, I believe, wrong too. With
> GLX 1.2 and earlier, the visual and the drawable had to match exactly.
> Starting with GLX 1.3, the restrictions were relaxed somewhat. Section
> 2.1 (Rendering Contexts and Drawing Surfaces) of the GLX 1.4 spec says:
>
> A drawable and context are compatible if they
>
> • support the same type of rendering (e.g., RGBA or color index)
> • have color buffers and ancillary buffers of the same depth. For
> example, a GLXDrawable that has a front left buffer and a back
> left buffer with red, green and blue sizes of 4 would not be
> compatible with a context that was created with a visual or
> GLXFBConfig that has only a front left buffer with red, green and
> blue sizes of 8. However, it would be compatible with a context
> that was created with a GLXFBConfig that has only a front left
> buffer if the red, green and blue sizes are 4.
> • were created with respect to the same X screen"
>
> What check_compatible currently enforces is stronger than the second
> bullet. It requires that if the context visual has a buffer, then the
> drawable must have a buffer that matches. What it should enforces is
> that if the context and drawable have a buffer, those buffers must
> match. I think fixing that would fix your test case in bug #93955.
>
> I have some concrete suggestions below.
You are right, it's incorrect to completely remove the compatibility
check, and ctx->Visual
has to stay, too. I'll shortly post a new patch that only reworks the
compatibility criteria.
MM
>
>> -static GLboolean
>> -check_compatible(const struct gl_context *ctx,
>> - const struct gl_framebuffer *buffer)
>> -{
>> - const struct gl_config *ctxvis = &ctx->Visual;
>> - const struct gl_config *bufvis = &buffer->Visual;
>> -
>> - if (buffer == _mesa_get_incomplete_framebuffer())
>> - return GL_TRUE;
>> -
>> -#if 0
>> - /* disabling this fixes the fgl_glxgears pbuffer demo */
>> - if (ctxvis->doubleBufferMode && !bufvis->doubleBufferMode)
>> - return GL_FALSE;
>> -#endif
>> - if (ctxvis->stereoMode && !bufvis->stereoMode)
>> - return GL_FALSE;
>> - if (ctxvis->haveAccumBuffer && !bufvis->haveAccumBuffer)
>> - return GL_FALSE;
>> - if (ctxvis->haveDepthBuffer && !bufvis->haveDepthBuffer)
>> - return GL_FALSE;
>> - if (ctxvis->haveStencilBuffer && !bufvis->haveStencilBuffer)
>> - return GL_FALSE;
> Just delete the preceding 5 checks.
>
>> - if (ctxvis->redMask && ctxvis->redMask != bufvis->redMask)
>> - return GL_FALSE;
>> - if (ctxvis->greenMask && ctxvis->greenMask != bufvis->greenMask)
>> - return GL_FALSE;
>> - if (ctxvis->blueMask && ctxvis->blueMask != bufvis->blueMask)
>> - return GL_FALSE;
>> -#if 0
>> - /* disabled (see bug 11161) */
>> - if (ctxvis->depthBits && ctxvis->depthBits != bufvis->depthBits)
>> - return GL_FALSE;
>> -#endif
>> - if (ctxvis->stencilBits && ctxvis->stencilBits != bufvis->stencilBits)
>> - return GL_FALSE;
> Change all 5 of these to be like:
>
> if (ctxvis->HaveFoo && bufvis->HaveFoo &&
> ctxvis->fooBits != bufvis->fooBits)
> return GL_FALSE;
>
> Also remove the '#if 0' around the depthBits check.
>
>> -
>> - return GL_TRUE;
>> -}
>> -
>> -
>> -/**
>> * Check if the viewport/scissor size has not yet been initialized.
>> * Initialize the size if the given width and height are non-zero.
>> */
>> @@ -1614,7 +1554,7 @@ handle_first_current(struct gl_context *ctx)
>> /* According to GL_MESA_configless_context the default value of
>> * glDrawBuffers depends on the config of the first surface it is bound to.
>> * For GLES it is always GL_BACK which has a magic interpretation */
>> - if (!ctx->HasConfig && _mesa_is_desktop_gl(ctx)) {
>> + if (_mesa_is_desktop_gl(ctx)) {
>> if (ctx->DrawBuffer != _mesa_get_incomplete_framebuffer()) {
>> if (ctx->DrawBuffer->Visual.doubleBufferMode)
>> buffer = GL_BACK;
> I don't this this is right because it definitely changes behavior... and
> I wish we had a test case. :(
>
> In all versions of OpenGL ES, the draw buffer can only be GL_BACK. If
> you somehow get a single-buffered drawable, magic happens in the driver.
> The value reported by glGetIntegerv is still GL_BACK.
>
> In desktop OpenGL, the draw buffer setting is "sticky." There are a few
> scenarios to care about:
>
> 1. Create a configless context.
>
> Bind a single buffer drawable (draw buffer becomes GL_FRONT).
>
> Bind a different single buffer drawable (draw buffer remains
> GL_FRONT).
>
> 2. Create a configless context.
>
> Bind a double buffer drawable (draw buffer becomes GL_BACK).
>
> Bind a different double buffer drawable (draw buffer remains
> GL_BACK).
>
> 3. Create a configless context.
>
> Bind a single buffer drawable (draw buffer becomes GL_FRONT).
>
> Bind a double buffer drawable (draw buffer remains GL_FRONT). This
> patch changes this behavior.
>
> 4. Create a configless context.
>
> Bind a double buffer drawable (draw buffer becomes GL_BACK).
>
> Bind a double buffer drawable (draw buffer remains GL_BACK). This
> patch changes this behavior.
>
> 5. Create a configless context.
>
> Bind a double buffer drawable (draw buffer becomes GL_BACK).
>
> Call glDrawBuffer(GL_FRONT).
>
> Bind a double buffer drawable (draw buffer remains GL_FRONT). This
> patch changes this behavior.
>
> There is a similar set of scenarios involving contexts create with
> single- and double-buffer configs. A bunch of those change behavior
> around this change as well.
>
>> @@ -1673,24 +1613,7 @@ _mesa_make_current( struct gl_context *newCtx,
>> if (MESA_VERBOSE & VERBOSE_API)
>> _mesa_debug(newCtx, "_mesa_make_current()\n");
>>
>> - /* Check that the context's and framebuffer's visuals are compatible.
>> - */
>> - if (newCtx && drawBuffer && newCtx->WinSysDrawBuffer != drawBuffer) {
>> - if (!check_compatible(newCtx, drawBuffer)) {
>> - _mesa_warning(newCtx,
>> - "MakeCurrent: incompatible visuals for context and drawbuffer");
>> - return GL_FALSE;
>> - }
>> - }
>> - if (newCtx && readBuffer && newCtx->WinSysReadBuffer != readBuffer) {
>> - if (!check_compatible(newCtx, readBuffer)) {
>> - _mesa_warning(newCtx,
>> - "MakeCurrent: incompatible visuals for context and readbuffer");
>> - return GL_FALSE;
>> - }
>> - }
>> -
>> - if (curCtx &&
>> + if (curCtx &&
>> (curCtx->WinSysDrawBuffer || curCtx->WinSysReadBuffer) &&
>> /* make sure this context is valid for flushing */
>> curCtx != newCtx &&
>> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
>> index 4d29d6a..5c4ce89 100644
>> --- a/src/mesa/main/mtypes.h
>> +++ b/src/mesa/main/mtypes.h
>> @@ -4309,7 +4309,6 @@ struct gl_context
>> struct _glapi_table *CurrentDispatch;
>> /*@}*/
>>
>> - struct gl_config Visual;
>> struct gl_framebuffer *DrawBuffer; /**< buffer for writing */
>> struct gl_framebuffer *ReadBuffer; /**< buffer for reading */
>> struct gl_framebuffer *WinSysDrawBuffer; /**< set with MakeCurrent */
>> @@ -4548,12 +4547,6 @@ struct gl_context
>> GLboolean FirstTimeCurrent;
>> /*@}*/
>>
>> - /**
>> - * False if this context was created without a config. This is needed
>> - * because the initial state of glDrawBuffers depends on this
>> - */
>> - GLboolean HasConfig;
>> -
>> /** software compression/decompression supported or not */
>> GLboolean Mesa_DXTn;
>>
>> diff --git a/src/mesa/main/pixel.c b/src/mesa/main/pixel.c
>> index 608a545..ffdb31b 100644
>> --- a/src/mesa/main/pixel.c
>> +++ b/src/mesa/main/pixel.c
>> @@ -649,7 +649,7 @@ init_pixelmap(struct gl_pixelmap *map)
>> * Initialize the context's PIXEL attribute group.
>> */
>> void
>> -_mesa_init_pixel( struct gl_context *ctx )
>> +_mesa_init_pixel( struct gl_context *ctx, GLuint doubleBufferMode )
> s/GLuint/bool/ and spaces.
>
>> {
>> /* Pixel group */
>> ctx->Pixel.RedBias = 0.0;
>> @@ -679,7 +679,7 @@ _mesa_init_pixel( struct gl_context *ctx )
>> init_pixelmap(&ctx->PixelMaps.BtoB);
>> init_pixelmap(&ctx->PixelMaps.AtoA);
>>
>> - if (ctx->Visual.doubleBufferMode) {
>> + if (doubleBufferMode) {
>> ctx->Pixel.ReadBuffer = GL_BACK;
>> }
>> else {
>> diff --git a/src/mesa/main/pixel.h b/src/mesa/main/pixel.h
>> index fd1782e..51e4b51 100644
>> --- a/src/mesa/main/pixel.h
>> +++ b/src/mesa/main/pixel.h
>> @@ -66,8 +66,8 @@ _mesa_PixelTransferi( GLenum pname, GLint param );
>> extern void
>> _mesa_update_pixel( struct gl_context *ctx, GLuint newstate );
>>
>> -extern void
>> -_mesa_init_pixel( struct gl_context * ctx );
>> +extern void
>> +_mesa_init_pixel( struct gl_context * ctx, GLuint doubleBufferMode );
> s/GLuint/bool/ and spaces.
>
>>
>> /*@}*/
>>
>>
More information about the mesa-dev
mailing list