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

[Ben Widawsky]

On Tue, Oct 20, 2015 at 02:52:29PM -0700, Matt Turner wrote:
> On Tue, Oct 20, 2015 at 2:29 PM, Ben Widawsky
> <benjamin.widawsky at intel.com> wrote:
> > 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:
> > -    *
> > -    * 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 */
> 
> Do we gain anything by assigning values explicitly?

Just code readability. As a noob coming into the code, seeing a random "6" or
"4" in places was strange and it took a bit to figure out where to get the
sensible value from.

Is there any specific opposition toward doing this, or some reason it wasn't
done in the first place? I honestly don't care too much...