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

Nicolai Hähnle nhaehnle at gmail.com
Wed Jan 13 10:22:31 PST 2016


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 <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.

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 (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