[Mesa-dev] [PATCH 08/10] i965: implement DRIImage::createImageFromRenderbuffer2

[Emil Velikov]

On 17 October 2017 at 11:29, Eric Engestrom <eric.engestrom at imgtec.com> wrote:
> On Monday, 2017-10-16 16:04:10 +0000, Emil Velikov wrote:
>> From: Emil Velikov <emil.velikov at collabora.com>
>>
>> The new entry point has a way to feedback the error. Thus we no longer
>> need to call _mesa_error() but instead we can pass the correct value.
>>
>> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
>> ---
>>  src/mesa/drivers/dri/i965/intel_screen.c | 31 ++++++++++++++++++++++++++-----
>>  1 file changed, 26 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c
>> index ea04a72e860..7cbb5e3b060 100644
>> --- a/src/mesa/drivers/dri/i965/intel_screen.c
>> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
>> @@ -474,8 +474,9 @@ intel_create_image_from_name(__DRIscreen *dri_screen,
>>  }
>>
>>  static __DRIimage *
>> -intel_create_image_from_renderbuffer(__DRIcontext *context,
>> -                                  int renderbuffer, void *loaderPrivate)
>> +intel_create_image_from_renderbuffer2(__DRIcontext *context,
>> +                                   int renderbuffer, void *loaderPrivate,
>> +                                   unsigned *error)
>>  {
>>     __DRIimage *image;
>>     struct brw_context *brw = context->driverPrivate;
>> @@ -485,15 +486,17 @@ intel_create_image_from_renderbuffer(__DRIcontext *context,
>>
>>     rb = _mesa_lookup_renderbuffer(ctx, renderbuffer);
>>     if (!rb) {
>> -      _mesa_error(ctx, GL_INVALID_OPERATION, "glRenderbufferExternalMESA");
>> +      *error = __DRI_IMAGE_ERROR_BAD_PARAMETER;
>>        return NULL;
>>     }
>
> [there]
>
>>
>>     irb = intel_renderbuffer(rb);
>>     intel_miptree_make_shareable(brw, irb->mt);
>>     image = calloc(1, sizeof *image);
>> -   if (image == NULL)
>> +   if (image == NULL) {
>> +      *error = __DRI_IMAGE_ERROR_BAD_ALLOC;
>>        return NULL;
>> +   }
>>
>>     image->internal_format = rb->InternalFormat;
>>     image->format = rb->Format;
>> @@ -508,12 +511,29 @@ intel_create_image_from_renderbuffer(__DRIcontext *context,
>>     image->height = rb->Height;
>>     image->pitch = irb->mt->surf.row_pitch;
>>     image->dri_format = driGLFormatToImageFormat(image->format);
>> +   if (image->dri_format == __DRI_IMAGE_FORMAT_NONE) {
>
> __DRI_IMAGE_FORMAT_NONE can only come from MESA_FORMAT_NONE; did you
> mean `== 0`? Or both?
>
Right, I've assumed that __DRI_IMAGE_FORMAT_NONE is the default case
which doesn't seems to be the case ...
The whole __DRI_IMAGE_FORMAT handling seems to need some work, but the
first one would be to fixup driGLFormatToImageFormat()

Patch coming in v2.

>> +      *error = __DRI_IMAGE_ERROR_BAD_PARAMETER;
>> +      brw_bo_unreference(irb->mt->bo);
>> +      free(image);
>> +      return NULL;
>> +   }
>
> You can move this check before the calloc and ref ([there] above), so
> that the error path becomes just setting `error` and returning NULL.
>
> You'll want a temp var to avoid calling driGLFormatToImageFormat()
> twice:
>         uint32_t dri_format = driGLFormatToImageFormat(rb->Format);
>
Thought about it, then decided again it... not sure why. Will fix.

>> +
>>     image->has_depthstencil = irb->mt->stencil_mt? true : false;
>>
>>     rb->NeedsFinishRenderTexture = true;
>> +   *error = __DRI_IMAGE_ERROR_SUCCESS;
>>     return image;
>>  }
>>
>> +static __DRIimage *
>> +intel_create_image_from_renderbuffer(__DRIcontext *context,
>> +                                  int renderbuffer, void *loaderPrivate)
>> +{
>> +   unsigned error;
>> +   return intel_create_image_from_renderbuffer2(context, renderbuffer,
>> +                                                loaderPrivate, &error);
>
>         if (error == __DRI_IMAGE_ERROR_BAD_PARAMETER)
>                 _mesa_error(ctx, GL_INVALID_OPERATION, "glRenderbufferExternalMESA");
>
> (maybe even `if (error != __DRI_IMAGE_ERROR_SUCCESS)`?)
>
The use of _mesa_error() is a bug in the original code. It generates a
GL error, whereas a EGL one is expected.
Thus programs thoroughly checking the error states will be mislead/lied to.

I'll add an explicit note in the commit message about it.

Thanks
Emil