[Mesa-dev] [PATCH 17/27] i965: Create correctly sized mcs for an image

[Ben Widawsky]

On 16-12-10 15:36:06, Pohjolainen, Topi wrote:
>On Thu, Dec 01, 2016 at 02:09:58PM -0800, Ben Widawsky wrote:
>> From: Ben Widawsky <ben at bwidawsk.net>
>>
>> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
>> ---
>>  src/mesa/drivers/dri/i965/intel_screen.c | 37 ++++++++++++++++++++++++++++----
>>  1 file changed, 33 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c
>> index 0f19a6e..91eb7ec 100644
>> --- a/src/mesa/drivers/dri/i965/intel_screen.c
>> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
>> @@ -545,8 +545,11 @@ create_image_with_modifier(struct intel_screen *screen,
>>  {
>>     uint32_t tiling;
>>     unsigned long pitch;
>> +   unsigned ccs_height = 0;
>>
>>     switch (modifier) {
>> +   case /* I915_FORMAT_MOD_CCS */ fourcc_mod_code(INTEL, 4):
>> +      ccs_height = ALIGN(DIV_ROUND_UP(height, 16), 32);
>>     case I915_FORMAT_MOD_Y_TILED:
>>        tiling = I915_TILING_Y;
>>     }
>> @@ -554,10 +557,35 @@ create_image_with_modifier(struct intel_screen *screen,
>>     /* For now, all modifiers require some tiling */
>>     assert(tiling);
>>
>> +   /*
>> +    * CCS width is always going to be less than or equal to the image's width.
>> +    * All we need to do is make sure we add extra rows (height) for the CCS.
>> +    *
>> +    * A pair of CCS bits correspond to 8x4 pixels, and must be cacheline
>> +    * granularity. Each CCS tile is laid out in 8b strips, which corresponds to
>> +    * 1024x512 pixel region. In memory, it looks like the following:
>> +    *
>> +    * ?????????????????????????????????????????????????????????
>> +    * ???                 ???
>> +    * ???                 ???
>> +    * ???                 ???
>> +    * ???      Image      ???
>> +    * ???                 ???
>> +    * ???                 ???
>> +    * ???xxxxxxxxxxxxxxxxx???
>> +    * ?????????????????????????????????????????????????????????
>> +    * ???     ???           |
>> +    * ???ccs  ???  unused   |
>> +    * ?????????????????????-----------???
>
>I guess this looks okay as actual source code and Mutt just displays it like
>this for me.
>

It's UTF-8 codes. The problem is more likely your terminal than mutt. I can use
ASCII if people prefer.

>> +    * <------pitch------>
>> +    */
>> +   unsigned y_tiled_height = ALIGN(height, 32);
>> +
>>     cpp = _mesa_get_format_bytes(image->format);
>> -   image->bo = drm_intel_bo_alloc_tiled(screen->bufmgr, "image+mod",
>> -                                        width, height, cpp, &tiling,
>> -                                        &pitch, 0);
>> +   image->bo = drm_intel_bo_alloc_tiled(screen->bufmgr,
>> +                                        ccs_height ? "image+ccs" : "image",
>
>Do want to keep "image+mod" for the non-ccs case?
>

Yes, thanks. This was originally a bit different before as the function was
called regardless of whether or not there were modifiers. I'll change it.

>> +                                        width, y_tiled_height + ccs_height,
>> +                                        cpp, &tiling, &pitch, 0);
>>     if (image->bo == NULL)
>>        return false;
>>
>> @@ -575,7 +603,8 @@ create_image_with_modifier(struct intel_screen *screen,
>>     if (image->planar_format)
>>        assert(image->planar_format->nplanes == 1);
>>
>> -   image->aux_offset = 0; /* y_tiled_height * pitch; */
>> +   if (ccs_height)
>> +      image->aux_offset = y_tiled_height * pitch;
>
>Here you set 'aux_offset' relative to the beginning of the actual color region
>and therefore in the previous patch you shouldn't add mt->offset to it,
>right? (Just like I wrote in the previous patch, I think mt->offset is only
>used for moving to subsequent slices within color region).
>

Originally I did add offset here. The formula was:
aux_offset = mt->offset + y_tiled_height * pitch;

So it was consistent at least with the previous patch. We can finish the
discussion of what to do on that patch, and I'll have this part match it.
>>
>>     return true;
>>  }
>> --
>> 2.10.2
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev