[PATCH 1/3] Float fbconfigs exchange patch [1/3] Improved handling of renderType attribute in GLX structures.
Ian Romanick
idr at freedesktop.org
Thu Oct 3 09:22:12 PDT 2013
On 07/15/2013 08:06 AM, Tomasz Lis wrote:
> From: Tomasz Lis <tomasz.lis at intel.com>
>
> Support of GLX_RGBA*_FLOAT_BIT*, and correct setting of the
> flags. Also commented each renderType use with information
> which (fbconfig or context) RENDER_TYPE it is.
I know almost nothing about the DMX or xquartz code, so I don't feel
comfortable offering definitive review on those bits.
> Signed-off-by: Tomasz Lis <listom at gmail.com>
> ---
> glx/createcontext.c | 2 ++
> glx/glxext.h | 15 ++++++++++++++
> hw/dmx/dmx_glxvisuals.c | 7 +++++--
> hw/dmx/glxProxy/glxcmds.c | 42 +++++++++++++++++++++++++++++++++-------
> hw/xquartz/GL/glcontextmodes.c | 10 ++++++++--
> 5 files changed, 65 insertions(+), 11 deletions(-)
>
> diff --git a/glx/createcontext.c b/glx/createcontext.c
> index 13d21cc..41ecd11 100644
> --- a/glx/createcontext.c
> +++ b/glx/createcontext.c
> @@ -68,6 +68,8 @@ validate_render_type(uint32_t render_type)
> switch (render_type) {
> case GLX_RGBA_TYPE:
> case GLX_COLOR_INDEX_TYPE:
> + case GLX_RGBA_FLOAT_TYPE_ARB:
> + case GLX_RGBA_UNSIGNED_FLOAT_TYPE_EXT:
> return True;
> default:
> return False;
> diff --git a/glx/glxext.h b/glx/glxext.h
> index 9b0978b..2d67af3 100644
> --- a/glx/glxext.h
> +++ b/glx/glxext.h
> @@ -35,6 +35,21 @@
> * Silicon Graphics, Inc.
> */
>
> +// doing #include <GL/glx.h> & #include <GL/glxext.h> could cause problems with
> +// overlapping definitions, so let's use the easy way
> +#ifndef GLX_RGBA_FLOAT_BIT_ARB
> +#define GLX_RGBA_FLOAT_BIT_ARB 0x00000004
> +#endif
> +#ifndef GLX_RGBA_FLOAT_TYPE_ARB
> +#define GLX_RGBA_FLOAT_TYPE_ARB 0x20B9
> +#endif
> +#ifndef GLX_RGBA_UNSIGNED_FLOAT_BIT_EXT
> +#define GLX_RGBA_UNSIGNED_FLOAT_BIT_EXT 0x00000008
> +#endif
> +#ifndef GLX_RGBA_UNSIGNED_FLOAT_TYPE_EXT
> +#define GLX_RGBA_UNSIGNED_FLOAT_TYPE_EXT 0x20B1
> +#endif
> +
> extern GLboolean __glXFreeContext(__GLXcontext * glxc);
> extern void __glXFlushContextCache(void);
>
> diff --git a/hw/dmx/dmx_glxvisuals.c b/hw/dmx/dmx_glxvisuals.c
> index f903b74..ba051a4 100644
> --- a/hw/dmx/dmx_glxvisuals.c
> +++ b/hw/dmx/dmx_glxvisuals.c
> @@ -439,8 +439,11 @@ GetGLXFBConfigs(Display * dpy, int glxMajorOpcode, int *nconfigs)
> /* Fill in derived values */
> config->screen = screen;
>
> - config->rgbMode = config->renderType & GLX_RGBA_BIT;
> - config->colorIndexMode = !config->rgbMode;
> + /* The rgbMode should be true for any mode which has distinguishible R, G and B components */
> + config->rgbMode = (config->renderType & (GLX_RGBA_BIT |
> + GLX_RGBA_FLOAT_BIT_ARB | GLX_RGBA_UNSIGNED_FLOAT_BIT_EXT)) != 0;
Blank line here.
> + /* The colorIndexMode isn't really needed, as all non-RGB modes are CI modes */
> + config->colorIndexMode = (config->renderType & GLX_COLOR_INDEX_BIT) != 0;
If this comment is correct, why change the code? In a hunk below you
also use colorIndexMode = !rgbMode, so that seems fine.
>
> config->haveAccumBuffer =
> config->accumRedBits > 0 ||
> diff --git a/hw/dmx/glxProxy/glxcmds.c b/hw/dmx/glxProxy/glxcmds.c
> index 4538274..b325f4d 100644
> --- a/hw/dmx/glxProxy/glxcmds.c
> +++ b/hw/dmx/glxProxy/glxcmds.c
> @@ -308,12 +308,12 @@ CreateContext(__GLXclientState * cl,
> /* send the create context request to the back-end server */
> dpy = GetBackEndDisplay(cl, screen);
> if (glxc->pFBConfig) {
> - /*Since for a certain visual both RGB and COLOR INDEX
> - *can be on then the only parmeter to choose the renderType
> - * should be the class of the colormap since all 4 first
> - * classes does not support RGB mode only COLOR INDEX ,
> - * and so TrueColor and DirectColor does not support COLOR INDEX*/
> - int renderType = glxc->pFBConfig->renderType;
> + /* For a specific visual, multiple render types (ie. both RGB and COLOR INDEX)
> + * can be accessible. The only parameter to choose the renderType
> + * should be the class of the colormap, since all 4 first classes
> + * does not support RGB mode only COLOR INDEX,
> + * and so TrueColor and DirectColor does not support COLOR INDEX. */
This comment is formatted weird (the old one was too). All of the *
should be in the same column, and the */ should be on its own line.
> + int renderType = GLX_RGBA_TYPE;
>
> if (pVisual) {
> switch (pVisual->class) {
> @@ -329,7 +329,21 @@ CreateContext(__GLXclientState * cl,
> renderType = GLX_RGBA_TYPE;
> break;
> }
> + } else {
Hmm... if the fbconfig has multiple bits set, does the spec give any
guidance as to which mode should actually be used? There's an implicit
prioritization here, but is that correct?
> + /* Convert the render type bits from fbconfig into context render type. */
> + if (glxc->pFBConfig->renderType & GLX_RGBA_BIT)
> + renderType = GLX_RGBA_TYPE;
> + else if (glxc->pFBConfig->renderType & GLX_COLOR_INDEX_BIT)
> + renderType = GLX_COLOR_INDEX_TYPE;
> + else if (glxc->pFBConfig->renderType & GLX_RGBA_FLOAT_BIT_ARB)
> + renderType = GLX_RGBA_FLOAT_TYPE_ARB;
> + else if (glxc->pFBConfig->renderType & GLX_RGBA_UNSIGNED_FLOAT_BIT_EXT)
> + renderType = GLX_RGBA_UNSIGNED_FLOAT_TYPE_EXT;
> + else { /* There's no recognized renderType in the config */
> + renderType = GLX_RGBA_TYPE;
> + }
> }
> +
> if (__GLX_IS_VERSION_SUPPORTED(1, 3)) {
> LockDisplay(dpy);
> GetReq(GLXCreateNewContext, be_new_req);
> @@ -3193,6 +3207,7 @@ __glXQueryContext(__GLXclientState * cl, GLbyte * pc)
> __GLXcontext *ctx;
> xGLXQueryContextReq *req;
> xGLXQueryContextReply reply;
> + int renderType;
> int nProps;
> int *sendBuf, *pSendBuf;
> int nReplyBytes;
> @@ -3205,6 +3220,19 @@ __glXQueryContext(__GLXclientState * cl, GLbyte * pc)
> return __glXBadContext;
> }
>
> + /* Convert the render type bits from fbconfig into context render type. */
This code appears twice, so it should be refactored to its own function.
> + if (ctx->pFBConfig->renderType & GLX_RGBA_BIT)
> + renderType = GLX_RGBA_TYPE;
> + else if (ctx->pFBConfig->renderType & GLX_COLOR_INDEX_BIT)
> + renderType = GLX_COLOR_INDEX_TYPE;
> + else if (ctx->pFBConfig->renderType & GLX_RGBA_FLOAT_BIT_ARB)
> + renderType = GLX_RGBA_FLOAT_TYPE_ARB;
> + else if (ctx->pFBConfig->renderType & GLX_RGBA_UNSIGNED_FLOAT_BIT_EXT)
> + renderType = GLX_RGBA_UNSIGNED_FLOAT_TYPE_EXT;
> + else { /* There's no recognized renderType in the config */
> + renderType = GLX_RGBA_TYPE;
> + }
> +
> nProps = 3;
>
> reply = (xGLXQueryContextReply) {
> @@ -3220,7 +3248,7 @@ __glXQueryContext(__GLXclientState * cl, GLbyte * pc)
> *pSendBuf++ = GLX_FBCONFIG_ID;
> *pSendBuf++ = (int) (ctx->pFBConfig->id);
> *pSendBuf++ = GLX_RENDER_TYPE;
> - *pSendBuf++ = (int) (ctx->pFBConfig->renderType);
> + *pSendBuf++ = (int)(renderType); /* context render type (one of GLX_*_TYPE values) */
You can drop the (int)() casting since renderType is already int.
> *pSendBuf++ = GLX_SCREEN;
> *pSendBuf++ = (int) (ctx->pScreen->myNum);
>
> diff --git a/hw/xquartz/GL/glcontextmodes.c b/hw/xquartz/GL/glcontextmodes.c
> index dc97f89..1fffaa8 100644
> --- a/hw/xquartz/GL/glcontextmodes.c
> +++ b/hw/xquartz/GL/glcontextmodes.c
> @@ -141,9 +141,15 @@ _gl_copy_visual_to_context_mode(__GLcontextModes * mode,
> mode->drawableType = GLX_WINDOW_BIT | GLX_PIXMAP_BIT;
>
> mode->rgbMode = (config->rgba != 0);
> - mode->renderType = (mode->rgbMode) ? GLX_RGBA_BIT : GLX_COLOR_INDEX_BIT;
> -
> mode->colorIndexMode = !(mode->rgbMode);
Blank line here.
> + /* It's impossible to create float config from visual. */
> + mode->floatMode = GL_FALSE;
Blank line here.
> + /* The fact that we can't have float mode makes the code below unequivocal.
> + * The rgbMode is true for both float or integer modes, but this time we're sure it's integer.
> + * We could even skip checking colorIndexMode as all non-RGB modes are CI. */
> + mode->renderType = (mode->rgbMode) ? GLX_RGBA_BIT : 0 |
> + (mode->colorIndexMode) ? GLX_COLOR_INDEX_BIT : 0;
> +
I'm confused. A few lines earlier you set mode->colorIndexMode to
!mode->rgbMode. If that's the case, why change this line? I can
understand grouping all of the mode->*Mode lines together, but this last
change seems spurious.
> mode->doubleBufferMode = (config->doubleBuffer != 0);
> mode->stereoMode = (config->stereo != 0);
>
>
More information about the xorg-devel
mailing list