[Mesa-dev] [PATCH] intel: implement create image from texture (v8)

[Abdiel Janulgue]

On Thursday, January 31, 2013 10:31:33 AM Chad Versace wrote:

> > +#include "egl/main/eglcurrent.h"
> > 
> > 
> > +static __DRIimage *
> > +intel_create_image_from_texture(__DRIcontext *context, int target,
> > +                                unsigned texture, int zoffset,
> > +                                int level,
> > +                                void *loaderPrivate)
> > +{
> 
> This function directly calls _eglError(). Nowhere else
> in the i965 driver are direct calls made to GLX or EGL. If,
> in the future, we wish to use this function when a GLXContext
> is current, the behavior would be undefined.

See last comment below.

> 
> To fix this, I would add an out parameter `int *dri_error` to
> __DRIimageExtensionRec::createImageFromTexture. Then
> dri2_create_image_khr_texture() would inspect the DRI error
> code and emit the matching EGL error. This is exactly what the DRI interface
> does for DRIdri2ExtensionRec::CreateContext, which is implemented by
> intelCreateContext().
> 
> However, dri_interface.h does not define any appropriate error codes for
> EGL_BAD_{ALLOC,PARAMETER,MATCH}. So you would need to define
> new ones. I would name them to match the EGL names, such
> as __DRI_ERROR_BAD_{ALLOC,PARAMETER,MATCH}. Since the set of
> possible error codes is part of a function's ABI, the
> __DRIimageExtensionRec::createImageFromTexture documentation
> should explain what that set is.

Sounds reasonable.

> 
> > +   __DRIimage *image;
> > +   struct intel_context *intel = context->driverPrivate;
> > +   struct gl_texture_object *obj;
> > +   struct intel_texture_object *iobj;
> > +   GLuint face = 0;
> > +
> > +   obj = _mesa_lookup_texture(&intel->ctx, texture);
> > +   if (!obj || obj->Target != target) {
> > +      _eglError(EGL_BAD_PARAMETER, __func__);
> > +      return NULL;
> > +   }
> > +
> > +   if (target == GL_TEXTURE_CUBE_MAP)
> > +      face = zoffset;
> > +
> > +   _mesa_test_texobj_completeness(&intel->ctx, obj);
> > +   iobj = intel_texture_object(obj);
> > +   if (!obj->_BaseComplete || (level > 0 && !obj->_MipmapComplete)) {
> > +      _eglError(EGL_BAD_PARAMETER, __func__);
> > +      return NULL;
> > +   }
> > +
> > +   if (level < obj->BaseLevel || level > obj->_MaxLevel) {
> > +      _eglError(EGL_BAD_MATCH, __func__);
> > +      return NULL;
> > +   }
> > +
> > +   if (target == GL_TEXTURE_3D && obj->Image[face][level]->Depth <
> > zoffset) { +      _eglError(EGL_BAD_MATCH, __func__);
> > +      return NULL;
> > +   }
> > +   image = calloc(1, sizeof *image);
> > +   if (image == NULL) {
> > +      _eglError(EGL_BAD_ALLOC, __func__);
> > +      return NULL;
> > +   }
> > +
> > +   image->internal_format = obj->Image[face][level]->InternalFormat;
> > +   image->format = obj->Image[face][level]->TexFormat;
> > +   image->data = loaderPrivate;
> > +   if (iobj->mt->stencil_mt ||
> > +       !intel_setup_image_from_mipmap_tree(intel, image, iobj->mt, level,
> > zoffset)) { +      _mesa_error(&intel->ctx, GL_INVALID_OPERATION,
> > __func__);
> 
> This function emits a mixture of EGL and GL errors. Is that allowed by the
> extension spec?

Bummer, you are right. Spec says the GL_INVALID_OPERATION should  be 
implemented on the OES_EGL_Image side of the wall. Anyway, I think I can move 
the page alignment surface address checks from the image-to-texture to the 
texture-from-image side. The only thing I foresee here is we need another 
field in DRIimage to indicate if the  EGLimages representing them are 
depth/stencil textures.

Well back to the drawing board I guess. But I think this should take a couple 
of changes in patches 2 and 8 at least.

> 
> > +      free(image);
> > +      return NULL;
> > +   }
> > +   image->dri_format = intel_dri_format(image->format);
> > +   if (image->dri_format == MESA_FORMAT_NONE) {
> > +      fprintf(stderr, "%s: Cannot make EGL image from invalid format.\n",
> > __func__); +      free(image);
> 
> An error code needs to be emitted here, or the EGL client might get
> confused. Perhaps __DRI_ERROR_BAD_PARAMETER?
> 
> > +      return NULL;
> > 
> >     }
> 
> Removing the calls to _eglError fixes the Android build.

Yup, earlier it was in the back of my head that calling _eglError to error out 
in driver code doesn't seem to sound good at all. But it  worked quite well on 
normal non-Android build. So i tried poking around Android compiler flags to 
try to pacify its complaint. Anyway, I'll implement the *dri_error suggestion 
you had above.