On Thu, May 28, 2015 at 10:21:29AM -0700, Ben Widawsky wrote:
> I think pretty much everyone agrees that having more than a single bool as a
> function argument is bordering on a bad idea. What sucks about the current
> code is in several instances it's necessary to propagate these boolean
> selections down to lower layers of the code. This requires plumbing
> (mechanical,
> but still churn) pretty much all of the miptree functions each time. By
> introducing the flags paramater, it is possible to add miptree constraints
> very
> easily.
I like this a lot. I have a few simple comments below.
>
> The use of this, as is already the case, is sometimes we have some information
> at the time we create the miptree that needs to be known all the way at the
> lowest levels of the create/allocation, disable_aux_buffers is currently one
> such example. There will be another example coming up in a few patches.
>
> Cc: Chad Versace <chad.vers...@linux.intel.com>
> Signed-off-by: Ben Widawsky <b...@bwidawsk.net>
> ---
> src/mesa/drivers/dri/i965/intel_fbo.c | 5 +-
> src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 86
> +++++++++++++-------------
> src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 9 ++-
> src/mesa/drivers/dri/i965/intel_pixel_draw.c | 2 +-
> src/mesa/drivers/dri/i965/intel_tex.c | 8 +--
> src/mesa/drivers/dri/i965/intel_tex.h | 2 +-
> src/mesa/drivers/dri/i965/intel_tex_image.c | 14 ++---
> src/mesa/drivers/dri/i965/intel_tex_validate.c | 3 +-
> 8 files changed, 63 insertions(+), 66 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_fbo.c
> b/src/mesa/drivers/dri/i965/intel_fbo.c
> index aebed72..1b3a72f 100644
> --- a/src/mesa/drivers/dri/i965/intel_fbo.c
> +++ b/src/mesa/drivers/dri/i965/intel_fbo.c
> @@ -390,7 +390,7 @@ intel_image_target_renderbuffer_storage(struct gl_context
> *ctx,
> image->height,
> 1,
> image->pitch,
> - true /*disable_aux_buffers*/);
> + MIPTREE_LAYOUT_DISABLE_AUX);
> if (!irb->mt)
> return;
>
> @@ -1027,10 +1027,9 @@ intel_renderbuffer_move_to_temp(struct brw_context
> *brw,
> intel_image->base.Base.Level,
> intel_image->base.Base.Level,
> width, height, depth,
> - true,
> irb->mt->num_samples,
> INTEL_MIPTREE_TILING_ANY,
> - false);
> + MIPTREE_LAYOUT_ACCELERATED_UPLOAD);
>
> if (intel_miptree_wants_hiz_buffer(brw, new_mt)) {
> intel_miptree_alloc_hiz(brw, new_mt);
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index 24a5c3d..b243f8a 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -244,10 +244,8 @@ intel_miptree_create_layout(struct brw_context *brw,
> GLuint width0,
> GLuint height0,
> GLuint depth0,
> - bool for_bo,
> GLuint num_samples,
> - bool force_all_slices_at_each_lod,
> - bool disable_aux_buffers)
> + uint32_t layout_flags)
> {
> struct intel_mipmap_tree *mt = calloc(sizeof(*mt), 1);
> if (!mt)
> @@ -286,7 +284,7 @@ intel_miptree_create_layout(struct brw_context *brw,
> mt->logical_height0 = height0;
> mt->logical_depth0 = depth0;
> mt->fast_clear_state = INTEL_FAST_CLEAR_STATE_NO_MCS;
> - mt->disable_aux_buffers = disable_aux_buffers;
> + mt->disable_aux_buffers = !!(layout_flags & MIPTREE_LAYOUT_DISABLE_AUX);
You didn't mean to have double negation (!!), did you?
> exec_list_make_empty(&mt->hiz_map);
>
> /* The cpp is bytes per (1, blockheight)-sized block for compressed
> @@ -422,12 +420,14 @@ intel_miptree_create_layout(struct brw_context *brw,
> mt->physical_height0 = height0;
> mt->physical_depth0 = depth0;
>
> - if (!for_bo &&
> + if (!(layout_flags & MIPTREE_LAYOUT_FOR_BO) &&
> _mesa_get_format_base_format(format) == GL_DEPTH_STENCIL &&
> (brw->must_use_separate_stencil ||
> (brw->has_separate_stencil &&
> intel_miptree_wants_hiz_buffer(brw, mt)))) {
> - const bool force_all_slices_at_each_lod = brw->gen == 6;
> + uint32_t stencil_flags = MIPTREE_LAYOUT_ACCELERATED_UPLOAD;
> + if (brw->gen == 6)
> + stencil_flags |= MIPTREE_LAYOUT_FORCE_ALL_SLICE_AT_LOD;
Perhaps a separating line here, your choice.
> mt->stencil_mt = intel_miptree_create(brw,
> mt->target,
> MESA_FORMAT_S_UINT8,
> @@ -436,10 +436,10 @@ intel_miptree_create_layout(struct brw_context *brw,
> mt->logical_width0,
> mt->logical_height0,
> mt->logical_depth0,
> - true,
> num_samples,
> INTEL_MIPTREE_TILING_ANY,
> - force_all_slices_at_each_lod);
> + stencil_flags);
> +
> if (!mt->stencil_mt) {
> intel_miptree_release(&mt);
> return NULL;
> @@ -457,7 +457,7 @@ intel_miptree_create_layout(struct brw_context *brw,
> }
> }
>
> - if (force_all_slices_at_each_lod)
> + if (layout_flags & MIPTREE_LAYOUT_FORCE_ALL_SLICE_AT_LOD)
> mt->array_layout = ALL_SLICES_AT_EACH_LOD;
>
> brw_miptree_layout(brw, mt);
> @@ -622,10 +622,9 @@ intel_miptree_create(struct brw_context *brw,
> GLuint width0,
> GLuint height0,
> GLuint depth0,
> - bool expect_accelerated_upload,
> GLuint num_samples,
> enum intel_miptree_tiling_mode requested_tiling,
> - bool force_all_slices_at_each_lod)
> + uint32_t layout_flags)
> {
> struct intel_mipmap_tree *mt;
> mesa_format tex_format = format;
> @@ -636,12 +635,12 @@ intel_miptree_create(struct brw_context *brw,
>
> etc_format = (format != tex_format) ? tex_format : MESA_FORMAT_NONE;
>
> + assert((layout_flags & MIPTREE_LAYOUT_DISABLE_AUX) == 0);
> + assert((layout_flags & MIPTREE_LAYOUT_FOR_BO) == 0);
> mt = intel_miptree_create_layout(brw, target, format,
> - first_level, last_level, width0,
> - height0, depth0,
> - false, num_samples,
> - force_all_slices_at_each_lod,
> - false /*disable_aux_buffers*/);
> + first_level, last_level, width0,
> + height0, depth0, num_samples,
> + layout_flags);
> /*
> * pitch == 0 || height == 0 indicates the null texture
> */
> @@ -673,11 +672,11 @@ intel_miptree_create(struct brw_context *brw,
>
> unsigned long pitch;
> mt->etc_format = etc_format;
> - mt->bo = drm_intel_bo_alloc_tiled(brw->bufmgr, "miptree",
> - total_width, total_height, mt->cpp,
> - &mt->tiling, &pitch,
> - (expect_accelerated_upload ?
> - BO_ALLOC_FOR_RENDER : 0));
> + mt->bo =
> + drm_intel_bo_alloc_tiled(brw->bufmgr, "miptree", total_width,
> total_height,
> + mt->cpp, &mt->tiling, &pitch,
> + (layout_flags &
> MIPTREE_LAYOUT_ACCELERATED_UPLOAD) ?
Overflowing 80 columns (at least the documented 78).
> + BO_ALLOC_FOR_RENDER : 0);
> mt->pitch = pitch;
>
> /* If the BO is too large to fit in the aperture, we need to use the
> @@ -690,11 +689,12 @@ intel_miptree_create(struct brw_context *brw,
>
> mt->tiling = I915_TILING_X;
> drm_intel_bo_unreference(mt->bo);
> - mt->bo = drm_intel_bo_alloc_tiled(brw->bufmgr, "miptree",
> - total_width, total_height, mt->cpp,
> - &mt->tiling, &pitch,
> - (expect_accelerated_upload ?
> - BO_ALLOC_FOR_RENDER : 0));
> + mt->bo =
> + drm_intel_bo_alloc_tiled(brw->bufmgr, "miptree",
> + total_width, total_height, mt->cpp,
> + &mt->tiling, &pitch,
> + (layout_flags &
> MIPTREE_LAYOUT_ACCELERATED_UPLOAD) ?
Same here, better to wrap the line.
> + BO_ALLOC_FOR_RENDER : 0);
> mt->pitch = pitch;
> }
>
> @@ -733,7 +733,7 @@ intel_miptree_create_for_bo(struct brw_context *brw,
> uint32_t height,
> uint32_t depth,
> int pitch,
> - bool disable_aux_buffers)
> + uint32_t layout_flags)
> {
> struct intel_mipmap_tree *mt;
> uint32_t tiling, swizzle;
> @@ -754,11 +754,11 @@ intel_miptree_create_for_bo(struct brw_context *brw,
>
> target = depth > 1 ? GL_TEXTURE_2D_ARRAY : GL_TEXTURE_2D;
>
> + layout_flags |= MIPTREE_LAYOUT_FOR_BO;
> mt = intel_miptree_create_layout(brw, target, format,
> 0, 0,
> - width, height, depth,
> - true, 0, false,
> - disable_aux_buffers);
> + width, height, depth, 0,
> + layout_flags);
> if (!mt)
> return NULL;
>
> @@ -808,7 +808,7 @@ intel_update_winsys_renderbuffer_miptree(struct
> brw_context *intel,
> height,
> 1,
> pitch,
> - false);
> + 0);
> if (!singlesample_mt)
> goto fail;
>
> @@ -866,8 +866,9 @@ intel_miptree_create_for_renderbuffer(struct brw_context
> *brw,
> GLenum target = num_samples > 1 ? GL_TEXTURE_2D_MULTISAMPLE :
> GL_TEXTURE_2D;
>
> mt = intel_miptree_create(brw, target, format, 0, 0,
> - width, height, depth, true, num_samples,
> - INTEL_MIPTREE_TILING_ANY, false);
> + width, height, depth, num_samples,
> + INTEL_MIPTREE_TILING_ANY,
> + MIPTREE_LAYOUT_ACCELERATED_UPLOAD);
> if (!mt)
> goto fail;
>
> @@ -1378,10 +1379,9 @@ intel_miptree_alloc_mcs(struct brw_context *brw,
> mt->logical_width0,
> mt->logical_height0,
> mt->logical_depth0,
> - true,
> 0 /* num_samples */,
> INTEL_MIPTREE_TILING_Y,
> - false);
> + MIPTREE_LAYOUT_ACCELERATED_UPLOAD);
>
> /* From the Ivy Bridge PRM, Vol 2 Part 1 p326:
> *
> @@ -1437,10 +1437,9 @@ intel_miptree_alloc_non_msrt_mcs(struct brw_context
> *brw,
> mcs_width,
> mcs_height,
> mt->logical_depth0,
> - true,
> 0 /* num_samples */,
> INTEL_MIPTREE_TILING_Y,
> - false);
> + MIPTREE_LAYOUT_ACCELERATED_UPLOAD);
>
> return mt->mcs_mt;
> }
> @@ -1682,7 +1681,10 @@ intel_hiz_miptree_buf_create(struct brw_context *brw,
> struct intel_mipmap_tree *mt)
> {
> struct intel_miptree_aux_buffer *buf = calloc(sizeof(*buf), 1);
> - const bool force_all_slices_at_each_lod = brw->gen == 6;
> + uint32_t layout_flags = MIPTREE_LAYOUT_ACCELERATED_UPLOAD;
> +
> + if (brw->gen == 6)
> + layout_flags |= MIPTREE_LAYOUT_FORCE_ALL_SLICE_AT_LOD;
>
> if (!buf)
> return NULL;
> @@ -1695,10 +1697,9 @@ intel_hiz_miptree_buf_create(struct brw_context *brw,
> mt->logical_width0,
> mt->logical_height0,
> mt->logical_depth0,
> - true,
> mt->num_samples,
> INTEL_MIPTREE_TILING_ANY,
> - force_all_slices_at_each_lod);
> + layout_flags);
> if (!buf->mt) {
> free(buf);
> return NULL;
> @@ -2128,9 +2129,8 @@ intel_miptree_map_blit(struct brw_context *brw,
> map->mt = intel_miptree_create(brw, GL_TEXTURE_2D, mt->format,
> 0, 0,
> map->w, map->h, 1,
> - false, 0,
> - INTEL_MIPTREE_TILING_NONE,
> - false);
> + 0, INTEL_MIPTREE_TILING_NONE, 0);
> +
> if (!map->mt) {
> fprintf(stderr, "Failed to allocate blit temporary\n");
> goto fail;
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> index 8b42e4a..4722353 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> @@ -527,6 +527,10 @@ bool
> intel_miptree_alloc_non_msrt_mcs(struct brw_context *brw,
> struct intel_mipmap_tree *mt);
>
> +#define MIPTREE_LAYOUT_ACCELERATED_UPLOAD (1<<0)
I was wondering if we could call the the flags something else than layout.
The manner of upload doesn't technically have much to do with layout. Maybe
just flags, shrug.
> +#define MIPTREE_LAYOUT_FORCE_ALL_SLICE_AT_LOD (1<<1)
> +#define MIPTREE_LAYOUT_FOR_BO (1<<2)
> +#define MIPTREE_LAYOUT_DISABLE_AUX (1<<3)
Maybe a separating new line here as well?
> struct intel_mipmap_tree *intel_miptree_create(struct brw_context *brw,
> GLenum target,
> mesa_format format,
> @@ -535,10 +539,9 @@ struct intel_mipmap_tree *intel_miptree_create(struct
> brw_context *brw,
> GLuint width0,
> GLuint height0,
> GLuint depth0,
> - bool expect_accelerated_upload,
> GLuint num_samples,
> enum
> intel_miptree_tiling_mode,
> - bool
> force_all_slices_at_each_lod);
> + uint32_t flags);
>
> struct intel_mipmap_tree *
> intel_miptree_create_for_bo(struct brw_context *brw,
> @@ -549,7 +552,7 @@ intel_miptree_create_for_bo(struct brw_context *brw,
> uint32_t height,
> uint32_t depth,
> int pitch,
> - bool disable_aux_buffers);
> + uint32_t layout_flags);
>
> void
> intel_update_winsys_renderbuffer_miptree(struct brw_context *intel,
> diff --git a/src/mesa/drivers/dri/i965/intel_pixel_draw.c
> b/src/mesa/drivers/dri/i965/intel_pixel_draw.c
> index 4ecefc8..b72baa0 100644
> --- a/src/mesa/drivers/dri/i965/intel_pixel_draw.c
> +++ b/src/mesa/drivers/dri/i965/intel_pixel_draw.c
> @@ -112,7 +112,7 @@ do_blit_drawpixels(struct gl_context * ctx,
> src_offset,
> width, height, 1,
> src_stride,
> - false /*disable_aux_buffers*/);
> + 0);
> if (!pbo_mt)
> return false;
>
> diff --git a/src/mesa/drivers/dri/i965/intel_tex.c
> b/src/mesa/drivers/dri/i965/intel_tex.c
> index 777a682..b0181ad 100644
> --- a/src/mesa/drivers/dri/i965/intel_tex.c
> +++ b/src/mesa/drivers/dri/i965/intel_tex.c
> @@ -93,7 +93,7 @@ intel_alloc_texture_image_buffer(struct gl_context *ctx,
> } else {
> intel_image->mt = intel_miptree_create_for_teximage(brw, intel_texobj,
> intel_image,
> - false);
> + 0);
>
> /* Even if the object currently has a mipmap tree associated
> * with it, this one is a more likely candidate to represent the
> @@ -144,10 +144,8 @@ intel_alloc_texture_storage(struct gl_context *ctx,
> first_image->TexFormat,
> 0, levels - 1,
> width, height, depth,
> - false, /* expect_accelerated */
> num_samples,
> - INTEL_MIPTREE_TILING_ANY,
> - false);
> + INTEL_MIPTREE_TILING_ANY, 0);
>
> if (intel_texobj->mt == NULL) {
> return false;
> @@ -341,7 +339,7 @@ intel_set_texture_storage_for_buffer_object(struct
> gl_context *ctx,
> buffer_offset,
> image->Width, image->Height, image->Depth,
> row_stride,
> - false /*disable_aux_buffers*/);
> + 0);
> if (!intel_texobj->mt)
> return false;
>
> diff --git a/src/mesa/drivers/dri/i965/intel_tex.h
> b/src/mesa/drivers/dri/i965/intel_tex.h
> index f048e84..402a389 100644
> --- a/src/mesa/drivers/dri/i965/intel_tex.h
> +++ b/src/mesa/drivers/dri/i965/intel_tex.h
> @@ -53,7 +53,7 @@ struct intel_mipmap_tree *
> intel_miptree_create_for_teximage(struct brw_context *brw,
> struct intel_texture_object *intelObj,
> struct intel_texture_image *intelImage,
> - bool expect_accelerated_upload);
> + uint32_t layout_flags);
>
> GLuint intel_finalize_mipmap_tree(struct brw_context *brw, GLuint unit);
>
> diff --git a/src/mesa/drivers/dri/i965/intel_tex_image.c
> b/src/mesa/drivers/dri/i965/intel_tex_image.c
> index 85d3d04..ebe84b6 100644
> --- a/src/mesa/drivers/dri/i965/intel_tex_image.c
> +++ b/src/mesa/drivers/dri/i965/intel_tex_image.c
> @@ -36,7 +36,7 @@ struct intel_mipmap_tree *
> intel_miptree_create_for_teximage(struct brw_context *brw,
> struct intel_texture_object *intelObj,
> struct intel_texture_image *intelImage,
> - bool expect_accelerated_upload)
> + uint32_t layout_flags)
> {
> GLuint lastLevel;
> int width, height, depth;
> @@ -79,10 +79,9 @@ intel_miptree_create_for_teximage(struct brw_context *brw,
> width,
> height,
> depth,
> - expect_accelerated_upload,
> intelImage->base.Base.NumSamples,
> INTEL_MIPTREE_TILING_ANY,
> - false);
> + layout_flags);
> }
>
> static void
> @@ -155,7 +154,7 @@ intel_set_texture_image_bo(struct gl_context *ctx,
> GLuint width, GLuint height,
> GLuint pitch,
> GLuint tile_x, GLuint tile_y,
> - bool disable_aux_buffers)
> + uint32_t layout_flags)
> {
> struct brw_context *brw = brw_context(ctx);
> struct intel_texture_image *intel_image = intel_texture_image(image);
> @@ -171,7 +170,7 @@ intel_set_texture_image_bo(struct gl_context *ctx,
>
> intel_image->mt = intel_miptree_create_for_bo(brw, bo, image->TexFormat,
> 0, width, height, 1, pitch,
> - disable_aux_buffers);
> + layout_flags);
> if (intel_image->mt == NULL)
> return;
> intel_image->mt->target = target;
> @@ -255,8 +254,7 @@ intelSetTexBuffer2(__DRIcontext *pDRICtx, GLint target,
> rb->Base.Base.Width,
> rb->Base.Base.Height,
> rb->mt->pitch,
> - 0, 0,
> - false /*disable_aux_buffers*/);
> + 0, 0, 0);
> _mesa_unlock_texture(&brw->ctx, texObj);
> }
>
> @@ -349,7 +347,7 @@ intel_image_target_texture_2d(struct gl_context *ctx,
> GLenum target,
> image->width, image->height,
> image->pitch,
> image->tile_x, image->tile_y,
> - true /*disable_aux_buffers*/);
> + MIPTREE_LAYOUT_DISABLE_AUX);
> }
>
> /**
> diff --git a/src/mesa/drivers/dri/i965/intel_tex_validate.c
> b/src/mesa/drivers/dri/i965/intel_tex_validate.c
> index c581e14..4991c29 100644
> --- a/src/mesa/drivers/dri/i965/intel_tex_validate.c
> +++ b/src/mesa/drivers/dri/i965/intel_tex_validate.c
> @@ -144,10 +144,9 @@ intel_finalize_mipmap_tree(struct brw_context *brw,
> GLuint unit)
> width,
> height,
> depth,
> - true,
> 0 /* num_samples */,
> INTEL_MIPTREE_TILING_ANY,
> - false);
> + MIPTREE_LAYOUT_ACCELERATED_UPLOAD);
> if (!intelObj->mt)
> return false;
> }
> --
> 2.4.2
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
