On Tue, Jul 7, 2015 at 2:35 AM, Kenneth Graunke <kenn...@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.pho...@gmail.com> >> Cc: Ben Widawsky <b...@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 >> _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev