Re: [RFC 21/21] drm/i915: Remove 'obj->ring'

[John Harrison <John.C.Harrison@Intel.com>]

On 19/10/2014 15:12, Daniel Vetter wrote:
> On Mon, Oct 06, 2014 at 03:15:25PM +0100, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> For: VIZ-4377
>> Signed-off-by: John.C.Harrison@Intel.com
> I think this should be split up into the different parts:
>
> - s/obj->ring/obj->last_read_req->ring/ for all the cases that just want
>    the current ring.
> - s/obj->ring/obj->last_read_req/ I think in a bunch of places the code
>    would actually be more readable if we'd check for obj->active instead.
> - All the oddball special cases probably deserve their own commit + nice
>    explanation in the commit message about why the change is correct.

Can you explain which is which? As for why the change is correct, it is 
not a functional change. obj->ring was assigned with 
obj->last_read_seqno and cleared with obj->last_read_seqno. Thus 
querying obj->ring is equivalent to querying obj->last_read_req->ring in 
all cases. Given that the ring is now available from obj->lrr, it seemed 
redundant to also have it explicitly saved in obj->ring, hence the patch 
to remove it.

AFAICT, there are two examples in debugfs that should just be querying 
obj->active. The rest are pretty much wanting the currently in use ring 
and/or are about to use last_read_req anyway. It seems more sensible to 
say 'if(obj->lrr) { do_stuff(obj->lrr); }' than 'if(obj->active) { 
do_stuff(obj->lrr); }'. Nothing looks particularly 'oddball' to me!


> Cheers, Daniel
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c     |    9 +++++----
>>   drivers/gpu/drm/i915/i915_drv.h         |    2 --
>>   drivers/gpu/drm/i915/i915_gem.c         |   32 +++++++++++++++++++------------
>>   drivers/gpu/drm/i915/i915_gem_context.c |   12 +++++++++++-
>>   drivers/gpu/drm/i915/i915_gpu_error.c   |    3 ++-
>>   drivers/gpu/drm/i915/intel_display.c    |   15 ++++++++-------
>>   6 files changed, 46 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index df53515..b1d989f 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -168,8 +168,9 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>>   		*t = '\0';
>>   		seq_printf(m, " (%s mappable)", s);
>>   	}
>> -	if (obj->ring != NULL)
>> -		seq_printf(m, " (%s)", obj->ring->name);
>> +	if (obj->last_read_req != NULL)
>> +		seq_printf(m, " (%s)",
>> +			   i915_gem_request_get_ring(obj->last_read_req)->name);
>>   	if (obj->frontbuffer_bits)
>>   		seq_printf(m, " (frontbuffer: 0x%03x)", obj->frontbuffer_bits);
>>   }
>> @@ -336,7 +337,7 @@ static int per_file_stats(int id, void *ptr, void *data)
>>   			if (ppgtt->file_priv != stats->file_priv)
>>   				continue;
>>   
>> -			if (obj->ring) /* XXX per-vma statistic */
>> +			if (obj->last_read_req) /* XXX per-vma statistic */
>>   				stats->active += obj->base.size;
>>   			else
>>   				stats->inactive += obj->base.size;
>> @@ -346,7 +347,7 @@ static int per_file_stats(int id, void *ptr, void *data)
>>   	} else {
>>   		if (i915_gem_obj_ggtt_bound(obj)) {
>>   			stats->global += obj->base.size;
>> -			if (obj->ring)
>> +			if (obj->last_read_req)
>>   				stats->active += obj->base.size;
>>   			else
>>   				stats->inactive += obj->base.size;
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 0790593..cdbbdeb 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1859,8 +1859,6 @@ struct drm_i915_gem_object {
>>   	void *dma_buf_vmapping;
>>   	int vmapping_count;
>>   
>> -	struct intel_engine_cs *ring;
>> -
>>   	/** Breadcrumb of last rendering to the buffer. */
>>   	struct drm_i915_gem_request *last_read_req;
>>   	struct drm_i915_gem_request *last_write_req;
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index d40dad7..561fb81 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2184,14 +2184,18 @@ static void
>>   i915_gem_object_move_to_active(struct drm_i915_gem_object *obj,
>>   			       struct intel_engine_cs *ring)
>>   {
>> -	struct drm_i915_gem_request *req = intel_ring_get_request(ring);
>> +	struct drm_i915_gem_request *req;
>> +	struct intel_engine_cs *old_ring;
>>   
>>   	BUG_ON(ring == NULL);
>> -	if (obj->ring != ring && obj->last_write_req) {
>> +
>> +	req = intel_ring_get_request(ring);
>> +	old_ring = i915_gem_request_get_ring(obj->last_read_req);
>> +
>> +	if (old_ring != ring && obj->last_write_req) {
>>   		/* Keep the request relative to the current ring */
>>   		obj->last_write_req = req;
>>   	}
>> -	obj->ring = ring;
>>   
>>   	/* Add a reference if we're newly entering the active list. */
>>   	if (!obj->active) {
>> @@ -2230,7 +2234,6 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
>>   	intel_fb_obj_flush(obj, true);
>>   
>>   	list_del_init(&obj->ring_list);
>> -	obj->ring = NULL;
>>   
>>   	obj->last_read_req = NULL;
>>   	obj->last_write_req = NULL;
>> @@ -2247,9 +2250,7 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
>>   static void
>>   i915_gem_object_retire(struct drm_i915_gem_object *obj)
>>   {
>> -	struct intel_engine_cs *ring = obj->ring;
>> -
>> -	if (ring == NULL)
>> +	if (obj->last_read_req == NULL)
>>   		return;
>>   
>>   	if (i915_gem_request_completed(obj->last_read_req, true))
>> @@ -2769,14 +2770,17 @@ i915_gem_idle_work_handler(struct work_struct *work)
>>   static int
>>   i915_gem_object_flush_active(struct drm_i915_gem_object *obj)
>>   {
>> +	struct intel_engine_cs *ring;
>>   	int ret;
>>   
>>   	if (obj->active) {
>> +		ring = i915_gem_request_get_ring(obj->last_read_req);
>> +
>>   		ret = i915_gem_check_olr(obj->last_read_req);
>>   		if (ret)
>>   			return ret;
>>   
>> -		i915_gem_retire_requests_ring(obj->ring);
>> +		i915_gem_retire_requests_ring(ring);
>>   	}
>>   
>>   	return 0;
>> @@ -2876,9 +2880,11 @@ int
>>   i915_gem_object_sync(struct drm_i915_gem_object *obj,
>>   		     struct intel_engine_cs *to)
>>   {
>> -	struct intel_engine_cs *from = obj->ring;
>> +	struct intel_engine_cs *from;
>>   	int ret, idx;
>>   
>> +	from = i915_gem_request_get_ring(obj->last_read_req);
>> +
>>   	if (from == NULL || to == from)
>>   		return 0;
>>   
>> @@ -3889,7 +3895,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>>   	bool was_pin_display;
>>   	int ret;
>>   
>> -	if (pipelined != obj->ring) {
>> +	if (pipelined != i915_gem_request_get_ring(obj->last_read_req)) {
>>   		ret = i915_gem_object_sync(obj, pipelined);
>>   		if (ret)
>>   			return ret;
>> @@ -4303,9 +4309,11 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
>>   	ret = i915_gem_object_flush_active(obj);
>>   
>>   	args->busy = obj->active;
>> -	if (obj->ring) {
>> +	if (obj->last_read_req) {
>> +		struct intel_engine_cs *ring;
>>   		BUILD_BUG_ON(I915_NUM_RINGS > 16);
>> -		args->busy |= intel_ring_flag(obj->ring) << 16;
>> +		ring = i915_gem_request_get_ring(obj->last_read_req);
>> +		args->busy |= intel_ring_flag(ring) << 16;
>>   	}
>>   
>>   	drm_gem_object_unreference(&obj->base);
>> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
>> index a5221d8..8f24831 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_context.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
>> @@ -613,7 +613,17 @@ static int do_switch(struct intel_engine_cs *ring,
>>   		 * swapped, but there is no way to do that yet.
>>   		 */
>>   		from->legacy_hw_ctx.rcs_state->dirty = 1;
>> -		BUG_ON(from->legacy_hw_ctx.rcs_state->ring != ring);
>> +
>> +		/* BUG_ON(i915_gem_request_get_ring(
>> +			from->legacy_hw_ctx.rcs_state->last_read_req) != ring); */
>> +		/* NB: last_read_req has already been updated to the current
>> +		 * request however, that request has not yet been submitted.
>> +		 * Thus last_read_req->ring is guaranteed to be null!
>> +		 * NB2: Doing the check before the update of last_read_req
>> +		 * (which happens in i915_vma_move_to_active() just above),
>> +		 * also fails because last_read_req is almost always null on
>> +		 * entry!
>> +		 */
>>   
>>   		/* obj is kept alive until the next request by its active ref */
>>   		i915_gem_object_ggtt_unpin(from->legacy_hw_ctx.rcs_state);
>> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
>> index 9545d96..b9ecbd9 100644
>> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
>> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
>> @@ -681,7 +681,8 @@ 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->ring ? obj->ring->id : -1;
>> +	err->ring = obj->last_read_req ?
>> +			i915_gem_request_get_ring(obj->last_read_req)->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 9ca8f94..8238aac 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -9728,7 +9728,7 @@ static bool use_mmio_flip(struct intel_engine_cs *ring,
>>   	else if (i915.enable_execlists)
>>   		return true;
>>   	else
>> -		return ring != obj->ring;
>> +		return ring != i915_gem_request_get_ring(obj->last_read_req);
>>   }
>>   
>>   static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
>> @@ -9769,8 +9769,6 @@ static int intel_postpone_flip(struct drm_i915_gem_object *obj)
>>   	if (!obj->last_write_req)
>>   		return 0;
>>   
>> -	ring = obj->ring;
>> -
>>   	if (i915_gem_request_completed(obj->last_write_req, true))
>>   		return 0;
>>   
>> @@ -9778,6 +9776,7 @@ static int intel_postpone_flip(struct drm_i915_gem_object *obj)
>>   	if (ret)
>>   		return ret;
>>   
>> +	ring = i915_gem_request_get_ring(obj->last_write_req);
>>   	if (WARN_ON(!ring->irq_get(ring)))
>>   		return 0;
>>   
>> @@ -9837,14 +9836,15 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
>>   	spin_lock_irqsave(&dev_priv->mmio_flip_lock, irq_flags);
>>   	intel_crtc->mmio_flip.req = obj->last_write_req;
>>   	i915_gem_request_reference(intel_crtc->mmio_flip.req);
>> -	intel_crtc->mmio_flip.ring_id = obj->ring->id;
>> +	BUG_ON(ring != i915_gem_request_get_ring(intel_crtc->mmio_flip.req));
>> +	intel_crtc->mmio_flip.ring_id = ring->id;
>>   	spin_unlock_irqrestore(&dev_priv->mmio_flip_lock, irq_flags);
>>   
>>   	/*
>>   	 * Double check to catch cases where irq fired before
>>   	 * mmio flip data was ready
>>   	 */
>> -	intel_notify_mmio_flip(obj->ring);
>> +	intel_notify_mmio_flip(ring);
>>   	return 0;
>>   }
>>   
>> @@ -10022,7 +10022,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>>   	} else if (IS_IVYBRIDGE(dev)) {
>>   		ring = &dev_priv->ring[BCS];
>>   	} else if (INTEL_INFO(dev)->gen >= 7) {
>> -		ring = obj->ring;
>> +		ring = i915_gem_request_get_ring(obj->last_read_req);
>>   		if (ring == NULL || ring->id != RCS)
>>   			ring = &dev_priv->ring[BCS];
>>   	} else {
>> @@ -10043,7 +10043,8 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>>   			goto cleanup_unpin;
>>   
>>   		work->flip_queued_req = obj->last_write_req;
>> -		work->flip_queued_ring = obj->ring;
>> +		work->flip_queued_ring =
>> +				i915_gem_request_get_ring(obj->last_write_req);
>>   	} else {
>>   		ret = dev_priv->display.queue_flip(dev, crtc, fb, obj, ring,
>>   						   page_flip_flags);
>> -- 
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx