[Mesa-dev] [PATCH 03/13] i965/blorp: Remove the clear params classes

[Pohjolainen, Topi]

On Sat, Apr 23, 2016 at 04:28:33PM -0700, Jason Ekstrand wrote:
>    On Sat, Apr 23, 2016 at 8:35 AM, Pohjolainen, Topi
>    <[1]topi.pohjolainen at intel.com> wrote:
> 
>    On Fri, Apr 22, 2016 at 04:19:10PM -0700, Jason Ekstrand wrote:
>    > They didn't really add anything other than a key and extra layers of
>    > function calls.  This commit just inlines the extra functions and
>    gets rid
>    > of the extra classes.
>    > ---
>    >  src/mesa/drivers/dri/i965/brw_blorp_clear.cpp | 224
>    ++++++++++----------------
>    >  1 file changed, 87 insertions(+), 137 deletions(-)
>    >
>    > diff --git a/src/mesa/drivers/dri/i965/brw_blorp_clear.cpp
>    b/src/mesa/drivers/dri/i965/brw_blorp_clear.cpp
>    > index c7e57bc..b16102c 100644
>    > --- a/src/mesa/drivers/dri/i965/brw_blorp_clear.cpp
>    > +++ b/src/mesa/drivers/dri/i965/brw_blorp_clear.cpp
>    > @@ -44,43 +44,6 @@ struct brw_blorp_const_color_prog_key
>    >     bool pad[3];
>    >  };
>    >
>    > -/**
>    > - * Parameters for a blorp operation where the fragment shader
>    outputs a
>    > - * constant color.  This is used for both fast color clears and
>    color
>    > - * resolves.
>    > - */
>    > -class brw_blorp_const_color_params : public brw_blorp_params
>    > -{
>    > -public:
>    > -   brw_blorp_const_color_prog_key wm_prog_key;
>    > -};
>    > -
>    > -class brw_blorp_clear_params : public brw_blorp_const_color_params
>    > -{
>    > -public:
>    > -   brw_blorp_clear_params(struct brw_context *brw,
>    > -                          struct gl_framebuffer *fb,
>    > -                          struct gl_renderbuffer *rb,
>    > -                          GLubyte *color_mask,
>    > -                          bool partial_clear,
>    > -                          bool encode_srgb,
>    > -                          unsigned layer);
>    > -};
>    > -
>    > -
>    > -/**
>    > - * Parameters for a blorp operation that performs a "render target
>    resolve".
>    > - * This is used to resolve pending fast clear pixels before a color
>    buffer is
>    > - * used for texturing, ReadPixels, or scanout.
>    > - */
>    > -class brw_blorp_rt_resolve_params : public
>    brw_blorp_const_color_params
>    > -{
>    > -public:
>    > -   brw_blorp_rt_resolve_params(struct brw_context *brw,
>    > -                               struct intel_mipmap_tree *mt);
>    > -};
>    > -
>    > -
>    >  class brw_blorp_const_color_program
>    >  {
>    >  public:
>    > @@ -151,102 +114,6 @@ brw_blorp_params_get_clear_kernel(struct
>    brw_context *brw,
>    >     }
>    >  }
>    >
>    > -brw_blorp_clear_params::brw_blorp_clear_params(struct brw_context
>    *brw,
>    > -                                               struct gl_framebuffer
>    *fb,
>    > -                                               struct
>    gl_renderbuffer *rb,
>    > -                                               GLubyte *color_mask,
>    > -                                               bool partial_clear,
>    > -                                               bool encode_srgb,
>    > -                                               unsigned layer)
> 
>      I would have kept this similarly you changed the default
>      constructors to
>      init() functions (do_single_blorp_clear() becomes way over 100 lines
>      long).
>      Maybe
>      static void
>      set_clear_params(struct brw_context *brw,
>                       struct gl_framebuffer *fb,
>                       struct gl_renderbuffer *rb,
>                       bool partial_clear,
>                       bool encode_srgb,
>                       unsigned layer)
>      {
>         const GLubyte *color_mask = brw->ctx.Color.ColorMask[buf];
>      But I don't care too much. I suppose you have further plans for this
>      anyway.
> 
>    Right.  My thinking was mostly that we had a bunch of logic for
>    handling different cases and half of it was in the constructor and half
>    was in do_single_blorp_clear and there didn't seem to be a particularly
>    good reason why any given piece was in one or the other.  So I figured
>    it't be easier to see what was going on if they were all together.  It
>    may be worth splitting it back out again in the future.
>    Also, one of the things I hope to do eventually is to start using ISL
>    internally in blorp.  If we do this, we'll have to divide the logic
>    back up but the lines will be drawn differently.
>    If you want it kept split up, I can do that but that's my reasoning for
>    just cramming it together.

This is fine by me, like I suspected you have further plans for this anyway.