[Mesa-dev] [PATCH 1/6] mesa: wire up InvalidateFramebuffer
Ian Romanick
idr at freedesktop.org
Tue Dec 11 23:40:18 UTC 2018
On 12/11/18 3:15 PM, Rob Clark wrote:
> On Tue, Dec 11, 2018 at 6:06 PM Ian Romanick <idr at freedesktop.org> wrote:
>>
>> On 12/11/18 2:50 PM, Rob Clark wrote:
>>> And before someone actually starts implementing DiscardFramebuffer()
>>> lets rework the interface to something that is actually usable.
>>>
>>> Signed-off-by: Rob Clark <robdclark at gmail.com>
>>> ---
>>> src/mesa/main/dd.h | 5 +--
>>> src/mesa/main/fbobject.c | 79 ++++++++++++++++++++++++++++++++++++++--
>>> 2 files changed, 77 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/src/mesa/main/dd.h b/src/mesa/main/dd.h
>>> index f14c3e04e91..1214eeaa474 100644
>>> --- a/src/mesa/main/dd.h
>>> +++ b/src/mesa/main/dd.h
>>> @@ -784,9 +784,8 @@ struct dd_function_table {
>>> GLint srcX0, GLint srcY0, GLint srcX1, GLint srcY1,
>>> GLint dstX0, GLint dstY0, GLint dstX1, GLint dstY1,
>>> GLbitfield mask, GLenum filter);
>>> - void (*DiscardFramebuffer)(struct gl_context *ctx,
>>> - GLenum target, GLsizei numAttachments,
>>> - const GLenum *attachments);
>>> + void (*DiscardFramebuffer)(struct gl_context *ctx, struct gl_framebuffer *fb,
>>> + struct gl_renderbuffer_attachment *att);
>>>
>>> /**
>>> * \name Functions for GL_ARB_sample_locations
>>> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
>>> index 23e49396199..f931e8f76b1 100644
>>> --- a/src/mesa/main/fbobject.c
>>> +++ b/src/mesa/main/fbobject.c
>>> @@ -4637,6 +4637,67 @@ invalid_enum:
>>> return;
>>> }
>>>
>>> +static struct gl_renderbuffer_attachment *
>>> +get_fb_attachment(struct gl_context *ctx, struct gl_framebuffer *fb,
>>> + const GLenum attachment)
>>> +{
>>> + GLint idx;
>>
>> Nancy Reagan says "Just say no... to silly GL types." I'd also be
>> inclined to move this...
>
> defn agree.. that just seemed to be the 'when in Rome' convention
>
> But I don't mind bucking convention
This used to be the convention, but I think we stopped using GL types
for things that aren't exposed to the application some time ago. I
think Eric (wisely) led that charge. Then it became another one of
those cases where we didn't go back and change everything.
Which reminds me...
>>> +
>>> + switch (attachment) {
>>> + case GL_COLOR:
>>> + case GL_COLOR_ATTACHMENT0_EXT:
>>> + case GL_COLOR_ATTACHMENT1_EXT:
>>> + case GL_COLOR_ATTACHMENT2_EXT:
>>> + case GL_COLOR_ATTACHMENT3_EXT:
>>> + case GL_COLOR_ATTACHMENT4_EXT:
>>> + case GL_COLOR_ATTACHMENT5_EXT:
>>> + case GL_COLOR_ATTACHMENT6_EXT:
>>> + case GL_COLOR_ATTACHMENT7_EXT:
>>> + case GL_COLOR_ATTACHMENT8_EXT:
>>> + case GL_COLOR_ATTACHMENT9_EXT:
>>> + case GL_COLOR_ATTACHMENT10_EXT:
>>> + case GL_COLOR_ATTACHMENT11_EXT:
>>> + case GL_COLOR_ATTACHMENT12_EXT:
>>> + case GL_COLOR_ATTACHMENT13_EXT:
>>> + case GL_COLOR_ATTACHMENT14_EXT:
>>> + case GL_COLOR_ATTACHMENT15_EXT:
s/_EXT// here and elsewhere. :)
>>> + if (attachment == GL_COLOR) {
>>> + idx = 0;
>>> + } else {
>>> + idx = attachment - GL_COLOR_ATTACHMENT0_EXT;
>>> + }
>>
>> ...here and do
>>
>> const unsigned idx = attachment == GL_COLOR ? 0 : attachment - GL_COLOR_ATTACHMENT0_EXT;
>>
>
> mostly just trying to keep it in 80(ish) columns
Yeah... I'll usually break these at the " = " if they get too long. I
don't know if we actually have a proper coding style guideline for this.
I know Matt does something different from what I do. Not sure about
Ken or Jason. *shrug*
>> but that's just my personal preference.
>>
>>> + return &fb->Attachment[BUFFER_COLOR0 + idx];
>>> + case GL_DEPTH:
>>> + case GL_DEPTH_ATTACHMENT_EXT:
>>> + case GL_DEPTH_STENCIL_ATTACHMENT:
>>> + return &fb->Attachment[BUFFER_DEPTH];
>>> + case GL_STENCIL:
>>> + case GL_STENCIL_ATTACHMENT_EXT:
>>> + return &fb->Attachment[BUFFER_STENCIL];
>>> + default:
>>> + return NULL;
>>> + }
>>> +}
>>> +
>>> +static void
>>> +discard_framebuffer(struct gl_context *ctx, struct gl_framebuffer *fb,
>>> + GLsizei numAttachments, const GLenum *attachments)
>>> +{
>>> + GLint i;
>>> +
>>> + if (!ctx->Driver.DiscardFramebuffer)
>>> + return;
>>> +
>>> + for (i = 0; i < numAttachments; i++) {
>>
>> I'd definitely move 'int i' inside the for.
>
> will do
>
>>
>> With at least s/GLint/int/ or similar, this patch is
>>
>> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>
>
> thanks
>
> BR,
> -R
>
>>
>>> + struct gl_renderbuffer_attachment *att =
>>> + get_fb_attachment(ctx, fb, attachments[i]);
>>> +
>>> + if (!att)
>>> + continue;
>>> +
>>> + ctx->Driver.DiscardFramebuffer(ctx, fb, att);
>>> + }
>>> +}
>>>
>>> void GLAPIENTRY
>>> _mesa_InvalidateSubFramebuffer_no_error(GLenum target, GLsizei numAttachments,
>>> @@ -4695,14 +4756,23 @@ _mesa_InvalidateNamedFramebufferSubData(GLuint framebuffer,
>>> invalidate_framebuffer_storage(ctx, fb, numAttachments, attachments,
>>> x, y, width, height,
>>> "glInvalidateNamedFramebufferSubData");
>>> -}
>>>
>>> + discard_sub_framebuffer(ctx, fb, numAttachments, attachments,
>>> + x, y, width, height);
>>> +}
>>>
>>> void GLAPIENTRY
>>> _mesa_InvalidateFramebuffer_no_error(GLenum target, GLsizei numAttachments,
>>> const GLenum *attachments)
>>> {
>>> - /* no-op */
>>> + struct gl_framebuffer *fb;
>>> + GET_CURRENT_CONTEXT(ctx);
>>> +
>>> + fb = get_framebuffer_target(ctx, target);
>>> + if (!fb)
>>> + return;
>>> +
>>> + discard_framebuffer(ctx, fb, numAttachments, attachments);
>>> }
>>>
>>>
>>> @@ -4738,6 +4808,8 @@ _mesa_InvalidateFramebuffer(GLenum target, GLsizei numAttachments,
>>> ctx->Const.MaxViewportWidth,
>>> ctx->Const.MaxViewportHeight,
>>> "glInvalidateFramebuffer");
>>> +
>>> + discard_framebuffer(ctx, fb, numAttachments, attachments);
>>> }
>>>
>>>
>>> @@ -4824,8 +4896,7 @@ _mesa_DiscardFramebufferEXT(GLenum target, GLsizei numAttachments,
>>> }
>>> }
>>>
>>> - if (ctx->Driver.DiscardFramebuffer)
>>> - ctx->Driver.DiscardFramebuffer(ctx, target, numAttachments, attachments);
>>> + discard_framebuffer(ctx, fb, numAttachments, attachments);
>>>
>>> return;
>>>
More information about the mesa-dev
mailing list