On Wed, Jan 13, 2016 at 11:41 AM, Fredrik Höglund <[email protected]> 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 <[email protected]> >> >> >> >> --- >> >> 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 _______________________________________________ mesa-dev mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/mesa-dev
