[Mesa-dev] [PATCH 01/13] i965/blorp: Refactor to get rid of the get_wm_prog virtual function

[Pohjolainen, Topi]

On Sat, Apr 23, 2016 at 07:32:33AM -0700, Jason Ekstrand wrote:
>    On Apr 23, 2016 3:46 AM, "Pohjolainen, Topi"
>    <[1]topi.pohjolainen at intel.com> wrote:
>    >
>    > On Fri, Apr 22, 2016 at 04:19:08PM -0700, Jason Ekstrand wrote:
>    > > Instead of having a virtual member function for getting the WM/PS
>    kernel,
>    > > we simply add fields for prog_data and the kernel to
>    brw_blorp_parms and
>    > > always make sure those get set as part of the different
>    constructors.
>    > > ---
>    > >  src/mesa/drivers/dri/i965/brw_blorp.cpp       | 12 ++-----
>    > >  src/mesa/drivers/dri/i965/brw_blorp.h         | 19 +++++-----
>    > >  src/mesa/drivers/dri/i965/brw_blorp_blit.cpp  | 12 ++-----
>    > >  src/mesa/drivers/dri/i965/brw_blorp_clear.cpp | 50
>    ++++++++++++---------------
>    > >  src/mesa/drivers/dri/i965/gen6_blorp.cpp      | 25 ++++++--------
>    > >  src/mesa/drivers/dri/i965/gen7_blorp.cpp      | 28 +++++++--------
>    > >  src/mesa/drivers/dri/i965/gen8_blorp.cpp      | 18 ++++------
>    > >  7 files changed, 68 insertions(+), 96 deletions(-)
>    > >
>    > > diff --git a/src/mesa/drivers/dri/i965/brw_blorp.cpp
>    b/src/mesa/drivers/dri/i965/brw_blorp.cpp
>    > > index ce09b09..9dbbd83 100644
>    > > --- a/src/mesa/drivers/dri/i965/brw_blorp.cpp
>    > > +++ b/src/mesa/drivers/dri/i965/brw_blorp.cpp
>    > > @@ -165,10 +165,11 @@ brw_blorp_params::brw_blorp_params(unsigned
>    num_varyings,
>    > >       depth_format(0),
>    > >       hiz_op(GEN6_HIZ_OP_NONE),
>    > >       fast_clear_op(0),
>    > > -     use_wm_prog(false),
>    > >       num_varyings(num_varyings),
>    > >       num_draw_buffers(num_draw_buffers),
>    > > -     num_layers(num_layers)
>    > > +     num_layers(num_layers),
>    > > +     wm_prog_kernel(BRW_BLORP_NO_WM_PROG),
>    > > +     wm_prog_data(NULL)
>    > >  {
>    > >     color_write_disable[0] = false;
>    > >     color_write_disable[1] = false;
>    > > @@ -354,10 +355,3 @@ brw_hiz_op_params::brw_hiz_op_params(struct
>    intel_mipmap_tree *mt,
>    > >     default:                    unreachable("not reached");
>    > >     }
>    > >  }
>    > > -
>    > > -uint32_t
>    > > -brw_hiz_op_params::get_wm_prog(struct brw_context *brw,
>    > > -                               brw_blorp_prog_data **prog_data)
>    const
>    > > -{
>    > > -   return 0;
>    > > -}
>    > > diff --git a/src/mesa/drivers/dri/i965/brw_blorp.h
>    b/src/mesa/drivers/dri/i965/brw_blorp.h
>    > > index 79dc59a..4981afd 100644
>    > > --- a/src/mesa/drivers/dri/i965/brw_blorp.h
>    > > +++ b/src/mesa/drivers/dri/i965/brw_blorp.h
>    > > @@ -229,6 +229,7 @@ struct brw_blorp_prog_data
>    > >     bool persample_msaa_dispatch;
>    > >  };
>    > >
>    > > +#define BRW_BLORP_NO_WM_PROG 1
>    >
>    > So in other words any offset other than one is regarded as valid? I
>    was
>    > wondering if could drop this and use the existence of wm_prog_data to
>    tell
>    > if there is kernel available or not. At least in current form valid
>    kernel
>    > always has prog_data and vice versa.
> 
>    I thought about that and would be happy to make the change If you
>    wanted.  I just couldn't decide which I thought was cleaner.

If you don't mind. It is in the end matter of taste but somehow I find it
cleaner. You can slab there:

Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>

I'll go through the rest next.