This set has a bunch of fixes and additional checks on top of Jesse's drm plane series.
Jesse, feel free to squash some or all of these with your patches.
From: Ville Syrjälä ville.syrjala@linux.intel.com
Avoids having junk in offsets[] and pitches[] arrays.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_crtc.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 39cccb4..7058ed3 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1967,7 +1967,7 @@ int drm_mode_addfb(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_fb_cmd *or = data; - struct drm_mode_fb_cmd2 r; + struct drm_mode_fb_cmd2 r = {}; struct drm_mode_config *config = &dev->mode_config; struct drm_framebuffer *fb; int ret = 0;
On Fri, 11 Nov 2011 18:04:01 +0200 ville.syrjala@linux.intel.com wrote:
Yep, doesn't hurt...
Acked-by: Jesse Barnes jbarnes@virtuousgeek.org
From: Ville Syrjälä ville.syrjala@linux.intel.com
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_crtc.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 7058ed3..d3b884e 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -563,7 +563,7 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane, return -ENOMEM; }
- memcpy(plane->format_types, formats, format_count); + memcpy(plane->format_types, formats, sizeof(uint32_t) * format_count); plane->format_count = format_count; plane->possible_crtcs = possible_crtcs;
From: Ville Syrjälä ville.syrjala@linux.intel.com
These are the only indication to user space that the plane was disabled.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_crtc.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d3b884e..70f5747 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1677,6 +1677,8 @@ int drm_mode_setplane(struct drm_device *dev, void *data, /* No fb means shut it down */ if (!plane_req->fb_id) { plane->funcs->disable_plane(plane); + plane->crtc = NULL; + plane->fb = NULL; goto out; }
On Fri, 11 Nov 2011 18:04:03 +0200 ville.syrjala@linux.intel.com wrote:
A 0 return code should also indicate that the function succeeded, but this doesn't hurt.
Acked-by: Jesse Barnes jbarnes@virtuousgeek.org
From: Ville Syrjälä ville.syrjala@linux.intel.com
Make sure the source coordinates stay within the buffer.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_crtc.c | 23 +++++++++++++++++++++++ 1 files changed, 23 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 70f5747..098cc50 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1654,6 +1654,7 @@ int drm_mode_setplane(struct drm_device *dev, void *data, struct drm_crtc *crtc; struct drm_framebuffer *fb; int ret = 0; + unsigned int fb_width, fb_height;
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL; @@ -1702,6 +1703,28 @@ int drm_mode_setplane(struct drm_device *dev, void *data, } fb = obj_to_fb(obj);
+ fb_width = fb->width << 16; + 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) { + DRM_DEBUG_KMS("Invalid source coordinates " + "%01u.%06ux%01u.%06u+%01u.%06u+%01u.%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); + ret = -EINVAL; + goto out; + } + ret = plane->funcs->update_plane(plane, crtc, fb, plane_req->crtc_x, plane_req->crtc_y, plane_req->crtc_w, plane_req->crtc_h,
On Fri, 11 Nov 2011 18:04:04 +0200 ville.syrjala@linux.intel.com wrote:
Good sanity check (saves the drivers from having to do it), but I wonder if we can use a better return value like ENOSPC or something to make it easier for userspace to figure out.
On Fri, Nov 11, 2011 at 08:24:18AM -0800, Jesse Barnes wrote:
Yeah, getting EINVAL for every kind of failure is rather annoying. The only issue I have with ENOSPC is the strerror() output. It doesn't exactly fit this use case. But if there's nothing better I'm OK with ENOSPC.
On Fri, 11 Nov 2011 19:18:52 +0200 Ville Syrjälä ville.syrjala@linux.intel.com wrote:
Yeah, dunno what's best. Check out include/asm-generic/errno-base.h and errno.h; maybe you'll find a better fit.
From: Ville Syrjälä ville.syrjala@linux.intel.com
Help drivers a little by guaranteeing that crtc_x+crtc_w and crtc_y+crtc_h don't overflow.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_crtc.c | 12 ++++++++++++ 1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 098cc50..2410a9a 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1725,6 +1725,18 @@ int drm_mode_setplane(struct drm_device *dev, void *data, 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) { + 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); + ret = -EINVAL; + goto out; + } + ret = plane->funcs->update_plane(plane, crtc, fb, plane_req->crtc_x, plane_req->crtc_y, plane_req->crtc_w, plane_req->crtc_h,
On Fri, 11 Nov 2011 18:04:05 +0200 ville.syrjala@linux.intel.com wrote:
Not sure this helps much in practice, since the drivers will have to validate the target CRTC rect against the actual pipe dimensions anyway.
But it doesn't hurt either, so: Acked-by: Jesse Barnes jbarnes@virtuousgeek.org
On Fri, Nov 11, 2011 at 09:01:46AM -0800, Jesse Barnes wrote:
My master plan is that drivers would just stick these into a drm_region (introduced in my other patchset) and clip that to the pipe dimensions.
On Fri, 11 Nov 2011 19:11:39 +0200 Ville Syrjälä ville.syrjala@linux.intel.com wrote:
Ok that makes sense. The new patchset looks really nice; having helpers to support new chipsets and for regions should make things a lot cleaner.
From: Ville Syrjälä ville.syrjala@linux.intel.com
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_crtc.c | 2 +- include/drm/drm_crtc.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 2410a9a..27d46b1 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -548,7 +548,7 @@ EXPORT_SYMBOL(drm_encoder_cleanup); int drm_plane_init(struct drm_device *dev, struct drm_plane *plane, unsigned long possible_crtcs, const struct drm_plane_funcs *funcs, - uint32_t *formats, uint32_t format_count) + const uint32_t *formats, uint32_t format_count) { mutex_lock(&dev->mode_config.mutex);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 84db125..49dc288 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -727,7 +727,7 @@ extern int drm_plane_init(struct drm_device *dev, struct drm_plane *plane, unsigned long possible_crtcs, const struct drm_plane_funcs *funcs, - uint32_t *formats, uint32_t format_count); + const uint32_t *formats, uint32_t format_count); extern void drm_plane_cleanup(struct drm_plane *plane);
extern void drm_encoder_cleanup(struct drm_encoder *encoder);
On Fri, 11 Nov 2011 18:04:06 +0200 ville.syrjala@linux.intel.com wrote:
Yeah good fix. I made the structure const at ickle's request (once I stopped assigning it directly) but didn't update the prototype.
From: Ville Syrjälä ville.syrjala@linux.intel.com
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_crtc.c | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 27d46b1..04680bc 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1655,6 +1655,7 @@ int drm_mode_setplane(struct drm_device *dev, void *data, struct drm_framebuffer *fb; int ret = 0; unsigned int fb_width, fb_height; + int i;
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL; @@ -1703,6 +1704,16 @@ int drm_mode_setplane(struct drm_device *dev, void *data, } fb = obj_to_fb(obj);
+ /* 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]) + break; + if (i == plane->format_count) { + DRM_DEBUG_KMS("Invalid pixel format %x\n", fb->pixel_format); + ret = -EINVAL; + goto out; + } + fb_width = fb->width << 16; fb_height = fb->height << 16;
On Fri, 11 Nov 2011 18:04:07 +0200 ville.syrjala@linux.intel.com wrote:
Yeah it's reasonable to hoist this up into generic code.
Thanks,
dri-devel@lists.freedesktop.org
-
Jesse Barnes -
Ville Syrjälä -
ville.syrjala＠linux.intel.com