[Glamor] [PATCH 1/1 v2] Refine CloseScreen and FreeScreen processes.
Zhigang Gong
zhigang.gong at linux.intel.com
Tue Feb 7 21:34:42 PST 2012
> -----Original Message-----
> From:
> glamor-bounces+zhigang.gong=linux.intel.com at lists.freedesktop.org
> [mailto:glamor-bounces+zhigang.gong=linux.intel.com at lists.freedesktop.o
> rg] On Behalf Of Li, Peng
> Sent: Wednesday, February 08, 2012 1:20 PM
> To: zhigang.gong at linux.intel.com; glamor at lists.freedesktop.org
> Subject: Re: [Glamor] [PATCH 1/1 v2] Refine CloseScreen and FreeScreen
> processes.
>
> Zhigang
>
> Please add #if GLAMOR_HAS_GBM before below piece of code.
> Otherwise it won't build success if no gbm available.
> Besides that, The patch works well for me, you can add my name in Test-By.
>
> + if (glamor_egl->gbm)
> + gbm_device_destroy(glamor_egl->gbm);
> +
Thanks, will add that.
>
> Thanks
> Peng
>
> -----Original Message-----
> From: glamor-bounces+peng.li=intel.com at lists.freedesktop.org
> [mailto:glamor-bounces+peng.li=intel.com at lists.freedesktop.org] On
> Behalf Of zhigang.gong at linux.intel.com
> Sent: Wednesday, February 01, 2012 7:46 PM
> To: glamor at lists.freedesktop.org
> Cc: zhigang.gong at linux.intel.com
> Subject: [Glamor] [PATCH 1/1 v2] Refine CloseScreen and FreeScreen
> processes.
>
> From: Zhigang Gong <zhigang.gong at linux.intel.com>
>
> This commit move the calling to glamor_close_screen from
> glamor_egl_free_screen to glamor_egl_close_screen, as this is the right
> place to do this.
>
> We should detach screen fbo and destroy the corresponding KHR image at
> glamor_egl_close_screen stage. As latter DDX driver will call
> DestroyPixmap to destroy screen pixmap, if the fbo and image are still
> there but glamor screen private data pointer has been freed, then it
> causes segfault.
>
> This commit also introduces a new flag GLAMOR_USE_EGL_SCREEN.
> if DDX driver is using EGL layer then should set this bit when call to
> glamor_init and then both glamor_close_screen and
> glamor_egl_close_screen will be registered correctly, DDX layer will not
> need to call these two functions manually.
> This way is also the preferred method within Xorg domain.
>
> As interfaces changed, bump the version to 0.3.0.
>
> Signed-off-by: Zhigang Gong <zhigang.gong at linux.intel.com>
> ---
> configure.ac | 2 +-
> src/glamor.c | 24 +++++++----
> src/glamor.h | 73 +++++++++++++++++----------------
> src/glamor_egl.c | 117
> ++++++++++++++++++++++++++++++++++-------------------
> src/glamor_fbo.c | 2 +-
> src/glamor_priv.h | 1 +
> 6 files changed, 131 insertions(+), 88 deletions(-)
>
> diff --git a/configure.ac b/configure.ac index f283d4f..1d19534 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -24,7 +24,7 @@
> # Initialize Autoconf
> AC_PREREQ([2.63])
> AC_INIT([glamor-egl],
> - [0.2.0],
> + [0.3.0],
> [https://bugs.freedesktop.org/enter_bug.cgi?product=glamor],
> [glamor-egl])
>
> diff --git a/src/glamor.c b/src/glamor.c index 80434e9..e900a91 100644
> --- a/src/glamor.c
> +++ b/src/glamor.c
> @@ -318,6 +318,13 @@ glamor_init(ScreenPtr screen, unsigned int flags)
> #else
> glamor_priv->gl_flavor = GLAMOR_GL_DESKTOP; #endif
> + /* If we are using egl screen, call egl screen init to
> + * register correct close screen function. */
> + if (flags & GLAMOR_USE_EGL_SCREEN)
> + glamor_egl_screen_init(screen);
> +
> + glamor_priv->saved_procs.close_screen = screen->CloseScreen;
> + screen->CloseScreen = glamor_close_screen;
>
> if (flags & GLAMOR_USE_SCREEN) {
> if (!RegisterBlockAndWakeupHandlers(_glamor_block_handler,
> @@ -327,9 +334,6 @@ glamor_init(ScreenPtr screen, unsigned int flags)
> goto fail;
> }
>
> - glamor_priv->saved_procs.close_screen = screen->CloseScreen;
> - screen->CloseScreen = glamor_close_screen;
> -
> glamor_priv->saved_procs.create_gc = screen->CreateGC;
> screen->CreateGC = glamor_create_gc;
>
> @@ -427,6 +431,9 @@ Bool
> glamor_close_screen(int idx, ScreenPtr screen) {
> glamor_screen_private *glamor_priv;
> + PixmapPtr screen_pixmap;
> + glamor_pixmap_private *screen_pixmap_priv;
> + glamor_pixmap_fbo *fbo;
> int flags;
>
> glamor_priv = glamor_get_screen_private(screen);
> @@ -435,9 +442,9 @@ glamor_close_screen(int idx, ScreenPtr screen)
> PictureScreenPtr ps = GetPictureScreenIfSet(screen); #endif
> glamor_glyphs_fini(screen);
> + screen->CloseScreen = glamor_priv->saved_procs.close_screen;
> if (flags & GLAMOR_USE_SCREEN) {
>
> - screen->CloseScreen = glamor_priv->saved_procs.close_screen;
> screen->CreateGC = glamor_priv->saved_procs.create_gc;
> screen->CreatePixmap =
> glamor_priv->saved_procs.create_pixmap;
> screen->DestroyPixmap =
> glamor_priv->saved_procs.destroy_pixmap;
> @@ -457,12 +464,13 @@ glamor_close_screen(int idx, ScreenPtr
> screen)
> ps->CreatePicture = glamor_priv->saved_procs.create_picture;
> }
> #endif
> + screen_pixmap = screen->GetScreenPixmap(screen);
> + screen_pixmap_priv = glamor_get_pixmap_private(screen_pixmap);
> + fbo = glamor_pixmap_detach_fbo(screen_pixmap_priv);
> + glamor_purge_fbo(fbo);
> glamor_release_screen_priv(screen);
>
> - if (flags & GLAMOR_USE_SCREEN)
> - return screen->CloseScreen(idx, screen);
> - else
> - return TRUE;
> + return screen->CloseScreen(idx, screen);
> }
>
>
> diff --git a/src/glamor.h b/src/glamor.h index fe15700..e29a3e4 100644
> --- a/src/glamor.h
> +++ b/src/glamor.h
> @@ -41,36 +41,6 @@
>
> #endif /* GLAMOR_H */
>
> -/* @GLAMOR_INVERTED_Y_AXIS:
> - * set 1 means the GL env's origin (0,0) is at top-left.
> - * EGL/DRM platform is an example need to set this bit.
> - * glx platform's origin is at bottom-left thus need to
> - * clear this bit.*/
> -
> -#define GLAMOR_INVERTED_Y_AXIS 1
> -
> -/* @GLAMOR_USE_SCREEN:
> - * If want to let glamor to do everything including the
> - * create/destroy pixmap and handle the gc ops. need to
> - * set this bit. Standalone glamor DDX driver need to set
> - * this bit.
> - * Otherwise, need to clear this bit, as the intel video
> - * driver with glamor enabled.
> - * */
> -#define GLAMOR_USE_SCREEN 2
> -/* @GLAMOR_USE_PICTURE_SCREEN:
> - * If want to let glamor to do all the composition related
> - * things, need to set this bit. Just as standalone glamor
> - * DDX driver.
> - * Otherwise, need to clear this bit, as the intel video
> - * driver with glamor enabled.
> - */
> -#define GLAMOR_USE_PICTURE_SCREEN 4
> -
> -#define GLAMOR_VALID_FLAGS (GLAMOR_INVERTED_Y_AXIS
> \
> - | GLAMOR_USE_SCREEN \
> - | GLAMOR_USE_PICTURE_SCREEN)
> -
> /*
> * glamor_pixmap_type : glamor pixmap's type.
> * @MEMORY: pixmap is in memory.
> @@ -90,15 +60,49 @@ typedef enum glamor_pixmap_type { }
> glamor_pixmap_type_t;
>
> #define GLAMOR_EGL_EXTERNAL_BUFFER 3
> +#define GLAMOR_INVERTED_Y_AXIS 1
> +#define GLAMOR_USE_SCREEN (1 << 1)
> +#define GLAMOR_USE_PICTURE_SCREEN (1 << 2)
> +#define GLAMOR_USE_EGL_SCREEN (1 << 3)
> +#define GLAMOR_VALID_FLAGS (GLAMOR_INVERTED_Y_AXIS
> \
> + | GLAMOR_USE_SCREEN \
> + | GLAMOR_USE_PICTURE_SCREEN
> \
> + | GLAMOR_USE_EGL_SCREEN)
> +
> /* @glamor_init: Initialize glamor internal data structure.
> *
> * @screen: Current screen pointer.
> * @flags: Please refer the flags description above.
> *
> + * @GLAMOR_INVERTED_Y_AXIS:
> + * set 1 means the GL env's origin (0,0) is at top-left.
> + * EGL/DRM platform is an example need to set this bit.
> + * glx platform's origin is at bottom-left thus need to
> + * clear this bit.
> + *
> + * @GLAMOR_USE_SCREEN:
> + * If running in an pre-existing X environment, and the
> + * gl context is GLX, then you should set this bit and
> + * let the glamor to handle all the screen related
> + * functions such as GC ops and CreatePixmap/DestroyPixmap.
> + *
> + * @GLAMOR_USE_PICTURE_SCREEN:
> + * If don't use any other underlying DDX driver to handle
> + * the picture related rendering functions, please set this
> + * bit on. Otherwise, clear this bit. And then it is the DDX
> + * driver's responsibility to determine how/when to jump to
> + * glamor's picture compositing path.
> + *
> + * @GLAMOR_USE_EGL_SCREEN:
> + * If you are using EGL layer, then please set this bit
> + * on, otherwise, clear it.
> + *
> * This function initializes necessary internal data structure
> * for glamor. And before calling into this function, the OpenGL
> * environment should be ready. Should be called before any real
> - * glamor rendering or texture allocation functions.
> + * glamor rendering or texture allocation functions. And should
> + * be called after the DDX's screen initialization or at the last
> + * step of the DDX's screen initialization.
> */
> extern _X_EXPORT Bool glamor_init(ScreenPtr screen, unsigned int flags);
> extern _X_EXPORT void glamor_fini(ScreenPtr screen); @@ -139,6
> +143,8 @@ extern _X_EXPORT void glamor_block_handler(ScreenPtr
> screen); extern _X_EXPORT PixmapPtr
> glamor_create_pixmap(ScreenPtr screen, int w, int h, int depth,
> unsigned int usage);
>
> +extern _X_EXPORT void glamor_egl_screen_init(ScreenPtr screen);
> +
> #ifdef GLAMOR_FOR_XORG
> /* @glamor_egl_init: Initialize EGL environment.
> *
> @@ -173,7 +179,6 @@ extern _X_EXPORT Bool
> glamor_egl_init_textured_pixmap(ScreenPtr screen); extern
> _X_EXPORT Bool glamor_egl_create_textured_screen(ScreenPtr screen,
> int handle,
> int stride);
> -
> /*
> * @glamor_egl_create_textured_pixmap: Try to create a textured
> pixmap from
> * a BO handle.
> @@ -191,10 +196,6 @@ extern _X_EXPORT Bool
> glamor_egl_create_textured_pixmap(PixmapPtr pixmap,
> int stride);
>
> extern _X_EXPORT void
> glamor_egl_destroy_textured_pixmap(PixmapPtr pixmap);
> -
> -extern _X_EXPORT Bool glamor_egl_close_screen(ScreenPtr screen);
> -extern _X_EXPORT void glamor_egl_free_screen(int scrnIndex, int flags);
> -
> #endif
>
> extern _X_EXPORT int glamor_create_gc(GCPtr gc); diff --git
> a/src/glamor_egl.c b/src/glamor_egl.c index 71121bc..ed98aad 100644
> --- a/src/glamor_egl.c
> +++ b/src/glamor_egl.c
> @@ -84,7 +84,7 @@ struct glamor_egl_screen_private {
> CreateScreenResourcesProcPtr CreateScreenResources;
> CloseScreenProcPtr CloseScreen;
> int fd;
> - int front_buffer_handle;
> + EGLImageKHR front_image;
> int cpp;
> #ifdef GLAMOR_HAS_GBM
> struct gbm_device *gbm;
> @@ -95,6 +95,8 @@ struct glamor_egl_screen_private {
> PFNEGLDESTROYIMAGEKHRPROC egl_destroy_image_khr;
> PFNGLEGLIMAGETARGETTEXTURE2DOESPROC
> egl_image_target_texture2d_oes;
> struct glamor_gl_dispatch *dispatch;
> + CloseScreenProcPtr saved_close_screen;
> + xf86FreeScreenProc *saved_free_screen;
> };
>
> int xf86GlamorEGLPrivateIndex = -1;
> @@ -185,6 +187,8 @@ glamor_egl_create_textured_screen(ScreenPtr
> screen, int handle, int stride)
> return FALSE;
> }
>
> + glamor_egl->front_image =
> dixLookupPrivate(&screen_pixmap->devPrivates,
> +
glamor_egl_pixmap_private_key);
> glamor_set_screen_pixmap(screen_pixmap);
> return TRUE;
> }
> @@ -242,37 +246,51 @@
> glamor_egl_create_textured_pixmap(PixmapPtr pixmap, int handle, int
> stride)
> return TRUE;
> }
>
> -void
> -glamor_egl_destroy_textured_pixmap(PixmapPtr pixmap)
> +static void
> +_glamor_egl_destroy_pixmap_image(PixmapPtr pixmap)
> {
> - EGLImageKHR image;
> ScrnInfoPtr scrn =
> xf86Screens[pixmap->drawable.pScreen->myNum];
> + EGLImageKHR image;
> struct glamor_egl_screen_private *glamor_egl =
> glamor_egl_get_screen_private(scrn);
>
> - if (pixmap->refcnt == 1) {
> - image = dixLookupPrivate(&pixmap->devPrivates,
> - glamor_egl_pixmap_private_key);
> - if (image != EGL_NO_IMAGE_KHR && image != NULL) {
> - /* Before destroy an image which was attached to
> - * a texture. we must call glFlush to make sure the
> - * operation on that texture has been done.*/
> - glamor_block_handler(pixmap->drawable.pScreen);
> -
glamor_egl->egl_destroy_image_khr(glamor_egl->display,
> image);
> - }
> + image = dixLookupPrivate(&pixmap->devPrivates,
> + glamor_egl_pixmap_private_key);
> + if (image != EGL_NO_IMAGE_KHR && image != NULL) {
> + /* Before destroy an image which was attached to
> + * a texture. we must call glFlush to make sure the
> + * operation on that texture has been done.*/
> + glamor_block_handler(pixmap->drawable.pScreen);
> + glamor_egl->egl_destroy_image_khr(glamor_egl->display,
> image);
> + dixSetPrivate(&pixmap->devPrivates,
> glamor_egl_pixmap_private_key,
> +NULL);
> }
> +}
> +
> +void
> +glamor_egl_destroy_textured_pixmap(PixmapPtr pixmap) {
> + if (pixmap->refcnt == 1)
> + _glamor_egl_destroy_pixmap_image(pixmap);
> glamor_destroy_textured_pixmap(pixmap);
> }
>
> -Bool
> -glamor_egl_close_screen(ScreenPtr screen)
> +static Bool
> +glamor_egl_close_screen(int idx, ScreenPtr screen)
> {
> - ScrnInfoPtr scrn = xf86Screens[screen->myNum];
> - struct glamor_egl_screen_private *glamor_egl =
> - glamor_egl_get_screen_private(scrn);
> - glamor_fini(screen);
> + ScrnInfoPtr scrn;
> + struct glamor_egl_screen_private *glamor_egl;
> + PixmapPtr screen_pixmap;
>
> - return TRUE;
> + scrn = xf86Screens[screen->myNum];
> + glamor_egl = glamor_egl_get_screen_private(scrn);
> + screen_pixmap = screen->GetScreenPixmap(screen);
> + glamor_egl->egl_destroy_image_khr(glamor_egl->display,
> glamor_egl->front_image);
> + dixSetPrivate(&screen_pixmap->devPrivates,
> glamor_egl_pixmap_private_key, NULL);
> + glamor_egl->front_image = NULL;
> +
> + screen->CloseScreen = glamor_egl->saved_close_screen;
> +
> + return screen->CloseScreen(idx, screen);
> }
>
> static Bool
> @@ -299,6 +317,40 @@ glamor_egl_has_extension(struct
> glamor_egl_screen_private *glamor_egl,
> return FALSE;
> }
>
> +void
> +glamor_egl_screen_init(ScreenPtr screen) {
> + ScrnInfoPtr scrn = xf86Screens[screen->myNum];
> + struct glamor_egl_screen_private *glamor_egl =
> + glamor_egl_get_screen_private(scrn);
> +
> + glamor_egl->saved_close_screen = screen->CloseScreen;
> + screen->CloseScreen = glamor_egl_close_screen; }
> +
> +static void
> +glamor_egl_free_screen(int scrnIndex, int flags) {
> + ScrnInfoPtr scrn = xf86Screens[scrnIndex];
> + struct glamor_egl_screen_private *glamor_egl;
> +
> + ErrorF("free egl screen resources\n");
> + glamor_egl = glamor_egl_get_screen_private(scrn);
> + if (glamor_egl != NULL) {
> +
> + eglMakeCurrent(glamor_egl->display,
> + EGL_NO_SURFACE, EGL_NO_SURFACE,
> + EGL_NO_CONTEXT);
> +
> + if (glamor_egl->gbm)
> + gbm_device_destroy(glamor_egl->gbm);
> +
> + scrn->FreeScreen = glamor_egl->saved_free_screen;
> + free(glamor_egl);
> + scrn->FreeScreen(scrnIndex, flags);
> + }
> +}
> +
> Bool
> glamor_egl_init(ScrnInfoPtr scrn, int fd) { @@ -396,6 +448,8 @@
> glamor_egl_init(ScrnInfoPtr scrn, int fd)
> return FALSE;
> }
>
> + glamor_egl->saved_free_screen = scrn->FreeScreen;
> + scrn->FreeScreen = glamor_egl_free_screen;
> return TRUE;
> }
>
> @@ -412,27 +466,6 @@ glamor_egl_init_textured_pixmap(ScreenPtr
> screen)
> return TRUE;
> }
>
> -void
> -glamor_egl_free_screen(int scrnIndex, int flags) -{
> - ScrnInfoPtr scrn = xf86Screens[scrnIndex];
> - struct glamor_egl_screen_private *glamor_egl =
> - glamor_egl_get_screen_private(scrn);
> -
> - if (glamor_egl != NULL) {
> - if (!(flags & GLAMOR_EGL_EXTERNAL_BUFFER)) {
> - eglMakeCurrent(glamor_egl->display,
> - EGL_NO_SURFACE, EGL_NO_SURFACE,
> - EGL_NO_CONTEXT);
> -
> - eglTerminate(glamor_egl->display);
> - }
> - free(glamor_egl);
> - }
> -
> - glamor_close_screen(scrn->scrnIndex, scrn->pScreen);
> -}
> -
> Bool
> glamor_gl_dispatch_init(ScreenPtr screen,
> struct glamor_gl_dispatch *dispatch, diff --git
> a/src/glamor_fbo.c b/src/glamor_fbo.c index ffda04a..2243564 100644
> --- a/src/glamor_fbo.c
> +++ b/src/glamor_fbo.c
> @@ -123,7 +123,7 @@
> glamor_pixmap_fbo_cache_get(glamor_screen_private *glamor_priv,
> return NULL;
> }
>
> -static void
> +void
> glamor_purge_fbo(glamor_pixmap_fbo *fbo) {
> glamor_gl_dispatch *dispatch = &fbo->glamor_priv->dispatch; diff
> --git a/src/glamor_priv.h b/src/glamor_priv.h index 9c2881c..d8168a5
> 100644
> --- a/src/glamor_priv.h
> +++ b/src/glamor_priv.h
> @@ -346,6 +346,7 @@ glamor_pixmap_fbo *
> glamor_create_fbo_from_tex(glamor_screen_private *glamor_pri
> glamor_pixmap_fbo * glamor_create_fbo(glamor_screen_private
> *glamor_priv,
> int w, int h, int depth, int flag);
void
> glamor_destroy_fbo(glamor_pixmap_fbo *fbo);
> +void glamor_purge_fbo(glamor_pixmap_fbo *fbo);
>
> void glamor_init_pixmap_fbo(ScreenPtr screen); void
> glamor_fini_pixmap_fbo(ScreenPtr screen);
> --
> 1.7.4.4
>
> _______________________________________________
> Glamor mailing list
> Glamor at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/glamor
> _______________________________________________
> Glamor mailing list
> Glamor at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/glamor
More information about the Glamor
mailing list