Re: [PATCH v6 1/3] drm/i915: Replaced Blitter ring based flips with MMIO flips

["Gupta, Sourab" <sourab.gupta@intel.com>]

On Tue, 2014-05-20 at 11:59 +0000, Chris Wilson wrote:
> On Tue, May 20, 2014 at 04:19:46PM +0530, sourab.gupta@intel.com wrote:
> > +int i915_gem_check_olr(struct intel_ring_buffer *ring, u32 seqno);
> 
> Be strict and add __must_check
> 
We'll add this.

> > +static bool intel_use_mmio_flip(struct drm_device *dev)
> > +{
> > +	/* If module parameter is disabled, use CS flips.
> > +	 * Otherwise, use MMIO flips starting from Gen5.
> > +	 * This is not being used for older platforms because of
> > +	 * non-availability of flip done interrupt.
> > +	 */
> 
> What? Where is the dependence on flip-done?

Hi Chris,
>From an earlier mail by Ville, 
"It should work on gen5+ since all of those have a flip done interrupt.
For older platforms we use some clever tricks involving the flip_pending
status bits and vblank irqs. That code won't work for mmio flips. We'd
need to add another way to complete the flips based. That would involve
using the frame counter to make it accurate. To avoid races there we'd
definitely need to use the vblank evade mechanism to make sure we sample
the frame counter within the same frame as when we write the registers.
Also gen2 has the extra complication that it lacks a hardware frame
counter."
So, we had put the Gen5+ check here.

> 
> > +	if (i915.use_mmio_flip == 0)
> > +		return false;
> > +
> > +	if (INTEL_INFO(dev)->gen >= 5)
> > +		return true;
> > +	else
> > +		return false;
> 
> You have not justified the change in default settings for existing hw.
> Your argument is based on media power wells which does not support the
> general change. It would seem that we may want to mix mmio / CS flips
> depending on workload based on your vague statements.
> 

> I quite fancy a tristate here for force-CS flips, force-MMIO flips, at
> driver discretion. Then enabling it on an architecture as a seperate
> patch with justification - it is then easier to do each architecture on
> a case-by-case basis and revert if need be.
> 
We agree that the using mmio flips gives better residency in cases where
render and blitter engines reside in different power wells. This is
helpful in case of pure 3D workloads on valleyview. We have enabled it
in the second patch of series for valleyview.
This patch has put forth 2 states - 0 for force-CS flips and 1 for
force-MMIO flips. The second patch in this series enables it for
Valleyview architecture.

> > +static int intel_postpone_flip(struct drm_i915_gem_object *obj)
> > +{
> > +	int ret;
> 
> if (WARN_ON(crtc->mmio_flip_data.seqno)) return -EBUSY;
> 
> You need a tiling check here as you do not update dspcntr. Or fix
> mmio_done.
> 
We were not updating dspcntr here because atomicity concerns. We could
add tiling update also if its ok in that regard.
Ville, what's your opinion here.
> > +	if (!obj->ring)
> > +		return 0;
> > +
> > +	if (i915_seqno_passed(obj->ring->get_seqno(obj->ring, true),
> > +				obj->last_write_seqno))
> > +		return 0;
> > +
> > +	if (ret = i915_gem_check_olr(obj->ring, obj->last_write_seqno))
> > +		return ret;
> 
> Please don't anger gcc.
> 
> > +
> > +	if (WARN_ON(!obj->ring->irq_get(obj->ring)))
> > +		return 0;
> > +
> > +	return 1;
> > +}
> 
> > @@ -11377,6 +11497,9 @@ static void intel_init_display(struct drm_device *dev)
> >  		break;
> >  	}
> >  
> > +	if(intel_use_mmio_flip(dev))
> 
> Please don't anger checkpatch.
> 
> > +		dev_priv->display.queue_flip = intel_queue_mmio_flip;
> > +
> >  	intel_panel_init_backlight_funcs(dev);
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 32a74e1..08d65a4 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -351,6 +351,11 @@ struct intel_pipe_wm {
> >  	bool sprites_scaled;
> >  };
> >  
> > +struct intel_mmio_flip {
> > +	u32 seqno;
> > +	u32 ring_id;
> > +};
> > +
> >  struct intel_crtc {
> >  	struct drm_crtc base;
> >  	enum pipe pipe;
> > @@ -403,6 +408,7 @@ struct intel_crtc {
> >  	} wm;
> >  
> >  	wait_queue_head_t vbl_wait;
> > +	struct intel_mmio_flip mmio_flip_data;
> 
> Does _data add anything meaningful here to the description of mmio_flip?
> Just mmio_flip will suffice, as pending_mmio_flip is overkill but would
> make a useful comment.
> -Chris