| Message ID | 1400279808-25979-2-git-send-email-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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); } /**
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(-)