[Mesa-dev] [PATCH 09/10] egl: enhance KHR_gl_image extensions checks

[Emil Velikov]

On 6 July 2017 at 15:45, Eric Engestrom <eric.engestrom at imgtec.com> wrote:
> On Friday, 2017-06-30 12:15:19 +0100, Emil Velikov wrote:
>> From: Emil Velikov <emil.velikov at collabora.com>
>>
>> Drop the (duplicate) top-level check in dri2_create_image_khr() and add
>> the respective checks in dri2_create_image_khr_{texture,renderbuffer}
>>
>> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
>> ---
>>  src/egl/drivers/dri2/egl_dri2.c | 36 +++++++++++++++++++++---------------
>>  1 file changed, 21 insertions(+), 15 deletions(-)
>>
>> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
>> index a641d774523..78a6d5f2219 100644
>> --- a/src/egl/drivers/dri2/egl_dri2.c
>> +++ b/src/egl/drivers/dri2/egl_dri2.c
>> @@ -1682,6 +1682,11 @@ dri2_create_image_khr_renderbuffer(_EGLDisplay *disp, _EGLContext *ctx,
>>        return EGL_NO_IMAGE_KHR;
>>     }
>>
>> +   if (!disp->Extensions.KHR_gl_renderbuffer_image) {
>> +      _eglError(EGL_BAD_PARAMETER, "dri2_create_image_khr");
>> +      return EGL_NO_IMAGE_KHR;
>> +   }
>> +
>>     dri_image =
>>        dri2_dpy->image->createImageFromRenderbuffer(dri2_ctx->dri_context,
>>                                                     renderbuffer, NULL);
>> @@ -1820,30 +1825,38 @@ dri2_create_image_khr_texture(_EGLDisplay *disp, _EGLContext *ctx,
>>
>>     switch (target) {
>>     case EGL_GL_TEXTURE_2D_KHR:
>> +      if (!disp->Extensions.KHR_gl_texture_2D_image) {
>> +         _eglError(EGL_BAD_PARAMETER, "dri2_create_image_khr");
>> +         return EGL_NO_IMAGE_KHR;
>> +      }
>>        depth = 0;
>>        gl_target = GL_TEXTURE_2D;
>>        break;
>>     case EGL_GL_TEXTURE_3D_KHR:
>> -      if (disp->Extensions.KHR_gl_texture_3D_image) {
>> -         depth = attrs.GLTextureZOffset;
>> -         gl_target = GL_TEXTURE_3D;
>> -         break;
>> -      }
>> -      else {
>> +      if (!disp->Extensions.KHR_gl_texture_3D_image) {
>>           _eglError(EGL_BAD_PARAMETER, "dri2_create_image_khr");
>>           return EGL_NO_IMAGE_KHR;
>>        }
>> +
>> +      depth = attrs.GLTextureZOffset;
>> +      gl_target = GL_TEXTURE_3D;
>> +      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:
>> +      if (!disp->Extensions.KHR_gl_texture_cubemap_image) {
>> +         _eglError(EGL_BAD_PARAMETER, "dri2_create_image_khr");
>> +         return EGL_NO_IMAGE_KHR;
>> +      }
>> +
>>        depth = target - EGL_GL_TEXTURE_CUBE_MAP_POSITIVE_X_KHR;
>>        gl_target = GL_TEXTURE_CUBE_MAP;
>>        break;
>>     default:
>> -      _eglError(EGL_BAD_PARAMETER, "dri2_create_image_khr");
>> +      assert(!"Unexpected target in dri2_create_image_khr_texture()");
>
> I think the _eglError() should stay.
>
This function is called (by dri2_create_image_khr) when the target is
either TEXTURE_2D/3D or any of the 6 TEXTURE_CUBE_MAP.
Hence the default case is unreachable. I've opted for an assert, since
we'd want to quickly get feedback if we get there.

> My comment on patch 1/10 clearly shows that I didn't read the whole
> series before replying: you can ignore it :]
>
> Series is:
> Reviewed-by: Eric Engestrom <eric.engestrom at imgtec.com>
>
> Btw, I really like the solution in patch 3/10, I had this split on my
> todo-list for a while, but I hadn't thought of a good way to avoid an
> `if`-nesting-chain; your solution looks better than what I had in mind :)
>
Glad to hear you like it. There's still a bit of ugly in there, but
it's should be easier to follow than the previous 200+ lines switch
statement ;-)

Thanks
Emil