[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 <fredrik at kde.org> 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 <nicolai.haehnle at amd.com>
>> >>
>> >> ---
>> >>   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(&rctx->b, &rbuffer->b.b);
>> >> +  } else {
>> >> +          util_range_set_empty(&rbuffer->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