On Fri, May 29, 2015 at 09:32:53AM +0300, Pohjolainen, Topi wrote:
> 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?
I actually meant that isn't "layout_flags & MIPTREE_LAYOUT_DISABLE_AUX" enough
on its own?
>
> > 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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
