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

[Anuj Phogat]

On Tue, Jul 7, 2015 at 12:11 PM, Anuj Phogat <anuj.phogat at gmail.com> wrote:
> 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
Correction: assign mt->pitch in intel_get_yf_ys_bo_size()
> 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
>>>