On Fri, Mar 16, 2012 at 11:13:23AM -0700, Eric Anholt wrote:
> On Thu, 15 Mar 2012 14:42:53 +0800, Yuanhan Liu <yuanhan....@linux.intel.com>
> wrote:
> > There are two mipmap layout modes: below and right. And we currently just
> > use _below_ mode. And in some cases, like height is greater than width,
> > it would be better to use the _right_ mode for saving memory.
> >
> > And it also fix some issues like the gl-max-texture-dimensions.html webglc
> > test case on pineview in a hardware way(no fallback). Since when
> > rendering with 1x2048 texture using below mode would make the draw
> > offset exceed the max allowed size, but will not when using right mode.
>
> I think you should be able to get hardware rendering without changing
> layouts by taking advantage of intel_renderbuffer_get_tile_offsets().
>
> > diff --git a/src/mesa/drivers/dri/i915/i915_texstate.c
> > b/src/mesa/drivers/dri/i915/i915_texstate.c
> > index 9022548..54f32a4 100644
> > --- a/src/mesa/drivers/dri/i915/i915_texstate.c
> > +++ b/src/mesa/drivers/dri/i915/i915_texstate.c
> > @@ -192,6 +192,8 @@ i915_update_tex_unit(struct intel_context *intel,
> > GLuint unit, GLuint ss3)
> > (U_FIXED(CLAMP(maxlod, 0.0, 11.0), 2) << MS4_MAX_LOD_SHIFT) |
> > ((firstImage->Depth - 1) << MS4_VOLUME_DEPTH_SHIFT));
> >
> > + if (intel->is_945)
> > + state[I915_TEXREG_MS4] |= intelObj->mt->layout <<
> > MS4_MIP_LAYOUT_MODE_SHIFT;
> >
> > {
> > GLuint minFilt, mipFilt, magFilt;
>
> You're checking 945 here, but your decision to choose right layout
> didn't check for 945.
The decision to choose right layout is done at i945_miptree_choose_layout,
that's where we did check.
>
> > diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > index a3de2e3..b6565fe 100644
> > --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > @@ -654,7 +654,7 @@ brw_update_texture_surface( struct gl_context *ctx,
> > GLuint unit )
> > 6 * 4, 32, &brw->wm.surf_offset[surf_index]);
> >
> > surf[0] = (translate_tex_target(tObj->Target) << BRW_SURFACE_TYPE_SHIFT
> > |
> > - BRW_SURFACE_MIPMAPLAYOUT_BELOW << BRW_SURFACE_MIPLAYOUT_SHIFT |
> > + mt->layout << BRW_SURFACE_MIPLAYOUT_SHIFT |
> > BRW_SURFACE_CUBEFACE_ENABLES |
> > (translate_tex_format(mt->format,
> > firstImage->InternalFormat,
>
> You didn't update gen7.
Wierdly, I didn't find mipmap layout mode option at SURFRACE_STATE for
IVB. But you remind me that I forgot to do that for gen6 hiz.
>
> > diff --git a/src/mesa/drivers/dri/intel/intel_mipmap_tree.h
> > b/src/mesa/drivers/dri/intel/intel_mipmap_tree.h
> > index 9082864..d175f50 100644
> > --- a/src/mesa/drivers/dri/intel/intel_mipmap_tree.h
> > +++ b/src/mesa/drivers/dri/intel/intel_mipmap_tree.h
> > @@ -215,6 +215,10 @@ struct intel_mipmap_tree
> > /* These are also refcounted:
> > */
> > GLuint refcount;
> > +
> > +#define INTEL_LAYOUT_BELOW 0
> > +#define INTEL_LAYOUT_RIGHT 1
> > + int layout;
> > };
>
> Make it a bool called "miplayout_right" or something instead of
> #defines.
It's fine to me. But I guess it's less readable than using macros, say
the following code:
if ((mt->layout == INTEL_LAYOUT_BELOW && level == mt->first_level + 1) ||
(mt->layout == INTEL_LAYOUT_RIGHT && level == mt->first_level)) {
x += ALIGN(width, mt->align_w);
} else {
y += img_height;
}
which translated to
if ((mt->miplayout_right && level == mt->first_level) ||
(!mt->miplayout_right && level == mt->first_level + 1)) {
x += ALIGN(width, mt->align_w);
} else {
y += img_height;
}
>
> > diff --git a/src/mesa/drivers/dri/intel/intel_tex_layout.c
> > b/src/mesa/drivers/dri/intel/intel_tex_layout.c
> > index 65645bc..4687dd3 100644
> > --- a/src/mesa/drivers/dri/intel/intel_tex_layout.c
> > +++ b/src/mesa/drivers/dri/intel/intel_tex_layout.c
> > @@ -138,7 +138,27 @@ intel_get_texture_alignment_unit(struct intel_context
> > *intel,
> > *h = intel_vertical_texture_alignment_unit(intel, format);
> > }
> >
> > -void i945_miptree_layout_2d(struct intel_mipmap_tree *mt)
> > +static int
> > +i945_miptree_choose_layout(struct intel_context *intel, GLenum target,
> > + GLuint width, GLuint height)
> > +{
> > + int layout = INTEL_LAYOUT_BELOW; /* Use layout _below_ by default */
> > +
> > + /*
> > + * INTEL_LAYOUT_RIGHT is only for:
> > + * GL_TEXTURE_1D, GL_TEXTURE_2D [945+, aka all platforms here]
> > + * GL_TEXTURE_1D, GL_TEXTURE_2D, GL_TEXTURE_CUBE_MAP [gen5+]
> > + */
> > + if (target == GL_TEXTURE_1D || target == GL_TEXTURE_2D ||
> > + (intel->gen >= 5 && target == GL_TEXTURE_CUBE_MAP)) {
> > + if ((height >> 1) >= width)
> > + layout = INTEL_LAYOUT_RIGHT;
> > + }
> > +
> > + return layout;
> > +}
>
> You should explain why the choice is being made here.
Yes, will add it.
>
> > +void i945_miptree_layout_2d(struct intel_context *intel, struct
> > intel_mipmap_tree *mt)
> > {
> > GLuint level;
> > GLuint x = 0;
> > @@ -153,24 +173,32 @@ void i945_miptree_layout_2d(struct intel_mipmap_tree
> > *mt)
> > mt->total_width = ALIGN(mt->width0, mt->align_w);
> > }
> >
> > - /* May need to adjust width to accomodate the placement of
> > - * the 2nd mipmap. This occurs when the alignment
> > - * constraints of mipmap placement push the right edge of the
> > - * 2nd mipmap out past the width of its parent.
> > - */
> > + mt->layout = i945_miptree_choose_layout(intel, mt->target, width,
> > height);
> > + /* Determine the max width */
> > if (mt->first_level != mt->last_level) {
> > - GLuint mip1_width;
> > -
> > - if (mt->compressed) {
> > - mip1_width = ALIGN(minify(mt->width0), mt->align_w)
> > - + ALIGN(minify(minify(mt->width0)), mt->align_w);
> > + GLuint tmp;
> > +
> > + if (mt->layout == INTEL_LAYOUT_BELOW) {
> > + if (mt->compressed) {
> > + tmp = ALIGN(minify(mt->width0), mt->align_w) +
> > + ALIGN(minify(minify(mt->width0)), mt->align_w);
> > + } else {
> > + tmp = ALIGN(minify(mt->width0), mt->align_w) +
> > + minify(minify(mt->width0));
> > + }
> > + } else if (mt->layout == INTEL_LAYOUT_RIGHT) {
> > + if (mt->compressed) {
> > + tmp = ALIGN(mt->width0, mt->align_w) +
> > + ALIGN(minify(mt->width0), mt->align_w);
> > + } else {
> > + tmp = ALIGN(mt->width0, mt->align_w) + minify(mt->width0);
> > + }
> > } else {
> > - mip1_width = ALIGN(minify(mt->width0), mt->align_w)
> > - + minify(minify(mt->width0));
> > + assert(!"mipmap: wrong layout!\n");
> > }
> >
> > - if (mip1_width > mt->total_width) {
> > - mt->total_width = mip1_width;
> > + if (tmp > mt->total_width) {
> > + mt->total_width = tmp;
> > }
> > }
>
> don't call variables "tmp"
Yes, but I don't think that 'mip1_width' is good, too. Since it's
not the width of mip1, but maybe the width of mip1 plus mip2(below mode)
or mip0 plus mip1(right mode). So, I may call it max_mip_width.
Thanks,
Yuanhan Liu
>
> > @@ -191,13 +219,11 @@ void i945_miptree_layout_2d(struct intel_mipmap_tree
> > *mt)
> > */
> > mt->total_height = MAX2(mt->total_height, y + img_height);
> >
> > - /* Layout_below: step right after second mipmap.
> > - */
> > - if (level == mt->first_level + 1) {
> > - x += ALIGN(width, mt->align_w);
> > - }
> > - else {
> > - y += img_height;
> > + if ((mt->layout == INTEL_LAYOUT_BELOW && level == mt->first_level +
> > 1) ||
> > + (mt->layout == INTEL_LAYOUT_RIGHT && level == mt->first_level)) {
> > + x += ALIGN(width, mt->align_w);
> > + } else {
> > + y += img_height;
> > }
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
