[virglrenderer-devel] [PATCH 2/3] vrend: If available use glCopyImageSubData to execute memcopy like blits
Gurchetan Singh
gurchetansingh at chromium.org
Tue Jun 26 16:27:03 UTC 2018
On Tue, Jun 26, 2018 at 12:10 AM Gert Wollny <gert.wollny at 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 at 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 at 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
>
More information about the virglrenderer-devel
mailing list