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

[Anuj Phogat]

On Tue, Jul 7, 2015 at 2:35 AM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On Tuesday, June 23, 2015 01:23:05 PM 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.
>>
>> V2: Delete min/max buffer size restrictions not valid for i965+.
>>     Remove redundant align to tile size statements.
>>     Remove some redundant code now when there are no min/max buffer size.
>>
>> 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 | 62 +++++++++++++++++++++++++--
>>  1 file changed, 58 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>> index 80c52f2..5bcb094 100644
>> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>> @@ -558,6 +558,48 @@ intel_lower_compressed_format(struct brw_context *brw, mesa_format format)
>>     }
>>  }
>>
>> +/* This function computes Yf/Ys tiled bo size, alignment and pitch. */
>> +static uint64_t
>> +intel_get_yf_ys_bo_size(struct intel_mipmap_tree *mt, unsigned *alignment,
>> +                        uint64_t *pitch)
>
> Hi Anuj,
>
> This patch has a subtle bug: you've specified pitch and stride to be
> uint64_t here, but below when you call it....
>
> [snip]
>> @@ -616,11 +658,23 @@ intel_miptree_create(struct brw_context *brw,
>>        alloc_flags |= BO_ALLOC_FOR_RENDER;
>>
>>     unsigned long pitch;
>> -   mt->bo = drm_intel_bo_alloc_tiled(brw->bufmgr, "miptree", total_width,
>> -                                     total_height, mt->cpp, &mt->tiling,
>> -                                     &pitch, alloc_flags);
>>     mt->etc_format = etc_format;
>> -   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, &pitch);
>
> ...you're passing a pointer to an unsigned long.  On 32-bit builds,
> unsigned long is a 4 byte value, while uint64_t is 8 bytes.  This could
> lead to stack corruption.  (GCC warns about this during a 32-bit build.)
>
Thanks for noticing this Ken. I think I never did 32 bit build with these
patches :(.

> I assumed the solution was to make everything uint32_t, but apparently
> drm_intel_bo_alloc_tiled actually expects an unsigned long.  So we can't
> change that.
>
How about changing the parameter type of pitch to unsigned long*
and types of size and stride to unsigned long? This fixes the 32 bit
build warnings.

> Then I looked at your code, and realized that nothing even uses the
> pitch value.  Is there some point to the parameter existing at all?
>
pitch value is later assigned to mt->pitch. I could have avoided
passing &pitch parameter and instead assign mt->pitch in
drm_intel_bo_alloc_for_render(). But, I used the current approach
to keep mt->pitch assignments at a single place.

I'm working on some refactoring to make this code look better.

> --Ken
>
>> +      assert(size);
>> +      mt->bo = drm_intel_bo_alloc_for_render(brw->bufmgr, "miptree",
>> +                                             size, alignment);
>> +      mt->pitch = pitch;
>> +   } else {
>> +      mt->bo = drm_intel_bo_alloc_tiled(brw->bufmgr, "miptree",
>> +                                        total_width, total_height, mt->cpp,
>> +                                        &mt->tiling, &pitch,
>> +                                        alloc_flags);
>> +      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
>>