On Mon, Jan 6, 2014 at 11:00 AM, Kenneth Graunke <kenn...@whitecape.org> wrote:
> On 01/05/2014 02:28 PM, Eric Anholt wrote:
>> Anuj Phogat <anuj.pho...@gmail.com> writes:
>>
>>> On Mon, Dec 23, 2013 at 4:08 PM, Eric Anholt <e...@anholt.net> wrote:
>>>> Noticed by tex3d-maxsize on my next commit to check that our addresses
>>>> don't overflow.
>>>> ---
>>>> src/mesa/drivers/dri/i965/intel_blit.c | 20 ++++++++++++++++++++
>>>> src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 23 ++++++++++++++++++++---
>>>> 2 files changed, 40 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/src/mesa/drivers/dri/i965/intel_blit.c
>>>> b/src/mesa/drivers/dri/i965/intel_blit.c
>>>> index 7bc289f..13cc777 100644
>>>> --- a/src/mesa/drivers/dri/i965/intel_blit.c
>>>> +++ b/src/mesa/drivers/dri/i965/intel_blit.c
>>>> @@ -229,12 +229,32 @@ intel_miptree_blit(struct brw_context *brw,
>>>> src_x += src_image_x;
>>>> src_y += src_image_y;
>>>>
>>>> + /* The blitter interprets the 16-bit src x/y as a signed 16-bit value,
>>>> + * where negative values are invalid. The values we're working with
>>>> are
>>>> + * unsigned, so make sure we don't overflow.
>>>> + */
>>>> + if (src_x >= 32768 || src_y >= 32768) {
>>>> + perf_debug("Falling back due to >=32k src offset (%d, %d)\n",
>>>> + src_x, src_y);
>>>> + return false;
>>>> + }
>>>> +
>>>> uint32_t dst_image_x, dst_image_y;
>>>> intel_miptree_get_image_offset(dst_mt, dst_level, dst_slice,
>>>> &dst_image_x, &dst_image_y);
>>>> dst_x += dst_image_x;
>>>> dst_y += dst_image_y;
>>>>
>>>> + /* The blitter interprets the 16-bit destination x/y as a signed 16-bit
>>>> + * value. The values we're working with are unsigned, so make sure we
>>>> + * don't overflow.
>>>> + */
>>>> + if (dst_x >= 32768 || dst_y >= 32768) {
>>>> + perf_debug("Falling back due to >=32k dst offset (%d, %d)\n",
>>>> + dst_x, dst_y);
>>>> + return false;
>>>> + }
>>>> +
>>>> if (!intelEmitCopyBlit(brw,
>>>> src_mt->cpp,
>>>> src_pitch,
>>>> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>>>> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>>>> index de47143..0818226 100644
>>>> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>>>> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>>>> @@ -443,7 +443,8 @@ intel_miptree_choose_tiling(struct brw_context *brw,
>>>> if (minimum_pitch < 64)
>>>> return I915_TILING_NONE;
>>>>
>>>> - if (ALIGN(minimum_pitch, 512) >= 32768) {
>>>> + if (ALIGN(minimum_pitch, 512) >= 32768 ||
>>>> + mt->total_width >= 32768 || mt->total_height >= 32768) {
>>>> perf_debug("%dx%d miptree too large to blit, falling back to
>>>> untiled",
>>>> mt->total_width, mt->total_height);
>>>> return I915_TILING_NONE;
>>>> @@ -2233,6 +2234,22 @@ intel_miptree_release_map(struct intel_mipmap_tree
>>>> *mt,
>>>> *map = NULL;
>>>> }
>>>>
>>>> +static bool
>>>> +can_blit_slice(struct intel_mipmap_tree *mt,
>>>> + unsigned int level, unsigned int slice)
>>>> +{
>>>> + uint32_t image_x;
>>>> + uint32_t image_y;
>>>> + intel_miptree_get_image_offset(mt, level, slice, &image_x, &image_y);
>>>> + if (image_x >= 32768 || image_y >= 32768)
>>>> + return false;
>>>> +
>>>> + if (mt->region->pitch >= 32768)
>>>> + return false;
>>>> +
>>>> + return true;
>>>> +}
>>>> +
>>>> static void
>>>> intel_miptree_map_singlesample(struct brw_context *brw,
>>>> struct intel_mipmap_tree *mt,
>>>> @@ -2276,11 +2293,11 @@ intel_miptree_map_singlesample(struct brw_context
>>>> *brw,
>>>> !mt->compressed &&
>>>> (mt->region->tiling == I915_TILING_X ||
>>>> (brw->gen >= 6 && mt->region->tiling == I915_TILING_Y)) &&
>>>> - mt->region->pitch < 32768) {
>>>> + can_blit_slice(mt, level, slice)) {
>>>> intel_miptree_map_blit(brw, mt, map, level, slice);
>>>> } else if (mt->region->tiling != I915_TILING_NONE &&
>>>> mt->region->bo->size >= brw->max_gtt_map_object_size) {
>>>> - assert(mt->region->pitch < 32768);
>>>> + assert(can_blit_slice(mt, level, slice));
>>> I think the right thing to do here is:
>>> - mt->region->bo->size >= brw->max_gtt_map_object_size) {
>>> - assert(mt->region->pitch < 32768);
>>> + mt->region->bo->size >= brw->max_gtt_map_object_size &&
>>> + can_blit_slice(mt, level, slice)) {
>>>
>>> This allow us falling back to intel_miptree_map_gtt(). I suggested this
>>> change
>>> as a part of '[PATCH] i965: Fix the region's pitch condition to use
>>> blitter'.
>>
>> If region->bo->size >= max_gtt_map_object_size, you can't fall back to
>> GTT mapping, though. Since you can't do that, previous code needs to
>> have made sure you can't reach this point.
>
> I agree with Eric - we can't fall through to the next cases, since
> they'll fail too. Our only hope is to ensure giant objects are forced
> untiled (which I'm pretty sure we do these days).
depth_texture_mipmap.test in Khronos OpenGL CTS fails this assertion.
>
> --Ken
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
