[Mesa-dev] [PATCH 1/6] mesa: wire up InvalidateFramebuffer

[Rob Clark]

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

>
> > +
> > +   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:
> > +      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

> 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;
> >
> >
>