[Mesa-dev] [PATCH] i965: Fail to blit rather than assert on invalid pitch requirements.

[Ian Romanick]

On 12/26/2012 09:29 AM, Kenneth Graunke wrote:
> On 12/26/2012 08:32 AM, Paul Berry wrote:
>> On 25 December 2012 20:55, Kenneth Graunke <kenneth at whitecape.org
>> <mailto:kenneth at whitecape.org>> wrote:
>>
>>     Dungeon Defenders hits TexImage()'s try_pbo_upload() path where
>>     image->Width == 2, which doesn't meet intelEmitCopyBlit's requirement
>>     that the pitch needs to be a multiple of 4.
>>
>>     Since intelEmitCopyBlit can already fail for a myriad of other
>> reasons,
>>     and it's not clear that other callers are immune to this failure
>> mode,
>>     simply make it return false rather than assert.
>>
>>     Fixes Dungeon Defenders on i965/Ivybridge.  Now playable (aside from
>>     having to work around the EXT_bindable_uniform issue).
>>
>>     NOTE: This is probably a candidate for the 9.0 branch.
>>     ---
>>       src/mesa/drivers/dri/intel/intel_blit.c | 4 ++--
>>       1 file changed, 2 insertions(+), 2 deletions(-)
>>
>>     diff --git a/src/mesa/drivers/dri/intel/intel_blit.c
>>     b/src/mesa/drivers/dri/intel/intel_blit.c
>>     index 521e6eb..867d7b3 100644
>>     --- a/src/mesa/drivers/dri/intel/intel_blit.c
>>     +++ b/src/mesa/drivers/dri/intel/intel_blit.c
>>     @@ -150,8 +150,8 @@ intelEmitCopyBlit(struct intel_context *intel,
>>          /* Blit pitch must be dword-aligned.  Otherwise, the hardware
>>     appears to drop
>>           * the low bits.
>>           */
>>     -   assert(src_pitch % 4 == 0);
>>     -   assert(dst_pitch % 4 == 0);
>>     +   if (src_pitch % 4 != 0 || dst_pitch % 4 != 0)
>>     +      return false;
>>
>>          /* For big formats (such as floating point), do the copy using
>>     32bpp and
>>           * multiply the coordinates.
>>     --
>>     1.8.0.2
>>
>>
>> Looks reasonable to me.
>>
>> Reviewed-by: Paul Berry <stereotype441 at gmail.com
>> <mailto:stereotype441 at gmail.com>>
>>
>> As a follow-up, we should probably check all of the code paths that lead
>> into this function to make sure they invoke the proper fallback logic if
>> there is a return value of false.  I think that _mesa_ReadnPixelsARB,
>> for example, will just fail to copy any data.
>
> They don't, which is bothersome...I'm not sure they check the
> preconditions nor the return value.

Blarg.  Can we come up with a list of cases where these paths can fail? 
  It sounds like we need some poor soul to write a pile of piglit tests...