On 07/02/2012 01:16 AM, Daniel Vetter wrote: > On Sun, Jul 01, 2012 at 07:59:56PM -0700, Kenneth Graunke wrote: >> On 06/26/2012 07:28 AM, Daniel Vetter wrote: >>> ... and the hardware seems to take the lenght of the pipe control >>> command to indicate whether the write is 64bit or 32bit. Which makes >>> sense for immediate writes. >>> >>> I've discovered this by writing a pattern into the query object bo and >>> noticing that the high 32bits are left intact, even on those pipe >>> control writes that seemingly worked. >> >> I can't find any documentation or other justification for this, but if >> you think it's useful, adding the extra 0 shouldn't hurt anything. > > Hm, I wonder whether fullsim has any indication?
I couldn't find any, but that could just mean I didn't look hard enough. Or, it could be that the actual hardware behaves differently. I've found one other instance of that. >>> diff --git a/src/mesa/drivers/dri/intel/intel_reg.h >>> b/src/mesa/drivers/dri/intel/intel_reg.h >>> index e2a6ee2..04f7a8d 100644 >>> --- a/src/mesa/drivers/dri/intel/intel_reg.h >>> +++ b/src/mesa/drivers/dri/intel/intel_reg.h >>> @@ -59,6 +59,7 @@ >>> * additional flushing control. >>> */ >>> #define _3DSTATE_PIPE_CONTROL (CMD_3D | (3 << 27) | (2 << 24) >>> | 2) >>> +#define _3DSTATE_PIPE_CONTROL_5 (CMD_3D | (3 << 27) | (2 << 24) >>> | 3) >>> #define PIPE_CONTROL_CS_STALL (1 << 20) >>> #define PIPE_CONTROL_GLOBAL_SNAPSHOT_COUNT_RESET (1 << 19) >>> #define PIPE_CONTROL_TLB_INVALIDATE (1 << 18) >>> >> >> and then no need to add this. >> >> With that change, this is: >> Acked-by: Kenneth Graunke <kenn...@whitecape.org> >> >> Though, again, I'm not sure why it'd be necessary. > > Well, it is, if we don't do it the hw doesn't write the upper 32bits. As > the commit message says, I've proved this by writing garbage into the bo > and noticing that the upper 32bit stay. Of course, it's pretty hard to > actually hit this, rendering 2^32 pixels and then doing an occlusion query > right accross the warp-around is really hard to do ;-) Same thing applies > to timer queries. Yeah, I believe you :) It just would be nice to find some justification. But it's not really worth the extra hunting if this fixes the issue. > I'll do the bikeshed as a separate patch - we use pipe control at tons of > other places. > -Daniel To be clear: I wasn't suggesting changing all instances of PIPE_CONTROL to have | (4 - 2) in them, just adding | (5 - 2) here, since | 2 | 3 is equivalent to | 3. Incremental change. Of course, if you want to change them all, I'd gladly ack that! --Ken _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
