[Mesa-stable] [Mesa-dev] [PATCH] meta: Fix the pbo usage in meta for GLES{1, 2} contexts

[Anuj Phogat]

On Fri, Feb 5, 2016 at 11:30 AM, Ian Romanick <idr at freedesktop.org> wrote:
>
> On 12/24/2015 01:04 PM, Anuj Phogat wrote:
> > OpenGL ES 1.0 doesn't support using GL_STREAM_DRAW and both
> > ES 1.0 and 2.0 don't support GL_STREAM_READ in glBufferData().
> > So, handle it correctly by calling the _mesa_meta_begin()
> > before create_texture_for_pbo().
> >
> > Cc: "11.1" <mesa-stable at lists.freedesktop.org>
> > Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> > ---
> >  src/mesa/drivers/common/meta_tex_subimage.c | 27 ++++++++++++++++-----------
> >  1 file changed, 16 insertions(+), 11 deletions(-)
> >
> > diff --git a/src/mesa/drivers/common/meta_tex_subimage.c b/src/mesa/drivers/common/meta_tex_subimage.c
> > index 4adaad7..8ef306e 100644
> > --- a/src/mesa/drivers/common/meta_tex_subimage.c
> > +++ b/src/mesa/drivers/common/meta_tex_subimage.c
> > @@ -211,19 +211,21 @@ _mesa_meta_pbo_TexSubImage(struct gl_context *ctx, GLuint dims,
> >      */
> >     image_height = packing->ImageHeight == 0 ? height : packing->ImageHeight;
> >
> > +   if (allocate_storage)
> > +      ctx->Driver.AllocTextureImageBuffer(ctx, tex_image);
> > +
> > +   _mesa_meta_begin(ctx, ~(MESA_META_PIXEL_TRANSFER |
> > +                           MESA_META_PIXEL_STORE));
> > +
>
> Moving the call to _mesa_meta_begin makes sense from the commit message.
>  It's not obvious to me why the call to AllocTextureImageBuffer was also
> moved.
>
I wasn't sure if the state changes in  _mesa_meta_begin() will affect the
AllocTextureImageBuffer(), so I chose to keep the sequence of calls same.
I'll drop the changes for AllocTextureImageBuffer() because 'allocate_storage'
passed to this function is anyways always false. I'll send out a separate patch
to remove this parameter.

> Also... is there a test case that hits this?  I think some of the
> patches in my meta-cannot-use-Gen series might also fix this.
>
One of the piglit test hit this on my 'tile_resource_mode' branch. I had changes
which forced the driver to always use meta pbo path and create a temporary
pbo to read from / write to textures using meta path. Currently, we create a
temporary pbo in meta only when 'tex_busy' is true.

> >     pbo_tex_image = create_texture_for_pbo(ctx, create_pbo,
> >                                            GL_PIXEL_UNPACK_BUFFER,
> >                                            dims, width, height, depth,
> >                                            format, type, pixels, packing,
> >                                            &pbo, &pbo_tex);
> > -   if (!pbo_tex_image)
> > +   if (!pbo_tex_image) {
> > +      _mesa_meta_end(ctx);
> >        return false;
> > -
> > -   if (allocate_storage)
> > -      ctx->Driver.AllocTextureImageBuffer(ctx, tex_image);
> > -
> > -   _mesa_meta_begin(ctx, ~(MESA_META_PIXEL_TRANSFER |
> > -                           MESA_META_PIXEL_STORE));
> > +   }
> >
> >     _mesa_GenFramebuffers(2, fbos);
> >     _mesa_BindFramebuffer(GL_READ_FRAMEBUFFER, fbos[0]);
> > @@ -346,15 +348,18 @@ _mesa_meta_pbo_GetTexSubImage(struct gl_context *ctx, GLuint dims,
> >      */
> >     image_height = packing->ImageHeight == 0 ? height : packing->ImageHeight;
> >
> > +   _mesa_meta_begin(ctx, ~(MESA_META_PIXEL_TRANSFER |
> > +                           MESA_META_PIXEL_STORE));
> > +
> >     pbo_tex_image = create_texture_for_pbo(ctx, false, GL_PIXEL_PACK_BUFFER,
> >                                            dims, width, height, depth,
> >                                            format, type, pixels, packing,
> >                                            &pbo, &pbo_tex);
> > -   if (!pbo_tex_image)
> > -      return false;
> >
> > -   _mesa_meta_begin(ctx, ~(MESA_META_PIXEL_TRANSFER |
> > -                           MESA_META_PIXEL_STORE));
> > +   if (!pbo_tex_image) {
> > +      _mesa_meta_end(ctx);
> > +      return false;
> > +   }
> >
> >     /* GL_CLAMP_FRAGMENT_COLOR doesn't affect ReadPixels and GettexImage */
> >     if (ctx->Extensions.ARB_color_buffer_float)
> >
>