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

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

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.

> > 
> > >  }
> > >  
> > >  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.

Ander


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