public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH 1/2] drm: Refactor framebuffer creation to allow internal use
@ 2014-05-16 22:36 Matt Roper
  2014-05-16 22:36 ` [PATCH 2/2] drm: Refactor setplane " Matt Roper
  2014-05-19  7:17 ` [PATCH 1/2] drm: Refactor framebuffer creation " Daniel Vetter
  0 siblings, 2 replies; 4+ messages in thread
From: Matt Roper @ 2014-05-16 22:36 UTC (permalink / raw)
  To: intel-gfx, dri-devel

Refactor DRM framebuffer creation into a new function that returns a
struct drm_framebuffer directly.  The upcoming universal cursor support
will want to create framebuffers internally to wrap cursor buffers, so
we want to be able to share that framebuffer creation with the
drm_mode_addfb2 ioctl handler.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 64 +++++++++++++++++++++++++++-------------------
 1 file changed, 38 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index b6d6c04..1a1a5f4 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2796,56 +2796,39 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
 	return 0;
 }
 
-/**
- * drm_mode_addfb2 - add an FB to the graphics configuration
- * @dev: drm device for the ioctl
- * @data: data pointer for the ioctl
- * @file_priv: drm file for the ioctl call
- *
- * Add a new FB to the specified CRTC, given a user request with format. This is
- * the 2nd version of the addfb ioctl, which supports multi-planar framebuffers
- * and uses fourcc codes as pixel format specifiers.
- *
- * Called by the user via ioctl.
- *
- * Returns:
- * Zero on success, errno on failure.
- */
-int drm_mode_addfb2(struct drm_device *dev,
-		    void *data, struct drm_file *file_priv)
+static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev,
+							void *data,
+							struct drm_file *file_priv)
 {
 	struct drm_mode_fb_cmd2 *r = data;
 	struct drm_mode_config *config = &dev->mode_config;
 	struct drm_framebuffer *fb;
 	int ret;
 
-	if (!drm_core_check_feature(dev, DRIVER_MODESET))
-		return -EINVAL;
-
 	if (r->flags & ~DRM_MODE_FB_INTERLACED) {
 		DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r->flags);
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 	}
 
 	if ((config->min_width > r->width) || (r->width > config->max_width)) {
 		DRM_DEBUG_KMS("bad framebuffer width %d, should be >= %d && <= %d\n",
 			  r->width, config->min_width, config->max_width);
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 	}
 	if ((config->min_height > r->height) || (r->height > config->max_height)) {
 		DRM_DEBUG_KMS("bad framebuffer height %d, should be >= %d && <= %d\n",
 			  r->height, config->min_height, config->max_height);
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 	}
 
 	ret = framebuffer_check(r);
 	if (ret)
-		return ret;
+		return ERR_PTR(ret);
 
 	fb = dev->mode_config.funcs->fb_create(dev, file_priv, r);
 	if (IS_ERR(fb)) {
 		DRM_DEBUG_KMS("could not create framebuffer\n");
-		return PTR_ERR(fb);
+		return fb;
 	}
 
 	mutex_lock(&file_priv->fbs_lock);
@@ -2854,8 +2837,37 @@ int drm_mode_addfb2(struct drm_device *dev,
 	DRM_DEBUG_KMS("[FB:%d]\n", fb->base.id);
 	mutex_unlock(&file_priv->fbs_lock);
 
+	return fb;
+}
+
+/**
+ * drm_mode_addfb2 - add an FB to the graphics configuration
+ * @dev: drm device for the ioctl
+ * @data: data pointer for the ioctl
+ * @file_priv: drm file for the ioctl call
+ *
+ * Add a new FB to the specified CRTC, given a user request with format. This is
+ * the 2nd version of the addfb ioctl, which supports multi-planar framebuffers
+ * and uses fourcc codes as pixel format specifiers.
+ *
+ * Called by the user via ioctl.
+ *
+ * Returns:
+ * Zero on success, errno on failure.
+ */
+int drm_mode_addfb2(struct drm_device *dev,
+		    void *data, struct drm_file *file_priv)
+{
+	struct drm_framebuffer *fb;
 
-	return ret;
+	if (!drm_core_check_feature(dev, DRIVER_MODESET))
+		return -EINVAL;
+
+	fb = add_framebuffer_internal(dev, data, file_priv);
+	if (IS_ERR(fb))
+		return PTR_ERR(fb);
+
+	return 0;
 }
 
 /**
-- 
1.8.5.1

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH 2/2] drm: Refactor setplane to allow internal use
  2014-05-16 22:36 [PATCH 1/2] drm: Refactor framebuffer creation to allow internal use Matt Roper
@ 2014-05-16 22:36 ` Matt Roper
  2014-05-19  7:24   ` Daniel Vetter
  2014-05-19  7:17 ` [PATCH 1/2] drm: Refactor framebuffer creation " Daniel Vetter
  1 sibling, 1 reply; 4+ messages in thread
From: Matt Roper @ 2014-05-16 22:36 UTC (permalink / raw)
  To: intel-gfx, dri-devel

Refactor DRM setplane code into a new setplane_internal() function that
takes DRM objects directly as parameters rather than looking them up by
ID.  We'll use this in a future patch when we implement legacy cursor
ioctls on top of the universal plane interface.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 178 +++++++++++++++++++++++++--------------------
 1 file changed, 100 insertions(+), 78 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 1a1a5f4..201c317 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2075,48 +2075,39 @@ out:
 	return ret;
 }
 
-/**
- * drm_mode_setplane - configure a plane's configuration
- * @dev: DRM device
- * @data: ioctl data*
- * @file_priv: DRM file info
+/*
+ * setplane_internal - setplane handler for internal callers
  *
- * Set plane configuration, including placement, fb, scaling, and other factors.
- * Or pass a NULL fb to disable.
+ * Note that we assume an extra reference has already been taken on fb.  If the
+ * update fails, this reference will be dropped before return; if it succeeds,
+ * the previous framebuffer (if any) will be unreferenced instead.
  *
- * Returns:
- * Zero on success, errno on failure.
+ * src_{x,y,w,h} are provided in 16.16 fixed point format
  */
-int drm_mode_setplane(struct drm_device *dev, void *data,
-		      struct drm_file *file_priv)
+static int setplane_internal(struct drm_crtc *crtc,
+			     struct drm_plane *plane,
+			     struct drm_framebuffer *fb,
+			     int32_t crtc_x, int32_t crtc_y,
+			     uint32_t crtc_w, uint32_t crtc_h,
+			     /* src_{x,y,w,h} values are 16.16 fixed point */
+			     uint32_t src_x, uint32_t src_y,
+			     uint32_t src_w, uint32_t src_h)
 {
-	struct drm_mode_set_plane *plane_req = data;
-	struct drm_mode_object *obj;
-	struct drm_plane *plane;
-	struct drm_crtc *crtc;
-	struct drm_framebuffer *fb = NULL, *old_fb = NULL;
+	struct drm_device *dev = crtc->dev;
+	struct drm_framebuffer *old_fb = NULL;
 	int ret = 0;
 	unsigned int fb_width, fb_height;
 	int i;
 
-	if (!drm_core_check_feature(dev, DRIVER_MODESET))
-		return -EINVAL;
-
-	/*
-	 * First, find the plane, crtc, and fb objects.  If not available,
-	 * we don't bother to call the driver.
-	 */
-	obj = drm_mode_object_find(dev, plane_req->plane_id,
-				   DRM_MODE_OBJECT_PLANE);
-	if (!obj) {
-		DRM_DEBUG_KMS("Unknown plane ID %d\n",
-			      plane_req->plane_id);
-		return -ENOENT;
+	/* Check whether this plane is usable on this CRTC */
+	if (!(plane->possible_crtcs & drm_crtc_mask(crtc))) {
+		DRM_DEBUG_KMS("Invalid crtc for plane\n");
+		ret = -EINVAL;
+		goto out;
 	}
-	plane = obj_to_plane(obj);
 
 	/* No fb means shut it down */
-	if (!plane_req->fb_id) {
+	if (!fb) {
 		drm_modeset_lock_all(dev);
 		old_fb = plane->fb;
 		ret = plane->funcs->disable_plane(plane);
@@ -2130,31 +2121,6 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 		goto out;
 	}
 
-	obj = drm_mode_object_find(dev, plane_req->crtc_id,
-				   DRM_MODE_OBJECT_CRTC);
-	if (!obj) {
-		DRM_DEBUG_KMS("Unknown crtc ID %d\n",
-			      plane_req->crtc_id);
-		ret = -ENOENT;
-		goto out;
-	}
-	crtc = obj_to_crtc(obj);
-
-	/* Check whether this plane is usable on this CRTC */
-	if (!(plane->possible_crtcs & drm_crtc_mask(crtc))) {
-		DRM_DEBUG_KMS("Invalid crtc for plane\n");
-		ret = -EINVAL;
-		goto out;
-	}
-
-	fb = drm_framebuffer_lookup(dev, plane_req->fb_id);
-	if (!fb) {
-		DRM_DEBUG_KMS("Unknown framebuffer ID %d\n",
-			      plane_req->fb_id);
-		ret = -ENOENT;
-		goto out;
-	}
-
 	/* Check whether this plane supports the fb pixel format. */
 	for (i = 0; i < plane->format_count; i++)
 		if (fb->pixel_format == plane->format_types[i])
@@ -2170,32 +2136,27 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 	fb_height = fb->height << 16;
 
 	/* Make sure source coordinates are inside the fb. */
-	if (plane_req->src_w > fb_width ||
-	    plane_req->src_x > fb_width - plane_req->src_w ||
-	    plane_req->src_h > fb_height ||
-	    plane_req->src_y > fb_height - plane_req->src_h) {
+	if (src_w > fb_width ||
+	    src_x > fb_width - src_w ||
+	    src_h > fb_height ||
+	    src_y > fb_height - src_h) {
 		DRM_DEBUG_KMS("Invalid source coordinates "
 			      "%u.%06ux%u.%06u+%u.%06u+%u.%06u\n",
-			      plane_req->src_w >> 16,
-			      ((plane_req->src_w & 0xffff) * 15625) >> 10,
-			      plane_req->src_h >> 16,
-			      ((plane_req->src_h & 0xffff) * 15625) >> 10,
-			      plane_req->src_x >> 16,
-			      ((plane_req->src_x & 0xffff) * 15625) >> 10,
-			      plane_req->src_y >> 16,
-			      ((plane_req->src_y & 0xffff) * 15625) >> 10);
+			      src_w >> 16, ((src_w & 0xffff) * 15625) >> 10,
+			      src_h >> 16, ((src_h & 0xffff) * 15625) >> 10,
+			      src_x >> 16, ((src_x & 0xffff) * 15625) >> 10,
+			      src_y >> 16, ((src_y & 0xffff) * 15625) >> 10);
 		ret = -ENOSPC;
 		goto out;
 	}
 
 	/* Give drivers some help against integer overflows */
-	if (plane_req->crtc_w > INT_MAX ||
-	    plane_req->crtc_x > INT_MAX - (int32_t) plane_req->crtc_w ||
-	    plane_req->crtc_h > INT_MAX ||
-	    plane_req->crtc_y > INT_MAX - (int32_t) plane_req->crtc_h) {
+	if (crtc_w > INT_MAX ||
+	    crtc_x > INT_MAX - (int32_t) crtc_w ||
+	    crtc_h > INT_MAX ||
+	    crtc_y > INT_MAX - (int32_t) crtc_h) {
 		DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n",
-			      plane_req->crtc_w, plane_req->crtc_h,
-			      plane_req->crtc_x, plane_req->crtc_y);
+			      crtc_w, crtc_h, crtc_x, crtc_y);
 		ret = -ERANGE;
 		goto out;
 	}
@@ -2203,10 +2164,8 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
 	drm_modeset_lock_all(dev);
 	old_fb = plane->fb;
 	ret = plane->funcs->update_plane(plane, crtc, fb,
-					 plane_req->crtc_x, plane_req->crtc_y,
-					 plane_req->crtc_w, plane_req->crtc_h,
-					 plane_req->src_x, plane_req->src_y,
-					 plane_req->src_w, plane_req->src_h);
+					 crtc_x, crtc_y, crtc_w, crtc_h,
+					 src_x, src_y, src_w, src_h);
 	if (!ret) {
 		plane->crtc = crtc;
 		plane->fb = fb;
@@ -2223,6 +2182,69 @@ out:
 		drm_framebuffer_unreference(old_fb);
 
 	return ret;
+
+}
+
+/**
+ * drm_mode_setplane - configure a plane's configuration
+ * @dev: DRM device
+ * @data: ioctl data*
+ * @file_priv: DRM file info
+ *
+ * Set plane configuration, including placement, fb, scaling, and other factors.
+ * Or pass a NULL fb to disable.
+ *
+ * Returns:
+ * Zero on success, errno on failure.
+ */
+int drm_mode_setplane(struct drm_device *dev, void *data,
+		      struct drm_file *file_priv)
+{
+	struct drm_mode_set_plane *plane_req = data;
+	struct drm_mode_object *obj;
+	struct drm_plane *plane;
+	struct drm_crtc *crtc;
+	struct drm_framebuffer *fb = NULL;
+
+	if (!drm_core_check_feature(dev, DRIVER_MODESET))
+		return -EINVAL;
+
+	/*
+	 * First, find the plane, crtc, and fb objects.  If not available,
+	 * we don't bother to call the driver.
+	 */
+	obj = drm_mode_object_find(dev, plane_req->plane_id,
+				   DRM_MODE_OBJECT_PLANE);
+	if (!obj) {
+		DRM_DEBUG_KMS("Unknown plane ID %d\n",
+			      plane_req->plane_id);
+		return -ENOENT;
+	}
+	plane = obj_to_plane(obj);
+
+	obj = drm_mode_object_find(dev, plane_req->crtc_id,
+				   DRM_MODE_OBJECT_CRTC);
+	if (!obj) {
+		DRM_DEBUG_KMS("Unknown crtc ID %d\n",
+			      plane_req->crtc_id);
+		return -ENOENT;
+	}
+	crtc = obj_to_crtc(obj);
+
+	if (plane_req->fb_id) {
+		fb = drm_framebuffer_lookup(dev, plane_req->fb_id);
+		if (!fb) {
+			DRM_DEBUG_KMS("Unknown framebuffer ID %d\n",
+				      plane_req->fb_id);
+			return -ENOENT;
+		}
+	}
+
+	return setplane_internal(crtc, plane, fb, plane_req->crtc_x,
+				 plane_req->crtc_y, plane_req->crtc_w,
+				 plane_req->crtc_h, plane_req->src_x,
+				 plane_req->src_y, plane_req->src_w,
+				 plane_req->src_h);
 }
 
 /**
-- 
1.8.5.1

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH 1/2] drm: Refactor framebuffer creation to allow internal use
  2014-05-16 22:36 [PATCH 1/2] drm: Refactor framebuffer creation to allow internal use Matt Roper
  2014-05-16 22:36 ` [PATCH 2/2] drm: Refactor setplane " Matt Roper
@ 2014-05-19  7:17 ` Daniel Vetter
  1 sibling, 0 replies; 4+ messages in thread
From: Daniel Vetter @ 2014-05-19  7:17 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx, dri-devel

On Fri, May 16, 2014 at 03:36:47PM -0700, Matt Roper wrote:
> Refactor DRM framebuffer creation into a new function that returns a
> struct drm_framebuffer directly.  The upcoming universal cursor support
> will want to create framebuffers internally to wrap cursor buffers, so
> we want to be able to share that framebuffer creation with the
> drm_mode_addfb2 ioctl handler.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 64 +++++++++++++++++++++++++++-------------------
>  1 file changed, 38 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index b6d6c04..1a1a5f4 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2796,56 +2796,39 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
>  	return 0;
>  }
>  
> -/**
> - * drm_mode_addfb2 - add an FB to the graphics configuration
> - * @dev: drm device for the ioctl
> - * @data: data pointer for the ioctl
> - * @file_priv: drm file for the ioctl call
> - *
> - * Add a new FB to the specified CRTC, given a user request with format. This is
> - * the 2nd version of the addfb ioctl, which supports multi-planar framebuffers
> - * and uses fourcc codes as pixel format specifiers.
> - *
> - * Called by the user via ioctl.
> - *
> - * Returns:
> - * Zero on success, errno on failure.
> - */
> -int drm_mode_addfb2(struct drm_device *dev,
> -		    void *data, struct drm_file *file_priv)
> +static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev,
> +							void *data,

s/void/struct drm_mode_fb_cmd2/

With that fixed this is Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +							struct drm_file *file_priv)
>  {
>  	struct drm_mode_fb_cmd2 *r = data;
>  	struct drm_mode_config *config = &dev->mode_config;
>  	struct drm_framebuffer *fb;
>  	int ret;
>  
> -	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> -		return -EINVAL;
> -
>  	if (r->flags & ~DRM_MODE_FB_INTERLACED) {
>  		DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r->flags);
> -		return -EINVAL;
> +		return ERR_PTR(-EINVAL);
>  	}
>  
>  	if ((config->min_width > r->width) || (r->width > config->max_width)) {
>  		DRM_DEBUG_KMS("bad framebuffer width %d, should be >= %d && <= %d\n",
>  			  r->width, config->min_width, config->max_width);
> -		return -EINVAL;
> +		return ERR_PTR(-EINVAL);
>  	}
>  	if ((config->min_height > r->height) || (r->height > config->max_height)) {
>  		DRM_DEBUG_KMS("bad framebuffer height %d, should be >= %d && <= %d\n",
>  			  r->height, config->min_height, config->max_height);
> -		return -EINVAL;
> +		return ERR_PTR(-EINVAL);
>  	}
>  
>  	ret = framebuffer_check(r);
>  	if (ret)
> -		return ret;
> +		return ERR_PTR(ret);
>  
>  	fb = dev->mode_config.funcs->fb_create(dev, file_priv, r);
>  	if (IS_ERR(fb)) {
>  		DRM_DEBUG_KMS("could not create framebuffer\n");
> -		return PTR_ERR(fb);
> +		return fb;
>  	}
>  
>  	mutex_lock(&file_priv->fbs_lock);
> @@ -2854,8 +2837,37 @@ int drm_mode_addfb2(struct drm_device *dev,
>  	DRM_DEBUG_KMS("[FB:%d]\n", fb->base.id);
>  	mutex_unlock(&file_priv->fbs_lock);
>  
> +	return fb;
> +}
> +
> +/**
> + * drm_mode_addfb2 - add an FB to the graphics configuration
> + * @dev: drm device for the ioctl
> + * @data: data pointer for the ioctl
> + * @file_priv: drm file for the ioctl call
> + *
> + * Add a new FB to the specified CRTC, given a user request with format. This is
> + * the 2nd version of the addfb ioctl, which supports multi-planar framebuffers
> + * and uses fourcc codes as pixel format specifiers.
> + *
> + * Called by the user via ioctl.
> + *
> + * Returns:
> + * Zero on success, errno on failure.
> + */
> +int drm_mode_addfb2(struct drm_device *dev,
> +		    void *data, struct drm_file *file_priv)
> +{
> +	struct drm_framebuffer *fb;
>  
> -	return ret;
> +	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> +		return -EINVAL;
> +
> +	fb = add_framebuffer_internal(dev, data, file_priv);
> +	if (IS_ERR(fb))
> +		return PTR_ERR(fb);
> +
> +	return 0;
>  }
>  
>  /**
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 2/2] drm: Refactor setplane to allow internal use
  2014-05-16 22:36 ` [PATCH 2/2] drm: Refactor setplane " Matt Roper
@ 2014-05-19  7:24   ` Daniel Vetter
  0 siblings, 0 replies; 4+ messages in thread
From: Daniel Vetter @ 2014-05-19  7:24 UTC (permalink / raw)
  To: Matt Roper; +Cc: intel-gfx, dri-devel

On Fri, May 16, 2014 at 03:36:48PM -0700, Matt Roper wrote:
> Refactor DRM setplane code into a new setplane_internal() function that
> takes DRM objects directly as parameters rather than looking them up by
> ID.  We'll use this in a future patch when we implement legacy cursor
> ioctls on top of the universal plane interface.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

A few small things below, with those addressed this is

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Aside: Disabling a plane doesn't require a valid crtc, I think we should
have an igt testcase for this.

> ---
>  drivers/gpu/drm/drm_crtc.c | 178 +++++++++++++++++++++++++--------------------
>  1 file changed, 100 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 1a1a5f4..201c317 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2075,48 +2075,39 @@ out:
>  	return ret;
>  }
>  
> -/**
> - * drm_mode_setplane - configure a plane's configuration
> - * @dev: DRM device
> - * @data: ioctl data*
> - * @file_priv: DRM file info
> +/*
> + * setplane_internal - setplane handler for internal callers
>   *
> - * Set plane configuration, including placement, fb, scaling, and other factors.
> - * Or pass a NULL fb to disable.
> + * Note that we assume an extra reference has already been taken on fb.  If the
> + * update fails, this reference will be dropped before return; if it succeeds,
> + * the previous framebuffer (if any) will be unreferenced instead.
>   *
> - * Returns:
> - * Zero on success, errno on failure.
> + * src_{x,y,w,h} are provided in 16.16 fixed point format
>   */
> -int drm_mode_setplane(struct drm_device *dev, void *data,
> -		      struct drm_file *file_priv)
> +static int setplane_internal(struct drm_crtc *crtc,
> +			     struct drm_plane *plane,
> +			     struct drm_framebuffer *fb,
> +			     int32_t crtc_x, int32_t crtc_y,
> +			     uint32_t crtc_w, uint32_t crtc_h,
> +			     /* src_{x,y,w,h} values are 16.16 fixed point */
> +			     uint32_t src_x, uint32_t src_y,
> +			     uint32_t src_w, uint32_t src_h)
>  {
> -	struct drm_mode_set_plane *plane_req = data;
> -	struct drm_mode_object *obj;
> -	struct drm_plane *plane;
> -	struct drm_crtc *crtc;
> -	struct drm_framebuffer *fb = NULL, *old_fb = NULL;
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_framebuffer *old_fb = NULL;
>  	int ret = 0;
>  	unsigned int fb_width, fb_height;
>  	int i;
>  
> -	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> -		return -EINVAL;
> -
> -	/*
> -	 * First, find the plane, crtc, and fb objects.  If not available,
> -	 * we don't bother to call the driver.
> -	 */
> -	obj = drm_mode_object_find(dev, plane_req->plane_id,
> -				   DRM_MODE_OBJECT_PLANE);
> -	if (!obj) {
> -		DRM_DEBUG_KMS("Unknown plane ID %d\n",
> -			      plane_req->plane_id);
> -		return -ENOENT;
> +	/* Check whether this plane is usable on this CRTC */
> +	if (!(plane->possible_crtcs & drm_crtc_mask(crtc))) {
> +		DRM_DEBUG_KMS("Invalid crtc for plane\n");
> +		ret = -EINVAL;
> +		goto out;
>  	}

Should we keep this check below the no fb case? For shutting off a plane
you don't need to spec a valid crtc afaik.

> -	plane = obj_to_plane(obj);
>  
>  	/* No fb means shut it down */
> -	if (!plane_req->fb_id) {
> +	if (!fb) {
>  		drm_modeset_lock_all(dev);
>  		old_fb = plane->fb;
>  		ret = plane->funcs->disable_plane(plane);
> @@ -2130,31 +2121,6 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  		goto out;
>  	}
>  
> -	obj = drm_mode_object_find(dev, plane_req->crtc_id,
> -				   DRM_MODE_OBJECT_CRTC);
> -	if (!obj) {
> -		DRM_DEBUG_KMS("Unknown crtc ID %d\n",
> -			      plane_req->crtc_id);
> -		ret = -ENOENT;
> -		goto out;
> -	}
> -	crtc = obj_to_crtc(obj);
> -
> -	/* Check whether this plane is usable on this CRTC */
> -	if (!(plane->possible_crtcs & drm_crtc_mask(crtc))) {
> -		DRM_DEBUG_KMS("Invalid crtc for plane\n");
> -		ret = -EINVAL;
> -		goto out;
> -	}
> -
> -	fb = drm_framebuffer_lookup(dev, plane_req->fb_id);
> -	if (!fb) {
> -		DRM_DEBUG_KMS("Unknown framebuffer ID %d\n",
> -			      plane_req->fb_id);
> -		ret = -ENOENT;
> -		goto out;
> -	}
> -
>  	/* Check whether this plane supports the fb pixel format. */
>  	for (i = 0; i < plane->format_count; i++)
>  		if (fb->pixel_format == plane->format_types[i])
> @@ -2170,32 +2136,27 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  	fb_height = fb->height << 16;
>  
>  	/* Make sure source coordinates are inside the fb. */
> -	if (plane_req->src_w > fb_width ||
> -	    plane_req->src_x > fb_width - plane_req->src_w ||
> -	    plane_req->src_h > fb_height ||
> -	    plane_req->src_y > fb_height - plane_req->src_h) {
> +	if (src_w > fb_width ||
> +	    src_x > fb_width - src_w ||
> +	    src_h > fb_height ||
> +	    src_y > fb_height - src_h) {
>  		DRM_DEBUG_KMS("Invalid source coordinates "
>  			      "%u.%06ux%u.%06u+%u.%06u+%u.%06u\n",
> -			      plane_req->src_w >> 16,
> -			      ((plane_req->src_w & 0xffff) * 15625) >> 10,
> -			      plane_req->src_h >> 16,
> -			      ((plane_req->src_h & 0xffff) * 15625) >> 10,
> -			      plane_req->src_x >> 16,
> -			      ((plane_req->src_x & 0xffff) * 15625) >> 10,
> -			      plane_req->src_y >> 16,
> -			      ((plane_req->src_y & 0xffff) * 15625) >> 10);
> +			      src_w >> 16, ((src_w & 0xffff) * 15625) >> 10,
> +			      src_h >> 16, ((src_h & 0xffff) * 15625) >> 10,
> +			      src_x >> 16, ((src_x & 0xffff) * 15625) >> 10,
> +			      src_y >> 16, ((src_y & 0xffff) * 15625) >> 10);
>  		ret = -ENOSPC;
>  		goto out;
>  	}
>  
>  	/* Give drivers some help against integer overflows */
> -	if (plane_req->crtc_w > INT_MAX ||
> -	    plane_req->crtc_x > INT_MAX - (int32_t) plane_req->crtc_w ||
> -	    plane_req->crtc_h > INT_MAX ||
> -	    plane_req->crtc_y > INT_MAX - (int32_t) plane_req->crtc_h) {
> +	if (crtc_w > INT_MAX ||
> +	    crtc_x > INT_MAX - (int32_t) crtc_w ||
> +	    crtc_h > INT_MAX ||
> +	    crtc_y > INT_MAX - (int32_t) crtc_h) {
>  		DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n",
> -			      plane_req->crtc_w, plane_req->crtc_h,
> -			      plane_req->crtc_x, plane_req->crtc_y);
> +			      crtc_w, crtc_h, crtc_x, crtc_y);
>  		ret = -ERANGE;
>  		goto out;
>  	}
> @@ -2203,10 +2164,8 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
>  	drm_modeset_lock_all(dev);
>  	old_fb = plane->fb;
>  	ret = plane->funcs->update_plane(plane, crtc, fb,
> -					 plane_req->crtc_x, plane_req->crtc_y,
> -					 plane_req->crtc_w, plane_req->crtc_h,
> -					 plane_req->src_x, plane_req->src_y,
> -					 plane_req->src_w, plane_req->src_h);
> +					 crtc_x, crtc_y, crtc_w, crtc_h,
> +					 src_x, src_y, src_w, src_h);
>  	if (!ret) {
>  		plane->crtc = crtc;
>  		plane->fb = fb;
> @@ -2223,6 +2182,69 @@ out:
>  		drm_framebuffer_unreference(old_fb);
>  
>  	return ret;
> +
> +}
> +
> +/**
> + * drm_mode_setplane - configure a plane's configuration
> + * @dev: DRM device
> + * @data: ioctl data*
> + * @file_priv: DRM file info
> + *
> + * Set plane configuration, including placement, fb, scaling, and other factors.
> + * Or pass a NULL fb to disable.
> + *
> + * Returns:
> + * Zero on success, errno on failure.
> + */
> +int drm_mode_setplane(struct drm_device *dev, void *data,
> +		      struct drm_file *file_priv)
> +{
> +	struct drm_mode_set_plane *plane_req = data;
> +	struct drm_mode_object *obj;
> +	struct drm_plane *plane;
> +	struct drm_crtc *crtc;
> +	struct drm_framebuffer *fb = NULL;
> +
> +	if (!drm_core_check_feature(dev, DRIVER_MODESET))
> +		return -EINVAL;
> +
> +	/*
> +	 * First, find the plane, crtc, and fb objects.  If not available,
> +	 * we don't bother to call the driver.
> +	 */
> +	obj = drm_mode_object_find(dev, plane_req->plane_id,
> +				   DRM_MODE_OBJECT_PLANE);
> +	if (!obj) {
> +		DRM_DEBUG_KMS("Unknown plane ID %d\n",
> +			      plane_req->plane_id);
> +		return -ENOENT;
> +	}
> +	plane = obj_to_plane(obj);
> +
> +	obj = drm_mode_object_find(dev, plane_req->crtc_id,
> +				   DRM_MODE_OBJECT_CRTC);
> +	if (!obj) {
> +		DRM_DEBUG_KMS("Unknown crtc ID %d\n",
> +			      plane_req->crtc_id);
> +		return -ENOENT;
> +	}
> +	crtc = obj_to_crtc(obj);

The crtc lookup should be moved into the "have fb" since for disabling a
plane we don't need a valid crtc.

> +
> +	if (plane_req->fb_id) {
> +		fb = drm_framebuffer_lookup(dev, plane_req->fb_id);
> +		if (!fb) {
> +			DRM_DEBUG_KMS("Unknown framebuffer ID %d\n",
> +				      plane_req->fb_id);
> +			return -ENOENT;
> +		}
> +	}
> +
> +	return setplane_internal(crtc, plane, fb, plane_req->crtc_x,

Bikeshed: I tend to split lines such that x/y and w/h pairs stay on the
same line. Bonus points if you match the line-breaking of the function
definition ;-)

> +				 plane_req->crtc_y, plane_req->crtc_w,
> +				 plane_req->crtc_h, plane_req->src_x,
> +				 plane_req->src_y, plane_req->src_w,
> +				 plane_req->src_h);
>  }
>  
>  /**
> -- 
> 1.8.5.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2014-05-19  7:24 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-05-16 22:36 [PATCH 1/2] drm: Refactor framebuffer creation to allow internal use Matt Roper
2014-05-16 22:36 ` [PATCH 2/2] drm: Refactor setplane " Matt Roper
2014-05-19  7:24   ` Daniel Vetter
2014-05-19  7:17 ` [PATCH 1/2] drm: Refactor framebuffer creation " Daniel Vetter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox