On Tue, Oct 20, 2015 at 03:19:48PM -0700, Matt Turner wrote:
> On Tue, Oct 20, 2015 at 3:11 PM, Ben Widawsky <b...@bwidawsk.net> wrote:
> > On Tue, Oct 20, 2015 at 02:57:24PM -0700, Matt Turner wrote:
> >> On Tue, Oct 20, 2015 at 2:54 PM, Ben Widawsky <b...@bwidawsk.net> wrote:
> >> > 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.widaw...@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 <curroje...@riseup.net>
> >> >> > Signed-off-by: Ben Widawsky <b...@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...
> >>
> >> If everything just uses the new enum values (and their values don't
> >> matter per se), we shouldn't assign them specifically. Patch 4/6 would
> >> be simpler if you didn't have to renumber some of the enums, for
> >> instance.
> >
> > Yes, I suppose patch 4/6 does end up without the first hunk in the patch if
> > I
> > did away with this, but I still think the readability gained outweighs that.
> > However, I admit my knowledge in this part of the codebase is likely the
> > minority (in between 0 and expert).
>
> What I should have said in the previous email is that assigning
> arbitrary numbers to enums in brw_defines.h is confusing because one
> might be led to believe that these are hardware values (like almost
> everything else is in brw_defines.h).
Oh. I see what you mean. Yeah, I can drop the explicit numbering. You're cool
with the enumeration though?
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
