[2/3] vrend: If available use glCopyImageSubData to execute memcopy like blits

Submitted by Gert Wollny on June 20, 2018, 9:02 a.m.

Details

Message ID 20180620090226.5687-3-gert.wollny@collabora.com
State New
Series "Fix GLES31 copy_image.non-compressed tests"
Headers show

Commit Message

Gert Wollny June 20, 2018, 9:02 a.m.
When the host is  gles >= 3.2, gl >= 4.3 or when the extension
GL_(ARB|EXT|OES)_copy_image is available, memcopy like blitting and region
copying can be done for many color format combinations by using
glCopyImageSubData.

This fixes a number of tests from the subset
  dEQP-GLES31.functional.copy_image.non_compressed.viewclass_*

Signed-off-by: Gert Wollny <gert.wollny@collabora.com>
---
 src/vrend_formats.c  | 25 ++++++++++++++++++++++
 src/vrend_renderer.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 src/vrend_renderer.h |  2 ++
 3 files changed, 83 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/vrend_formats.c b/src/vrend_formats.c
index eb9f217..d1653a4 100644
--- a/src/vrend_formats.c
+++ b/src/vrend_formats.c
@@ -443,3 +443,28 @@  void vrend_build_format_list_gles(void)
    */
   add_formats(gles_z32_format);
 }
+
+bool vrend_is_canonical_format(enum pipe_format format)
+{
+   /* These are the canonical formats that are set by gallium */
+
+   switch (format) {
+   case PIPE_FORMAT_R32G32B32A32_UINT:
+   case PIPE_FORMAT_R32G32B32_UINT:
+   case PIPE_FORMAT_R32G32_UINT:
+   case PIPE_FORMAT_R16G16B16A16_UINT:
+   case PIPE_FORMAT_R16G16B16_UINT:
+   case PIPE_FORMAT_R8G8B8A8_UNORM:
+   case PIPE_FORMAT_A8B8G8R8_UNORM:
+   case PIPE_FORMAT_R16G16_UNORM:
+   case PIPE_FORMAT_R32_UINT:
+   case PIPE_FORMAT_R8G8B8_UINT:
+   case PIPE_FORMAT_G8R8_UNORM:
+   case PIPE_FORMAT_R8G8_UNORM:
+   case PIPE_FORMAT_R16_UINT:
+   case PIPE_FORMAT_R8_UINT:
+      return true;
+   default:
+      return false;
+   }
+}
\ No newline at end of file
diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c
index b769e8a..8690eb2 100644
--- a/src/vrend_renderer.c
+++ b/src/vrend_renderer.c
@@ -118,6 +118,7 @@  struct global_renderer_state {
    bool have_polygon_offset_clamp;
    bool have_texture_storage;
    bool have_tessellation;
+   bool have_copy_image;
 
    /* these appeared broken on at least one driver */
    bool use_explicit_locations;
@@ -4449,6 +4450,12 @@  int vrend_renderer_init(struct vrend_if_cbs *cbs, uint32_t flags)
    if (gl_ver >= 46 || epoxy_has_gl_extension("GL_ARB_polygon_offset_clamp"))
       vrend_state.have_polygon_offset_clamp = true;
 
+   if (gl_ver >= 43 || (gles && gl_ver >= 32) ||
+       epoxy_has_gl_extension("GL_ARB_copy_image") ||
+       epoxy_has_gl_extension("GL_EXT_copy_image") ||
+       epoxy_has_gl_extension("GL_OES_copy_image"))
+       vrend_state.have_copy_image = true;
+
    /* callbacks for when we are cleaning up the object table */
    vrend_resource_set_destroy_callback(vrend_destroy_resource_object);
    vrend_object_set_destroy_callback(VIRGL_OBJECT_QUERY, vrend_destroy_query_object);
@@ -6260,6 +6269,22 @@  static void vrend_resource_copy_fallback(struct vrend_context *ctx,
    free(tptr);
 }
 
+
+static inline void
+vrend_copy_sub_image(struct vrend_resource* src_res, struct vrend_resource * dst_res,
+                     uint32_t src_level, const struct pipe_box *src_box,
+                     uint32_t dst_level, uint32_t dstx, uint32_t dsty, uint32_t dstz)
+{
+   glCopyImageSubData(src_res->id,
+                      tgsitargettogltarget(src_res->base.target, src_res->base.nr_samples),
+                      src_level, src_box->x, src_box->y, src_box->z,
+                      dst_res->id,
+                      tgsitargettogltarget(dst_res->base.target, dst_res->base.nr_samples),
+                      dst_level, dstx, dsty, dstz,
+                      src_box->width, src_box->height,src_box->depth);
+}
+
+
 void vrend_renderer_resource_copy_region(struct vrend_context *ctx,
                                          uint32_t dst_handle, uint32_t dst_level,
                                          uint32_t dstx, uint32_t dsty, uint32_t dstz,
@@ -6292,11 +6317,20 @@  void vrend_renderer_resource_copy_region(struct vrend_context *ctx,
       return;
    }
 
+   if (vrend_state.have_copy_image) {
+      const struct util_format_description *src_desc = util_format_description(src_res->base.format);
+      const struct util_format_description *dst_desc = util_format_description(dst_res->base.format);
+      if (util_is_format_compatible(src_desc,dst_desc)) {
+         vrend_copy_sub_image(src_res, dst_res, src_level, src_box,
+                              dst_level, dstx, dsty, dstz);
+         return;
+      }
+   }
+
    if (!vrend_format_can_render(src_res->base.format) ||
        !vrend_format_can_render(dst_res->base.format)) {
       vrend_resource_copy_fallback(ctx, src_res, dst_res, dst_level, dstx,
                                    dsty, dstz, src_level, src_box);
-
       return;
    }
 
@@ -6568,7 +6602,27 @@  void vrend_renderer_blit(struct vrend_context *ctx,
    if (info->render_condition_enable == false)
       vrend_pause_render_condition(ctx, true);
 
-   vrend_renderer_blit_int(ctx, src_res, dst_res, info);
+   /* The gallium blit function can be called for a general blit that may
+    * scale and convert the data or for a memcopy blit. In the latter case
+    * the info->formats are equal and set to one of the canonical formats.
+    * In addition some other restrictions apply. For this latter case use the
+    * glCopyImageSubData function.
+    */
+   if (vrend_state.have_copy_image &&
+       vrend_is_canonical_format(info->src.format) &&
+       (info->src.format == info->dst.format) &&
+       !info->scissor_enable && (info->filter == PIPE_TEX_FILTER_NEAREST) &&
+       !info->alpha_blend && (info->mask == PIPE_MASK_RGBA) &&
+       (src_res->base.nr_samples == dst_res->base.nr_samples) &&
+       info->src.box.width == info->dst.box.width &&
+       info->src.box.height == info->dst.box.height &&
+       info->src.box.depth == info->dst.box.depth) {
+      vrend_copy_sub_image(src_res, dst_res, info->src.level, &info->src.box,
+                           info->dst.level, info->dst.box.x, info->dst.box.y,
+                           info->dst.box.z);
+   } else {
+       vrend_renderer_blit_int(ctx, src_res, dst_res, info);
+   }
 
    if (info->render_condition_enable == false)
       vrend_pause_render_condition(ctx, false);
diff --git a/src/vrend_renderer.h b/src/vrend_renderer.h
index 258c439..badf2b1 100644
--- a/src/vrend_renderer.h
+++ b/src/vrend_renderer.h
@@ -323,6 +323,8 @@  GLint64 vrend_renderer_get_timestamp(void);
 void vrend_build_format_list_common(void);
 void vrend_build_format_list_gl(void);
 void vrend_build_format_list_gles(void);
+bool vrend_is_canonical_format(enum pipe_format format);
+
 
 int vrend_renderer_resource_attach_iov(int res_handle, struct iovec *iov,
                                        int num_iovs);

Comments

Gurchetan Singh June 26, 2018, 3:52 a.m.
On Wed, Jun 20, 2018 at 2:03 AM Gert Wollny <gert.wollny@collabora.com> wrote:
>
> When the host is  gles >= 3.2, gl >= 4.3 or when the extension
> GL_(ARB|EXT|OES)_copy_image is available, memcopy like blitting and region
> copying can be done for many color format combinations by using
> glCopyImageSubData.
>
> This fixes a number of tests from the subset
>   dEQP-GLES31.functional.copy_image.non_compressed.viewclass_*
>
> Signed-off-by: Gert Wollny <gert.wollny@collabora.com>
> ---
>  src/vrend_formats.c  | 25 ++++++++++++++++++++++
>  src/vrend_renderer.c | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  src/vrend_renderer.h |  2 ++
>  3 files changed, 83 insertions(+), 2 deletions(-)
>
> diff --git a/src/vrend_formats.c b/src/vrend_formats.c
> index eb9f217..d1653a4 100644
> --- a/src/vrend_formats.c
> +++ b/src/vrend_formats.c
> @@ -443,3 +443,28 @@ void vrend_build_format_list_gles(void)
>     */
>    add_formats(gles_z32_format);
>  }
> +
> +bool vrend_is_canonical_format(enum pipe_format format)
> +{
> +   /* These are the canonical formats that are set by gallium */
> +
> +   switch (format) {
> +   case PIPE_FORMAT_R32G32B32A32_UINT:
> +   case PIPE_FORMAT_R32G32B32_UINT:
> +   case PIPE_FORMAT_R32G32_UINT:
> +   case PIPE_FORMAT_R16G16B16A16_UINT:
> +   case PIPE_FORMAT_R16G16B16_UINT:
> +   case PIPE_FORMAT_R8G8B8A8_UNORM:
> +   case PIPE_FORMAT_A8B8G8R8_UNORM:
> +   case PIPE_FORMAT_R16G16_UNORM:
> +   case PIPE_FORMAT_R32_UINT:
> +   case PIPE_FORMAT_R8G8B8_UINT:
> +   case PIPE_FORMAT_G8R8_UNORM:
> +   case PIPE_FORMAT_R8G8_UNORM:
> +   case PIPE_FORMAT_R16_UINT:
> +   case PIPE_FORMAT_R8_UINT:
> +      return true;
> +   default:
> +      return false;
> +   }
> +}

What do you mean by "canonical" formats?  st_cb_copyimage.c makes some
references to canonical formats, but doesn't really list it like this
as this patch does.


> \ No newline at end of file
> diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c
> index b769e8a..8690eb2 100644
> --- a/src/vrend_renderer.c
> +++ b/src/vrend_renderer.c
> @@ -118,6 +118,7 @@ struct global_renderer_state {
>     bool have_polygon_offset_clamp;
>     bool have_texture_storage;
>     bool have_tessellation;
> +   bool have_copy_image;
>
>     /* these appeared broken on at least one driver */
>     bool use_explicit_locations;
> @@ -4449,6 +4450,12 @@ int vrend_renderer_init(struct vrend_if_cbs *cbs, uint32_t flags)
>     if (gl_ver >= 46 || epoxy_has_gl_extension("GL_ARB_polygon_offset_clamp"))
>        vrend_state.have_polygon_offset_clamp = true;
>
> +   if (gl_ver >= 43 || (gles && gl_ver >= 32) ||
> +       epoxy_has_gl_extension("GL_ARB_copy_image") ||
> +       epoxy_has_gl_extension("GL_EXT_copy_image") ||
> +       epoxy_has_gl_extension("GL_OES_copy_image"))
> +       vrend_state.have_copy_image = true;
> +
>     /* callbacks for when we are cleaning up the object table */
>     vrend_resource_set_destroy_callback(vrend_destroy_resource_object);
>     vrend_object_set_destroy_callback(VIRGL_OBJECT_QUERY, vrend_destroy_query_object);
> @@ -6260,6 +6269,22 @@ static void vrend_resource_copy_fallback(struct vrend_context *ctx,
>     free(tptr);
>  }
>
> +
> +static inline void
> +vrend_copy_sub_image(struct vrend_resource* src_res, struct vrend_resource * dst_res,
> +                     uint32_t src_level, const struct pipe_box *src_box,
> +                     uint32_t dst_level, uint32_t dstx, uint32_t dsty, uint32_t dstz)
> +{
> +   glCopyImageSubData(src_res->id,
> +                      tgsitargettogltarget(src_res->base.target, src_res->base.nr_samples),
> +                      src_level, src_box->x, src_box->y, src_box->z,
> +                      dst_res->id,
> +                      tgsitargettogltarget(dst_res->base.target, dst_res->base.nr_samples),
> +                      dst_level, dstx, dsty, dstz,
> +                      src_box->width, src_box->height,src_box->depth);
> +}
> +
> +
>  void vrend_renderer_resource_copy_region(struct vrend_context *ctx,
>                                           uint32_t dst_handle, uint32_t dst_level,
>                                           uint32_t dstx, uint32_t dsty, uint32_t dstz,
> @@ -6292,11 +6317,20 @@ void vrend_renderer_resource_copy_region(struct vrend_context *ctx,
>        return;
>     }
>
> +   if (vrend_state.have_copy_image) {
> +      const struct util_format_description *src_desc = util_format_description(src_res->base.format);
> +      const struct util_format_description *dst_desc = util_format_description(dst_res->base.format);
> +      if (util_is_format_compatible(src_desc,dst_desc)) {
> +         vrend_copy_sub_image(src_res, dst_res, src_level, src_box,
> +                              dst_level, dstx, dsty, dstz);
> +         return;
> +      }
> +   }
> +
>     if (!vrend_format_can_render(src_res->base.format) ||
>         !vrend_format_can_render(dst_res->base.format)) {
>        vrend_resource_copy_fallback(ctx, src_res, dst_res, dst_level, dstx,
>                                     dsty, dstz, src_level, src_box);
> -
>        return;
>     }
>
> @@ -6568,7 +6602,27 @@ void vrend_renderer_blit(struct vrend_context *ctx,
>     if (info->render_condition_enable == false)
>        vrend_pause_render_condition(ctx, true);
>
> -   vrend_renderer_blit_int(ctx, src_res, dst_res, info);
> +   /* The gallium blit function can be called for a general blit that may
> +    * scale and convert the data or for a memcopy blit. In the latter case
> +    * the info->formats are equal and set to one of the canonical formats.
> +    * In addition some other restrictions apply. For this latter case use the
> +    * glCopyImageSubData function.
> +    */
> +   if (vrend_state.have_copy_image &&
> +       vrend_is_canonical_format(info->src.format) &&
> +       (info->src.format == info->dst.format) &&
> +       !info->scissor_enable && (info->filter == PIPE_TEX_FILTER_NEAREST) &&
> +       !info->alpha_blend && (info->mask == PIPE_MASK_RGBA) &&
> +       (src_res->base.nr_samples == dst_res->base.nr_samples) &&
> +       info->src.box.width == info->dst.box.width &&
> +       info->src.box.height == info->dst.box.height &&
> +       info->src.box.depth == info->dst.box.depth) {
> +      vrend_copy_sub_image(src_res, dst_res, info->src.level, &info->src.box,
> +                           info->dst.level, info->dst.box.x, info->dst.box.y,
> +                           info->dst.box.z);
> +   } else {
> +       vrend_renderer_blit_int(ctx, src_res, dst_res, info);
> +   }

Hmm, do the GLES31 tests fail when doing blit framebuffer or during
the GL fallback?  Since this requires GLES32 features, but an
important use case is GLES31 on GLES31.  Would it be possible to fix
those cases without copy_sub_image??  +Alexandros Frantzis who's
worked with this code before and knows where the bodies are buried :-)


>
>     if (info->render_condition_enable == false)
>        vrend_pause_render_condition(ctx, false);
> diff --git a/src/vrend_renderer.h b/src/vrend_renderer.h
> index 258c439..badf2b1 100644
> --- a/src/vrend_renderer.h
> +++ b/src/vrend_renderer.h
> @@ -323,6 +323,8 @@ GLint64 vrend_renderer_get_timestamp(void);
>  void vrend_build_format_list_common(void);
>  void vrend_build_format_list_gl(void);
>  void vrend_build_format_list_gles(void);
> +bool vrend_is_canonical_format(enum pipe_format format);
> +
>
>  int vrend_renderer_resource_attach_iov(int res_handle, struct iovec *iov,
>                                         int num_iovs);
> --
> 2.16.4
>
> _______________________________________________
> virglrenderer-devel mailing list
> virglrenderer-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/virglrenderer-devel
Gert Wollny June 26, 2018, 7:10 a.m.
Am Montag, den 25.06.2018, 20:52 -0700 schrieb Gurchetan Singh:
> On Wed, Jun 20, 2018 at 2:03 AM Gert Wollny <gert.wollny@collabora.co
> m> wrote:
> > 
> > When the host is  gles >= 3.2, gl >= 4.3 or when the extension
> > GL_(ARB|EXT|OES)_copy_image is available, memcopy like blitting and
> > region
> > copying can be done for many color format combinations by using
> > glCopyImageSubData.
> > 
> > This fixes a number of tests from the subset
> >   dEQP-GLES31.functional.copy_image.non_compressed.viewclass_*
> > 
> > Signed-off-by: Gert Wollny <gert.wollny@collabora.com>
> > ---
> >  src/vrend_formats.c  | 25 ++++++++++++++++++++++
> >  src/vrend_renderer.c | 58
> > ++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  src/vrend_renderer.h |  2 ++
> >  3 files changed, 83 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/vrend_formats.c b/src/vrend_formats.c
> > index eb9f217..d1653a4 100644
> > --- a/src/vrend_formats.c
> > +++ b/src/vrend_formats.c
> > @@ -443,3 +443,28 @@ void vrend_build_format_list_gles(void)
> >     */
> >    add_formats(gles_z32_format);
> >  }
> > +
> > +bool vrend_is_canonical_format(enum pipe_format format)
> > +{
> > +   /* These are the canonical formats that are set by gallium */
> > +
> > +   switch (format) {
> > +   case PIPE_FORMAT_R32G32B32A32_UINT:
> > +   case PIPE_FORMAT_R32G32B32_UINT:
> > +   case PIPE_FORMAT_R32G32_UINT:
> > +   case PIPE_FORMAT_R16G16B16A16_UINT:
> > +   case PIPE_FORMAT_R16G16B16_UINT:
> > +   case PIPE_FORMAT_R8G8B8A8_UNORM:
> > +   case PIPE_FORMAT_A8B8G8R8_UNORM:
> > +   case PIPE_FORMAT_R16G16_UNORM:
> > +   case PIPE_FORMAT_R32_UINT:
> > +   case PIPE_FORMAT_R8G8B8_UINT:
> > +   case PIPE_FORMAT_G8R8_UNORM:
> > +   case PIPE_FORMAT_R8G8_UNORM:
> > +   case PIPE_FORMAT_R16_UINT:
> > +   case PIPE_FORMAT_R8_UINT:
> > +      return true;
> > +   default:
> > +      return false;
> > +   }
> > +}
> 
> What do you mean by "canonical" formats?  st_cb_copyimage.c makes
> some references to canonical formats, but doesn't really list it like
> this as this patch does.

In st_cb_copyimage.c get_canonocal_format the various format names are
mapped to these PIPE_FORMAT_* types, here I check whether the the
current format is one of those that is returned by this function,
that's why I thought the name would be apropriated. 

> 
> 
> > \ No newline at end of file
> > diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c
> > index b769e8a..8690eb2 100644
> > --- a/src/vrend_renderer.c
> > +++ b/src/vrend_renderer.c
> > @@ -118,6 +118,7 @@ struct global_renderer_state {
> >     bool have_polygon_offset_clamp;
> >     bool have_texture_storage;
> >     bool have_tessellation;
> > +   bool have_copy_image;
> > 
> >     /* these appeared broken on at least one driver */
> >     bool use_explicit_locations;
> > @@ -4449,6 +4450,12 @@ int vrend_renderer_init(struct vrend_if_cbs
> > *cbs, uint32_t flags)
> >     if (gl_ver >= 46 ||
> > epoxy_has_gl_extension("GL_ARB_polygon_offset_clamp"))
> >        vrend_state.have_polygon_offset_clamp = true;
> > 
> > +   if (gl_ver >= 43 || (gles && gl_ver >= 32) ||
> > +       epoxy_has_gl_extension("GL_ARB_copy_image") ||
> > +       epoxy_has_gl_extension("GL_EXT_copy_image") ||
> > +       epoxy_has_gl_extension("GL_OES_copy_image"))
> > +       vrend_state.have_copy_image = true;
> > +
> >     /* callbacks for when we are cleaning up the object table */
> >     vrend_resource_set_destroy_callback(vrend_destroy_resource_obje
> > ct);
> >     vrend_object_set_destroy_callback(VIRGL_OBJECT_QUERY,
> > vrend_destroy_query_object);
> > @@ -6260,6 +6269,22 @@ static void
> > vrend_resource_copy_fallback(struct vrend_context *ctx,
> >     free(tptr);
> >  }
> > 
> > +
> > +static inline void
> > +vrend_copy_sub_image(struct vrend_resource* src_res, struct
> > vrend_resource * dst_res,
> > +                     uint32_t src_level, const struct pipe_box
> > *src_box,
> > +                     uint32_t dst_level, uint32_t dstx, uint32_t
> > dsty, uint32_t dstz)
> > +{
> > +   glCopyImageSubData(src_res->id,
> > +                      tgsitargettogltarget(src_res->base.target,
> > src_res->base.nr_samples),
> > +                      src_level, src_box->x, src_box->y, src_box-
> > >z,
> > +                      dst_res->id,
> > +                      tgsitargettogltarget(dst_res->base.target,
> > dst_res->base.nr_samples),
> > +                      dst_level, dstx, dsty, dstz,
> > +                      src_box->width, src_box->height,src_box-
> > >depth);
> > +}
> > +
> > +
> >  void vrend_renderer_resource_copy_region(struct vrend_context
> > *ctx,
> >                                           uint32_t dst_handle,
> > uint32_t dst_level,
> >                                           uint32_t dstx, uint32_t
> > dsty, uint32_t dstz,
> > @@ -6292,11 +6317,20 @@ void
> > vrend_renderer_resource_copy_region(struct vrend_context *ctx,
> >        return;
> >     }
> > 
> > +   if (vrend_state.have_copy_image) {
> > +      const struct util_format_description *src_desc =
> > util_format_description(src_res->base.format);
> > +      const struct util_format_description *dst_desc =
> > util_format_description(dst_res->base.format);
> > +      if (util_is_format_compatible(src_desc,dst_desc)) {
> > +         vrend_copy_sub_image(src_res, dst_res, src_level,
> > src_box,
> > +                              dst_level, dstx, dsty, dstz);
> > +         return;
> > +      }
> > +   }
> > +
> >     if (!vrend_format_can_render(src_res->base.format) ||
> >         !vrend_format_can_render(dst_res->base.format)) {
> >        vrend_resource_copy_fallback(ctx, src_res, dst_res,
> > dst_level, dstx,
> >                                     dsty, dstz, src_level,
> > src_box);
> > -
> >        return;
> >     }
> > 
> > @@ -6568,7 +6602,27 @@ void vrend_renderer_blit(struct
> > vrend_context *ctx,
> >     if (info->render_condition_enable == false)
> >        vrend_pause_render_condition(ctx, true);
> > 
> > -   vrend_renderer_blit_int(ctx, src_res, dst_res, info);
> > +   /* The gallium blit function can be called for a general blit
> > that may
> > +    * scale and convert the data or for a memcopy blit. In the
> > latter case
> > +    * the info->formats are equal and set to one of the canonical
> > formats.
> > +    * In addition some other restrictions apply. For this latter
> > case use the
> > +    * glCopyImageSubData function.
> > +    */
> > +   if (vrend_state.have_copy_image &&
> > +       vrend_is_canonical_format(info->src.format) &&
> > +       (info->src.format == info->dst.format) &&
> > +       !info->scissor_enable && (info->filter ==
> > PIPE_TEX_FILTER_NEAREST) &&
> > +       !info->alpha_blend && (info->mask == PIPE_MASK_RGBA) &&
> > +       (src_res->base.nr_samples == dst_res->base.nr_samples) &&
> > +       info->src.box.width == info->dst.box.width &&
> > +       info->src.box.height == info->dst.box.height &&
> > +       info->src.box.depth == info->dst.box.depth) {
> > +      vrend_copy_sub_image(src_res, dst_res, info->src.level,
> > &info->src.box,
> > +                           info->dst.level, info->dst.box.x, info-
> > >dst.box.y,
> > +                           info->dst.box.z);
> > +   } else {
> > +       vrend_renderer_blit_int(ctx, src_res, dst_res, info);
> > +   }
> 
> Hmm, do the GLES31 tests fail when doing blit framebuffer or during
> the GL fallback?  
The framebuffer blits usually fail because the formats are not
compatible (for a blit source and target both either have to be float,
or, if they are integer, then they have to be of the same signedness). 
Also, a blit does _convert_, e.g. a R16 format copied onto a R8G8
format will only fill the red destination component and set the green
component to zero. However, copy_image is supposed to do the equivalent
of a memcopy. 

In the GL path one could probably fix this with the right shaders, but
there one still would have to deal with sRGB <-> RGB copies, because
sRGB is normally converted by the texturing hardware (if I understood
this right), and a shader would either have to undo this, or emulate it
(depending on the copy direction).

> Since this requires GLES32 features, but an important use case is
> GLES31 on GLES31.  Would it be possible to fix those cases without
> copy_sub_image??  
I have to admid, I kind of ignored this, but now that you point it out:
the reason why I came up with using glCopyImageSubData is because
that's what is actually used by the tests I'm trying to fix with these
patches, and these are from the GLES31 set. So if glCopyImageSubData is
not available on the host, one can not expect these tests to pass,
right?  In any case, does the hardware you're using not expose one of
the *_copy_image extensions? 

Apart from that, when texture_view lands, it might also be possible to
work around a missing glCopyImageSubData, because a texture view can be
used to make a texture with one internal format pose as another one,
and then one can blit without converting. I already started to review
Dave's texture view patches, and when they land I'll try to come up
with a code path that doesn't require glCopyImageSubData. 

Best, 
Gert
Gert Wollny June 26, 2018, 9:53 a.m.
Am Dienstag, den 26.06.2018, 09:10 +0200 schrieb Gert Wollny:
> 
[...]
> Apart from that, when texture_view lands, it might also be possible
> to work around a missing glCopyImageSubData, ... 
Only that glTextureView is also not part of GLES 3.1 ...
Alexandros Frantzis June 26, 2018, 11:30 a.m.
On Tue, Jun 26, 2018 at 09:10:16AM +0200, Gert Wollny wrote:
> Am Montag, den 25.06.2018, 20:52 -0700 schrieb Gurchetan Singh:
> > On Wed, Jun 20, 2018 at 2:03 AM Gert Wollny <gert.wollny@collabora.co
> > m> wrote:
> > > 
> > > When the host is  gles >= 3.2, gl >= 4.3 or when the extension
> > > GL_(ARB|EXT|OES)_copy_image is available, memcopy like blitting and
> > > region
> > > copying can be done for many color format combinations by using
> > > glCopyImageSubData.
> > > 
> > > This fixes a number of tests from the subset
> > >   dEQP-GLES31.functional.copy_image.non_compressed.viewclass_*
> > > 
> > > Signed-off-by: Gert Wollny <gert.wollny@collabora.com>
> > > ---
> > >  src/vrend_formats.c  | 25 ++++++++++++++++++++++
> > >  src/vrend_renderer.c | 58
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++--
> > >  src/vrend_renderer.h |  2 ++
> > >  3 files changed, 83 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/vrend_formats.c b/src/vrend_formats.c
> > > index eb9f217..d1653a4 100644
> > > --- a/src/vrend_formats.c
> > > +++ b/src/vrend_formats.c
> > > @@ -443,3 +443,28 @@ void vrend_build_format_list_gles(void)
> > >     */
> > >    add_formats(gles_z32_format);
> > >  }
> > > +
> > > +bool vrend_is_canonical_format(enum pipe_format format)
> > > +{
> > > +   /* These are the canonical formats that are set by gallium */
> > > +
> > > +   switch (format) {
> > > +   case PIPE_FORMAT_R32G32B32A32_UINT:
> > > +   case PIPE_FORMAT_R32G32B32_UINT:
> > > +   case PIPE_FORMAT_R32G32_UINT:
> > > +   case PIPE_FORMAT_R16G16B16A16_UINT:
> > > +   case PIPE_FORMAT_R16G16B16_UINT:
> > > +   case PIPE_FORMAT_R8G8B8A8_UNORM:
> > > +   case PIPE_FORMAT_A8B8G8R8_UNORM:
> > > +   case PIPE_FORMAT_R16G16_UNORM:
> > > +   case PIPE_FORMAT_R32_UINT:
> > > +   case PIPE_FORMAT_R8G8B8_UINT:
> > > +   case PIPE_FORMAT_G8R8_UNORM:
> > > +   case PIPE_FORMAT_R8G8_UNORM:
> > > +   case PIPE_FORMAT_R16_UINT:
> > > +   case PIPE_FORMAT_R8_UINT:
> > > +      return true;
> > > +   default:
> > > +      return false;
> > > +   }
> > > +}
> > 
> > What do you mean by "canonical" formats?  st_cb_copyimage.c makes
> > some references to canonical formats, but doesn't really list it like
> > this as this patch does.
> 
> In st_cb_copyimage.c get_canonocal_format the various format names are
> mapped to these PIPE_FORMAT_* types, here I check whether the the
> current format is one of those that is returned by this function,
> that's why I thought the name would be apropriated. 
> 
> > 
> > 
> > > \ No newline at end of file
> > > diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c
> > > index b769e8a..8690eb2 100644
> > > --- a/src/vrend_renderer.c
> > > +++ b/src/vrend_renderer.c
> > > @@ -118,6 +118,7 @@ struct global_renderer_state {
> > >     bool have_polygon_offset_clamp;
> > >     bool have_texture_storage;
> > >     bool have_tessellation;
> > > +   bool have_copy_image;
> > > 
> > >     /* these appeared broken on at least one driver */
> > >     bool use_explicit_locations;
> > > @@ -4449,6 +4450,12 @@ int vrend_renderer_init(struct vrend_if_cbs
> > > *cbs, uint32_t flags)
> > >     if (gl_ver >= 46 ||
> > > epoxy_has_gl_extension("GL_ARB_polygon_offset_clamp"))
> > >        vrend_state.have_polygon_offset_clamp = true;
> > > 
> > > +   if (gl_ver >= 43 || (gles && gl_ver >= 32) ||
> > > +       epoxy_has_gl_extension("GL_ARB_copy_image") ||
> > > +       epoxy_has_gl_extension("GL_EXT_copy_image") ||
> > > +       epoxy_has_gl_extension("GL_OES_copy_image"))
> > > +       vrend_state.have_copy_image = true;
> > > +
> > >     /* callbacks for when we are cleaning up the object table */
> > >     vrend_resource_set_destroy_callback(vrend_destroy_resource_obje
> > > ct);
> > >     vrend_object_set_destroy_callback(VIRGL_OBJECT_QUERY,
> > > vrend_destroy_query_object);
> > > @@ -6260,6 +6269,22 @@ static void
> > > vrend_resource_copy_fallback(struct vrend_context *ctx,
> > >     free(tptr);
> > >  }
> > > 
> > > +
> > > +static inline void
> > > +vrend_copy_sub_image(struct vrend_resource* src_res, struct
> > > vrend_resource * dst_res,
> > > +                     uint32_t src_level, const struct pipe_box
> > > *src_box,
> > > +                     uint32_t dst_level, uint32_t dstx, uint32_t
> > > dsty, uint32_t dstz)
> > > +{
> > > +   glCopyImageSubData(src_res->id,
> > > +                      tgsitargettogltarget(src_res->base.target,
> > > src_res->base.nr_samples),
> > > +                      src_level, src_box->x, src_box->y, src_box-
> > > >z,
> > > +                      dst_res->id,
> > > +                      tgsitargettogltarget(dst_res->base.target,
> > > dst_res->base.nr_samples),
> > > +                      dst_level, dstx, dsty, dstz,
> > > +                      src_box->width, src_box->height,src_box-
> > > >depth);
> > > +}
> > > +
> > > +
> > >  void vrend_renderer_resource_copy_region(struct vrend_context
> > > *ctx,
> > >                                           uint32_t dst_handle,
> > > uint32_t dst_level,
> > >                                           uint32_t dstx, uint32_t
> > > dsty, uint32_t dstz,
> > > @@ -6292,11 +6317,20 @@ void
> > > vrend_renderer_resource_copy_region(struct vrend_context *ctx,
> > >        return;
> > >     }
> > > 
> > > +   if (vrend_state.have_copy_image) {
> > > +      const struct util_format_description *src_desc =
> > > util_format_description(src_res->base.format);
> > > +      const struct util_format_description *dst_desc =
> > > util_format_description(dst_res->base.format);
> > > +      if (util_is_format_compatible(src_desc,dst_desc)) {
> > > +         vrend_copy_sub_image(src_res, dst_res, src_level,
> > > src_box,
> > > +                              dst_level, dstx, dsty, dstz);
> > > +         return;
> > > +      }
> > > +   }
> > > +
> > >     if (!vrend_format_can_render(src_res->base.format) ||
> > >         !vrend_format_can_render(dst_res->base.format)) {
> > >        vrend_resource_copy_fallback(ctx, src_res, dst_res,
> > > dst_level, dstx,
> > >                                     dsty, dstz, src_level,
> > > src_box);
> > > -
> > >        return;
> > >     }
> > > 
> > > @@ -6568,7 +6602,27 @@ void vrend_renderer_blit(struct
> > > vrend_context *ctx,
> > >     if (info->render_condition_enable == false)
> > >        vrend_pause_render_condition(ctx, true);
> > > 
> > > -   vrend_renderer_blit_int(ctx, src_res, dst_res, info);
> > > +   /* The gallium blit function can be called for a general blit
> > > that may
> > > +    * scale and convert the data or for a memcopy blit. In the
> > > latter case
> > > +    * the info->formats are equal and set to one of the canonical
> > > formats.
> > > +    * In addition some other restrictions apply. For this latter
> > > case use the
> > > +    * glCopyImageSubData function.
> > > +    */
> > > +   if (vrend_state.have_copy_image &&
> > > +       vrend_is_canonical_format(info->src.format) &&
> > > +       (info->src.format == info->dst.format) &&
> > > +       !info->scissor_enable && (info->filter ==
> > > PIPE_TEX_FILTER_NEAREST) &&
> > > +       !info->alpha_blend && (info->mask == PIPE_MASK_RGBA) &&
> > > +       (src_res->base.nr_samples == dst_res->base.nr_samples) &&
> > > +       info->src.box.width == info->dst.box.width &&
> > > +       info->src.box.height == info->dst.box.height &&
> > > +       info->src.box.depth == info->dst.box.depth) {
> > > +      vrend_copy_sub_image(src_res, dst_res, info->src.level,
> > > &info->src.box,
> > > +                           info->dst.level, info->dst.box.x, info-
> > > >dst.box.y,
> > > +                           info->dst.box.z);
> > > +   } else {
> > > +       vrend_renderer_blit_int(ctx, src_res, dst_res, info);
> > > +   }
> > 
> > Hmm, do the GLES31 tests fail when doing blit framebuffer or during
> > the GL fallback?  
> The framebuffer blits usually fail because the formats are not
> compatible (for a blit source and target both either have to be float,
> or, if they are integer, then they have to be of the same signedness). 
> Also, a blit does _convert_, e.g. a R16 format copied onto a R8G8
> format will only fill the red destination component and set the green
> component to zero. However, copy_image is supposed to do the equivalent
> of a memcopy. 
> 
> In the GL path one could probably fix this with the right shaders, but
> there one still would have to deal with sRGB <-> RGB copies, because
> sRGB is normally converted by the texturing hardware (if I understood
> this right), and a shader would either have to undo this, or emulate it
> (depending on the copy direction).

Hi,

Concerning sRGB, sample values from an sRGB texture are converted to
linear space automatically when read in a shader. Then, if
GL_FRAMEBUFFER_SRGB is enabled, the values written by the shader to an
sRGB target are converted to sRGB before the final write (after
blending).

To avoid/revert the color space conversions we would need to:

1. For the sRGB->RGB case, convert from linear to sRGB in the shader
2. For the RGB->sRGB case, disable GL_FRAMEBUFFER_SRGB

Beyond sRGB, there are other behaviors introduced by the sampling logic
that we would have to avoid/revert in order to get a memcpy-like result,
and with the various format combinations it could get tricky.

> > Since this requires GLES32 features, but an important use case is
> > GLES31 on GLES31.  Would it be possible to fix those cases without
> > copy_sub_image??  
> I have to admid, I kind of ignored this, but now that you point it out:
> the reason why I came up with using glCopyImageSubData is because
> that's what is actually used by the tests I'm trying to fix with these
> patches, and these are from the GLES31 set. So if glCopyImageSubData is
> not available on the host, one can not expect these tests to pass,
> right?  In any case, does the hardware you're using not expose one of
> the *_copy_image extensions? 

Since glCopyImageSubData is GLES 3.2+ (or extension) feature, I agree
that it's reasonable to expect a 3.1 guest to support this only if the
host already supports this feature. Given the feasibility/complexity of
implementing this with 3.1-only features, unless there is a very
compelling reason to do so, I am not convinced it is worth the trouble.

Thanks,
Alexandros
Gurchetan Singh June 26, 2018, 4:27 p.m.
On Tue, Jun 26, 2018 at 12:10 AM Gert Wollny <gert.wollny@collabora.com> wrote:
>
> Am Montag, den 25.06.2018, 20:52 -0700 schrieb Gurchetan Singh:
> > On Wed, Jun 20, 2018 at 2:03 AM Gert Wollny <gert.wollny@collabora.co
> > m> wrote:
> > >
> > > When the host is  gles >= 3.2, gl >= 4.3 or when the extension
> > > GL_(ARB|EXT|OES)_copy_image is available, memcopy like blitting and
> > > region
> > > copying can be done for many color format combinations by using
> > > glCopyImageSubData.
> > >
> > > This fixes a number of tests from the subset
> > >   dEQP-GLES31.functional.copy_image.non_compressed.viewclass_*
> > >
> > > Signed-off-by: Gert Wollny <gert.wollny@collabora.com>
> > > ---
> > >  src/vrend_formats.c  | 25 ++++++++++++++++++++++
> > >  src/vrend_renderer.c | 58
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++--
> > >  src/vrend_renderer.h |  2 ++
> > >  3 files changed, 83 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/vrend_formats.c b/src/vrend_formats.c
> > > index eb9f217..d1653a4 100644
> > > --- a/src/vrend_formats.c
> > > +++ b/src/vrend_formats.c
> > > @@ -443,3 +443,28 @@ void vrend_build_format_list_gles(void)
> > >     */
> > >    add_formats(gles_z32_format);
> > >  }
> > > +
> > > +bool vrend_is_canonical_format(enum pipe_format format)
> > > +{
> > > +   /* These are the canonical formats that are set by gallium */
> > > +
> > > +   switch (format) {
> > > +   case PIPE_FORMAT_R32G32B32A32_UINT:
> > > +   case PIPE_FORMAT_R32G32B32_UINT:
> > > +   case PIPE_FORMAT_R32G32_UINT:
> > > +   case PIPE_FORMAT_R16G16B16A16_UINT:
> > > +   case PIPE_FORMAT_R16G16B16_UINT:
> > > +   case PIPE_FORMAT_R8G8B8A8_UNORM:
> > > +   case PIPE_FORMAT_A8B8G8R8_UNORM:
> > > +   case PIPE_FORMAT_R16G16_UNORM:
> > > +   case PIPE_FORMAT_R32_UINT:
> > > +   case PIPE_FORMAT_R8G8B8_UINT:
> > > +   case PIPE_FORMAT_G8R8_UNORM:
> > > +   case PIPE_FORMAT_R8G8_UNORM:
> > > +   case PIPE_FORMAT_R16_UINT:
> > > +   case PIPE_FORMAT_R8_UINT:
> > > +      return true;
> > > +   default:
> > > +      return false;
> > > +   }
> > > +}
> >
> > What do you mean by "canonical" formats?  st_cb_copyimage.c makes
> > some references to canonical formats, but doesn't really list it like
> > this as this patch does.
>
> In st_cb_copyimage.c get_canonocal_format the various format names are
> mapped to these PIPE_FORMAT_* types, here I check whether the the
> current format is one of those that is returned by this function,
> that's why I thought the name would be apropriated.
>
> >
> >
> > > \ No newline at end of file
> > > diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c
> > > index b769e8a..8690eb2 100644
> > > --- a/src/vrend_renderer.c
> > > +++ b/src/vrend_renderer.c
> > > @@ -118,6 +118,7 @@ struct global_renderer_state {
> > >     bool have_polygon_offset_clamp;
> > >     bool have_texture_storage;
> > >     bool have_tessellation;
> > > +   bool have_copy_image;
> > >
> > >     /* these appeared broken on at least one driver */
> > >     bool use_explicit_locations;
> > > @@ -4449,6 +4450,12 @@ int vrend_renderer_init(struct vrend_if_cbs
> > > *cbs, uint32_t flags)
> > >     if (gl_ver >= 46 ||
> > > epoxy_has_gl_extension("GL_ARB_polygon_offset_clamp"))
> > >        vrend_state.have_polygon_offset_clamp = true;
> > >
> > > +   if (gl_ver >= 43 || (gles && gl_ver >= 32) ||
> > > +       epoxy_has_gl_extension("GL_ARB_copy_image") ||
> > > +       epoxy_has_gl_extension("GL_EXT_copy_image") ||
> > > +       epoxy_has_gl_extension("GL_OES_copy_image"))
> > > +       vrend_state.have_copy_image = true;
> > > +
> > >     /* callbacks for when we are cleaning up the object table */
> > >     vrend_resource_set_destroy_callback(vrend_destroy_resource_obje
> > > ct);
> > >     vrend_object_set_destroy_callback(VIRGL_OBJECT_QUERY,
> > > vrend_destroy_query_object);
> > > @@ -6260,6 +6269,22 @@ static void
> > > vrend_resource_copy_fallback(struct vrend_context *ctx,
> > >     free(tptr);
> > >  }
> > >
> > > +
> > > +static inline void
> > > +vrend_copy_sub_image(struct vrend_resource* src_res, struct
> > > vrend_resource * dst_res,
> > > +                     uint32_t src_level, const struct pipe_box
> > > *src_box,
> > > +                     uint32_t dst_level, uint32_t dstx, uint32_t
> > > dsty, uint32_t dstz)
> > > +{
> > > +   glCopyImageSubData(src_res->id,
> > > +                      tgsitargettogltarget(src_res->base.target,
> > > src_res->base.nr_samples),
> > > +                      src_level, src_box->x, src_box->y, src_box-
> > > >z,
> > > +                      dst_res->id,
> > > +                      tgsitargettogltarget(dst_res->base.target,
> > > dst_res->base.nr_samples),
> > > +                      dst_level, dstx, dsty, dstz,
> > > +                      src_box->width, src_box->height,src_box-
> > > >depth);
> > > +}
> > > +
> > > +
> > >  void vrend_renderer_resource_copy_region(struct vrend_context
> > > *ctx,
> > >                                           uint32_t dst_handle,
> > > uint32_t dst_level,
> > >                                           uint32_t dstx, uint32_t
> > > dsty, uint32_t dstz,
> > > @@ -6292,11 +6317,20 @@ void
> > > vrend_renderer_resource_copy_region(struct vrend_context *ctx,
> > >        return;
> > >     }
> > >
> > > +   if (vrend_state.have_copy_image) {
> > > +      const struct util_format_description *src_desc =
> > > util_format_description(src_res->base.format);
> > > +      const struct util_format_description *dst_desc =
> > > util_format_description(dst_res->base.format);
> > > +      if (util_is_format_compatible(src_desc,dst_desc)) {
> > > +         vrend_copy_sub_image(src_res, dst_res, src_level,
> > > src_box,
> > > +                              dst_level, dstx, dsty, dstz);
> > > +         return;
> > > +      }
> > > +   }
> > > +
> > >     if (!vrend_format_can_render(src_res->base.format) ||
> > >         !vrend_format_can_render(dst_res->base.format)) {
> > >        vrend_resource_copy_fallback(ctx, src_res, dst_res,
> > > dst_level, dstx,
> > >                                     dsty, dstz, src_level,
> > > src_box);
> > > -
> > >        return;
> > >     }
> > >
> > > @@ -6568,7 +6602,27 @@ void vrend_renderer_blit(struct
> > > vrend_context *ctx,
> > >     if (info->render_condition_enable == false)
> > >        vrend_pause_render_condition(ctx, true);
> > >
> > > -   vrend_renderer_blit_int(ctx, src_res, dst_res, info);
> > > +   /* The gallium blit function can be called for a general blit
> > > that may
> > > +    * scale and convert the data or for a memcopy blit. In the
> > > latter case
> > > +    * the info->formats are equal and set to one of the canonical
> > > formats.
> > > +    * In addition some other restrictions apply. For this latter
> > > case use the
> > > +    * glCopyImageSubData function.
> > > +    */
> > > +   if (vrend_state.have_copy_image &&
> > > +       vrend_is_canonical_format(info->src.format) &&
> > > +       (info->src.format == info->dst.format) &&
> > > +       !info->scissor_enable && (info->filter ==
> > > PIPE_TEX_FILTER_NEAREST) &&
> > > +       !info->alpha_blend && (info->mask == PIPE_MASK_RGBA) &&
> > > +       (src_res->base.nr_samples == dst_res->base.nr_samples) &&
> > > +       info->src.box.width == info->dst.box.width &&
> > > +       info->src.box.height == info->dst.box.height &&
> > > +       info->src.box.depth == info->dst.box.depth) {
> > > +      vrend_copy_sub_image(src_res, dst_res, info->src.level,
> > > &info->src.box,
> > > +                           info->dst.level, info->dst.box.x, info-
> > > >dst.box.y,
> > > +                           info->dst.box.z);
> > > +   } else {
> > > +       vrend_renderer_blit_int(ctx, src_res, dst_res, info);
> > > +   }
> >
> > Hmm, do the GLES31 tests fail when doing blit framebuffer or during
> > the GL fallback?
> The framebuffer blits usually fail because the formats are not
> compatible (for a blit source and target both either have to be float,
> or, if they are integer, then they have to be of the same signedness).
> Also, a blit does _convert_, e.g. a R16 format copied onto a R8G8
> format will only fill the red destination component and set the green
> component to zero. However, copy_image is supposed to do the equivalent
> of a memcopy.
>
> In the GL path one could probably fix this with the right shaders, but
> there one still would have to deal with sRGB <-> RGB copies, because
> sRGB is normally converted by the texturing hardware (if I understood
> this right), and a shader would either have to undo this, or emulate it
> (depending on the copy direction).
>
> > Since this requires GLES32 features, but an important use case is
> > GLES31 on GLES31.  Would it be possible to fix those cases without
> > copy_sub_image??
> I have to admid, I kind of ignored this, but now that you point it out:
> the reason why I came up with using glCopyImageSubData is because
> that's what is actually used by the tests I'm trying to fix with these
> patches, and these are from the GLES31 set. So if glCopyImageSubData is
> not available on the host, one can not expect these tests to pass,
> right?  In any case, does the hardware you're using not expose one of
> the *_copy_image extensions?

I found this relevant snippet in es31fCopyImageTests.cpp:

        if (!isES32 && !ctxInfo->isExtensionSupported("GL_EXT_copy_image"))
                throw tcu::NotSupportedError("Extension
GL_EXT_copy_image not supported.", "", __FILE__, __LINE__);

Does the guest advertise glCopyImageSubData when we shouldn't?  If so,
we can not advertise this and hold off on this feature (from the
Chromium's team perspective, GLES31 > Vulkan > GLES32).


>
> Apart from that, when texture_view lands, it might also be possible to
> work around a missing glCopyImageSubData, because a texture view can be
> used to make a texture with one internal format pose as another one,
> and then one can blit without converting. I already started to review
> Dave's texture view patches, and when they land I'll try to come up
> with a code path that doesn't require glCopyImageSubData.
>
> Best,
> Gert
>
Gert Wollny June 26, 2018, 4:51 p.m.
Am Dienstag, den 26.06.2018, 09:27 -0700 schrieb Gurchetan Singh:
...
> > I have to admid, I kind of ignored this, but now that you point it
> > out: the reason why I came up with using glCopyImageSubData is
> > because that's what is actually used by the tests I'm trying to fix
> > with these patches, and these are from the GLES31 set. So if
> > glCopyImageSubData is not available on the host, one can not expect
> > these tests to pass, right?  In any case, does the hardware you're
> > using not expose one of the *_copy_image extensions?
> 
> I found this relevant snippet in es31fCopyImageTests.cpp:
> 
>         if (!isES32 && !ctxInfo-
> >isExtensionSupported("GL_EXT_copy_image"))
>                 throw tcu::NotSupportedError("Extension
> GL_EXT_copy_image not supported.", "", __FILE__, __LINE__);
> 
> Does the guest advertise glCopyImageSubData when we shouldn't?  

When I run Qemu with 

  MESA_EXTENSION_OVERRIDE=-GL_ARB_copy_image 

then GL_*_copy_image is not advertised  in the guest (as it should be),
so having a code path in the host that uses this extenstion when
evailable shouldn't pose a problem, we only have to make sure that the
there are no regressions with the old code path. 

In any case, this makes me wonder why these tests are in the GLES31 set

Best, 
Gert
Gurchetan Singh June 26, 2018, 7:46 p.m.
On Tue, Jun 26, 2018 at 9:51 AM Gert Wollny <gert.wollny@collabora.com> wrote:
>
> Am Dienstag, den 26.06.2018, 09:27 -0700 schrieb Gurchetan Singh:
> ...
> > > I have to admid, I kind of ignored this, but now that you point it
> > > out: the reason why I came up with using glCopyImageSubData is
> > > because that's what is actually used by the tests I'm trying to fix
> > > with these patches, and these are from the GLES31 set. So if
> > > glCopyImageSubData is not available on the host, one can not expect
> > > these tests to pass, right?  In any case, does the hardware you're
> > > using not expose one of the *_copy_image extensions?
> >
> > I found this relevant snippet in es31fCopyImageTests.cpp:
> >
> >         if (!isES32 && !ctxInfo-
> > >isExtensionSupported("GL_EXT_copy_image"))
> >                 throw tcu::NotSupportedError("Extension
> > GL_EXT_copy_image not supported.", "", __FILE__, __LINE__);
> >
> > Does the guest advertise glCopyImageSubData when we shouldn't?
>
> When I run Qemu with
>
>   MESA_EXTENSION_OVERRIDE=-GL_ARB_copy_image
>
> then GL_*_copy_image is not advertised  in the guest (as it should be),
> so having a code path in the host that uses this extenstion when
> evailable shouldn't pose a problem, we only have to make sure that the
> there are no regressions with the old code path.

That's odd -- we don't ever query for GL_ARB_copy_image and send it
via the capability set (usually caps->v1.bset).  How does the guest
know this capability exists on the host?  Maybe Mesa in the guest
creates a GLES32 context?  I agree we should support copy_image if the
host supports it, but we also should get to the bottom of this.

> In st_cb_copyimage.c get_canonocal_format the various format names are
mapped to these PIPE_FORMAT_* types, here I check whether the the
current format is one of those that is returned by this function,
that's why I thought the name would be apropriated.

So you're trying to return the formats which get_canonical_format can
possibly return, correct?  It looks we're missing
PIPE_FORMAT_B8G8R8A8_UNORM.

Also, from swizzled_copy in st_cb_copyimage.c it seems that the
memcopy blit should work for cases (info->src.format !=
info->dst.format), from this comment:

/* BGRA8 -> R32 is realized as BGRA8 -> RGBA8        */

(There's still a lot failures with
dEQP-GLES31.functional.copy_image.mixed* and
dEQP-GLES31.functional.copy_image.compressed*).
Gert Wollny June 27, 2018, 6:11 a.m.
Am Dienstag, den 26.06.2018, 12:46 -0700 schrieb Gurchetan Singh:
> On Tue, Jun 26, 2018 at 9:51 AM Gert Wollny <gert.wollny@collabora.co
> m> wrote:
> > 
> > Am Dienstag, den 26.06.2018, 09:27 -0700 schrieb Gurchetan Singh:
> > ...
> > > > I have to admid, I kind of ignored this, but now that you point
> > > > it 
> > > > out: the reason why I came up with using glCopyImageSubData is
> > > > because that's what is actually used by the tests I'm trying to
> > > > fix
> > > > with these patches, and these are from the GLES31 set. So if
> > > > glCopyImageSubData is not available on the host, one can not
> > > > expect
> > > > these tests to pass, right?  In any case, does the hardware
> > > > you're
> > > > using not expose one of the *_copy_image extensions?
> > > 
> > > I found this relevant snippet in es31fCopyImageTests.cpp:
> > > 
> > >         if (!isES32 && !ctxInfo-
> > > > isExtensionSupported("GL_EXT_copy_image"))
> > > 
> > >                 throw tcu::NotSupportedError("Extension
> > > GL_EXT_copy_image not supported.", "", __FILE__, __LINE__);
> > > 
> > > Does the guest advertise glCopyImageSubData when we shouldn't?
> > 
> > When I run Qemu with
> > 
> >   MESA_EXTENSION_OVERRIDE=-GL_ARB_copy_image
> > 
> > then GL_*_copy_image is not advertised  in the guest (as it should
> > be), so having a code path in the host that uses this extenstion
> > when evailable shouldn't pose a problem, we only have to make sure
> > that the there are no regressions with the old code path.
> 
> That's odd -- we don't ever query for GL_ARB_copy_image and send it
> via the capability set (usually caps->v1.bset).  How does the guest
> know this capability exists on the host?  
I was wrong, I was running with the master virglrenderer/mesa trees and
they never advertise this extension. 

In the experimental one its is forced to get guest OpenGL 4.3, and
disabling it there on the host doesn't change anything in the guest.
I'll have to add this CAP to the patch and also send a patch to mesa.

> 
> > In st_cb_copyimage.c get_canonocal_format the various format names
> > are
> 
> mapped to these PIPE_FORMAT_* types, here I check whether the the
> current format is one of those that is returned by this function,
> that's why I thought the name would be apropriated.
> 
> So you're trying to return the formats which get_canonical_format can
> possibly return, correct?  
Not exactly, I'm looking at whether the format is one that is returned
from "canonical_format_from_bits", here you have for each bitcount
exactly one return value, and if they are equal then either the formats
were equal to begin with, or glCopyImageSubData was called (At least
that is my assumption). In both cases I can call glCopyImageSubData on
the host (given the other restrictions apply). 

> It looks we're missing PIPE_FORMAT_B8G8R8A8_UNORM.
I don't think so, because "see above". 

> Also, from swizzled_copy in st_cb_copyimage.c it seems that the
> memcopy blit should work for cases (info->src.format !=
> info->dst.format), from this comment:
> 
> /* BGRA8 -> R32 is realized as BGRA8 -> RGBA8        */
I think this refers to non glCopyImageSubData blits. 

> 
> (There's still a lot failures with
> dEQP-GLES31.functional.copy_image.mixed* and
> dEQP-GLES31.functional.copy_image.compressed*).
One step at a time ;) First I have to figure out the regression Dave
sees and I've seen on Intel but not on R600. 

Best, 
Gert
Gurchetan Singh June 29, 2018, 3:16 a.m.
On Tue, Jun 26, 2018 at 11:11 PM Gert Wollny <gert.wollny@collabora.com> wrote:
>
> Am Dienstag, den 26.06.2018, 12:46 -0700 schrieb Gurchetan Singh:
> > On Tue, Jun 26, 2018 at 9:51 AM Gert Wollny <gert.wollny@collabora.co
> > m> wrote:
> > >
> > > Am Dienstag, den 26.06.2018, 09:27 -0700 schrieb Gurchetan Singh:
> > > ...
> > > > > I have to admid, I kind of ignored this, but now that you point
> > > > > it
> > > > > out: the reason why I came up with using glCopyImageSubData is
> > > > > because that's what is actually used by the tests I'm trying to
> > > > > fix
> > > > > with these patches, and these are from the GLES31 set. So if
> > > > > glCopyImageSubData is not available on the host, one can not
> > > > > expect
> > > > > these tests to pass, right?  In any case, does the hardware
> > > > > you're
> > > > > using not expose one of the *_copy_image extensions?
> > > >
> > > > I found this relevant snippet in es31fCopyImageTests.cpp:
> > > >
> > > >         if (!isES32 && !ctxInfo-
> > > > > isExtensionSupported("GL_EXT_copy_image"))
> > > >
> > > >                 throw tcu::NotSupportedError("Extension
> > > > GL_EXT_copy_image not supported.", "", __FILE__, __LINE__);
> > > >
> > > > Does the guest advertise glCopyImageSubData when we shouldn't?
> > >
> > > When I run Qemu with
> > >
> > >   MESA_EXTENSION_OVERRIDE=-GL_ARB_copy_image
> > >
> > > then GL_*_copy_image is not advertised  in the guest (as it should
> > > be), so having a code path in the host that uses this extenstion
> > > when evailable shouldn't pose a problem, we only have to make sure
> > > that the there are no regressions with the old code path.
> >
> > That's odd -- we don't ever query for GL_ARB_copy_image and send it
> > via the capability set (usually caps->v1.bset).  How does the guest
> > know this capability exists on the host?
> I was wrong, I was running with the master virglrenderer/mesa trees and
> they never advertise this extension.
>
> In the experimental one its is forced to get guest OpenGL 4.3, and
> disabling it there on the host doesn't change anything in the guest.
> I'll have to add this CAP to the patch and also send a patch to mesa.
>
> >
> > > In st_cb_copyimage.c get_canonocal_format the various format names
> > > are
> >
> > mapped to these PIPE_FORMAT_* types, here I check whether the the
> > current format is one of those that is returned by this function,
> > that's why I thought the name would be apropriated.
> >
> > So you're trying to return the formats which get_canonical_format can
> > possibly return, correct?
> Not exactly, I'm looking at whether the format is one that is returned
> from "canonical_format_from_bits", here you have for each bitcount
> exactly one return value,

Why do you have:

PIPE_FORMAT_G8R8_UNORM,
PIPE_FORMAT_R8G8B8_UINT,
PIPE_FORMAT_A8B8G8R8_UNORM,
PIPE_FORMAT_R16G16B16_UINT,
PIPE_FORMAT_R32G32B32_UINT

when they can't be returned by canonical_format_from_bits?

> and if they are equal then either the formats
> were equal to begin with, or glCopyImageSubData was called (At least
> that is my assumption).

Does pipe->blit() expect a pure memcpy (like glCopyImageSubData
performs -- R32 to RGBA8 pure copy) or a per-channel memcpy (R32 to
RGBA8 only fills in R component)?  It's not completely clear from
reading pipe_context.h or struct pipe_blit_info:

   /* Optimal hardware path for blitting pixels.
    * Scaling, format conversion, up- and downsampling (resolve) are allowed.
    */
   void (*blit)(struct pipe_context *pipe,
                const struct pipe_blit_info *info);

glCopyImageSubData is completely different:

"CopyImageSubData does not perform general-purpose conversions
    such as scaling, resizing, blending, color-space, or format
    conversions. "

How does Gallium differentiate between the two for drivers?

> In both cases I can call glCopyImageSubData on
> the host (given the other restrictions apply)
>> > It looks we're missing PIPE_FORMAT_B8G8R8A8_UNORM.
> I don't think so, because "see above".
>
> > Also, from swizzled_copy in st_cb_copyimage.c it seems that the
> > memcopy blit should work for cases (info->src.format !=
> > info->dst.format), from this comment:
> >
> > /* BGRA8 -> R32 is realized as BGRA8 -> RGBA8        */
> I think this refers to non glCopyImageSubData blits.





> >
> > (There's still a lot failures with
> > dEQP-GLES31.functional.copy_image.mixed* and
> > dEQP-GLES31.functional.copy_image.compressed*).
> One step at a time ;) First I have to figure out the regression Dave
> sees and I've seen on Intel but not on R600.
>
> Best,
> Gert
Gert Wollny June 29, 2018, 6:34 a.m.
Am Donnerstag, den 28.06.2018, 20:16 -0700 schrieb Gurchetan Singh:
> 
> > Not exactly, I'm looking at whether the format is one that is
> > returned from "canonical_format_from_bits", here you have for each
> > bitcount exactly one return value,
> 
> Why do you have:
> 
> PIPE_FORMAT_G8R8_UNORM,
> PIPE_FORMAT_R8G8B8_UINT,
> PIPE_FORMAT_A8B8G8R8_UNORM,
> PIPE_FORMAT_R16G16B16_UINT,
> PIPE_FORMAT_R32G32B32_UINT
> 
> when they can't be returned by canonical_format_from_bits?
Good catch, that is left over from my initial attempt to figure out
which textures can be copied by using glCopyImageSubData. I think that
might be one reason for the piglit regressions that I still see. 

> > Does pipe->blit() expect a pure memcpy (like glCopyImageSubData
> performs -- R32 to RGBA8 pure copy) or a per-channel memcpy (R32 to
> RGBA8 only fills in R component)?  It's not completely clear from
> reading pipe_context.h or struct pipe_blit_info:
> 
>    /* Optimal hardware path for blitting pixels.
>     * Scaling, format conversion, up- and downsampling (resolve) are
> allowed.
>     */
>    void (*blit)(struct pipe_context *pipe,
>                 const struct pipe_blit_info *info);
> 
> glCopyImageSubData is completely different:
> 
> "CopyImageSubData does not perform general-purpose conversions
>     such as scaling, resizing, blending, color-space, or format
>     conversions. "
> 
> How does Gallium differentiate between the two for drivers?
This is a very good question. My initial shot at making the copy_image
tests pass was simply comparing bit counts (hence the leftovers you
comented above), which, of course, introduced many regressions with
other tests. Then I figured out that the  blit that's called from
glCopyImageSubData seems to be the only one that change the
info.*.format to a canonical format. And one can check for not
resizing, equal sample counts etc (I think that clipping is already
taken care of on the guest size).

Yeah, there are still some piglits failing but I'm getting there ... 

Best, 
Gert