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.pohjolai...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
