[4/5] drm/i915: Add async page flip support for IVB

Message ID	1374544202-15496-5-git-send-email-keithp@keithp.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org> From: Keith Packard <keithp@keithp.com> To: linux-kernel@vger.kernel.org, Dave Airlie <airlied@redhat.com> Subject: [PATCH 4/5] drm/i915: Add async page flip support for IVB Date: Mon, 22 Jul 2013 18:50:01 -0700 Message-Id: <1374544202-15496-5-git-send-email-keithp@keithp.com> In-Reply-To: <1374544202-15496-1-git-send-email-keithp@keithp.com> References: <1374544202-15496-1-git-send-email-keithp@keithp.com> Cc: dri-devel@lists.freedesktop.org Precedence: list MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org Errors-To: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org

Keith Packard July 23, 2013, 1:50 a.m. UTC

This adds the necesary register defines for async page flipping
through the command ring, and then hooks those up for Ivybridge (gen7)
page flipping.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |  6 ++++++
 drivers/gpu/drm/i915/intel_display.c | 40 ++++++++++++++++++++++++++++++++++--
 2 files changed, 44 insertions(+), 2 deletions(-)

Daniel Vetter July 23, 2013, 5:28 a.m. UTC | #1

On Mon, Jul 22, 2013 at 06:50:01PM -0700, Keith Packard wrote:
> This adds the necesary register defines for async page flipping
> through the command ring, and then hooks those up for Ivybridge (gen7)
> page flipping.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |  6 ++++++
>  drivers/gpu/drm/i915/intel_display.c | 40 ++++++++++++++++++++++++++++++++++--
>  2 files changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index dc3d6a7..029cfb0 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -209,6 +209,7 @@
>  #define MI_LOAD_SCAN_LINES_INCL MI_INSTR(0x12, 0)
>  #define MI_DISPLAY_FLIP		MI_INSTR(0x14, 2)
>  #define MI_DISPLAY_FLIP_I915	MI_INSTR(0x14, 1)
> +#define   MI_DISPLAY_FLIP_ASYNC_INDICATOR	(1 << 22)
>  #define   MI_DISPLAY_FLIP_PLANE(n) ((n) << 20)
>  /* IVB has funny definitions for which plane to flip. */
>  #define   MI_DISPLAY_FLIP_IVB_PLANE_A  (0 << 19)
> @@ -217,6 +218,11 @@
>  #define   MI_DISPLAY_FLIP_IVB_SPRITE_B (3 << 19)
>  #define   MI_DISPLAY_FLIP_IVB_PLANE_C  (4 << 19)
>  #define   MI_DISPLAY_FLIP_IVB_SPRITE_C (5 << 19)
> +/* These go in the bottom of the base address value */
> +#define   MI_DISPLAY_FLIP_TYPE_SYNC    (0 << 0)
> +#define   MI_DISPLAY_FLIP_TYPE_ASYNC   (1 << 0)
> +#define   MI_DISPLAY_FLIP_TYPE_STEREO  (2 << 0)
> +#define   MI_DISPLAY_FLIP_TYPE_SYNCHRONOUS	(0 << 0)
>  #define MI_ARB_ON_OFF		MI_INSTR(0x08, 0)
>  #define   MI_ARB_ENABLE			(1<<0)
>  #define   MI_ARB_DISABLE		(0<<0)
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index bdb8854..1bcc6b4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7514,6 +7514,8 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  	struct intel_ring_buffer *ring = &dev_priv->ring[BCS];
>  	uint32_t plane_bit = 0;
> +	uint32_t cmd;
> +	uint32_t base;
>  	int ret;
>  
>  	ret = intel_pin_and_fence_fb_obj(dev, obj, ring);
> @@ -7536,13 +7538,43 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
>  		goto err_unpin;
>  	}
>  
> +	cmd = MI_DISPLAY_FLIP_I915 | plane_bit;
> +	base = i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset;
> +
> +	if (flags & DRM_MODE_PAGE_FLIP_ASYNC) {
> +
> +		/* XXX check limitations for async flip here */
> +
> +		if (fb->pitches[0] != I915_READ(DSPSTRIDE(intel_crtc->plane))) {
> +			WARN_ONCE(1, "mismatching stride in async plane flip (%d != %d)\n",
> +				  fb->pitches[0], I915_READ(DSPSTRIDE(intel_crtc->plane)));
> +			ret = -EINVAL;
> +			goto err_unpin;
> +		}
> +
> +		if (obj->tiling_mode != I915_TILING_X) {
> +			WARN_ONCE(1, "async plane flip requires X tiling\n");
> +			ret = -EINVAL;
> +			goto err_unpin;
> +		}

Matching tiling modes is actually already a requirement on gen4+ (since
the tiling bit and the linear/tiled offset registers can't be changed with
a MI_DISPLAY_FLIP command). But atm we fail to check that in the common
code, so imo better to move that to there.

We already check for matching strides in common code, so with the tile
check added we could drop this all here.
-Daniel

> +
> +		if ((I915_READ(DSPCNTR(intel_crtc->plane)) & DISPPLANE_TILED) == 0) {
> +			WARN_ONCE(1, "display not currently tiled in async plane flip\n");
> +			ret = -EINVAL;
> +			goto err_unpin;
> +		}
> +		
> +		cmd |= MI_DISPLAY_FLIP_ASYNC_INDICATOR;
> +		base |= MI_DISPLAY_FLIP_TYPE_ASYNC;
> +	}
> +
>  	ret = intel_ring_begin(ring, 4);
>  	if (ret)
>  		goto err_unpin;
>  
> -	intel_ring_emit(ring, MI_DISPLAY_FLIP_I915 | plane_bit);
> +	intel_ring_emit(ring, cmd);
>  	intel_ring_emit(ring, (fb->pitches[0] | obj->tiling_mode));
> -	intel_ring_emit(ring, i915_gem_obj_ggtt_offset(obj) + intel_crtc->dspaddr_offset);
> +	intel_ring_emit(ring, base);
>  	intel_ring_emit(ring, (MI_NOOP));
>  
>  	intel_mark_page_flip_active(intel_crtc);
> @@ -9705,6 +9737,10 @@ void intel_modeset_init(struct drm_device *dev)
>  		dev->mode_config.max_width = 8192;
>  		dev->mode_config.max_height = 8192;
>  	}
> +
> +	if (IS_GEN7(dev))
> +		dev->mode_config.async_page_flip = true;
> +
>  	dev->mode_config.fb_base = dev_priv->gtt.mappable_base;
>  
>  	DRM_DEBUG_KMS("%d display pipe%s available.\n",
> -- 
> 1.8.3.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Keith Packard July 24, 2013, 8:26 p.m. UTC | #2

Daniel Vetter <daniel@ffwll.ch> writes:

> Matching tiling modes is actually already a requirement on gen4+ (since
> the tiling bit and the linear/tiled offset registers can't be changed with
> a MI_DISPLAY_FLIP command).

Async flip has a harder requirement -- you must use X tiling, both
before and after the flip. Oh, and async flips require a 32KB aligned
buffer, which I'm not actually checking for. Not sure how to

> But atm we fail to check that in the common
> code, so imo better to move that to there.
>
> We already check for matching strides in common code, so with the tile
> check added we could drop this all here.

I don't see a requirement for matching stride and tile parameter in the
MI_DISPLAY_FLIP docs for synchronous operations, at least on DevGT+. Is
the common code too restrictive?

Daniel Vetter July 24, 2013, 9:23 p.m. UTC | #3

On Wed, Jul 24, 2013 at 01:26:32PM -0700, Keith Packard wrote:
> Daniel Vetter <daniel@ffwll.ch> writes:
> 
> > Matching tiling modes is actually already a requirement on gen4+ (since
> > the tiling bit and the linear/tiled offset registers can't be changed with
> > a MI_DISPLAY_FLIP command).
> 
> Async flip has a harder requirement -- you must use X tiling, both
> before and after the flip. Oh, and async flips require a 32KB aligned
> buffer, which I'm not actually checking for. Not sure how to

We could just unconditionally increase the alignement in
intel_pin_and_fence_fb_obj - we already have more strict requirements due
to a bunch of w/a in other places. So shouldn't hurt at all really.

> > But atm we fail to check that in the common
> > code, so imo better to move that to there.
> >
> > We already check for matching strides in common code, so with the tile
> > check added we could drop this all here.
> 
> I don't see a requirement for matching stride and tile parameter in the
> MI_DISPLAY_FLIP docs for synchronous operations, at least on DevGT+. Is
> the common code too restrictive?

Hm right, I've mixed that. I guess we could be a bit more sloppy with
this, otoh no one yet seems to want it and Ville's pageflip stuff is
rather madly flexible.
-Daniel

Keith Packard July 25, 2013, 1:40 a.m. UTC | #4

Daniel Vetter <daniel@ffwll.ch> writes:

> We could just unconditionally increase the alignement in
> intel_pin_and_fence_fb_obj - we already have more strict requirements due
> to a bunch of w/a in other places. So shouldn't hurt at all really.

That seems like a fine plan; 32kB isn't that onerous. Do you want the
trivial patch to do this from me then?

Daniel Vetter July 25, 2013, 7:47 a.m. UTC | #5

On Wed, Jul 24, 2013 at 06:40:16PM -0700, Keith Packard wrote:
> Daniel Vetter <daniel@ffwll.ch> writes:
> 
> > We could just unconditionally increase the alignement in
> > intel_pin_and_fence_fb_obj - we already have more strict requirements due
> > to a bunch of w/a in other places. So shouldn't hurt at all really.
> 
> That seems like a fine plan; 32kB isn't that onerous. Do you want the
> trivial patch to do this from me then?

Yes please, merging patches from other people is much easier than begging for
review for my own ;-)
-Daniel

[4/5] drm/i915: Add async page flip support for IVB

Commit Message

Comments

Patch