Re: [PATCH v2 02/11] drm/i915: Remove stallcheck special handling.

From: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Ander Conselvan De Oliveira <conselvan2@gmail.com>,
	intel-gfx@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2 02/11] drm/i915: Remove stallcheck special handling.
Date: Mon, 18 Apr 2016 10:23:36 +0200	[thread overview]
Message-ID: <57149988.7040205@linux.intel.com> (raw)
In-Reply-To: <1460966275.3739.18.camel@gmail.com>

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 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.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel