<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">Woah, wall of e-mails. :-) </div><div class="gmail_quote"> On Tue, May 9, 2017 at 5:02 AM, Pohjolainen, Topi <<a href="mailto:topi.pohjolainen@gmail.com" target="_blank">topi.pohjolainen@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Tue, May 09, 2017 at 02:44:35PM +0300, Pohjolainen, Topi wrote: > On Tue, May 09, 2017 at 02:36:47PM +0300, Pohjolainen, Topi wrote: > > On Tue, May 09, 2017 at 02:29:13PM +0300, Pohjolainen, Topi wrote: > > > On Tue, May 09, 2017 at 08:25:28AM +0300, Pohjolainen, Topi wrote: > > > > On Mon, May 08, 2017 at 03:31:55PM -0700, Jason Ekstrand wrote: > > > > > On Wed, May 3, 2017 at 2:22 AM, Topi Pohjolainen <<a href="mailto:topi.pohjolainen@gmail.com">topi.pohjolainen@gmail.com</a> > > > > > > wrote: > > > > > > > > > > > Signed-off-by: Topi Pohjolainen <<a href="mailto:topi.pohjolainen@intel.com">topi.pohjolainen@intel.com</a>> > > > > > > --- > > > > > > src/intel/isl/isl_gen7.c | 6 +++++- > > > > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/src/intel/isl/isl_gen7.c b/src/intel/isl/isl_gen7.c > > > > > > index 18687b5..cf5b377 100644 > > > > > > --- a/src/intel/isl/isl_gen7.c > > > > > > +++ b/src/intel/isl/isl_gen7.c > > > > > > @@ -375,7 +375,11 @@ gen7_choose_valign_el(const struct isl_device *dev, > > > > > > * FINISHME(chadv): Decide to set valign=4 or valign=8 after isl's > > > > > > API > > > > > > * is more polished. > > > > > > */ > > > > > > - require_valign4 = true; > > > > > > + > > > > > > + /* Using valign 4 upsets all depthstencil-render-miplevels on IVB > > > > > > and > > > > > > + * HSW. Use alignment 8 instead. > > > > > > + */ > > > > > > + return 8; > > > > > > > > > > > > > > > > I'm confused. This seems to contradict the documentation which says that > > > > > valign for depth is hard-coded to 4 in the hardware. > > > > > > > > Just as I am. It took me a while to actually try increasing the alignment - it > > > > was just after I compared what the current logic in i965 does. The thing here > > > > is not the alignment itself but the amount of space that gets allocated. I'll > > > > get back with more details shortly. > > > > > > In intel_miptree_set_alignment() we have: > > > > > > } else if (mt->format == MESA_FORMAT_S_UINT8) { > > > mt->halign = 8; > > > mt->valign = brw->gen >= 7 ? 8 : 4; > > > } else { > > > > > > There are a few refactors it seems but I'll try to find the original commit. > > > > Well that looks to go a long way back to a file named intel_tex_layout.c. > > Some documentation is still left: > > > > /** > > * +----------------------------------------------------------------------+ > > * | | alignment unit height ("j") | > > * | Surface Property |-----------------------------| > > * | | 915 | 965 | ILK | SNB | IVB | > > * +----------------------------------------------------------------------+ > > * | BC1-5 compressed format (DXTn/S3TC) | 4 | 4 | 4 | 4 | 4 | > > * | FXT1 compressed format | 4 | 4 | 4 | 4 | 4 | > > * | Depth Buffer | 2 | 2 | 2 | 4 | 4 | > > * | Separate Stencil Buffer | N/A | N/A | N/A | 4 | 8 | > > * | Multisampled (4x or 8x) render target | N/A | N/A | N/A | 4 | 4 | > > * | All Others | 2 | 2 | 2 | * | * | > > * +----------------------------------------------------------------------+ > > * > > * Where "*" means either VALIGN_2 or VALIGN_4 depending on the setting of > > * the SURFACE_STATE "Surface Vertical Alignment" field. > > */ > > > > Original included also a reference to bspec: > > > > * From the "Alignment Unit Size" section of various specs, namely: > > * - Gen3 Spec: "Memory Data Formats" Volume, Section 1.20.1.4 > > * - i965 and G45 PRMs: Volume 1, Section 6.17.3.4. > > * - Ironlake and Sandybridge PRMs: Volume 1, Part 1, Section 7.18.3.4 > > * - BSpec (for Ivybridge and slight variations in separate stencil) > > > > > > Original commit: > > commit e7e81714f3ecf67a975d35e74bdb7fd15d924e4d > Author: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> > Date: Mon Nov 7 15:58:43 2011 -0800 > > i965: Implement the actual tables for texture alignment units [v2] > > I implemented functions for horizontal/vertical alignment units separately > because I find it easier to read that way...especially with all the > corner-cases. > > [chad] Corrected the vertical alignment calculation by checking for > depthstencil formats. > > v2: > - Fix typos in intel_horizontal_texture_alignment_unit(): > s/height/width/ and s/VALIGN/HALIGN. > - Remove special case for compressed formats in > intel_get_texture_alignment unit(). Compressed formats are already > handled in the halign and valign functions. > - Replace check ``_mesa_is_depth_format(...) || > _mesa_is_depthstencil_format(...)`` with explcitit checks against > GL_DEPTH_COMPONENT and GL_DEPTH_STENCIL. > > Reviewed-by: Eric Anholt <<a href="mailto:eric@anholt.net">eric@anholt.net</a>> > Signed-off-by: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> > Signed-off-by: Chad Versace <<a href="mailto:chad.versace@linux.intel.com">chad.versace@linux.intel.com</a>> > </div></div>And digging PRMs: <a href="https://01.org/sites/default/files/documentation/ivb_ihd_os_vol1_part1.pdf" rel="noreferrer" target="_blank">https://01.org/sites/default/files/documentation/ivb_ihd_os_vol1_part1.pdf</a> 6.18.4.4 Alignment Unit Size This section documents the alignment parameters i and j that are used depending on the surface. surface defined by surface format unit width “i” unit height “j” 3DSTATE_STENCIL_BUFFER N/A 8 8 </blockquote></div> </div><div class="gmail_extra">Reading things a bit more, chad's comment there is entirely bunk. The fact that you have to multiply pitch by 2 is, in my understanding, completely unrelated to the halign/valign parameters used for texture layout. Well, not completely unrelated. W-tiling is just Y-tiling with a bit of extra swizzling which turns 8x8 W-tiled chunks into 16x4 y-tiled chunks so it makes everything easier in hardware if miplevels are always aligned to 8x8. The fact that RENDER_SURFACE_STATE::SurfaceVerticalAlignmet doesn't have a VALIGN_8 option doesn't matter since this is SNB and you can't do stencil texturing anyway. </div><div class="gmail_extra">Let's believe the docs and call it 8. We should also delete most of chad's comment while we're at it. Better yet, for gen8 I copied the table out of the PRM and made it a nicely formatted comment. Maybe we should do the same for gen7 and get rid of Chad's comment entirely? </div><div class="gmail_extra">As a further side-note, I don't think I like the way the alignment getter fuctions work today all that much. I'll send out an RFC patch that I think makes things better. </div><div class="gmail_extra">--Jason </div></div>