[Mesa-dev] ipers performance regression (Was: u_upload_mgr changes)

[Jose Fonseca]

On 04/01/16 12:25, Jose Fonseca wrote:
> On 21/12/15 22:35, Marek Olšák wrote:
>> Hi,
>>
>> This patch series adds more flexibility to u_upload_mgr. First, it
>> adds the ability to specify the alignment per suballocation. The idea
>> is that several users can use the same upload buffer, but each may
>> need a different alignment. Finally, it allows specifying PIPE_USAGE,
>> which usually affects memory placement.
>>
>> Please review.
>>
>> Marek
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>
> Hi Marek,
>
> This patch series, or the commit
>
>
> http://cgit.freedesktop.org/mesa/mesa/commit/?id=36c93a6fae275614b6004ec5ab085774d527e1bc

I bisected and it turns out that 
36c93a6fae275614b6004ec5ab085774d527e1bc is the bad commit, and not the 
u_upload_mgr series.

In particular the issue steams from the hunk:
+
+   /* We uploaded modified constants, need to invalidate them. */
+   st->dirty.mesa |= _NEW_PROGRAM_CONSTANTS;

I suspect that drawing the text via glBitmap suddently became a huge 
hotspot becuase of this.  If one disables the help text (pressing h) 
there's no performance regression (frame rate is the same).



The need for the constants is explained above:

   /* As an optimization, Mesa's fragment programs will sometimes get the
       * primary color from a statevar/constant rather than a varying 
variable.
       * when that's the case, we need to ensure that we use the 'color'
       * parameter and not the current attribute color (which may have 
changed
       * through glRasterPos and state validation.
       * So, we force the proper color here.  Not elegant, but it works.
       */
      {
         GLfloat colorSave[4];
         COPY_4V(colorSave, ctx->Current.Attrib[VERT_ATTRIB_COLOR0]);
         COPY_4V(ctx->Current.Attrib[VERT_ATTRIB_COLOR0], color);
         st_upload_constants(st, st->fp->Base.Base.Parameters,
                             PIPE_SHADER_FRAGMENT);
         COPY_4V(ctx->Current.Attrib[VERT_ATTRIB_COLOR0], colorSave);
      }

I wonder if putting colors in constants as opposed to vertex buffer with 
zero stride or an INT_MAX instancing divisor is really a good idea.


Anyway, if this only affects llvmpipe, no biggie, but if this change 
makes text rendering via glBitmap much slower for all HW drivers, then 
we should probably revisit this.


Jose