[Mesa-dev] i965: Don't create a temp PBO when uploading data from glTexImage*

Submitted by Neil Roberts on June 12, 2015, 2:34 p.m.

Details

Message ID 1434119668-16806-1-git-send-email-neil@linux.intel.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Neil Roberts June 12, 2015, 2:34 p.m.
Previously when glTexImage* is called it would attempt to create a
temporary PBO if the texture is busy in order to avoid blocking when
mapping the texture. This doesn't make much sense for glTexImage
because in that case we are completely replacing the texture anyway so
instead of allocating a PBO we can just allocate new storage for the
texture.

The code was buggy anyway because it was checking whether the buffer
was busy before calling Driver->AllocTextureImageBuffer. That function
actually always frees the buffer and recreates a new one so it was
checking whether the previous buffer was busy and this is irrelevant.

In practice I think this wouldn't matter too much because the upper
layers of Mesa always call Driver->FreeTextureImageBuffer before
calling Driver->TexImage anyway so there would never be a buffer that
could be busy.
---
 src/mesa/drivers/dri/i965/intel_tex_image.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/mesa/drivers/dri/i965/intel_tex_image.c b/src/mesa/drivers/dri/i965/intel_tex_image.c
index 85d3d04..2874e5b 100644
--- a/src/mesa/drivers/dri/i965/intel_tex_image.c
+++ b/src/mesa/drivers/dri/i965/intel_tex_image.c
@@ -95,8 +95,6 @@  intelTexImage(struct gl_context * ctx,
    struct intel_texture_image *intelImage = intel_texture_image(texImage);
    bool ok;
 
-   bool tex_busy = intelImage->mt && drm_intel_bo_busy(intelImage->mt->bo);
-
    DBG("%s mesa_format %s target %s format %s type %s level %d %dx%dx%d\n",
        __func__, _mesa_get_format_name(texImage->TexFormat),
        _mesa_lookup_enum_by_nr(texImage->TexObject->Target),
@@ -116,7 +114,8 @@  intelTexImage(struct gl_context * ctx,
                                    texImage->Depth,
                                    format, type, pixels,
                                    false /*allocate_storage*/,
-                                   tex_busy, unpack);
+                                   false /*create_pbo*/,
+                                   unpack);
    if (ok)
       return;
 

Comments

On Fri, Jun 12, 2015 at 7:34 AM, Neil Roberts <neil@linux.intel.com> wrote:
> Previously when glTexImage* is called it would attempt to create a
> temporary PBO if the texture is busy in order to avoid blocking when
> mapping the texture. This doesn't make much sense for glTexImage
> because in that case we are completely replacing the texture anyway so
> instead of allocating a PBO we can just allocate new storage for the
> texture.
>
> The code was buggy anyway because it was checking whether the buffer
> was busy before calling Driver->AllocTextureImageBuffer. That function
> actually always frees the buffer and recreates a new one so it was
> checking whether the previous buffer was busy and this is irrelevant.
>
> In practice I think this wouldn't matter too much because the upper
> layers of Mesa always call Driver->FreeTextureImageBuffer before
> calling Driver->TexImage anyway so there would never be a buffer that
> could be busy.
> ---
>  src/mesa/drivers/dri/i965/intel_tex_image.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_tex_image.c b/src/mesa/drivers/dri/i965/intel_tex_image.c
> index 85d3d04..2874e5b 100644
> --- a/src/mesa/drivers/dri/i965/intel_tex_image.c
> +++ b/src/mesa/drivers/dri/i965/intel_tex_image.c
> @@ -95,8 +95,6 @@ intelTexImage(struct gl_context * ctx,
>     struct intel_texture_image *intelImage = intel_texture_image(texImage);
>     bool ok;
>
> -   bool tex_busy = intelImage->mt && drm_intel_bo_busy(intelImage->mt->bo);
> -
>     DBG("%s mesa_format %s target %s format %s type %s level %d %dx%dx%d\n",
>         __func__, _mesa_get_format_name(texImage->TexFormat),
>         _mesa_lookup_enum_by_nr(texImage->TexObject->Target),
> @@ -116,7 +114,8 @@ intelTexImage(struct gl_context * ctx,
>                                     texImage->Depth,
>                                     format, type, pixels,
>                                     false /*allocate_storage*/,
> -                                   tex_busy, unpack);
> +                                   false /*create_pbo*/,
> +                                   unpack);
>     if (ok)
>        return;
>
> --
> 1.9.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev

It makes sense.
Reviewed-by: Anuj Phogat <anuj.phogat@gmail.com>
On Fri, Jun 12, 2015 at 7:34 AM, Neil Roberts <neil@linux.intel.com> wrote:
> Previously when glTexImage* is called it would attempt to create a
> temporary PBO if the texture is busy in order to avoid blocking when
> mapping the texture. This doesn't make much sense for glTexImage
> because in that case we are completely replacing the texture anyway so
> instead of allocating a PBO we can just allocate new storage for the
> texture.
>
> The code was buggy anyway because it was checking whether the buffer
> was busy before calling Driver->AllocTextureImageBuffer. That function
> actually always frees the buffer and recreates a new one so it was
> checking whether the previous buffer was busy and this is irrelevant.

I'm not sure this is correct.  You can still do partial updates with
TexImage if you are updating a single miplevel or cube face.

> In practice I think this wouldn't matter too much because the upper
> layers of Mesa always call Driver->FreeTextureImageBuffer before
> calling Driver->TexImage anyway so there would never be a buffer that
> could be busy.
> ---
>  src/mesa/drivers/dri/i965/intel_tex_image.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_tex_image.c b/src/mesa/drivers/dri/i965/intel_tex_image.c
> index 85d3d04..2874e5b 100644
> --- a/src/mesa/drivers/dri/i965/intel_tex_image.c
> +++ b/src/mesa/drivers/dri/i965/intel_tex_image.c
> @@ -95,8 +95,6 @@ intelTexImage(struct gl_context * ctx,
>     struct intel_texture_image *intelImage = intel_texture_image(texImage);
>     bool ok;
>
> -   bool tex_busy = intelImage->mt && drm_intel_bo_busy(intelImage->mt->bo);
> -
>     DBG("%s mesa_format %s target %s format %s type %s level %d %dx%dx%d\n",
>         __func__, _mesa_get_format_name(texImage->TexFormat),
>         _mesa_lookup_enum_by_nr(texImage->TexObject->Target),
> @@ -116,7 +114,8 @@ intelTexImage(struct gl_context * ctx,
>                                     texImage->Depth,
>                                     format, type, pixels,
>                                     false /*allocate_storage*/,
> -                                   tex_busy, unpack);
> +                                   false /*create_pbo*/,
> +                                   unpack);
>     if (ok)
>        return;
>
> --
> 1.9.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> On Fri, Jun 12, 2015 at 7:34 AM, Neil Roberts <neil@linux.intel.com> wrote:
>> The code was buggy anyway because it was checking whether the buffer
>> was busy before calling Driver->AllocTextureImageBuffer. That function
>> actually always frees the buffer and recreates a new one so it was
>> checking whether the previous buffer was busy and this is irrelevant.

Jason Ekstrand <jason@jlekstrand.net> writes:

> I'm not sure this is correct.  You can still do partial updates with
> TexImage if you are updating a single miplevel or cube face.

Ah yes, you're right. I didn't look into the code far enough. When it
calls Driver->AllocTextureImageBuffer it will likely get the same
miptree back because there is a pointer to it from the texture object.

However, it still seems like creating a PBO would be counter-productive
most of the time because you are most probably going to replace all of
the mipmaps if you replace one of them. Maybe a good thing to do would
be to check whether intel_texobj->mt is busy in
intel_alloc_texture_image_buffer and then let it allocate a new mt in
that case. That way in the likely case that the rest of the images are
updated too then it would use a new buffer and avoid blocking without
introducing an extra copy. If it turns out that the application really
is just updating one image then it will be copied in anyway when the
texture is finalized which achieves more or less the same thing as using
a PBO.

>> In practice I think this wouldn't matter too much because the upper
>> layers of Mesa always call Driver->FreeTextureImageBuffer before
>> calling Driver->TexImage anyway so there would never be a buffer that
>> could be busy.

I think this point still holds and means it's effectively impossible to
hit the create_pbo=true case.

Regards,
- Neil