[Mesa-dev] [PATCH 2/6] i965/fs: Enumerate logical fb writes arguments

[Francisco Jerez]

Ben Widawsky <benjamin.widawsky at intel.com> writes:

> Gen9 adds the ability to write out a stencil value, so we need to expand the
> virtual payload by one. Abstracting this now makes that change easier to read.
>
> I was admittedly confused early on about some of the hardcoding. If people
> believe the resulting code is inferior, I am not super attached to the patch.
>
> Cc: Francisco Jerez <currojerez at riseup.net>
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> ---
>  src/mesa/drivers/dri/i965/brw_defines.h | 18 ++++++++++--------
>  src/mesa/drivers/dri/i965/brw_fs.cpp    | 21 +++++++++++----------
>  2 files changed, 21 insertions(+), 18 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_defines.h b/src/mesa/drivers/dri/i965/brw_defines.h
> index 7a5ee1b..e06c9d6 100644
> --- a/src/mesa/drivers/dri/i965/brw_defines.h
> +++ b/src/mesa/drivers/dri/i965/brw_defines.h
> @@ -912,14 +912,6 @@ enum opcode {
>     /**
>      * Same as FS_OPCODE_FB_WRITE but expects its arguments separately as
>      * individual sources instead of as a single payload blob:

Maybe replace the colon with a full stop and add a reference to the
right enumeration in the doxygen comment.

> -    *
> -    * Source 0: [required] Color 0.
> -    * Source 1: [optional] Color 1 (for dual source blend messages).
> -    * Source 2: [optional] Src0 Alpha.
> -    * Source 3: [optional] Source Depth (gl_FragDepth)
> -    * Source 4: [optional (gen4-5)] Destination Depth passthrough from thread
> -    * Source 5: [optional] Sample Mask (gl_SampleMask).
> -    * Source 6: [required] Number of color components (as a UD immediate).
>      */
>     FS_OPCODE_FB_WRITE_LOGICAL,
>  
> @@ -1318,6 +1310,16 @@ enum brw_urb_write_flags {
>        BRW_URB_WRITE_ALLOCATE | BRW_URB_WRITE_COMPLETE,
>  };
>  
> +enum fb_write_logical_args {
> +   FB_WRITE_COLOR0 = 0,      /* REQUIRED */
> +   FB_WRITE_COLOR1 = 1,      /* for dual source blend messages */
> +   FB_WRITE_SRC0_ALPHA = 2,
> +   FB_WRITE_SRC_DEPTH = 3,   /* gl_FragDepth */
> +   FB_WRITE_DST_DEPTH = 4,   /* GEN4-5: passthrough from thread */
> +   FB_WRITE_OMASK = 5,       /* Sample Mask (gl_SampleMask) */
> +   FB_WRITE_COMPONENTS = 6,  /* REQUIRED */
> +};
> +

AFAIK this is the first time that we're defining symbolic names for the
sources of an instruction, so I guess it would make sense to come up
with some naming convention early on.  How about:

| enum <opcode name>_srcs {
|    <OPCODE NAME>_SRC_<SOURCE NAME>,
|    ...
| }

In this case the opcode name should probably include the "logical"
suffix, because there's also a plain FB_WRITE opcode that takes a
different set of arguments.

Other than the naming nit-picking the change looks correct and seems
like an improvement.

>  #ifdef __cplusplus
>  /**
>   * Allow brw_urb_write_flags enums to be ORed together.
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
> index 49323eb..e2e3761 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> @@ -695,10 +695,10 @@ fs_inst::components_read(unsigned i) const
>        return 2;
>  
>     case FS_OPCODE_FB_WRITE_LOGICAL:
> -      assert(src[6].file == IMM);
> +      assert(src[FB_WRITE_COMPONENTS].file == IMM);
>        /* First/second FB write color. */
>        if (i < 2)
> -         return src[6].fixed_hw_reg.dw1.ud;
> +         return src[FB_WRITE_COMPONENTS].fixed_hw_reg.dw1.ud;
>        else
>           return 1;
>  
> @@ -3337,15 +3337,16 @@ lower_fb_write_logical_send(const fs_builder &bld, fs_inst *inst,
>                              const brw_wm_prog_key *key,
>                              const fs_visitor::thread_payload &payload)
>  {
> -   assert(inst->src[6].file == IMM);
> +   assert(inst->src[FB_WRITE_COMPONENTS].file == IMM);
>     const brw_device_info *devinfo = bld.shader->devinfo;
> -   const fs_reg &color0 = inst->src[0];
> -   const fs_reg &color1 = inst->src[1];
> -   const fs_reg &src0_alpha = inst->src[2];
> -   const fs_reg &src_depth = inst->src[3];
> -   const fs_reg &dst_depth = inst->src[4];
> -   fs_reg sample_mask = inst->src[5];
> -   const unsigned components = inst->src[6].fixed_hw_reg.dw1.ud;
> +   const fs_reg &color0 = inst->src[FB_WRITE_COLOR0];
> +   const fs_reg &color1 = inst->src[FB_WRITE_COLOR1];
> +   const fs_reg &src0_alpha = inst->src[FB_WRITE_SRC0_ALPHA];
> +   const fs_reg &src_depth = inst->src[FB_WRITE_SRC_DEPTH];
> +   const fs_reg &dst_depth = inst->src[FB_WRITE_DST_DEPTH];
> +   fs_reg sample_mask = inst->src[FB_WRITE_OMASK];
> +   const unsigned components =
> +      inst->src[FB_WRITE_COMPONENTS].fixed_hw_reg.dw1.ud;
>  
>     /* We can potentially have a message length of up to 15, so we have to set
>      * base_mrf to either 0 or 1 in order to fit in m0..m15.
> -- 
> 2.6.1
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151021/b5f335c1/attachment.sig>