Op 18-04-16 om 09:57 schreef Ander Conselvan De Oliveira:
On Mon, 2016-04-18 at 07:31 +0200, Maarten Lankhorst wrote:
Op 15-04-16 om 09:07 schreef Ander Conselvan De Oliveira:
On Wed, 2016-04-13 at 11:18 +0200, Maarten Lankhorst wrote:
Re-use unpin_work->pending, but also set vblank count before intel_mark_page_flip_active to be sure.
Be sure of what?
Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
drivers/gpu/drm/i915/i915_debugfs.c | 11 ++++++----- drivers/gpu/drm/i915/intel_display.c | 31 ++++++++++++------------------- drivers/gpu/drm/i915/intel_drv.h | 1 - 3 files changed, 18 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 9640738aabf2..df8073a2ffbe 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -582,9 +582,14 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data) seq_printf(m, "No flip due on pipe %c (plane %c)\n", pipe, plane); } else {
u32 pending; u32 addr;
if (atomic_read(&work->pending) <INTEL_FLIP_COMPLETE) {
pending = atomic_read(&work->pending);if (pending == INTEL_FLIP_INACTIVE) {seq_printf(m, "Flip ioctl preparing onpipe %c (plane %c)\n",
pipe, plane);} else if (pending >= INTEL_FLIP_COMPLETE) { seq_printf(m, "Flip queued on pipe %c(plane %c)\n", pipe, plane); } else { @@ -606,10 +611,6 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data) work->flip_queued_vblank, work->flip_ready_vblank, drm_crtc_vblank_count(&crtc->base));
if (work->enable_stall_check)seq_puts(m, "Stall check enabled, ");elseseq_puts(m, "Stall check waiting for pageflip ioctl, "); seq_printf(m, "%d prepares\n", atomic_read(&work ->pending));
if (INTEL_INFO(dev)->gen >= 4)diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f2be54a48727..618e034a7a5e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11415,8 +11415,6 @@ static void intel_do_mmio_flip(struct intel_mmio_flip *mmio_flip) if (work == NULL) return;
intel_mark_page_flip_active(work);
intel_pipe_update_start(crtc);
if (INTEL_INFO(mmio_flip->i915)->gen >= 9)
@@ -11426,6 +11424,8 @@ static void intel_do_mmio_flip(struct intel_mmio_flip *mmio_flip) ilk_do_mmio_flip(crtc, work);
intel_pipe_update_end(crtc);
- intel_mark_page_flip_active(work);
Is this to avoid triggering the stall check during the wait from a vblank evasion?
It's to ensure that if a vblank happens before pipe_update_end, we don't mark the flip as completed until we actually updated the mmio registers.
But interrupts are disabled between pipe_update_start() and pipe_update_end(), so if that happens it either happens before or during pipe_update_start(), no?
Is it possible the vblank happens just after pipe_update_end() and before marking it active? Seems to me that in that case, first prepare_page_flip() will increase unpin_work->pending (so it will go from INACTIVE to PENDING) and then marking it active will set it again to PENDING, so it never gets to COMPLETE.
But even if the above can happen, that is fixed by the removal of the COMPLETE state in patch 3.
We disable local interrupts, but interrupts can still happen on another cpu.
}
static void intel_mmio_flip_work_func(struct work_struct *work) @@ -11492,15 +11492,11 @@ static bool __intel_pageflip_stall_check(struct drm_device *dev, struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct intel_unpin_work *work = intel_crtc->unpin_work; u32 addr;
- u32 pending;
- if (atomic_read(&work->pending) >= INTEL_FLIP_COMPLETE)
return true;- if (atomic_read(&work->pending) < INTEL_FLIP_PENDING)
return false;- if (!work->enable_stall_check)
return false;
pending = atomic_read(&work->pending);
if (pending != INTEL_FLIP_PENDING)
return pending == INTEL_FLIP_COMPLETE;if (work->flip_ready_vblank == 0) { if (work->flip_queued_req &&
@@ -11676,6 +11672,11 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, */ if (!mmio_flip) { ret = i915_gem_object_sync(obj, engine, &request);
if (!ret && !request) {request = i915_gem_request_alloc(engine, NULL);ret = PTR_ERR_OR_ZERO(request);}- if (ret) goto cleanup_pending; }
@@ -11687,6 +11688,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, work->gtt_offset = intel_plane_obj_offset(to_intel_plane(primary), obj, 0); work->gtt_offset += intel_crtc->dspaddr_offset;
work->flip_queued_vblank = drm_crtc_vblank_count(crtc);
if (mmio_flip) { ret = intel_queue_mmio_flip(dev, crtc, obj);
@@ -11696,14 +11698,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, i915_gem_request_assign(&work->flip_queued_req, obj->last_write_req); } else {
if (!request) {request = i915_gem_request_alloc(engine, NULL);if (IS_ERR(request)) {ret = PTR_ERR(request);goto cleanup_unpin;}}- ret = dev_priv->display.queue_flip(dev, crtc, fb, obj,
request, page_flip_flags); if (ret) @@ -11716,7 +11710,6 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc, i915_add_request_no_flush(request);
work->flip_queued_vblank = drm_crtc_vblank_count(crtc);
Do we still need the assigment above?
It's used for rps boosting, so likely...
But you added the same assignment above, so removing it would only change the timing of the boost if the count were to flip while queuing but not prevent the boost from happening. It's no big deal, I just find it odd that we need to set that value twice.
I think I'm not paranoid enough here. I should probably stop touching work as soon as I call intel_mark_page_flip_active, so moving it from .queue_flip to after i915_gem_request_assign would be best.
This would allow us to ensure the work struct is completely filled in when mark_page_flip_active is called, with no intermediate states to worry about.