[PATCH libdrm v2] intel: error out on has_error in exec2

[Emil Velikov]

On 31 August 2015 at 19:26, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> On Mon, Aug 31, 2015 at 07:14:12PM +0100, Emil Velikov wrote:
>> Just like we do for the original exec()
>>
>> v2: move bo_gem declaration to the top of the function.
>>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: intel-gfx at lists.freedesktop.org
>> Signed-off-by: Emil Velikov <emil.l.velikov at gmail.com>
>> ---
>>  intel/intel_bufmgr_gem.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c
>> index 7303903..5287419 100644
>> --- a/intel/intel_bufmgr_gem.c
>> +++ b/intel/intel_bufmgr_gem.c
>> @@ -2185,10 +2185,14 @@ do_exec2(drm_intel_bo *bo, int used, drm_intel_context *ctx,
>>        unsigned int flags)
>>  {
>>       drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *)bo->bufmgr;
>> +     drm_intel_bo_gem *bo_gem = (drm_intel_bo_gem *) bo;
>>       struct drm_i915_gem_execbuffer2 execbuf;
>>       int ret = 0;
>>       int i;
>>
>> +     if (bo_gem->has_error)
>> +             return -ENOMEM;
>> +
>>       switch (flags & 0x7) {
>>       default:
>>               return -EINVAL;
>> @@ -2259,8 +2263,7 @@ skip_execution:
>>               drm_intel_gem_dump_validation_list(bufmgr_gem);
>>
>>       for (i = 0; i < bufmgr_gem->exec_count; i++) {
>> -             drm_intel_bo_gem *bo_gem =
>> -                     (drm_intel_bo_gem *) bufmgr_gem->exec_bos[i];
>> +             bo_gem = (drm_intel_bo_gem *) bufmgr_gem->exec_bos[i];
>
> Reusing bo_gem here is a little worrying as it would be very easy for
> someone to add code to the end of the function thinking that bo_gem
> still was the batch.
>
Doesn't this concert apply to drm_intel_gem_bo_exec() as well ?

> If we had
>
> static inline drm_intel_bo_gem *to_bo_gem(drm_intel_bo *bo)
> {
>         return (drm_intel_bo_gem *)bo;
> }
>
> then we can start doing one offs like
>
> if (to_bo_gem(bo)->has_error) return -ENOMEM;
>
> and of course
>         for (i = 0; i < bufmgr_gem->exec_count; i++) {
>                 drm_intel_bo_gem *bo_gem = to_bo_gem(bufmgr_gem->exec_bos[i]);

How about we do this as a follow up patch (4.1/17) that covers both functions ?

Thanks again,
Emil