diff mbox

drm/i915: Intel-specific primary plane handling (v4)

Message ID 1397250822-17280-1-git-send-email-matthew.d.roper@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Matt Roper April 11, 2014, 9:13 p.m. UTC
Intel hardware allows the primary plane to be disabled independently of
the CRTC.  Provide custom primary plane handling to allow this.

v4:
 - Don't add a primary_plane field to intel_crtc; that was left over
   from a much earlier iteration of this patch series, but is no longer
   needed/used now that the DRM core primary plane support has been
   merged.
v3:
 - Provide gen-specific primary plane format lists (suggested by Daniel
   Vetter).
 - If the primary plane is already enabled, go ahead and just call the
   primary plane helper to do the update (suggested by Daniel Vetter).
 - Don't try to disable the primary plane on destruction; the DRM layer
   should have already taken care of this for us.
v2:
 - Unpin fb properly on primary plane disable
 - Provide an Intel-specific set of primary plane formats
 - Additional sanity checks on setplane (in line with the checks
   currently being done by the DRM core primary plane helper)

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 197 ++++++++++++++++++++++++++++++++++-
 1 file changed, 195 insertions(+), 2 deletions(-)

Comments

Ville Syrjälä April 22, 2014, 12:47 p.m. UTC | #1
On Fri, Apr 11, 2014 at 02:13:42PM -0700, Matt Roper wrote:
> Intel hardware allows the primary plane to be disabled independently of
> the CRTC.  Provide custom primary plane handling to allow this.
> 
> v4:
>  - Don't add a primary_plane field to intel_crtc; that was left over
>    from a much earlier iteration of this patch series, but is no longer
>    needed/used now that the DRM core primary plane support has been
>    merged.
> v3:
>  - Provide gen-specific primary plane format lists (suggested by Daniel
>    Vetter).
>  - If the primary plane is already enabled, go ahead and just call the
>    primary plane helper to do the update (suggested by Daniel Vetter).
>  - Don't try to disable the primary plane on destruction; the DRM layer
>    should have already taken care of this for us.
> v2:
>  - Unpin fb properly on primary plane disable
>  - Provide an Intel-specific set of primary plane formats
>  - Additional sanity checks on setplane (in line with the checks
>    currently being done by the DRM core primary plane helper)
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 197 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 195 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1390ab5..3e4d36a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -39,8 +39,35 @@
>  #include "i915_trace.h"
>  #include <drm/drm_dp_helper.h>
>  #include <drm/drm_crtc_helper.h>
> +#include <drm/drm_plane_helper.h>
> +#include <drm/drm_rect.h>
>  #include <linux/dma_remapping.h>
>  
> +/* Primary plane formats supported by all gen */
> +#define COMMON_PRIMARY_FORMATS \
> +	DRM_FORMAT_C8, \
> +	DRM_FORMAT_RGB565, \
> +	DRM_FORMAT_XRGB8888, \
> +	DRM_FORMAT_ARGB8888
> +
> +/* Primary plane formats for gen <= 3 */
> +const static uint32_t intel_primary_formats_gen3[] = {q

'static const' is the more typical order

> +	COMMON_PRIMARY_FORMATS,
> +	DRM_FORMAT_XRGB1555,
> +	DRM_FORMAT_ARGB1555,
> +};
> +
> +/* Primary plane formats for gen >= 4 */
> +const static uint32_t intel_primary_formats_gen4[] = {

ditto

> +	COMMON_PRIMARY_FORMATS, \
> +	DRM_FORMAT_XBGR8888,
> +	DRM_FORMAT_ABGR8888,
> +	DRM_FORMAT_XRGB2101010,
> +	DRM_FORMAT_ARGB2101010,
> +	DRM_FORMAT_XBGR2101010,
> +	DRM_FORMAT_ABGR2101010,
> +};
> +
>  static void intel_increase_pllclock(struct drm_crtc *crtc);
>  static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
>  
> @@ -10556,17 +10583,183 @@ static void intel_shared_dpll_init(struct drm_device *dev)
>  	BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS);
>  }
>  
> +static int
> +intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
> +			     struct drm_framebuffer *fb, int crtc_x, int crtc_y,
> +			     unsigned int crtc_w, unsigned int crtc_h,
> +			     uint32_t src_x, uint32_t src_y,
> +			     uint32_t src_w, uint32_t src_h)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct drm_framebuffer *tmpfb;
> +	struct drm_rect dest = {
> +		.x1 = crtc_x,
> +		.y1 = crtc_y,
> +		.x2 = crtc_x + crtc_w,
> +		.y2 = crtc_y + crtc_h,
> +	};
> +	struct drm_rect clip = {
> +		.x2 = crtc->mode.hdisplay,
> +		.y2 = crtc->mode.vdisplay,
> +	};

'clip' can be const, and these should be pipe_src_{w,h} just like
we have in the sprite code.

> +	int ret;
> +
> +	/*
> +	 * At the moment we use the same set of setplane restrictions as the
> +	 * DRM primary plane helper, so go ahead and just call the helper if
> +	 * the primary plane is already enabled.  We only need to take special
> +	 * action if the primary plane is disabled (something i915 can do but
> +	 * the generic helper can't).
> +	 */
> +	if (intel_crtc->primary_enabled)
> +		return drm_primary_helper_update(plane, crtc, fb,
> +						 crtc_x, crtc_y,
> +						 crtc_w, crtc_h,
> +						 src_x, src_y,
> +						 src_w, src_h);

Why would we want to call that if we have a custom implementation
anyway?

> +
> +	/* setplane API takes shifted source rectangle values; unshift them */
> +	src_x >>= 16;
> +	src_y >>= 16;
> +	src_w >>= 16;
> +	src_h >>= 16;
> +
> +	/*
> +	 * Current hardware can't reposition the primary plane or scale it
> +	 * (although this could change in the future).
> +	 */
> +	drm_rect_intersect(&dest, &clip);

src needs to be clipped too. I guess we should get a sufficiently good
result if we just call 'drm_rect_clip_scaled(..., 1, 1)' here. Ie.
clip after throwing away the sub-pixel parts from the src coordinates.

To match the sprite behaviour a bit better we should also allow cases
where the primary plane becomes fully clipped. In such cases we should
just disable the plane.

> +	if (dest.x1 != 0 || dest.y1 != 0 ||
> +	    dest.x2 != crtc->mode.hdisplay || dest.y2 != crtc->mode.vdisplay) {

!drm_rect_equals(&dest, &clip)

> +		DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n");
> +		return -EINVAL;
> +	}
> +
> +	if (crtc_w != src_w || crtc_h != src_h) {
> +		DRM_DEBUG_KMS("Can't scale primary plane\n");
> +		return -EINVAL;
> +	}

This check could be moved to happen before we clip.

> +
> +	/* Primary planes are locked to their owning CRTC */
> +	if (plane->possible_crtcs != drm_crtc_mask(crtc)) {
> +		DRM_DEBUG_KMS("Cannot change primary plane CRTC\n");
> +		return -EINVAL;
> +	}

Looks like possible_crtcs check could be in drm_mode_setplane(). No need
to special case primary planes here.

> +
> +	/* Framebuffer must be big enough to cover entire plane */
> +	ret = drm_crtc_check_viewport(crtc, crtc_x, crtc_y, &crtc->mode, fb);
> +	if (ret)
> +		return ret;

drm_mode_setplane() should already have checked that the viewport
doesn't exceed the fb.

> +
> +	/*
> +	 * pipe_set_base() adjusts crtc->primary->fb; however the DRM setplane
> +	 * code that called us expects to handle the framebuffer update and
> +	 * reference counting; save and restore the current fb before
> +	 * calling it.
> +	 */
> +	tmpfb = plane->fb;
> +	ret = intel_pipe_set_base(crtc, src_x, src_y, fb);
> +	if (ret)
> +		return ret;
> +	plane->fb = tmpfb;
> +
> +	intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
> +				      intel_crtc->pipe);

Needs a primary_enabled check (+ a visibility check for the fully
clipped case).

> +
> +	return 0;
> +}
> +
> +static int
> +intel_primary_plane_disable(struct drm_plane *plane)
> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	struct intel_crtc *intel_crtc;
> +
> +	if (!plane->fb)
> +		return 0;
> +
> +	if (WARN_ON(!plane->crtc))
> +		return -EINVAL;
> +
> +	intel_crtc = to_intel_crtc(plane->crtc);
> +	if (intel_crtc->primary_enabled)
> +		intel_disable_primary_hw_plane(dev_priv, intel_plane->plane,
> +					       intel_plane->pipe);
> +
> +	/*
> +	 * N.B. The DRM setplane code will update the plane->fb pointer after
> +	 * we finish here.
> +	 */
> +	intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
> +
> +	return 0;
> +}
> +
> +static void intel_primary_plane_destroy(struct drm_plane *plane)
> +{
> +	struct intel_plane *intel_plane = to_intel_plane(plane);
> +	drm_plane_cleanup(plane);
> +	kfree(intel_plane);
> +}
> +
> +static const struct drm_plane_funcs intel_primary_plane_funcs = {
> +	.update_plane = intel_primary_plane_setplane,
> +	.disable_plane = intel_primary_plane_disable,
> +	.destroy = intel_primary_plane_destroy,
> +};
> +
> +static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> +						    int pipe)
> +{
> +	struct intel_plane *primary;
> +	const uint32_t *intel_primary_formats;
> +	int num_formats;
> +
> +	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> +	if (primary == NULL)
> +		return NULL;
> +
> +	primary->can_scale = false;
> +	primary->pipe = pipe;
> +	primary->plane = pipe;

We need to handle the pre-gen4 fbc primary plane swapping somewhere. I
guess we still have intel_crtc->plane as well. That should probably get
converted to use crtc->primary->plane instead.

> +
> +	if (INTEL_INFO(dev)->gen <= 3) {
> +		intel_primary_formats = intel_primary_formats_gen3;
> +		num_formats = ARRAY_SIZE(intel_primary_formats_gen3);
> +	} else {
> +		intel_primary_formats = intel_primary_formats_gen4;
> +		num_formats = ARRAY_SIZE(intel_primary_formats_gen4);
> +	}
> +
> +	drm_plane_init(dev, &primary->base, 0,
> +		       &intel_primary_plane_funcs, intel_primary_formats,
> +		       num_formats, DRM_PLANE_TYPE_PRIMARY);
> +	return &primary->base;
> +}
> +
>  static void intel_crtc_init(struct drm_device *dev, int pipe)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc;
> -	int i;
> +	struct drm_plane *primary;
> +	int i, ret;
>  
>  	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
>  	if (intel_crtc == NULL)
>  		return;
>  
> -	drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs);
> +	primary = intel_primary_plane_create(dev, pipe);
> +	ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
> +					NULL, &intel_crtc_funcs);
> +	if (ret) {
> +		drm_crtc_cleanup(&intel_crtc->base);

That should be drm_plane_cleanup() no?

> +		kfree(intel_crtc);
> +		return;
> +	}
>  
>  	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
>  	for (i = 0; i < 256; i++) {
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Matt Roper April 22, 2014, 3:18 p.m. UTC | #2
On Tue, Apr 22, 2014 at 03:47:37PM +0300, Ville Syrjälä wrote:
> On Fri, Apr 11, 2014 at 02:13:42PM -0700, Matt Roper wrote:
...
> > +	int ret;
> > +
> > +	/*
> > +	 * At the moment we use the same set of setplane restrictions as the
> > +	 * DRM primary plane helper, so go ahead and just call the helper if
> > +	 * the primary plane is already enabled.  We only need to take special
> > +	 * action if the primary plane is disabled (something i915 can do but
> > +	 * the generic helper can't).
> > +	 */
> > +	if (intel_crtc->primary_enabled)
> > +		return drm_primary_helper_update(plane, crtc, fb,
> > +						 crtc_x, crtc_y,
> > +						 crtc_w, crtc_h,
> > +						 src_x, src_y,
> > +						 src_w, src_h);
> 
> Why would we want to call that if we have a custom implementation
> anyway?

This was something Daniel requested on a previous patch iteration; even
though we're stuck duplicating most of the checks here for the !enabled
case, he still wanted to see us call into the helper for the enabled
case (although this will have to change in the future if/when we want to
start relaxing some of the tests that the helper does, such as plane
scaling).

...
> > +	/*
> > +	 * pipe_set_base() adjusts crtc->primary->fb; however the DRM setplane
> > +	 * code that called us expects to handle the framebuffer update and
> > +	 * reference counting; save and restore the current fb before
> > +	 * calling it.
> > +	 */
> > +	tmpfb = plane->fb;
> > +	ret = intel_pipe_set_base(crtc, src_x, src_y, fb);
> > +	if (ret)
> > +		return ret;
> > +	plane->fb = tmpfb;
> > +
> > +	intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
> > +				      intel_crtc->pipe);
> 
> Needs a primary_enabled check (+ a visibility check for the fully
> clipped case).

If primary_enabled, we already called directly into the helper and
returned up at the top of the function.

I'll go ahead and add the visibility test farther up the function
though.

...
> > +static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
> > +						    int pipe)
> > +{
> > +	struct intel_plane *primary;
> > +	const uint32_t *intel_primary_formats;
> > +	int num_formats;
> > +
> > +	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
> > +	if (primary == NULL)
> > +		return NULL;
> > +
> > +	primary->can_scale = false;
> > +	primary->pipe = pipe;
> > +	primary->plane = pipe;
> 
> We need to handle the pre-gen4 fbc primary plane swapping somewhere. I
> guess we still have intel_crtc->plane as well. That should probably get
> converted to use crtc->primary->plane instead.

Hmm, this is something I'm not terribly familiar with.  I'm guessing I
just need to copy the logic from intel_crtc_init()?

I can write a Coccinelle patch to replace the intel_crtc->plane with
crtc->primary->plane as well; thanks for pointing that out.


Thanks for the review; I'll send along a new patch that incorporates
your other feedback shortly (and I think you've pointed out a few things
here that apply to the primary plane helper code too).  I'm still
reworking my i-g-t patches for this functionality as well, but I've been
traveling lately and haven't had too much time to work on it.


Matt
Daniel Vetter April 22, 2014, 5:14 p.m. UTC | #3
On Tue, Apr 22, 2014 at 08:18:30AM -0700, Matt Roper wrote:
> On Tue, Apr 22, 2014 at 03:47:37PM +0300, Ville Syrjälä wrote:
> > On Fri, Apr 11, 2014 at 02:13:42PM -0700, Matt Roper wrote:
> ...
> > > +	int ret;
> > > +
> > > +	/*
> > > +	 * At the moment we use the same set of setplane restrictions as the
> > > +	 * DRM primary plane helper, so go ahead and just call the helper if
> > > +	 * the primary plane is already enabled.  We only need to take special
> > > +	 * action if the primary plane is disabled (something i915 can do but
> > > +	 * the generic helper can't).
> > > +	 */
> > > +	if (intel_crtc->primary_enabled)
> > > +		return drm_primary_helper_update(plane, crtc, fb,
> > > +						 crtc_x, crtc_y,
> > > +						 crtc_w, crtc_h,
> > > +						 src_x, src_y,
> > > +						 src_w, src_h);
> > 
> > Why would we want to call that if we have a custom implementation
> > anyway?
> 
> This was something Daniel requested on a previous patch iteration; even
> though we're stuck duplicating most of the checks here for the !enabled
> case, he still wanted to see us call into the helper for the enabled
> case (although this will have to change in the future if/when we want to
> start relaxing some of the tests that the helper does, such as plane
> scaling).

To clarify my request: I was unhappy with all the duplicated tests we have
and would like some way to share them with the plane helper code. If
there's no sane way to do that, then I'm ok with duplication.

I'm not sure any more what was the issue with extracting the tests from
the plane helper into a new function and reusing them with i915 though.
-Daniel
Matt Roper April 22, 2014, 5:32 p.m. UTC | #4
On Tue, Apr 22, 2014 at 07:14:22PM +0200, Daniel Vetter wrote:
> On Tue, Apr 22, 2014 at 08:18:30AM -0700, Matt Roper wrote:
> > On Tue, Apr 22, 2014 at 03:47:37PM +0300, Ville Syrjälä wrote:
> > > On Fri, Apr 11, 2014 at 02:13:42PM -0700, Matt Roper wrote:
> > ...
> > > > +	int ret;
> > > > +
> > > > +	/*
> > > > +	 * At the moment we use the same set of setplane restrictions as the
> > > > +	 * DRM primary plane helper, so go ahead and just call the helper if
> > > > +	 * the primary plane is already enabled.  We only need to take special
> > > > +	 * action if the primary plane is disabled (something i915 can do but
> > > > +	 * the generic helper can't).
> > > > +	 */
> > > > +	if (intel_crtc->primary_enabled)
> > > > +		return drm_primary_helper_update(plane, crtc, fb,
> > > > +						 crtc_x, crtc_y,
> > > > +						 crtc_w, crtc_h,
> > > > +						 src_x, src_y,
> > > > +						 src_w, src_h);
> > > 
> > > Why would we want to call that if we have a custom implementation
> > > anyway?
> > 
> > This was something Daniel requested on a previous patch iteration; even
> > though we're stuck duplicating most of the checks here for the !enabled
> > case, he still wanted to see us call into the helper for the enabled
> > case (although this will have to change in the future if/when we want to
> > start relaxing some of the tests that the helper does, such as plane
> > scaling).
> 
> To clarify my request: I was unhappy with all the duplicated tests we have
> and would like some way to share them with the plane helper code. If
> there's no sane way to do that, then I'm ok with duplication.
> 
> I'm not sure any more what was the issue with extracting the tests from
> the plane helper into a new function and reusing them with i915 though.
> -Daniel

Ah, okay.  I think I may have misunderstood what you were asking for the
previous time around.  Extracting the tests from the helper into a new
function should be doable; I'll include that in my next iteration.
Sorry for the confusion.


Matt
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1390ab5..3e4d36a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -39,8 +39,35 @@ 
 #include "i915_trace.h"
 #include <drm/drm_dp_helper.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_plane_helper.h>
+#include <drm/drm_rect.h>
 #include <linux/dma_remapping.h>
 
+/* Primary plane formats supported by all gen */
+#define COMMON_PRIMARY_FORMATS \
+	DRM_FORMAT_C8, \
+	DRM_FORMAT_RGB565, \
+	DRM_FORMAT_XRGB8888, \
+	DRM_FORMAT_ARGB8888
+
+/* Primary plane formats for gen <= 3 */
+const static uint32_t intel_primary_formats_gen3[] = {
+	COMMON_PRIMARY_FORMATS,
+	DRM_FORMAT_XRGB1555,
+	DRM_FORMAT_ARGB1555,
+};
+
+/* Primary plane formats for gen >= 4 */
+const static uint32_t intel_primary_formats_gen4[] = {
+	COMMON_PRIMARY_FORMATS, \
+	DRM_FORMAT_XBGR8888,
+	DRM_FORMAT_ABGR8888,
+	DRM_FORMAT_XRGB2101010,
+	DRM_FORMAT_ARGB2101010,
+	DRM_FORMAT_XBGR2101010,
+	DRM_FORMAT_ABGR2101010,
+};
+
 static void intel_increase_pllclock(struct drm_crtc *crtc);
 static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
 
@@ -10556,17 +10583,183 @@  static void intel_shared_dpll_init(struct drm_device *dev)
 	BUG_ON(dev_priv->num_shared_dpll > I915_NUM_PLLS);
 }
 
+static int
+intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
+			     struct drm_framebuffer *fb, int crtc_x, int crtc_y,
+			     unsigned int crtc_w, unsigned int crtc_h,
+			     uint32_t src_x, uint32_t src_y,
+			     uint32_t src_w, uint32_t src_h)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct drm_framebuffer *tmpfb;
+	struct drm_rect dest = {
+		.x1 = crtc_x,
+		.y1 = crtc_y,
+		.x2 = crtc_x + crtc_w,
+		.y2 = crtc_y + crtc_h,
+	};
+	struct drm_rect clip = {
+		.x2 = crtc->mode.hdisplay,
+		.y2 = crtc->mode.vdisplay,
+	};
+	int ret;
+
+	/*
+	 * At the moment we use the same set of setplane restrictions as the
+	 * DRM primary plane helper, so go ahead and just call the helper if
+	 * the primary plane is already enabled.  We only need to take special
+	 * action if the primary plane is disabled (something i915 can do but
+	 * the generic helper can't).
+	 */
+	if (intel_crtc->primary_enabled)
+		return drm_primary_helper_update(plane, crtc, fb,
+						 crtc_x, crtc_y,
+						 crtc_w, crtc_h,
+						 src_x, src_y,
+						 src_w, src_h);
+
+	/* setplane API takes shifted source rectangle values; unshift them */
+	src_x >>= 16;
+	src_y >>= 16;
+	src_w >>= 16;
+	src_h >>= 16;
+
+	/*
+	 * Current hardware can't reposition the primary plane or scale it
+	 * (although this could change in the future).
+	 */
+	drm_rect_intersect(&dest, &clip);
+	if (dest.x1 != 0 || dest.y1 != 0 ||
+	    dest.x2 != crtc->mode.hdisplay || dest.y2 != crtc->mode.vdisplay) {
+		DRM_DEBUG_KMS("Primary plane must cover entire CRTC\n");
+		return -EINVAL;
+	}
+
+	if (crtc_w != src_w || crtc_h != src_h) {
+		DRM_DEBUG_KMS("Can't scale primary plane\n");
+		return -EINVAL;
+	}
+
+	/* Primary planes are locked to their owning CRTC */
+	if (plane->possible_crtcs != drm_crtc_mask(crtc)) {
+		DRM_DEBUG_KMS("Cannot change primary plane CRTC\n");
+		return -EINVAL;
+	}
+
+	/* Framebuffer must be big enough to cover entire plane */
+	ret = drm_crtc_check_viewport(crtc, crtc_x, crtc_y, &crtc->mode, fb);
+	if (ret)
+		return ret;
+
+	/*
+	 * pipe_set_base() adjusts crtc->primary->fb; however the DRM setplane
+	 * code that called us expects to handle the framebuffer update and
+	 * reference counting; save and restore the current fb before
+	 * calling it.
+	 */
+	tmpfb = plane->fb;
+	ret = intel_pipe_set_base(crtc, src_x, src_y, fb);
+	if (ret)
+		return ret;
+	plane->fb = tmpfb;
+
+	intel_enable_primary_hw_plane(dev_priv, intel_crtc->plane,
+				      intel_crtc->pipe);
+
+	return 0;
+}
+
+static int
+intel_primary_plane_disable(struct drm_plane *plane)
+{
+	struct drm_device *dev = plane->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	struct intel_crtc *intel_crtc;
+
+	if (!plane->fb)
+		return 0;
+
+	if (WARN_ON(!plane->crtc))
+		return -EINVAL;
+
+	intel_crtc = to_intel_crtc(plane->crtc);
+	if (intel_crtc->primary_enabled)
+		intel_disable_primary_hw_plane(dev_priv, intel_plane->plane,
+					       intel_plane->pipe);
+
+	/*
+	 * N.B. The DRM setplane code will update the plane->fb pointer after
+	 * we finish here.
+	 */
+	intel_unpin_fb_obj(to_intel_framebuffer(plane->fb)->obj);
+
+	return 0;
+}
+
+static void intel_primary_plane_destroy(struct drm_plane *plane)
+{
+	struct intel_plane *intel_plane = to_intel_plane(plane);
+	drm_plane_cleanup(plane);
+	kfree(intel_plane);
+}
+
+static const struct drm_plane_funcs intel_primary_plane_funcs = {
+	.update_plane = intel_primary_plane_setplane,
+	.disable_plane = intel_primary_plane_disable,
+	.destroy = intel_primary_plane_destroy,
+};
+
+static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
+						    int pipe)
+{
+	struct intel_plane *primary;
+	const uint32_t *intel_primary_formats;
+	int num_formats;
+
+	primary = kzalloc(sizeof(*primary), GFP_KERNEL);
+	if (primary == NULL)
+		return NULL;
+
+	primary->can_scale = false;
+	primary->pipe = pipe;
+	primary->plane = pipe;
+
+	if (INTEL_INFO(dev)->gen <= 3) {
+		intel_primary_formats = intel_primary_formats_gen3;
+		num_formats = ARRAY_SIZE(intel_primary_formats_gen3);
+	} else {
+		intel_primary_formats = intel_primary_formats_gen4;
+		num_formats = ARRAY_SIZE(intel_primary_formats_gen4);
+	}
+
+	drm_plane_init(dev, &primary->base, 0,
+		       &intel_primary_plane_funcs, intel_primary_formats,
+		       num_formats, DRM_PLANE_TYPE_PRIMARY);
+	return &primary->base;
+}
+
 static void intel_crtc_init(struct drm_device *dev, int pipe)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc;
-	int i;
+	struct drm_plane *primary;
+	int i, ret;
 
 	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
 	if (intel_crtc == NULL)
 		return;
 
-	drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs);
+	primary = intel_primary_plane_create(dev, pipe);
+	ret = drm_crtc_init_with_planes(dev, &intel_crtc->base, primary,
+					NULL, &intel_crtc_funcs);
+	if (ret) {
+		drm_crtc_cleanup(&intel_crtc->base);
+		kfree(intel_crtc);
+		return;
+	}
 
 	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
 	for (i = 0; i < 256; i++) {