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

[Ben Widawsky]

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


>
>> 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.
>>
>>