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

Fredrik Höglund fredrik at kde.org
Wed Jan 13 02:41:57 PST 2016 

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.

> 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(&rctx->b, &rbuffer->b.b);
> >> -		} else {
> >> -			util_range_set_empty(&rbuffer->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:
> >>   	case PIPE_CAP_FORCE_PERSAMPLE_INTERP:
> >>   	case PIPE_CAP_COPY_BETWEEN_COMPRESSED_AND_PLAIN_FORMATS:
> >> +	case PIPE_CAP_INVALIDATE_BUFFER:
> >>   		return 1;
> >>
> >>   	case PIPE_CAP_RESOURCE_FROM_USER_MEMORY:
> >> @@ -347,7 +348,6 @@ static int si_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:
> >>
> >
>

More information about the mesa-dev mailing list