[Mesa-dev] [PATCH 4/7] mesa: remove check_compatible() in make_current
Ian Romanick
idr at freedesktop.org
Tue Feb 9 04:21:19 UTC 2016
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.
> -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