<div dir="ltr"> <div><div><div><div><div class="gmail_extra"> <div class="gmail_quote">On Sun, Jan 4, 2015 at 1:07 PM, Ben Widawsky <<a href="mailto:ben@bwidawsk.net" target="_blank">ben@bwidawsk.net</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">I just did a very cursory review. I assume someone smarter than me will do a real review, but if not, feel free to ping me. I think all the comments apply to both functions. <div><div> On Sat, Jan 03, 2015 at 11:54:15AM -0800, Jason Ekstrand wrote: > From: Sisinty Sasmita Patra <<a href="mailto:sisinty.patra@intel.com" target="_blank">sisinty.patra@intel.com</a>> > > Added intel_readpixels_tiled_mempcpy and intel_gettexsubimage_tiled_mempcpy > functions. These are the fast paths for glReadPixels and glGetTexImage. > > v2: Jason Ekstrand <<a href="mailto:jason.ekstrand@intel.com" target="_blank">jason.ekstrand@intel.com</a>> > - Refactor to make the functions look more like the old > intel_tex_subimage_tiled_memcpy > - Don't export the readpixels_tiled_memcpy function > - Fix some pointer arithmatic bugs in partial image downloads (using > ReadPixels with a non-zero x or y offset) > - Fix a bug when ReadPixels is performed on an FBO wrapping a texture > miplevel other than zero. > > Signed-off-by: Jason Ekstrand <<a href="mailto:jason.ekstrand@intel.com" target="_blank">jason.ekstrand@intel.com</a>> > --- > src/mesa/drivers/dri/i965/intel_pixel_read.c | 134 +++++++++++++++++++++++++- > src/mesa/drivers/dri/i965/intel_tex.h | 9 ++ > src/mesa/drivers/dri/i965/intel_tex_image.c | 137 ++++++++++++++++++++++++++- > 3 files changed, 277 insertions(+), 3 deletions(-) > > diff --git a/src/mesa/drivers/dri/i965/intel_pixel_read.c b/src/mesa/drivers/dri/i965/intel_pixel_read.c > index beb3152..d1e7798 100644 > --- a/src/mesa/drivers/dri/i965/intel_pixel_read.c > +++ b/src/mesa/drivers/dri/i965/intel_pixel_read.c > @@ -38,14 +38,16 @@ > > #include "brw_context.h" > #include "intel_screen.h" > +#include "intel_batchbuffer.h" > #include "intel_blit.h" > #include "intel_buffers.h" > #include "intel_fbo.h" > #include "intel_mipmap_tree.h" > #include "intel_pixel.h" > #include "intel_buffer_objects.h" > +#include "intel_tiled_memcpy.h" > > -#define FILE_DEBUG_FLAG DEBUG_PIXEL > +#define FILE_DEBUG_FLAG DEBUG_TEXTURE > > /* For many applications, the new ability to pull the source buffers > * back out of the GTT and then do the packing/conversion operations > @@ -161,17 +163,147 @@ do_blit_readpixels(struct gl_context * ctx, > return true; > } > > +/** > + * \brief A fast path for glReadPixels > + * > + * This fast path is taken when the source format is BGRA, RGBA, > + * A or L and when the texture memory is X- or Y-tiled. It downloads > + * the source data by mapping the memory without a GTT fence, thus > + * acquiring a linear view of the memory. </div></div>That last sentence is confusing since you're using linear differently than just about everywhere else (though the statement is accurate). How about something like, "It maps the source data with a CPU mapping which then needs to be de-tiled [by the CPU] before presenting linear data back to the user." </blockquote><div> </div><div>Yeah, I'll get that fixed. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div> > + * > + * This is a performance win over the conventional texture download path. > + * In the conventional texture download path, > + * > + */ > + > +static bool > +intel_readpixels_tiled_memcpy(struct gl_context * ctx, > + GLint xoffset, GLint yoffset, > + GLsizei width, GLsizei height, > + GLenum format, GLenum type, > + GLvoid * pixels, > + const struct gl_pixelstore_attrib *pack) > +{ > + struct brw_context *brw = brw_context(ctx); > + struct gl_renderbuffer *rb = ctx->ReadBuffer->_ColorReadBuffer; > + > + /* This path supports reading from color buffers only */ > + if (rb == NULL) > + return false; > + > + struct intel_renderbuffer *irb = intel_renderbuffer(rb); > + int dst_pitch; > + > + /* The miptree's buffer. */ > + drm_intel_bo *bo; > + > + int error = 0; > + > + uint32_t cpp; > + mem_copy_fn mem_copy = NULL; > + > + /* This fastpath is restricted to specific renderbuffer types: > + * a 2D BGRA, RGBA, L8 or A8 texture. It could be generalized to support > + * more types. > + */ > + if (!brw->has_llc || > + !(type == GL_UNSIGNED_BYTE || type == GL_UNSIGNED_INT_8_8_8_8_REV) || > + pixels == NULL || > + _mesa_is_bufferobj(pack->BufferObj) || > + pack->Alignment > 4 || > + pack->SkipPixels > 0 || > + pack->SkipRows > 0 || > + (pack->RowLength != 0 && pack->RowLength != width) || > + pack->SwapBytes || > + pack->LsbFirst || > + pack->Invert) > + return false; > + > + if (!intel_get_memcpy(rb->Format, format, type, &mem_copy, &cpp)) > + return false; > + > + if (!irb->mt || > + (irb->mt->tiling != I915_TILING_X && > + irb->mt->tiling != I915_TILING_Y)) { > + /* The algorithm is written only for X- or Y-tiled memory. */ > + return false; > + } > + > + /* Since we are going to read raw data to the miptree, we need to resolve > + * any pending fast color clears before we start. > + */ > + intel_miptree_resolve_color(brw, irb->mt); > + > + bo = irb->mt->bo; > + > + if (drm_intel_bo_references(brw-><a href="http://batch.bo" target="_blank">batch.bo</a>, bo)) { > + perf_debug("Flushing before mapping a referenced bo.\n"); > + intel_batchbuffer_flush(brw); > + } </div></div>I don't know much about the spec, but is this even possible for glReadPixels? </blockquote><div> </div><div>Yes, it is. All you have to do is render and then readpixels without a flush. I added that for precicely that reason. :-) </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > + > + error = brw_bo_map(brw, bo, false /* write enable */, "miptree"); > + if (error || bo->virtual == NULL) { I realize this was probably just copied from the original code, but it should be superfluous (as is the comment about write enable). Not really sure why Eric initially added it. </blockquote><div> </div><div>Sure, the comment about write enable probably isn't needed, but it does explain the boolean. </div><div> </div><div>As for the other, I talked to Kristian and he said that, while unlikely, it is still possible for brw_bo_map to fail so we should handle the error case. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div> > + DBG("%s: failed to map bo\n", __FUNCTION__); > + return false; > + } > + > + dst_pitch = _mesa_image_row_stride(pack, width, format, type); > + > + /* We postponed printing this message until having committed to executing > + * the function. > + */ > + DBG("%s: x,y=(%d,%d) (w,h)=(%d,%d) format=0x%x type=0x%x " > + "mesa_format=0x%x tiling=%d " > + "pack=(alignment=%d row_length=%d skip_pixels=%d skip_rows=%d)\n", > + __FUNCTION__, xoffset, yoffset, width, height, > + format, type, rb->Format, irb->mt->tiling, > + pack->Alignment, pack->RowLength, pack->SkipPixels, > + pack->SkipRows); > + > + /* This renderbuffer can come from a texture which may not be miplevel 0. > + * We need to adjust accordingly. > + */ > + if (rb->TexImage) { > + int level = rb->TexImage->Level + rb->TexImage->TexObject->MinLevel; > + > + /* Adjust x and y offset based on miplevel */ > + xoffset += irb->mt->level[level].level_x; > + yoffset += irb->mt->level[level].level_y; > + } </div></div>Can the slice be non-zero? Should this be using intel_miptree_get_image_offset() instead? </blockquote><div> </div><div>Good catch. For now, I'm going to just implse that the texture's target is one of GL_TEXTURE_2D or GL_TEXTURE_RECTANGLE. None of the download paths handle arrays either, so that seems sane for now. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div> > + > + tiled_to_linear( > + xoffset * cpp, (xoffset + width) * cpp, > + yoffset, yoffset + height, > + pixels - (ptrdiff_t) yoffset * dst_pitch - (ptrdiff_t) xoffset * cpp, > + bo->virtual, > + dst_pitch, irb->mt->pitch, > + brw->has_swizzling, > + irb->mt->tiling, > + mem_copy > + ); > + > + drm_intel_bo_unmap(bo); > + return true; > +} > + > void > intelReadPixels(struct gl_context * ctx, > GLint x, GLint y, GLsizei width, GLsizei height, > GLenum format, GLenum type, > const struct gl_pixelstore_attrib *pack, GLvoid * pixels) > { > + bool ok; > + > struct brw_context *brw = brw_context(ctx); > bool dirty; > > DBG("%s\n", __FUNCTION__); > > + ok = intel_readpixels_tiled_memcpy(ctx, x, y, width, height, > + format, type, pixels, pack); > + if(ok) > + return; > + > if (_mesa_is_bufferobj(pack->BufferObj)) { > /* Using PBOs, so try the BLT based path. */ > if (do_blit_readpixels(ctx, x, y, width, height, format, type, pack, > diff --git a/src/mesa/drivers/dri/i965/intel_tex.h b/src/mesa/drivers/dri/i965/intel_tex.h > index 27f7f11..f048e84 100644 > --- a/src/mesa/drivers/dri/i965/intel_tex.h > +++ b/src/mesa/drivers/dri/i965/intel_tex.h > @@ -68,4 +68,13 @@ intel_texsubimage_tiled_memcpy(struct gl_context *ctx, > const struct gl_pixelstore_attrib *packing, > bool for_glTexImage); > > +bool > +intel_gettexsubimage_tiled_memcpy(struct gl_context *ctx, > + struct gl_texture_image *texImage, > + GLint xoffset, GLint yofset, > + GLsizei width, GLsizei height, > + GLenum format, GLenum type, > + GLvoid *pixels, > + const struct gl_pixelstore_attrib *packing); > + > #endif > diff --git a/src/mesa/drivers/dri/i965/intel_tex_image.c b/src/mesa/drivers/dri/i965/intel_tex_image.c > index 3317779..075315d 100644 > --- a/src/mesa/drivers/dri/i965/intel_tex_image.c > +++ b/src/mesa/drivers/dri/i965/intel_tex_image.c > @@ -24,7 +24,7 @@ > #include "intel_blit.h" > #include "intel_fbo.h" > #include "intel_image.h" > - > +#include "intel_tiled_memcpy.h" > #include "brw_context.h" > > #define FILE_DEBUG_FLAG DEBUG_TEXTURE > @@ -507,12 +507,145 @@ blit_texture_to_pbo(struct gl_context *ctx, > return true; > } > > +/** > + * \brief A fast path for glGetTexImage. > + * > + * > + * This fast path is taken when the texture format is BGRA, RGBA, > + * A or L and when the texture memory is X- or Y-tiled. It downloads > + * the texture data by mapping the texture memory without a GTT fence, thus > + * acquiring a linear view of the memory. > + * > + * This is a performance win over the conventional texture download path. > + * In the conventional texture download path, > + * > + */ > + > +bool > +intel_gettexsubimage_tiled_memcpy(struct gl_context *ctx, > + struct gl_texture_image *texImage, > + GLint xoffset, GLint yoffset, > + GLsizei width, GLsizei height, > + GLenum format, GLenum type, > + GLvoid *pixels, > + const struct gl_pixelstore_attrib *packing) > +{ > + struct brw_context *brw = brw_context(ctx); > + struct intel_texture_image *image = intel_texture_image(texImage); > + int dst_pitch; > + > + /* The miptree's buffer. */ > + drm_intel_bo *bo; > + > + int error = 0; > + > + uint32_t cpp; > + mem_copy_fn mem_copy = NULL; > + > + /* This fastpath is restricted to specific texture types: > + * a 2D BGRA, RGBA, L8 or A8 texture. It could be generalized to support > + * more types. > + * > + * FINISHME: The restrictions below on packing alignment and packing row > + * length are likely unneeded now because we calculate the destination stride > + * with _mesa_image_row_stride. However, before removing the restrictions > + * we need tests. > + */ > + if (!brw->has_llc || > + !(type == GL_UNSIGNED_BYTE || type == GL_UNSIGNED_INT_8_8_8_8_REV) || > + !(texImage->TexObject->Target == GL_TEXTURE_2D || > + texImage->TexObject->Target == GL_TEXTURE_RECTANGLE) || > + pixels == NULL || > + _mesa_is_bufferobj(packing->BufferObj) || > + packing->Alignment > 4 || > + packing->SkipPixels > 0 || > + packing->SkipRows > 0 || > + (packing->RowLength != 0 && packing->RowLength != width) || > + packing->SwapBytes || > + packing->LsbFirst || > + packing->Invert) > + return false; > + > + if (!intel_get_memcpy(texImage->TexFormat, format, type, &mem_copy, &cpp)) > + return false; > + > + /* If this is a nontrivial texture view, let another path handle it instead. */ > + if (texImage->TexObject->MinLayer) > + return false; > + > + if (!image->mt || > + (image->mt->tiling != I915_TILING_X && > + image->mt->tiling != I915_TILING_Y)) { > + /* The algorithm is written only for X- or Y-tiled memory. */ > + return false; > + } > + > + /* Since we are going to write raw data to the miptree, we need to resolve > + * any pending fast color clears before we start. > + */ > + intel_miptree_resolve_color(brw, image->mt); > + > + bo = image->mt->bo; > + > + if (drm_intel_bo_references(brw-><a href="http://batch.bo" target="_blank">batch.bo</a>, bo)) { > + perf_debug("Flushing before mapping a referenced bo.\n"); > + intel_batchbuffer_flush(brw); > + } > + > + error = brw_bo_map(brw, bo, false /* write enable */, "miptree"); > + if (error || bo->virtual == NULL) { > + DBG("%s: failed to map bo\n", __FUNCTION__); > + return false; > + } > + > + dst_pitch = _mesa_image_row_stride(packing, width, format, type); > + > + DBG("%s: level=%d x,y=(%d,%d) (w,h)=(%d,%d) format=0x%x type=0x%x " > + "mesa_format=0x%x tiling=%d " > + "packing=(alignment=%d row_length=%d skip_pixels=%d skip_rows=%d)\n", > + __FUNCTION__, texImage->Level, xoffset, yoffset, width, height, > + format, type, texImage->TexFormat, image->mt->tiling, > + packing->Alignment, packing->RowLength, packing->SkipPixels, > + packing->SkipRows); > + > + int level = texImage->Level + texImage->TexObject->MinLevel; > + > + /* Adjust x and y offset based on miplevel */ > + xoffset += image->mt->level[level].level_x; > + yoffset += image->mt->level[level].level_y; > + > + tiled_to_linear( > + xoffset * cpp, (xoffset + width) * cpp, > + yoffset, yoffset + height, > + pixels - (ptrdiff_t) yoffset * dst_pitch - (ptrdiff_t) xoffset * cpp, > + bo->virtual, > + dst_pitch, image->mt->pitch, > + brw->has_swizzling, > + image->mt->tiling, > + mem_copy > + ); > + > + drm_intel_bo_unmap(bo); > + return true; > +} > + > + > static void > intel_get_tex_image(struct gl_context *ctx, > GLenum format, GLenum type, GLvoid *pixels, > - struct gl_texture_image *texImage) { > + struct gl_texture_image *texImage) > +{ > + bool ok; > + > DBG("%s\n", __FUNCTION__); > > + ok = intel_gettexsubimage_tiled_memcpy(ctx, texImage, 0, 0, > + texImage->Width, texImage->Height, > + format, type, pixels, &ctx->Pack); > + > + if(ok) > + return; > + > if (_mesa_is_bufferobj(ctx->Pack.BufferObj)) { > /* Using PBOs, so try the BLT based path. */ > if (blit_texture_to_pbo(ctx, format, type, pixels, texImage)) > -- > 2.2.0 > </div></div>> _______________________________________________ > mesa-dev mailing list > <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> -- Ben Widawsky, Intel Open Source Technology Center </blockquote></div> </div></div></div></div></div></div>