[Mesa-dev] [PATCH 2/7] intel: Remove the pbo zero-copy code.

Keith Whitwell keithw at vmware.com
Wed Sep 21 10:24:23 PDT 2011


I'm suprised that fragile code lasted as long as it did...

Looks good to me.

Keith

On Wed, 2011-09-21 at 10:15 -0700, 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)
>  {
> -   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