[PATCH 16/27] glamor: Replace some goofy enum-likes with a real enum.
Eric Anholt
eric at anholt.net
Thu Mar 13 10:23:51 PDT 2014
Markus Wick <markus at selfnet.de> writes:
> Am 2014-03-11 22:30, schrieb Eric Anholt:
>> This unpacks the bitfield into an int size, but my experience has been
>> that packing bitfields doesn't matter for performance.
>>
>> Signed-off-by: Eric Anholt <eric at anholt.net>
>> ---
>> glamor/glamor_fbo.c | 2 +-
>> glamor/glamor_priv.h | 25 ++++++++++++++++---------
>> 2 files changed, 17 insertions(+), 10 deletions(-)
>>
>> diff --git a/glamor/glamor_fbo.c b/glamor/glamor_fbo.c
>> index 281cf83..640b6fd 100644
>> --- a/glamor/glamor_fbo.c
>> +++ b/glamor/glamor_fbo.c
>> @@ -505,7 +505,7 @@ glamor_pixmap_attach_fbo(PixmapPtr pixmap,
>> glamor_pixmap_fbo *fbo)
>> case GLAMOR_TEXTURE_LARGE:
>> case GLAMOR_TEXTURE_ONLY:
>> case GLAMOR_TEXTURE_DRM:
>> - pixmap_priv->base.gl_fbo = 1;
>> + pixmap_priv->base.gl_fbo = GLAMOR_FBO_NORMAL;
>> if (fbo->tex != 0)
>> pixmap_priv->base.gl_tex = 1;
>> else {
>> diff --git a/glamor/glamor_priv.h b/glamor/glamor_priv.h
>> index 4dc2c75..52dad35 100644
>> --- a/glamor/glamor_priv.h
>> +++ b/glamor/glamor_priv.h
>> @@ -291,8 +291,21 @@ typedef enum glamor_access {
>> GLAMOR_ACCESS_WO,
>> } glamor_access_t;
>>
>> -#define GLAMOR_FBO_NORMAL 1
>> -#define GLAMOR_FBO_DOWNLOADED 2
>> +enum glamor_fbo_state {
>> + /** There is no storage attached to the pixmap. */
>> + GLAMOR_FBO_UNATTACHED,
>> + /**
>> + * The pixmap has FBO storage attached, but devPrivate.ptr doesn't
>> + * point at anything.
>> + */
>> + GLAMOR_FBO_NORMAL,
>> + /**
>> + * The FBO is present and can be accessed as a linear memory
>> + * mapping through devPrivate.ptr.
>> + */
>> + GLAMOR_FBO_DOWNLOADED,
>> +};
>> +
>> /* glamor_pixmap_fbo:
>> * @list: to be used to link to the cache pool list.
>> * @expire: when push to cache pool list, set a expire count.
>> @@ -324,12 +337,6 @@ typedef struct glamor_pixmap_fbo {
>>
>> /*
>> * glamor_pixmap_private - glamor pixmap's private structure.
>> - * @gl_fbo:
>> - * 0 - The pixmap doesn't has a fbo attached to it.
>> - * GLAMOR_FBO_NORMAL - The pixmap has a fbo and can be accessed
>> normally.
>> - * GLAMOR_FBO_DOWNLOADED - The pixmap has a fbo and already
>> downloaded to
>> - * CPU, so it can only be treated as a in-memory pixmap
>> - * if this bit is set.
>> * @gl_tex: The pixmap is in a gl texture originally.
>> * @is_picture: The drawable is attached to a picture.
>> * @pict_format: the corresponding picture's format.
>> @@ -403,7 +410,7 @@ typedef struct glamor_pixmap_clipped_regions {
>>
>> typedef struct glamor_pixmap_private_base {
>> glamor_pixmap_type_t type;
>> - unsigned char gl_fbo:2;
>> + enum glamor_fbo_state gl_fbo;
>> unsigned char is_picture:1;
>> unsigned char gl_tex:1;
>> glamor_pixmap_fbo *fbo;
>
> base.gl_fbo seems often to be compared vs 0 instead of
> GLAMOR_FBO_UNATTACHED.
>
> To be honest, I'm a bit confused about this flag. Is this really about
> fbo or more likely about general textures?
Squashed in this fix:
diff --git a/glamor/glamor_core.c b/glamor/glamor_core.c
index 5883809..f6aca15 100644
--- a/glamor/glamor_core.c
+++ b/glamor/glamor_core.c
@@ -42,7 +42,7 @@ glamor_get_drawable_location(const DrawablePtr drawable)
glamor_pixmap_private *pixmap_priv = glamor_get_pixmap_private(pixmap);
glamor_screen_private *glamor_priv =
glamor_get_screen_private(drawable->pScreen);
- if (pixmap_priv == NULL || pixmap_priv->base.gl_fbo == 0)
+ if (pixmap_priv == NULL || pixmap_priv->base.gl_fbo == GL_FBO_UNATTACHED)
return 'm';
if (pixmap_priv->base.fbo->fb == glamor_priv->screen_fbo)
return 's';
diff --git a/glamor/glamor_render.c b/glamor/glamor_render.c
index 086526d..8a98a9f 100644
--- a/glamor/glamor_render.c
+++ b/glamor/glamor_render.c
@@ -961,7 +961,7 @@ glamor_composite_choose_shader(CARD8 op,
* Does it need special handle? */
glamor_fallback("source == dest\n");
}
- if (source_pixmap_priv->base.gl_fbo == 0) {
+ if (source_pixmap_priv->base.gl_fbo == GL_FBO_UNATTACHED) {
/* XXX in Xephyr, we may have gl_fbo equal to 1 but gl_tex
* equal to zero when the pixmap is screen pixmap. Then we may
* refer the tex zero directly latter in the composition.
@@ -982,7 +982,7 @@ glamor_composite_choose_shader(CARD8 op,
glamor_fallback("mask == dest\n");
goto fail;
}
- if (mask_pixmap_priv->base.gl_fbo == 0) {
+ if (mask_pixmap_priv->base.gl_fbo == GL_FBO_UNATTACHED) {
#ifdef GLAMOR_PIXMAP_DYNAMIC_UPLOAD
mask_status = GLAMOR_UPLOAD_PENDING;
#else
and this:
diff --git a/glamor/glamor_copyarea.c b/glamor/glamor_copyarea.c
index d6bcacd..d03f708 100644
--- a/glamor/glamor_copyarea.c
+++ b/glamor/glamor_copyarea.c
@@ -137,7 +137,7 @@ glamor_copy_n_to_n_textured(DrawablePtr src,
src_pixmap_priv = glamor_get_pixmap_private(src_pixmap);
dst_pixmap_priv = glamor_get_pixmap_private(dst_pixmap);
- if (!src_pixmap_priv->base.gl_fbo) {
+ if (src_pixmap_priv->base.gl_fbo == GLAMOR_FBO_UNATTACHED) {
#ifndef GLAMOR_PIXMAP_DYNAMIC_UPLOAD
glamor_delayed_fallback(dst->pScreen, "src has no fbo.\n");
return FALSE;
diff --git a/glamor/glamor_pixmap.c b/glamor/glamor_pixmap.c
index 119e4d9..6ecabb5 100644
--- a/glamor/glamor_pixmap.c
+++ b/glamor/glamor_pixmap.c
@@ -886,7 +886,7 @@ glamor_pixmap_upload_prepare(PixmapPtr pixmap, GLenum format, int no_alpha,
pixmap_priv = glamor_get_pixmap_private(pixmap);
glamor_priv = glamor_get_screen_private(pixmap->drawable.pScreen);
- if (pixmap_priv->base.gl_fbo)
+ if (pixmap_priv->base.gl_fbo != GLAMOR_FBO_UNATTACHED)
return 0;
if (pixmap_priv->base.fbo
diff --git a/glamor/glamor_priv.h b/glamor/glamor_priv.h
index 52dad35..0c69990 100644
--- a/glamor/glamor_priv.h
+++ b/glamor/glamor_priv.h
@@ -784,7 +784,7 @@ glamor_put_vbo_space(ScreenPtr screen);
* One copy of current pixmap's texture will be put into
* the pixmap->devPrivate.ptr. Will use pbo to map to
* the pointer if possible.
- * The pixmap must be a gl texture pixmap. gl_fbo and
+ * The pixmap must be a gl texture pixmap. gl_fbo must be GLAMOR_FBO_NORMAL and
* gl_tex must be 1. Used by glamor_prepare_access.
*
*/
@@ -799,9 +799,8 @@ void *glamor_download_sub_pixmap_to_cpu(PixmapPtr pixmap, int x, int y, int w,
* glamor_download_pixmap_to_cpu to its original
* gl texture. Used by glamor_finish_access.
*
- * The pixmap must be
- * in texture originally. In other word, the gl_fbo
- * must be 1.
+ * The pixmap must originally be a texture -- gl_fbo must be
+ * GLAMOR_FBO_NORMAL.
**/
void glamor_restore_pixmap_to_texture(PixmapPtr pixmap);
Plus wrote a new commit:
commit a58c974368bcdc725f1230989932fb8ba9e7b040
Author: Eric Anholt <eric at anholt.net>
Date: Thu Mar 13 10:09:08 2014 -0700
glamor: Drop stale comment.
The old Xephyr codebase was using the GL window system framebuffer for
the screen pixmap, but that meant you couldn't texture from it to do
operations sourcing from the screen, so in the version that landed I
instead had the screen just be a plain texture.
diff --git a/glamor/glamor_render.c b/glamor/glamor_render.c
index 8a98a9f..f59af66 100644
--- a/glamor/glamor_render.c
+++ b/glamor/glamor_render.c
@@ -962,10 +962,6 @@ glamor_composite_choose_shader(CARD8 op,
glamor_fallback("source == dest\n");
}
if (source_pixmap_priv->base.gl_fbo == GL_FBO_UNATTACHED) {
- /* XXX in Xephyr, we may have gl_fbo equal to 1 but gl_tex
- * equal to zero when the pixmap is screen pixmap. Then we may
- * refer the tex zero directly latter in the composition.
- * It seems that it works fine, but it may have potential problem*/
#ifdef GLAMOR_PIXMAP_DYNAMIC_UPLOAD
source_status = GLAMOR_UPLOAD_PENDING;
#else
As far as I can tell, the reason to have gl_tex and gl_fbo is that you
want to be able to tell quickly whether a pixmap priv can be rendered
from/to for fallback purposes. The alternative would apparently be to
switch on ->type, but there are a bunch of different types, or to just
look at the name of the fbo/texture for != 0, but those might be hidden
in the case of "large" pixmaps.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20140313/dac0887d/attachment.pgp>
More information about the xorg-devel
mailing list