On 07/28/2015 11:10 AM, John Harrison wrote:
[snip]
static inline void
@@ -2267,7 +2284,7 @@ i915_gem_request_unreference__unlocked(struct
drm_i915_gem_request *req)
return;
dev = req->ring->dev;
- if (kref_put_mutex(&req->ref, i915_gem_request_free,
&dev->struct_mutex))
+ if (kref_put_mutex(&req->fence.refcount, fence_release,
&dev->struct_mutex))
mutex_unlock(&dev->struct_mutex);
Would it be nicer to add fence_put_mutex(struct fence *, struct mutex
*) for this? It would avoid the layering violation of requests peeking
into fence implementation details.
Maarten pointed out that adding 'fence_put_mutex()' is breaking the
fence ABI itself. It requires users of any random fence to know and
worry about what mutex objects that fence's internal implementation
might require.
I don't see what it has to do with the ABI? I dislike both approaches
actually and don't like the question of what is the lesser evil. :)
This is a bit more hacky from our point of view but it is only a
temporary measure until the mutex lock is not required for
dereferencing. At that point all the nasty stuff disappears completely.
Yes saw that in later patches. No worries then, just a consequence of
going patch by patch.
+
if (req->file_priv)
i915_gem_request_remove_from_client(req);
@@ -2637,6 +2639,47 @@ void i915_gem_request_free(struct kref *req_ref)
kmem_cache_free(req->i915->requests, req);
}
+static const char *i915_gem_request_get_driver_name(struct fence
*req_fence)
+{
+ return "i915_request";
+}
I think this becomes kind of ABI once added so we need to make sure
the best name is chosen to start with. I couldn't immediately figure
out why not just "i915"?
The thought was that we might start using fences for other purposes at
some point in the future. At which point it is helpful to differentiate
them. If nothing else, a previous iteration of the native sync
implementation was using different fence objects due to worries about
allowing user land to get its grubby mitts on important driver internal
structures.
I don't follow on the connection between these names and the last
concern? If I did, would it explain why driver name "i915" and
differentiation by timeline names would be problematic? Looks much
cleaner to me.
+
+static const char *i915_gem_request_get_timeline_name(struct fence
*req_fence)
+{
+ struct drm_i915_gem_request *req = container_of(req_fence,
+ typeof(*req), fence);
+ return req->ring->name;
+}
+
+static bool i915_gem_request_enable_signaling(struct fence *req_fence)
+{
+ /* Interrupt driven fences are not implemented yet.*/
+ WARN(true, "This should not be called!");
+ return true;
I suppose WARN is not really needed in the interim patch. Would return
false work?
The point is to keep the driver 'safe' if that patch is viewed as stand
alone. I.e., if the interrupt follow up does not get merged immediately
after (or not at all) then this prevents people accidentally trying to
use an unsupported interface without first implementing it.
I assumed true means "signaling enabled successfully" but "false"
actually means "fence already passed" so you are right. I blame the
un-intuitive API. :)
+ fence_base = fence_context_alloc(I915_NUM_RINGS);
+
/* Now it is safe to go back round and do everything else: */
for_each_ring(ring, dev_priv, i) {
struct drm_i915_gem_request *req;
WARN_ON(!ring->default_context);
+ ring->fence_context = fence_base + i;
Could you store fence_base in dev_priv and then ring->init_hw could
set up the fence_context on its own?
Probably. It seemed to make more sense to me to keep the fence
allocation all in one place rather than splitting it in half and
distributing it. It gets removed again with the per context per ring
timelines anyway.
Yes saw that later, never mind then.
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
