On 9 April 2012 10:59, Kenneth Graunke <<a href="mailto:kenneth@whitecape.org">kenneth@whitecape.org</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="im">On 04/09/2012 09:43 AM, Paul Berry wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> When using a separate stencil buffer, i965 requires that the pitch of the buffer (in the 3DSTATE_STENCIL_BUFFER command) be specified as 2x the actual pitch. Previously this was accomplished by doubling the "cpp" and "pitch" values stored in the intel_region data structure, and halving the height. However, this was confusing, and it led to a subtle (but benign) bug: since a stencil buffer is W-tiled, its true height must be aligned to a multiple of 64; we were accidentally aligning its faux height to a multiple of 64, causing memory to be wasted. Note that for window system stencil buffers, the X server also doubles the cpp and pitch values. To facilitate fixing this X server bug in the future, we fix the cpp and pitch values we receive from the X server only if cpp has the "incorrect" value of 2. </blockquote> </div> Paul, This seems like a nice clean-up. I'm glad to see this. I reviewed the code and it seems to make sense, but I'd really want to see Chad or Eric's review on it before it gets pushed. Acked-by: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>> One small comment below:<div><div class="h5"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> diff --git a/src/mesa/drivers/dri/intel/intel_context.c b/src/mesa/drivers/dri/intel/intel_context.c index 16a9887..fc3258b 100644 --- a/src/mesa/drivers/dri/intel/intel_context.c +++ b/src/mesa/drivers/dri/intel/intel_context.c @@ -1251,17 +1251,34 @@ intel_process_dri2_buffer_with_separate_stencil(struct intel_context *intel, int buffer_width; int buffer_height; + int buffer_cpp = buffer->cpp; + int buffer_pitch = buffer->pitch; if (buffer->attachment == __DRI_BUFFER_STENCIL) { - /* The stencil buffer has quirky pitch requirements. From Section - * 2.11.5.6.2.1 3DSTATE_STENCIL_BUFFER, field "Surface Pitch": - * The pitch must be set to 2x the value computed based on width, as - * the stencil buffer is stored with two rows interleaved. + /* Stencil buffers use W tiling, a tiling format that the DRM functions + * don't properly account for. Therefore, when we allocate a stencil + * buffer that is private to Mesa (see intel_miptree_create), we round + * the height and width up to the next multiple of the tile size (64x64) + * and then ask DRM to allocate an untiled buffer. Consequently, the + * height and the width stored in the stencil buffer's region structure + * are always multiples of 64, even if the stencil buffer itself is + * smaller. * - * To satisfy the pitch requirement, the X driver allocated the region - * with the following dimensions. + * To avoid inconsistencies between how we represent private buffers and + * buffers shared with the window system, round up the height and width + * for window system buffers too. */ buffer_width = ALIGN(drawable->w, 64); - buffer_height = ALIGN(ALIGN(drawable->h, 2) / 2, 64); + buffer_height = ALIGN(drawable->h, 64); + + /* As of 4/6/2012, the X server lies and sends cpp and pitch values + * that are two times too large for stencil buffers. Hopefully this + * will be fixed someday, but for now detect the bug by checking if cpp + * is 2, and fixing cpp and pitch if it is. + */ + if (buffer_cpp == 2) { + buffer_cpp = 1; + buffer_pitch /= 2; + } } else { buffer_width = drawable->w; buffer_height = drawable->h; </blockquote> </div></div> I was going to suggest saying "xserver 1.12 lies and sends..." rather than using a date...but...I don't think it's the X server that's really at fault. Isn't it xf86-video-intel (sometimes called the DDX)? So, maybe "As of 4/6/2012, the DDX lies and sends cpp and pitch values"? </blockquote><div> (After some discussion with Chad) how about: "xf86-video-intel versions 2.17.0 and earlier lie and send.... Hopefully this will be fixed in a future version of xf86-video-intel." </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Sadly, I'm not sure how to fix that without breaking new DDX + old Mesa. </blockquote></div> Chad and I have had some discussion about this. We are hoping to add GetParam and SetParam messages to the DDX protocol, to allow Mesa and the DDX to exchange arbitrary integer parameters describing protocol/behavior options. GetParam() will return 0 when queried about a parameter that it does not recognize. The idea is that at startup, Mesa will query the DDX with GetParam to ask "are you capable of telling the truth about cpp and pitch of stencil buffers?" If it gets back a 1 (meaning yes), it will then send a SetParam message to say "henceforth, please tell the truth about cpp and pitch of stencil buffers." Not the prettiest thing in the world, I know, but it would allow old and new versions of the DDX and Mesa to be arbitrarily intermingled.