[Mesa-dev] [PATCH] i965: Don't do the temporary-and-blit-copy for INVALIDATE_RANGE maps.

[Kenneth Graunke]

On 12/24/2013 03:47 PM, Eric Anholt wrote:
> We definitely want to fall through to the unsynchronized map case, instead
> of wasting bandwidth on a copy.  Prevents a -43.2407% +/- 1.06113% (n=49)
> performance regression on aa10perf when teaching glamor to provide the
> GL_INVALIDATE_RANGE_BIT information.
> 
> This is a performance fix, which I usually wouldn't cherry-pick to stable.
> But this was really was just a bug in the code, its presence would
> discourage developers from giving us the best information they can, and I
> think we've got fairly high confidence in the unsynchronized map path
> already.
> 
> Cc: 10.0 9.2 <mesa-stable at lists.freedesktop.org>
> ---
>  src/mesa/drivers/dri/i965/intel_buffer_objects.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Yep, obvious improvement.  I would've sent this out earlier, but never
found an application affected by it.  Glad you found one.

Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

I won't object to it landing on stable...it seems like an obvious small
fix.  Though, I suppose it could expose bugs in broken applications.
Seems unlikely though, so it should be fine.

A few comments about potential follow-ups...

> diff --git a/src/mesa/drivers/dri/i965/intel_buffer_objects.c b/src/mesa/drivers/dri/i965/intel_buffer_objects.c
> index cab805a..84bc29d 100644
> --- a/src/mesa/drivers/dri/i965/intel_buffer_objects.c
> +++ b/src/mesa/drivers/dri/i965/intel_buffer_objects.c
> @@ -357,7 +357,8 @@ intel_bufferobj_map_range(struct gl_context * ctx,
>      * BO, and we'll copy what they put in there out at unmap or
>      * FlushRange time.
>      */
> -   if ((access & GL_MAP_INVALIDATE_RANGE_BIT) &&
> +   if (!(access & GL_MAP_UNSYNCHRONIZED_BIT) &&
> +       (access & GL_MAP_INVALIDATE_RANGE_BIT) &&
>         drm_intel_bo_busy(intel_obj->buffer)) {
>        if (access & GL_MAP_FLUSH_EXPLICIT_BIT) {
>  	 intel_obj->range_map_buffer = malloc(length);
> 

This function is getting pretty awful to read.

The INVALIDATE_RANGE block here looks fairly redundant with the
INVALIDATE_BUFFER block, above.  Both call drm_intel_bo_busy, which
isn't that cheap.  It seems like the two blocks could be
combined...check references, check busy, either chuck the whole buffer
or arrange for a subrange blit BO...

If you don't have time, I can always try and give it a shot.

--Ken