[Mesa-dev] [PATCH 2/7] i965: Don't call the blitter on addresses it can't handle.

[Kenneth Graunke]

On 01/05/2014 02:28 PM, Eric Anholt wrote:
> Anuj Phogat <anuj.phogat at gmail.com> writes:
> 
>> On Mon, Dec 23, 2013 at 4:08 PM, Eric Anholt <eric at 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).

--Ken