[PATCH 0/4] Expose primary planes to userspace

[PATCH 0/4] Expose primary planes to userspace

[Matt Roper]

One of the stepping stones on the way to atomic/nuclear operation is to expose
CRTC primary planes (and eventually cursor planes too) to userspace in the same
manner that sprites/overlays are today.  This patch series takes the first step
of allowing drivers to register a CRTC's primary plane with the DRM core; the
core will then include this primary plane in the plane list returned to
userspace for clients that set an appropriate client capability bit.

This patchset isn't terribly interesting on its own; you can call the
traditional KMS plane API's (like SetPlane) on the CRTC's primary plane now,
but this doesn't really allow us to do anything we couldn't before.  The real
goal here is to pave the way for a cleaner API as we move toward atomic/nuclear
updates and the eventual property-ification of all settings.

This patch set only updates the i915 driver at the moment, but drivers that
aren't updated should continue to function as they have in the past (i.e., the
plane list returned to userspace will contain only sprite/overlay planes,
regardless of whether the userspace client sets the capability bit or not).

The patch set is organized as follows:
 - Patch 1 updates the DRM core with general support for primary planes, adding
   the interface for drivers to register primary planes, and a client cap bit
   that determines whether these primary planes should be returned to userspace
   clients.

 - Patch 2 adds a new "plane type" property to plane objects which allow
   userspace to distinguish primary planes (and probably cursor planes in the
   future) from regular sprite/overlay planes.

 - Patch 3 simply renames the i915 'update_plane' functions in intel_display.c
   to 'update_primary_plane.'  This change just helps prevent confusion between
   these functions and the similarly named sprite-related ones in
   intel_sprite.c.  We also rename the intel_disable_primary_plane() function
   to prevent confusion with a new intel_primary_plane_disable() function
   added in patch #4.

 - Patch 4 updates the i915 driver to create a primary plane for each CRTC at
   CRTC initialization time and register it with the DRM core.  The setplane
   handler for primary planes simply performs an MMIO flip of the provided
   framebuffer.

Matt Roper (4):
  drm: Add support for CRTC primary planes
  drm: Add plane type property
  drm/i915: Rename similar plane functions to avoid confusion
  drm/i915: Register primary plane for each CRTC

 drivers/gpu/drm/drm_crtc.c           | 200 ++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/drm_ioctl.c          |   5 +
 drivers/gpu/drm/i915/i915_drv.h      |   5 +-
 drivers/gpu/drm/i915/intel_display.c | 132 ++++++++++++++++++++---
 include/drm/drmP.h                   |   2 +
 include/drm/drm_crtc.h               |  12 +++
 include/uapi/drm/drm.h               |   8 ++
 include/uapi/drm/drm_mode.h          |   3 +
 8 files changed, 343 insertions(+), 24 deletions(-)

-- 
1.8.5.1

[PATCH 1/4] drm: Add support for CRTC primary planes

[Matt Roper]

Allow drivers to provide a drm_plane structure corresponding to a CRTC's
primary plane.  These planes will be included in the plane list for any
clients setting the DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES capability bit.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_crtc.c  | 168 ++++++++++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/drm_ioctl.c |   5 ++
 include/drm/drmP.h          |   2 +
 include/drm/drm_crtc.h      |  11 +++
 include/uapi/drm/drm.h      |   8 +++
 5 files changed, 189 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 35ea15d..21c6d4b 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -274,6 +274,74 @@ const char *drm_get_connector_status_name(enum drm_connector_status status)
 }
 EXPORT_SYMBOL(drm_get_connector_status_name);
 
+/*
+ * Default set of pixel formats supported by primary planes.  This matches the
+ * set of formats accepted by the traditional modesetting interfaces; drivers
+ * need only provide their own format list if it differs from the default.
+ */
+static const uint32_t default_primary_plane_formats[] = {
+	DRM_FORMAT_C8,
+	DRM_FORMAT_RGB332,
+	DRM_FORMAT_BGR233,
+	DRM_FORMAT_XRGB4444,
+	DRM_FORMAT_XBGR4444,
+	DRM_FORMAT_RGBX4444,
+	DRM_FORMAT_BGRX4444,
+	DRM_FORMAT_ARGB4444,
+	DRM_FORMAT_ABGR4444,
+	DRM_FORMAT_RGBA4444,
+	DRM_FORMAT_BGRA4444,
+	DRM_FORMAT_XRGB1555,
+	DRM_FORMAT_XBGR1555,
+	DRM_FORMAT_RGBX5551,
+	DRM_FORMAT_BGRX5551,
+	DRM_FORMAT_ARGB1555,
+	DRM_FORMAT_ABGR1555,
+	DRM_FORMAT_RGBA5551,
+	DRM_FORMAT_BGRA5551,
+	DRM_FORMAT_RGB565,
+	DRM_FORMAT_BGR565,
+	DRM_FORMAT_RGB888,
+	DRM_FORMAT_BGR888,
+	DRM_FORMAT_XRGB8888,
+	DRM_FORMAT_XBGR8888,
+	DRM_FORMAT_RGBX8888,
+	DRM_FORMAT_BGRX8888,
+	DRM_FORMAT_ARGB8888,
+	DRM_FORMAT_ABGR8888,
+	DRM_FORMAT_RGBA8888,
+	DRM_FORMAT_BGRA8888,
+	DRM_FORMAT_XRGB2101010,
+	DRM_FORMAT_XBGR2101010,
+	DRM_FORMAT_RGBX1010102,
+	DRM_FORMAT_BGRX1010102,
+	DRM_FORMAT_ARGB2101010,
+	DRM_FORMAT_ABGR2101010,
+	DRM_FORMAT_RGBA1010102,
+	DRM_FORMAT_BGRA1010102,
+	DRM_FORMAT_YUYV,
+	DRM_FORMAT_YVYU,
+	DRM_FORMAT_UYVY,
+	DRM_FORMAT_VYUY,
+	DRM_FORMAT_AYUV,
+	DRM_FORMAT_NV12,
+	DRM_FORMAT_NV21,
+	DRM_FORMAT_NV16,
+	DRM_FORMAT_NV61,
+	DRM_FORMAT_NV24,
+	DRM_FORMAT_NV42,
+	DRM_FORMAT_YUV410,
+	DRM_FORMAT_YVU410,
+	DRM_FORMAT_YUV411,
+	DRM_FORMAT_YVU411,
+	DRM_FORMAT_YUV420,
+	DRM_FORMAT_YVU420,
+	DRM_FORMAT_YUV422,
+	DRM_FORMAT_YVU422,
+	DRM_FORMAT_YUV444,
+	DRM_FORMAT_YVU444,
+};
+
 /**
  * drm_get_subpixel_order_name - return a string for a given subpixel enum
  * @order: enum of subpixel_order
@@ -921,7 +989,7 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
 EXPORT_SYMBOL(drm_encoder_cleanup);
 
 /**
- * drm_plane_init - Initialise a new plane object
+ * drm_plane_init - Initialise a new sprite plane object
  * @dev: DRM device
  * @plane: plane object to init
  * @possible_crtcs: bitmask of possible CRTCs
@@ -930,7 +998,9 @@ EXPORT_SYMBOL(drm_encoder_cleanup);
  * @format_count: number of elements in @formats
  * @priv: plane is private (hidden from userspace)?
  *
- * Inits a new object created as base part of a driver plane object.
+ * Inits a new object created as base part of a driver plane object.  This
+ * function should only be called for traditional sprite/overlay planes.
+ * Primary planes should be initialized via @drm_plane_set_primary.
  *
  * RETURNS:
  * Zero on success, error code on failure.
@@ -984,6 +1054,74 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
 EXPORT_SYMBOL(drm_plane_init);
 
 /**
+ * drm_plane_set_primary - Supply a primary plane for a CRTC
+ * @dev DRM device
+ * @plane: plane object representing the primary plane
+ * @crtc: CRTC this plane is associated with
+ * @funcs: callbacks for the new plane
+ * @formats: array of supported formats (%DRM_FORMAT_*).  If NULL, the
+ *           default list of formats traditionally supported by modesetting
+ *           API's will be used and @format_count will be ignored.
+ * @format_count: number of elements in @formats
+ *
+ * Provides a drm_plane representing a CRTC's primary plane.  This plane will
+ * only be exposed to clients that set the DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES
+ * capability bit.
+ *
+ * RETURNS:
+ * Zero on success, error code on failure.
+ */
+int drm_plane_set_primary(struct drm_device *dev, struct drm_plane *plane,
+			  struct drm_crtc *crtc,
+			  const struct drm_plane_funcs *funcs,
+			  const uint32_t *formats, uint32_t format_count)
+{
+	int ret;
+
+	drm_modeset_lock_all(dev);
+
+	ret = drm_mode_object_get(dev, &plane->base, DRM_MODE_OBJECT_PLANE);
+	if (ret)
+		goto out;
+
+	if (formats == NULL) {
+		formats = default_primary_plane_formats;
+		format_count = ARRAY_SIZE(default_primary_plane_formats);
+	}
+
+	plane->base.properties = &plane->properties;
+	plane->dev = dev;
+	plane->funcs = funcs;
+	plane->format_types = kmalloc(sizeof(uint32_t) * format_count,
+				      GFP_KERNEL);
+	if (!plane->format_types) {
+		DRM_DEBUG_KMS("out of memory when setting up primary plane\n");
+		drm_mode_object_put(dev, &plane->base);
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	memcpy(plane->format_types, formats, format_count * sizeof(uint32_t));
+	plane->format_count = format_count;
+	plane->possible_crtcs = drm_crtc_mask(crtc);
+
+	/*
+	 * Primary planes are not added to the general plane list, only linked
+	 * to their CRTC.  They will only be advertised to userspace clients
+	 * that indicate support.
+	 */
+	crtc->primary_plane = plane;
+	dev->mode_config.num_primary_plane++;
+	INIT_LIST_HEAD(&plane->head);
+
+ out:
+	drm_modeset_unlock_all(dev);
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_plane_set_primary);
+
+/**
  * drm_plane_cleanup - Clean up the core plane usage
  * @plane: plane to cleanup
  *
@@ -1836,8 +1974,10 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data,
 	struct drm_mode_get_plane_res *plane_resp = data;
 	struct drm_mode_config *config;
 	struct drm_plane *plane;
+	struct drm_crtc *crtc;
 	uint32_t __user *plane_ptr;
 	int copied = 0, ret = 0;
+	unsigned num_planes;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
@@ -1845,14 +1985,32 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data,
 	drm_modeset_lock_all(dev);
 	config = &dev->mode_config;
 
+	num_planes = config->num_plane;
+	if (file_priv->expose_primary_planes)
+		num_planes += config->num_primary_plane;
+
 	/*
 	 * This ioctl is called twice, once to determine how much space is
 	 * needed, and the 2nd time to fill it.
 	 */
-	if (config->num_plane &&
-	    (plane_resp->count_planes >= config->num_plane)) {
+	if (num_planes &&
+	    (plane_resp->count_planes >= num_planes)) {
 		plane_ptr = (uint32_t __user *)(unsigned long)plane_resp->plane_id_ptr;
 
+		if (file_priv->expose_primary_planes) {
+			list_for_each_entry(crtc, &config->crtc_list, head) {
+				if (!crtc->primary_plane)
+					continue;
+
+				if (put_user(crtc->primary_plane->base.id,
+					     plane_ptr + copied)) {
+					ret = -EFAULT;
+					goto out;
+				}
+				copied++;
+			}
+		}
+
 		list_for_each_entry(plane, &config->plane_list, head) {
 			if (put_user(plane->base.id, plane_ptr + copied)) {
 				ret = -EFAULT;
@@ -1861,7 +2019,7 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data,
 			copied++;
 		}
 	}
-	plane_resp->count_planes = config->num_plane;
+	plane_resp->count_planes = num_planes;
 
 out:
 	drm_modeset_unlock_all(dev);
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index dffc836..03579d6 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -316,6 +316,11 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
 			return -EINVAL;
 		file_priv->stereo_allowed = req->value;
 		break;
+	case DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES:
+		if (req->value > 1)
+			return -EINVAL;
+		file_priv->expose_primary_planes = req->value;
+		break;
 	default:
 		return -EINVAL;
 	}
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 04a7f31..997fa29 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -437,6 +437,8 @@ struct drm_file {
 	unsigned is_master :1; /* this file private is a master for a minor */
 	/* true when the client has asked us to expose stereo 3D mode flags */
 	unsigned stereo_allowed :1;
+	/* true if client understands CRTC primary planes in the plane list */
+	unsigned expose_primary_planes:1;
 
 	struct pid *pid;
 	kuid_t uid;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index ce9ee60..33a955b 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -429,6 +429,11 @@ struct drm_crtc {
 	 * by drm_mode_set_config_internal to implement correct refcounting. */
 	struct drm_framebuffer *old_fb;
 
+	/* Primary plane to expose to userspace if the client sets the 'expose
+	 * primary planes' cap.
+	 */
+	struct drm_plane *primary_plane;
+
 	bool enabled;
 
 	/* Requested mode from modesetting. */
@@ -859,6 +864,7 @@ struct drm_mode_config {
 	int num_plane;
 	struct list_head plane_list;
 
+	int num_primary_plane;
 	int num_crtc;
 	struct list_head crtc_list;
 
@@ -984,6 +990,11 @@ extern int drm_plane_init(struct drm_device *dev,
 			  const struct drm_plane_funcs *funcs,
 			  const uint32_t *formats, uint32_t format_count,
 			  bool priv);
+extern int drm_plane_set_primary(struct drm_device *dev,
+				 struct drm_plane *plane, struct drm_crtc *crtc,
+				 const struct drm_plane_funcs *funcs,
+				 const uint32_t *formats,
+				 uint32_t format_count);
 extern void drm_plane_cleanup(struct drm_plane *plane);
 extern void drm_plane_force_disable(struct drm_plane *plane);
 
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 3c9a833..58a2468 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -635,6 +635,14 @@ struct drm_get_cap {
  */
 #define DRM_CLIENT_CAP_STEREO_3D	1
 
+/**
+ * DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES
+ *
+ * If set to 1, the DRM core will expose CRTC primary planes along with
+ * sprite/overlay planes.
+ */
+#define DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES  2
+
 /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
 struct drm_set_client_cap {
 	__u64 capability;
-- 
1.8.5.1

Re: [PATCH 1/4] drm: Add support for CRTC primary planes

[Damien Lespiau]

On Thu, Feb 27, 2014 at 02:14:40PM -0800, Matt Roper wrote:
> Allow drivers to provide a drm_plane structure corresponding to a CRTC's
> primary plane.  These planes will be included in the plane list for any
> clients setting the DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES capability bit.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Two small notes here and comments inside:

- In term of new API enabling and to make the tree bisectable, one needs
  to 1/ do the preparation work 2/ enable the API. In this case, I would
  split up the patch to make the ioctl bits independent and the last
  patch of the series.

- I don't see a point in introducing the complexity to enable sprite and
  cursor planes independently from each other. So before enabling the
  new setcap() ioctl, could we also expose the cursor planes and call
  the capability "universal planes" or similar?

-- 
Damien

> ---
>  drivers/gpu/drm/drm_crtc.c  | 168 ++++++++++++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/drm_ioctl.c |   5 ++
>  include/drm/drmP.h          |   2 +
>  include/drm/drm_crtc.h      |  11 +++
>  include/uapi/drm/drm.h      |   8 +++
>  5 files changed, 189 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 35ea15d..21c6d4b 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -274,6 +274,74 @@ const char *drm_get_connector_status_name(enum drm_connector_status status)
>  }
>  EXPORT_SYMBOL(drm_get_connector_status_name);
>  
> +/*
> + * Default set of pixel formats supported by primary planes.  This matches the
> + * set of formats accepted by the traditional modesetting interfaces; drivers
> + * need only provide their own format list if it differs from the default.
> + */
> +static const uint32_t default_primary_plane_formats[] = {
> +	DRM_FORMAT_C8,
> +	DRM_FORMAT_RGB332,
> +	DRM_FORMAT_BGR233,
> +	DRM_FORMAT_XRGB4444,
> +	DRM_FORMAT_XBGR4444,
> +	DRM_FORMAT_RGBX4444,
> +	DRM_FORMAT_BGRX4444,
> +	DRM_FORMAT_ARGB4444,
> +	DRM_FORMAT_ABGR4444,
> +	DRM_FORMAT_RGBA4444,
> +	DRM_FORMAT_BGRA4444,
> +	DRM_FORMAT_XRGB1555,
> +	DRM_FORMAT_XBGR1555,
> +	DRM_FORMAT_RGBX5551,
> +	DRM_FORMAT_BGRX5551,
> +	DRM_FORMAT_ARGB1555,
> +	DRM_FORMAT_ABGR1555,
> +	DRM_FORMAT_RGBA5551,
> +	DRM_FORMAT_BGRA5551,
> +	DRM_FORMAT_RGB565,
> +	DRM_FORMAT_BGR565,
> +	DRM_FORMAT_RGB888,
> +	DRM_FORMAT_BGR888,
> +	DRM_FORMAT_XRGB8888,
> +	DRM_FORMAT_XBGR8888,
> +	DRM_FORMAT_RGBX8888,
> +	DRM_FORMAT_BGRX8888,
> +	DRM_FORMAT_ARGB8888,
> +	DRM_FORMAT_ABGR8888,
> +	DRM_FORMAT_RGBA8888,
> +	DRM_FORMAT_BGRA8888,
> +	DRM_FORMAT_XRGB2101010,
> +	DRM_FORMAT_XBGR2101010,
> +	DRM_FORMAT_RGBX1010102,
> +	DRM_FORMAT_BGRX1010102,
> +	DRM_FORMAT_ARGB2101010,
> +	DRM_FORMAT_ABGR2101010,
> +	DRM_FORMAT_RGBA1010102,
> +	DRM_FORMAT_BGRA1010102,
> +	DRM_FORMAT_YUYV,
> +	DRM_FORMAT_YVYU,
> +	DRM_FORMAT_UYVY,
> +	DRM_FORMAT_VYUY,
> +	DRM_FORMAT_AYUV,
> +	DRM_FORMAT_NV12,
> +	DRM_FORMAT_NV21,
> +	DRM_FORMAT_NV16,
> +	DRM_FORMAT_NV61,
> +	DRM_FORMAT_NV24,
> +	DRM_FORMAT_NV42,
> +	DRM_FORMAT_YUV410,
> +	DRM_FORMAT_YVU410,
> +	DRM_FORMAT_YUV411,
> +	DRM_FORMAT_YVU411,
> +	DRM_FORMAT_YUV420,
> +	DRM_FORMAT_YVU420,
> +	DRM_FORMAT_YUV422,
> +	DRM_FORMAT_YVU422,
> +	DRM_FORMAT_YUV444,
> +	DRM_FORMAT_YVU444,
> +};
> +
>  /**
>   * drm_get_subpixel_order_name - return a string for a given subpixel enum
>   * @order: enum of subpixel_order
> @@ -921,7 +989,7 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
>  EXPORT_SYMBOL(drm_encoder_cleanup);
>  
>  /**
> - * drm_plane_init - Initialise a new plane object
> + * drm_plane_init - Initialise a new sprite plane object
>   * @dev: DRM device
>   * @plane: plane object to init
>   * @possible_crtcs: bitmask of possible CRTCs
> @@ -930,7 +998,9 @@ EXPORT_SYMBOL(drm_encoder_cleanup);
>   * @format_count: number of elements in @formats
>   * @priv: plane is private (hidden from userspace)?
>   *
> - * Inits a new object created as base part of a driver plane object.
> + * Inits a new object created as base part of a driver plane object.  This
> + * function should only be called for traditional sprite/overlay planes.
> + * Primary planes should be initialized via @drm_plane_set_primary.
>   *
>   * RETURNS:
>   * Zero on success, error code on failure.
> @@ -984,6 +1054,74 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  EXPORT_SYMBOL(drm_plane_init);
>  
>  /**
> + * drm_plane_set_primary - Supply a primary plane for a CRTC
> + * @dev DRM device
> + * @plane: plane object representing the primary plane
> + * @crtc: CRTC this plane is associated with
> + * @funcs: callbacks for the new plane
> + * @formats: array of supported formats (%DRM_FORMAT_*).  If NULL, the
> + *           default list of formats traditionally supported by modesetting
> + *           API's will be used and @format_count will be ignored.
> + * @format_count: number of elements in @formats
> + *
> + * Provides a drm_plane representing a CRTC's primary plane.  This plane will
> + * only be exposed to clients that set the DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES
> + * capability bit.
> + *
> + * RETURNS:
> + * Zero on success, error code on failure.
> + */
> +int drm_plane_set_primary(struct drm_device *dev, struct drm_plane *plane,
> +			  struct drm_crtc *crtc,
> +			  const struct drm_plane_funcs *funcs,
> +			  const uint32_t *formats, uint32_t format_count)

The "priv" parameter of drm_plane_init() seems to be used by drivers to
initialize those planes attached to the CRTC object (needs double
checking). Could we refactor drm_plane_init() so drm_plane_set_primary()
calls drm_plane_init() with priv=true (argument that could then renamed
to primary).

> +{
> +	int ret;
> +
> +	drm_modeset_lock_all(dev);
> +
> +	ret = drm_mode_object_get(dev, &plane->base, DRM_MODE_OBJECT_PLANE);
> +	if (ret)
> +		goto out;
> +
> +	if (formats == NULL) {
> +		formats = default_primary_plane_formats;
> +		format_count = ARRAY_SIZE(default_primary_plane_formats);
> +	}

We're defining a new interface here, I don't think there's any value in
providing a default set of format that is unlikely to match any
hardware. I'd just enforce that drivers need to provide the list of
acceptable formats.

> +
> +	plane->base.properties = &plane->properties;
> +	plane->dev = dev;
> +	plane->funcs = funcs;
> +	plane->format_types = kmalloc(sizeof(uint32_t) * format_count,
> +				      GFP_KERNEL);
> +	if (!plane->format_types) {
> +		DRM_DEBUG_KMS("out of memory when setting up primary plane\n");
> +		drm_mode_object_put(dev, &plane->base);
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	memcpy(plane->format_types, formats, format_count * sizeof(uint32_t));
> +	plane->format_count = format_count;
> +	plane->possible_crtcs = drm_crtc_mask(crtc);
> +
> +	/*
> +	 * Primary planes are not added to the general plane list, only linked
> +	 * to their CRTC.  They will only be advertised to userspace clients
> +	 * that indicate support.
> +	 */
> +	crtc->primary_plane = plane;
> +	dev->mode_config.num_primary_plane++;
> +	INIT_LIST_HEAD(&plane->head);
> +
> + out:
> +	drm_modeset_unlock_all(dev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_plane_set_primary);
> +
> +/**
>   * drm_plane_cleanup - Clean up the core plane usage
>   * @plane: plane to cleanup
>   *
> @@ -1836,8 +1974,10 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data,
>  	struct drm_mode_get_plane_res *plane_resp = data;
>  	struct drm_mode_config *config;
>  	struct drm_plane *plane;
> +	struct drm_crtc *crtc;
>  	uint32_t __user *plane_ptr;
>  	int copied = 0, ret = 0;
> +	unsigned num_planes;
>  
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EINVAL;
> @@ -1845,14 +1985,32 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data,
>  	drm_modeset_lock_all(dev);
>  	config = &dev->mode_config;
>  
> +	num_planes = config->num_plane;
> +	if (file_priv->expose_primary_planes)
> +		num_planes += config->num_primary_plane;
> +
>  	/*
>  	 * This ioctl is called twice, once to determine how much space is
>  	 * needed, and the 2nd time to fill it.
>  	 */
> -	if (config->num_plane &&
> -	    (plane_resp->count_planes >= config->num_plane)) {
> +	if (num_planes &&
> +	    (plane_resp->count_planes >= num_planes)) {
>  		plane_ptr = (uint32_t __user *)(unsigned long)plane_resp->plane_id_ptr;
>  
> +		if (file_priv->expose_primary_planes) {
> +			list_for_each_entry(crtc, &config->crtc_list, head) {
> +				if (!crtc->primary_plane)
> +					continue;
> +
> +				if (put_user(crtc->primary_plane->base.id,
> +					     plane_ptr + copied)) {
> +					ret = -EFAULT;
> +					goto out;
> +				}
> +				copied++;
> +			}
> +		}
> +
>  		list_for_each_entry(plane, &config->plane_list, head) {
>  			if (put_user(plane->base.id, plane_ptr + copied)) {
>  				ret = -EFAULT;
> @@ -1861,7 +2019,7 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data,
>  			copied++;
>  		}
>  	}
> -	plane_resp->count_planes = config->num_plane;
> +	plane_resp->count_planes = num_planes;
>  
>  out:
>  	drm_modeset_unlock_all(dev);
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index dffc836..03579d6 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -316,6 +316,11 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
>  			return -EINVAL;
>  		file_priv->stereo_allowed = req->value;
>  		break;
> +	case DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES:
> +		if (req->value > 1)
> +			return -EINVAL;
> +		file_priv->expose_primary_planes = req->value;
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 04a7f31..997fa29 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -437,6 +437,8 @@ struct drm_file {
>  	unsigned is_master :1; /* this file private is a master for a minor */
>  	/* true when the client has asked us to expose stereo 3D mode flags */
>  	unsigned stereo_allowed :1;
> +	/* true if client understands CRTC primary planes in the plane list */
> +	unsigned expose_primary_planes:1;
>  
>  	struct pid *pid;
>  	kuid_t uid;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index ce9ee60..33a955b 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -429,6 +429,11 @@ struct drm_crtc {
>  	 * by drm_mode_set_config_internal to implement correct refcounting. */
>  	struct drm_framebuffer *old_fb;
>  
> +	/* Primary plane to expose to userspace if the client sets the 'expose
> +	 * primary planes' cap.
> +	 */
> +	struct drm_plane *primary_plane;
> +
>  	bool enabled;
>  
>  	/* Requested mode from modesetting. */
> @@ -859,6 +864,7 @@ struct drm_mode_config {
>  	int num_plane;
>  	struct list_head plane_list;
>  
> +	int num_primary_plane;
>  	int num_crtc;
>  	struct list_head crtc_list;
>  
> @@ -984,6 +990,11 @@ extern int drm_plane_init(struct drm_device *dev,
>  			  const struct drm_plane_funcs *funcs,
>  			  const uint32_t *formats, uint32_t format_count,
>  			  bool priv);
> +extern int drm_plane_set_primary(struct drm_device *dev,
> +				 struct drm_plane *plane, struct drm_crtc *crtc,
> +				 const struct drm_plane_funcs *funcs,
> +				 const uint32_t *formats,
> +				 uint32_t format_count);
>  extern void drm_plane_cleanup(struct drm_plane *plane);
>  extern void drm_plane_force_disable(struct drm_plane *plane);
>  
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 3c9a833..58a2468 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -635,6 +635,14 @@ struct drm_get_cap {
>   */
>  #define DRM_CLIENT_CAP_STEREO_3D	1
>  
> +/**
> + * DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES
> + *
> + * If set to 1, the DRM core will expose CRTC primary planes along with
> + * sprite/overlay planes.
> + */
> +#define DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES  2
> +
>  /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
>  struct drm_set_client_cap {
>  	__u64 capability;
> -- 
> 1.8.5.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm: Add support for CRTC primary planes

[Matt Roper]

On Mon, Mar 03, 2014 at 03:47:43PM +0000, Damien Lespiau wrote:
> On Thu, Feb 27, 2014 at 02:14:40PM -0800, Matt Roper wrote:
> > Allow drivers to provide a drm_plane structure corresponding to a CRTC's
> > primary plane.  These planes will be included in the plane list for any
> > clients setting the DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES capability bit.
> > 
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> 
> Two small notes here and comments inside:
> 
> - In term of new API enabling and to make the tree bisectable, one needs
>   to 1/ do the preparation work 2/ enable the API. In this case, I would
>   split up the patch to make the ioctl bits independent and the last
>   patch of the series.
> 
> - I don't see a point in introducing the complexity to enable sprite and
>   cursor planes independently from each other. So before enabling the
>   new setcap() ioctl, could we also expose the cursor planes and call
>   the capability "universal planes" or similar?

I'm not sure I follow what you're saying on this second point.  Sprites
(or overlays) are already exposed to userspace, so existing clients
expect anything they receive via drmModeGetPlaneResources() to behave in
the traditional manner.  If we start throwing cursor planes into that
list without hiding them behind a capability bit, existing userspace try
to blindly use the cursors as full overlays without realizing that they
may carry additional restrictions on size and such, which will lead to
userspace breakage.

In cases where userspace has set a capability bit, I agree that
sprites/overlays/cursors could all pretty much be treated the same as
long as there are additional plane properties to let userspace
understand the exact restrictions of each plane.  Is that what you were
referring to here?

I agree with your other comments; I'll work on a second revision that
incorporates the suggestions from you and Rob.  Thanks for the review.


Matt

> 
> -- 
> Damien
> 
> > ---
> >  drivers/gpu/drm/drm_crtc.c  | 168 ++++++++++++++++++++++++++++++++++++++++++--
> >  drivers/gpu/drm/drm_ioctl.c |   5 ++
> >  include/drm/drmP.h          |   2 +
> >  include/drm/drm_crtc.h      |  11 +++
> >  include/uapi/drm/drm.h      |   8 +++
> >  5 files changed, 189 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 35ea15d..21c6d4b 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -274,6 +274,74 @@ const char *drm_get_connector_status_name(enum drm_connector_status status)
> >  }
> >  EXPORT_SYMBOL(drm_get_connector_status_name);
> >  
> > +/*
> > + * Default set of pixel formats supported by primary planes.  This matches the
> > + * set of formats accepted by the traditional modesetting interfaces; drivers
> > + * need only provide their own format list if it differs from the default.
> > + */
> > +static const uint32_t default_primary_plane_formats[] = {
> > +	DRM_FORMAT_C8,
> > +	DRM_FORMAT_RGB332,
> > +	DRM_FORMAT_BGR233,
> > +	DRM_FORMAT_XRGB4444,
> > +	DRM_FORMAT_XBGR4444,
> > +	DRM_FORMAT_RGBX4444,
> > +	DRM_FORMAT_BGRX4444,
> > +	DRM_FORMAT_ARGB4444,
> > +	DRM_FORMAT_ABGR4444,
> > +	DRM_FORMAT_RGBA4444,
> > +	DRM_FORMAT_BGRA4444,
> > +	DRM_FORMAT_XRGB1555,
> > +	DRM_FORMAT_XBGR1555,
> > +	DRM_FORMAT_RGBX5551,
> > +	DRM_FORMAT_BGRX5551,
> > +	DRM_FORMAT_ARGB1555,
> > +	DRM_FORMAT_ABGR1555,
> > +	DRM_FORMAT_RGBA5551,
> > +	DRM_FORMAT_BGRA5551,
> > +	DRM_FORMAT_RGB565,
> > +	DRM_FORMAT_BGR565,
> > +	DRM_FORMAT_RGB888,
> > +	DRM_FORMAT_BGR888,
> > +	DRM_FORMAT_XRGB8888,
> > +	DRM_FORMAT_XBGR8888,
> > +	DRM_FORMAT_RGBX8888,
> > +	DRM_FORMAT_BGRX8888,
> > +	DRM_FORMAT_ARGB8888,
> > +	DRM_FORMAT_ABGR8888,
> > +	DRM_FORMAT_RGBA8888,
> > +	DRM_FORMAT_BGRA8888,
> > +	DRM_FORMAT_XRGB2101010,
> > +	DRM_FORMAT_XBGR2101010,
> > +	DRM_FORMAT_RGBX1010102,
> > +	DRM_FORMAT_BGRX1010102,
> > +	DRM_FORMAT_ARGB2101010,
> > +	DRM_FORMAT_ABGR2101010,
> > +	DRM_FORMAT_RGBA1010102,
> > +	DRM_FORMAT_BGRA1010102,
> > +	DRM_FORMAT_YUYV,
> > +	DRM_FORMAT_YVYU,
> > +	DRM_FORMAT_UYVY,
> > +	DRM_FORMAT_VYUY,
> > +	DRM_FORMAT_AYUV,
> > +	DRM_FORMAT_NV12,
> > +	DRM_FORMAT_NV21,
> > +	DRM_FORMAT_NV16,
> > +	DRM_FORMAT_NV61,
> > +	DRM_FORMAT_NV24,
> > +	DRM_FORMAT_NV42,
> > +	DRM_FORMAT_YUV410,
> > +	DRM_FORMAT_YVU410,
> > +	DRM_FORMAT_YUV411,
> > +	DRM_FORMAT_YVU411,
> > +	DRM_FORMAT_YUV420,
> > +	DRM_FORMAT_YVU420,
> > +	DRM_FORMAT_YUV422,
> > +	DRM_FORMAT_YVU422,
> > +	DRM_FORMAT_YUV444,
> > +	DRM_FORMAT_YVU444,
> > +};
> > +
> >  /**
> >   * drm_get_subpixel_order_name - return a string for a given subpixel enum
> >   * @order: enum of subpixel_order
> > @@ -921,7 +989,7 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
> >  EXPORT_SYMBOL(drm_encoder_cleanup);
> >  
> >  /**
> > - * drm_plane_init - Initialise a new plane object
> > + * drm_plane_init - Initialise a new sprite plane object
> >   * @dev: DRM device
> >   * @plane: plane object to init
> >   * @possible_crtcs: bitmask of possible CRTCs
> > @@ -930,7 +998,9 @@ EXPORT_SYMBOL(drm_encoder_cleanup);
> >   * @format_count: number of elements in @formats
> >   * @priv: plane is private (hidden from userspace)?
> >   *
> > - * Inits a new object created as base part of a driver plane object.
> > + * Inits a new object created as base part of a driver plane object.  This
> > + * function should only be called for traditional sprite/overlay planes.
> > + * Primary planes should be initialized via @drm_plane_set_primary.
> >   *
> >   * RETURNS:
> >   * Zero on success, error code on failure.
> > @@ -984,6 +1054,74 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
> >  EXPORT_SYMBOL(drm_plane_init);
> >  
> >  /**
> > + * drm_plane_set_primary - Supply a primary plane for a CRTC
> > + * @dev DRM device
> > + * @plane: plane object representing the primary plane
> > + * @crtc: CRTC this plane is associated with
> > + * @funcs: callbacks for the new plane
> > + * @formats: array of supported formats (%DRM_FORMAT_*).  If NULL, the
> > + *           default list of formats traditionally supported by modesetting
> > + *           API's will be used and @format_count will be ignored.
> > + * @format_count: number of elements in @formats
> > + *
> > + * Provides a drm_plane representing a CRTC's primary plane.  This plane will
> > + * only be exposed to clients that set the DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES
> > + * capability bit.
> > + *
> > + * RETURNS:
> > + * Zero on success, error code on failure.
> > + */
> > +int drm_plane_set_primary(struct drm_device *dev, struct drm_plane *plane,
> > +			  struct drm_crtc *crtc,
> > +			  const struct drm_plane_funcs *funcs,
> > +			  const uint32_t *formats, uint32_t format_count)
> 
> The "priv" parameter of drm_plane_init() seems to be used by drivers to
> initialize those planes attached to the CRTC object (needs double
> checking). Could we refactor drm_plane_init() so drm_plane_set_primary()
> calls drm_plane_init() with priv=true (argument that could then renamed
> to primary).
> 
> > +{
> > +	int ret;
> > +
> > +	drm_modeset_lock_all(dev);
> > +
> > +	ret = drm_mode_object_get(dev, &plane->base, DRM_MODE_OBJECT_PLANE);
> > +	if (ret)
> > +		goto out;
> > +
> > +	if (formats == NULL) {
> > +		formats = default_primary_plane_formats;
> > +		format_count = ARRAY_SIZE(default_primary_plane_formats);
> > +	}
> 
> We're defining a new interface here, I don't think there's any value in
> providing a default set of format that is unlikely to match any
> hardware. I'd just enforce that drivers need to provide the list of
> acceptable formats.
> 
> > +
> > +	plane->base.properties = &plane->properties;
> > +	plane->dev = dev;
> > +	plane->funcs = funcs;
> > +	plane->format_types = kmalloc(sizeof(uint32_t) * format_count,
> > +				      GFP_KERNEL);
> > +	if (!plane->format_types) {
> > +		DRM_DEBUG_KMS("out of memory when setting up primary plane\n");
> > +		drm_mode_object_put(dev, &plane->base);
> > +		ret = -ENOMEM;
> > +		goto out;
> > +	}
> > +
> > +	memcpy(plane->format_types, formats, format_count * sizeof(uint32_t));
> > +	plane->format_count = format_count;
> > +	plane->possible_crtcs = drm_crtc_mask(crtc);
> > +
> > +	/*
> > +	 * Primary planes are not added to the general plane list, only linked
> > +	 * to their CRTC.  They will only be advertised to userspace clients
> > +	 * that indicate support.
> > +	 */
> > +	crtc->primary_plane = plane;
> > +	dev->mode_config.num_primary_plane++;
> > +	INIT_LIST_HEAD(&plane->head);
> > +
> > + out:
> > +	drm_modeset_unlock_all(dev);
> > +
> > +	return ret;
> > +}
> > +EXPORT_SYMBOL(drm_plane_set_primary);
> > +
> > +/**
> >   * drm_plane_cleanup - Clean up the core plane usage
> >   * @plane: plane to cleanup
> >   *
> > @@ -1836,8 +1974,10 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data,
> >  	struct drm_mode_get_plane_res *plane_resp = data;
> >  	struct drm_mode_config *config;
> >  	struct drm_plane *plane;
> > +	struct drm_crtc *crtc;
> >  	uint32_t __user *plane_ptr;
> >  	int copied = 0, ret = 0;
> > +	unsigned num_planes;
> >  
> >  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> >  		return -EINVAL;
> > @@ -1845,14 +1985,32 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data,
> >  	drm_modeset_lock_all(dev);
> >  	config = &dev->mode_config;
> >  
> > +	num_planes = config->num_plane;
> > +	if (file_priv->expose_primary_planes)
> > +		num_planes += config->num_primary_plane;
> > +
> >  	/*
> >  	 * This ioctl is called twice, once to determine how much space is
> >  	 * needed, and the 2nd time to fill it.
> >  	 */
> > -	if (config->num_plane &&
> > -	    (plane_resp->count_planes >= config->num_plane)) {
> > +	if (num_planes &&
> > +	    (plane_resp->count_planes >= num_planes)) {
> >  		plane_ptr = (uint32_t __user *)(unsigned long)plane_resp->plane_id_ptr;
> >  
> > +		if (file_priv->expose_primary_planes) {
> > +			list_for_each_entry(crtc, &config->crtc_list, head) {
> > +				if (!crtc->primary_plane)
> > +					continue;
> > +
> > +				if (put_user(crtc->primary_plane->base.id,
> > +					     plane_ptr + copied)) {
> > +					ret = -EFAULT;
> > +					goto out;
> > +				}
> > +				copied++;
> > +			}
> > +		}
> > +
> >  		list_for_each_entry(plane, &config->plane_list, head) {
> >  			if (put_user(plane->base.id, plane_ptr + copied)) {
> >  				ret = -EFAULT;
> > @@ -1861,7 +2019,7 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data,
> >  			copied++;
> >  		}
> >  	}
> > -	plane_resp->count_planes = config->num_plane;
> > +	plane_resp->count_planes = num_planes;
> >  
> >  out:
> >  	drm_modeset_unlock_all(dev);
> > diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> > index dffc836..03579d6 100644
> > --- a/drivers/gpu/drm/drm_ioctl.c
> > +++ b/drivers/gpu/drm/drm_ioctl.c
> > @@ -316,6 +316,11 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
> >  			return -EINVAL;
> >  		file_priv->stereo_allowed = req->value;
> >  		break;
> > +	case DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES:
> > +		if (req->value > 1)
> > +			return -EINVAL;
> > +		file_priv->expose_primary_planes = req->value;
> > +		break;
> >  	default:
> >  		return -EINVAL;
> >  	}
> > diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> > index 04a7f31..997fa29 100644
> > --- a/include/drm/drmP.h
> > +++ b/include/drm/drmP.h
> > @@ -437,6 +437,8 @@ struct drm_file {
> >  	unsigned is_master :1; /* this file private is a master for a minor */
> >  	/* true when the client has asked us to expose stereo 3D mode flags */
> >  	unsigned stereo_allowed :1;
> > +	/* true if client understands CRTC primary planes in the plane list */
> > +	unsigned expose_primary_planes:1;
> >  
> >  	struct pid *pid;
> >  	kuid_t uid;
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index ce9ee60..33a955b 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -429,6 +429,11 @@ struct drm_crtc {
> >  	 * by drm_mode_set_config_internal to implement correct refcounting. */
> >  	struct drm_framebuffer *old_fb;
> >  
> > +	/* Primary plane to expose to userspace if the client sets the 'expose
> > +	 * primary planes' cap.
> > +	 */
> > +	struct drm_plane *primary_plane;
> > +
> >  	bool enabled;
> >  
> >  	/* Requested mode from modesetting. */
> > @@ -859,6 +864,7 @@ struct drm_mode_config {
> >  	int num_plane;
> >  	struct list_head plane_list;
> >  
> > +	int num_primary_plane;
> >  	int num_crtc;
> >  	struct list_head crtc_list;
> >  
> > @@ -984,6 +990,11 @@ extern int drm_plane_init(struct drm_device *dev,
> >  			  const struct drm_plane_funcs *funcs,
> >  			  const uint32_t *formats, uint32_t format_count,
> >  			  bool priv);
> > +extern int drm_plane_set_primary(struct drm_device *dev,
> > +				 struct drm_plane *plane, struct drm_crtc *crtc,
> > +				 const struct drm_plane_funcs *funcs,
> > +				 const uint32_t *formats,
> > +				 uint32_t format_count);
> >  extern void drm_plane_cleanup(struct drm_plane *plane);
> >  extern void drm_plane_force_disable(struct drm_plane *plane);
> >  
> > diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > index 3c9a833..58a2468 100644
> > --- a/include/uapi/drm/drm.h
> > +++ b/include/uapi/drm/drm.h
> > @@ -635,6 +635,14 @@ struct drm_get_cap {
> >   */
> >  #define DRM_CLIENT_CAP_STEREO_3D	1
> >  
> > +/**
> > + * DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES
> > + *
> > + * If set to 1, the DRM core will expose CRTC primary planes along with
> > + * sprite/overlay planes.
> > + */
> > +#define DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES  2
> > +
> >  /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
> >  struct drm_set_client_cap {
> >  	__u64 capability;
> > -- 
> > 1.8.5.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Matt Roper
Graphics Software Engineer
ISG Platform Enabling & Development
Intel Corporation
(916) 356-2795

Re: [PATCH 1/4] drm: Add support for CRTC primary planes

[Damien Lespiau]

On Mon, Mar 03, 2014 at 09:45:53AM -0800, Matt Roper wrote:
> On Mon, Mar 03, 2014 at 03:47:43PM +0000, Damien Lespiau wrote:
> > On Thu, Feb 27, 2014 at 02:14:40PM -0800, Matt Roper wrote:
> > > Allow drivers to provide a drm_plane structure corresponding to a CRTC's
> > > primary plane.  These planes will be included in the plane list for any
> > > clients setting the DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES capability bit.
> > > 
> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > 
> > Two small notes here and comments inside:
> > 
> > - In term of new API enabling and to make the tree bisectable, one needs
> >   to 1/ do the preparation work 2/ enable the API. In this case, I would
> >   split up the patch to make the ioctl bits independent and the last
> >   patch of the series.
> > 
> > - I don't see a point in introducing the complexity to enable sprite and
> >   cursor planes independently from each other. So before enabling the
> >   new setcap() ioctl, could we also expose the cursor planes and call
> >   the capability "universal planes" or similar?
> 
> I'm not sure I follow what you're saying on this second point.  Sprites
> (or overlays) are already exposed to userspace, so existing clients
> expect anything they receive via drmModeGetPlaneResources() to behave in
> the traditional manner.  If we start throwing cursor planes into that
> list without hiding them behind a capability bit, existing userspace try
> to blindly use the cursors as full overlays without realizing that they
> may carry additional restrictions on size and such, which will lead to
> userspace breakage.

I mean, I don't see the point of having 2 separate calls to the SET_CAP
ioctl() to enable both primary and cursor planes. I think we should have
a single cap called "Universal planes" that expose both (which of course
means we need to do the same work for cursor plane before the ioctl
patch). This way we have fewer corner cases to care about.

-- 
Damien

Re: [PATCH 1/4] drm: Add support for CRTC primary planes

[David Herrmann]

Hi

On Thu, Feb 27, 2014 at 11:14 PM, Matt Roper <matthew.d.roper@intel.com> wrote:
> Allow drivers to provide a drm_plane structure corresponding to a CRTC's
> primary plane.  These planes will be included in the plane list for any
> clients setting the DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES capability bit.
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c  | 168 ++++++++++++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/drm_ioctl.c |   5 ++
>  include/drm/drmP.h          |   2 +
>  include/drm/drm_crtc.h      |  11 +++
>  include/uapi/drm/drm.h      |   8 +++
>  5 files changed, 189 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 35ea15d..21c6d4b 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -274,6 +274,74 @@ const char *drm_get_connector_status_name(enum drm_connector_status status)
>  }
>  EXPORT_SYMBOL(drm_get_connector_status_name);
>
> +/*
> + * Default set of pixel formats supported by primary planes.  This matches the
> + * set of formats accepted by the traditional modesetting interfaces; drivers
> + * need only provide their own format list if it differs from the default.
> + */
> +static const uint32_t default_primary_plane_formats[] = {
> +       DRM_FORMAT_C8,
> +       DRM_FORMAT_RGB332,
> +       DRM_FORMAT_BGR233,
> +       DRM_FORMAT_XRGB4444,
> +       DRM_FORMAT_XBGR4444,
> +       DRM_FORMAT_RGBX4444,
> +       DRM_FORMAT_BGRX4444,
> +       DRM_FORMAT_ARGB4444,
> +       DRM_FORMAT_ABGR4444,
> +       DRM_FORMAT_RGBA4444,
> +       DRM_FORMAT_BGRA4444,
> +       DRM_FORMAT_XRGB1555,
> +       DRM_FORMAT_XBGR1555,
> +       DRM_FORMAT_RGBX5551,
> +       DRM_FORMAT_BGRX5551,
> +       DRM_FORMAT_ARGB1555,
> +       DRM_FORMAT_ABGR1555,
> +       DRM_FORMAT_RGBA5551,
> +       DRM_FORMAT_BGRA5551,
> +       DRM_FORMAT_RGB565,
> +       DRM_FORMAT_BGR565,
> +       DRM_FORMAT_RGB888,
> +       DRM_FORMAT_BGR888,
> +       DRM_FORMAT_XRGB8888,
> +       DRM_FORMAT_XBGR8888,
> +       DRM_FORMAT_RGBX8888,
> +       DRM_FORMAT_BGRX8888,
> +       DRM_FORMAT_ARGB8888,
> +       DRM_FORMAT_ABGR8888,
> +       DRM_FORMAT_RGBA8888,
> +       DRM_FORMAT_BGRA8888,
> +       DRM_FORMAT_XRGB2101010,
> +       DRM_FORMAT_XBGR2101010,
> +       DRM_FORMAT_RGBX1010102,
> +       DRM_FORMAT_BGRX1010102,
> +       DRM_FORMAT_ARGB2101010,
> +       DRM_FORMAT_ABGR2101010,
> +       DRM_FORMAT_RGBA1010102,
> +       DRM_FORMAT_BGRA1010102,
> +       DRM_FORMAT_YUYV,
> +       DRM_FORMAT_YVYU,
> +       DRM_FORMAT_UYVY,
> +       DRM_FORMAT_VYUY,
> +       DRM_FORMAT_AYUV,
> +       DRM_FORMAT_NV12,
> +       DRM_FORMAT_NV21,
> +       DRM_FORMAT_NV16,
> +       DRM_FORMAT_NV61,
> +       DRM_FORMAT_NV24,
> +       DRM_FORMAT_NV42,
> +       DRM_FORMAT_YUV410,
> +       DRM_FORMAT_YVU410,
> +       DRM_FORMAT_YUV411,
> +       DRM_FORMAT_YVU411,
> +       DRM_FORMAT_YUV420,
> +       DRM_FORMAT_YVU420,
> +       DRM_FORMAT_YUV422,
> +       DRM_FORMAT_YVU422,
> +       DRM_FORMAT_YUV444,
> +       DRM_FORMAT_YVU444,
> +};
> +

As Damien said, I'd drop that list. I don't think we save much by
providing a default..

>  /**
>   * drm_get_subpixel_order_name - return a string for a given subpixel enum
>   * @order: enum of subpixel_order
> @@ -921,7 +989,7 @@ void drm_encoder_cleanup(struct drm_encoder *encoder)
>  EXPORT_SYMBOL(drm_encoder_cleanup);
>
>  /**
> - * drm_plane_init - Initialise a new plane object
> + * drm_plane_init - Initialise a new sprite plane object
>   * @dev: DRM device
>   * @plane: plane object to init
>   * @possible_crtcs: bitmask of possible CRTCs
> @@ -930,7 +998,9 @@ EXPORT_SYMBOL(drm_encoder_cleanup);
>   * @format_count: number of elements in @formats
>   * @priv: plane is private (hidden from userspace)?
>   *
> - * Inits a new object created as base part of a driver plane object.
> + * Inits a new object created as base part of a driver plane object.  This
> + * function should only be called for traditional sprite/overlay planes.
> + * Primary planes should be initialized via @drm_plane_set_primary.
>   *
>   * RETURNS:
>   * Zero on success, error code on failure.
> @@ -984,6 +1054,74 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
>  EXPORT_SYMBOL(drm_plane_init);
>
>  /**
> + * drm_plane_set_primary - Supply a primary plane for a CRTC
> + * @dev DRM device
> + * @plane: plane object representing the primary plane
> + * @crtc: CRTC this plane is associated with
> + * @funcs: callbacks for the new plane
> + * @formats: array of supported formats (%DRM_FORMAT_*).  If NULL, the
> + *           default list of formats traditionally supported by modesetting
> + *           API's will be used and @format_count will be ignored.
> + * @format_count: number of elements in @formats
> + *
> + * Provides a drm_plane representing a CRTC's primary plane.  This plane will
> + * only be exposed to clients that set the DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES
> + * capability bit.
> + *
> + * RETURNS:
> + * Zero on success, error code on failure.
> + */
> +int drm_plane_set_primary(struct drm_device *dev, struct drm_plane *plane,
> +                         struct drm_crtc *crtc,
> +                         const struct drm_plane_funcs *funcs,
> +                         const uint32_t *formats, uint32_t format_count)
> +{
> +       int ret;
> +
> +       drm_modeset_lock_all(dev);
> +
> +       ret = drm_mode_object_get(dev, &plane->base, DRM_MODE_OBJECT_PLANE);
> +       if (ret)
> +               goto out;
> +
> +       if (formats == NULL) {
> +               formats = default_primary_plane_formats;
> +               format_count = ARRAY_SIZE(default_primary_plane_formats);
> +       }
> +
> +       plane->base.properties = &plane->properties;
> +       plane->dev = dev;
> +       plane->funcs = funcs;
> +       plane->format_types = kmalloc(sizeof(uint32_t) * format_count,
> +                                     GFP_KERNEL);
> +       if (!plane->format_types) {
> +               DRM_DEBUG_KMS("out of memory when setting up primary plane\n");
> +               drm_mode_object_put(dev, &plane->base);
> +               ret = -ENOMEM;
> +               goto out;
> +       }
> +
> +       memcpy(plane->format_types, formats, format_count * sizeof(uint32_t));
> +       plane->format_count = format_count;
> +       plane->possible_crtcs = drm_crtc_mask(crtc);
> +
> +       /*
> +        * Primary planes are not added to the general plane list, only linked
> +        * to their CRTC.  They will only be advertised to userspace clients
> +        * that indicate support.
> +        */
> +       crtc->primary_plane = plane;
> +       dev->mode_config.num_primary_plane++;
> +       INIT_LIST_HEAD(&plane->head);
> +
> + out:
> +       drm_modeset_unlock_all(dev);
> +
> +       return ret;
> +}
> +EXPORT_SYMBOL(drm_plane_set_primary);
> +
> +/**
>   * drm_plane_cleanup - Clean up the core plane usage
>   * @plane: plane to cleanup
>   *
> @@ -1836,8 +1974,10 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data,
>         struct drm_mode_get_plane_res *plane_resp = data;
>         struct drm_mode_config *config;
>         struct drm_plane *plane;
> +       struct drm_crtc *crtc;
>         uint32_t __user *plane_ptr;
>         int copied = 0, ret = 0;
> +       unsigned num_planes;
>
>         if (!drm_core_check_feature(dev, DRIVER_MODESET))
>                 return -EINVAL;
> @@ -1845,14 +1985,32 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data,
>         drm_modeset_lock_all(dev);
>         config = &dev->mode_config;
>
> +       num_planes = config->num_plane;
> +       if (file_priv->expose_primary_planes)
> +               num_planes += config->num_primary_plane;
> +
>         /*
>          * This ioctl is called twice, once to determine how much space is
>          * needed, and the 2nd time to fill it.
>          */
> -       if (config->num_plane &&
> -           (plane_resp->count_planes >= config->num_plane)) {
> +       if (num_planes &&
> +           (plane_resp->count_planes >= num_planes)) {
>                 plane_ptr = (uint32_t __user *)(unsigned long)plane_resp->plane_id_ptr;
>
> +               if (file_priv->expose_primary_planes) {
> +                       list_for_each_entry(crtc, &config->crtc_list, head) {
> +                               if (!crtc->primary_plane)
> +                                       continue;
> +
> +                               if (put_user(crtc->primary_plane->base.id,
> +                                            plane_ptr + copied)) {
> +                                       ret = -EFAULT;
> +                                       goto out;
> +                               }
> +                               copied++;
> +                       }
> +               }
> +

How can user-space distinguish primary-planes from others? With
atomic-modesetting I can understand that there's no need to
distinguish them, but without it, we need to know which planes not to
use. Same problem for the possible_crtc field. The plane might work
with other CRTCs, but for legacy modesetting we don't care as we have
no API to use it.

I'd be nice to see somehow which primary plane is assigned to which
crtc. But maybe this all is just not intended to be used without
atomic-modesetting.

I like the proposal!
David

>                 list_for_each_entry(plane, &config->plane_list, head) {
>                         if (put_user(plane->base.id, plane_ptr + copied)) {
>                                 ret = -EFAULT;
> @@ -1861,7 +2019,7 @@ int drm_mode_getplane_res(struct drm_device *dev, void *data,
>                         copied++;
>                 }
>         }
> -       plane_resp->count_planes = config->num_plane;
> +       plane_resp->count_planes = num_planes;
>
>  out:
>         drm_modeset_unlock_all(dev);
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index dffc836..03579d6 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -316,6 +316,11 @@ drm_setclientcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
>                         return -EINVAL;
>                 file_priv->stereo_allowed = req->value;
>                 break;
> +       case DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES:
> +               if (req->value > 1)
> +                       return -EINVAL;
> +               file_priv->expose_primary_planes = req->value;
> +               break;
>         default:
>                 return -EINVAL;
>         }
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 04a7f31..997fa29 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -437,6 +437,8 @@ struct drm_file {
>         unsigned is_master :1; /* this file private is a master for a minor */
>         /* true when the client has asked us to expose stereo 3D mode flags */
>         unsigned stereo_allowed :1;
> +       /* true if client understands CRTC primary planes in the plane list */
> +       unsigned expose_primary_planes:1;
>
>         struct pid *pid;
>         kuid_t uid;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index ce9ee60..33a955b 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -429,6 +429,11 @@ struct drm_crtc {
>          * by drm_mode_set_config_internal to implement correct refcounting. */
>         struct drm_framebuffer *old_fb;
>
> +       /* Primary plane to expose to userspace if the client sets the 'expose
> +        * primary planes' cap.
> +        */
> +       struct drm_plane *primary_plane;
> +
>         bool enabled;
>
>         /* Requested mode from modesetting. */
> @@ -859,6 +864,7 @@ struct drm_mode_config {
>         int num_plane;
>         struct list_head plane_list;
>
> +       int num_primary_plane;
>         int num_crtc;
>         struct list_head crtc_list;
>
> @@ -984,6 +990,11 @@ extern int drm_plane_init(struct drm_device *dev,
>                           const struct drm_plane_funcs *funcs,
>                           const uint32_t *formats, uint32_t format_count,
>                           bool priv);
> +extern int drm_plane_set_primary(struct drm_device *dev,
> +                                struct drm_plane *plane, struct drm_crtc *crtc,
> +                                const struct drm_plane_funcs *funcs,
> +                                const uint32_t *formats,
> +                                uint32_t format_count);
>  extern void drm_plane_cleanup(struct drm_plane *plane);
>  extern void drm_plane_force_disable(struct drm_plane *plane);
>
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 3c9a833..58a2468 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -635,6 +635,14 @@ struct drm_get_cap {
>   */
>  #define DRM_CLIENT_CAP_STEREO_3D       1
>
> +/**
> + * DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES
> + *
> + * If set to 1, the DRM core will expose CRTC primary planes along with
> + * sprite/overlay planes.
> + */
> +#define DRM_CLIENT_CAP_EXPOSE_PRIMARY_PLANES  2
> +
>  /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
>  struct drm_set_client_cap {
>         __u64 capability;
> --
> 1.8.5.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/4] drm: Add support for CRTC primary planes

[Ville Syrjälä]

On Mon, Mar 03, 2014 at 07:24:04PM +0100, David Herrmann wrote:
> How can user-space distinguish primary-planes from others? With
> atomic-modesetting I can understand that there's no need to
> distinguish them, but without it, we need to know which planes not to
> use. Same problem for the possible_crtc field. The plane might work
> with other CRTCs, but for legacy modesetting we don't care as we have
> no API to use it.
> 
> I'd be nice to see somehow which primary plane is assigned to which
> crtc. But maybe this all is just not intended to be used without
> atomic-modesetting.
> 
> I like the proposal!

My take on this is that if the client has enable the new capbility,
it should not use the legacy cursor ioctls, nor should it specify
a framebuffer in the setcrtc ioctl. In fact I'd just return an error
if it does that.

But we still have the issue of figuring out what each plane can and
can't do. We need to expose that somehow, ideally in some form that's
not too hardware specific. So some kind of plane caps would be needed,
either as a separate ioctl or as read-only properties.

-- 
Ville Syrjälä
Intel OTC

[PATCH 2/4] drm: Add plane type property

[Matt Roper]

Add a plane type property to allow userspace to distinguish
sprite/overlay planes from primary planes.  In the future we may extend
this to cover cursor planes as well.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_crtc.c  | 32 ++++++++++++++++++++++++++++++++
 include/drm/drm_crtc.h      |  1 +
 include/uapi/drm/drm_mode.h |  3 +++
 3 files changed, 36 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 21c6d4b..1032eaf 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -114,6 +114,14 @@ static const struct drm_prop_enum_list drm_dpms_enum_list[] =
 
 DRM_ENUM_NAME_FN(drm_get_dpms_name, drm_dpms_enum_list)
 
+static const struct drm_prop_enum_list drm_plane_type_enum_list[] =
+{
+	{ DRM_MODE_PLANE_TYPE_SPRITE, "Sprite" },
+	{ DRM_MODE_PLANE_TYPE_PRIMARY, "Primary" },
+};
+
+DRM_ENUM_NAME_FN(drm_get_plane_type, drm_plane_type_enum_list)
+
 /*
  * Optional properties
  */
@@ -1046,6 +1054,10 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
 		INIT_LIST_HEAD(&plane->head);
 	}
 
+	drm_object_attach_property(&plane->base,
+				   dev->mode_config.plane_type_property,
+				   DRM_MODE_PLANE_TYPE_SPRITE);
+
  out:
 	drm_modeset_unlock_all(dev);
 
@@ -1114,6 +1126,10 @@ int drm_plane_set_primary(struct drm_device *dev, struct drm_plane *plane,
 	dev->mode_config.num_primary_plane++;
 	INIT_LIST_HEAD(&plane->head);
 
+	drm_object_attach_property(&plane->base,
+				   dev->mode_config.plane_type_property,
+				   DRM_MODE_PLANE_TYPE_PRIMARY);
+
  out:
 	drm_modeset_unlock_all(dev);
 
@@ -1236,6 +1252,21 @@ static int drm_mode_create_standard_connector_properties(struct drm_device *dev)
 	return 0;
 }
 
+static int drm_mode_create_standard_plane_properties(struct drm_device *dev)
+{
+	struct drm_property *type;
+
+	/*
+	 * Standard properties (apply to all planes)
+	 */
+	type = drm_property_create_enum(dev, 0,
+					"TYPE", drm_plane_type_enum_list,
+					ARRAY_SIZE(drm_plane_type_enum_list));
+	dev->mode_config.plane_type_property = type;
+
+	return 0;
+}
+
 /**
  * drm_mode_create_dvi_i_properties - create DVI-I specific connector properties
  * @dev: DRM device
@@ -4211,6 +4242,7 @@ void drm_mode_config_init(struct drm_device *dev)
 
 	drm_modeset_lock_all(dev);
 	drm_mode_create_standard_connector_properties(dev);
+	drm_mode_create_standard_plane_properties(dev);
 	drm_modeset_unlock_all(dev);
 
 	/* Just to be sure */
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 33a955b..d25cd9c 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -884,6 +884,7 @@ struct drm_mode_config {
 	struct list_head property_blob_list;
 	struct drm_property *edid_property;
 	struct drm_property *dpms_property;
+	struct drm_property *plane_type_property;
 
 	/* DVI-I properties */
 	struct drm_property *dvi_i_subconnector_property;
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index f104c26..c19705b 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -496,4 +496,7 @@ struct drm_mode_destroy_dumb {
 	uint32_t handle;
 };
 
+#define DRM_MODE_PLANE_TYPE_SPRITE  0
+#define DRM_MODE_PLANE_TYPE_PRIMARY 1
+
 #endif
-- 
1.8.5.1

Re: [PATCH 2/4] drm: Add plane type property

[Rob Clark]

On Thu, Feb 27, 2014 at 5:14 PM, Matt Roper <matthew.d.roper@intel.com> wrote:
> Add a plane type property to allow userspace to distinguish
> sprite/overlay planes from primary planes.  In the future we may extend
> this to cover cursor planes as well.
>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c  | 32 ++++++++++++++++++++++++++++++++
>  include/drm/drm_crtc.h      |  1 +
>  include/uapi/drm/drm_mode.h |  3 +++
>  3 files changed, 36 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 21c6d4b..1032eaf 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -114,6 +114,14 @@ static const struct drm_prop_enum_list drm_dpms_enum_list[] =
>
>  DRM_ENUM_NAME_FN(drm_get_dpms_name, drm_dpms_enum_list)
>
> +static const struct drm_prop_enum_list drm_plane_type_enum_list[] =
> +{
> +       { DRM_MODE_PLANE_TYPE_SPRITE, "Sprite" },

I'm not the *hugest* fan of using the name "sprite".. at least that
too me implies sort of a subset of possible functionality of a plane..

> +       { DRM_MODE_PLANE_TYPE_PRIMARY, "Primary" },
> +};
> +
> +DRM_ENUM_NAME_FN(drm_get_plane_type, drm_plane_type_enum_list)
> +
>  /*
>   * Optional properties
>   */
> @@ -1046,6 +1054,10 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
>                 INIT_LIST_HEAD(&plane->head);
>         }
>
> +       drm_object_attach_property(&plane->base,
> +                                  dev->mode_config.plane_type_property,
> +                                  DRM_MODE_PLANE_TYPE_SPRITE);
> +
>   out:
>         drm_modeset_unlock_all(dev);
>
> @@ -1114,6 +1126,10 @@ int drm_plane_set_primary(struct drm_device *dev, struct drm_plane *plane,


fwiw, this comment probably belongs in #1/4 but:

you probably don't need to introduce drm_plane_set_primary()..
instead you could just rename the 'bool priv' to 'bool prim'.  I think
there are just three drivers using primary planes..  I'm not 100% sure
about exynos, but both omap and msm, the private plane == primary
plane.  At least it was the intention to morph that into primary
planes.

Anyways, other than that I like the patchset.  Hopefully I should get
to rebasing the atomic patches real soon now, so I'll try rebasing on
top of this and see how it goes.

BR,
-R


>         dev->mode_config.num_primary_plane++;
>         INIT_LIST_HEAD(&plane->head);
>
> +       drm_object_attach_property(&plane->base,
> +                                  dev->mode_config.plane_type_property,
> +                                  DRM_MODE_PLANE_TYPE_PRIMARY);
> +
>   out:
>         drm_modeset_unlock_all(dev);
>
> @@ -1236,6 +1252,21 @@ static int drm_mode_create_standard_connector_properties(struct drm_device *dev)
>         return 0;
>  }
>
> +static int drm_mode_create_standard_plane_properties(struct drm_device *dev)
> +{
> +       struct drm_property *type;
> +
> +       /*
> +        * Standard properties (apply to all planes)
> +        */
> +       type = drm_property_create_enum(dev, 0,
> +                                       "TYPE", drm_plane_type_enum_list,
> +                                       ARRAY_SIZE(drm_plane_type_enum_list));
> +       dev->mode_config.plane_type_property = type;
> +
> +       return 0;
> +}
> +
>  /**
>   * drm_mode_create_dvi_i_properties - create DVI-I specific connector properties
>   * @dev: DRM device
> @@ -4211,6 +4242,7 @@ void drm_mode_config_init(struct drm_device *dev)
>
>         drm_modeset_lock_all(dev);
>         drm_mode_create_standard_connector_properties(dev);
> +       drm_mode_create_standard_plane_properties(dev);
>         drm_modeset_unlock_all(dev);
>
>         /* Just to be sure */
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 33a955b..d25cd9c 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -884,6 +884,7 @@ struct drm_mode_config {
>         struct list_head property_blob_list;
>         struct drm_property *edid_property;
>         struct drm_property *dpms_property;
> +       struct drm_property *plane_type_property;
>
>         /* DVI-I properties */
>         struct drm_property *dvi_i_subconnector_property;
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index f104c26..c19705b 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -496,4 +496,7 @@ struct drm_mode_destroy_dumb {
>         uint32_t handle;
>  };
>
> +#define DRM_MODE_PLANE_TYPE_SPRITE  0
> +#define DRM_MODE_PLANE_TYPE_PRIMARY 1
> +
>  #endif
> --
> 1.8.5.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] drm: Add plane type property

[Matt Roper]

On Thu, Feb 27, 2014 at 05:39:00PM -0500, Rob Clark wrote:
> On Thu, Feb 27, 2014 at 5:14 PM, Matt Roper <matthew.d.roper@intel.com> wrote:
> > Add a plane type property to allow userspace to distinguish
> > sprite/overlay planes from primary planes.  In the future we may extend
> > this to cover cursor planes as well.
> >
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  drivers/gpu/drm/drm_crtc.c  | 32 ++++++++++++++++++++++++++++++++
> >  include/drm/drm_crtc.h      |  1 +
> >  include/uapi/drm/drm_mode.h |  3 +++
> >  3 files changed, 36 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 21c6d4b..1032eaf 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -114,6 +114,14 @@ static const struct drm_prop_enum_list drm_dpms_enum_list[] =
> >
> >  DRM_ENUM_NAME_FN(drm_get_dpms_name, drm_dpms_enum_list)
> >
> > +static const struct drm_prop_enum_list drm_plane_type_enum_list[] =
> > +{
> > +       { DRM_MODE_PLANE_TYPE_SPRITE, "Sprite" },
> 
> I'm not the *hugest* fan of using the name "sprite".. at least that
> too me implies sort of a subset of possible functionality of a plane..

Any suggestions on a better name?  Maybe call them "traditional" planes
and then just give new names to the other types (primary, cursor) that
we wind up exposing when appropriate client caps are set?

> 
> > +       { DRM_MODE_PLANE_TYPE_PRIMARY, "Primary" },
> > +};
> > +
> > +DRM_ENUM_NAME_FN(drm_get_plane_type, drm_plane_type_enum_list)
> > +
> >  /*
> >   * Optional properties
> >   */
> > @@ -1046,6 +1054,10 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
> >                 INIT_LIST_HEAD(&plane->head);
> >         }
> >
> > +       drm_object_attach_property(&plane->base,
> > +                                  dev->mode_config.plane_type_property,
> > +                                  DRM_MODE_PLANE_TYPE_SPRITE);
> > +
> >   out:
> >         drm_modeset_unlock_all(dev);
> >
> > @@ -1114,6 +1126,10 @@ int drm_plane_set_primary(struct drm_device *dev, struct drm_plane *plane,
> 
> 
> fwiw, this comment probably belongs in #1/4 but:
> 
> you probably don't need to introduce drm_plane_set_primary()..
> instead you could just rename the 'bool priv' to 'bool prim'.  I think
> there are just three drivers using primary planes..  I'm not 100% sure
> about exynos, but both omap and msm, the private plane == primary
> plane.  At least it was the intention to morph that into primary
> planes.

I'd like to handle cursors with this eventually as well, so I'm not sure
whether just changing the meaning of priv by itself will get us
everything we need.  It seems like we probably need to provide a whole
lot more information about the capabilities and limitations of each
plane at drm_plane_init() and then expose those all as plane
properties so that userspace knows what it can and can't do.  In theory
we could expose cursor planes exactly the same way we expose
"traditional" planes today as long as we made sufficient plane
properties available to userspace to describe the min/max size
limitations and such.

> 
> Anyways, other than that I like the patchset.  Hopefully I should get
> to rebasing the atomic patches real soon now, so I'll try rebasing on
> top of this and see how it goes.
> 
> BR,
> -R

Sounds good; thanks for the review.


Matt

-- 
Matt Roper
Graphics Software Engineer
ISG Platform Enabling & Development
Intel Corporation
(916) 356-2795

Re: [PATCH 2/4] drm: Add plane type property

[Rob Clark]

On Thu, Feb 27, 2014 at 6:24 PM, Matt Roper <matthew.d.roper@intel.com> wrote:
> On Thu, Feb 27, 2014 at 05:39:00PM -0500, Rob Clark wrote:
>> On Thu, Feb 27, 2014 at 5:14 PM, Matt Roper <matthew.d.roper@intel.com> wrote:
>> > Add a plane type property to allow userspace to distinguish
>> > sprite/overlay planes from primary planes.  In the future we may extend
>> > this to cover cursor planes as well.
>> >
>> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>> > ---
>> >  drivers/gpu/drm/drm_crtc.c  | 32 ++++++++++++++++++++++++++++++++
>> >  include/drm/drm_crtc.h      |  1 +
>> >  include/uapi/drm/drm_mode.h |  3 +++
>> >  3 files changed, 36 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> > index 21c6d4b..1032eaf 100644
>> > --- a/drivers/gpu/drm/drm_crtc.c
>> > +++ b/drivers/gpu/drm/drm_crtc.c
>> > @@ -114,6 +114,14 @@ static const struct drm_prop_enum_list drm_dpms_enum_list[] =
>> >
>> >  DRM_ENUM_NAME_FN(drm_get_dpms_name, drm_dpms_enum_list)
>> >
>> > +static const struct drm_prop_enum_list drm_plane_type_enum_list[] =
>> > +{
>> > +       { DRM_MODE_PLANE_TYPE_SPRITE, "Sprite" },
>>
>> I'm not the *hugest* fan of using the name "sprite".. at least that
>> too me implies sort of a subset of possible functionality of a plane..
>
> Any suggestions on a better name?  Maybe call them "traditional" planes
> and then just give new names to the other types (primary, cursor) that
> we wind up exposing when appropriate client caps are set?

Maybe just "overlay"?  I'm not sure, I was hoping that by mentioning
it, that would trigger an idea in someone ;-)

>>
>> > +       { DRM_MODE_PLANE_TYPE_PRIMARY, "Primary" },
>> > +};
>> > +
>> > +DRM_ENUM_NAME_FN(drm_get_plane_type, drm_plane_type_enum_list)
>> > +
>> >  /*
>> >   * Optional properties
>> >   */
>> > @@ -1046,6 +1054,10 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
>> >                 INIT_LIST_HEAD(&plane->head);
>> >         }
>> >
>> > +       drm_object_attach_property(&plane->base,
>> > +                                  dev->mode_config.plane_type_property,
>> > +                                  DRM_MODE_PLANE_TYPE_SPRITE);
>> > +
>> >   out:
>> >         drm_modeset_unlock_all(dev);
>> >
>> > @@ -1114,6 +1126,10 @@ int drm_plane_set_primary(struct drm_device *dev, struct drm_plane *plane,
>>
>>
>> fwiw, this comment probably belongs in #1/4 but:
>>
>> you probably don't need to introduce drm_plane_set_primary()..
>> instead you could just rename the 'bool priv' to 'bool prim'.  I think
>> there are just three drivers using primary planes..  I'm not 100% sure
>> about exynos, but both omap and msm, the private plane == primary
>> plane.  At least it was the intention to morph that into primary
>> planes.
>
> I'd like to handle cursors with this eventually as well, so I'm not sure
> whether just changing the meaning of priv by itself will get us
> everything we need.  It seems like we probably need to provide a whole
> lot more information about the capabilities and limitations of each
> plane at drm_plane_init() and then expose those all as plane
> properties so that userspace knows what it can and can't do.  In theory
> we could expose cursor planes exactly the same way we expose
> "traditional" planes today as long as we made sufficient plane
> properties available to userspace to describe the min/max size
> limitations and such.

We could also just go the opposite direction, ie. keep _set_primary()
and drop the 'priv' arg.. I don't really mind too much either way, but
the 'private' plane stuff was intended to eventually be exposed to
userspace..  so if we call it primary now (which is a much better
name, IMO), we should clean out the remaining references to 'private'.

BR,
-R

>>
>> Anyways, other than that I like the patchset.  Hopefully I should get
>> to rebasing the atomic patches real soon now, so I'll try rebasing on
>> top of this and see how it goes.
>>
>> BR,
>> -R
>
> Sounds good; thanks for the review.
>
>
> Matt
>
> --
> Matt Roper
> Graphics Software Engineer
> ISG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

Re: [PATCH 2/4] drm: Add plane type property

[Damien Lespiau]

On Thu, Feb 27, 2014 at 11:03:07PM -0500, Rob Clark wrote:
> >> > @@ -1114,6 +1126,10 @@ int drm_plane_set_primary(struct drm_device *dev, struct drm_plane *plane,
> >>
> >>
> >> fwiw, this comment probably belongs in #1/4 but:
> >>
> >> you probably don't need to introduce drm_plane_set_primary()..
> >> instead you could just rename the 'bool priv' to 'bool prim'.  I think
> >> there are just three drivers using primary planes..  I'm not 100% sure
> >> about exynos, but both omap and msm, the private plane == primary
> >> plane.  At least it was the intention to morph that into primary
> >> planes.
> >
> > I'd like to handle cursors with this eventually as well, so I'm not sure
> > whether just changing the meaning of priv by itself will get us
> > everything we need.  It seems like we probably need to provide a whole
> > lot more information about the capabilities and limitations of each
> > plane at drm_plane_init() and then expose those all as plane
> > properties so that userspace knows what it can and can't do.  In theory
> > we could expose cursor planes exactly the same way we expose
> > "traditional" planes today as long as we made sufficient plane
> > properties available to userspace to describe the min/max size
> > limitations and such.
> 
> We could also just go the opposite direction, ie. keep _set_primary()
> and drop the 'priv' arg.. I don't really mind too much either way, but
> the 'private' plane stuff was intended to eventually be exposed to
> userspace..  so if we call it primary now (which is a much better
> name, IMO), we should clean out the remaining references to 'private'.

Ah, I had the same comment in patch 1/4. Why not have drm_init_plane()
take the type of plane as the last argument then? (instead of bool
primary, just the plane_type enum, primary, "sprite", cursor).

-- 
Damien

Re: [PATCH 2/4] drm: Add plane type property

[Daniel Vetter]

On Thu, Feb 27, 2014 at 03:24:08PM -0800, Matt Roper wrote:
> On Thu, Feb 27, 2014 at 05:39:00PM -0500, Rob Clark wrote:
> > On Thu, Feb 27, 2014 at 5:14 PM, Matt Roper <matthew.d.roper@intel.com> wrote:
> > > Add a plane type property to allow userspace to distinguish
> > > sprite/overlay planes from primary planes.  In the future we may extend
> > > this to cover cursor planes as well.
> > >
> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_crtc.c  | 32 ++++++++++++++++++++++++++++++++
> > >  include/drm/drm_crtc.h      |  1 +
> > >  include/uapi/drm/drm_mode.h |  3 +++
> > >  3 files changed, 36 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > > index 21c6d4b..1032eaf 100644
> > > --- a/drivers/gpu/drm/drm_crtc.c
> > > +++ b/drivers/gpu/drm/drm_crtc.c
> > > @@ -114,6 +114,14 @@ static const struct drm_prop_enum_list drm_dpms_enum_list[] =
> > >
> > >  DRM_ENUM_NAME_FN(drm_get_dpms_name, drm_dpms_enum_list)
> > >
> > > +static const struct drm_prop_enum_list drm_plane_type_enum_list[] =
> > > +{
> > > +       { DRM_MODE_PLANE_TYPE_SPRITE, "Sprite" },
> > 
> > I'm not the *hugest* fan of using the name "sprite".. at least that
> > too me implies sort of a subset of possible functionality of a plane..
> 
> Any suggestions on a better name?  Maybe call them "traditional" planes
> and then just give new names to the other types (primary, cursor) that
> we wind up exposing when appropriate client caps are set?

What about "secondary" for any plane exposed which doesn't match one of
the special-purpose planes for backwards compat? We'd then have "primary"
(fixed to a crtc and used for legacy setCrtc/pageflips), "cursor" (again
fixed to a crtc for use by the legacy setcurso ioctl) and a pile of
secondary planes without special meaning attached to them.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 3/4] drm/i915: Rename similar plane functions to avoid confusion

[Matt Roper]

The name 'update_plane' was used both for the primary plane functions in
intel_display.c and the sprite/overlay functions in intel_sprite.c.
Rename the primary plane functions to 'update_primary_plane' to avoid
confusion.

On a similar note, intel_display.c already had a function called
intel_disable_primary_plane() that programs the hardware to disable a
pipe's primary plane.  When we hook up primary planes through the DRM
plane interface, one of the natural handler names will be
intel_primary_plane_disable(), which is very similar.  To avoid
confusion, rename the existing intel_disable_primary_plane() to
intel_disable_primary_hw_plane() to make the two names a little more
distinct.

Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  5 +++--
 drivers/gpu/drm/i915/intel_display.c | 40 +++++++++++++++++++++---------------
 2 files changed, 26 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8c64831..fc36e6c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -450,8 +450,9 @@ struct drm_i915_display_funcs {
 			  struct drm_framebuffer *fb,
 			  struct drm_i915_gem_object *obj,
 			  uint32_t flags);
-	int (*update_plane)(struct drm_crtc *crtc, struct drm_framebuffer *fb,
-			    int x, int y);
+	int (*update_primary_plane)(struct drm_crtc *crtc,
+				    struct drm_framebuffer *fb,
+				    int x, int y);
 	void (*hpd_irq_setup)(struct drm_device *dev);
 	/* clock updates for mode set */
 	/* cursor updates */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f19e6ea..9757010 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1905,15 +1905,15 @@ static void intel_enable_primary_plane(struct drm_i915_private *dev_priv,
 }
 
 /**
- * intel_disable_primary_plane - disable the primary plane
+ * intel_disable_primary_hw_plane - disable the primary hardware plane
  * @dev_priv: i915 private structure
  * @plane: plane to disable
  * @pipe: pipe consuming the data
  *
  * Disable @plane; should be an independent operation.
  */
-static void intel_disable_primary_plane(struct drm_i915_private *dev_priv,
-					enum plane plane, enum pipe pipe)
+static void intel_disable_primary_hw_plane(struct drm_i915_private *dev_priv,
+					   enum plane plane, enum pipe pipe)
 {
 	struct intel_crtc *intel_crtc =
 		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
@@ -2047,8 +2047,9 @@ unsigned long intel_gen4_compute_page_offset(int *x, int *y,
 	}
 }
 
-static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
-			     int x, int y)
+static int i9xx_update_primary_plane(struct drm_crtc *crtc,
+				     struct drm_framebuffer *fb,
+				     int x, int y)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -2147,8 +2148,9 @@ static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 	return 0;
 }
 
-static int ironlake_update_plane(struct drm_crtc *crtc,
-				 struct drm_framebuffer *fb, int x, int y)
+static int ironlake_update_primary_plane(struct drm_crtc *crtc,
+					 struct drm_framebuffer *fb,
+					 int x, int y)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -2252,7 +2254,7 @@ intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
 		dev_priv->display.disable_fbc(dev);
 	intel_increase_pllclock(crtc);
 
-	return dev_priv->display.update_plane(crtc, fb, x, y);
+	return dev_priv->display.update_primary_plane(crtc, fb, x, y);
 }
 
 void intel_display_handle_reset(struct drm_device *dev)
@@ -2292,7 +2294,7 @@ void intel_display_handle_reset(struct drm_device *dev)
 		 * a NULL crtc->fb.
 		 */
 		if (intel_crtc->active && crtc->fb)
-			dev_priv->display.update_plane(crtc, crtc->fb,
+			dev_priv->display.update_primary_plane(crtc, crtc->fb,
 						       crtc->x, crtc->y);
 		mutex_unlock(&crtc->mutex);
 	}
@@ -2385,7 +2387,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 		intel_crtc->config.pipe_src_h = adjusted_mode->crtc_vdisplay;
 	}
 
-	ret = dev_priv->display.update_plane(crtc, fb, x, y);
+	ret = dev_priv->display.update_primary_plane(crtc, fb, x, y);
 	if (ret) {
 		intel_unpin_fb_obj(to_intel_framebuffer(fb)->obj);
 		mutex_unlock(&dev->struct_mutex);
@@ -3653,7 +3655,7 @@ static void haswell_crtc_disable_planes(struct drm_crtc *crtc)
 
 	intel_crtc_update_cursor(crtc, false);
 	intel_disable_planes(crtc);
-	intel_disable_primary_plane(dev_priv, plane, pipe);
+	intel_disable_primary_hw_plane(dev_priv, plane, pipe);
 }
 
 /*
@@ -3781,7 +3783,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
 
 	intel_crtc_update_cursor(crtc, false);
 	intel_disable_planes(crtc);
-	intel_disable_primary_plane(dev_priv, plane, pipe);
+	intel_disable_primary_hw_plane(dev_priv, plane, pipe);
 
 	if (intel_crtc->config.has_pch_encoder)
 		intel_set_pch_fifo_underrun_reporting(dev, pipe, false);
@@ -4247,7 +4249,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	intel_crtc_dpms_overlay(intel_crtc, false);
 	intel_crtc_update_cursor(crtc, false);
 	intel_disable_planes(crtc);
-	intel_disable_primary_plane(dev_priv, plane, pipe);
+	intel_disable_primary_hw_plane(dev_priv, plane, pipe);
 
 	intel_set_cpu_fifo_underrun_reporting(dev, pipe, false);
 	intel_disable_pipe(dev_priv, pipe);
@@ -10719,28 +10721,32 @@ static void intel_init_display(struct drm_device *dev)
 		dev_priv->display.crtc_enable = haswell_crtc_enable;
 		dev_priv->display.crtc_disable = haswell_crtc_disable;
 		dev_priv->display.off = haswell_crtc_off;
-		dev_priv->display.update_plane = ironlake_update_plane;
+		dev_priv->display.update_primary_plane =
+			ironlake_update_primary_plane;
 	} else if (HAS_PCH_SPLIT(dev)) {
 		dev_priv->display.get_pipe_config = ironlake_get_pipe_config;
 		dev_priv->display.crtc_mode_set = ironlake_crtc_mode_set;
 		dev_priv->display.crtc_enable = ironlake_crtc_enable;
 		dev_priv->display.crtc_disable = ironlake_crtc_disable;
 		dev_priv->display.off = ironlake_crtc_off;
-		dev_priv->display.update_plane = ironlake_update_plane;
+		dev_priv->display.update_primary_plane =
+			ironlake_update_primary_plane;
 	} else if (IS_VALLEYVIEW(dev)) {
 		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
 		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
 		dev_priv->display.crtc_enable = valleyview_crtc_enable;
 		dev_priv->display.crtc_disable = i9xx_crtc_disable;
 		dev_priv->display.off = i9xx_crtc_off;
-		dev_priv->display.update_plane = i9xx_update_plane;
+		dev_priv->display.update_primary_plane =
+			i9xx_update_primary_plane;
 	} else {
 		dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
 		dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
 		dev_priv->display.crtc_enable = i9xx_crtc_enable;
 		dev_priv->display.crtc_disable = i9xx_crtc_disable;
 		dev_priv->display.off = i9xx_crtc_off;
-		dev_priv->display.update_plane = i9xx_update_plane;
+		dev_priv->display.update_primary_plane =
+			i9xx_update_primary_plane;
 	}
 
 	/* Returns the core display clock speed */
-- 
1.8.5.1

[PATCH 4/4] drm/i915: Register primary plane for each CRTC

[Matt Roper]

Create a primary plane at CRTC init and hook up handlers for the various
operations that may be performed on it.  The DRM core will only
advertise the primary planes to clients that set the appropriate
capability bit.

Since we're limited to the legacy plane operations at the moment
(SetPlane and such) this isn't terribly interesting yet; the plane
update handler will perform an MMIO flip of the display plane and the
disable handler will disable the CRTC.  Once we migrate more of the
plane and CRTC info over to properties in preparation for atomic/nuclear
operations, primary planes will be more useful.

Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 92 ++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9757010..d9a5cd5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8260,6 +8260,10 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
 
 	intel_crtc_cursor_set(crtc, NULL, 0, 0, 0);
 
+	drm_plane_cleanup(crtc->primary_plane);
+	kfree(crtc->primary_plane);
+	crtc->primary_plane = NULL;
+
 	drm_crtc_cleanup(crtc);
 
 	kfree(intel_crtc);
@@ -10272,17 +10276,105 @@ 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;
+
+	/* setplane API takes shifted source rectangle values; unshift them */
+	src_x >>= 16;
+	src_y >>= 16;
+	src_w >>= 16;
+	src_h >>= 16;
+
+	if (fb->width < 3 || fb->height < 3 || fb->pitches[0] > 16384) {
+		DRM_DEBUG_KMS("Unsuitable framebuffer for primary plane\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Current hardware can't reposition the primary plane or scale it
+	 * (although this could change in the future).  This means that we
+	 * don't actually need any of the destination (crtc) rectangle values,
+	 * or the source rectangle width/height; only the source x/y winds up
+	 * getting used for panning.  Nevertheless, let's sanity check the
+	 * incoming values to make sure userspace didn't think it could scale
+	 * or reposition this plane.
+	 */
+	if (crtc_w != crtc->mode.hdisplay || crtc_h != crtc->mode.vdisplay ||
+	    crtc_x != 0 || crtc_y != 0) {
+		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;
+	}
+
+	intel_pipe_set_base(crtc, src_x, src_y, fb);
+	dev_priv->display.crtc_enable(crtc);
+
+	return 0;
+}
+
+static int
+intel_primary_plane_disable(struct drm_plane *plane)
+{
+	struct drm_device *dev = plane->dev;
+	drm_i915_private_t *dev_priv = dev->dev_private;
+
+	if (!plane->fb)
+		return 0;
+
+	if (WARN_ON(!plane->crtc || plane->crtc->primary_plane != plane))
+		return -EINVAL;
+
+	dev_priv->display.crtc_disable(plane->crtc);
+
+	return 0;
+}
+
+static void intel_primary_plane_destroy(struct drm_plane *plane)
+{
+	/*
+	 * Since primary planes are never put on the mode_config plane list,
+	 * this entry point should never be called.  Primary plane cleanup
+	 * happens during CRTC destruction.
+	 */
+	BUG();
+}
+
+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 void intel_crtc_init(struct drm_device *dev, int pipe)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc;
+	struct drm_plane *primary_plane;
 	int i;
 
 	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
 	if (intel_crtc == NULL)
 		return;
 
+	primary_plane = kzalloc(sizeof(*primary_plane), GFP_KERNEL);
+	if (primary_plane == NULL) {
+		kfree(intel_crtc);
+		return;
+	}
+
 	drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs);
+	drm_plane_set_primary(dev, primary_plane, &intel_crtc->base,
+			      &intel_primary_plane_funcs, NULL, 0);
 
 	drm_mode_crtc_set_gamma_size(&intel_crtc->base, 256);
 	for (i = 0; i < 256; i++) {
-- 
1.8.5.1

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Register primary plane for each CRTC

[Ville Syrjälä]

On Thu, Feb 27, 2014 at 02:14:43PM -0800, Matt Roper wrote:
> Create a primary plane at CRTC init and hook up handlers for the various
> operations that may be performed on it.  The DRM core will only
> advertise the primary planes to clients that set the appropriate
> capability bit.
> 
> Since we're limited to the legacy plane operations at the moment
> (SetPlane and such) this isn't terribly interesting yet; the plane
> update handler will perform an MMIO flip of the display plane and the
> disable handler will disable the CRTC.  Once we migrate more of the
> plane and CRTC info over to properties in preparation for atomic/nuclear
> operations, primary planes will be more useful.
> 
> Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 92 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 92 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9757010..d9a5cd5 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8260,6 +8260,10 @@ static void intel_crtc_destroy(struct drm_crtc *crtc)
>  
>  	intel_crtc_cursor_set(crtc, NULL, 0, 0, 0);
>  
> +	drm_plane_cleanup(crtc->primary_plane);
> +	kfree(crtc->primary_plane);
> +	crtc->primary_plane = NULL;
> +
>  	drm_crtc_cleanup(crtc);
>  
>  	kfree(intel_crtc);
> @@ -10272,17 +10276,105 @@ 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;
> +
> +	/* setplane API takes shifted source rectangle values; unshift them */
> +	src_x >>= 16;
> +	src_y >>= 16;
> +	src_w >>= 16;
> +	src_h >>= 16;
> +
> +	if (fb->width < 3 || fb->height < 3 || fb->pitches[0] > 16384) {
> +		DRM_DEBUG_KMS("Unsuitable framebuffer for primary plane\n");
> +		return -EINVAL;
> +	}

These aren't quite right for all gens. Actually I think the primary plane
limits are genereally more relaxed than the sprite plane limits, so 
intel_framebuffer_init() should have already done the necessary checks,
at least as far the stride is concerned. So I'd just drop the stride
check. In the future I suppose we might need to add some extra checks
here too, but for now I think we should fine w/o them.

> +
> +	/*
> +	 * Current hardware can't reposition the primary plane or scale it
> +	 * (although this could change in the future).  This means that we
> +	 * don't actually need any of the destination (crtc) rectangle values,
> +	 * or the source rectangle width/height; only the source x/y winds up
> +	 * getting used for panning.  Nevertheless, let's sanity check the
> +	 * incoming values to make sure userspace didn't think it could scale
> +	 * or reposition this plane.
> +	 */
> +	if (crtc_w != crtc->mode.hdisplay || crtc_h != crtc->mode.vdisplay ||
> +	    crtc_x != 0 || crtc_y != 0) {
> +		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;
> +	}

I'd do the clipping anyway, and then check that the dst size matches the
pipe src size post clipping. Otherwise the behaviour is rather
inconsistent with the sprite planes.

> +
> +	intel_pipe_set_base(crtc, src_x, src_y, fb);

This can return an error.

> +	dev_priv->display.crtc_enable(crtc);

Shouldn't enable the entire pipe, just the plane.

> +
> +	return 0;
> +}
> +
> +static int
> +intel_primary_plane_disable(struct drm_plane *plane)
> +{
> +	struct drm_device *dev = plane->dev;
> +	drm_i915_private_t *dev_priv = dev->dev_private;
> +
> +	if (!plane->fb)
> +		return 0;
> +
> +	if (WARN_ON(!plane->crtc || plane->crtc->primary_plane != plane))
> +		return -EINVAL;
> +
> +	dev_priv->display.crtc_disable(plane->crtc);

This should just disable the plane, not the entire pipe.

> +
> +	return 0;
> +}
> +
> +static void intel_primary_plane_destroy(struct drm_plane *plane)
> +{
> +	/*
> +	 * Since primary planes are never put on the mode_config plane list,
> +	 * this entry point should never be called.  Primary plane cleanup
> +	 * happens during CRTC destruction.
> +	 */
> +	BUG();

Just leave the func pointer as NULL, you get a backtrace either way.

> +}
> +
> +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 void intel_crtc_init(struct drm_device *dev, int pipe)
>  {
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	struct intel_crtc *intel_crtc;
> +	struct drm_plane *primary_plane;
>  	int i;
>  
>  	intel_crtc = kzalloc(sizeof(*intel_crtc), GFP_KERNEL);
>  	if (intel_crtc == NULL)
>  		return;
>  
> +	primary_plane = kzalloc(sizeof(*primary_plane), GFP_KERNEL);
> +	if (primary_plane == NULL) {
> +		kfree(intel_crtc);
> +		return;
> +	}
> +
>  	drm_crtc_init(dev, &intel_crtc->base, &intel_crtc_funcs);
> +	drm_plane_set_primary(dev, primary_plane, &intel_crtc->base,
> +			      &intel_primary_plane_funcs, NULL, 0);
>  
>  	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

-- 
Ville Syrjälä
Intel OTC