Re: [PATCH 08/19] drm/i915: Convert flip_work to a list.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Op 02-05-16 om 11:22 schreef Patrik Jakobsson:
> On Tue, Apr 19, 2016 at 09:52:28AM +0200, Maarten Lankhorst wrote:
>> This will be required to allow more than 1 update in the future.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c  |  90 +++++++++++++++++-------------
>>  drivers/gpu/drm/i915/i915_drv.h      |   2 +-
>>  drivers/gpu/drm/i915/intel_display.c | 105 ++++++++++++++++++++---------------
>>  drivers/gpu/drm/i915/intel_drv.h     |   4 +-
>>  4 files changed, 114 insertions(+), 87 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index aabd7a13cec7..513c7da24c3d 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -589,6 +589,53 @@ static int i915_gem_gtt_info(struct seq_file *m, void *data)
>>  	return 0;
>>  }
>>  
>> +static void i915_dump_pageflip(struct seq_file *m,
>> +			       struct drm_i915_private *dev_priv,
>> +			       struct intel_crtc *crtc,
>> +			       struct intel_flip_work *work)
>> +{
>> +	const char pipe = pipe_name(crtc->pipe);
>> +	const char plane = plane_name(crtc->plane);
>> +	u32 pending;
>> +	u32 addr;
>> +
>> +	pending = atomic_read(&work->pending);
>> +	if (pending == INTEL_FLIP_INACTIVE) {
>> +		seq_printf(m, "Flip ioctl preparing on pipe %c (plane %c)\n",
>> +			   pipe, plane);
>> +	} else {
>> +		seq_printf(m, "Flip pending (waiting for vsync) on pipe %c (plane %c)\n",
>> +			   pipe, plane);
>> +	}
>> +	if (work->flip_queued_req) {
>> +		struct intel_engine_cs *engine = i915_gem_request_get_engine(work->flip_queued_req);
>> +
>> +		seq_printf(m, "Flip queued on %s at seqno %x, next seqno %x [current breadcrumb %x], completed? %d\n",
>> +			   engine->name,
>> +			   i915_gem_request_get_seqno(work->flip_queued_req),
>> +			   dev_priv->next_seqno,
>> +			   engine->get_seqno(engine),
>> +			   i915_gem_request_completed(work->flip_queued_req, true));
>> +	} else
>> +		seq_printf(m, "Flip not associated with any ring\n");
>> +	seq_printf(m, "Flip queued on frame %d, (was ready on frame %d), now %d\n",
>> +		   work->flip_queued_vblank,
>> +		   work->flip_ready_vblank,
>> +		   intel_crtc_get_vblank_counter(crtc));
>> +	seq_printf(m, "%d prepares\n", atomic_read(&work->pending));
>> +
>> +	if (INTEL_INFO(dev_priv)->gen >= 4)
>> +		addr = I915_HI_DISPBASE(I915_READ(DSPSURF(crtc->plane)));
>> +	else
>> +		addr = I915_READ(DSPADDR(crtc->plane));
>> +	seq_printf(m, "Current scanout address 0x%08x\n", addr);
>> +
>> +	if (work->pending_flip_obj) {
>> +		seq_printf(m, "New framebuffer address 0x%08lx\n", (long)work->gtt_offset);
>> +		seq_printf(m, "MMIO update completed? %d\n",  addr == work->gtt_offset);
>> +	}
>> +}
>> +
>>  static int i915_gem_pageflip_info(struct seq_file *m, void *data)
>>  {
>>  	struct drm_info_node *node = m->private;
>> @@ -607,48 +654,13 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data)
>>  		struct intel_flip_work *work;
>>  
>>  		spin_lock_irq(&dev->event_lock);
>> -		work = crtc->flip_work;
>> -		if (work == NULL) {
>> +		if (list_empty(&crtc->flip_work)) {
>>  			seq_printf(m, "No flip due on pipe %c (plane %c)\n",
>>  				   pipe, plane);
>>  		} else {
>> -			u32 pending;
>> -			u32 addr;
>> -
>> -			pending = atomic_read(&work->pending);
>> -			if (pending == INTEL_FLIP_INACTIVE) {
>> -				seq_printf(m, "Flip ioctl preparing on pipe %c (plane %c)\n",
>> -					   pipe, plane);
>> -			} else {
>> -				seq_printf(m, "Flip pending (waiting for vsync) on pipe %c (plane %c)\n",
>> -					   pipe, plane);
>> -			}
>> -			if (work->flip_queued_req) {
>> -				struct intel_engine_cs *engine = i915_gem_request_get_engine(work->flip_queued_req);
>> -
>> -				seq_printf(m, "Flip queued on %s at seqno %x, next seqno %x [current breadcrumb %x], completed? %d\n",
>> -					   engine->name,
>> -					   i915_gem_request_get_seqno(work->flip_queued_req),
>> -					   dev_priv->next_seqno,
>> -					   engine->get_seqno(engine),
>> -					   i915_gem_request_completed(work->flip_queued_req, true));
>> -			} else
>> -				seq_printf(m, "Flip not associated with any ring\n");
>> -			seq_printf(m, "Flip queued on frame %d, (was ready on frame %d), now %d\n",
>> -				   work->flip_queued_vblank,
>> -				   work->flip_ready_vblank,
>> -				   intel_crtc_get_vblank_counter(crtc));
>> -			seq_printf(m, "%d prepares\n", atomic_read(&work->pending));
>> -
>> -			if (INTEL_INFO(dev)->gen >= 4)
>> -				addr = I915_HI_DISPBASE(I915_READ(DSPSURF(crtc->plane)));
>> -			else
>> -				addr = I915_READ(DSPADDR(crtc->plane));
>> -			seq_printf(m, "Current scanout address 0x%08x\n", addr);
>> -
>> -			if (work->pending_flip_obj) {
>> -				seq_printf(m, "New framebuffer address 0x%08lx\n", (long)work->gtt_offset);
>> -				seq_printf(m, "MMIO update completed? %d\n",  addr == work->gtt_offset);
>> +			list_for_each_entry(work, &crtc->flip_work, head); {
> Accidental semi-colon after list_for_each_entry()
Oops indeed! Thanks for catching.
>> +				i915_dump_pageflip(m, dev_priv, crtc, work);
>> +				seq_puts(m, "\n");
>>  			}
>>  		}
>>  		spin_unlock_irq(&dev->event_lock);
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 85102ad75962..fb19fee24584 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -611,7 +611,7 @@ struct drm_i915_display_funcs {
>>  			  struct drm_framebuffer *fb,
>>  			  struct drm_i915_gem_object *obj,
>>  			  struct drm_i915_gem_request *req,
>> -			  uint32_t flags);
>> +			  uint64_t gtt_offset);
>>  	void (*hpd_irq_setup)(struct drm_device *dev);
>>  	/* clock updates for mode set */
>>  	/* cursor updates */
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index d1181089512a..a2b4be06eb84 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3300,17 +3300,12 @@ static bool intel_crtc_has_pending_flip(struct drm_crtc *crtc)
>>  	struct drm_device *dev = crtc->dev;
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>  	unsigned reset_counter;
>> -	bool pending;
>>  
>>  	reset_counter = i915_reset_counter(&to_i915(dev)->gpu_error);
>>  	if (intel_crtc->reset_counter != reset_counter)
>>  		return false;
>>  
>> -	spin_lock_irq(&dev->event_lock);
>> -	pending = to_intel_crtc(crtc)->flip_work != NULL;
>> -	spin_unlock_irq(&dev->event_lock);
>> -
>> -	return pending;
>> +	return !list_empty_careful(&to_intel_crtc(crtc)->flip_work);
>>  }
>>  
>>  static void intel_update_pipe_config(struct intel_crtc *crtc,
>> @@ -3886,7 +3881,7 @@ bool intel_has_pending_fb_unpin(struct drm_device *dev)
>>  		if (atomic_read(&crtc->unpin_work_count) == 0)
>>  			continue;
>>  
>> -		if (crtc->flip_work)
>> +		if (!list_empty_careful(&crtc->flip_work))
>>  			intel_wait_for_vblank(dev, crtc->pipe);
>>  
>>  		return true;
>> @@ -3895,12 +3890,11 @@ bool intel_has_pending_fb_unpin(struct drm_device *dev)
>>  	return false;
>>  }
>>  
>> -static void page_flip_completed(struct intel_crtc *intel_crtc)
>> +static void page_flip_completed(struct intel_crtc *intel_crtc, struct intel_flip_work *work)
>>  {
>>  	struct drm_i915_private *dev_priv = to_i915(intel_crtc->base.dev);
>> -	struct intel_flip_work *work = intel_crtc->flip_work;
>>  
>> -	intel_crtc->flip_work = NULL;
>> +	list_del_init(&work->head);
>>  
>>  	if (work->event)
>>  		drm_crtc_send_vblank_event(&intel_crtc->base, work->event);
>> @@ -3935,10 +3929,11 @@ static int intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc)
>>  		struct intel_flip_work *work;
>>  
>>  		spin_lock_irq(&dev->event_lock);
>> -		work = intel_crtc->flip_work;
>> +		work = list_first_entry_or_null(&intel_crtc->flip_work,
>> +						struct intel_flip_work, head);
> I'm not quite following this. Why is it enough for us to only look at the first
> entry. Is it hardcoded atm because we only have a list with a single entry. If
> so we should write a comment about this.
Yes, plus if a page flip gets stuck it would be the first entry in the list.
>>  		if (work && !is_mmio_work(work)) {
>>  			WARN_ONCE(1, "Removing stuck page flip\n");
>> -			page_flip_completed(intel_crtc);
>> +			page_flip_completed(intel_crtc, work);
>>  		}
>>  		spin_unlock_irq(&dev->event_lock);
>>  	}
>> @@ -6335,7 +6330,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
>>  		return;
>>  
>>  	if (to_intel_plane_state(crtc->primary->state)->visible) {
>> -		WARN_ON(intel_crtc->flip_work);
>> +		WARN_ON(list_empty(&intel_crtc->flip_work));
>>  
>>  		intel_pre_disable_primary_noatomic(crtc);
>>  
>> @@ -10924,17 +10919,24 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>  	struct drm_device *dev = crtc->dev;
>>  	struct intel_flip_work *work;
>> +	struct list_head head;
>> +
>> +	INIT_LIST_HEAD(&head);
> What's this?
Bogus leftover.
>>  
>>  	spin_lock_irq(&dev->event_lock);
>> -	work = intel_crtc->flip_work;
>> -	intel_crtc->flip_work = NULL;
>> -	spin_unlock_irq(&dev->event_lock);
>> +	while (!list_empty(&intel_crtc->flip_work)) {
>> +		work = list_first_entry(&intel_crtc->flip_work,
>> +					struct intel_flip_work, head);
> Hardcoded assumption here again?
Nope, this removes all because of the while !list_empty.
>> +		list_del_init(&work->head);
>> +		spin_unlock_irq(&dev->event_lock);
>>  
>> -	if (work) {
>>  		cancel_work_sync(&work->mmio_work);
>>  		cancel_work_sync(&work->unpin_work);
>>  		kfree(work);
>> +
>> +		spin_lock_irq(&dev->event_lock);
>>  	}
>> +	spin_unlock_irq(&dev->event_lock);
>>  
>>  	drm_crtc_cleanup(crtc);
>>  
>> @@ -10976,7 +10978,8 @@ static bool g4x_flip_count_after_eq(u32 a, u32 b)
>>  	return !((a - b) & 0x80000000);
>>  }
>>  
>> -static bool page_flip_finished(struct intel_crtc *crtc)
>> +static bool page_flip_finished(struct intel_crtc *crtc,
>> +			       struct intel_flip_work *work)
>>  {
>>  	struct drm_device *dev = crtc->base.dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>> @@ -11021,9 +11024,9 @@ static bool page_flip_finished(struct intel_crtc *crtc)
>>  	 * anyway, we don't really care.
>>  	 */
>>  	return (I915_READ(DSPSURFLIVE(crtc->plane)) & ~0xfff) ==
>> -		crtc->flip_work->gtt_offset &&
>> +		work->gtt_offset &&
>>  		g4x_flip_count_after_eq(I915_READ(PIPE_FLIPCOUNT_G4X(crtc->pipe)),
>> -				    crtc->flip_work->flip_count);
>> +					work->flip_count);
>>  }
>>  
>>  static void do_intel_finish_page_flip(struct drm_device *dev,
>> @@ -11042,12 +11045,14 @@ static void do_intel_finish_page_flip(struct drm_device *dev,
>>  	 * lost pageflips) so needs the full irqsave spinlocks.
>>  	 */
>>  	spin_lock_irqsave(&dev->event_lock, flags);
>> -	work = intel_crtc->flip_work;
>> +	work = list_first_entry_or_null(&intel_crtc->flip_work,
>> +					struct intel_flip_work,
>> +					head);
> Hardcoded...?
I'm only assuming a single flip will complete per call, so it makes sense here.

This might change in the future.
>>  
>>  	if (work != NULL &&
>>  	    atomic_read(&work->pending) == INTEL_FLIP_PENDING &&
>> -	    page_flip_finished(intel_crtc))
>> -		page_flip_completed(intel_crtc);
>> +	    page_flip_finished(intel_crtc, work))
>> +		page_flip_completed(intel_crtc, work);
>>  
>>  	spin_unlock_irqrestore(&dev->event_lock, flags);
>>  }
>> @@ -11083,7 +11088,7 @@ static int intel_gen2_queue_flip(struct drm_device *dev,
>>  				 struct drm_framebuffer *fb,
>>  				 struct drm_i915_gem_object *obj,
>>  				 struct drm_i915_gem_request *req,
>> -				 uint32_t flags)
>> +				 uint64_t gtt_offset)
>>  {
>>  	struct intel_engine_cs *engine = req->engine;
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> @@ -11106,7 +11111,7 @@ static int intel_gen2_queue_flip(struct drm_device *dev,
>>  	intel_ring_emit(engine, MI_DISPLAY_FLIP |
>>  			MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
>>  	intel_ring_emit(engine, fb->pitches[0]);
>> -	intel_ring_emit(engine, intel_crtc->flip_work->gtt_offset);
>> +	intel_ring_emit(engine, gtt_offset);
>>  	intel_ring_emit(engine, 0); /* aux display base address, unused */
>>  
>>  	return 0;
>> @@ -11117,7 +11122,7 @@ static int intel_gen3_queue_flip(struct drm_device *dev,
>>  				 struct drm_framebuffer *fb,
>>  				 struct drm_i915_gem_object *obj,
>>  				 struct drm_i915_gem_request *req,
>> -				 uint32_t flags)
>> +				 uint64_t gtt_offset)
>>  {
>>  	struct intel_engine_cs *engine = req->engine;
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> @@ -11137,7 +11142,7 @@ static int intel_gen3_queue_flip(struct drm_device *dev,
>>  	intel_ring_emit(engine, MI_DISPLAY_FLIP_I915 |
>>  			MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
>>  	intel_ring_emit(engine, fb->pitches[0]);
>> -	intel_ring_emit(engine, intel_crtc->flip_work->gtt_offset);
>> +	intel_ring_emit(engine, gtt_offset);
>>  	intel_ring_emit(engine, MI_NOOP);
>>  
>>  	return 0;
>> @@ -11148,7 +11153,7 @@ static int intel_gen4_queue_flip(struct drm_device *dev,
>>  				 struct drm_framebuffer *fb,
>>  				 struct drm_i915_gem_object *obj,
>>  				 struct drm_i915_gem_request *req,
>> -				 uint32_t flags)
>> +				 uint64_t gtt_offset)
>>  {
>>  	struct intel_engine_cs *engine = req->engine;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>> @@ -11167,8 +11172,7 @@ static int intel_gen4_queue_flip(struct drm_device *dev,
>>  	intel_ring_emit(engine, MI_DISPLAY_FLIP |
>>  			MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
>>  	intel_ring_emit(engine, fb->pitches[0]);
>> -	intel_ring_emit(engine, intel_crtc->flip_work->gtt_offset |
>> -			obj->tiling_mode);
>> +	intel_ring_emit(engine, gtt_offset | obj->tiling_mode);
>>  
>>  	/* XXX Enabling the panel-fitter across page-flip is so far
>>  	 * untested on non-native modes, so ignore it for now.
>> @@ -11186,7 +11190,7 @@ static int intel_gen6_queue_flip(struct drm_device *dev,
>>  				 struct drm_framebuffer *fb,
>>  				 struct drm_i915_gem_object *obj,
>>  				 struct drm_i915_gem_request *req,
>> -				 uint32_t flags)
>> +				 uint64_t gtt_offset)
>>  {
>>  	struct intel_engine_cs *engine = req->engine;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>> @@ -11201,7 +11205,7 @@ static int intel_gen6_queue_flip(struct drm_device *dev,
>>  	intel_ring_emit(engine, MI_DISPLAY_FLIP |
>>  			MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
>>  	intel_ring_emit(engine, fb->pitches[0] | obj->tiling_mode);
>> -	intel_ring_emit(engine, intel_crtc->flip_work->gtt_offset);
>> +	intel_ring_emit(engine, gtt_offset);
>>  
>>  	/* Contrary to the suggestions in the documentation,
>>  	 * "Enable Panel Fitter" does not seem to be required when page
>> @@ -11221,7 +11225,7 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
>>  				 struct drm_framebuffer *fb,
>>  				 struct drm_i915_gem_object *obj,
>>  				 struct drm_i915_gem_request *req,
>> -				 uint32_t flags)
>> +				 uint64_t gtt_offset)
>>  {
>>  	struct intel_engine_cs *engine = req->engine;
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> @@ -11304,7 +11308,7 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
>>  
>>  	intel_ring_emit(engine, MI_DISPLAY_FLIP_I915 | plane_bit);
>>  	intel_ring_emit(engine, (fb->pitches[0] | obj->tiling_mode));
>> -	intel_ring_emit(engine, intel_crtc->flip_work->gtt_offset);
>> +	intel_ring_emit(engine, gtt_offset);
>>  	intel_ring_emit(engine, (MI_NOOP));
>>  
>>  	return 0;
>> @@ -11371,17 +11375,17 @@ static int intel_default_queue_flip(struct drm_device *dev,
>>  				    struct drm_framebuffer *fb,
>>  				    struct drm_i915_gem_object *obj,
>>  				    struct drm_i915_gem_request *req,
>> -				    uint32_t flags)
>> +				    uint64_t gtt_offset)
>>  {
>>  	return -ENODEV;
>>  }
>>  
>>  static bool __intel_pageflip_stall_check(struct drm_device *dev,
>> -					 struct drm_crtc *crtc)
>> +					 struct drm_crtc *crtc,
>> +					 struct intel_flip_work *work)
>>  {
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> -	struct intel_flip_work *work = intel_crtc->flip_work;
>>  	u32 addr, vblank;
>>  	u32 pending;
>>  
>> @@ -11437,12 +11441,14 @@ void intel_check_page_flip(struct drm_device *dev, int pipe)
>>  		return;
>>  
>>  	spin_lock(&dev->event_lock);
>> -	work = intel_crtc->flip_work;
>> -	if (work != NULL && __intel_pageflip_stall_check(dev, crtc)) {
>> +	work = list_first_entry_or_null(&intel_crtc->flip_work,
>> +					struct intel_flip_work, head);
> ...
Yeah, should probably retry instead here if page flip is stalled.
>> +
>> +	if (work != NULL && __intel_pageflip_stall_check(dev, crtc, work)) {
>>  		WARN_ONCE(!is_mmio_work(work),
>>  			  "Kicking stuck page flip: queued at %d, now %d\n",
>>  			 work->flip_queued_vblank, intel_crtc_get_vblank_counter(intel_crtc));
>> -		page_flip_completed(intel_crtc);
>> +		page_flip_completed(intel_crtc, work);
>>  		work = NULL;
>>  	}
>>  	if (work != NULL && !is_mmio_work(work) &&
>> @@ -11508,13 +11514,18 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>>  
>>  	/* We borrow the event spin lock for protecting flip_work */
>>  	spin_lock_irq(&dev->event_lock);
>> -	if (intel_crtc->flip_work) {
>> +	if (!list_empty(&intel_crtc->flip_work)) {
>> +		struct intel_flip_work *old_work;
>> +
>> +		old_work = list_first_entry(&intel_crtc->flip_work,
>> +					    struct intel_flip_work, head);
>> +
> ...
Should probably be last entry here.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux