[Mesa-dev] i965: Transition the rest of surfaces (i.e., color) to isl

[Tapani Pälli]

On 07/29/2017 08:21 AM, Jason Ekstrand wrote:
> On Fri, Jul 28, 2017 at 10:14 PM, Tomasz Figa <tfiga at chromium.org 
> <mailto:tfiga at chromium.org>> wrote:
> 
>     On Sat, Jul 29, 2017 at 2:06 PM, Jason Ekstrand
>     <jason at jlekstrand.net <mailto:jason at jlekstrand.net>> wrote:
>      > On Fri, Jul 28, 2017 at 8:17 PM, Tomasz Figa <tfiga at chromium.org
>     <mailto:tfiga at chromium.org>> wrote:
>      >>
>      >> Hi Topi, Jason,
>      >>
>      >> On Sat, Jul 22, 2017 at 12:00 AM, Topi Pohjolainen
>      >> <topi.pohjolainen at gmail.com <mailto:topi.pohjolainen at gmail.com>>
>     wrote:
>      >> > First patch actually should have been included already when
>      >> > gen6 stencil got transitioned - it has been giving warning ever
>      >> > since.
>      >> >
>      >> > Most of the work actually got already done for depth surfaces
>     (which
>      >> > is y-tiled such as color surfaces). What is left are color surface
>      >> > specifics, mostly preparing for corner cases.
>      >> >
>      >> > This is now all green in ci-system. For snb and older i965 wasn't
>      >> > checking hardware incapabilities as hard as isl does. Certain
>      >> > format/size/msaa combinations were allowed that shouldn't have.
>      >> > Moving to isl exposed code paths that didn't report surface
>     creation
>      >> > failures resulting in asserts firing later on. Patches 10 and 11
>      >> > now properly tell the client if the surface type can't be
>     supported
>      >> > allowing piglit tests to skip them.
>      >>
>      >> I think it might be related to previously merged patches, but the
>      >> topic is still ISL, so let me ask my question here. Is there any
>      >> possibility to add some diagnostic information to the validation
>     code?
>      >> We've been seeing EGL image import failures on new Mesa as a
>     result of
>      >> ISL catching issues in our allocator (cros_gralloc on top of
>     ChromeOS
>      >> minigbm), but it's close to impossible to identify the cause without
>      >> manually inserting some printfs and recompiling the code. I think
>      >> having some error messages printed in case of a buffer validation
>      >> failure would be a great benefit.
>      >
>      >
>      > What kind of prints are you looking for exactly?  I've run into
>     some issues
>      > myself but if you could provide a concrete example, that would help.
> 
>     For example, we hit two different issues where the total size check in
>     intel_create_image_from_fds_common() was failing and the only way to
>     figure out the exact reason was digging deep into the calculations in
>     ISL:
> 
>     1) missing 64-byte padding from the end of linear buffers,
> 
>     2) (total) height of the buffer not aligned to 4-lines - actually the
>     problem is much more interesting, because this was specifically with
>     Android's HAL_PIXEL_FORMAT_YV12 (3-plane YVU420), which doesn't permit
>     height alignment higher than 2 defined by the spec. Mesa seems to care
>     about total size of the buffer only, so we could work around this by
>     simply adding enough padding at the end of the buffer.
> 
> 
> Right... I was concerned about that when I landed those changes but 
> never saw a problem in my testing (probably due to piglit always 
> choosing "nice" sizes).  I've considered simply deleting that code from 
> ISL because I'm not at all convinced that it's needed.  Chad, do you 
> have any thoughts on this?  my understanding is that the padding is only 
> needed to avoid page faults in the GPU and, since every byte of our 
> address space has pages (thanks to the scratch page) this shouldn't 
> actually be a problem.
> 

Please CC me if some of these restrictions will be removed, we applied 
fixes for these in minigbm when Mesa was rebased in Android-IA.

// Tapani