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

[Kenneth Graunke]

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

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.

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?

--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
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150707/2dadbb29/attachment.sig>