[Mesa-dev] [PATCH 27/34] i965: Make CCS stride match kernel's expectations

[Ben Widawsky]

On 17-02-28 11:44:24, Jason Ekstrand wrote:
>On Mon, Feb 27, 2017 at 7:23 PM, Ben Widawsky <ben at bwidawsk.net> wrote:
>
>> On 17-02-27 18:40:41, Jason Ekstrand wrote:
>>
>>> On Mon, Feb 27, 2017 at 5:38 PM, Jason Ekstrand <jason at jlekstrand.net>
>>> wrote:
>>>
>>> On Mon, Feb 27, 2017 at 4:56 PM, Ben Widawsky <ben at bwidawsk.net> wrote:
>>>>
>>>> On 17-01-31 13:24:55, Jason Ekstrand wrote:
>>>>>
>>>>> On Mon, Jan 23, 2017 at 10:21 PM, Ben Widawsky <ben at bwidawsk.net>
>>>>>> wrote:
>>>>>>
>>>>>> v2: Put the commit message as a comment (Topi)
>>>>>>
>>>>>>>
>>>>>>> Cc: Topi Pohjolainen <topi.pohjolainen at gmail.com>
>>>>>>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>>>>> Cc: Jason Ekstrand <jason at jlekstrand.net>
>>>>>>> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
>>>>>>> Acked-by: Daniel Stone <daniels at collabora.com>
>>>>>>> ---
>>>>>>>  src/mesa/drivers/dri/i965/intel_screen.c | 5 ++++-
>>>>>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c
>>>>>>> b/src/mesa/drivers/dri/i965/intel_screen.c
>>>>>>> index 85070bb54d..12b3b071e4 100644
>>>>>>> --- a/src/mesa/drivers/dri/i965/intel_screen.c
>>>>>>> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
>>>>>>> @@ -1023,7 +1023,10 @@ intel_from_planar(__DRIimage *parent, int
>>>>>>> plane,
>>>>>>> void *loaderPrivate)
>>>>>>>      if (parent == NULL || parent->planar_format == NULL) {
>>>>>>>         if (is_aux) {
>>>>>>>            offset = parent->aux_offset;
>>>>>>> -          stride = ALIGN(parent->pitch / 32, 128);
>>>>>>> +          /* Make CCS stride match kernel's expectations. Mesa's
>>>>>>> internals
>>>>>>> +           * expect: stride = ALIGN(parent->pitch / 32, 128)
>>>>>>> +           */
>>>>>>> +          stride = ALIGN(parent->pitch / 64, 128);
>>>>>>>
>>>>>>>
>>>>>>> I just realized that this doesn't match the picture you drew in 22/32
>>> where
>>> the CCS and the main color surface have the same stride.
>>>
>>>
>> Here is the image, it seems correct to me. Is there some clarification
>> you'd
>> like me to make?
>>
>> ┌─────────────────┐
>> │                 │
>> │                 │
>> │                 │
>> │      Image      │
>> │                 │
>> │                 │
>> │xxxxxxxxxxxxxxxxx│
>> ├─────┬───────────┘
>> │     │           |
>> │ccs  │  unused   |
>> └─────┘-----------┘
>> <------pitch------>
>>
>
>This picture matches what you do for allocation.  You divide the main
>surface height by 64 (rounded up) to get the CCS surface height in Y-tiled
>lines which is correct.  Then you add this to the height of the primary
>surface and pass that to libdrm.  The amount of data allocated for the ccs
>is then DIV_ROUND_UP(color.height, 64) * color.pitch.  That is way more
>than enough data but the waste isn't too bad so whatever.
>
>In patch 19, we use ISL to compute the CCS layout and it picks whatever
>stride it wants which, I believe, will be ALIGN(color.width, 1024) / 8.
>(Note this calculation is done based on the width of the color surface, not
>the stride)
>
>Then, in this patch, we tell the client that the pitch is ALIGN(color.pitch
>/ 64, 128).  There is a factor of two in here thanks to the current kernel
>API, but it's also based on color.pitch not color.width so it's liable to
>mismatch with ISL.
>
>We have three different calculations for the CCS pitch in three different
>places and no two of them match.
>
>It's fine if the allocation doesn't match the others so long as it's it's
>large enough.  The other two, however, need to match (with a possible
>factor of 2 difference).
>
>Is that making sense?
>
>--Jason
>

Unfortunately not, the drawing still seems correct as it's not so specific to
have calculations. Please draw the picture how you think it should be drawn, and
let's ignore the factor of two issue (I separately responded we should just
submit stride in units of tiles). I'll gladly make the picture whatever you want
it to be.

>
>>
>>> Does the kernel expect the alignment to be 128 or 64?  Given that ville
>>>>
>>>>> likes 64-wide tiles, I think it should be 64.  Really, I think the more
>>>>>> accurate calculation would be
>>>>>>
>>>>>> stride = ALIGN(parent->pitch, 4096) / 64;
>>>>>>
>>>>>>
>>>>> Isn't the actual CCS stride stored in parent->strides[0]?  Why are we
>>> re-computing it from the plane 0 pitch?
>>>
>>>
>> I believe the parent is just the parent image (ie. pixel data), in real
>> planar
>> images, we have the planes for the formats, but it's somewhat of a hack job
>> here. If I have this wrong, or you have a better idea, please let me know.
>>
>>
>>
>>> 4096 is the stride in bytes in the primary surface required to cross a
>>>>
>>>>> single CCS tile.  The calculation you have above will work in the sense
>>>>>> that the worst that happens is for it to align up a bit too far.  In
>>>>>> any
>>>>>> case, what matters is that we a) have enough space and b) exactly match
>>>>>> the
>>>>>> kernel's calculation.
>>>>>>
>>>>>> --Jason
>>>>>>
>>>>>>
>>>>>>
>>>>>> This formula doesn't match the kernel's expectations (unless Ville has
>>>>> updated
>>>>> patches somewhere).
>>>>>
>>>>>
>>>> Hrm... Ok.  Having the alignment too big won't break anything, it's just
>>>> a
>>>> bit odd.
>>>>
>>>>
>>>> If you want I can do:
>>>>> stride = ALIGN(parent->pitch, 4096) / 128;
>>>>>
>>>>
>>>>
>>>>  That' wouldn't match either.  Let's make sure the formulas match
>>>> exactly.
>>>>
>>>>
>>>>