[073/190] drm/i915: Introduce i915_gem_active for request tracking
diff mbox

Message ID 1452503961-14837-73-git-send-email-chris@chris-wilson.co.uk
State New
Headers show

Commit Message

Chris Wilson Jan. 11, 2016, 9:17 a.m. UTC
In the next patch, request tracking is made more generic and for that we
need a new expanded struct and to separate out the logic changes from
the mechanical churn, we split out the structure renaming into this
patch.

v2: Writer's block. Add some spiel about why we track requests.
v3: Now i915_gem_active.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_debugfs.c        | 10 +++---
 drivers/gpu/drm/i915/i915_drv.h            |  9 +++--
 drivers/gpu/drm/i915/i915_gem.c            | 56 +++++++++++++++---------------
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  4 +--
 drivers/gpu/drm/i915/i915_gem_fence.c      |  6 ++--
 drivers/gpu/drm/i915/i915_gem_request.h    | 38 ++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_tiling.c     |  2 +-
 drivers/gpu/drm/i915/i915_gpu_error.c      |  6 ++--
 drivers/gpu/drm/i915/intel_display.c       | 10 +++---
 9 files changed, 89 insertions(+), 52 deletions(-)

Comments

Tvrtko Ursulin Jan. 11, 2016, 5:32 p.m. UTC | #1
On 11/01/16 09:17, Chris Wilson wrote:
> In the next patch, request tracking is made more generic and for that we
> need a new expanded struct and to separate out the logic changes from
> the mechanical churn, we split out the structure renaming into this
> patch.
>
> v2: Writer's block. Add some spiel about why we track requests.
> v3: Now i915_gem_active.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c        | 10 +++---
>   drivers/gpu/drm/i915/i915_drv.h            |  9 +++--
>   drivers/gpu/drm/i915/i915_gem.c            | 56 +++++++++++++++---------------
>   drivers/gpu/drm/i915/i915_gem_execbuffer.c |  4 +--
>   drivers/gpu/drm/i915/i915_gem_fence.c      |  6 ++--
>   drivers/gpu/drm/i915/i915_gem_request.h    | 38 ++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_gem_tiling.c     |  2 +-
>   drivers/gpu/drm/i915/i915_gpu_error.c      |  6 ++--
>   drivers/gpu/drm/i915/intel_display.c       | 10 +++---
>   9 files changed, 89 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 8de944ed3369..65cb1d6a5d64 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -146,10 +146,10 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>   		   obj->base.write_domain);
>   	for_each_ring(ring, dev_priv, i)
>   		seq_printf(m, "%x ",
> -				i915_gem_request_get_seqno(obj->last_read_req[i]));
> +				i915_gem_request_get_seqno(obj->last_read[i].request));
>   	seq_printf(m, "] %x %x%s%s%s",
> -		   i915_gem_request_get_seqno(obj->last_write_req),
> -		   i915_gem_request_get_seqno(obj->last_fenced_req),
> +		   i915_gem_request_get_seqno(obj->last_write.request),
> +		   i915_gem_request_get_seqno(obj->last_fence.request),
>   		   i915_cache_level_str(to_i915(obj->base.dev), obj->cache_level),
>   		   obj->dirty ? " dirty" : "",
>   		   obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
> @@ -184,8 +184,8 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>   		*t = '\0';
>   		seq_printf(m, " (%s mappable)", s);
>   	}
> -	if (obj->last_write_req != NULL)
> -		seq_printf(m, " (%s)", obj->last_write_req->engine->name);
> +	if (obj->last_write.request != NULL)
> +		seq_printf(m, " (%s)", obj->last_write.request->engine->name);
>   	if (obj->frontbuffer_bits)
>   		seq_printf(m, " (frontbuffer: 0x%03x)", obj->frontbuffer_bits);
>   }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index cae448e238ca..c577f86d94f8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2110,11 +2110,10 @@ struct drm_i915_gem_object {
>   	 * requests on one ring where the write request is older than the
>   	 * read request. This allows for the CPU to read from an active
>   	 * buffer by only waiting for the write to complete.
> -	 * */
> -	struct drm_i915_gem_request *last_read_req[I915_NUM_RINGS];
> -	struct drm_i915_gem_request *last_write_req;
> -	/** Breadcrumb of last fenced GPU access to the buffer. */
> -	struct drm_i915_gem_request *last_fenced_req;
> +	 */
> +	struct i915_gem_active last_read[I915_NUM_RINGS];
> +	struct i915_gem_active last_write;
> +	struct i915_gem_active last_fence;
>
>   	/** Current tiling stride for the object, if it's tiled. */
>   	uint32_t stride;
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index b0230e7151ce..77c253ddf060 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1117,23 +1117,23 @@ i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj,
>   		return 0;
>
>   	if (readonly) {
> -		if (obj->last_write_req != NULL) {
> -			ret = i915_wait_request(obj->last_write_req);
> +		if (obj->last_write.request != NULL) {
> +			ret = i915_wait_request(obj->last_write.request);
>   			if (ret)
>   				return ret;
>
> -			i = obj->last_write_req->engine->id;
> -			if (obj->last_read_req[i] == obj->last_write_req)
> +			i = obj->last_write.request->engine->id;
> +			if (obj->last_read[i].request == obj->last_write.request)
>   				i915_gem_object_retire__read(obj, i);
>   			else
>   				i915_gem_object_retire__write(obj);
>   		}
>   	} else {
>   		for (i = 0; i < I915_NUM_RINGS; i++) {
> -			if (obj->last_read_req[i] == NULL)
> +			if (obj->last_read[i].request == NULL)
>   				continue;
>
> -			ret = i915_wait_request(obj->last_read_req[i]);
> +			ret = i915_wait_request(obj->last_read[i].request);
>   			if (ret)
>   				return ret;
>
> @@ -1151,9 +1151,9 @@ i915_gem_object_retire_request(struct drm_i915_gem_object *obj,
>   {
>   	int ring = req->engine->id;
>
> -	if (obj->last_read_req[ring] == req)
> +	if (obj->last_read[ring].request == req)
>   		i915_gem_object_retire__read(obj, ring);
> -	else if (obj->last_write_req == req)
> +	else if (obj->last_write.request == req)
>   		i915_gem_object_retire__write(obj);
>
>   	i915_gem_request_retire_upto(req);
> @@ -1181,7 +1181,7 @@ i915_gem_object_wait_rendering__nonblocking(struct drm_i915_gem_object *obj,
>   	if (readonly) {
>   		struct drm_i915_gem_request *req;
>
> -		req = obj->last_write_req;
> +		req = obj->last_write.request;
>   		if (req == NULL)
>   			return 0;
>
> @@ -1190,7 +1190,7 @@ i915_gem_object_wait_rendering__nonblocking(struct drm_i915_gem_object *obj,
>   		for (i = 0; i < I915_NUM_RINGS; i++) {
>   			struct drm_i915_gem_request *req;
>
> -			req = obj->last_read_req[i];
> +			req = obj->last_read[i].request;
>   			if (req == NULL)
>   				continue;
>
> @@ -2070,7 +2070,7 @@ void i915_vma_move_to_active(struct i915_vma *vma,
>   	obj->active |= intel_engine_flag(engine);
>
>   	list_move_tail(&obj->ring_list[engine->id], &engine->active_list);
> -	i915_gem_request_assign(&obj->last_read_req[engine->id], req);
> +	i915_gem_request_mark_active(req, &obj->last_read[engine->id]);
>
>   	list_move_tail(&vma->mm_list, &vma->vm->active_list);
>   }
> @@ -2078,10 +2078,10 @@ void i915_vma_move_to_active(struct i915_vma *vma,
>   static void
>   i915_gem_object_retire__write(struct drm_i915_gem_object *obj)
>   {
> -	GEM_BUG_ON(obj->last_write_req == NULL);
> -	GEM_BUG_ON(!(obj->active & intel_engine_flag(obj->last_write_req->engine)));
> +	GEM_BUG_ON(obj->last_write.request == NULL);
> +	GEM_BUG_ON(!(obj->active & intel_engine_flag(obj->last_write.request->engine)));
>
> -	i915_gem_request_assign(&obj->last_write_req, NULL);
> +	i915_gem_request_assign(&obj->last_write.request, NULL);
>   	intel_fb_obj_flush(obj, true, ORIGIN_CS);
>   }
>
> @@ -2090,13 +2090,13 @@ i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring)
>   {
>   	struct i915_vma *vma;
>
> -	GEM_BUG_ON(obj->last_read_req[ring] == NULL);
> +	GEM_BUG_ON(obj->last_read[ring].request == NULL);
>   	GEM_BUG_ON(!(obj->active & (1 << ring)));
>
>   	list_del_init(&obj->ring_list[ring]);
> -	i915_gem_request_assign(&obj->last_read_req[ring], NULL);
> +	i915_gem_request_assign(&obj->last_read[ring].request, NULL);
>
> -	if (obj->last_write_req && obj->last_write_req->engine->id == ring)
> +	if (obj->last_write.request && obj->last_write.request->engine->id == ring)
>   		i915_gem_object_retire__write(obj);
>
>   	obj->active &= ~(1 << ring);
> @@ -2115,7 +2115,7 @@ i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring)
>   			list_move_tail(&vma->mm_list, &vma->vm->inactive_list);
>   	}
>
> -	i915_gem_request_assign(&obj->last_fenced_req, NULL);
> +	i915_gem_request_assign(&obj->last_fence.request, NULL);
>   	drm_gem_object_unreference(&obj->base);
>   }
>
> @@ -2336,7 +2336,7 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
>   				      struct drm_i915_gem_object,
>   				      ring_list[ring->id]);
>
> -		if (!list_empty(&obj->last_read_req[ring->id]->list))
> +		if (!list_empty(&obj->last_read[ring->id].request->list))
>   			break;
>
>   		i915_gem_object_retire__read(obj, ring->id);
> @@ -2445,7 +2445,7 @@ i915_gem_object_flush_active(struct drm_i915_gem_object *obj)
>   	for (i = 0; i < I915_NUM_RINGS; i++) {
>   		struct drm_i915_gem_request *req;
>
> -		req = obj->last_read_req[i];
> +		req = obj->last_read[i].request;
>   		if (req == NULL)
>   			continue;
>
> @@ -2525,10 +2525,10 @@ i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>   	drm_gem_object_unreference(&obj->base);
>
>   	for (i = 0; i < I915_NUM_RINGS; i++) {
> -		if (obj->last_read_req[i] == NULL)
> +		if (obj->last_read[i].request == NULL)
>   			continue;
>
> -		req[n++] = i915_gem_request_get(obj->last_read_req[i]);
> +		req[n++] = i915_gem_request_get(obj->last_read[i].request);
>   	}
>
>   	mutex_unlock(&dev->struct_mutex);
> @@ -2619,12 +2619,12 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
>
>   	n = 0;
>   	if (readonly) {
> -		if (obj->last_write_req)
> -			req[n++] = obj->last_write_req;
> +		if (obj->last_write.request)
> +			req[n++] = obj->last_write.request;
>   	} else {
>   		for (i = 0; i < I915_NUM_RINGS; i++)
> -			if (obj->last_read_req[i])
> -				req[n++] = obj->last_read_req[i];
> +			if (obj->last_read[i].request)
> +				req[n++] = obj->last_read[i].request;
>   	}
>   	for (i = 0; i < n; i++) {
>   		ret = __i915_gem_object_sync(obj, to, req[i]);
> @@ -3695,8 +3695,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
>
>   	BUILD_BUG_ON(I915_NUM_RINGS > 16);
>   	args->busy = obj->active << 16;
> -	if (obj->last_write_req)
> -		args->busy |= obj->last_write_req->engine->id;
> +	if (obj->last_write.request)
> +		args->busy |= obj->last_write.request->engine->id;
>
>   unref:
>   	drm_gem_object_unreference(&obj->base);
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 6dee27224ddb..56d6b5dbb121 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1125,7 +1125,7 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
>
>   		i915_vma_move_to_active(vma, req);
>   		if (obj->base.write_domain) {
> -			i915_gem_request_assign(&obj->last_write_req, req);
> +			i915_gem_request_mark_active(req, &obj->last_write);
>
>   			intel_fb_obj_invalidate(obj, ORIGIN_CS);
>
> @@ -1133,7 +1133,7 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
>   			obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS;
>   		}
>   		if (entry->flags & EXEC_OBJECT_NEEDS_FENCE) {
> -			i915_gem_request_assign(&obj->last_fenced_req, req);
> +			i915_gem_request_mark_active(req, &obj->last_fence);
>   			if (entry->flags & __EXEC_OBJECT_HAS_FENCE) {
>   				struct drm_i915_private *dev_priv = req->i915;
>   				list_move_tail(&dev_priv->fence_regs[obj->fence_reg].lru_list,
> diff --git a/drivers/gpu/drm/i915/i915_gem_fence.c b/drivers/gpu/drm/i915/i915_gem_fence.c
> index 598198543dcd..ab29c237ffa9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_fence.c
> +++ b/drivers/gpu/drm/i915/i915_gem_fence.c
> @@ -261,12 +261,12 @@ static inline void i915_gem_object_fence_lost(struct drm_i915_gem_object *obj)
>   static int
>   i915_gem_object_wait_fence(struct drm_i915_gem_object *obj)
>   {
> -	if (obj->last_fenced_req) {
> -		int ret = i915_wait_request(obj->last_fenced_req);
> +	if (obj->last_fence.request) {
> +		int ret = i915_wait_request(obj->last_fence.request);
>   		if (ret)
>   			return ret;
>
> -		i915_gem_request_assign(&obj->last_fenced_req, NULL);
> +		i915_gem_request_assign(&obj->last_fence.request, NULL);
>   	}
>
>   	return 0;
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h
> index 2da9e0b5dfc7..0a21986c332b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.h
> +++ b/drivers/gpu/drm/i915/i915_gem_request.h
> @@ -208,4 +208,42 @@ static inline bool i915_gem_request_completed(struct drm_i915_gem_request *req)
>   				 req->fence.seqno);
>   }
>
> +/* We treat requests as fences. This is not be to confused with our
> + * "fence registers" but pipeline synchronisation objects ala GL_ARB_sync.
> + * We use the fences to synchronize access from the CPU with activity on the
> + * GPU, for example, we should not rewrite an object's PTE whilst the GPU
> + * is reading them. We also track fences at a higher level to provide
> + * implicit synchronisation around GEM objects, e.g. set-domain will wait
> + * for outstanding GPU rendering before marking the object ready for CPU
> + * access, or a pageflip will wait until the GPU is complete before showing
> + * the frame on the scanout.
> + *
> + * In order to use a fence, the object must track the fence it needs to
> + * serialise with. For example, GEM objects want to track both read and
> + * write access so that we can perform concurrent read operations between
> + * the CPU and GPU engines, as well as waiting for all rendering to
> + * complete, or waiting for the last GPU user of a "fence register". The
> + * object then embeds a @i915_gem_active to track the most recent (in
> + * retirment order) request relevant for the desired mode of access.
> + * The @i915_gem_active is updated with i915_gem_request_mark_active() to
> + * track the most recent fence request, typically this is done as part of
> + * i915_vma_move_to_active().
> + *
> + * When the @i915_gem_active completes (is retired), it will
> + * signal its completion to the owner through a callback as well as mark
> + * itself as idle (i915_gem_active.request == NULL). The owner
> + * can then perform any action, such as delayed freeing of an active
> + * resource including itself.
> + */
> +struct i915_gem_active {
> +	struct drm_i915_gem_request *request;
> +};
> +
> +static inline void
> +i915_gem_request_mark_active(struct drm_i915_gem_request *request,
> +			     struct i915_gem_active *active)
> +{
> +	i915_gem_request_assign(&active->request, request);
> +}

This function name bothers me since I think the name is misleading and 
unintuitive. It is not marking a request as active but associating it 
with the second data structure.

Maybe i915_gem_request_move_to_active to keep the mental association 
with the well established vma_move_to_active ?

Maybe struct i915_gem_active could also be better called 
i915_gem_active_list ?

It is not ideal because of the future little reversal of who is in who's 
list, so maybe there is something even better. But I think an intuitive 
name is really important for code clarity and maintainability.

Regards,

Tvrtko
Chris Wilson Jan. 11, 2016, 10:49 p.m. UTC | #2
On Mon, Jan 11, 2016 at 05:32:27PM +0000, Tvrtko Ursulin wrote:
> >+struct i915_gem_active {
> >+	struct drm_i915_gem_request *request;
> >+};
> >+
> >+static inline void
> >+i915_gem_request_mark_active(struct drm_i915_gem_request *request,
> >+			     struct i915_gem_active *active)
> >+{
> >+	i915_gem_request_assign(&active->request, request);
> >+}
> 
> This function name bothers me since I think the name is misleading
> and unintuitive. It is not marking a request as active but
> associating it with the second data structure.
> 
> Maybe i915_gem_request_move_to_active to keep the mental association
> with the well established vma_move_to_active ?

That's backwards imo, since it is the i915_gem_active that gets added to
the request. (Or at least will be.)
 
> Maybe struct i915_gem_active could also be better called
> i915_gem_active_list ?

It's not a list but a node. I started with drm_i915_gem_request_node,
but that's too unwieldy and I felt even more confusing.

> It is not ideal because of the future little reversal of who is in
> who's list, so maybe there is something even better. But I think an
> intuitive name is really important for code clarity and
> maintainability.

In userspace, I have the request (which is actually a userspace fence
itself) containing a list of fences (that are identical to i915_gem_active,
they track which request contains the reference and a callback for
signalling) and those fences have a direct correspondence to,
ARB_sync_objects, for example. But we already have plenty of conflict
regarding the term fence, so that's no go.

i915_gem_active, for me, made the association with the active-reference
tracking that is ingrained into the objects and beyond. I quite like the
connection with GPU activity

i915_gem_retire_notifier? Hmm, I still like how
i915_gem_active.request != NULL is quite self-descriptive.
-Chris
Tvrtko Ursulin Jan. 12, 2016, 10:04 a.m. UTC | #3
On 11/01/16 22:49, Chris Wilson wrote:
> On Mon, Jan 11, 2016 at 05:32:27PM +0000, Tvrtko Ursulin wrote:
>>> +struct i915_gem_active {
>>> +	struct drm_i915_gem_request *request;
>>> +};
>>> +
>>> +static inline void
>>> +i915_gem_request_mark_active(struct drm_i915_gem_request *request,
>>> +			     struct i915_gem_active *active)
>>> +{
>>> +	i915_gem_request_assign(&active->request, request);
>>> +}
>>
>> This function name bothers me since I think the name is misleading
>> and unintuitive. It is not marking a request as active but
>> associating it with the second data structure.
>>
>> Maybe i915_gem_request_move_to_active to keep the mental association
>> with the well established vma_move_to_active ?
>
> That's backwards imo, since it is the i915_gem_active that gets added to
> the request. (Or at least will be.)
>
>> Maybe struct i915_gem_active could also be better called
>> i915_gem_active_list ?
>
> It's not a list but a node. I started with drm_i915_gem_request_node,
> but that's too unwieldy and I felt even more confusing.
>
>> It is not ideal because of the future little reversal of who is in
>> who's list, so maybe there is something even better. But I think an
>> intuitive name is really important for code clarity and
>> maintainability.
>
> In userspace, I have the request (which is actually a userspace fence
> itself) containing a list of fences (that are identical to i915_gem_active,
> they track which request contains the reference and a callback for
> signalling) and those fences have a direct correspondence to,
> ARB_sync_objects, for example. But we already have plenty of conflict
> regarding the term fence, so that's no go.
>
> i915_gem_active, for me, made the association with the active-reference
> tracking that is ingrained into the objects and beyond. I quite like the
> connection with GPU activity
>
> i915_gem_retire_notifier? Hmm, I still like how
> i915_gem_active.request != NULL is quite self-descriptive.

Yes the last bit is neat.

Perhaps then leave the structure name as is and just rename the function 
to i915_gem_request_assign_active? I think that describes better what is 
actually happening.

Regards,

Tvrtko
Chris Wilson Jan. 12, 2016, 11:01 a.m. UTC | #4
On Tue, Jan 12, 2016 at 10:04:20AM +0000, Tvrtko Ursulin wrote:
> Perhaps then leave the structure name as is and just rename the
> function to i915_gem_request_assign_active? I think that describes
> better what is actually happening.

i915_gem_request_update_active()?

request_assign_active() says to me that it is the request we are acting
on and it can have only one active entity. "update" could go either way.

i915_gem_active_add_to_request() is the full version I guess, or just
i915_gem_active_set().

i915_gem_request_mark_active() -> i915_gem_active_set()
-Chris
Tvrtko Ursulin Jan. 12, 2016, 1:42 p.m. UTC | #5
On 12/01/16 11:01, Chris Wilson wrote:
> On Tue, Jan 12, 2016 at 10:04:20AM +0000, Tvrtko Ursulin wrote:
>> Perhaps then leave the structure name as is and just rename the
>> function to i915_gem_request_assign_active? I think that describes
>> better what is actually happening.
>
> i915_gem_request_update_active()?
>
> request_assign_active() says to me that it is the request we are acting
> on and it can have only one active entity. "update" could go either way.
>
> i915_gem_active_add_to_request() is the full version I guess, or just
> i915_gem_active_set().

i915_gem_active_add_to_request sounds good, but need to reorder the 
parameters then I think.

Regards,

Tvrtko
Tvrtko Ursulin Jan. 12, 2016, 1:44 p.m. UTC | #6
On 12/01/16 11:01, Chris Wilson wrote:
> On Tue, Jan 12, 2016 at 10:04:20AM +0000, Tvrtko Ursulin wrote:
>> Perhaps then leave the structure name as is and just rename the
>> function to i915_gem_request_assign_active? I think that describes
>> better what is actually happening.
>
> i915_gem_request_update_active()?
>
> request_assign_active() says to me that it is the request we are acting
> on and it can have only one active entity. "update" could go either way.
>
> i915_gem_active_add_to_request() is the full version I guess, or just
> i915_gem_active_set().
>
> i915_gem_request_mark_active() -> i915_gem_active_set()

Sorry, or the short version might be good enough and better in the code 
since shorter. Just I think parameters need to be re-ordered.

Regards,

Tvrtko
Chris Wilson Jan. 12, 2016, 2:08 p.m. UTC | #7
On Tue, Jan 12, 2016 at 01:44:13PM +0000, Tvrtko Ursulin wrote:
> 
> On 12/01/16 11:01, Chris Wilson wrote:
> >On Tue, Jan 12, 2016 at 10:04:20AM +0000, Tvrtko Ursulin wrote:
> >>Perhaps then leave the structure name as is and just rename the
> >>function to i915_gem_request_assign_active? I think that describes
> >>better what is actually happening.
> >
> >i915_gem_request_update_active()?
> >
> >request_assign_active() says to me that it is the request we are acting
> >on and it can have only one active entity. "update" could go either way.
> >
> >i915_gem_active_add_to_request() is the full version I guess, or just
> >i915_gem_active_set().
> >
> >i915_gem_request_mark_active() -> i915_gem_active_set()
> 
> Sorry, or the short version might be good enough and better in the
> code since shorter. Just I think parameters need to be re-ordered.

Yes, i915_gem_active_set() would operate on the i915_gem_active and take
i915_gem_request as its parameter.
-Chris

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 8de944ed3369..65cb1d6a5d64 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -146,10 +146,10 @@  describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
 		   obj->base.write_domain);
 	for_each_ring(ring, dev_priv, i)
 		seq_printf(m, "%x ",
-				i915_gem_request_get_seqno(obj->last_read_req[i]));
+				i915_gem_request_get_seqno(obj->last_read[i].request));
 	seq_printf(m, "] %x %x%s%s%s",
-		   i915_gem_request_get_seqno(obj->last_write_req),
-		   i915_gem_request_get_seqno(obj->last_fenced_req),
+		   i915_gem_request_get_seqno(obj->last_write.request),
+		   i915_gem_request_get_seqno(obj->last_fence.request),
 		   i915_cache_level_str(to_i915(obj->base.dev), obj->cache_level),
 		   obj->dirty ? " dirty" : "",
 		   obj->madv == I915_MADV_DONTNEED ? " purgeable" : "");
@@ -184,8 +184,8 @@  describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
 		*t = '\0';
 		seq_printf(m, " (%s mappable)", s);
 	}
-	if (obj->last_write_req != NULL)
-		seq_printf(m, " (%s)", obj->last_write_req->engine->name);
+	if (obj->last_write.request != NULL)
+		seq_printf(m, " (%s)", obj->last_write.request->engine->name);
 	if (obj->frontbuffer_bits)
 		seq_printf(m, " (frontbuffer: 0x%03x)", obj->frontbuffer_bits);
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index cae448e238ca..c577f86d94f8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2110,11 +2110,10 @@  struct drm_i915_gem_object {
 	 * requests on one ring where the write request is older than the
 	 * read request. This allows for the CPU to read from an active
 	 * buffer by only waiting for the write to complete.
-	 * */
-	struct drm_i915_gem_request *last_read_req[I915_NUM_RINGS];
-	struct drm_i915_gem_request *last_write_req;
-	/** Breadcrumb of last fenced GPU access to the buffer. */
-	struct drm_i915_gem_request *last_fenced_req;
+	 */
+	struct i915_gem_active last_read[I915_NUM_RINGS];
+	struct i915_gem_active last_write;
+	struct i915_gem_active last_fence;
 
 	/** Current tiling stride for the object, if it's tiled. */
 	uint32_t stride;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index b0230e7151ce..77c253ddf060 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1117,23 +1117,23 @@  i915_gem_object_wait_rendering(struct drm_i915_gem_object *obj,
 		return 0;
 
 	if (readonly) {
-		if (obj->last_write_req != NULL) {
-			ret = i915_wait_request(obj->last_write_req);
+		if (obj->last_write.request != NULL) {
+			ret = i915_wait_request(obj->last_write.request);
 			if (ret)
 				return ret;
 
-			i = obj->last_write_req->engine->id;
-			if (obj->last_read_req[i] == obj->last_write_req)
+			i = obj->last_write.request->engine->id;
+			if (obj->last_read[i].request == obj->last_write.request)
 				i915_gem_object_retire__read(obj, i);
 			else
 				i915_gem_object_retire__write(obj);
 		}
 	} else {
 		for (i = 0; i < I915_NUM_RINGS; i++) {
-			if (obj->last_read_req[i] == NULL)
+			if (obj->last_read[i].request == NULL)
 				continue;
 
-			ret = i915_wait_request(obj->last_read_req[i]);
+			ret = i915_wait_request(obj->last_read[i].request);
 			if (ret)
 				return ret;
 
@@ -1151,9 +1151,9 @@  i915_gem_object_retire_request(struct drm_i915_gem_object *obj,
 {
 	int ring = req->engine->id;
 
-	if (obj->last_read_req[ring] == req)
+	if (obj->last_read[ring].request == req)
 		i915_gem_object_retire__read(obj, ring);
-	else if (obj->last_write_req == req)
+	else if (obj->last_write.request == req)
 		i915_gem_object_retire__write(obj);
 
 	i915_gem_request_retire_upto(req);
@@ -1181,7 +1181,7 @@  i915_gem_object_wait_rendering__nonblocking(struct drm_i915_gem_object *obj,
 	if (readonly) {
 		struct drm_i915_gem_request *req;
 
-		req = obj->last_write_req;
+		req = obj->last_write.request;
 		if (req == NULL)
 			return 0;
 
@@ -1190,7 +1190,7 @@  i915_gem_object_wait_rendering__nonblocking(struct drm_i915_gem_object *obj,
 		for (i = 0; i < I915_NUM_RINGS; i++) {
 			struct drm_i915_gem_request *req;
 
-			req = obj->last_read_req[i];
+			req = obj->last_read[i].request;
 			if (req == NULL)
 				continue;
 
@@ -2070,7 +2070,7 @@  void i915_vma_move_to_active(struct i915_vma *vma,
 	obj->active |= intel_engine_flag(engine);
 
 	list_move_tail(&obj->ring_list[engine->id], &engine->active_list);
-	i915_gem_request_assign(&obj->last_read_req[engine->id], req);
+	i915_gem_request_mark_active(req, &obj->last_read[engine->id]);
 
 	list_move_tail(&vma->mm_list, &vma->vm->active_list);
 }
@@ -2078,10 +2078,10 @@  void i915_vma_move_to_active(struct i915_vma *vma,
 static void
 i915_gem_object_retire__write(struct drm_i915_gem_object *obj)
 {
-	GEM_BUG_ON(obj->last_write_req == NULL);
-	GEM_BUG_ON(!(obj->active & intel_engine_flag(obj->last_write_req->engine)));
+	GEM_BUG_ON(obj->last_write.request == NULL);
+	GEM_BUG_ON(!(obj->active & intel_engine_flag(obj->last_write.request->engine)));
 
-	i915_gem_request_assign(&obj->last_write_req, NULL);
+	i915_gem_request_assign(&obj->last_write.request, NULL);
 	intel_fb_obj_flush(obj, true, ORIGIN_CS);
 }
 
@@ -2090,13 +2090,13 @@  i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring)
 {
 	struct i915_vma *vma;
 
-	GEM_BUG_ON(obj->last_read_req[ring] == NULL);
+	GEM_BUG_ON(obj->last_read[ring].request == NULL);
 	GEM_BUG_ON(!(obj->active & (1 << ring)));
 
 	list_del_init(&obj->ring_list[ring]);
-	i915_gem_request_assign(&obj->last_read_req[ring], NULL);
+	i915_gem_request_assign(&obj->last_read[ring].request, NULL);
 
-	if (obj->last_write_req && obj->last_write_req->engine->id == ring)
+	if (obj->last_write.request && obj->last_write.request->engine->id == ring)
 		i915_gem_object_retire__write(obj);
 
 	obj->active &= ~(1 << ring);
@@ -2115,7 +2115,7 @@  i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring)
 			list_move_tail(&vma->mm_list, &vma->vm->inactive_list);
 	}
 
-	i915_gem_request_assign(&obj->last_fenced_req, NULL);
+	i915_gem_request_assign(&obj->last_fence.request, NULL);
 	drm_gem_object_unreference(&obj->base);
 }
 
@@ -2336,7 +2336,7 @@  i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
 				      struct drm_i915_gem_object,
 				      ring_list[ring->id]);
 
-		if (!list_empty(&obj->last_read_req[ring->id]->list))
+		if (!list_empty(&obj->last_read[ring->id].request->list))
 			break;
 
 		i915_gem_object_retire__read(obj, ring->id);
@@ -2445,7 +2445,7 @@  i915_gem_object_flush_active(struct drm_i915_gem_object *obj)
 	for (i = 0; i < I915_NUM_RINGS; i++) {
 		struct drm_i915_gem_request *req;
 
-		req = obj->last_read_req[i];
+		req = obj->last_read[i].request;
 		if (req == NULL)
 			continue;
 
@@ -2525,10 +2525,10 @@  i915_gem_wait_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
 	drm_gem_object_unreference(&obj->base);
 
 	for (i = 0; i < I915_NUM_RINGS; i++) {
-		if (obj->last_read_req[i] == NULL)
+		if (obj->last_read[i].request == NULL)
 			continue;
 
-		req[n++] = i915_gem_request_get(obj->last_read_req[i]);
+		req[n++] = i915_gem_request_get(obj->last_read[i].request);
 	}
 
 	mutex_unlock(&dev->struct_mutex);
@@ -2619,12 +2619,12 @@  i915_gem_object_sync(struct drm_i915_gem_object *obj,
 
 	n = 0;
 	if (readonly) {
-		if (obj->last_write_req)
-			req[n++] = obj->last_write_req;
+		if (obj->last_write.request)
+			req[n++] = obj->last_write.request;
 	} else {
 		for (i = 0; i < I915_NUM_RINGS; i++)
-			if (obj->last_read_req[i])
-				req[n++] = obj->last_read_req[i];
+			if (obj->last_read[i].request)
+				req[n++] = obj->last_read[i].request;
 	}
 	for (i = 0; i < n; i++) {
 		ret = __i915_gem_object_sync(obj, to, req[i]);
@@ -3695,8 +3695,8 @@  i915_gem_busy_ioctl(struct drm_device *dev, void *data,
 
 	BUILD_BUG_ON(I915_NUM_RINGS > 16);
 	args->busy = obj->active << 16;
-	if (obj->last_write_req)
-		args->busy |= obj->last_write_req->engine->id;
+	if (obj->last_write.request)
+		args->busy |= obj->last_write.request->engine->id;
 
 unref:
 	drm_gem_object_unreference(&obj->base);
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 6dee27224ddb..56d6b5dbb121 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1125,7 +1125,7 @@  i915_gem_execbuffer_move_to_active(struct list_head *vmas,
 
 		i915_vma_move_to_active(vma, req);
 		if (obj->base.write_domain) {
-			i915_gem_request_assign(&obj->last_write_req, req);
+			i915_gem_request_mark_active(req, &obj->last_write);
 
 			intel_fb_obj_invalidate(obj, ORIGIN_CS);
 
@@ -1133,7 +1133,7 @@  i915_gem_execbuffer_move_to_active(struct list_head *vmas,
 			obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS;
 		}
 		if (entry->flags & EXEC_OBJECT_NEEDS_FENCE) {
-			i915_gem_request_assign(&obj->last_fenced_req, req);
+			i915_gem_request_mark_active(req, &obj->last_fence);
 			if (entry->flags & __EXEC_OBJECT_HAS_FENCE) {
 				struct drm_i915_private *dev_priv = req->i915;
 				list_move_tail(&dev_priv->fence_regs[obj->fence_reg].lru_list,
diff --git a/drivers/gpu/drm/i915/i915_gem_fence.c b/drivers/gpu/drm/i915/i915_gem_fence.c
index 598198543dcd..ab29c237ffa9 100644
--- a/drivers/gpu/drm/i915/i915_gem_fence.c
+++ b/drivers/gpu/drm/i915/i915_gem_fence.c
@@ -261,12 +261,12 @@  static inline void i915_gem_object_fence_lost(struct drm_i915_gem_object *obj)
 static int
 i915_gem_object_wait_fence(struct drm_i915_gem_object *obj)
 {
-	if (obj->last_fenced_req) {
-		int ret = i915_wait_request(obj->last_fenced_req);
+	if (obj->last_fence.request) {
+		int ret = i915_wait_request(obj->last_fence.request);
 		if (ret)
 			return ret;
 
-		i915_gem_request_assign(&obj->last_fenced_req, NULL);
+		i915_gem_request_assign(&obj->last_fence.request, NULL);
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/i915_gem_request.h b/drivers/gpu/drm/i915/i915_gem_request.h
index 2da9e0b5dfc7..0a21986c332b 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.h
+++ b/drivers/gpu/drm/i915/i915_gem_request.h
@@ -208,4 +208,42 @@  static inline bool i915_gem_request_completed(struct drm_i915_gem_request *req)
 				 req->fence.seqno);
 }
 
+/* We treat requests as fences. This is not be to confused with our
+ * "fence registers" but pipeline synchronisation objects ala GL_ARB_sync.
+ * We use the fences to synchronize access from the CPU with activity on the
+ * GPU, for example, we should not rewrite an object's PTE whilst the GPU
+ * is reading them. We also track fences at a higher level to provide
+ * implicit synchronisation around GEM objects, e.g. set-domain will wait
+ * for outstanding GPU rendering before marking the object ready for CPU
+ * access, or a pageflip will wait until the GPU is complete before showing
+ * the frame on the scanout.
+ *
+ * In order to use a fence, the object must track the fence it needs to
+ * serialise with. For example, GEM objects want to track both read and
+ * write access so that we can perform concurrent read operations between
+ * the CPU and GPU engines, as well as waiting for all rendering to
+ * complete, or waiting for the last GPU user of a "fence register". The
+ * object then embeds a @i915_gem_active to track the most recent (in
+ * retirment order) request relevant for the desired mode of access.
+ * The @i915_gem_active is updated with i915_gem_request_mark_active() to
+ * track the most recent fence request, typically this is done as part of
+ * i915_vma_move_to_active().
+ *
+ * When the @i915_gem_active completes (is retired), it will
+ * signal its completion to the owner through a callback as well as mark
+ * itself as idle (i915_gem_active.request == NULL). The owner
+ * can then perform any action, such as delayed freeing of an active
+ * resource including itself.
+ */
+struct i915_gem_active {
+	struct drm_i915_gem_request *request;
+};
+
+static inline void
+i915_gem_request_mark_active(struct drm_i915_gem_request *request,
+			     struct i915_gem_active *active)
+{
+	i915_gem_request_assign(&active->request, request);
+}
+
 #endif /* I915_GEM_REQUEST_H */
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 7410f6c962e7..c7588135a82d 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -242,7 +242,7 @@  i915_gem_set_tiling(struct drm_device *dev, void *data,
 			}
 
 			obj->fence_dirty =
-				obj->last_fenced_req ||
+				obj->last_fence.request ||
 				obj->fence_reg != I915_FENCE_REG_NONE;
 
 			obj->tiling_mode = args->tiling_mode;
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 2785f2d1f073..5027636e3624 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -708,8 +708,8 @@  static void capture_bo(struct drm_i915_error_buffer *err,
 	err->size = obj->base.size;
 	err->name = obj->base.name;
 	for (i = 0; i < I915_NUM_RINGS; i++)
-		err->rseqno[i] = i915_gem_request_get_seqno(obj->last_read_req[i]);
-	err->wseqno = i915_gem_request_get_seqno(obj->last_write_req);
+		err->rseqno[i] = i915_gem_request_get_seqno(obj->last_read[i].request);
+	err->wseqno = i915_gem_request_get_seqno(obj->last_write.request);
 	err->gtt_offset = vma->node.start;
 	err->read_domains = obj->base.read_domains;
 	err->write_domain = obj->base.write_domain;
@@ -721,7 +721,7 @@  static void capture_bo(struct drm_i915_error_buffer *err,
 	err->dirty = obj->dirty;
 	err->purgeable = obj->madv != I915_MADV_WILLNEED;
 	err->userptr = obj->userptr.mm != NULL;
-	err->ring = obj->last_write_req ?  obj->last_write_req->engine->id : -1;
+	err->ring = obj->last_write.request ? obj->last_write.request->engine->id : -1;
 	err->cache_level = obj->cache_level;
 }
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ec52fff7e0b0..eef858d5376f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11310,7 +11310,7 @@  static bool use_mmio_flip(struct intel_engine_cs *ring,
 						       false))
 		return true;
 	else
-		return ring != i915_gem_request_get_engine(obj->last_write_req);
+		return ring != i915_gem_request_get_engine(obj->last_write.request);
 }
 
 static void skl_do_mmio_flip(struct intel_crtc *intel_crtc,
@@ -11455,7 +11455,7 @@  static int intel_queue_mmio_flip(struct drm_device *dev,
 		return -ENOMEM;
 
 	mmio_flip->i915 = to_i915(dev);
-	mmio_flip->req = i915_gem_request_get(obj->last_write_req);
+	mmio_flip->req = i915_gem_request_get(obj->last_write.request);
 	mmio_flip->crtc = to_intel_crtc(crtc);
 	mmio_flip->rotation = crtc->primary->state->rotation;
 
@@ -11654,7 +11654,7 @@  static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	} else if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev)) {
 		ring = &dev_priv->ring[BCS];
 	} else if (INTEL_INFO(dev)->gen >= 7) {
-		ring = i915_gem_request_get_engine(obj->last_write_req);
+		ring = i915_gem_request_get_engine(obj->last_write.request);
 		if (ring == NULL || ring->id != RCS)
 			ring = &dev_priv->ring[BCS];
 	} else {
@@ -11695,7 +11695,7 @@  static int intel_crtc_page_flip(struct drm_crtc *crtc,
 			goto cleanup_unpin;
 
 		i915_gem_request_assign(&work->flip_queued_req,
-					obj->last_write_req);
+					obj->last_write.request);
 	} else {
 		ret = dev_priv->display.queue_flip(dev, crtc, fb, obj, request,
 						   page_flip_flags);
@@ -13895,7 +13895,7 @@  intel_prepare_plane_fb(struct drm_plane *plane,
 				to_intel_plane_state(new_state);
 
 			i915_gem_request_assign(&plane_state->wait_req,
-						obj->last_write_req);
+						obj->last_write.request);
 		}
 
 		i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit);