[Mesa-dev] [PATCH 2/3] i965: we want 64bit writes for depth count

[Kenneth Graunke]

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 <kenneth at 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