On Mon, Sep 13, 2010 at 2:53 AM, Gregory Prisament
<g...@lycheesoftware.com> wrote:
> Hi,
>
> Attached is a patch that implements the following EGL extensions:
>
> EGL_KHR_gl_texture_2d_image
> EGL_KHR_gl_texture_3d_image
> EGL_KHR_gl_renderbuffer_image
> EGL_KHR_vg_parent_image
>
> Please review and, if you find it acceptable, someone can go ahead and apply
> it.
>
> This is my first contribution so please let me know if I should do things
> differently (either code-wise or process-wise).
Great work! I have a few comments below. It would also be good to
split the patch into implement the get_resource hook for OpenGL, for
OpenVG, and implement the EGL extensions.
> Thanks!
> -Greg, Lychee Software
> From c834c77d9f1b9512fde7ed7e36cdb36452286e1e Mon Sep 17 00:00:00 2001
> From: Gregory Prisament <g...@lycheesoftware.com>
> Date: Sun, 12 Sep 2010 11:26:38 -0700
> Subject: [PATCH 1/2] Implement additional EGL image extension.
>
> Implements:
> - EGL_KHR_gl_texture_2d_image
> - EGL_KHR_gl_texture_3d_image
> - EGL_KHR_gl_cubemap_image
> - EGL_KHR_gl_renderbuffer_image
> - EGL_KHR_vg_parent_image
> ---
> src/gallium/include/state_tracker/st_api.h | 6 +-
> src/gallium/state_trackers/egl/common/egl_g3d.c | 6 +
> .../state_trackers/egl/common/egl_g3d_image.c | 128
> ++++++++++++++++++--
> src/gallium/state_trackers/vega/vg_manager.c | 29 +++++
> src/mesa/state_tracker/st_manager.c | 61 +++++++++
> 5 files changed, 219 insertions(+), 11 deletions(-)
>
> diff --git a/src/gallium/include/state_tracker/st_api.h
> b/src/gallium/include/state_tracker/st_api.h
> index 8ea1554..d265d69 100644
> --- a/src/gallium/include/state_tracker/st_api.h
> +++ b/src/gallium/include/state_tracker/st_api.h
> @@ -117,7 +117,11 @@ enum st_context_resource_type {
> ST_CONTEXT_RESOURCE_OPENGL_TEXTURE_CUBE_MAP_POSITIVE_Z,
> ST_CONTEXT_RESOURCE_OPENGL_TEXTURE_CUBE_MAP_NEGATIVE_Z,
> ST_CONTEXT_RESOURCE_OPENGL_RENDERBUFFER,
> - ST_CONTEXT_RESOURCE_OPENVG_PARENT_IMAGE
> + ST_CONTEXT_RESOURCE_OPENVG_PARENT_IMAGE,
> +
> + ST_CONTEXT_RESOURCE_COUNT,
> + ST_CONTEXT_RESOURCE_INVALID = -1
> +
> };
>
> /**
> diff --git a/src/gallium/state_trackers/egl/common/egl_g3d.c
> b/src/gallium/state_trackers/egl/common/egl_g3d.c
> index 33a838f..d955ab4 100644
> --- a/src/gallium/state_trackers/egl/common/egl_g3d.c
> +++ b/src/gallium/state_trackers/egl/common/egl_g3d.c
> @@ -530,6 +530,12 @@ egl_g3d_initialize(_EGLDriver *drv, _EGLDisplay *dpy,
> if (gdpy->native->get_param(gdpy->native, NATIVE_PARAM_USE_NATIVE_BUFFER))
> dpy->Extensions.KHR_image_pixmap = EGL_TRUE;
>
> + dpy->Extensions.KHR_vg_parent_image = EGL_TRUE;
> + dpy->Extensions.KHR_gl_texture_2D_image = EGL_TRUE;
> + dpy->Extensions.KHR_gl_texture_cubemap_image = EGL_TRUE;
> + dpy->Extensions.KHR_gl_texture_3D_image = EGL_TRUE;
> + dpy->Extensions.KHR_gl_renderbuffer_image = EGL_TRUE;
> +
> dpy->Extensions.KHR_reusable_sync = EGL_TRUE;
> dpy->Extensions.KHR_fence_sync = EGL_TRUE;
>
> diff --git a/src/gallium/state_trackers/egl/common/egl_g3d_image.c
> b/src/gallium/state_trackers/egl/common/egl_g3d_image.c
> index 558638e..6bf7f7c 100644
> --- a/src/gallium/state_trackers/egl/common/egl_g3d_image.c
> +++ b/src/gallium/state_trackers/egl/common/egl_g3d_image.c
> @@ -230,6 +230,37 @@ egl_g3d_reference_drm_buffer(_EGLDisplay *dpy, EGLint
> name,
>
> #endif /* EGL_MESA_drm_image */
>
> +/* Map <target> param of eglCreateImageKHR to st_context_resource_type enum.
> */
> +static enum st_context_resource_type
> +egl_g3d_target_to_resource_type(EGLenum target)
> +{
> + switch (target) {
> + case EGL_GL_TEXTURE_2D_KHR:
> + return ST_CONTEXT_RESOURCE_OPENGL_TEXTURE_2D;
> + case EGL_GL_TEXTURE_3D_KHR:
> + return ST_CONTEXT_RESOURCE_OPENGL_TEXTURE_3D;
> + case EGL_GL_TEXTURE_CUBE_MAP_POSITIVE_X_KHR:
> + return ST_CONTEXT_RESOURCE_OPENGL_TEXTURE_CUBE_MAP_POSITIVE_X;
> + case EGL_GL_TEXTURE_CUBE_MAP_NEGATIVE_X_KHR:
> + return ST_CONTEXT_RESOURCE_OPENGL_TEXTURE_CUBE_MAP_NEGATIVE_X;
> + case EGL_GL_TEXTURE_CUBE_MAP_POSITIVE_Y_KHR:
> + return ST_CONTEXT_RESOURCE_OPENGL_TEXTURE_CUBE_MAP_POSITIVE_Y;
> + case EGL_GL_TEXTURE_CUBE_MAP_NEGATIVE_Y_KHR:
> + return ST_CONTEXT_RESOURCE_OPENGL_TEXTURE_CUBE_MAP_NEGATIVE_Y;
> + case EGL_GL_TEXTURE_CUBE_MAP_POSITIVE_Z_KHR:
> + return ST_CONTEXT_RESOURCE_OPENGL_TEXTURE_CUBE_MAP_POSITIVE_Z;
> + case EGL_GL_TEXTURE_CUBE_MAP_NEGATIVE_Z_KHR:
> + return ST_CONTEXT_RESOURCE_OPENGL_TEXTURE_CUBE_MAP_NEGATIVE_Z;
> + case EGL_GL_RENDERBUFFER_KHR:
> + return ST_CONTEXT_RESOURCE_OPENGL_RENDERBUFFER;
> + case EGL_VG_PARENT_IMAGE_KHR:
> + return ST_CONTEXT_RESOURCE_OPENVG_PARENT_IMAGE;
> + default:
> + return ST_CONTEXT_RESOURCE_INVALID;
> + }
> +}
> +
> +
> _EGLImage *
> egl_g3d_create_image(_EGLDriver *drv, _EGLDisplay *dpy, _EGLContext *ctx,
> EGLenum target, EGLClientBuffer buffer,
> @@ -237,6 +268,7 @@ egl_g3d_create_image(_EGLDriver *drv, _EGLDisplay *dpy,
> _EGLContext *ctx,
> {
> struct pipe_resource *ptex;
> struct egl_g3d_image *gimg;
> + struct egl_g3d_context *gctx;
> unsigned face = 0, level = 0, zslice = 0;
>
> gimg = CALLOC_STRUCT(egl_g3d_image);
> @@ -250,11 +282,76 @@ egl_g3d_create_image(_EGLDriver *drv, _EGLDisplay *dpy,
> _EGLContext *ctx,
> return NULL;
> }
>
> + gctx = egl_g3d_context(ctx);
> + if (!gctx) {
> + _eglError(EGL_BAD_CONTEXT, "eglCreateImageKHR");
> + FREE(gimg);
> + return NULL;
> + }
> +
> + /* Parse attributes */
> + while (*attribs != EGL_NONE)
> + {
> + EGLint attr_val = attribs[1];
> + switch (*attribs) {
> + case EGL_GL_TEXTURE_LEVEL_KHR:
> + level = attr_val;
> + break;
> + case EGL_GL_TEXTURE_ZOFFSET_KHR:
> + if (target != EGL_GL_TEXTURE_3D_KHR) {
> + goto handle_bad_parameter;
> + }
> + zslice = attr_val;
> + break;
> + default:
> + /* unsupported attribute */
> + goto handle_bad_parameter;
> + }
> + attribs += 2;
> + }
_eglInitImage parses these attributes. You can use
gimg->base.{GLTextureLevel,GLTextureZOffset}. The spec states that
unrecognized attributes should be ignored IIRC.
> + /* Lookup client-API resource */
> switch (target) {
> case EGL_NATIVE_PIXMAP_KHR:
> ptex = egl_g3d_reference_native_pixmap(dpy,
> (EGLNativePixmapType) buffer);
> break;
> + case EGL_GL_TEXTURE_CUBE_MAP_POSITIVE_X_KHR:
> + case EGL_GL_TEXTURE_CUBE_MAP_NEGATIVE_X_KHR:
> + case EGL_GL_TEXTURE_CUBE_MAP_POSITIVE_Y_KHR:
> + case EGL_GL_TEXTURE_CUBE_MAP_NEGATIVE_Y_KHR:
> + case EGL_GL_TEXTURE_CUBE_MAP_POSITIVE_Z_KHR:
> + case EGL_GL_TEXTURE_CUBE_MAP_NEGATIVE_Z_KHR:
> + face = (unsigned int)target -
> + (unsigned int)EGL_GL_TEXTURE_CUBE_MAP_POSITIVE_X_KHR;
> + /* Intentional fall-through: */
> + case EGL_GL_TEXTURE_2D_KHR:
> + case EGL_GL_TEXTURE_3D_KHR:
> + case EGL_GL_RENDERBUFFER_KHR:
> + case EGL_VG_PARENT_IMAGE_KHR:
> + {
> + boolean result;
> + struct st_context_resource stres;
> +
> + stres.type = egl_g3d_target_to_resource_type(target);
> + assert(stres.type != ST_CONTEXT_RESOURCE_INVALID);
> +
> + stres.resource = (void *)buffer;
> +
> + if (!gctx->stctxi->get_resource_for_egl_image) {
> + goto handle_bad_parameter;
> + }
> +
> + result =
> + gctx->stctxi->get_resource_for_egl_image(gctx->stctxi, &stres);
> + if (!result) {
> + goto handle_bad_parameter;
> + }
> +
> + ptex = stres.texture;
> + break;
> + }
> +
> #ifdef EGL_MESA_drm_image
> case EGL_DRM_BUFFER_MESA:
> ptex = egl_g3d_reference_drm_buffer(dpy, (EGLint) buffer, attribs);
> @@ -266,21 +363,14 @@ egl_g3d_create_image(_EGLDriver *drv, _EGLDisplay *dpy,
> _EGLContext *ctx,
> }
>
> if (!ptex) {
> - FREE(gimg);
> - return NULL;
> + goto handle_bad_parameter;
> }
>
> if (level > ptex->last_level) {
> - _eglError(EGL_BAD_MATCH, "eglCreateEGLImageKHR");
> - pipe_resource_reference(&gimg->texture, NULL);
> - FREE(gimg);
> - return NULL;
> + goto handle_bad_parameter;
> }
> if (zslice > ptex->depth0) {
> - _eglError(EGL_BAD_PARAMETER, "eglCreateEGLImageKHR");
> - pipe_resource_reference(&gimg->texture, NULL);
> - FREE(gimg);
> - return NULL;
> + goto handle_bad_parameter;
> }
>
> /* transfer the ownership to the image */
> @@ -290,6 +380,24 @@ egl_g3d_create_image(_EGLDriver *drv, _EGLDisplay *dpy,
> _EGLContext *ctx,
> gimg->zslice = zslice;
>
> return &gimg->base;
> +
> + /* error handling */
> +handle_bad_parameter:
> + _eglError(EGL_BAD_PARAMETER, "eglCreateEGLImageKHR");
> + goto handle_error;
> +
> +handle_bad_match:
> + _eglError(EGL_BAD_MATCH, "eglCreateEGLImageKHR");
> + goto handle_error;
> +
> +handle_error:
> + if (gimg && gimg->texture)
> + pipe_resource_reference(&gimg->texture, NULL);
> +
> + if (gimg)
> + FREE(gimg);
> +
> + return NULL;
> }
>
> EGLBoolean
> diff --git a/src/gallium/state_trackers/vega/vg_manager.c
> b/src/gallium/state_trackers/vega/vg_manager.c
> index e799674..86221e2 100644
> --- a/src/gallium/state_trackers/vega/vg_manager.c
> +++ b/src/gallium/state_trackers/vega/vg_manager.c
> @@ -332,6 +332,32 @@ vg_context_flush(struct st_context_iface *stctxi,
> unsigned flags,
> vg_manager_flush_frontbuffer(ctx);
> }
>
> +static boolean
> +vg_context_get_resource_for_egl_image(struct st_context_iface *stctxi,
> + struct st_context_resource *stres)
> +{
> + struct vg_context *ctx;
> + struct vg_image *img;
> + struct pipe_sampler_view *sampler_view;
> +
> + ctx = (struct vg_context *) stctxi;
> + if (!ctx)
> + return FALSE;
This check could be replaced by an assertion. Calling stctxi's method
without passing stctxi is a bug elsewhere.
> +
> + img = (struct vg_image *)stres->resource;
A check on stres->type and res->resource (!= VG_INVALID_HANDLE) would
be good before the assignment.
> + if (!img)
> + return FALSE;
> +
> + sampler_view = img->sampler_view;
> + if (!sampler_view)
> + return FALSE;
> +
> + stres->texture = sampler_view->texture;
It should be
stres->texture = NULL;
pipe_resource_reference(&stres->texture, sampler_view->texture);
stres->texture is caller owned.
> +
> + return TRUE;
> +}
> +
> +
> static void
> vg_context_destroy(struct st_context_iface *stctxi)
> {
> @@ -373,6 +399,9 @@ vg_api_create_context(struct st_api *stapi, struct
> st_manager *smapi,
> ctx->iface.teximage = NULL;
> ctx->iface.copy = NULL;
>
> + ctx->iface.get_resource_for_egl_image =
> + vg_context_get_resource_for_egl_image;
> +
> ctx->iface.st_context_private = (void *) smapi;
>
> return &ctx->iface;
> diff --git a/src/mesa/state_tracker/st_manager.c
> b/src/mesa/state_tracker/st_manager.c
> index 450b045..f8f4223 100644
> --- a/src/mesa/state_tracker/st_manager.c
> +++ b/src/mesa/state_tracker/st_manager.c
> @@ -601,6 +601,66 @@ st_context_teximage(struct st_context_iface *stctxi,
> enum st_texture_type target
> return TRUE;
> }
>
> +static boolean
> +st_context_get_resource_for_egl_image(struct st_context_iface *stctxi,
> + struct st_context_resource *stres)
> +{
> + struct st_context *st = (struct st_context *) stctxi;
> +
> + switch (stres->type) {
> + case ST_CONTEXT_RESOURCE_OPENGL_TEXTURE_2D:
> + case ST_CONTEXT_RESOURCE_OPENGL_TEXTURE_3D:
> + case ST_CONTEXT_RESOURCE_OPENGL_TEXTURE_CUBE_MAP_POSITIVE_X:
> + case ST_CONTEXT_RESOURCE_OPENGL_TEXTURE_CUBE_MAP_NEGATIVE_X:
> + case ST_CONTEXT_RESOURCE_OPENGL_TEXTURE_CUBE_MAP_POSITIVE_Y:
> + case ST_CONTEXT_RESOURCE_OPENGL_TEXTURE_CUBE_MAP_NEGATIVE_Y:
> + case ST_CONTEXT_RESOURCE_OPENGL_TEXTURE_CUBE_MAP_POSITIVE_Z:
> + case ST_CONTEXT_RESOURCE_OPENGL_TEXTURE_CUBE_MAP_NEGATIVE_Z:
> + {
> + GLuint texID;
> + struct gl_texture_object *texObj;
> + struct pipe_resource *resource;
> +
> + texID = (GLuint)stres->resource;
> +
> + texObj = _mesa_lookup_texture(st->ctx, texID);
> + if (!texObj)
> + return FALSE;
> +
Should texObj be _mesa_lock_texture here? It seems so to me, but I am
not entirely sure.
> + resource = st_get_texobj_resource(texObj);
> + if (!resource)
> + return FALSE;
> +
> + stres->texture = resource;
stres->texture is caller owned.
> + return TRUE;
> + }
> + case ST_CONTEXT_RESOURCE_OPENGL_RENDERBUFFER:
> + {
> + GLuint rbID;
> + struct gl_renderbuffer *rbObj;
> + struct st_renderbuffer *stRb;
> +
> + rbID = (GLuint)stres->resource;
> +
> + rbObj = _mesa_lookup_renderbuffer(st->ctx, rbID);
> + if (!rbObj)
> + return FALSE;
> +
> + stRb = st_renderbuffer(rbObj);
> + if (!stRb)
> + return FALSE;
> +
> + stres->texture = stRb->texture;
Likewise.
> + return TRUE;
> + }
> + default:
> + return FALSE;
> + }
> +
> + return FALSE;
> +}
> +
> +
> static void
> st_context_destroy(struct st_context_iface *stctxi)
> {
> @@ -669,6 +729,7 @@ st_api_create_context(struct st_api *stapi, struct
> st_manager *smapi,
> st->iface.flush = st_context_flush;
> st->iface.teximage = st_context_teximage;
> st->iface.copy = NULL;
> + st->iface.get_resource_for_egl_image =
> st_context_get_resource_for_egl_image;
> st->iface.st_context_private = (void *) smapi;
>
> return &st->iface;
> --
> 1.6.3.3
>
I got some trailing space complaints when git am. Please fix them.
The rest of the patch looks good to me.
--
o...@lunarg.com
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
