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 at 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 on
>>>> pipe
>>>> %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, ");
>>>> - else
>>>> - seq_puts(m, "Stall check waiting for page
>>>> flip 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.
