[Mesa-dev] [PATCH 10/13] mesa: add KHR_no_error support for FramebufferTextureLayer

[Timothy Arceri]

On 10/05/17 22:00, Nicolai Hähnle wrote:
> On 10.05.2017 02:28, Eric Anholt wrote:
>> Timothy Arceri <tarceri at itsqueeze.com> writes:
>>
>>> ---
>>>  src/mapi/glapi/gen/ARB_framebuffer_object.xml |  2 +-
>>>  src/mesa/main/fbobject.c                      | 31 
>>> +++++++++++++++++++++++++++
>>>  src/mesa/main/fbobject.h                      |  4 ++++
>>>  3 files changed, 36 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/mapi/glapi/gen/ARB_framebuffer_object.xml 
>>> b/src/mapi/glapi/gen/ARB_framebuffer_object.xml
>>> index ce5e45d..76114eb 100644
>>> --- a/src/mapi/glapi/gen/ARB_framebuffer_object.xml
>>> +++ b/src/mapi/glapi/gen/ARB_framebuffer_object.xml
>>> @@ -239,21 +239,21 @@
>>>      <function name="FramebufferTexture3D" no_error="true">
>>>          <param name="target" type="GLenum"/>
>>>          <param name="attachment" type="GLenum"/>
>>>          <param name="textarget" type="GLenum"/>
>>>          <param name="texture" type="GLuint"/>
>>>          <param name="level" type="GLint"/>
>>>          <param name="layer" type="GLint"/>
>>>      <glx rop="4323"/>
>>>      </function>
>>>
>>> -    <function name="FramebufferTextureLayer" es2="3.0">
>>> +    <function name="FramebufferTextureLayer" es2="3.0" no_error="true">
>>>          <param name="target" type="GLenum"/>
>>>          <param name="attachment" type="GLenum"/>
>>>          <param name="texture" type="GLuint"/>
>>>          <param name="level" type="GLint"/>
>>>          <param name="layer" type="GLint"/>
>>>      <glx rop="237"/>
>>>      </function>
>>>
>>>      <function name="FramebufferRenderbuffer" es2="2.0">
>>>          <param name="target" type="GLenum"/>
>>> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
>>> index ba01d0c..08e7347 100644
>>> --- a/src/mesa/main/fbobject.c
>>> +++ b/src/mesa/main/fbobject.c
>>> @@ -3424,20 +3424,51 @@ void GLAPIENTRY
>>>  _mesa_FramebufferTexture3D(GLenum target, GLenum attachment,
>>>                             GLenum textarget, GLuint texture,
>>>                             GLint level, GLint layer)
>>>  {
>>>     framebuffer_texture_with_dims(3, target, attachment, textarget, 
>>> texture,
>>>                                   level, layer, 
>>> "glFramebufferTexture3D");
>>>  }
>>>
>>>
>>>  void GLAPIENTRY
>>> +_mesa_FramebufferTextureLayer_no_error(GLenum target, GLenum 
>>> attachment,
>>> +                                       GLuint texture, GLint level,
>>> +                                       GLint layer)
>>> +{
>>> +   GET_CURRENT_CONTEXT(ctx);
>>> +
>>> +   /* Get the framebuffer object */
>>> +   struct gl_framebuffer *fb = get_framebuffer_target(ctx, target);
>>> +
>>> +   /* Get the texture object */
>>> +   struct gl_texture_object *texObj =
>>> +      get_texture_for_framebuffer(ctx, texture);
>>> +
>>> +   struct gl_renderbuffer_attachment *att =
>>> +      get_attachment(ctx, fb, attachment, NULL);
>>> +
>>> +   GLenum textarget = 0;
>>> +   if (texObj) {
>>> +      if (texObj->Target == GL_TEXTURE_CUBE_MAP) {
>>> +         assert(layer >= 0 && layer < 6);
>>> +         textarget = GL_TEXTURE_CUBE_MAP_POSITIVE_X + layer;
>>> +         layer = 0;
>>> +      }
>>> +   }
>>> +
>>> +   _mesa_framebuffer_texture(ctx, fb, attachment, att, texObj, 
>>> textarget,
>>> +                             level, layer, GL_FALSE);
>>> +}
>>
>> I've got stuck on reading this series a couple of times because this
>> felt like too much code duplication.  I understand that you're
>> replicating what the current error-validating code does, but it seems
>> like for no_error we can do a lot better, quite easily.
>>
>> What I'd like for these last 4 patches is to have two patches, each of
>> which implements a Named and non-Named no_error function by doing the
>> appropriate lookup of the fb and then calling a helper function that
>> does the rest of their current bodies.
> 
> Not to be too annoying, but I still think that collapsing the functions 
> entirely (and using ALWAYS_INLINE) is a good idea as well.

Yeah I've done that with the Tex*SubImage fuctions in an upcoming series 
and I think I've changed my mind. I still think the extra complexity is 
a little ugly, but I think its worth it.

I'll redo these patches to share the inline function between the dsa/no 
error variants also.

> 
> Cheers,
> Nicolai
> 
>>
>> Assuming patch 4.5 is to be squashed into 4, then other than the ';'
>> comment patches 1-9 are:
>>
>> Reviewed-by: Eric Anholt <eric at anholt.net>
>>
>>
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
> 
>