[Mesa-dev] [PATCH 2/7] intel: Remove the pbo zero-copy code.
Ian Romanick
idr at freedesktop.org
Thu Sep 22 09:54:43 PDT 2011
On 09/21/2011 10:15 AM, Eric Anholt wrote:
> There were notes about the possibility of slowdowns due to zcopy from
> a PBO due to thrashing around of the region. Slowdowns are even more
> likely now that textures are generally tiled, which a zcopy wouldn't
> get. Additionally, there were no checks on the buffer size to ensure
> that the hardware-required rounding was present, which could result in
> GPU hangs on large zcopy PBOs.
> ---
> src/mesa/drivers/dri/intel/intel_buffer_objects.c | 45 --------
> src/mesa/drivers/dri/intel/intel_buffer_objects.h | 12 --
> src/mesa/drivers/dri/intel/intel_regions.c | 119 ---------------------
> src/mesa/drivers/dri/intel/intel_regions.h | 11 --
> src/mesa/drivers/dri/intel/intel_tex_image.c | 60 -----------
> 5 files changed, 0 insertions(+), 247 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/intel/intel_buffer_objects.c b/src/mesa/drivers/dri/intel/intel_buffer_objects.c
> index d35a50e..4df2d76 100644
> --- a/src/mesa/drivers/dri/intel/intel_buffer_objects.c
> +++ b/src/mesa/drivers/dri/intel/intel_buffer_objects.c
> @@ -79,30 +79,6 @@ intel_bufferobj_alloc(struct gl_context * ctx, GLuint name, GLenum target)
> return&obj->Base;
> }
>
> -/* Break the COW tie to the region. The region gets to keep the data.
> - */
> -void
> -intel_bufferobj_release_region(struct intel_buffer_object *intel_obj)
> -{
> - assert(intel_obj->region->buffer == intel_obj->buffer);
> - intel_obj->region->pbo = NULL;
> - intel_obj->region = NULL;
> -
> - release_buffer(intel_obj);
> -}
> -
> -/* Break the COW tie to the region. Both the pbo and the region end
> - * up with a copy of the data.
> - */
> -void
> -intel_bufferobj_cow(struct intel_context *intel,
> - struct intel_buffer_object *intel_obj)
> -{
> - assert(intel_obj->region);
> - intel_region_cow(intel, intel_obj->region);
> -}
> -
> -
> /**
> * Deallocate/free a vertex/pixel buffer object.
> * Called via glDeleteBuffersARB().
> @@ -122,9 +98,6 @@ intel_bufferobj_free(struct gl_context * ctx, struct gl_buffer_object *obj)
> intel_bufferobj_unmap(ctx, obj);
>
> free(intel_obj->sys_buffer);
> - if (intel_obj->region) {
> - intel_bufferobj_release_region(intel_obj);
> - }
>
> drm_intel_bo_unreference(intel_obj->buffer);
> free(intel_obj);
> @@ -160,9 +133,6 @@ intel_bufferobj_data(struct gl_context * ctx,
>
> assert(!obj->Pointer); /* Mesa should have unmapped it */
>
> - if (intel_obj->region)
> - intel_bufferobj_release_region(intel_obj);
> -
> if (intel_obj->buffer != NULL)
> release_buffer(intel_obj);
>
> @@ -219,9 +189,6 @@ intel_bufferobj_subdata(struct gl_context * ctx,
>
> assert(intel_obj);
>
> - if (intel_obj->region)
> - intel_bufferobj_cow(intel, intel_obj);
> -
> /* If we have a single copy in system memory, update that */
> if (intel_obj->sys_buffer) {
> if (intel_obj->source)
> @@ -347,9 +314,6 @@ intel_bufferobj_map_range(struct gl_context * ctx,
> intel_obj->sys_buffer = NULL;
> }
>
> - if (intel_obj->region)
> - intel_bufferobj_cow(intel, intel_obj);
> -
> /* If the mapping is synchronized with other GL operations, flush
> * the batchbuffer so that GEM knows about the buffer access for later
> * syncing.
> @@ -510,15 +474,6 @@ intel_bufferobj_buffer(struct intel_context *intel,
> struct intel_buffer_object *intel_obj,
> GLuint flag)
> {
> - if (intel_obj->region) {
> - if (flag == INTEL_WRITE_PART)
> - intel_bufferobj_cow(intel, intel_obj);
> - else if (flag == INTEL_WRITE_FULL) {
> - intel_bufferobj_release_region(intel_obj);
> - intel_bufferobj_alloc_buffer(intel, intel_obj);
> - }
> - }
> -
> if (intel_obj->source)
> release_buffer(intel_obj);
>
> diff --git a/src/mesa/drivers/dri/intel/intel_buffer_objects.h b/src/mesa/drivers/dri/intel/intel_buffer_objects.h
> index d75cdbf..b174e93 100644
> --- a/src/mesa/drivers/dri/intel/intel_buffer_objects.h
> +++ b/src/mesa/drivers/dri/intel/intel_buffer_objects.h
> @@ -31,7 +31,6 @@
> #include "main/mtypes.h"
>
> struct intel_context;
> -struct intel_region;
> struct gl_buffer_object;
>
>
> @@ -47,10 +46,6 @@ struct intel_buffer_object
> /** System memory buffer data, if not using a BO to store the data. */
> void *sys_buffer;
>
> - struct intel_region *region; /* Is there a zero-copy texture
> - associated with this (pixel)
> - buffer object? */
> -
> drm_intel_bo *range_map_bo;
> void *range_map_buffer;
> unsigned int range_map_offset;
> @@ -102,11 +97,4 @@ intel_buffer_object(struct gl_buffer_object *obj)
> return (struct intel_buffer_object *) obj;
> }
>
> -/* Helpers for zerocopy image uploads. See also intel_regions.h:
> - */
> -void intel_bufferobj_cow(struct intel_context *intel,
> - struct intel_buffer_object *intel_obj);
> -void intel_bufferobj_release_region(struct intel_buffer_object *intel_obj);
> -
> -
> #endif
> diff --git a/src/mesa/drivers/dri/intel/intel_regions.c b/src/mesa/drivers/dri/intel/intel_regions.c
> index 4c4945c..4d4ddd9 100644
> --- a/src/mesa/drivers/dri/intel/intel_regions.c
> +++ b/src/mesa/drivers/dri/intel/intel_regions.c
> @@ -115,9 +115,6 @@ intel_region_map(struct intel_context *intel, struct intel_region *region)
>
> _DBG("%s %p\n", __FUNCTION__, region);
> if (!region->map_refcount++) {
> - if (region->pbo)
> - intel_region_cow(intel, region);
> -
> if (region->tiling != I915_TILING_NONE)
> drm_intel_gem_bo_map_gtt(region->buffer);
> else
> @@ -295,9 +292,6 @@ intel_region_release(struct intel_region **region_handle)
> if (region->refcount == 0) {
> assert(region->map_refcount == 0);
>
> - if (region->pbo)
> - region->pbo->region = NULL;
> - region->pbo = NULL;
> drm_intel_bo_unreference(region->buffer);
>
> if (region->name> 0)
> @@ -364,14 +358,6 @@ intel_region_data(struct intel_context *intel,
> if (intel == NULL)
> return;
>
> - if (dst->pbo) {
> - if (dstx == 0&&
> - dsty == 0&& width == dst->pitch&& height == dst->height)
> - intel_region_release_pbo(intel, dst);
> - else
> - intel_region_cow(intel, dst);
> - }
> -
> intel_prepare_render(intel);
>
> _mesa_copy_rect(intel_region_map(intel, dst) + dst_offset,
> @@ -403,14 +389,6 @@ intel_region_copy(struct intel_context *intel,
> if (intel == NULL)
> return GL_FALSE;
>
> - if (dst->pbo) {
> - if (dstx == 0&&
> - dsty == 0&& width == dst->pitch&& height == dst->height)
> - intel_region_release_pbo(intel, dst);
> - else
> - intel_region_cow(intel, dst);
> - }
> -
> assert(src->cpp == dst->cpp);
>
> if (flip)
> @@ -424,106 +402,9 @@ intel_region_copy(struct intel_context *intel,
> logicop);
> }
>
> -/* Attach to a pbo, discarding our data. Effectively zero-copy upload
> - * the pbo's data.
> - */
> -void
> -intel_region_attach_pbo(struct intel_context *intel,
> - struct intel_region *region,
> - struct intel_buffer_object *pbo)
> -{
> - drm_intel_bo *buffer;
> -
> - if (region->pbo == pbo)
> - return;
> -
> - _DBG("%s %p %p\n", __FUNCTION__, region, pbo);
> -
> - /* If there is already a pbo attached, break the cow tie now.
> - * Don't call intel_region_release_pbo() as that would
> - * unnecessarily allocate a new buffer we would have to immediately
> - * discard.
> - */
> - if (region->pbo) {
> - region->pbo->region = NULL;
> - region->pbo = NULL;
> - }
> -
> - if (region->buffer) {
> - drm_intel_bo_unreference(region->buffer);
> - region->buffer = NULL;
> - }
> -
> - /* make sure pbo has a buffer of its own */
> - buffer = intel_bufferobj_buffer(intel, pbo, INTEL_WRITE_FULL);
> -
> - region->pbo = pbo;
> - region->pbo->region = region;
> - drm_intel_bo_reference(buffer);
> - region->buffer = buffer;
> - region->tiling = I915_TILING_NONE;
> -}
> -
> -
> -/* Break the COW tie to the pbo and allocate a new buffer.
> - * The pbo gets to keep the data.
> - */
> -void
> -intel_region_release_pbo(struct intel_context *intel,
> - struct intel_region *region)
> -{
> - _DBG("%s %p\n", __FUNCTION__, region);
> - assert(region->buffer == region->pbo->buffer);
> - region->pbo->region = NULL;
> - region->pbo = NULL;
> - drm_intel_bo_unreference(region->buffer);
> - region->buffer = NULL;
> -
> - region->buffer = drm_intel_bo_alloc(intel->bufmgr, "region",
> - region->pitch * region->cpp *
> - region->height,
> - 64);
> -}
> -
> -/* Break the COW tie to the pbo. Both the pbo and the region end up
> - * with a copy of the data.
> - */
> -void
> -intel_region_cow(struct intel_context *intel, struct intel_region *region)
> -{
> - struct intel_buffer_object *pbo = region->pbo;
> - GLboolean ok;
> -
> - intel_region_release_pbo(intel, region);
> -
> - assert(region->cpp * region->pitch * region->height == pbo->Base.Size);
> -
> - _DBG("%s %p (%d bytes)\n", __FUNCTION__, region, (int)pbo->Base.Size);
> -
> - /* Now blit from the texture buffer to the new buffer:
> - */
> -
> - intel_prepare_render(intel);
> - ok = intelEmitCopyBlit(intel,
> - region->cpp,
> - region->pitch, pbo->buffer, 0, region->tiling,
> - region->pitch, region->buffer, 0, region->tiling,
> - 0, 0, 0, 0,
> - region->pitch, region->height,
> - GL_COPY);
> - assert(ok);
> -}
> -
> drm_intel_bo *
> intel_region_buffer(struct intel_context *intel,
> struct intel_region *region, GLuint flag)
Does this function need to exist? If so, can it lose the intel and flag
parameters?
> {
> - if (region->pbo) {
> - if (flag == INTEL_WRITE_PART)
> - intel_region_cow(intel, region);
> - else if (flag == INTEL_WRITE_FULL)
> - intel_region_release_pbo(intel, region);
> - }
> -
> return region->buffer;
> }
> diff --git a/src/mesa/drivers/dri/intel/intel_regions.h b/src/mesa/drivers/dri/intel/intel_regions.h
> index 91f7121..f3f6a07 100644
> --- a/src/mesa/drivers/dri/intel/intel_regions.h
> +++ b/src/mesa/drivers/dri/intel/intel_regions.h
> @@ -63,7 +63,6 @@ struct intel_region
> GLuint map_refcount; /**< Reference count for mapping */
>
> uint32_t tiling; /**< Which tiling mode the region is in */
> - struct intel_buffer_object *pbo; /* zero-copy uploads */
>
> uint32_t name; /**< Global name for the bo */
> struct intel_screen *screen;
> @@ -125,16 +124,6 @@ intel_region_copy(struct intel_context *intel,
> GLboolean flip,
> GLenum logicop);
>
> -/* Helpers for zerocopy uploads, particularly texture image uploads:
> - */
> -void intel_region_attach_pbo(struct intel_context *intel,
> - struct intel_region *region,
> - struct intel_buffer_object *pbo);
> -void intel_region_release_pbo(struct intel_context *intel,
> - struct intel_region *region);
> -void intel_region_cow(struct intel_context *intel,
> - struct intel_region *region);
> -
> drm_intel_bo *intel_region_buffer(struct intel_context *intel,
> struct intel_region *region,
> GLuint flag);
> diff --git a/src/mesa/drivers/dri/intel/intel_tex_image.c b/src/mesa/drivers/dri/intel/intel_tex_image.c
> index 16a0a24..6db27bc 100644
> --- a/src/mesa/drivers/dri/intel/intel_tex_image.c
> +++ b/src/mesa/drivers/dri/intel/intel_tex_image.c
> @@ -213,49 +213,6 @@ try_pbo_upload(struct intel_context *intel,
> return GL_TRUE;
> }
>
> -
> -static GLboolean
> -try_pbo_zcopy(struct intel_context *intel,
> - struct intel_texture_image *intelImage,
> - const struct gl_pixelstore_attrib *unpack,
> - GLint width, const void *pixels)
> -{
> - struct intel_buffer_object *pbo = intel_buffer_object(unpack->BufferObj);
> - GLuint src_offset, src_stride;
> - GLuint dst_x, dst_y, dst_stride;
> -
> - if (!_mesa_is_bufferobj(unpack->BufferObj) ||
> - intel->ctx._ImageTransferState ||
> - unpack->SkipPixels || unpack->SkipRows) {
> - DBG("%s: failure 1\n", __FUNCTION__);
> - return GL_FALSE;
> - }
> -
> - /* note: potential 64-bit ptr to 32-bit int cast */
> - src_offset = (GLuint) (unsigned long) pixels;
> -
> - if (unpack->RowLength> 0)
> - src_stride = unpack->RowLength;
> - else
> - src_stride = width;
> -
> - intel_miptree_get_image_offset(intelImage->mt, intelImage->base.Base.Level,
> - intelImage->base.Base.Face, 0,
> - &dst_x,&dst_y);
> -
> - dst_stride = intelImage->mt->region->pitch;
> -
> - if (src_stride != dst_stride || dst_x != 0 || dst_y != 0 ||
> - src_offset != 0) {
> - DBG("%s: failure 2\n", __FUNCTION__);
> - return GL_FALSE;
> - }
> -
> - intel_region_attach_pbo(intel, intelImage->mt->region, pbo);
> -
> - return GL_TRUE;
> -}
> -
> /**
> * \param scatter Scatter if true. Gather if false.
> *
> @@ -451,23 +408,6 @@ intelTexImage(struct gl_context * ctx,
>
> DBG("trying pbo upload\n");
>
> - /* Attempt to texture directly from PBO data (zero copy upload).
> - *
> - * Currently disable as it can lead to worse as well as better
> - * performance (in particular when intel_region_cow() is
> - * required).
> - */
> - if (intelObj->mt == intelImage->mt&&
> - intelObj->mt->first_level == level&&
> - intelObj->mt->last_level == level) {
> -
> - if (try_pbo_zcopy(intel, intelImage, unpack, width, pixels)) {
> - DBG("pbo zcopy upload succeeded\n");
> - return;
> - }
> - }
> -
> -
> /* Otherwise, attempt to use the blitter for PBO image uploads.
> */
> if (try_pbo_upload(intel, intelImage, unpack, width, height, pixels)) {
More information about the mesa-dev
mailing list