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

[Ian Romanick]

On 04/29/2016 11:22 AM, Antía Puentes wrote:
> On jue, 2016-04-28 at 15:16 +0200, Ian Romanick wrote:
>> On 04/28/2016 01:40 PM, 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."
>>> +       */
>>> +      if (input->glarray->Doubles) {
>>> +         switch (input->glarray->Size) {
>>> +         case 0:
>>> +         case 1:
>>> +         case 2:
>>> +            /*  Use 128-bits instead of 256-bits to write double
>>> and dvec2
>>> +             *  vertex elements.
>>> +             */
>>> +            comp2 = BRW_VE1_COMPONENT_NOSTORE;
>>> +            comp3 = BRW_VE1_COMPONENT_NOSTORE;
>>> +            break;
>>> +         case 3:
>>> +            /* Pad the output using VFCOMP_STORE_0 as suggested
>>> +             * by the BDW PRM.
>>> +             */
>>> +            comp3 = BRW_VE1_COMPONENT_STORE_0;
>>
>> I think this would be better as
>>
>>       if (input->glarray->Size < 3) {
>>          ...
>>       } else {
>>          ...
>>       }
> 
> It should be then:
> 
> 	if (input->glarray->Size < 3) {
> 		...
> 	} else if (input->glarray->Size == 3) {
> 		...
> 	}
> 
> because input->glarray->Size can be 4.

Ah, of course.  A switch is fine in that case.

>> At the very least... there should be a "break" at the end of the 3
>> case.
>>  It's not strictly necessary, but my eye keeps noticing that it's not
>> there.
>>
>>> +         }
>>> +      }
>>> +
>>>        OUT_BATCH((input->buffer << GEN6_VE0_INDEX_SHIFT) |
>>>                  GEN6_VE0_VALID |
>>>                  (format << BRW_VE0_FORMAT_SHIFT) |
>>>
>>
>>
>