On Sun, Oct 14, 2012 at 12:14 PM, Ben Hutchings <[email protected]> wrote:
> Please don't drop the context of which commit we are talking about.
>
> For further reference, it is 7dd490658627 ('drm/i915: Mark untiled BLT
> commands as fenced on gen2/3')
>
> On Mon, 2012-10-08 at 10:49 +0100, Luís Picciochi Oliveira wrote:
>> Hi,
>>
>> Here is the actual diff to the 3.2 kernel:
>>
>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c 2012-07-12
>> 04:32:21.000000000 +0100
>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c 2012-10-02
>> 22:17:56.079302807 +0100
>> @@ -573,8 +573,8 @@ i915_gem_execbuffer_reserve(struct intel
>> ret = i915_gem_object_put_fence(obj);
>> if (ret)
>> break;
>> + obj->pending_fenced_gpu_access = true;
>> }
>> - obj->pending_fenced_gpu_access = need_fence;
>> }
>>
>> entry->offset = obj->gtt_offset;
>
> I don't think this is right. Adding more context to the corresponding
> hunk in the upstream commit, it looks like:
>
> if (has_fenced_gpu_access) {
> if (entry->flags & EXEC_OBJECT_NEEDS_FENCE) {
> if (obj->tiling_mode) {
> ret = i915_gem_object_get_fence(obj, ring);
> if (ret)
> goto err_unpin;
>
> entry->flags |= __EXEC_OBJECT_HAS_FENCE;
> i915_gem_object_pin_fence(obj);
> } else {
> ret = i915_gem_object_put_fence(obj);
> if (ret)
> goto err_unpin;
> }
> + obj->pending_fenced_gpu_access = true;
> }
> - obj->pending_fenced_gpu_access = need_fence;
> }
>
> So the condition is changed from has_fenced_gpu_access && need_fence to
> has_fenced_gpu_access && (entry->flags & EXEC_OBJECT_NEEDS_FENCE).
>
> But in 3.2 we have:
>
> if (has_fenced_gpu_access) {
> if (need_fence) {
> ret = i915_gem_object_get_fence(obj,
> ring);
> if (ret)
> break;
> } else if (entry->flags &
> EXEC_OBJECT_NEEDS_FENCE &&
> obj->tiling_mode ==
> I915_TILING_NONE) {
> /* XXX pipelined! */
> ret = i915_gem_object_put_fence(obj);
> if (ret)
> break;
> }
> obj->pending_fenced_gpu_access = need_fence;
> }
>
> Your suggested hunk will change the condition to has_fenced_gpu_access
> && !need_fence && (entry->flags & EXEC_OBJECT_NEEDS_FENCE) &&
> (obj->tiling_mode == I915_TILING_NONE).
>
> Maybe it should be:
>
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -574,7 +574,7 @@
> if (ret)
> break;
> }
> - obj->pending_fenced_gpu_access = need_fence;
> + obj->pending_fenced_gpu_access = true;
> }
>
> entry->offset = obj->gtt_offset;
> }
This should work, although for safety I think I'd be better to keep
the 2nd conditional, i.e.
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -574,7 +574,7 @@
if (ret)
break;
}
- obj->pending_fenced_gpu_access = need_fence;
+ obj->pending_fenced_gpu_access
=entry->flags & EXEC_OBJECT_NEEDS_FENCE ;
}
entry->offset = obj->gtt_offset;
}
In any case, intel-gpu-tools
(http://cgit.freedesktop.org/xorg/app/intel-gpu-tools/) has a neat
testcase to exercise this corner case. If
i-g-t/tests/gem_set_tiling_vs_blt is still happy, the fix should be
correct.
Yours, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
