Mostly fixes for various bits and pieces that caught my eye while reading the mode setting code.
From: Ville Syrjälä ville.syrjala@linux.intel.com
When doing a mode set with the special fb id -1, reject the mode set if no fb is currently bound to the crtc.
Also remove the pointless list traversal to find the current crtc based on the current crtc :)
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_crtc.c | 14 ++++++-------- 1 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 6fdaf6f..bbcecdb 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1755,7 +1755,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, struct drm_mode_config *config = &dev->mode_config; struct drm_mode_crtc *crtc_req = data; struct drm_mode_object *obj; - struct drm_crtc *crtc, *crtcfb; + struct drm_crtc *crtc; struct drm_connector **connector_set = NULL, *connector; struct drm_framebuffer *fb = NULL; struct drm_display_mode *mode = NULL; @@ -1782,14 +1782,12 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, /* If we have a mode we need a framebuffer. */ /* If we pass -1, set the mode with the currently bound fb */ if (crtc_req->fb_id == -1) { - list_for_each_entry(crtcfb, - &dev->mode_config.crtc_list, head) { - if (crtcfb == crtc) { - DRM_DEBUG_KMS("Using current fb for " - "setmode\n"); - fb = crtc->fb; - } + if (!crtc->fb) { + DRM_DEBUG_KMS("CRTC doesn't have current FB\n"); + ret = -EINVAL; + goto out; } + fb = crtc->fb; } else { obj = drm_mode_object_find(dev, crtc_req->fb_id, DRM_MODE_OBJECT_FB);
From: Ville Syrjälä ville.syrjala@linux.intel.com
The drm_display_mode type is a bitmask so it should be unsigned.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- include/drm/drm_crtc.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 2a0872c..31715bd 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -121,7 +121,7 @@ struct drm_display_mode { char name[DRM_DISPLAY_MODE_LEN];
enum drm_mode_status status; - int type; + unsigned int type;
/* Proposed mode values */ int clock; /* in kHz */
From: Ville Syrjälä ville.syrjala@linux.intel.com
When converting from a drm_display_mode to drm_mode_modeinfo, print a warning if the the timings values don't fit into the __u16 datatype.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_crtc.c | 7 +++++++ 1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index bbcecdb..d11763f 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1002,6 +1002,13 @@ EXPORT_SYMBOL(drm_mode_config_cleanup); void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out, struct drm_display_mode *in) { + WARN(in->hdisplay > USHRT_MAX || in->hsync_start > USHRT_MAX || + in->hsync_end > USHRT_MAX || in->htotal > USHRT_MAX || + in->hskew > USHRT_MAX || in->vdisplay > USHRT_MAX || + in->vsync_start > USHRT_MAX || in->vsync_end > USHRT_MAX || + in->vtotal > USHRT_MAX || in->vscan > USHRT_MAX, + "timing values too large for mode info\n"); + out->clock = in->clock; out->hdisplay = in->hdisplay; out->hsync_start = in->hsync_start;
From: Ville Syrjälä ville.syrjala@linux.intel.com
The crtc x/y panning coordinates are stored as signed integers internally. The user provides them as unsigned, so we should check that the user provided values actually fit in the internal datatypes.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_crtc.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d11763f..3a42c9c 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1774,6 +1774,10 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;
+ /* For some reason crtc x/y offsets are signed internally. */ + if (crtc_req->x > INT_MAX || crtc_req->y > INT_MAX) + return -ERANGE; + mutex_lock(&dev->mode_config.mutex); obj = drm_mode_object_find(dev, crtc_req->crtc_id, DRM_MODE_OBJECT_CRTC);
From: Ville Syrjälä ville.syrjala@linux.intel.com
drm_mode_attachmode() always returns 0. Change the return type to void.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_crtc.c | 16 +++++----------- 1 files changed, 5 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 3a42c9c..d2e09d9 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -2379,21 +2379,17 @@ void drm_fb_release(struct drm_file *priv) * * Add @mode to @connector's user mode list. */ -static int drm_mode_attachmode(struct drm_device *dev, - struct drm_connector *connector, - struct drm_display_mode *mode) +static void drm_mode_attachmode(struct drm_device *dev, + struct drm_connector *connector, + struct drm_display_mode *mode) { - int ret = 0; - list_add_tail(&mode->head, &connector->user_modes); - return ret; }
int drm_mode_attachmode_crtc(struct drm_device *dev, struct drm_crtc *crtc, struct drm_display_mode *mode) { struct drm_connector *connector; - int ret = 0; struct drm_display_mode *dup_mode; int need_dup = 0; list_for_each_entry(connector, &dev->mode_config.connector_list, head) { @@ -2404,9 +2400,7 @@ int drm_mode_attachmode_crtc(struct drm_device *dev, struct drm_crtc *crtc, dup_mode = drm_mode_duplicate(dev, mode); else dup_mode = mode; - ret = drm_mode_attachmode(dev, connector, dup_mode); - if (ret) - return ret; + drm_mode_attachmode(dev, connector, dup_mode); need_dup = 1; } } @@ -2491,7 +2485,7 @@ int drm_mode_attachmode_ioctl(struct drm_device *dev,
drm_crtc_convert_umode(mode, umode);
- ret = drm_mode_attachmode(dev, connector, mode); + drm_mode_attachmode(dev, connector, mode); out: mutex_unlock(&dev->mode_config.mutex); return ret;
From: Ville Syrjälä ville.syrjala@linux.intel.com
The mode passed to the .set_config() hook was never freed. The drivers will make a copy of the mode, so simply free it when done.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_crtc.c | 9 +++++++++ 1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d2e09d9..9ccb92f 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -643,6 +643,9 @@ EXPORT_SYMBOL(drm_mode_create); */ void drm_mode_destroy(struct drm_device *dev, struct drm_display_mode *mode) { + if (!mode) + return; + drm_mode_object_put(dev, &mode->base);
kfree(mode); @@ -1812,6 +1815,11 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, }
mode = drm_mode_create(dev); + if (!mode) { + ret = -ENOMEM; + goto out; + } + drm_crtc_convert_umode(mode, &crtc_req->mode); drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V); } @@ -1881,6 +1889,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
out: kfree(connector_set); + drm_mode_destroy(dev, mode); mutex_unlock(&dev->mode_config.mutex); return ret; }
From: Ville Syrjälä ville.syrjala@linux.intel.com
The internal mode representation drm_display_mode uses signed data types. When converting the user mode to internal representation, check that the unsigned values don't overflow the signed datatypes.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_crtc.c | 33 ++++++++++++++++++++++++++++----- 1 files changed, 28 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 9ccb92f..4d9e69c 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1040,10 +1040,16 @@ void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out, * * Convert a drm_mode_modeinfo into a drm_display_mode structure to return to * the caller. + * + * RETURNS: + * Zero on success, errno on failure. */ -void drm_crtc_convert_umode(struct drm_display_mode *out, - struct drm_mode_modeinfo *in) +int drm_crtc_convert_umode(struct drm_display_mode *out, + struct drm_mode_modeinfo *in) { + if (in->clock > INT_MAX || in->vrefresh > INT_MAX) + return -ERANGE; + out->clock = in->clock; out->hdisplay = in->hdisplay; out->hsync_start = in->hsync_start; @@ -1060,6 +1066,8 @@ void drm_crtc_convert_umode(struct drm_display_mode *out, out->type = in->type; strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); out->name[DRM_DISPLAY_MODE_LEN-1] = 0; + + return 0; }
/** @@ -1820,7 +1828,12 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, goto out; }
- drm_crtc_convert_umode(mode, &crtc_req->mode); + ret = drm_crtc_convert_umode(mode, &crtc_req->mode); + if (ret) { + DRM_DEBUG_KMS("Invalid mode\n"); + goto out; + } + drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V); }
@@ -2492,7 +2505,12 @@ int drm_mode_attachmode_ioctl(struct drm_device *dev, goto out; }
- drm_crtc_convert_umode(mode, umode); + ret = drm_crtc_convert_umode(mode, umode); + if (ret) { + DRM_DEBUG_KMS("Invalid mode\n"); + drm_mode_destroy(dev, mode); + goto out; + }
drm_mode_attachmode(dev, connector, mode); out: @@ -2535,7 +2553,12 @@ int drm_mode_detachmode_ioctl(struct drm_device *dev, } connector = obj_to_connector(obj);
- drm_crtc_convert_umode(&mode, umode); + ret = drm_crtc_convert_umode(&mode, umode); + if (ret) { + DRM_DEBUG_KMS("Invalid mode\n"); + goto out; + } + ret = drm_mode_detachmode(dev, connector, &mode); out: mutex_unlock(&dev->mode_config.mutex);
From: Ville Syrjälä ville.syrjala@linux.intel.com
Make sure the requested CRTC viewport fits inside the framebuffer.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_crtc.c | 24 ++++++++++++++++++++++++ 1 files changed, 24 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 4d9e69c..3f5c603 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1835,6 +1835,18 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, }
drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V); + + if (mode->hdisplay > fb->width || + mode->vdisplay > fb->height || + crtc_req->x > fb->width - mode->hdisplay || + crtc_req->y > fb->height - mode->vdisplay) { + DRM_DEBUG_KMS("Invalid CRTC viewport %ux%u+%u+%u for fb size %ux%u.\n", + mode->hdisplay, mode->vdisplay, + crtc_req->x, crtc_req->y, + fb->width, fb->height); + ret = -ENOSPC; + goto out; + } }
if (crtc_req->count_connectors == 0 && mode) { @@ -3206,6 +3218,18 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, goto out; fb = obj_to_fb(obj);
+ if (crtc->mode.hdisplay > fb->width || + crtc->mode.vdisplay > fb->height || + crtc->x > fb->width - crtc->mode.hdisplay || + crtc->y > fb->height - crtc->mode.vdisplay) { + DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d.\n", + fb->width, fb->height, + crtc->mode.hdisplay, crtc->mode.vdisplay, + crtc->x, crtc->y); + ret = -ENOSPC; + goto out; + } + if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) { ret = -ENOMEM; spin_lock_irqsave(&dev->event_lock, flags);
From: Ville Syrjälä ville.syrjala@linux.intel.com
Change drm_mode_attachmode_crtc() to take an "all or nothing" approach. If an error is returned, there are no side effects visible.
Also change the function to always duplicate the mode passed in.
Also change the function to not give up when it finds the first connector without and encoder.
A simpler approach would be to just remove the function completely as it's unused currently.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_crtc.c | 38 +++++++++++++++++++++++++++----------- include/drm/drm_crtc.h | 2 +- 2 files changed, 28 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 3f5c603..37d34ad 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -2421,24 +2421,40 @@ static void drm_mode_attachmode(struct drm_device *dev, }
int drm_mode_attachmode_crtc(struct drm_device *dev, struct drm_crtc *crtc, - struct drm_display_mode *mode) + const struct drm_display_mode *mode) { struct drm_connector *connector; - struct drm_display_mode *dup_mode; - int need_dup = 0; + int ret = 0; + struct drm_display_mode *dup_mode, *next; + LIST_HEAD(list); + list_for_each_entry(connector, &dev->mode_config.connector_list, head) { if (!connector->encoder) - break; + continue; if (connector->encoder->crtc == crtc) { - if (need_dup) - dup_mode = drm_mode_duplicate(dev, mode); - else - dup_mode = mode; - drm_mode_attachmode(dev, connector, dup_mode); - need_dup = 1; + dup_mode = drm_mode_duplicate(dev, mode); + if (!dup_mode) { + ret = -ENOMEM; + goto out; + } + list_add_tail(&dup_mode->head, &list); } } - return 0; + + list_for_each_entry(connector, &dev->mode_config.connector_list, head) { + if (!connector->encoder) + continue; + if (connector->encoder->crtc == crtc) + list_move_tail(list.next, &connector->user_modes); + } + + WARN_ON(!list_empty(&list)); + + out: + list_for_each_entry_safe(dup_mode, next, &list, head) + drm_mode_destroy(dev, dup_mode); + + return ret; } EXPORT_SYMBOL(drm_mode_attachmode_crtc);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 31715bd..fe7ebc6 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -869,7 +869,7 @@ extern int drm_mode_height(struct drm_display_mode *mode); /* for us by fb module */ extern int drm_mode_attachmode_crtc(struct drm_device *dev, struct drm_crtc *crtc, - struct drm_display_mode *mode); + const struct drm_display_mode *mode); extern int drm_mode_detachmode_crtc(struct drm_device *dev, struct drm_display_mode *mode);
extern struct drm_display_mode *drm_mode_create(struct drm_device *dev);
From: Ville Syrjälä ville.syrjala@linux.intel.com
drm_crtc_convert_umode() and drm_crtc_convert_to_umode() are never used outside drm_crtc.c, so make them static. Also make the input mode structure const for both functions.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_crtc.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 37d34ad..e36bd43 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1002,8 +1002,8 @@ EXPORT_SYMBOL(drm_mode_config_cleanup); * Convert a drm_display_mode into a drm_mode_modeinfo structure to return to * the user. */ -void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out, - struct drm_display_mode *in) +static void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out, + const struct drm_display_mode *in) { WARN(in->hdisplay > USHRT_MAX || in->hsync_start > USHRT_MAX || in->hsync_end > USHRT_MAX || in->htotal > USHRT_MAX || @@ -1044,8 +1044,8 @@ void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out, * RETURNS: * Zero on success, errno on failure. */ -int drm_crtc_convert_umode(struct drm_display_mode *out, - struct drm_mode_modeinfo *in) +static int drm_crtc_convert_umode(struct drm_display_mode *out, + const struct drm_mode_modeinfo *in) { if (in->clock > INT_MAX || in->vrefresh > INT_MAX) return -ERANGE;
From: Ville Syrjälä ville.syrjala@linux.intel.com
Check drm_mode_object_get() return value everywhere.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_crtc.c | 95 ++++++++++++++++++++++++++++--------- drivers/gpu/drm/drm_crtc_helper.c | 2 + include/drm/drm_crtc.h | 22 ++++---- 3 files changed, 85 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index e36bd43..f5b098e 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -293,9 +293,8 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb, int ret;
ret = drm_mode_object_get(dev, &fb->base, DRM_MODE_OBJECT_FB); - if (ret) { + if (ret) return ret; - }
fb->dev = dev; fb->funcs = funcs; @@ -365,19 +364,31 @@ EXPORT_SYMBOL(drm_framebuffer_cleanup); * Caller must hold mode config lock. * * Inits a new object created as base part of an driver crtc object. + * + * RETURNS: + * Zero on success, error code on failure. */ -void drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, +int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, const struct drm_crtc_funcs *funcs) { + int ret; + crtc->dev = dev; crtc->funcs = funcs;
mutex_lock(&dev->mode_config.mutex); - drm_mode_object_get(dev, &crtc->base, DRM_MODE_OBJECT_CRTC); + + ret = drm_mode_object_get(dev, &crtc->base, DRM_MODE_OBJECT_CRTC); + if (ret) + goto out;
list_add_tail(&crtc->head, &dev->mode_config.crtc_list); dev->mode_config.num_crtc++; + + out: mutex_unlock(&dev->mode_config.mutex); + + return ret; } EXPORT_SYMBOL(drm_crtc_init);
@@ -453,17 +464,25 @@ EXPORT_SYMBOL(drm_mode_remove); * * Initialises a preallocated connector. Connectors should be * subclassed as part of driver connector objects. + * + * RETURNS: + * Zero on success, error code on failure. */ -void drm_connector_init(struct drm_device *dev, - struct drm_connector *connector, - const struct drm_connector_funcs *funcs, - int connector_type) +int drm_connector_init(struct drm_device *dev, + struct drm_connector *connector, + const struct drm_connector_funcs *funcs, + int connector_type) { + int ret; + mutex_lock(&dev->mode_config.mutex);
+ ret = drm_mode_object_get(dev, &connector->base, DRM_MODE_OBJECT_CONNECTOR); + if (ret) + goto out; + connector->dev = dev; connector->funcs = funcs; - drm_mode_object_get(dev, &connector->base, DRM_MODE_OBJECT_CONNECTOR); connector->connector_type = connector_type; connector->connector_type_id = ++drm_connector_enum_list[connector_type].count; /* TODO */ @@ -483,7 +502,10 @@ void drm_connector_init(struct drm_device *dev, drm_connector_attach_property(connector, dev->mode_config.dpms_property, 0);
+ out: mutex_unlock(&dev->mode_config.mutex); + + return ret; } EXPORT_SYMBOL(drm_connector_init);
@@ -518,23 +540,30 @@ void drm_connector_cleanup(struct drm_connector *connector) } EXPORT_SYMBOL(drm_connector_cleanup);
-void drm_encoder_init(struct drm_device *dev, - struct drm_encoder *encoder, - const struct drm_encoder_funcs *funcs, - int encoder_type) +int drm_encoder_init(struct drm_device *dev, + struct drm_encoder *encoder, + const struct drm_encoder_funcs *funcs, + int encoder_type) { + int ret; + mutex_lock(&dev->mode_config.mutex);
+ ret = drm_mode_object_get(dev, &encoder->base, DRM_MODE_OBJECT_ENCODER); + if (ret) + goto out; + encoder->dev = dev; - - drm_mode_object_get(dev, &encoder->base, DRM_MODE_OBJECT_ENCODER); encoder->encoder_type = encoder_type; encoder->funcs = funcs;
list_add_tail(&encoder->head, &dev->mode_config.encoder_list); dev->mode_config.num_encoder++;
+ out: mutex_unlock(&dev->mode_config.mutex); + + return ret; } EXPORT_SYMBOL(drm_encoder_init);
@@ -555,18 +584,23 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane, const uint32_t *formats, uint32_t format_count, bool priv) { + int ret; + mutex_lock(&dev->mode_config.mutex);
+ ret = drm_mode_object_get(dev, &plane->base, DRM_MODE_OBJECT_PLANE); + if (ret) + goto out; + plane->dev = dev; - drm_mode_object_get(dev, &plane->base, DRM_MODE_OBJECT_PLANE); 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 allocating plane\n"); drm_mode_object_put(dev, &plane->base); - mutex_unlock(&dev->mode_config.mutex); - return -ENOMEM; + ret = -ENOMEM; + goto out; }
memcpy(plane->format_types, formats, format_count * sizeof(uint32_t)); @@ -584,9 +618,10 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane, INIT_LIST_HEAD(&plane->head); }
+ out: mutex_unlock(&dev->mode_config.mutex);
- return 0; + return ret; } EXPORT_SYMBOL(drm_plane_init);
@@ -626,7 +661,11 @@ struct drm_display_mode *drm_mode_create(struct drm_device *dev) if (!nmode) return NULL;
- drm_mode_object_get(dev, &nmode->base, DRM_MODE_OBJECT_MODE); + if (drm_mode_object_get(dev, &nmode->base, DRM_MODE_OBJECT_MODE)) { + kfree(nmode); + return NULL; + } + return nmode; } EXPORT_SYMBOL(drm_mode_create); @@ -2597,6 +2636,7 @@ struct drm_property *drm_property_create(struct drm_device *dev, int flags, const char *name, int num_values) { struct drm_property *property = NULL; + int ret;
property = kzalloc(sizeof(struct drm_property), GFP_KERNEL); if (!property) @@ -2608,7 +2648,10 @@ struct drm_property *drm_property_create(struct drm_device *dev, int flags, goto fail; }
- drm_mode_object_get(dev, &property->base, DRM_MODE_OBJECT_PROPERTY); + ret = drm_mode_object_get(dev, &property->base, DRM_MODE_OBJECT_PROPERTY); + if (ret) + goto fail; + property->flags = flags; property->num_values = num_values; INIT_LIST_HEAD(&property->enum_blob_list); @@ -2621,6 +2664,7 @@ struct drm_property *drm_property_create(struct drm_device *dev, int flags, list_add_tail(&property->head, &dev->mode_config.property_list); return property; fail: + kfree(property->values); kfree(property); return NULL; } @@ -2884,6 +2928,7 @@ static struct drm_property_blob *drm_property_create_blob(struct drm_device *dev void *data) { struct drm_property_blob *blob; + int ret;
if (!length || !data) return NULL; @@ -2892,13 +2937,17 @@ static struct drm_property_blob *drm_property_create_blob(struct drm_device *dev if (!blob) return NULL;
+ ret = drm_mode_object_get(dev, &blob->base, DRM_MODE_OBJECT_BLOB); + if (ret) { + kfree(blob); + return NULL; + } + blob->data = (void *)((char *)blob + sizeof(struct drm_property_blob)); blob->length = length;
memcpy(blob->data, data, length);
- drm_mode_object_get(dev, &blob->base, DRM_MODE_OBJECT_BLOB); - list_add_tail(&blob->head, &dev->mode_config.property_blob_list); return blob; } diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index d761d12..d9d6684 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -352,6 +352,8 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, return true;
adjusted_mode = drm_mode_duplicate(dev, mode); + if (!adjusted_mode) + return false;
saved_hwmode = crtc->hwmode; saved_mode = crtc->mode; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index fe7ebc6..00f4007 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -815,22 +815,22 @@ struct drm_prop_enum_list { char *name; };
-extern void drm_crtc_init(struct drm_device *dev, - struct drm_crtc *crtc, - const struct drm_crtc_funcs *funcs); +extern int drm_crtc_init(struct drm_device *dev, + struct drm_crtc *crtc, + const struct drm_crtc_funcs *funcs); extern void drm_crtc_cleanup(struct drm_crtc *crtc);
-extern void drm_connector_init(struct drm_device *dev, - struct drm_connector *connector, - const struct drm_connector_funcs *funcs, - int connector_type); +extern int drm_connector_init(struct drm_device *dev, + struct drm_connector *connector, + const struct drm_connector_funcs *funcs, + int connector_type);
extern void drm_connector_cleanup(struct drm_connector *connector);
-extern void drm_encoder_init(struct drm_device *dev, - struct drm_encoder *encoder, - const struct drm_encoder_funcs *funcs, - int encoder_type); +extern int drm_encoder_init(struct drm_device *dev, + struct drm_encoder *encoder, + const struct drm_encoder_funcs *funcs, + int encoder_type);
extern int drm_plane_init(struct drm_device *dev, struct drm_plane *plane,
From: Ville Syrjälä ville.syrjala@linux.intel.com
The blob property data is always allocated immediately after the object header. No need for the extra indirection when accessing it, just use a flexible array member.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_crtc.c | 1 - include/drm/drm_crtc.h | 2 +- 2 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index f5b098e..d2d9dc5 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -2943,7 +2943,6 @@ static struct drm_property_blob *drm_property_create_blob(struct drm_device *dev return NULL; }
- blob->data = (void *)((char *)blob + sizeof(struct drm_property_blob)); blob->length = length;
memcpy(blob->data, data, length); diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 00f4007..53cb49a 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -257,7 +257,7 @@ struct drm_property_blob { struct drm_mode_object base; struct list_head head; unsigned int length; - void *data; + unsigned char data[]; };
struct drm_property_enum {
From: Ville Syrjälä ville.syrjala@linux.intel.com
Use a do {} while() loop instead of a goto in drm_mode_object_get().
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_crtc.c | 20 ++++++++++---------- 1 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d2d9dc5..12333ca 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -224,17 +224,17 @@ static int drm_mode_object_get(struct drm_device *dev, int new_id = 0; int ret;
-again: - if (idr_pre_get(&dev->mode_config.crtc_idr, GFP_KERNEL) == 0) { - DRM_ERROR("Ran out memory getting a mode number\n"); - return -EINVAL; - } + do { + if (idr_pre_get(&dev->mode_config.crtc_idr, GFP_KERNEL) == 0) { + DRM_ERROR("Ran out memory getting a mode number\n"); + return -EINVAL; + }
- mutex_lock(&dev->mode_config.idr_mutex); - ret = idr_get_new_above(&dev->mode_config.crtc_idr, obj, 1, &new_id); - mutex_unlock(&dev->mode_config.idr_mutex); - if (ret == -EAGAIN) - goto again; + mutex_lock(&dev->mode_config.idr_mutex); + ret = idr_get_new_above(&dev->mode_config.crtc_idr, + obj, 1, &new_id); + mutex_unlock(&dev->mode_config.idr_mutex); + } while (ret == -EAGAIN);
obj->id = new_id; obj->type = obj_type;
Use a do {} while() loop instead of a goto in drm_mode_object_get().
I don't like this one just because it diverges our idr usage from the canonical idr usage at
http://lwn.net/Articles/103209/
So unless there is a good reason for the change I'd rather not apply it.
I've applied the other patches except this and the last one which I think depends on it.
Dave.
On Thu, Mar 15, 2012 at 09:57:53AM +0000, Dave Airlie wrote:
Use a do {} while() loop instead of a goto in drm_mode_object_get().
I don't like this one just because it diverges our idr usage from the canonical idr usage at
http://lwn.net/Articles/103209/
So unless there is a good reason for the change I'd rather not apply it.
That's fine by me. Although "canonical" may be an overstatement. I just quickly went through the idr_pre_get() usage in the tree, and between the three conteders (no loop, goto loop, while loop) it was a close race. No loop won with 29 idr_pre_get()s, goto loop came in second with 24, and while loop came in third with 21.
The drm subsystem itself seems to have 5 goto loops. So if we were to change them all, goto loop would lose to the while loop.
I've applied the other patches except this and the last one which I think depends on it.
The error handling in the goto version is equally broken. I'll respin the patch on top of the goto version, and I'll check the other idr usage under drm as well...
From: Ville Syrjälä ville.syrjala@linux.intel.com
Add a helper function to copy a display mode. Use it in drm_mode_duplicate() and nouveau mode_fixup hooks.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_modes.c | 28 +++++++++++++++++++++++----- drivers/gpu/drm/nouveau/nv50_dac.c | 7 ++----- drivers/gpu/drm/nouveau/nv50_sor.c | 7 ++----- include/drm/drm_crtc.h | 1 + 4 files changed, 28 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 7ff13bc..b7adb4a 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -714,6 +714,27 @@ EXPORT_SYMBOL(drm_mode_set_crtcinfo);
/** + * drm_mode_copy - copy the mode + * @dst: mode to overwrite + * @src: mode to copy + * + * LOCKING: + * None. + * + * Copy an existing mode into another mode, preserving the object id + * of the destination mode. + */ +void drm_mode_copy(struct drm_display_mode *dst, const struct drm_display_mode *src) +{ + int id = dst->base.id; + + *dst = *src; + dst->base.id = id; + INIT_LIST_HEAD(&dst->head); +} +EXPORT_SYMBOL(drm_mode_copy); + +/** * drm_mode_duplicate - allocate and duplicate an existing mode * @m: mode to duplicate * @@ -727,16 +748,13 @@ struct drm_display_mode *drm_mode_duplicate(struct drm_device *dev, const struct drm_display_mode *mode) { struct drm_display_mode *nmode; - int new_id;
nmode = drm_mode_create(dev); if (!nmode) return NULL;
- new_id = nmode->base.id; - *nmode = *mode; - nmode->base.id = new_id; - INIT_LIST_HEAD(&nmode->head); + drm_mode_copy(nmode, mode); + return nmode; } EXPORT_SYMBOL(drm_mode_duplicate); diff --git a/drivers/gpu/drm/nouveau/nv50_dac.c b/drivers/gpu/drm/nouveau/nv50_dac.c index a0f2beb..55c5633 100644 --- a/drivers/gpu/drm/nouveau/nv50_dac.c +++ b/drivers/gpu/drm/nouveau/nv50_dac.c @@ -190,11 +190,8 @@ nv50_dac_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *mode, }
if (connector->scaling_mode != DRM_MODE_SCALE_NONE && - connector->native_mode) { - int id = adjusted_mode->base.id; - *adjusted_mode = *connector->native_mode; - adjusted_mode->base.id = id; - } + connector->native_mode) + drm_mode_copy(adjusted_mode, connector->native_mode);
return true; } diff --git a/drivers/gpu/drm/nouveau/nv50_sor.c b/drivers/gpu/drm/nouveau/nv50_sor.c index c4423ba..1706964 100644 --- a/drivers/gpu/drm/nouveau/nv50_sor.c +++ b/drivers/gpu/drm/nouveau/nv50_sor.c @@ -162,11 +162,8 @@ nv50_sor_mode_fixup(struct drm_encoder *encoder, struct drm_display_mode *mode, }
if (connector->scaling_mode != DRM_MODE_SCALE_NONE && - connector->native_mode) { - int id = adjusted_mode->base.id; - *adjusted_mode = *connector->native_mode; - adjusted_mode->base.id = id; - } + connector->native_mode) + drm_mode_copy(adjusted_mode, connector->native_mode);
return true; } diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 53cb49a..9595c2c 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -855,6 +855,7 @@ extern struct edid *drm_get_edid(struct drm_connector *connector, extern int drm_add_edid_modes(struct drm_connector *connector, struct edid *edid); extern void drm_mode_probed_add(struct drm_connector *connector, struct drm_display_mode *mode); extern void drm_mode_remove(struct drm_connector *connector, struct drm_display_mode *mode); +extern void drm_mode_copy(struct drm_display_mode *dst, const struct drm_display_mode *src); extern struct drm_display_mode *drm_mode_duplicate(struct drm_device *dev, const struct drm_display_mode *mode); extern void drm_mode_debug_printmodeline(struct drm_display_mode *mode);
From: Ville Syrjälä ville.syrjala@linux.intel.com
Change drm_mode_object_get() to return -ENOMEM if idr_pre_get() fails, and also handle -ENOSPC from idr_get_new_above().
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_crtc.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 12333ca..8f66a15 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -227,7 +227,7 @@ static int drm_mode_object_get(struct drm_device *dev, do { if (idr_pre_get(&dev->mode_config.crtc_idr, GFP_KERNEL) == 0) { DRM_ERROR("Ran out memory getting a mode number\n"); - return -EINVAL; + return -ENOMEM; }
mutex_lock(&dev->mode_config.idr_mutex); @@ -236,6 +236,9 @@ static int drm_mode_object_get(struct drm_device *dev, mutex_unlock(&dev->mode_config.idr_mutex); } while (ret == -EAGAIN);
+ if (ret) + return ret; + obj->id = new_id; obj->type = obj_type; return 0;
On Tue, Mar 13, 2012 at 6:35 AM, ville.syrjala@linux.intel.com wrote:
Mostly fixes for various bits and pieces that caught my eye while reading the mode setting code.
For the series:
Reviewed-by: Alex Deucher alexander.deucher@amd.com
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Dave Airlie -
Ville Syrjälä -
ville.syrjala＠linux.intel.com