This code literally had zero comments, which made it impenetrable for basically everybody except the authors.
We still ought to comment the refcounting strategy. Signed-off-by: Kenneth Graunke <kenn...@whitecape.org> --- src/mesa/drivers/dri/i965/brw_context.h | 24 +++++++ src/mesa/drivers/dri/i965/intel_upload.c | 110 ++++++++++++++++++++++++++++++- 2 files changed, 133 insertions(+), 1 deletion(-) diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h index 734d273..659af92 100644 --- a/src/mesa/drivers/dri/i965/brw_context.h +++ b/src/mesa/drivers/dri/i965/brw_context.h @@ -1048,10 +1048,34 @@ struct brw_context bool no_batch_wrap; struct { + /** + * A buffer object for holding miscellaneous data. + */ drm_intel_bo *bo; + + /** + * Offset into "bo" for where to put the next piece of data. + * + * This is incremented when intel_upload_data or intel_upload_map + * ask for space to store data; the data may actually be staged and + * not appear in the BO until later. + */ GLuint offset; + + /** + * The amount of staged data needing to be copied to the BO. + */ uint32_t buffer_len; + + /** + * Offset into "bo" representing the starting destination address for + * the next batched copy. + */ uint32_t buffer_offset; + + /** + * The CPU-side staging buffer containing data yet to be uploaded. + */ char buffer[4096]; } upload; diff --git a/src/mesa/drivers/dri/i965/intel_upload.c b/src/mesa/drivers/dri/i965/intel_upload.c index ec3109b..c767b7d 100644 --- a/src/mesa/drivers/dri/i965/intel_upload.c +++ b/src/mesa/drivers/dri/i965/intel_upload.c @@ -25,7 +25,16 @@ /** * @file intel_upload.c * - * Batched upload via BOs. + * A batched data upload mechanism which can efficiently copy user supplied + * data into buffer objects. + * + * OpenGL allows applications to provide data in user arrays, which may + * disappear or change contents at any time. The GPU can't use this memory + * directly, so we need to copy it into buffer objects. + * + * Often times, the amount of data we need to copy is small. On non-LLC + * platforms, it's more efficient to stage this data in a CPU-sized buffer + * and only copy it to a BO when we have a larger amount of data. */ #include "main/imports.h" @@ -43,6 +52,7 @@ #include "brw_context.h" +/** The size of brw->upload.bo (the GPU buffer object). */ #define INTEL_UPLOAD_SIZE (64*1024) /** @@ -51,6 +61,15 @@ #define ALIGN_NPOT(value, alignment) \ (((value) + (alignment) - 1) / (alignment) * (alignment)) +/** + * Finish any outstanding uploads. + * + * We may have staged data that hasn't yet been copied to the BO; calling + * this function forces an immediate copy. + * + * This is necessary at certain points, such as submitting a batchbuffer + * that wants to actually use the data. + */ void intel_upload_finish(struct brw_context *brw) { @@ -69,11 +88,22 @@ intel_upload_finish(struct brw_context *brw) brw->upload.bo = NULL; } +/** + * Handle running out of space in the BO. + * + * Called when attempting to upload a new piece of data. If the existing BO + * doesn't have enough space, we need to finish uploading any existing staged + * data, and allocate a new BO. + */ static void wrap_buffers(struct brw_context *brw, GLuint size) { intel_upload_finish(brw); + /* Use the larger of size and INTEL_UPLOAD_SIZE. This ensures that the + * buffer is large enough to upload any arbitrary piece of data, but will + * be reasonably sized even if we wrap when uploading a small bit of data. + */ if (size < INTEL_UPLOAD_SIZE) size = INTEL_UPLOAD_SIZE; @@ -81,6 +111,16 @@ wrap_buffers(struct brw_context *brw, GLuint size) brw->upload.offset = 0; } +/** + * Upload some data (copy from CPU memory to a buffer object). + * + * @param[in] brw The i965 driver context. + * @param[in] ptr The source data to upload. + * @param[in] size The number of bytes to upload. + * @param[in] align Alignment requirement in bytes. + * @param[out] return_bo A pointer to the BO which will contain the data. + * @param[out] return_offset Offset into return_bo where the data will be. + */ void intel_upload_data(struct brw_context *brw, const void *ptr, GLuint size, GLuint align, @@ -89,6 +129,7 @@ intel_upload_data(struct brw_context *brw, { GLuint base, delta; + /* Make sure the BO has enough space for this data. */ base = ALIGN_NPOT(brw->upload.offset, align); if (brw->upload.bo == NULL || base + size > brw->upload.bo->size) { wrap_buffers(brw, size); @@ -96,10 +137,19 @@ intel_upload_data(struct brw_context *brw, } drm_intel_bo_reference(brw->upload.bo); + + /* The new data will (eventually) end up in brw->upload.bo with an + * offset of "base". Return this so the caller can refer to the data. + */ *return_bo = brw->upload.bo; *return_offset = base; + /* delta is the number of bytes needed for alignment. */ delta = base - brw->upload.offset; + + /* If there isn't enough space in the staging buffer, go upload the + * staged data to free up space. + */ if (brw->upload.buffer_len && brw->upload.buffer_len + delta + size > sizeof(brw->upload.buffer)) { drm_intel_bo_subdata(brw->upload.bo, @@ -110,6 +160,9 @@ intel_upload_data(struct brw_context *brw, } if (size < sizeof(brw->upload.buffer)) { + /* The new data is small enough to fit in the staging buffer, so + * stage it to be copied to the BO later. + */ if (brw->upload.buffer_len == 0) brw->upload.buffer_offset = base; else @@ -118,25 +171,53 @@ intel_upload_data(struct brw_context *brw, memcpy(brw->upload.buffer + brw->upload.buffer_len, ptr, size); brw->upload.buffer_len += size; } else { + /* The new data is too large to fit in the (empty) staging buffer, + * which means it wouldn't benefit from batched uploading anyway. + * + * Just upload it directly. + */ drm_intel_bo_subdata(brw->upload.bo, base, size, ptr); } brw->upload.offset = base + size; } +/** + * Provide a pointer to a region of memory which will be uploaded. + * + * Sometimes callers want to rearrange the source data when uploading. + * While they could allocate their own temporary memory, rearrange the data, + * and then call intel_upload_data, this would result in double staging. + * + * Instead, using intel_upload_map reserves space in the staging buffer + * (or a temporary allocation if it isn't large enough), and allows direct + * access to that. This results in a single staging operation. + * + * @param brw The i965 driver context. + * @param size The number of bytes to be uploaded. + * @param align Alignment requirement in bytes. + * + * It is an (unchecked) error to mismatch calls to map/unmap. + */ void * intel_upload_map(struct brw_context *brw, GLuint size, GLuint align) { GLuint base, delta; char *ptr; + /* Make sure the BO has enough space for this data. */ base = ALIGN_NPOT(brw->upload.offset, align); if (brw->upload.bo == NULL || base + size > brw->upload.bo->size) { wrap_buffers(brw, size); base = 0; } + /* delta is the number of bytes needed for alignment. */ delta = base - brw->upload.offset; + + /* If there isn't enough space in the staging buffer, go upload the + * staged data to free up space. + */ if (brw->upload.buffer_len && brw->upload.buffer_len + delta + size > sizeof(brw->upload.buffer)) { drm_intel_bo_subdata(brw->upload.bo, @@ -147,6 +228,9 @@ intel_upload_map(struct brw_context *brw, GLuint size, GLuint align) } if (size <= sizeof(brw->upload.buffer)) { + /* The new data is small enough to fit in the staging buffer, so + * stage it to be copied to the BO later. + */ if (brw->upload.buffer_len == 0) brw->upload.buffer_offset = base; else @@ -155,12 +239,27 @@ intel_upload_map(struct brw_context *brw, GLuint size, GLuint align) ptr = brw->upload.buffer + brw->upload.buffer_len; brw->upload.buffer_len += size; } else { + /* The new data is too large to fit into the default staging buffer, + * even when empty. Allocate a larger, one-shot staging buffer. + */ ptr = malloc(size); } return ptr; } +/** + * Complete the upload/staging operation started by intel_upload_map(). + * + * @param[in] brw The i965 driver context. + * @param[in] ptr The pointer returned by intel_upload_map(). + * @param[in] size Must match the value given to intel_upload_map(). + * @param[in] align Must match the value given to intel_upload_map(). + * @param[out] return_bo A pointer to the BO which will contain the data. + * @param[out] return_offset Offset into return_bo where the data will be. + * + * It is an (unchecked) error to mismatch calls to map/unmap. + */ void intel_upload_unmap(struct brw_context *brw, const void *ptr, GLuint size, GLuint align, @@ -171,11 +270,20 @@ intel_upload_unmap(struct brw_context *brw, base = ALIGN_NPOT(brw->upload.offset, align); if (size > sizeof(brw->upload.buffer)) { + /* The data was too large to fit in the default staging buffer, so we + * allocated a one-shot temporary staging buffer in intel_upload_map(). + * + * Immediately copy it to the BO and free the one-shot staging buffer. + */ drm_intel_bo_subdata(brw->upload.bo, base, size, ptr); free((void*)ptr); } drm_intel_bo_reference(brw->upload.bo); + + /* The new data will (eventually) end up in brw->upload.bo with an + * offset of "base". Return this so the caller can refer to the data. + */ *return_bo = brw->upload.bo; *return_offset = base; -- 1.9.0 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev