[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 <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?
>
> <Populate the buffer with data>
> <Use the buffer as a source for rendering operations>
> glInvalidateBufferData(buffer);
> <Don't touch the buffer again for the remainder of the frame>
>
> 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 alternative way to do something that can already
> be done more efficiently with a single call to an existing function.
>
> But as I said, your implementation is an improvement over what we
> have, and it does help at least one real application, so consider my
> objection withdrawn.
>
> Fredrik
>