Re: [Mesa-dev] [PATCH 7/8] gallium/radeon: implement PIPE_CAP_INVALIDATE_BUFFER

[Fredrik Höglund]

On Wednesday 13 January 2016, Nicolai Hähnle wrote:
> On 13.01.2016 05:41, Fredrik Höglund wrote:
> > On Tuesday 12 January 2016, Nicolai Hähnle wrote:
> >> On 12.01.2016 13:41, Fredrik Höglund wrote:
> >>> On Tuesday 12 January 2016, Nicolai Hähnle wrote:
>  From: Nicolai Hähnle 
> 
>  ---
> src/gallium/drivers/r600/r600_pipe.c|  2 +-
> src/gallium/drivers/radeon/r600_buffer_common.c | 23 
>  ---
> src/gallium/drivers/radeon/r600_pipe_common.c   |  1 +
> src/gallium/drivers/radeon/r600_pipe_common.h   |  3 +++
> src/gallium/drivers/radeonsi/si_pipe.c  |  2 +-
> 5 files changed, 22 insertions(+), 9 deletions(-)
> 
>  diff --git a/src/gallium/drivers/r600/r600_pipe.c 
>  b/src/gallium/drivers/r600/r600_pipe.c
>  index a8805f6..569f77c 100644
>  --- a/src/gallium/drivers/r600/r600_pipe.c
>  +++ b/src/gallium/drivers/r600/r600_pipe.c
>  @@ -278,6 +278,7 @@ static int r600_get_param(struct pipe_screen* 
>  pscreen, enum pipe_cap param)
>   case PIPE_CAP_TEXTURE_HALF_FLOAT_LINEAR:
>   case PIPE_CAP_TGSI_TXQS:
>   case PIPE_CAP_COPY_BETWEEN_COMPRESSED_AND_PLAIN_FORMATS:
>  +case PIPE_CAP_INVALIDATE_BUFFER:
>   return 1;
> 
>   case PIPE_CAP_DEVICE_RESET_STATUS_QUERY:
>  @@ -355,7 +356,6 @@ static int r600_get_param(struct pipe_screen* 
>  pscreen, enum pipe_cap param)
>   case PIPE_CAP_TGSI_FS_POSITION_IS_SYSVAL:
>   case PIPE_CAP_TGSI_FS_FACE_IS_INTEGER_SYSVAL:
>   case PIPE_CAP_SHADER_BUFFER_OFFSET_ALIGNMENT:
>  -case PIPE_CAP_INVALIDATE_BUFFER:
>   return 0;
> 
>   case PIPE_CAP_MAX_SHADER_PATCH_VARYINGS:
>  diff --git a/src/gallium/drivers/radeon/r600_buffer_common.c 
>  b/src/gallium/drivers/radeon/r600_buffer_common.c
>  index aeb9a20..09755e0 100644
>  --- a/src/gallium/drivers/radeon/r600_buffer_common.c
>  +++ b/src/gallium/drivers/radeon/r600_buffer_common.c
>  @@ -209,6 +209,21 @@ static void r600_buffer_destroy(struct pipe_screen 
>  *screen,
>   FREE(rbuffer);
> }
> 
>  +void r600_invalidate_resource(struct pipe_context *ctx,
>  +  struct pipe_resource *resource)
>  +{
>  +struct r600_common_context *rctx = (struct 
>  r600_common_context*)ctx;
>  +struct r600_resource *rbuffer = r600_resource(resource);
>  +
>  +/* Check if mapping this buffer would cause waiting for the 
>  GPU. */
>  +if (r600_rings_is_buffer_referenced(rctx, rbuffer->buf, 
>  RADEON_USAGE_READWRITE) ||
>  +!rctx->ws->buffer_wait(rbuffer->buf, 0, 
>  RADEON_USAGE_READWRITE)) {
>  +rctx->invalidate_buffer(>b, >b.b);
>  +} else {
>  +util_range_set_empty(>valid_buffer_range);
>  +}
> >>>
> >>> This implementation does not exactly comply with the specification.
> >>>
> >>> The point of InvalidateBuffer is to tell the driver that it may discard 
> >>> the
> >>> contents of the buffer if, for example, the buffer needs to be evicted.
> >>>
> >>> Calling InvalidateBuffer is not equivalent to calling MapBufferRange
> >>> with GL_MAP_INVALIDATE_BUFFER_BIT, since the former should invalidate
> >>> the buffer regardless of whether it is busy or not.
> >>
> >> Can you back this with a quote from the spec? Given that no-op seems to
> >> be a correct implmentation of InvalidateBuffer, I find what you write
> >> rather hard to believe.
> >
> > The overview says:
> >
> > "GL implementations often include several memory spaces, each with
> >  distinct performance characteristics, and the implementations
> >  transparently move allocations between memory spaces. With this
> >  extension, an application can tell the GL that the contents of a
> >  texture or buffer are no longer needed, and the implementation can
> >  avoid transferring the data unnecessarily."
> >
> > This to me makes the intent pretty clear.  The implementation is of
> > course free to do what it wants with this information, including nothing
> > at all.  My objection here is that your implementation only helps
> > applications that are using the extension incorrectly.  But it is still an
> > improvement over doing nothing at all.
> 
> This implementation helps applications that use glInvalidateBufferData 
> to invalidate a buffer that they use in a streaming fashion. It seems to 
> me that that is a correct use.
> 
> Perhaps you could give an example of what you think a correct use is, 
> and how it isn't helped by this patch?



glInvalidateBufferData(buffer);


At this point we know that the application is not going to use the

Re: [Mesa-dev] [PATCH 7/8] gallium/radeon: implement PIPE_CAP_INVALIDATE_BUFFER

[Nicolai Hähnle]

On 15.01.2016 09:47, Fredrik Höglund wrote:

On Wednesday 13 January 2016, Nicolai Hähnle wrote:

On 13.01.2016 05:41, Fredrik Höglund wrote:

On Tuesday 12 January 2016, Nicolai Hähnle wrote:

On 12.01.2016 13:41, Fredrik Höglund wrote:

On Tuesday 12 January 2016, Nicolai Hähnle wrote:

From: Nicolai Hähnle 

---
src/gallium/drivers/r600/r600_pipe.c|  2 +-
src/gallium/drivers/radeon/r600_buffer_common.c | 23 ---
src/gallium/drivers/radeon/r600_pipe_common.c   |  1 +
src/gallium/drivers/radeon/r600_pipe_common.h   |  3 +++
src/gallium/drivers/radeonsi/si_pipe.c  |  2 +-
5 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/src/gallium/drivers/r600/r600_pipe.c 
b/src/gallium/drivers/r600/r600_pipe.c
index a8805f6..569f77c 100644
--- a/src/gallium/drivers/r600/r600_pipe.c
+++ b/src/gallium/drivers/r600/r600_pipe.c
@@ -278,6 +278,7 @@ static int r600_get_param(struct pipe_screen* pscreen, enum 
pipe_cap param)
case PIPE_CAP_TEXTURE_HALF_FLOAT_LINEAR:
case PIPE_CAP_TGSI_TXQS:
case PIPE_CAP_COPY_BETWEEN_COMPRESSED_AND_PLAIN_FORMATS:
+   case PIPE_CAP_INVALIDATE_BUFFER:
return 1;

case PIPE_CAP_DEVICE_RESET_STATUS_QUERY:
@@ -355,7 +356,6 @@ static int r600_get_param(struct pipe_screen* pscreen, enum 
pipe_cap param)
case PIPE_CAP_TGSI_FS_POSITION_IS_SYSVAL:
case PIPE_CAP_TGSI_FS_FACE_IS_INTEGER_SYSVAL:
case PIPE_CAP_SHADER_BUFFER_OFFSET_ALIGNMENT:
-   case PIPE_CAP_INVALIDATE_BUFFER:
return 0;

case PIPE_CAP_MAX_SHADER_PATCH_VARYINGS:
diff --git a/src/gallium/drivers/radeon/r600_buffer_common.c 
b/src/gallium/drivers/radeon/r600_buffer_common.c
index aeb9a20..09755e0 100644
--- a/src/gallium/drivers/radeon/r600_buffer_common.c
+++ b/src/gallium/drivers/radeon/r600_buffer_common.c
@@ -209,6 +209,21 @@ static void r600_buffer_destroy(struct pipe_screen *screen,
FREE(rbuffer);
}

+void r600_invalidate_resource(struct pipe_context *ctx,
+ struct pipe_resource *resource)
+{
+   struct r600_common_context *rctx = (struct r600_common_context*)ctx;
+struct r600_resource *rbuffer = r600_resource(resource);
+
+   /* Check if mapping this buffer would cause waiting for the GPU. */
+   if (r600_rings_is_buffer_referenced(rctx, rbuffer->buf, 
RADEON_USAGE_READWRITE) ||
+   !rctx->ws->buffer_wait(rbuffer->buf, 0, RADEON_USAGE_READWRITE)) {
+   rctx->invalidate_buffer(>b, >b.b);
+   } else {
+   util_range_set_empty(>valid_buffer_range);
+   }


This implementation does not exactly comply with the specification.

The point of InvalidateBuffer is to tell the driver that it may discard the
contents of the buffer if, for example, the buffer needs to be evicted.

Calling InvalidateBuffer is not equivalent to calling MapBufferRange
with GL_MAP_INVALIDATE_BUFFER_BIT, since the former should invalidate
the buffer regardless of whether it is busy or not.


Can you back this with a quote from the spec? Given that no-op seems to
be a correct implmentation of InvalidateBuffer, I find what you write
rather hard to believe.


The overview says:

"GL implementations often include several memory spaces, each with
 distinct performance characteristics, and the implementations
 transparently move allocations between memory spaces. With this
 extension, an application can tell the GL that the contents of a
 texture or buffer are no longer needed, and the implementation can
 avoid transferring the data unnecessarily."

This to me makes the intent pretty clear.  The implementation is of
course free to do what it wants with this information, including nothing
at all.  My objection here is that your implementation only helps
applications that are using the extension incorrectly.  But it is still an
improvement over doing nothing at all.


This implementation helps applications that use glInvalidateBufferData
to invalidate a buffer that they use in a streaming fashion. It seems to
me that that is a correct use.

Perhaps you could give an example of what you think a correct use is,
and how it isn't helped by this patch?




glInvalidateBufferData(buffer);


At this point we know that the application is not going to use the
contents again, and doesn't expect it to still be there the next time
it accesses the buffer.  That makes it a good candidate for eviction
since we can just discard the contents.


Okay, I see now what you're getting at. It's definitely something to 
keep in mind for the future, we'll have to watch what kind of usage 
patterns crop up in practice.


Thanks,
Nicolai



While this function can be used for invalidating a buffer just before
mapping it, I think it's unlikely that Khronos would have added
it just to provide an 

Re: [Mesa-dev] [PATCH 7/8] gallium/radeon: implement PIPE_CAP_INVALIDATE_BUFFER

[Fredrik Höglund]

On Tuesday 12 January 2016, Nicolai Hähnle wrote:
> On 12.01.2016 13:41, Fredrik Höglund wrote:
> > On Tuesday 12 January 2016, Nicolai Hähnle wrote:
> >> From: Nicolai Hähnle 
> >>
> >> ---
> >>   src/gallium/drivers/r600/r600_pipe.c|  2 +-
> >>   src/gallium/drivers/radeon/r600_buffer_common.c | 23 
> >> ---
> >>   src/gallium/drivers/radeon/r600_pipe_common.c   |  1 +
> >>   src/gallium/drivers/radeon/r600_pipe_common.h   |  3 +++
> >>   src/gallium/drivers/radeonsi/si_pipe.c  |  2 +-
> >>   5 files changed, 22 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/src/gallium/drivers/r600/r600_pipe.c 
> >> b/src/gallium/drivers/r600/r600_pipe.c
> >> index a8805f6..569f77c 100644
> >> --- a/src/gallium/drivers/r600/r600_pipe.c
> >> +++ b/src/gallium/drivers/r600/r600_pipe.c
> >> @@ -278,6 +278,7 @@ static int r600_get_param(struct pipe_screen* pscreen, 
> >> enum pipe_cap param)
> >>case PIPE_CAP_TEXTURE_HALF_FLOAT_LINEAR:
> >>case PIPE_CAP_TGSI_TXQS:
> >>case PIPE_CAP_COPY_BETWEEN_COMPRESSED_AND_PLAIN_FORMATS:
> >> +  case PIPE_CAP_INVALIDATE_BUFFER:
> >>return 1;
> >>
> >>case PIPE_CAP_DEVICE_RESET_STATUS_QUERY:
> >> @@ -355,7 +356,6 @@ static int r600_get_param(struct pipe_screen* pscreen, 
> >> enum pipe_cap param)
> >>case PIPE_CAP_TGSI_FS_POSITION_IS_SYSVAL:
> >>case PIPE_CAP_TGSI_FS_FACE_IS_INTEGER_SYSVAL:
> >>case PIPE_CAP_SHADER_BUFFER_OFFSET_ALIGNMENT:
> >> -  case PIPE_CAP_INVALIDATE_BUFFER:
> >>return 0;
> >>
> >>case PIPE_CAP_MAX_SHADER_PATCH_VARYINGS:
> >> diff --git a/src/gallium/drivers/radeon/r600_buffer_common.c 
> >> b/src/gallium/drivers/radeon/r600_buffer_common.c
> >> index aeb9a20..09755e0 100644
> >> --- a/src/gallium/drivers/radeon/r600_buffer_common.c
> >> +++ b/src/gallium/drivers/radeon/r600_buffer_common.c
> >> @@ -209,6 +209,21 @@ static void r600_buffer_destroy(struct pipe_screen 
> >> *screen,
> >>FREE(rbuffer);
> >>   }
> >>
> >> +void r600_invalidate_resource(struct pipe_context *ctx,
> >> +struct pipe_resource *resource)
> >> +{
> >> +  struct r600_common_context *rctx = (struct r600_common_context*)ctx;
> >> +struct r600_resource *rbuffer = r600_resource(resource);
> >> +
> >> +  /* Check if mapping this buffer would cause waiting for the GPU. */
> >> +  if (r600_rings_is_buffer_referenced(rctx, rbuffer->buf, 
> >> RADEON_USAGE_READWRITE) ||
> >> +  !rctx->ws->buffer_wait(rbuffer->buf, 0, RADEON_USAGE_READWRITE)) {
> >> +  rctx->invalidate_buffer(>b, >b.b);
> >> +  } else {
> >> +  util_range_set_empty(>valid_buffer_range);
> >> +  }
> >
> > This implementation does not exactly comply with the specification.
> >
> > The point of InvalidateBuffer is to tell the driver that it may discard the
> > contents of the buffer if, for example, the buffer needs to be evicted.
> >
> > Calling InvalidateBuffer is not equivalent to calling MapBufferRange
> > with GL_MAP_INVALIDATE_BUFFER_BIT, since the former should invalidate
> > the buffer regardless of whether it is busy or not.
> 
> Can you back this with a quote from the spec? Given that no-op seems to 
> be a correct implmentation of InvalidateBuffer, I find what you write 
> rather hard to believe.

The overview says:

"GL implementations often include several memory spaces, each with
 distinct performance characteristics, and the implementations
 transparently move allocations between memory spaces. With this
 extension, an application can tell the GL that the contents of a
 texture or buffer are no longer needed, and the implementation can
 avoid transferring the data unnecessarily."

This to me makes the intent pretty clear.  The implementation is of
course free to do what it wants with this information, including nothing
at all.  My objection here is that your implementation only helps
applications that are using the extension incorrectly.  But it is still an
improvement over doing nothing at all.

> Part of the problems may be that the spec talks about "invalidating" 
> without - as far as I can tell - ever defining what that means. In any 
> case, I see no reason why the behavior should be different form 
> GL_MAP_INVALIDATE_BUFFER_BIT.
>
> Thanks,
> Nicolai
> 
> >
> >> +}
> >> +
> >>   static void *r600_buffer_get_transfer(struct pipe_context *ctx,
> >>  struct pipe_resource *resource,
> >> unsigned level,
> >> @@ -276,13 +291,7 @@ static void *r600_buffer_transfer_map(struct 
> >> pipe_context *ctx,
> >>!(usage & PIPE_TRANSFER_UNSYNCHRONIZED)) {
> >>assert(usage & PIPE_TRANSFER_WRITE);
> >>
> >> -  /* Check if mapping this buffer would cause waiting for the 
> >> GPU. */
> >> -  if (r600_rings_is_buffer_referenced(rctx, rbuffer->buf, 
> >> RADEON_USAGE_READWRITE) ||
> >> -  

Re: [Mesa-dev] [PATCH 7/8] gallium/radeon: implement PIPE_CAP_INVALIDATE_BUFFER

[Marek Olšák]

On Wed, Jan 13, 2016 at 11:41 AM, Fredrik Höglund  wrote:
> On Tuesday 12 January 2016, Nicolai Hähnle wrote:
>> On 12.01.2016 13:41, Fredrik Höglund wrote:
>> > On Tuesday 12 January 2016, Nicolai Hähnle wrote:
>> >> From: Nicolai Hähnle 
>> >>
>> >> ---
>> >>   src/gallium/drivers/r600/r600_pipe.c|  2 +-
>> >>   src/gallium/drivers/radeon/r600_buffer_common.c | 23 
>> >> ---
>> >>   src/gallium/drivers/radeon/r600_pipe_common.c   |  1 +
>> >>   src/gallium/drivers/radeon/r600_pipe_common.h   |  3 +++
>> >>   src/gallium/drivers/radeonsi/si_pipe.c  |  2 +-
>> >>   5 files changed, 22 insertions(+), 9 deletions(-)
>> >>
>> >> diff --git a/src/gallium/drivers/r600/r600_pipe.c 
>> >> b/src/gallium/drivers/r600/r600_pipe.c
>> >> index a8805f6..569f77c 100644
>> >> --- a/src/gallium/drivers/r600/r600_pipe.c
>> >> +++ b/src/gallium/drivers/r600/r600_pipe.c
>> >> @@ -278,6 +278,7 @@ static int r600_get_param(struct pipe_screen* 
>> >> pscreen, enum pipe_cap param)
>> >>case PIPE_CAP_TEXTURE_HALF_FLOAT_LINEAR:
>> >>case PIPE_CAP_TGSI_TXQS:
>> >>case PIPE_CAP_COPY_BETWEEN_COMPRESSED_AND_PLAIN_FORMATS:
>> >> +  case PIPE_CAP_INVALIDATE_BUFFER:
>> >>return 1;
>> >>
>> >>case PIPE_CAP_DEVICE_RESET_STATUS_QUERY:
>> >> @@ -355,7 +356,6 @@ static int r600_get_param(struct pipe_screen* 
>> >> pscreen, enum pipe_cap param)
>> >>case PIPE_CAP_TGSI_FS_POSITION_IS_SYSVAL:
>> >>case PIPE_CAP_TGSI_FS_FACE_IS_INTEGER_SYSVAL:
>> >>case PIPE_CAP_SHADER_BUFFER_OFFSET_ALIGNMENT:
>> >> -  case PIPE_CAP_INVALIDATE_BUFFER:
>> >>return 0;
>> >>
>> >>case PIPE_CAP_MAX_SHADER_PATCH_VARYINGS:
>> >> diff --git a/src/gallium/drivers/radeon/r600_buffer_common.c 
>> >> b/src/gallium/drivers/radeon/r600_buffer_common.c
>> >> index aeb9a20..09755e0 100644
>> >> --- a/src/gallium/drivers/radeon/r600_buffer_common.c
>> >> +++ b/src/gallium/drivers/radeon/r600_buffer_common.c
>> >> @@ -209,6 +209,21 @@ static void r600_buffer_destroy(struct pipe_screen 
>> >> *screen,
>> >>FREE(rbuffer);
>> >>   }
>> >>
>> >> +void r600_invalidate_resource(struct pipe_context *ctx,
>> >> +struct pipe_resource *resource)
>> >> +{
>> >> +  struct r600_common_context *rctx = (struct r600_common_context*)ctx;
>> >> +struct r600_resource *rbuffer = r600_resource(resource);
>> >> +
>> >> +  /* Check if mapping this buffer would cause waiting for the GPU. */
>> >> +  if (r600_rings_is_buffer_referenced(rctx, rbuffer->buf, 
>> >> RADEON_USAGE_READWRITE) ||
>> >> +  !rctx->ws->buffer_wait(rbuffer->buf, 0, RADEON_USAGE_READWRITE)) {
>> >> +  rctx->invalidate_buffer(>b, >b.b);
>> >> +  } else {
>> >> +  util_range_set_empty(>valid_buffer_range);
>> >> +  }
>> >
>> > This implementation does not exactly comply with the specification.
>> >
>> > The point of InvalidateBuffer is to tell the driver that it may discard the
>> > contents of the buffer if, for example, the buffer needs to be evicted.
>> >
>> > Calling InvalidateBuffer is not equivalent to calling MapBufferRange
>> > with GL_MAP_INVALIDATE_BUFFER_BIT, since the former should invalidate
>> > the buffer regardless of whether it is busy or not.
>>
>> Can you back this with a quote from the spec? Given that no-op seems to
>> be a correct implmentation of InvalidateBuffer, I find what you write
>> rather hard to believe.
>
> The overview says:
>
> "GL implementations often include several memory spaces, each with
>  distinct performance characteristics, and the implementations
>  transparently move allocations between memory spaces. With this
>  extension, an application can tell the GL that the contents of a
>  texture or buffer are no longer needed, and the implementation can
>  avoid transferring the data unnecessarily."
>
> This to me makes the intent pretty clear.  The implementation is of
> course free to do what it wants with this information, including nothing
> at all.  My objection here is that your implementation only helps
> applications that are using the extension incorrectly.  But it is still an
> improvement over doing nothing at all.

I wouldn't worry about the spec overview too much. It's just a
motivating introduction to the spec.

However, immediately before InvalidateBufferData, there is this sentence:

"After this command, data in the specified range have undefined values."

That's a very clear definition of the behavior, and this patch seems
to do the right thing.

Marek
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 7/8] gallium/radeon: implement PIPE_CAP_INVALIDATE_BUFFER

[Nicolai Hähnle]

On 13.01.2016 05:41, Fredrik Höglund wrote:

On Tuesday 12 January 2016, Nicolai Hähnle wrote:

On 12.01.2016 13:41, Fredrik Höglund wrote:

On Tuesday 12 January 2016, Nicolai Hähnle wrote:

From: Nicolai Hähnle 

---
   src/gallium/drivers/r600/r600_pipe.c|  2 +-
   src/gallium/drivers/radeon/r600_buffer_common.c | 23 ---
   src/gallium/drivers/radeon/r600_pipe_common.c   |  1 +
   src/gallium/drivers/radeon/r600_pipe_common.h   |  3 +++
   src/gallium/drivers/radeonsi/si_pipe.c  |  2 +-
   5 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/src/gallium/drivers/r600/r600_pipe.c 
b/src/gallium/drivers/r600/r600_pipe.c
index a8805f6..569f77c 100644
--- a/src/gallium/drivers/r600/r600_pipe.c
+++ b/src/gallium/drivers/r600/r600_pipe.c
@@ -278,6 +278,7 @@ static int r600_get_param(struct pipe_screen* pscreen, enum 
pipe_cap param)
case PIPE_CAP_TEXTURE_HALF_FLOAT_LINEAR:
case PIPE_CAP_TGSI_TXQS:
case PIPE_CAP_COPY_BETWEEN_COMPRESSED_AND_PLAIN_FORMATS:
+   case PIPE_CAP_INVALIDATE_BUFFER:
return 1;

case PIPE_CAP_DEVICE_RESET_STATUS_QUERY:
@@ -355,7 +356,6 @@ static int r600_get_param(struct pipe_screen* pscreen, enum 
pipe_cap param)
case PIPE_CAP_TGSI_FS_POSITION_IS_SYSVAL:
case PIPE_CAP_TGSI_FS_FACE_IS_INTEGER_SYSVAL:
case PIPE_CAP_SHADER_BUFFER_OFFSET_ALIGNMENT:
-   case PIPE_CAP_INVALIDATE_BUFFER:
return 0;

case PIPE_CAP_MAX_SHADER_PATCH_VARYINGS:
diff --git a/src/gallium/drivers/radeon/r600_buffer_common.c 
b/src/gallium/drivers/radeon/r600_buffer_common.c
index aeb9a20..09755e0 100644
--- a/src/gallium/drivers/radeon/r600_buffer_common.c
+++ b/src/gallium/drivers/radeon/r600_buffer_common.c
@@ -209,6 +209,21 @@ static void r600_buffer_destroy(struct pipe_screen *screen,
FREE(rbuffer);
   }

+void r600_invalidate_resource(struct pipe_context *ctx,
+ struct pipe_resource *resource)
+{
+   struct r600_common_context *rctx = (struct r600_common_context*)ctx;
+struct r600_resource *rbuffer = r600_resource(resource);
+
+   /* Check if mapping this buffer would cause waiting for the GPU. */
+   if (r600_rings_is_buffer_referenced(rctx, rbuffer->buf, 
RADEON_USAGE_READWRITE) ||
+   !rctx->ws->buffer_wait(rbuffer->buf, 0, RADEON_USAGE_READWRITE)) {
+   rctx->invalidate_buffer(>b, >b.b);
+   } else {
+   util_range_set_empty(>valid_buffer_range);
+   }


This implementation does not exactly comply with the specification.

The point of InvalidateBuffer is to tell the driver that it may discard the
contents of the buffer if, for example, the buffer needs to be evicted.

Calling InvalidateBuffer is not equivalent to calling MapBufferRange
with GL_MAP_INVALIDATE_BUFFER_BIT, since the former should invalidate
the buffer regardless of whether it is busy or not.


Can you back this with a quote from the spec? Given that no-op seems to
be a correct implmentation of InvalidateBuffer, I find what you write
rather hard to believe.


The overview says:

"GL implementations often include several memory spaces, each with
 distinct performance characteristics, and the implementations
 transparently move allocations between memory spaces. With this
 extension, an application can tell the GL that the contents of a
 texture or buffer are no longer needed, and the implementation can
 avoid transferring the data unnecessarily."

This to me makes the intent pretty clear.  The implementation is of
course free to do what it wants with this information, including nothing
at all.  My objection here is that your implementation only helps
applications that are using the extension incorrectly.  But it is still an
improvement over doing nothing at all.


This implementation helps applications that use glInvalidateBufferData 
to invalidate a buffer that they use in a streaming fashion. It seems to 
me that that is a correct use.


Perhaps you could give an example of what you think a correct use is, 
and how it isn't helped by this patch?


Thanks,
Nicolai




Part of the problems may be that the spec talks about "invalidating"
without - as far as I can tell - ever defining what that means. In any
case, I see no reason why the behavior should be different form
GL_MAP_INVALIDATE_BUFFER_BIT.

Thanks,
Nicolai




+}
+
   static void *r600_buffer_get_transfer(struct pipe_context *ctx,
  struct pipe_resource *resource,
 unsigned level,
@@ -276,13 +291,7 @@ static void *r600_buffer_transfer_map(struct pipe_context 
*ctx,
!(usage & PIPE_TRANSFER_UNSYNCHRONIZED)) {
assert(usage & PIPE_TRANSFER_WRITE);

-   /* Check if mapping this buffer would cause waiting for the 
GPU. */
-   if 

Re: [Mesa-dev] [PATCH 7/8] gallium/radeon: implement PIPE_CAP_INVALIDATE_BUFFER

[Nicolai Hähnle]

On 12.01.2016 13:41, Fredrik Höglund wrote:

On Tuesday 12 January 2016, Nicolai Hähnle wrote:

From: Nicolai Hähnle 

---
  src/gallium/drivers/r600/r600_pipe.c|  2 +-
  src/gallium/drivers/radeon/r600_buffer_common.c | 23 ---
  src/gallium/drivers/radeon/r600_pipe_common.c   |  1 +
  src/gallium/drivers/radeon/r600_pipe_common.h   |  3 +++
  src/gallium/drivers/radeonsi/si_pipe.c  |  2 +-
  5 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/src/gallium/drivers/r600/r600_pipe.c 
b/src/gallium/drivers/r600/r600_pipe.c
index a8805f6..569f77c 100644
--- a/src/gallium/drivers/r600/r600_pipe.c
+++ b/src/gallium/drivers/r600/r600_pipe.c
@@ -278,6 +278,7 @@ static int r600_get_param(struct pipe_screen* pscreen, enum 
pipe_cap param)
case PIPE_CAP_TEXTURE_HALF_FLOAT_LINEAR:
case PIPE_CAP_TGSI_TXQS:
case PIPE_CAP_COPY_BETWEEN_COMPRESSED_AND_PLAIN_FORMATS:
+   case PIPE_CAP_INVALIDATE_BUFFER:
return 1;

case PIPE_CAP_DEVICE_RESET_STATUS_QUERY:
@@ -355,7 +356,6 @@ static int r600_get_param(struct pipe_screen* pscreen, enum 
pipe_cap param)
case PIPE_CAP_TGSI_FS_POSITION_IS_SYSVAL:
case PIPE_CAP_TGSI_FS_FACE_IS_INTEGER_SYSVAL:
case PIPE_CAP_SHADER_BUFFER_OFFSET_ALIGNMENT:
-   case PIPE_CAP_INVALIDATE_BUFFER:
return 0;

case PIPE_CAP_MAX_SHADER_PATCH_VARYINGS:
diff --git a/src/gallium/drivers/radeon/r600_buffer_common.c 
b/src/gallium/drivers/radeon/r600_buffer_common.c
index aeb9a20..09755e0 100644
--- a/src/gallium/drivers/radeon/r600_buffer_common.c
+++ b/src/gallium/drivers/radeon/r600_buffer_common.c
@@ -209,6 +209,21 @@ static void r600_buffer_destroy(struct pipe_screen *screen,
FREE(rbuffer);
  }

+void r600_invalidate_resource(struct pipe_context *ctx,
+ struct pipe_resource *resource)
+{
+   struct r600_common_context *rctx = (struct r600_common_context*)ctx;
+struct r600_resource *rbuffer = r600_resource(resource);
+
+   /* Check if mapping this buffer would cause waiting for the GPU. */
+   if (r600_rings_is_buffer_referenced(rctx, rbuffer->buf, 
RADEON_USAGE_READWRITE) ||
+   !rctx->ws->buffer_wait(rbuffer->buf, 0, RADEON_USAGE_READWRITE)) {
+   rctx->invalidate_buffer(>b, >b.b);
+   } else {
+   util_range_set_empty(>valid_buffer_range);
+   }


This implementation does not exactly comply with the specification.

The point of InvalidateBuffer is to tell the driver that it may discard the
contents of the buffer if, for example, the buffer needs to be evicted.

Calling InvalidateBuffer is not equivalent to calling MapBufferRange
with GL_MAP_INVALIDATE_BUFFER_BIT, since the former should invalidate
the buffer regardless of whether it is busy or not.


Can you back this with a quote from the spec? Given that no-op seems to 
be a correct implmentation of InvalidateBuffer, I find what you write 
rather hard to believe.


Part of the problems may be that the spec talks about "invalidating" 
without - as far as I can tell - ever defining what that means. In any 
case, I see no reason why the behavior should be different form 
GL_MAP_INVALIDATE_BUFFER_BIT.


Thanks,
Nicolai




+}
+
  static void *r600_buffer_get_transfer(struct pipe_context *ctx,
  struct pipe_resource *resource,
unsigned level,
@@ -276,13 +291,7 @@ static void *r600_buffer_transfer_map(struct pipe_context 
*ctx,
!(usage & PIPE_TRANSFER_UNSYNCHRONIZED)) {
assert(usage & PIPE_TRANSFER_WRITE);

-   /* Check if mapping this buffer would cause waiting for the 
GPU. */
-   if (r600_rings_is_buffer_referenced(rctx, rbuffer->buf, 
RADEON_USAGE_READWRITE) ||
-   !rctx->ws->buffer_wait(rbuffer->buf, 0, 
RADEON_USAGE_READWRITE)) {
-   rctx->invalidate_buffer(>b, >b.b);
-   } else {
-   util_range_set_empty(>valid_buffer_range);
-   }
+   r600_invalidate_resource(ctx, resource);

/* At this point, the buffer is always idle. */
usage |= PIPE_TRANSFER_UNSYNCHRONIZED;
diff --git a/src/gallium/drivers/radeon/r600_pipe_common.c 
b/src/gallium/drivers/radeon/r600_pipe_common.c
index 52c365e..e926f56 100644
--- a/src/gallium/drivers/radeon/r600_pipe_common.c
+++ b/src/gallium/drivers/radeon/r600_pipe_common.c
@@ -257,6 +257,7 @@ bool r600_common_context_init(struct r600_common_context 
*rctx,
else
rctx->max_db = 4;

+   rctx->b.invalidate_resource = r600_invalidate_resource;
rctx->b.transfer_map = u_transfer_map_vtbl;
rctx->b.transfer_flush_region = u_transfer_flush_region_vtbl;
rctx->b.transfer_unmap = u_transfer_unmap_vtbl;
diff --git a/src/gallium/drivers/radeon/r600_pipe_common.h 

[Mesa-dev] [PATCH 7/8] gallium/radeon: implement PIPE_CAP_INVALIDATE_BUFFER

[Nicolai Hähnle]

From: Nicolai Hähnle 

---
 src/gallium/drivers/r600/r600_pipe.c|  2 +-
 src/gallium/drivers/radeon/r600_buffer_common.c | 23 ---
 src/gallium/drivers/radeon/r600_pipe_common.c   |  1 +
 src/gallium/drivers/radeon/r600_pipe_common.h   |  3 +++
 src/gallium/drivers/radeonsi/si_pipe.c  |  2 +-
 5 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/src/gallium/drivers/r600/r600_pipe.c 
b/src/gallium/drivers/r600/r600_pipe.c
index a8805f6..569f77c 100644
--- a/src/gallium/drivers/r600/r600_pipe.c
+++ b/src/gallium/drivers/r600/r600_pipe.c
@@ -278,6 +278,7 @@ static int r600_get_param(struct pipe_screen* pscreen, enum 
pipe_cap param)
case PIPE_CAP_TEXTURE_HALF_FLOAT_LINEAR:
case PIPE_CAP_TGSI_TXQS:
case PIPE_CAP_COPY_BETWEEN_COMPRESSED_AND_PLAIN_FORMATS:
+   case PIPE_CAP_INVALIDATE_BUFFER:
return 1;
 
case PIPE_CAP_DEVICE_RESET_STATUS_QUERY:
@@ -355,7 +356,6 @@ static int r600_get_param(struct pipe_screen* pscreen, enum 
pipe_cap param)
case PIPE_CAP_TGSI_FS_POSITION_IS_SYSVAL:
case PIPE_CAP_TGSI_FS_FACE_IS_INTEGER_SYSVAL:
case PIPE_CAP_SHADER_BUFFER_OFFSET_ALIGNMENT:
-   case PIPE_CAP_INVALIDATE_BUFFER:
return 0;
 
case PIPE_CAP_MAX_SHADER_PATCH_VARYINGS:
diff --git a/src/gallium/drivers/radeon/r600_buffer_common.c 
b/src/gallium/drivers/radeon/r600_buffer_common.c
index aeb9a20..09755e0 100644
--- a/src/gallium/drivers/radeon/r600_buffer_common.c
+++ b/src/gallium/drivers/radeon/r600_buffer_common.c
@@ -209,6 +209,21 @@ static void r600_buffer_destroy(struct pipe_screen *screen,
FREE(rbuffer);
 }
 
+void r600_invalidate_resource(struct pipe_context *ctx,
+ struct pipe_resource *resource)
+{
+   struct r600_common_context *rctx = (struct r600_common_context*)ctx;
+struct r600_resource *rbuffer = r600_resource(resource);
+
+   /* Check if mapping this buffer would cause waiting for the GPU. */
+   if (r600_rings_is_buffer_referenced(rctx, rbuffer->buf, 
RADEON_USAGE_READWRITE) ||
+   !rctx->ws->buffer_wait(rbuffer->buf, 0, RADEON_USAGE_READWRITE)) {
+   rctx->invalidate_buffer(>b, >b.b);
+   } else {
+   util_range_set_empty(>valid_buffer_range);
+   }
+}
+
 static void *r600_buffer_get_transfer(struct pipe_context *ctx,
  struct pipe_resource *resource,
   unsigned level,
@@ -276,13 +291,7 @@ static void *r600_buffer_transfer_map(struct pipe_context 
*ctx,
!(usage & PIPE_TRANSFER_UNSYNCHRONIZED)) {
assert(usage & PIPE_TRANSFER_WRITE);
 
-   /* Check if mapping this buffer would cause waiting for the 
GPU. */
-   if (r600_rings_is_buffer_referenced(rctx, rbuffer->buf, 
RADEON_USAGE_READWRITE) ||
-   !rctx->ws->buffer_wait(rbuffer->buf, 0, 
RADEON_USAGE_READWRITE)) {
-   rctx->invalidate_buffer(>b, >b.b);
-   } else {
-   util_range_set_empty(>valid_buffer_range);
-   }
+   r600_invalidate_resource(ctx, resource);
 
/* At this point, the buffer is always idle. */
usage |= PIPE_TRANSFER_UNSYNCHRONIZED;
diff --git a/src/gallium/drivers/radeon/r600_pipe_common.c 
b/src/gallium/drivers/radeon/r600_pipe_common.c
index 52c365e..e926f56 100644
--- a/src/gallium/drivers/radeon/r600_pipe_common.c
+++ b/src/gallium/drivers/radeon/r600_pipe_common.c
@@ -257,6 +257,7 @@ bool r600_common_context_init(struct r600_common_context 
*rctx,
else
rctx->max_db = 4;
 
+   rctx->b.invalidate_resource = r600_invalidate_resource;
rctx->b.transfer_map = u_transfer_map_vtbl;
rctx->b.transfer_flush_region = u_transfer_flush_region_vtbl;
rctx->b.transfer_unmap = u_transfer_unmap_vtbl;
diff --git a/src/gallium/drivers/radeon/r600_pipe_common.h 
b/src/gallium/drivers/radeon/r600_pipe_common.h
index 68b50a9..27f6e98 100644
--- a/src/gallium/drivers/radeon/r600_pipe_common.h
+++ b/src/gallium/drivers/radeon/r600_pipe_common.h
@@ -500,6 +500,9 @@ struct pipe_resource *
 r600_buffer_from_user_memory(struct pipe_screen *screen,
 const struct pipe_resource *templ,
 void *user_memory);
+void
+r600_invalidate_resource(struct pipe_context *ctx,
+struct pipe_resource *resource);
 
 /* r600_common_pipe.c */
 void r600_draw_rectangle(struct blitter_context *blitter,
diff --git a/src/gallium/drivers/radeonsi/si_pipe.c 
b/src/gallium/drivers/radeonsi/si_pipe.c
index 512f939..fd4bda9 100644
--- a/src/gallium/drivers/radeonsi/si_pipe.c
+++ b/src/gallium/drivers/radeonsi/si_pipe.c
@@ -301,6 +301,7 @@ static int si_get_param(struct pipe_screen* pscreen, enum 
pipe_cap param)
case PIPE_CAP_TGSI_TXQS:

Re: [Mesa-dev] [PATCH 7/8] gallium/radeon: implement PIPE_CAP_INVALIDATE_BUFFER

[Fredrik Höglund]

On Tuesday 12 January 2016, Nicolai Hähnle wrote:
> From: Nicolai Hähnle 
> 
> ---
>  src/gallium/drivers/r600/r600_pipe.c|  2 +-
>  src/gallium/drivers/radeon/r600_buffer_common.c | 23 ---
>  src/gallium/drivers/radeon/r600_pipe_common.c   |  1 +
>  src/gallium/drivers/radeon/r600_pipe_common.h   |  3 +++
>  src/gallium/drivers/radeonsi/si_pipe.c  |  2 +-
>  5 files changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/src/gallium/drivers/r600/r600_pipe.c 
> b/src/gallium/drivers/r600/r600_pipe.c
> index a8805f6..569f77c 100644
> --- a/src/gallium/drivers/r600/r600_pipe.c
> +++ b/src/gallium/drivers/r600/r600_pipe.c
> @@ -278,6 +278,7 @@ static int r600_get_param(struct pipe_screen* pscreen, 
> enum pipe_cap param)
>   case PIPE_CAP_TEXTURE_HALF_FLOAT_LINEAR:
>   case PIPE_CAP_TGSI_TXQS:
>   case PIPE_CAP_COPY_BETWEEN_COMPRESSED_AND_PLAIN_FORMATS:
> + case PIPE_CAP_INVALIDATE_BUFFER:
>   return 1;
>  
>   case PIPE_CAP_DEVICE_RESET_STATUS_QUERY:
> @@ -355,7 +356,6 @@ static int r600_get_param(struct pipe_screen* pscreen, 
> enum pipe_cap param)
>   case PIPE_CAP_TGSI_FS_POSITION_IS_SYSVAL:
>   case PIPE_CAP_TGSI_FS_FACE_IS_INTEGER_SYSVAL:
>   case PIPE_CAP_SHADER_BUFFER_OFFSET_ALIGNMENT:
> - case PIPE_CAP_INVALIDATE_BUFFER:
>   return 0;
>  
>   case PIPE_CAP_MAX_SHADER_PATCH_VARYINGS:
> diff --git a/src/gallium/drivers/radeon/r600_buffer_common.c 
> b/src/gallium/drivers/radeon/r600_buffer_common.c
> index aeb9a20..09755e0 100644
> --- a/src/gallium/drivers/radeon/r600_buffer_common.c
> +++ b/src/gallium/drivers/radeon/r600_buffer_common.c
> @@ -209,6 +209,21 @@ static void r600_buffer_destroy(struct pipe_screen 
> *screen,
>   FREE(rbuffer);
>  }
>  
> +void r600_invalidate_resource(struct pipe_context *ctx,
> +   struct pipe_resource *resource)
> +{
> + struct r600_common_context *rctx = (struct r600_common_context*)ctx;
> +struct r600_resource *rbuffer = r600_resource(resource);
> +
> + /* Check if mapping this buffer would cause waiting for the GPU. */
> + if (r600_rings_is_buffer_referenced(rctx, rbuffer->buf, 
> RADEON_USAGE_READWRITE) ||
> + !rctx->ws->buffer_wait(rbuffer->buf, 0, RADEON_USAGE_READWRITE)) {
> + rctx->invalidate_buffer(>b, >b.b);
> + } else {
> + util_range_set_empty(>valid_buffer_range);
> + }

This implementation does not exactly comply with the specification.

The point of InvalidateBuffer is to tell the driver that it may discard the
contents of the buffer if, for example, the buffer needs to be evicted.

Calling InvalidateBuffer is not equivalent to calling MapBufferRange
with GL_MAP_INVALIDATE_BUFFER_BIT, since the former should invalidate
the buffer regardless of whether it is busy or not.

> +}
> +
>  static void *r600_buffer_get_transfer(struct pipe_context *ctx,
> struct pipe_resource *resource,
>unsigned level,
> @@ -276,13 +291,7 @@ static void *r600_buffer_transfer_map(struct 
> pipe_context *ctx,
>   !(usage & PIPE_TRANSFER_UNSYNCHRONIZED)) {
>   assert(usage & PIPE_TRANSFER_WRITE);
>  
> - /* Check if mapping this buffer would cause waiting for the 
> GPU. */
> - if (r600_rings_is_buffer_referenced(rctx, rbuffer->buf, 
> RADEON_USAGE_READWRITE) ||
> - !rctx->ws->buffer_wait(rbuffer->buf, 0, 
> RADEON_USAGE_READWRITE)) {
> - rctx->invalidate_buffer(>b, >b.b);
> - } else {
> - util_range_set_empty(>valid_buffer_range);
> - }
> + r600_invalidate_resource(ctx, resource);
>  
>   /* At this point, the buffer is always idle. */
>   usage |= PIPE_TRANSFER_UNSYNCHRONIZED;
> diff --git a/src/gallium/drivers/radeon/r600_pipe_common.c 
> b/src/gallium/drivers/radeon/r600_pipe_common.c
> index 52c365e..e926f56 100644
> --- a/src/gallium/drivers/radeon/r600_pipe_common.c
> +++ b/src/gallium/drivers/radeon/r600_pipe_common.c
> @@ -257,6 +257,7 @@ bool r600_common_context_init(struct r600_common_context 
> *rctx,
>   else
>   rctx->max_db = 4;
>  
> + rctx->b.invalidate_resource = r600_invalidate_resource;
>   rctx->b.transfer_map = u_transfer_map_vtbl;
>   rctx->b.transfer_flush_region = u_transfer_flush_region_vtbl;
>   rctx->b.transfer_unmap = u_transfer_unmap_vtbl;
> diff --git a/src/gallium/drivers/radeon/r600_pipe_common.h 
> b/src/gallium/drivers/radeon/r600_pipe_common.h
> index 68b50a9..27f6e98 100644
> --- a/src/gallium/drivers/radeon/r600_pipe_common.h
> +++ b/src/gallium/drivers/radeon/r600_pipe_common.h
> @@ -500,6 +500,9 @@ struct pipe_resource *
>  r600_buffer_from_user_memory(struct pipe_screen *screen,
>const struct pipe_resource *templ,
>