[Mesa-dev] [PATCH 5/5] i965/gen9: Allocate YF/YS tiled buffer objects

[Anuj Phogat]

On Mon, Jun 22, 2015 at 5:13 PM, Ben Widawsky <ben at bwidawsk.net> wrote:
> On Wed, Jun 10, 2015 at 03:30:50PM -0700, Anuj Phogat wrote:
>> In case of I915_TILING_{X,Y} we need to pass tiling format to libdrm
>> using drm_intel_bo_alloc_tiled(). But, In case of YF/YS tiled buffers
>> libdrm need not know about the tiling format because these buffers
>> don't have hardware support to be tiled or detiled through a fenced
>> region. libdrm still need to know buffer alignment value for its use
>> in kernel when resolving the relocation.
>>
>> Using drm_intel_bo_alloc_for_render() for YF/YS tiled buffers
>> satisfy both the above conditions.
>>
>> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
>> Cc: Ben Widawsky <ben at bwidawsk.net>
>> ---
>>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 86 +++++++++++++++++++++++++--
>>  1 file changed, 80 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>> index 615cbfb..d4d9e76 100644
>> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>> @@ -522,6 +522,65 @@ intel_lower_compressed_format(struct brw_context *brw, mesa_format format)
>>     }
>>  }
>>
>> +/* This function computes Yf/Ys tiled bo size and alignment. */
>
> It also computes pitch for the yf/ys case
>
>> +static uint64_t
>> +intel_get_yf_ys_bo_size(struct intel_mipmap_tree *mt, unsigned *alignment)
>> +{
>> +   const uint32_t bpp = mt->cpp * 8;
>> +   const uint32_t aspect_ratio = (bpp == 16 || bpp == 64) ? 2 : 1;
>> +   uint32_t tile_width, tile_height;
>> +   const uint64_t min_size = 512 * 1024;
>> +   const uint64_t max_size = 64 * 1024 * 1024;
>
> Where do min/max come from? Add a comment?
>
Your suspicion is right. These values are not applicable to i965+.
I copied this from libdrm and is applicable only to older chips.

>> +   uint64_t i, stride, size, aligned_y;
>> +
>> +   assert(mt->tr_mode != INTEL_MIPTREE_TRMODE_NONE);
>> +
>> +   switch (bpp) {
>> +   case 8:
>> +      tile_height = 64;
>> +      break;
>> +   case 16:
>> +   case 32:
>> +      tile_height = 32;
>> +      break;
>> +   case 64:
>> +   case 128:
>> +      tile_height = 16;
>> +      break;
>> +   default:
>> +      tile_height = 0;
>
> make this unreachable()
>
sure
>> +      printf("Invalid bits per pixel in %s: bpp = %d\n",
>> +             __FUNCTION__, bpp);
>> +   }
>
> I think ideally you should roll this logic into intel_miptree_get_tile_masks().
>
I wasn't aware of this function. If it's fine with you, I'll do it in
a follow up patch.

>> +
>> +   if (mt->tr_mode == INTEL_MIPTREE_TRMODE_YS)
>> +      tile_height *= 4;
>> +
>> +   aligned_y = ALIGN(mt->total_height, tile_height);
>> +
>> +   stride = mt->total_width * mt->cpp;
>> +   tile_width = tile_height * mt->cpp * aspect_ratio;
>> +   stride = ALIGN(stride, tile_width);
>> +   size = stride * aligned_y;
>> +
>> +   if (mt->tr_mode == INTEL_MIPTREE_TRMODE_YF) {
>> +      *alignment = 4096;
>> +      size = ALIGN(size, 4096);
>> +   } else {
>> +      *alignment = 64 * 1024;
>> +      size = ALIGN(size, 64 * 1024);
>> +   }
>
> Hmm. I think the above calculation for size is redundant since you already
> aligned to tile_width and height, above. Right? assert((size % 64K) == 0);
>
right. I'll put in assert.
>> +
>> +   if (size > max_size) {
>> +      mt->tr_mode = INTEL_MIPTREE_TRMODE_NONE;
>> +      return 0;
>> +   } else {
>> +      mt->pitch = stride;
>> +      for (i = min_size; i < size; i <<= 1)
>> +         ;
>> +      return i;
>
> I don't understand this. Why don't you just return size? It seems incredibly
> wasteful to both start a 512K, and to increment by powers of 2. Did I miss
> something?
>
> Also, I don't understand max_size. I must be missing something in the spec with
> the min/max values, can you point me to them?
>
I'll get rid of this bogus code.

>> +   }
>> +}
>>
>>  struct intel_mipmap_tree *
>>  intel_miptree_create(struct brw_context *brw,
>> @@ -575,12 +634,27 @@ intel_miptree_create(struct brw_context *brw,
>>
>>     unsigned long pitch;
>>     mt->etc_format = etc_format;
>> -   mt->bo = drm_intel_bo_alloc_tiled(brw->bufmgr, "miptree",
>> -                                     total_width, total_height, mt->cpp,
>> -                                     &mt->tiling, &pitch,
>> -                                     (expect_accelerated_upload ?
>> -                                      BO_ALLOC_FOR_RENDER : 0));
>> -   mt->pitch = pitch;
>> +
>> +   if (mt->tr_mode != INTEL_MIPTREE_TRMODE_NONE) {
>> +      unsigned alignment = 0;
>> +      unsigned long size;
>> +      size = intel_get_yf_ys_bo_size(mt, &alignment);
>> +
>> +      /* intel_get_yf_ys_bo_size() might change the tr_mode. */
>> +      if (size > 0 && mt->tr_mode != INTEL_MIPTREE_TRMODE_NONE) {
>> +         mt->bo = drm_intel_bo_alloc_for_render(brw->bufmgr, "miptree",
>> +                                                size, alignment);
>> +      }
>> +   }
>> +
>> +   if (mt->tr_mode == INTEL_MIPTREE_TRMODE_NONE) {
>> +      mt->bo = drm_intel_bo_alloc_tiled(brw->bufmgr, "miptree",
>> +                                        total_width, total_height, mt->cpp,
>> +                                        &mt->tiling, &pitch,
>> +                                        (expect_accelerated_upload ?
>> +                                         BO_ALLOC_FOR_RENDER : 0));
>> +      mt->pitch = pitch;
>> +   }
>>
>>     /* If the BO is too large to fit in the aperture, we need to use the
>>      * BLT engine to support it.  Prior to Sandybridge, the BLT paths can't
>
> Assuming we can sort out the min/max stuff, and power of 2 size, the rest looks
> good.
You want to see a new version or I can use your r-b after the suggested changes
and zero piglit regressions?