[Mesa-dev] [PATCH] i965: Map GL_PATCHES to 3DPRIM_PATCHLIST_n.

[Kenneth Graunke]

On Tuesday, November 10, 2015 08:41:22 AM Ian Romanick wrote:
> On 11/10/2015 01:19 AM, Kenneth Graunke wrote:
> > Inspired by a patch by Fabian Bieler.
> > 
> > Fabian defined a _3DPRIM_PATCHLIST_0 macro (which isn't actually a valid
> > topology type); I instead chose to make a macro that takes an argument.
> > He also took the number of patch vertices from _mesa_prim (which was set
> > to ctx->TessCtrlProgram.patch_vertices) - I chose to use it directly to
> > avoid the need for the VBO patch.
> > 
> > Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> > ---
> >  src/mesa/drivers/dri/i965/brw_defines.h | 2 ++
> >  src/mesa/drivers/dri/i965/brw_draw.c    | 9 ++++++++-
> >  2 files changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_defines.h b/src/mesa/drivers/dri/i965/brw_defines.h
> > index eb91634..6dfaf8f 100644
> > --- a/src/mesa/drivers/dri/i965/brw_defines.h
> > +++ b/src/mesa/drivers/dri/i965/brw_defines.h
> > @@ -78,6 +78,8 @@
> >  #define _3DPRIM_LINESTRIP_BF      0x13
> >  #define _3DPRIM_LINESTRIP_CONT_BF 0x14
> >  #define _3DPRIM_TRIFAN_NOSTIPPLE  0x16
> > +#define _3DPRIM_PATCHLIST(n) ({ assert(n > 0 && n <= 32); 0x1F + n; })
> > +
> 
> Nice assert. :)
> 
> The docs list 3DPRIM_PATCHLIST_1 as 0x20.  It may be trivially better to
> use 0x20 + (n - 1) so that the value in the code matches the value in
> the docs.

Good idea.  I've made that change locally.

> >  /* We use this offset to be able to pass native primitive types in struct
> >   * _mesa_prim::mode.  Native primitive types are BRW_PRIM_OFFSET +
> > diff --git a/src/mesa/drivers/dri/i965/brw_draw.c b/src/mesa/drivers/dri/i965/brw_draw.c
> > index 39a26b0..bff484f 100644
> > --- a/src/mesa/drivers/dri/i965/brw_draw.c
> > +++ b/src/mesa/drivers/dri/i965/brw_draw.c
> > @@ -140,9 +140,16 @@ brw_set_prim(struct brw_context *brw, const struct _mesa_prim *prim)
> >  static void
> >  gen6_set_prim(struct brw_context *brw, const struct _mesa_prim *prim)
> >  {
> > +   const struct gl_context *ctx = &brw->ctx;
> > +   uint32_t hw_prim;
> > +
> >     DBG("PRIM: %s\n", _mesa_enum_to_string(prim->mode));
> >  
> > -   const uint32_t hw_prim = get_hw_prim_for_gl_prim(prim->mode);
> > +   if (prim->mode == GL_PATCHES)
> > +      hw_prim = _3DPRIM_PATCHLIST(ctx->TessCtrlProgram.patch_vertices);
> > +   else
> > +      hw_prim = get_hw_prim_for_gl_prim(prim->mode);
> > +
> 
> I'd be tempted to use ?: so that hw_prim could continue to be const, but
> it's not a biggie.

I do usually like using ?: and marking things const, but I thought the
above formatting was a bit less jumbled than:

   const uint32_t hw_prim = prim->mode == GL_PATCHES ?
      _3DPRIM_PATCHLIST(ctx->TessCtrlProgram.patch_vertices) :
      get_hw_prim_for_gl_prim(prim->mode);

Since the function is only ~10 lines of code, I think I'll eschew
the const and leave it as above, since neither of us feel too strongly
about it.

> With or without either of these changes, this patch is
> 
> Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

Thanks, Ian!

> >     if (hw_prim != brw->primitive) {
> >        brw->primitive = hw_prim;
> >        brw->ctx.NewDriverState |= BRW_NEW_PRIMITIVE;
> > 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151110/a40213de/attachment.sig>