[Mesa-dev] [PATCH 02/15] i965: Configure how to store *64*PASSTHRU vertex components

[Kenneth Graunke]

On Wednesday, May 4, 2016 1:01:33 PM PDT Antía Puentes wrote:
> Hi Kenneth,
> 
> thanks for reviewing.
> 
> On mié, 2016-05-04 at 03:36 -0700, Kenneth Graunke wrote:
> > On Thursday, April 28, 2016 1:40:32 PM PDT Antia Puentes wrote:
> > > 
> > > From the Broadwell specification, structure VERTEX_ELEMENT_STATE
> > > description:
> > > 
> > >    "When SourceElementFormat is set to one of the *64*_PASSTHRU
> > >     formats,  64-bit components are stored in the URB without any
> > >     conversion. In this case, vertex elements must be written as 128
> > >     or 256 bits, with VFCOMP_STORE_0 being used to pad the output
> > >     as required. E.g., if R64_PASSTHRU is used to copy a 64-bit Red 
> > component into
> > > 
> > >     the URB, Component 1 must be specified as VFCOMP_STORE_0 (with
> > >     Components 2,3 set to VFCOMP_NOSTORE) in order to output a 128-bit
> > >     vertex element, or Components 1-3 must be specified as 
VFCOMP_STORE_0
> > >     in order to output a 256-bit vertex element. Likewise, use of
> > >     R64G64B64_PASSTHRU requires Component 3 to be specified as 
> > VFCOMP_STORE_0
> > > 
> > >     in order to output a 256-bit vertex element."
> > > 
> > > Uses 128-bits to write double and dvec2 vertex elements, and 256-bits 
for
> > > dvec3 and dvec4 vertex elements.
> > > 
> > > Signed-off-by: Juan A. Suarez Romero <jasuarez at igalia.com>
> > > Signed-off-by: Antia Puentes <apuentes at igalia.com>
> > > ---
> > >  src/mesa/drivers/dri/i965/gen8_draw_upload.c | 34 +++++++++++++++++++++
++++
> > +++
> > > 
> > >  1 file changed, 34 insertions(+)
> > > 
> > > diff --git a/src/mesa/drivers/dri/i965/gen8_draw_upload.c b/src/mesa/
> > drivers/dri/i965/gen8_draw_upload.c
> > > 
> > > index fe5ed35..14f7091 100644
> > > --- a/src/mesa/drivers/dri/i965/gen8_draw_upload.c
> > > +++ b/src/mesa/drivers/dri/i965/gen8_draw_upload.c
> > > @@ -217,6 +217,40 @@ gen8_emit_vertices(struct brw_context *brw)
> > >           break;
> > >        }
> > >  
> > > +      /* From the BDW PRM, Volume 2d, page 586 (VERTEX_ELEMENT_STATE):
> > > +       *
> > > +       *     "When SourceElementFormat is set to one of the 
*64*_PASSTHRU
> > > +       *     formats, 64-bit components are stored in the URB without 
any
> > > +       *     conversion. In this case, vertex elements must be written 
as 
> > 128
> > > 
> > > +       *     or 256 bits, with VFCOMP_STORE_0 being used to pad the 
output
> > > +       *     as required. E.g., if R64_PASSTHRU is used to copy a 64-
bit 
> > Red
> > > 
> > > +       *     component into the URB, Component 1 must be specified as
> > > +       *     VFCOMP_STORE_0 (with Components 2,3 set to VFCOMP_NOSTORE)
> > > +       *     in order to output a 128-bit vertex element, or Components 
1-3 
> > must
> > > 
> > > +       *     be specified as VFCOMP_STORE_0 in order to output a 256-
bit 
> > vertex
> > > 
> > > +       *     element. Likewise, use of R64G64B64_PASSTHRU requires 
> > Component 3
> > > 
> > > +       *     to be specified as VFCOMP_STORE_0 in order to output a 
256-bit 
> > vertex
> > > 
> > > +       *     element."
> > > +       */
> > The above comment seems to indicate that R64 needs component 1 set to
> > STORE_0, and that looks to be missing.  I'm guessing you need to add:
> > 
> > > 
> > > +      if (input->glarray->Doubles) {
> > > +         switch (input->glarray->Size) {
> > > +         case 0:
> >                comp0 = BRW_VE1_COMPONENT_STORE_0;
> >                /* fallthrough */
> > 
> > > 
> > > +         case 1:
> >                comp1 = BRW_VE1_COMPONENT_STORE_0;
> >                /* fallthrough */
> > 
> 
> I have not added them because comp0 and comp1 are initialized in the
> code immediately above the switch I added. That already existing code
> sets the default values for the components:
> 
> switch (input->glarray->Size) {
>       case 0: comp0 = BRW_VE1_COMPONENT_STORE_0;
>       case 1: comp1 = BRW_VE1_COMPONENT_STORE_0;
>       case 2: comp2 = BRW_VE1_COMPONENT_STORE_0;
>       case 3: comp3 = input->glarray->Integer ?
>                        BRW_VE1_COMPONENT_STORE_1_INT
>                      : BRW_VE1_COMPONENT_STORE_1_FLT;
>          break;
>       }
> 
> In my switch I just override the values that are not right for the
> double-precision floating point cases.

Right, sorry :)  That seems totally fine then.

Patches 1-3 are:
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
-------------- 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: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160504/ee46ad5d/attachment-0001.sig>