[Mesa-dev] [PATCH v2 3/3] i965: if DEBUG_OUT_OF_BOUND_CHK is up, check that noise padding for each bo used in batchbuffer is correct

[Rogovin, Kevin]

Just to make sure of the reason of the original objection to trusting pread blindly: 
pread will miss writes IF the GEM BO was not listed in the execbuffer2 ioctl. 

If that is the case, I am ok with missing those writes because those writes (should) only
occur if the GPU writes to padding of the BO by bad-luck; this series is looking really to
just protect against that buffer B is written to at offset O where offset O is larger than
the size of buffer B (the series only checks for writes where O is also less than the size plus
the padding).

Detecting the bad-luck writes requires much more pain and suffering:
  1. Scratch contents read/write
  2. For BO's that driver knows are not writeable by GPU to then mirror 
       those BO contents.

(1) is not so bad, but (2) is terribly hideous.

With the goal that the patch series is only looking to detect out-of-bound
writes (up to padding size), I think pread would be fine then.

Thoughts?

-Kevin

P.S. I am already writing it to do mapping and such, but it is more code sadly.

-----Original Message-----
From: Chris Wilson [mailto:chris at chris-wilson.co.uk] 
Sent: Wednesday, December 13, 2017 1:23 PM
To: Rogovin, Kevin <kevin.rogovin at intel.com>; mesa-dev at lists.freedesktop.org
Cc: joonas.lahtinen at linux.intel.com
Subject: RE: [Mesa-dev] [PATCH v2 3/3] i965: if DEBUG_OUT_OF_BOUND_CHK is up, check that noise padding for each bo used in batchbuffer is correct

Quoting Rogovin, Kevin (2017-12-13 11:19:10)
> Hi,
> 
> > Actually that's not strictly true. Since you only do a pread here, it will only synchronize against the last declared write to the bo.
> > There's no guaranteed sync with the last batch for a set of read-only bo.
> > Similarly, because of no domain tracking, it won't also ensure that the bo is cache coherent before the read back.
> 
> Futz. Would then doing brw_bo_map() with flags having value MAP_READ 
> be sufficient then? This is what function
> brw_get_buffer_subdata() of intel_buffer_objects.c does to copy BO contents to application supplied buffer.

Yes. I would also pencil in a note to do a map_range, or at least MAP_INCOHERENT flag to avoid the gen_invalidate_range and do the cache invalidation manually (for !llc).
-Chris