[Mesa-dev] [PATCH 2/3] i965: add noise padding to buffer object and function to check if noise is correct

[Rogovin, Kevin]

Hi,

 Thankyou for reading the code and giving advice to improve upon it. Below are some thoughts:

> I can't help but think that this could be a bit simpler and involve throwing fewer pointers around.

I was thinking this too; the easiest way to do this is to just have the same noise for all the paddings;
that would mean there is just one pointer of data and that would be a private member of brw_bufmgr.

> This is 4096.  I think we could just have a single uint32_t padding field which is either 0 or 4096 (More on that later).

If the kernel supports huge pages, though now there might be "two" different things as far as page size goes then:
the page size for CPU things and the page size for the PPGTT. I don't know if they must be the same or if they can
be different. I also do not know how to actually get the page size used by the PPGTT, as getpagesize() is the page
size for the CPU page tabling magic. Any suggestions on how to get the page size used for the PPGTT? I think it is
worthwhile to make sure that atleast some of the noise is in the next page, but I admit that is just a hunchy thing.

> Does using rand() really help us?  Why not just come up with some hash-like thing which generates consistent pseudo-random data? 
> How about something like "value.[i] = i * 853 + 193"  (some random primes)?  That would mean that we can generate the data and check
> it without having to store it in a per-bo temporary.  If you want it to be magic per-bo, you could also seed it somehow with the bo handl
> (just add handle * 607).

I figured that rand() was the most reliable way to generate noise in addition to the least amount of code. However, if the padding values
are generated with an internal routine (maybe something like value[i] = 223 * value[i - 1] + 123, with value[0] =  handle; all truncated to 8-bits),
that would drop the need completely for the per-buffer storage.


> If we always allocate 4096B of padding, then you don't need to heap allocate it and can just put it on the stack for the purpose of interacting with
 > pread/pwrite.  It's a bit big but still perfectly reasonable.  If a day came when we wanted to make the padding size adjustable, it would still 
> probably be reasonable to make the heap allocations temporary so we have less random stuff in the BO we have to cleanup.

I was tempted to make it stack allocated, but the 4096 size scared me off... and when I thought of huge page support of 2M I ran screaming.
 
> There's a part of me that wants to kill pread/write.  However, I think you may have come up with the only good use of it I've ever seen. :-)

I was told that one of the advantages of pread is the kernel will then do the syncing magic for you, i.e. waiting for the GPU to be done with the
buffer;  I freely admit that now I am not 100% sure of this.
 
> If we still keep these heap allocations, deleting them should be keyed off of bo->padding.size or nothing at all.

That is how I originally wrote it, but to handle the case where creating a brw_bo fails midway (i.e. after GEM create ioctl, but during the tmp buffer allocation), checking the existence by .size > 0 was not going to work. However, for everywhere else it is fine.
 
At this point I am tempted to do the following for the noise padding:
  1. take your suggestion and make the noise per brw_bo, but the noise is generated with an incremental chaotic function that uses GEM-handle value as a start
  2. have a single integer in the brw_bo struct indicating the amount of noise padding it has;
  3. for checking use that single integer to heap-allocate a temporary buffer to store the pread contents OR have the necessary syncing operations and use the mapping pointer to read the values. The latter is more tempting I admit though.

-Kevin