[Mesa-dev] [PATCH] mesa: only update framebuffer-state for clears

[Erik Faye-Lund]

On on., sep. 19, 2018 at 1:04 PM, Jakob Bornecrantz 
<wallbraker at gmail.com> wrote:
> This patch regresses about 3000 dEQP [2,3,3.1] tests on virgl. Full
> setup is dEQP running on virgl with vtest that is running RadeonSI. So
> QEMU is not in the picture. All instances of the Mesa driver is from
> the same tree and have the patch applied.
> 
> Cheers, Jakob.

Hmm, that's a bit odd IMO. Perhaps a full flush is needed when clears 
are implemented using the graphics-pipeline or something? But I don't 
think virgl does this, so hmmm...

I guess I should revert, and try to respin with some more testing...

> On Wed, Sep 12, 2018 at 12:05 PM Erik Faye-Lund
> <erik.faye-lund at collabora.com> wrote:
>> 
>>  If we update the program-state etc, we risk compiling needless 
>> shaders,
>>  which can cost quite a bit of performance.
>> 
>>  Signed-off-by: Erik Faye-Lund <erik.faye-lund at collabora.com>
>>  ---
>>  This was motivated by seeing an unexpected shader-compile with
>>  nonsensical state on start-up in glxgears.
>> 
>>   src/mesa/main/clear.c | 34 ++++++++++++++++++++--------------
>>   1 file changed, 20 insertions(+), 14 deletions(-)
>> 
>>  diff --git a/src/mesa/main/clear.c b/src/mesa/main/clear.c
>>  index 6beff9ed84..3d2ca490c9 100644
>>  --- a/src/mesa/main/clear.c
>>  +++ b/src/mesa/main/clear.c
>>  @@ -35,6 +35,7 @@
>>   #include "context.h"
>>   #include "enums.h"
>>   #include "fbobject.h"
>>  +#include "framebuffer.h"
>>   #include "get.h"
>>   #include "macros.h"
>>   #include "mtypes.h"
>>  @@ -135,10 +136,10 @@ color_buffer_writes_enabled(const struct 
>> gl_context *ctx, unsigned idx)
>>    * \param mask bit-mask indicating the buffers to be cleared.
>>    *
>>    * Flushes the vertices and verifies the parameter.
>>  - * If __struct gl_contextRec::NewState is set then calls 
>> _mesa_update_state()
>>  - * to update gl_frame_buffer::_Xmin, etc.  If the rasterization 
>> mode is set to
>>  - * GL_RENDER then requests the driver to clear the buffers, via the
>>  - * dd_function_table::Clear callback.
>>  + * If __struct gl_contextRec::NewState contains _NEW_BUFFERS then 
>> calls
>>  + * _mesa_update_framebuffer() to update gl_frame_buffer::_Xmin, 
>> etc. If the
>>  + * rasterization mode is set to GL_RENDER then requests the driver 
>> to clear
>>  + * the buffers, via the dd_function_table::Clear callback.
>>    */
>>   static ALWAYS_INLINE void
>>   clear(struct gl_context *ctx, GLbitfield mask, bool no_error)
>>  @@ -165,8 +166,9 @@ clear(struct gl_context *ctx, GLbitfield mask, 
>> bool no_error)
>>         }
>>      }
>> 
>>  -   if (ctx->NewState) {
>>  -      _mesa_update_state( ctx );       /* update _Xmin, etc */
>>  +   if (ctx->NewState & _NEW_BUFFERS) {
>>  +      _mesa_update_framebuffer(ctx, ctx->ReadBuffer, 
>> ctx->DrawBuffer); /* update _Xmin, etc */
>>  +      ctx->NewState &= ~_NEW_BUFFERS;
>>      }
>> 
>>      if (!no_error && ctx->DrawBuffer->_Status != 
>> GL_FRAMEBUFFER_COMPLETE_EXT) {
>>  @@ -346,8 +348,9 @@ clear_bufferiv(struct gl_context *ctx, GLenum 
>> buffer, GLint drawbuffer,
>>      FLUSH_VERTICES(ctx, 0);
>>      FLUSH_CURRENT(ctx, 0);
>> 
>>  -   if (ctx->NewState) {
>>  -      _mesa_update_state( ctx );
>>  +   if (ctx->NewState & _NEW_BUFFERS) {
>>  +      _mesa_update_framebuffer(ctx, ctx->ReadBuffer, 
>> ctx->DrawBuffer);
>>  +      ctx->NewState &= ~_NEW_BUFFERS;
>>      }
>> 
>>      switch (buffer) {
>>  @@ -460,8 +463,9 @@ clear_bufferuiv(struct gl_context *ctx, GLenum 
>> buffer, GLint drawbuffer,
>>      FLUSH_VERTICES(ctx, 0);
>>      FLUSH_CURRENT(ctx, 0);
>> 
>>  -   if (ctx->NewState) {
>>  -      _mesa_update_state( ctx );
>>  +   if (ctx->NewState & _NEW_BUFFERS) {
>>  +      _mesa_update_framebuffer(ctx, ctx->ReadBuffer, 
>> ctx->DrawBuffer);
>>  +      ctx->NewState &= ~_NEW_BUFFERS;
>>      }
>> 
>>      switch (buffer) {
>>  @@ -549,8 +553,9 @@ clear_bufferfv(struct gl_context *ctx, GLenum 
>> buffer, GLint drawbuffer,
>>      FLUSH_VERTICES(ctx, 0);
>>      FLUSH_CURRENT(ctx, 0);
>> 
>>  -   if (ctx->NewState) {
>>  -      _mesa_update_state( ctx );
>>  +   if (ctx->NewState & _NEW_BUFFERS) {
>>  +      _mesa_update_framebuffer(ctx, ctx->ReadBuffer, 
>> ctx->DrawBuffer);
>>  +      ctx->NewState &= ~_NEW_BUFFERS;
>>      }
>> 
>>      switch (buffer) {
>>  @@ -691,8 +696,9 @@ clear_bufferfi(struct gl_context *ctx, GLenum 
>> buffer, GLint drawbuffer,
>>      if (ctx->RasterDiscard)
>>         return;
>> 
>>  -   if (ctx->NewState) {
>>  -      _mesa_update_state( ctx );
>>  +   if (ctx->NewState & _NEW_BUFFERS) {
>>  +      _mesa_update_framebuffer(ctx, ctx->ReadBuffer, 
>> ctx->DrawBuffer);
>>  +      ctx->NewState &= ~_NEW_BUFFERS;
>>      }
>> 
>>      if (ctx->DrawBuffer->Attachment[BUFFER_DEPTH].Renderbuffer)
>>  --
>>  2.17.1
>> 
>>  _______________________________________________
>>  mesa-dev mailing list
>>  mesa-dev at lists.freedesktop.org
>>  https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev