Hi, Building on the blob-property submission series[0], this provides a MODE_ID property that can be used to set the current mode for a CRTC under atomic modesetting, plus a way for users to create blob properties so they can specify their own modes.
Using this[1] I've been able to validate the atomic userspace API with a ported version of Weston[2]. This is currently undergoing review, but seems pretty OK. Both modesetting for every output together, and per-output async pageflips with events, works as well as the driver does.
Also tested with fbcon and X11, on Broadwell.
Cheers, Daniel
[0]: 1429554176-9865-1-git-send-email-daniels@collabora.com [1]: And a pile of other work in the i915 driver from Maarten/Ander, and my own bits on top; see git://git.collabora.co.uk/git/user/daniels/linux in the wip/4.1.x/blob-modes-i915 branch. [2]: http://lists.freedesktop.org/archives/wayland-devel/2015-May/022106.html
Change '@param foo' to '@foo:' to fit kerneldoc style.
672cb1d6ae mistakenly added an extra parameter to the kerneldoc for drm_property_unreference_blob which wasn't actually present.
Signed-off-by: Daniel Stone daniels@collabora.com --- drivers/gpu/drm/drm_crtc.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 4059f06..7e5085f 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -4270,7 +4270,7 @@ EXPORT_SYMBOL(drm_property_create_blob); * * Internal free function for blob properties; must not be used directly. * - * @param kref Reference + * @kref: Reference */ static void drm_property_free_blob(struct kref *kref) { @@ -4290,7 +4290,7 @@ static void drm_property_free_blob(struct kref *kref) * * Drop a reference on a blob property. May free the object. * - * @param blob Pointer to blob property + * @blob: Pointer to blob property */ void drm_property_unreference_blob(struct drm_property_blob *blob) { @@ -4318,8 +4318,7 @@ EXPORT_SYMBOL(drm_property_unreference_blob); * Drop a reference on a blob property. May free the object. This must be * called with blob_lock held. * - * @param dev Device the blob was created on - * @param blob Pointer to blob property + * @blob: Pointer to blob property */ static void drm_property_unreference_blob_locked(struct drm_property_blob *blob) { @@ -4336,7 +4335,7 @@ static void drm_property_unreference_blob_locked(struct drm_property_blob *blob) * * Take a new reference on an existing blob property. * - * @param blob Pointer to blob property + * @blob: Pointer to blob property */ struct drm_property_blob *drm_property_reference_blob(struct drm_property_blob *blob) {
Rather than open-coding our own CRTC state helpers, use the atomic helpers added in f5e7840b0c, and make our freeing behaviour consistent as well.
Signed-off-by: Daniel Stone daniels@collabora.com Tested-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/drm_crtc_helper.c | 42 ++++++++++++++++++--------------------- 1 file changed, 19 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index f2f42b1..9297a61 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -927,14 +927,15 @@ int drm_helper_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mod
if (crtc->funcs->atomic_duplicate_state) crtc_state = crtc->funcs->atomic_duplicate_state(crtc); - else if (crtc->state) - crtc_state = kmemdup(crtc->state, sizeof(*crtc_state), - GFP_KERNEL); - else + else { crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL); - if (!crtc_state) - return -ENOMEM; - crtc_state->crtc = crtc; + if (!crtc_state) + return -ENOMEM; + if (crtc->state) + __drm_atomic_helper_crtc_duplicate_state(crtc, crtc_state); + else + crtc_state->crtc = crtc; + }
crtc_state->enable = true; crtc_state->planes_changed = true; @@ -944,30 +945,25 @@ int drm_helper_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mod
if (crtc_funcs->atomic_check) { ret = crtc_funcs->atomic_check(crtc, crtc_state); - if (ret) { - if (crtc->funcs->atomic_destroy_state) { - crtc->funcs->atomic_destroy_state(crtc, - crtc_state); - } else { - kfree(crtc_state); - } - - return ret; - } + if (ret) + goto out; }
swap(crtc->state, crtc_state);
crtc_funcs->mode_set_nofb(crtc);
- if (crtc_state) { - if (crtc->funcs->atomic_destroy_state) - crtc->funcs->atomic_destroy_state(crtc, crtc_state); - else - kfree(crtc_state); + ret = drm_helper_crtc_mode_set_base(crtc, x, y, old_fb); + +out: + if (crtc->funcs->atomic_destroy_state) + crtc->funcs->atomic_destroy_state(crtc, crtc_state); + else { + __drm_atomic_helper_crtc_destroy_state(crtc, crtc_state); + kfree(crtc_state); }
- return drm_helper_crtc_mode_set_base(crtc, x, y, old_fb); + return ret; } EXPORT_SYMBOL(drm_helper_crtc_mode_set);
When we look up a blob property, make sure we retain a reference to the blob for the lifetime.
Signed-off-by: Daniel Stone daniels@collabora.com Tested-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/drm_crtc.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 7e5085f..da2b117 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -4661,7 +4661,9 @@ bool drm_property_change_valid_get(struct drm_property *property, for (i = 0; i < property->num_values; i++) valid_mask |= (1ULL << property->values[i]); return !(value & ~valid_mask); - } else if (drm_property_type_is(property, DRM_MODE_PROP_BLOB)) { + } else if (drm_property_type_is(property, DRM_MODE_PROP_BLOB) || + (drm_property_type_is(property, DRM_MODE_PROP_OBJECT) && + property->values[0] == DRM_MODE_OBJECT_BLOB)) { struct drm_property_blob *blob;
if (value == 0) @@ -4709,6 +4711,8 @@ void drm_property_change_valid_put(struct drm_property *property, if (drm_property_type_is(property, DRM_MODE_PROP_OBJECT)) { if (property->values[0] == DRM_MODE_OBJECT_FB) drm_framebuffer_unreference(obj_to_fb(ref)); + else if (property->values[0] == DRM_MODE_OBJECT_BLOB) + drm_property_unreference_blob(obj_to_blob(ref)); } }
On Fri, May 22, 2015 at 01:34:46PM +0100, Daniel Stone wrote:
When we look up a blob property, make sure we retain a reference to the blob for the lifetime.
Signed-off-by: Daniel Stone daniels@collabora.com Tested-by: Sean Paul seanpaul@chromium.org
drivers/gpu/drm/drm_crtc.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 7e5085f..da2b117 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -4661,7 +4661,9 @@ bool drm_property_change_valid_get(struct drm_property *property, for (i = 0; i < property->num_values; i++) valid_mask |= (1ULL << property->values[i]); return !(value & ~valid_mask);
- } else if (drm_property_type_is(property, DRM_MODE_PROP_BLOB)) {
} else if (drm_property_type_is(property, DRM_MODE_PROP_BLOB) ||
(drm_property_type_is(property, DRM_MODE_PROP_OBJECT) &&property->values[0] == DRM_MODE_OBJECT_BLOB)) {struct drm_property_blob *blob;
if (value == 0)
@@ -4709,6 +4711,8 @@ void drm_property_change_valid_put(struct drm_property *property, if (drm_property_type_is(property, DRM_MODE_PROP_OBJECT)) { if (property->values[0] == DRM_MODE_OBJECT_FB) drm_framebuffer_unreference(obj_to_fb(ref));
else if (property->values[0] == DRM_MODE_OBJECT_BLOB)drm_property_unreference_blob(obj_to_blob(ref));
Hm why this? I know it's a bit inconsistent that for all the other atomic obj props we use DRM_MODE_PROP_OBJ, but for blobs we have a special one. I guess I'll see the reason later on, but without more details I'd vote to not subsume props under the generic PROP_OBJECT. -Daniel
The only user of convert_umode was also performing mode validation, so do that in the same place.
Signed-off-by: Daniel Stone daniels@collabora.com Tested-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/drm_crtc.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index da2b117..9ab2fc6 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1785,11 +1785,15 @@ static void drm_crtc_convert_to_umode(struct drm_mode_modeinfo *out, 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; + int ret = -EINVAL; + + if (in->clock > INT_MAX || in->vrefresh > INT_MAX) { + ret = -ERANGE; + goto out; + }
if ((in->flags & DRM_MODE_FLAG_3D_MASK) > DRM_MODE_FLAG_3D_MAX) - return -EINVAL; + goto out;
out->clock = in->clock; out->hdisplay = in->hdisplay; @@ -1808,7 +1812,14 @@ static int drm_crtc_convert_umode(struct drm_display_mode *out, strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); out->name[DRM_DISPLAY_MODE_LEN-1] = 0;
- return 0; + out->status = drm_mode_validate_basic(out); + if (out->status != MODE_OK) + goto out; + + ret = 0; + +out: + return ret; }
/** @@ -2821,12 +2832,6 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, goto out; }
- mode->status = drm_mode_validate_basic(mode); - if (mode->status != MODE_OK) { - ret = -EINVAL; - goto out; - } - drm_mode_set_crtcinfo(mode, CRTC_INTERLACE_HALVE_V);
/*
Move the drm_display_mode <-> drm_mode_modeinfo conversion functions from drm_crtc.c to drm_modes.c, and make them non-static so that others can use them.
Signed-off-by: Daniel Stone daniels@collabora.com Tested-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/drm_crtc.c | 95 ++------------------------------------------- drivers/gpu/drm/drm_modes.c | 87 +++++++++++++++++++++++++++++++++++++++++ include/drm/drm_modes.h | 4 ++ 3 files changed, 95 insertions(+), 91 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 9ab2fc6..def7be8 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1736,93 +1736,6 @@ void drm_reinit_primary_mode_group(struct drm_device *dev) EXPORT_SYMBOL(drm_reinit_primary_mode_group);
/** - * drm_crtc_convert_to_umode - convert a drm_display_mode into a modeinfo - * @out: drm_mode_modeinfo struct to return to the user - * @in: drm_display_mode to use - * - * Convert a drm_display_mode into a drm_mode_modeinfo structure to return to - * the user. - */ -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 || - 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; - out->hsync_end = in->hsync_end; - out->htotal = in->htotal; - out->hskew = in->hskew; - out->vdisplay = in->vdisplay; - out->vsync_start = in->vsync_start; - out->vsync_end = in->vsync_end; - out->vtotal = in->vtotal; - out->vscan = in->vscan; - out->vrefresh = in->vrefresh; - out->flags = in->flags; - out->type = in->type; - strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); - out->name[DRM_DISPLAY_MODE_LEN-1] = 0; -} - -/** - * drm_crtc_convert_umode - convert a modeinfo into a drm_display_mode - * @out: drm_display_mode to return to the user - * @in: drm_mode_modeinfo to use - * - * Convert a drm_mode_modeinfo into a drm_display_mode structure to return to - * the caller. - * - * Returns: - * Zero on success, negative errno on failure. - */ -static int drm_crtc_convert_umode(struct drm_display_mode *out, - const struct drm_mode_modeinfo *in) -{ - int ret = -EINVAL; - - if (in->clock > INT_MAX || in->vrefresh > INT_MAX) { - ret = -ERANGE; - goto out; - } - - if ((in->flags & DRM_MODE_FLAG_3D_MASK) > DRM_MODE_FLAG_3D_MAX) - goto out; - - out->clock = in->clock; - out->hdisplay = in->hdisplay; - out->hsync_start = in->hsync_start; - out->hsync_end = in->hsync_end; - out->htotal = in->htotal; - out->hskew = in->hskew; - out->vdisplay = in->vdisplay; - out->vsync_start = in->vsync_start; - out->vsync_end = in->vsync_end; - out->vtotal = in->vtotal; - out->vscan = in->vscan; - out->vrefresh = in->vrefresh; - out->flags = in->flags; - out->type = in->type; - strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); - out->name[DRM_DISPLAY_MODE_LEN-1] = 0; - - out->status = drm_mode_validate_basic(out); - if (out->status != MODE_OK) - goto out; - - ret = 0; - -out: - return ret; -} - -/** * drm_mode_getresources - get graphics configuration * @dev: drm device for the ioctl * @data: data pointer for the ioctl @@ -2048,7 +1961,7 @@ int drm_mode_getcrtc(struct drm_device *dev, crtc_resp->x = crtc->primary->state->src_x >> 16; crtc_resp->y = crtc->primary->state->src_y >> 16; if (crtc->state->enable) { - drm_crtc_convert_to_umode(&crtc_resp->mode, &crtc->state->mode); + drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode); crtc_resp->mode_valid = 1;
} else { @@ -2058,7 +1971,7 @@ int drm_mode_getcrtc(struct drm_device *dev, crtc_resp->x = crtc->x; crtc_resp->y = crtc->y; if (crtc->enabled) { - drm_crtc_convert_to_umode(&crtc_resp->mode, &crtc->mode); + drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->mode); crtc_resp->mode_valid = 1;
} else { @@ -2215,7 +2128,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, if (!drm_mode_expose_to_userspace(mode, file_priv)) continue;
- drm_crtc_convert_to_umode(&u_mode, mode); + drm_mode_convert_to_umode(&u_mode, mode); if (copy_to_user(mode_ptr + copied, &u_mode, sizeof(u_mode))) { ret = -EFAULT; @@ -2826,7 +2739,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, goto out; }
- ret = drm_crtc_convert_umode(mode, &crtc_req->mode); + ret = drm_mode_convert_umode(mode, &crtc_req->mode); if (ret) { DRM_DEBUG_KMS("Invalid mode\n"); goto out; diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 213b11e..cd74a09 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -1405,3 +1405,90 @@ drm_mode_create_from_cmdline_mode(struct drm_device *dev, return mode; } EXPORT_SYMBOL(drm_mode_create_from_cmdline_mode); + +/** + * drm_crtc_convert_to_umode - convert a drm_display_mode into a modeinfo + * @out: drm_mode_modeinfo struct to return to the user + * @in: drm_display_mode to use + * + * Convert a drm_display_mode into a drm_mode_modeinfo structure to return to + * the user. + */ +void drm_mode_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 || + 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; + out->hsync_end = in->hsync_end; + out->htotal = in->htotal; + out->hskew = in->hskew; + out->vdisplay = in->vdisplay; + out->vsync_start = in->vsync_start; + out->vsync_end = in->vsync_end; + out->vtotal = in->vtotal; + out->vscan = in->vscan; + out->vrefresh = in->vrefresh; + out->flags = in->flags; + out->type = in->type; + strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); + out->name[DRM_DISPLAY_MODE_LEN-1] = 0; +} + +/** + * drm_crtc_convert_umode - convert a modeinfo into a drm_display_mode + * @out: drm_display_mode to return to the user + * @in: drm_mode_modeinfo to use + * + * Convert a drm_mode_modeinfo into a drm_display_mode structure to return to + * the caller. + * + * Returns: + * Zero on success, negative errno on failure. + */ +int drm_mode_convert_umode(struct drm_display_mode *out, + const struct drm_mode_modeinfo *in) +{ + int ret = -EINVAL; + + if (in->clock > INT_MAX || in->vrefresh > INT_MAX) { + ret = -ERANGE; + goto out; + } + + if ((in->flags & DRM_MODE_FLAG_3D_MASK) > DRM_MODE_FLAG_3D_MAX) + goto out; + + out->clock = in->clock; + out->hdisplay = in->hdisplay; + out->hsync_start = in->hsync_start; + out->hsync_end = in->hsync_end; + out->htotal = in->htotal; + out->hskew = in->hskew; + out->vdisplay = in->vdisplay; + out->vsync_start = in->vsync_start; + out->vsync_end = in->vsync_end; + out->vtotal = in->vtotal; + out->vscan = in->vscan; + out->vrefresh = in->vrefresh; + out->flags = in->flags; + out->type = in->type; + strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); + out->name[DRM_DISPLAY_MODE_LEN-1] = 0; + + out->status = drm_mode_validate_basic(out); + if (out->status != MODE_OK) + goto out; + + ret = 0; + +out: + return ret; +} \ No newline at end of file diff --git a/include/drm/drm_modes.h b/include/drm/drm_modes.h index 0616188..08a8cac 100644 --- a/include/drm/drm_modes.h +++ b/include/drm/drm_modes.h @@ -182,6 +182,10 @@ struct drm_cmdline_mode;
struct drm_display_mode *drm_mode_create(struct drm_device *dev); void drm_mode_destroy(struct drm_device *dev, struct drm_display_mode *mode); +void drm_mode_convert_to_umode(struct drm_mode_modeinfo *out, + const struct drm_display_mode *in); +int drm_mode_convert_umode(struct drm_display_mode *out, + const struct drm_mode_modeinfo *in); void drm_mode_probed_add(struct drm_connector *connector, struct drm_display_mode *mode); void drm_mode_debug_printmodeline(const struct drm_display_mode *mode);
Make the data parameter to drm_property_create_blob optional; if omitted, the copy will be skipped and the data will be empty.
Signed-off-by: Daniel Stone daniels@collabora.com Reviewed-by: Maarten Lankhorst maarten.lankhorst@intel.com Tested-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/drm_crtc.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index def7be8..e4a48e0 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -4145,6 +4145,16 @@ done: return ret; }
+/** + * drm_property_create_blob - Create new blob property + * + * Creates a new blob property for a specified DRM device, optionally + * copying data. + * + * @dev: DRM device to create property for + * @length: Length to allocate for blob data + * @data: If specified, copies data into blob + */ struct drm_property_blob * drm_property_create_blob(struct drm_device *dev, size_t length, const void *data) @@ -4152,7 +4162,7 @@ drm_property_create_blob(struct drm_device *dev, size_t length, struct drm_property_blob *blob; int ret;
- if (!length || !data) + if (!length) return NULL;
blob = kzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL); @@ -4162,7 +4172,8 @@ drm_property_create_blob(struct drm_device *dev, size_t length, blob->length = length; blob->dev = dev;
- memcpy(blob->data, data, length); + if (data) + memcpy(blob->data, data, length);
mutex_lock(&dev->mode_config.blob_lock);
Change drm_property_create_blob to return an ERR_PTR-encoded error on failure, so we can pass the failure reason down.
Signed-off-by: Daniel Stone daniels@collabora.com Cc: Maarten Lankhorst maarten.lankhorst@intel.com Tested-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/drm_crtc.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index e4a48e0..494c4a0 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -4154,6 +4154,10 @@ done: * @dev: DRM device to create property for * @length: Length to allocate for blob data * @data: If specified, copies data into blob + * + * Returns: + * New blob property with a single reference on success, or an ERR_PTR + * value on failure. */ struct drm_property_blob * drm_property_create_blob(struct drm_device *dev, size_t length, @@ -4163,11 +4167,11 @@ drm_property_create_blob(struct drm_device *dev, size_t length, int ret;
if (!length) - return NULL; + return ERR_PTR(-EINVAL);
blob = kzalloc(sizeof(struct drm_property_blob)+length, GFP_KERNEL); if (!blob) - return NULL; + return ERR_PTR(-ENOMEM);
blob->length = length; blob->dev = dev; @@ -4181,7 +4185,7 @@ drm_property_create_blob(struct drm_device *dev, size_t length, if (ret) { kfree(blob); mutex_unlock(&dev->mode_config.blob_lock); - return NULL; + return ERR_PTR(-EINVAL); }
kref_init(&blob->refcount); @@ -4370,8 +4374,8 @@ static int drm_property_replace_global_blob(struct drm_device *dev,
if (length && data) { new_blob = drm_property_create_blob(dev, length, data); - if (!new_blob) - return -EINVAL; + if (IS_ERR(new_blob)) + return PTR_ERR(new_blob); }
/* This does not need to be synchronised with blob_lock, as the
Add an ioctl which allows users to create blob properties from supplied data. Currently this only supports modes, creating a drm_display_mode from the userspace drm_mode_modeinfo.
v2: Removed size/type checks. Rebased on new patches to allow error propagation from create_blob, as well as avoiding double-allocation.
Signed-off-by: Daniel Stone daniels@collabora.com Reviewed-by: Maarten Lankhorst maarten.lankhorst@intel.com Tested-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/drm_crtc.c | 139 +++++++++++++++++++++++++++++++++++++++++++- drivers/gpu/drm/drm_fops.c | 5 +- drivers/gpu/drm/drm_ioctl.c | 2 + include/drm/drmP.h | 4 ++ include/drm/drm_crtc.h | 9 ++- include/uapi/drm/drm.h | 2 + include/uapi/drm/drm_mode.h | 20 +++++++ 7 files changed, 176 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 494c4a0..9263b22 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -4173,6 +4173,9 @@ drm_property_create_blob(struct drm_device *dev, size_t length, if (!blob) return ERR_PTR(-ENOMEM);
+ /* This must be explicitly initialised, so we can safely call list_del + * on it in the removal handler, even if it isn't in a file list. */ + INIT_LIST_HEAD(&blob->head_file); blob->length = length; blob->dev = dev;
@@ -4190,7 +4193,8 @@ drm_property_create_blob(struct drm_device *dev, size_t length,
kref_init(&blob->refcount);
- list_add_tail(&blob->head, &dev->mode_config.property_blob_list); + list_add_tail(&blob->head_global, + &dev->mode_config.property_blob_list);
mutex_unlock(&dev->mode_config.blob_lock);
@@ -4212,7 +4216,8 @@ static void drm_property_free_blob(struct kref *kref)
WARN_ON(!mutex_is_locked(&blob->dev->mode_config.blob_lock));
- list_del(&blob->head); + list_del(&blob->head_global); + list_del(&blob->head_file); drm_mode_object_put(blob->dev, &blob->base);
kfree(blob); @@ -4264,6 +4269,26 @@ static void drm_property_unreference_blob_locked(struct drm_property_blob *blob) }
/** + * drm_property_destroy_user_blobs - destroy all blobs created by this client + * @dev: DRM device + * @file_priv: destroy all blobs owned by this file handle + */ +void drm_property_destroy_user_blobs(struct drm_device *dev, + struct drm_file *file_priv) +{ + struct drm_property_blob *blob, *bt; + + mutex_lock(&dev->mode_config.blob_lock); + + list_for_each_entry_safe(blob, bt, &file_priv->blobs, head_file) { + list_del_init(&blob->head_file); + drm_property_unreference_blob_locked(blob); + } + + mutex_unlock(&dev->mode_config.blob_lock); +} + +/** * drm_property_reference_blob - Take a reference on an existing property * * Take a new reference on an existing blob property. @@ -4453,6 +4478,114 @@ done: }
/** + * drm_mode_createblob_ioctl - create a new blob property + * @dev: DRM device + * @data: ioctl data + * @file_priv: DRM file info + * + * This function creates a new blob property with user-defined values. In order + * to give us sensible validation and checking when creating, rather than at + * every potential use, we also require a type to be provided upfront. + * + * Called by the user via ioctl. + * + * Returns: + * Zero on success, negative errno on failure. + */ +int drm_mode_createblob_ioctl(struct drm_device *dev, + void *data, struct drm_file *file_priv) +{ + struct drm_mode_create_blob *out_resp = data; + struct drm_property_blob *blob; + void __user *blob_ptr; + int ret = 0; + + if (!drm_core_check_feature(dev, DRIVER_MODESET)) + return -EINVAL; + + blob = drm_property_create_blob(dev, out_resp->length, NULL); + if (IS_ERR(blob)) + return PTR_ERR(blob); + + blob_ptr = (void __user *)(unsigned long)out_resp->data; + if (copy_from_user(blob->data, blob_ptr, out_resp->length)) { + ret = -EFAULT; + goto out_blob; + } + + /* Dropping the lock between create_blob and our access here is safe + * as only the same file_priv can remove the blob; at this point, it is + * not associated with any file_priv. */ + mutex_lock(&dev->mode_config.blob_lock); + out_resp->blob_id = blob->base.id; + list_add_tail(&file_priv->blobs, &blob->head_file); + mutex_unlock(&dev->mode_config.blob_lock); + + return 0; + +out_blob: + drm_property_unreference_blob(blob); + return ret; +} + +/** + * drm_mode_destroyblob_ioctl - destroy a user blob property + * @dev: DRM device + * @data: ioctl data + * @file_priv: DRM file info + * + * Destroy an existing user-defined blob property. + * + * Called by the user via ioctl. + * + * Returns: + * Zero on success, negative errno on failure. + */ +int drm_mode_destroyblob_ioctl(struct drm_device *dev, + void *data, struct drm_file *file_priv) +{ + struct drm_mode_destroy_blob *out_resp = data; + struct drm_property_blob *blob = NULL, *bt; + bool found = false; + int ret = 0; + + if (!drm_core_check_feature(dev, DRIVER_MODESET)) + return -EINVAL; + + mutex_lock(&dev->mode_config.blob_lock); + blob = __drm_property_lookup_blob(dev, out_resp->blob_id); + if (!blob) { + ret = -ENOENT; + goto err; + } + + /* Ensure the property was actually created by this user. */ + list_for_each_entry(bt, &file_priv->blobs, head_file) { + if (bt == blob) { + found = true; + break; + } + } + + if (!found) { + ret = -EPERM; + goto err; + } + + /* We must drop head_file here, because we may not be the last + * reference on the blob. */ + list_del_init(&blob->head_file); + drm_property_unreference_blob_locked(blob); + mutex_unlock(&dev->mode_config.blob_lock); + + return 0; + +err: + mutex_unlock(&dev->mode_config.blob_lock); + return ret; +} + +/** * drm_mode_connector_set_path_property - set tile property on connector * @connector: connector to set property on. * @path: path to use for property; must not be NULL. @@ -5659,7 +5792,7 @@ void drm_mode_config_cleanup(struct drm_device *dev) }
list_for_each_entry_safe(blob, bt, &dev->mode_config.property_blob_list, - head) { + head_global) { drm_property_unreference_blob(blob); }
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 0f6a5c8..c59ce4d 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -167,6 +167,7 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor) INIT_LIST_HEAD(&priv->lhead); INIT_LIST_HEAD(&priv->fbs); mutex_init(&priv->fbs_lock); + INIT_LIST_HEAD(&priv->blobs); INIT_LIST_HEAD(&priv->event_list); init_waitqueue_head(&priv->event_wait); priv->event_space = 4096; /* set aside 4k for event buffer */ @@ -405,8 +406,10 @@ int drm_release(struct inode *inode, struct file *filp)
drm_events_release(file_priv);
- if (drm_core_check_feature(dev, DRIVER_MODESET)) + if (drm_core_check_feature(dev, DRIVER_MODESET)) { drm_fb_release(file_priv); + drm_property_destroy_user_blobs(dev, file_priv); + }
if (drm_core_check_feature(dev, DRIVER_GEM)) drm_gem_release(dev, file_priv); diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 266dcd6..9bac1b7 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -641,6 +641,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = { DRM_IOCTL_DEF(DRM_IOCTL_MODE_OBJ_SETPROPERTY, drm_mode_obj_set_property_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_CURSOR2, drm_mode_cursor2_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_ATOMIC, drm_mode_atomic_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), + DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATEPROPBLOB, drm_mode_createblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED), + DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROYPROPBLOB, drm_mode_destroyblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED), };
#define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls ) diff --git a/include/drm/drmP.h b/include/drm/drmP.h index df6d997..9fa6366 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -326,6 +326,10 @@ struct drm_file { struct list_head fbs; struct mutex fbs_lock;
+ /** User-created blob properties; this retains a reference on the + * property. */ + struct list_head blobs; + wait_queue_head_t event_wait; struct list_head event_list; int event_space; diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index dace1b6..72b60db 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -218,7 +218,8 @@ struct drm_property_blob { struct drm_mode_object base; struct drm_device *dev; struct kref refcount; - struct list_head head; + struct list_head head_global; + struct list_head head_file; size_t length; unsigned char data[]; }; @@ -1315,6 +1316,8 @@ extern const char *drm_get_dvi_i_select_name(int val); extern const char *drm_get_tv_subconnector_name(int val); extern const char *drm_get_tv_select_name(int val); extern void drm_fb_release(struct drm_file *file_priv); +extern void drm_property_destroy_user_blobs(struct drm_device *dev, + struct drm_file *file_priv); extern int drm_mode_group_init_legacy_group(struct drm_device *dev, struct drm_mode_group *group); extern void drm_mode_group_destroy(struct drm_mode_group *group); extern void drm_reinit_primary_mode_group(struct drm_device *dev); @@ -1460,6 +1463,10 @@ extern int drm_mode_getproperty_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); extern int drm_mode_getblob_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); +extern int drm_mode_createblob_ioctl(struct drm_device *dev, + void *data, struct drm_file *file_priv); +extern int drm_mode_destroyblob_ioctl(struct drm_device *dev, + void *data, struct drm_file *file_priv); extern int drm_mode_connector_property_set_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); extern int drm_mode_getencoder(struct drm_device *dev, diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index ff6ef62..3801584 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -786,6 +786,8 @@ struct drm_prime_handle { #define DRM_IOCTL_MODE_OBJ_SETPROPERTY DRM_IOWR(0xBA, struct drm_mode_obj_set_property) #define DRM_IOCTL_MODE_CURSOR2 DRM_IOWR(0xBB, struct drm_mode_cursor2) #define DRM_IOCTL_MODE_ATOMIC DRM_IOWR(0xBC, struct drm_mode_atomic) +#define DRM_IOCTL_MODE_CREATEPROPBLOB DRM_IOWR(0xBD, struct drm_mode_create_blob) +#define DRM_IOCTL_MODE_DESTROYPROPBLOB DRM_IOWR(0xBE, struct drm_mode_destroy_blob)
/** * Device specific ioctls should only be in their respective headers diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index dbeba94..359107a 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -558,4 +558,24 @@ struct drm_mode_atomic { __u64 user_data; };
+/** + * Create a new 'blob' data property, copying length bytes from data pointer, + * and returning new blob ID. + */ +struct drm_mode_create_blob { + /** Pointer to data to copy. */ + __u64 data; + /** Length of data to copy. */ + __u32 length; + /** Return: new property ID. */ + __u32 blob_id; +}; + +/** + * Destroy a user-created blob property. + */ +struct drm_mode_destroy_blob { + __u32 blob_id; +}; + #endif
Add a new helper, to be used later for blob property management, that sets the mode for a CRTC state, as well as updating the CRTC enable/active state at the same time.
Signed-off-by: Daniel Stone daniels@collabora.com Tested-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/drm_atomic.c | 42 ++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 24 ++++++--------------- drivers/gpu/drm/drm_crtc_helper.c | 6 +++--- drivers/gpu/drm/i915/intel_display.c | 16 +++++++++----- include/drm/drm_atomic.h | 6 ++++++ 5 files changed, 68 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 3134f50..67c251f 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -290,6 +290,48 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state, EXPORT_SYMBOL(drm_atomic_get_crtc_state);
/** + * drm_atomic_set_mode_for_crtc - set mode for CRTC + * @state: the CRTC whose incoming state to update + * @mode: kernel-internal mode to use for the CRTC, or NULL to disable + * + * Set a mode (originating from the kernel) on the desired CRTC state, and + * update the CRTC state's enable and mode_changed parameters as necessary. + * If disabling, it will also set active to false. + */ +int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, + struct drm_display_mode *mode) +{ + /* Early return for no change. */ + if (!mode && !state->enable) + return 0; + if (mode && state->enable && + memcmp(&state->mode, mode, sizeof(*mode)) == 0) + return 0; + + state->mode_changed = true; + + if (mode) { + drm_mode_copy(&state->mode, mode); + state->enable = true; + } else { + memset(&state->mode, 0, sizeof(state->mode)); + state->enable = false; + state->active = false; + } + + if (state->enable) + DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n", + mode->name, state); + else + DRM_DEBUG_ATOMIC("Set [NOMODE] for CRTC state %p\n", + state); + + return 0; +} +EXPORT_SYMBOL(drm_atomic_set_mode_for_crtc); + + +/** * drm_atomic_crtc_set_property - set property on CRTC * @crtc: the drm CRTC to set a property on * @state: the state object to update with the new property value diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index a64bacd..70ce243 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -369,20 +369,6 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, struct drm_connector_state *connector_state; int i, ret;
- for_each_crtc_in_state(state, crtc, crtc_state, i) { - if (!drm_mode_equal(&crtc->state->mode, &crtc_state->mode)) { - DRM_DEBUG_ATOMIC("[CRTC:%d] mode changed\n", - crtc->base.id); - crtc_state->mode_changed = true; - } - - if (crtc->state->enable != crtc_state->enable) { - DRM_DEBUG_ATOMIC("[CRTC:%d] enable changed\n", - crtc->base.id); - crtc_state->mode_changed = true; - } - } - for_each_connector_in_state(state, connector, connector_state, i) { /* * This only sets crtc->mode_changed for routing changes, @@ -1607,8 +1593,9 @@ retry: WARN_ON(set->fb); WARN_ON(set->num_connectors);
- crtc_state->enable = false; - crtc_state->active = false; + ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL); + if (ret != 0) + goto fail;
ret = drm_atomic_set_crtc_for_plane(primary_state, NULL); if (ret != 0) @@ -1622,9 +1609,10 @@ retry: WARN_ON(!set->fb); WARN_ON(!set->num_connectors);
- crtc_state->enable = true; + ret = drm_atomic_set_mode_for_crtc(crtc_state, set->mode); + if (ret != 0) + goto fail; crtc_state->active = true; - drm_mode_copy(&crtc_state->mode, set->mode);
ret = drm_atomic_set_crtc_for_plane(primary_state, crtc); if (ret != 0) diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 9297a61..cc77a4f 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -937,10 +937,10 @@ int drm_helper_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mod crtc_state->crtc = crtc; }
- crtc_state->enable = true; crtc_state->planes_changed = true; - crtc_state->mode_changed = true; - drm_mode_copy(&crtc_state->mode, mode); + ret = drm_atomic_set_mode_for_crtc(crtc_state, mode); + if (ret) + goto out; drm_mode_copy(&crtc_state->adjusted_mode, adjusted_mode);
if (crtc_funcs->atomic_check) { diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 240092a..7e31972 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9896,7 +9896,9 @@ retry: if (ret) goto fail;
- drm_mode_copy(&crtc_state->base.mode, mode); + ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode); + if (ret) + goto fail;
if (intel_set_mode(crtc, state)) { DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n"); @@ -12494,8 +12496,11 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
crtc_state->base.enable = intel_crtc->new_enabled;
- if (&intel_crtc->base == crtc) - drm_mode_copy(&crtc_state->base.mode, &crtc->mode); + if (&intel_crtc->base == crtc) { + ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, + &crtc->mode); + WARN_ON(ret); + } }
intel_modeset_setup_plane_state(state, crtc, &crtc->mode, @@ -12633,8 +12638,9 @@ intel_modeset_stage_output_state(struct drm_device *dev, if (IS_ERR(crtc_state)) return PTR_ERR(crtc_state);
- if (set->mode) - drm_mode_copy(&crtc_state->mode, set->mode); + ret = drm_atomic_set_mode_for_crtc(crtc_state, set->mode); + if (ret) + return ret;
if (set->num_connectors) crtc_state->active = true; diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index e89db0c..55f4604 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -110,6 +110,12 @@ drm_atomic_get_existing_connector_state(struct drm_atomic_state *state, }
int __must_check +drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, + struct drm_display_mode *mode); +int __must_check +drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state, + struct drm_property_blob *blob); +int __must_check drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state, struct drm_crtc *crtc); void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
On Fri, May 22, 2015 at 01:34:52PM +0100, Daniel Stone wrote:
Add a new helper, to be used later for blob property management, that sets the mode for a CRTC state, as well as updating the CRTC enable/active state at the same time.
Signed-off-by: Daniel Stone daniels@collabora.com Tested-by: Sean Paul seanpaul@chromium.org
A few small nits below. -Daniel
drivers/gpu/drm/drm_atomic.c | 42 ++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 24 ++++++--------------- drivers/gpu/drm/drm_crtc_helper.c | 6 +++--- drivers/gpu/drm/i915/intel_display.c | 16 +++++++++----- include/drm/drm_atomic.h | 6 ++++++ 5 files changed, 68 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 3134f50..67c251f 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -290,6 +290,48 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state, EXPORT_SYMBOL(drm_atomic_get_crtc_state);
/**
- drm_atomic_set_mode_for_crtc - set mode for CRTC
- @state: the CRTC whose incoming state to update
- @mode: kernel-internal mode to use for the CRTC, or NULL to disable
- Set a mode (originating from the kernel) on the desired CRTC state, and
- update the CRTC state's enable and mode_changed parameters as necessary.
- If disabling, it will also set active to false.
Kerneldoc for return values is missing. Fairly important since many atomic functions can return -EDEADLK, but this one doesn (even not when all the patches are applied).
- */
+int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
struct drm_display_mode *mode)+{
- /* Early return for no change. */
- if (!mode && !state->enable)
return 0;- if (mode && state->enable &&
memcmp(&state->mode, mode, sizeof(*mode)) == 0)return 0;- state->mode_changed = true;
Layering violation: ->mode_changed is purely something for the benefit of drivers/helpers and not something the core manadates. Hence this should get removed imo.
- if (mode) {
drm_mode_copy(&state->mode, mode);state->enable = true;- } else {
memset(&state->mode, 0, sizeof(state->mode));state->enable = false;state->active = false;- }
- if (state->enable)
DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",mode->name, state);- else
DRM_DEBUG_ATOMIC("Set [NOMODE] for CRTC state %p\n",state);- return 0;
+} +EXPORT_SYMBOL(drm_atomic_set_mode_for_crtc);
+/**
- drm_atomic_crtc_set_property - set property on CRTC
- @crtc: the drm CRTC to set a property on
- @state: the state object to update with the new property value
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index a64bacd..70ce243 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -369,20 +369,6 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, struct drm_connector_state *connector_state; int i, ret;
- for_each_crtc_in_state(state, crtc, crtc_state, i) {
if (!drm_mode_equal(&crtc->state->mode, &crtc_state->mode)) {DRM_DEBUG_ATOMIC("[CRTC:%d] mode changed\n",crtc->base.id);crtc_state->mode_changed = true;}if (crtc->state->enable != crtc_state->enable) {DRM_DEBUG_ATOMIC("[CRTC:%d] enable changed\n",crtc->base.id);crtc_state->mode_changed = true;}- }
Ofc then this hunk here needs to stay.
- for_each_connector_in_state(state, connector, connector_state, i) { /*
- This only sets crtc->mode_changed for routing changes,
@@ -1607,8 +1593,9 @@ retry: WARN_ON(set->fb); WARN_ON(set->num_connectors);
crtc_state->enable = false;crtc_state->active = false;
ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);if (ret != 0)goto fail;ret = drm_atomic_set_crtc_for_plane(primary_state, NULL); if (ret != 0)
@@ -1622,9 +1609,10 @@ retry: WARN_ON(!set->fb); WARN_ON(!set->num_connectors);
- crtc_state->enable = true;
- ret = drm_atomic_set_mode_for_crtc(crtc_state, set->mode);
- if (ret != 0)
goto fail;
drm_mode_copy(&crtc_state->mode, set->mode);
ret = drm_atomic_set_crtc_for_plane(primary_state, crtc); if (ret != 0)
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 9297a61..cc77a4f 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -937,10 +937,10 @@ int drm_helper_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mod crtc_state->crtc = crtc; }
- crtc_state->enable = true; crtc_state->planes_changed = true;
- crtc_state->mode_changed = true;
- drm_mode_copy(&crtc_state->mode, mode);
ret = drm_atomic_set_mode_for_crtc(crtc_state, mode);
if (ret)
goto out;drm_mode_copy(&crtc_state->adjusted_mode, adjusted_mode);
if (crtc_funcs->atomic_check) {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 240092a..7e31972 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9896,7 +9896,9 @@ retry: if (ret) goto fail;
- drm_mode_copy(&crtc_state->base.mode, mode);
ret = drm_atomic_set_mode_for_crtc(&crtc_state->base, mode);
if (ret)
goto fail;if (intel_set_mode(crtc, state)) { DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
@@ -12494,8 +12496,11 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
crtc_state->base.enable = intel_crtc->new_enabled;
if (&intel_crtc->base == crtc)drm_mode_copy(&crtc_state->base.mode, &crtc->mode);
if (&intel_crtc->base == crtc) {ret = drm_atomic_set_mode_for_crtc(&crtc_state->base,&crtc->mode);WARN_ON(ret);}}
intel_modeset_setup_plane_state(state, crtc, &crtc->mode,
@@ -12633,8 +12638,9 @@ intel_modeset_stage_output_state(struct drm_device *dev, if (IS_ERR(crtc_state)) return PTR_ERR(crtc_state);
- if (set->mode)
drm_mode_copy(&crtc_state->mode, set->mode);
ret = drm_atomic_set_mode_for_crtc(crtc_state, set->mode);
if (ret)
return ret;if (set->num_connectors) crtc_state->active = true;
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index e89db0c..55f4604 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -110,6 +110,12 @@ drm_atomic_get_existing_connector_state(struct drm_atomic_state *state, }
int __must_check +drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
struct drm_display_mode *mode);+int __must_check +drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
struct drm_property_blob *blob);+int __must_check drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state, struct drm_crtc *crtc); void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state, -- 2.4.1
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Add a blob property tracking the current mode to the CRTC state, and ensure it is properly updated and referenced.
Signed-off-by: Daniel Stone daniels@collabora.com Tested-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/drm_atomic.c | 62 +++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 11 ++++--- drivers/gpu/drm/drm_crtc.c | 3 +- include/drm/drm_crtc.h | 3 ++ 4 files changed, 73 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 67c251f..63fc58a 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -301,6 +301,8 @@ EXPORT_SYMBOL(drm_atomic_get_crtc_state); int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, struct drm_display_mode *mode) { + struct drm_mode_modeinfo umode; + /* Early return for no change. */ if (!mode && !state->enable) return 0; @@ -308,9 +310,20 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, memcmp(&state->mode, mode, sizeof(*mode)) == 0) return 0;
+ if (state->mode_blob) + drm_property_unreference_blob(state->mode_blob); + state->mode_blob = NULL; state->mode_changed = true;
if (mode) { + drm_mode_convert_to_umode(&umode, mode); + state->mode_blob = + drm_property_create_blob(state->crtc->dev, + sizeof(umode), + &umode); + if (!state->mode_blob) + return -ENOMEM; + drm_mode_copy(&state->mode, mode); state->enable = true; } else { @@ -330,6 +343,55 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, } EXPORT_SYMBOL(drm_atomic_set_mode_for_crtc);
+/** + * drm_atomic_set_mode_prop_for_crtc - set mode for CRTC + * @state: the CRTC whose incoming state to update + * @blob: pointer to blob property to use for mode + * + * Set a mode (originating from a blob property) on the desired CRTC state. + * This function will take a reference on the blob property for the CRTC state, + * and release the reference held on the state's existing mode property, if any + * was set. + */ +int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state, + struct drm_property_blob *blob) +{ + if (blob == state->mode_blob) + return 0; + + if (state->mode_blob) + drm_property_unreference_blob(state->mode_blob); + state->mode_blob = NULL; + + if (blob) { + if (blob->length != sizeof(struct drm_mode_modeinfo) || + drm_mode_convert_umode(&state->mode, + (const struct drm_mode_modeinfo *) + blob->data)) + return -EINVAL; + + state->mode_blob = drm_property_reference_blob(blob); + state->mode_changed = true; + state->enable = true; + } else { + memset(&state->mode, 0, sizeof(state->mode)); + + if (state->enable) + state->mode_changed = true; + state->enable = false; + state->active = false; + } + + if (state->enable) + DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n", + state->mode.name, state); + else + DRM_DEBUG_ATOMIC("Set [NOMODE] for CRTC state %p\n", + state); + + return 0; +} +EXPORT_SYMBOL(drm_atomic_set_mode_prop_for_crtc);
/** * drm_atomic_crtc_set_property - set property on CRTC diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 70ce243..5d596d9 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2027,6 +2027,8 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_dpms); */ void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc) { + if (crtc->state && crtc->state->mode_blob) + drm_property_unreference_blob(crtc->state->mode_blob); kfree(crtc->state); crtc->state = kzalloc(sizeof(*crtc->state), GFP_KERNEL);
@@ -2048,6 +2050,8 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc, { memcpy(state, crtc->state, sizeof(*state));
+ if (state->mode_blob) + drm_property_reference_blob(state->mode_blob); state->mode_changed = false; state->active_changed = false; state->planes_changed = false; @@ -2090,11 +2094,8 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state); void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc, struct drm_crtc_state *state) { - /* - * This is currently a placeholder so that drivers that subclass the - * state will automatically do the right thing if code is ever added - * to this function. - */ + if (state->mode_blob) + drm_property_unreference_blob(state->mode_blob); } EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 9263b22..ee87045 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1961,7 +1961,8 @@ int drm_mode_getcrtc(struct drm_device *dev, crtc_resp->x = crtc->primary->state->src_x >> 16; crtc_resp->y = crtc->primary->state->src_y >> 16; if (crtc->state->enable) { - drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode); + memcpy(&crtc_resp->mode, &crtc->state->mode_blob->data, + sizeof(crtc_resp->mode)); crtc_resp->mode_valid = 1;
} else { diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 72b60db..c54fa4a 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -299,6 +299,9 @@ struct drm_crtc_state {
struct drm_display_mode mode;
+ /* blob property to expose current mode to atomic userspace */ + struct drm_property_blob *mode_blob; + struct drm_pending_vblank_event *event;
struct drm_atomic_state *state;
On Fri, May 22, 2015 at 01:34:53PM +0100, Daniel Stone wrote:
Add a blob property tracking the current mode to the CRTC state, and ensure it is properly updated and referenced.
Signed-off-by: Daniel Stone daniels@collabora.com Tested-by: Sean Paul seanpaul@chromium.org
drivers/gpu/drm/drm_atomic.c | 62 +++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 11 ++++--- drivers/gpu/drm/drm_crtc.c | 3 +- include/drm/drm_crtc.h | 3 ++ 4 files changed, 73 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 67c251f..63fc58a 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -301,6 +301,8 @@ EXPORT_SYMBOL(drm_atomic_get_crtc_state); int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, struct drm_display_mode *mode) {
- struct drm_mode_modeinfo umode;
- /* Early return for no change. */ if (!mode && !state->enable) return 0;
@@ -308,9 +310,20 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, memcmp(&state->mode, mode, sizeof(*mode)) == 0) return 0;
if (state->mode_blob)
drm_property_unreference_blob(state->mode_blob);state->mode_blob = NULL; state->mode_changed = true;
if (mode) {
drm_mode_convert_to_umode(&umode, mode);state->mode_blob =drm_property_create_blob(state->crtc->dev,sizeof(umode),&umode);if (!state->mode_blob)return -ENOMEM;
IS_ERR and PTR_ERR? Dan Carpenter will catch this as soon as I'll apply ;-)
- drm_mode_copy(&state->mode, mode); state->enable = true; } else {
@@ -330,6 +343,55 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, } EXPORT_SYMBOL(drm_atomic_set_mode_for_crtc);
+/**
- drm_atomic_set_mode_prop_for_crtc - set mode for CRTC
- @state: the CRTC whose incoming state to update
- @blob: pointer to blob property to use for mode
- Set a mode (originating from a blob property) on the desired CRTC state.
- This function will take a reference on the blob property for the CRTC state,
- and release the reference held on the state's existing mode property, if any
- was set.
- */
+int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
struct drm_property_blob *blob)+{
- if (blob == state->mode_blob)
return 0;- if (state->mode_blob)
drm_property_unreference_blob(state->mode_blob);- state->mode_blob = NULL;
- if (blob) {
if (blob->length != sizeof(struct drm_mode_modeinfo) ||drm_mode_convert_umode(&state->mode,(const struct drm_mode_modeinfo *)blob->data))return -EINVAL;state->mode_blob = drm_property_reference_blob(blob);state->mode_changed = true;state->enable = true;- } else {
memset(&state->mode, 0, sizeof(state->mode));if (state->enable)state->mode_changed = true;
Same comment as in the previous patch: mode_changed isnt' the core atomic code's business.
state->enable = false;state->active = false;- }
- if (state->enable)
DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",state->mode.name, state);- else
DRM_DEBUG_ATOMIC("Set [NOMODE] for CRTC state %p\n",state);- return 0;
+} +EXPORT_SYMBOL(drm_atomic_set_mode_prop_for_crtc);
/**
- drm_atomic_crtc_set_property - set property on CRTC
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 70ce243..5d596d9 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2027,6 +2027,8 @@ EXPORT_SYMBOL(drm_atomic_helper_connector_dpms); */ void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc) {
- if (crtc->state && crtc->state->mode_blob)
drm_property_unreference_blob(crtc->state->mode_blob);@@ -2048,6 +2050,8 @@ void __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc, { memcpy(state, crtc->state, sizeof(*state));
- if (state->mode_blob)
drm_property_reference_blob(state->mode_blob);@@ -2090,11 +2094,8 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state); void __drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc, struct drm_crtc_state *state) {
- /*
* This is currently a placeholder so that drivers that subclass the* state will automatically do the right thing if code is ever added* to this function.*/
- if (state->mode_blob)
drm_property_unreference_blob(state->mode_blob);} EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 9263b22..ee87045 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -1961,7 +1961,8 @@ int drm_mode_getcrtc(struct drm_device *dev, crtc_resp->x = crtc->primary->state->src_x >> 16; crtc_resp->y = crtc->primary->state->src_y >> 16; if (crtc->state->enable) {
drm_mode_convert_to_umode(&crtc_resp->mode, &crtc->state->mode);
memcpy(&crtc_resp->mode, &crtc->state->mode_blob->data,sizeof(crtc_resp->mode));
Why this change? While transitioning I'd expect that some driver (probably the one with hw state readout) will get this wrong, imo safer to keep looking at crtc->state->mode.
lgtm otherwise. -Daniel
crtc_resp->mode_valid = 1; } else {diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 72b60db..c54fa4a 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -299,6 +299,9 @@ struct drm_crtc_state {
struct drm_display_mode mode;
/* blob property to expose current mode to atomic userspace */
struct drm_property_blob *mode_blob;
struct drm_pending_vblank_event *event;
struct drm_atomic_state *state;
-- 2.4.1
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Atomic modesetting: now with modesetting support.
Signed-off-by: Daniel Stone daniels@collabora.com Tested-by: Sean Paul seanpaul@chromium.org --- drivers/gpu/drm/drm_atomic.c | 12 +++++++++++- drivers/gpu/drm/drm_crtc.c | 7 +++++++ include/drm/drm_crtc.h | 1 + 3 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 63fc58a..a4ab03a 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -415,10 +415,18 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, { struct drm_device *dev = crtc->dev; struct drm_mode_config *config = &dev->mode_config; + int ret;
- /* FIXME: Mode prop is missing, which also controls ->enable. */ if (property == config->prop_active) state->active = val; + else if (property == config->prop_mode_id) { + struct drm_property_blob *mode = + drm_property_lookup_blob(dev, val); + ret = drm_atomic_set_mode_prop_for_crtc(state, mode); + if (mode) + drm_property_unreference_blob(mode); + return ret; + } else if (crtc->funcs->atomic_set_property) return crtc->funcs->atomic_set_property(crtc, state, property, val); else @@ -443,6 +451,8 @@ int drm_atomic_crtc_get_property(struct drm_crtc *crtc,
if (property == config->prop_active) *val = state->active; + else if (property == config->prop_mode_id) + *val = (state->mode_blob) ? state->mode_blob->base.id : 0; else if (crtc->funcs->atomic_get_property) return crtc->funcs->atomic_get_property(crtc, state, property, val); else diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index ee87045..8f18412 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -688,6 +688,7 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
if (drm_core_check_feature(dev, DRIVER_ATOMIC)) { drm_object_attach_property(&crtc->base, config->prop_active, 0); + drm_object_attach_property(&crtc->base, config->prop_mode_id, 0); }
return 0; @@ -1454,6 +1455,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.prop_active = prop;
+ prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC, + "MODE_ID", DRM_MODE_OBJECT_BLOB); + if (!prop) + return -ENOMEM; + dev->mode_config.prop_mode_id = prop; + return 0; }
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index c54fa4a..3b4d8a4 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1146,6 +1146,7 @@ struct drm_mode_config { struct drm_property *prop_fb_id; struct drm_property *prop_crtc_id; struct drm_property *prop_active; + struct drm_property *prop_mode_id;
/* DVI-I properties */ struct drm_property *dvi_i_subconnector_property;
On Fri, May 22, 2015 at 01:34:54PM +0100, Daniel Stone wrote:
Atomic modesetting: now with modesetting support.
Signed-off-by: Daniel Stone daniels@collabora.com Tested-by: Sean Paul seanpaul@chromium.org
drivers/gpu/drm/drm_atomic.c | 12 +++++++++++- drivers/gpu/drm/drm_crtc.c | 7 +++++++ include/drm/drm_crtc.h | 1 + 3 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 63fc58a..a4ab03a 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -415,10 +415,18 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc, { struct drm_device *dev = crtc->dev; struct drm_mode_config *config = &dev->mode_config;
- int ret;
- /* FIXME: Mode prop is missing, which also controls ->enable. */ if (property == config->prop_active) state->active = val;
- else if (property == config->prop_mode_id) {
struct drm_property_blob *mode =drm_property_lookup_blob(dev, val);ret = drm_atomic_set_mode_prop_for_crtc(state, mode);if (mode)drm_property_unreference_blob(mode);
Hm, maybe I need to revisit whether auto-clamping ->active is a good idea. We need it for legacy helpers, but for atomic userspace this code means depending upon whether active or mode_id is first in the prop list it will get clamped or not, which isn't awesome.
Imo that's a good reason to remove the ->active clamping from set_mode_pop_for_crtc. I guess we can keep it for set_mode_for_crtc since that's only used internally and in legacy paths. Perhaps with a comment as to why (and why not in set_mode_prop).
return ret;- } else if (crtc->funcs->atomic_set_property) return crtc->funcs->atomic_set_property(crtc, state, property, val); else
@@ -443,6 +451,8 @@ int drm_atomic_crtc_get_property(struct drm_crtc *crtc,
if (property == config->prop_active) *val = state->active;
- else if (property == config->prop_mode_id)
*val = (state->mode_blob) ? state->mode_blob->base.id : 0;diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index ee87045..8f18412 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -688,6 +688,7 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc,
if (drm_core_check_feature(dev, DRIVER_ATOMIC)) { drm_object_attach_property(&crtc->base, config->prop_active, 0);
drm_object_attach_property(&crtc->base, config->prop_mode_id, 0);}
return 0;
@@ -1454,6 +1455,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.prop_active = prop;
- prop = drm_property_create_object(dev, DRM_MODE_PROP_ATOMIC,
"MODE_ID", DRM_MODE_OBJECT_BLOB);
Ah, here we go. Why not DRM_MODE_PROP_BLOB? Imo confusing to userspace that some prop blobs are objects and some are old blob props. -Daniel
- if (!prop)
return -ENOMEM;- dev->mode_config.prop_mode_id = prop;
- return 0;
}
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index c54fa4a..3b4d8a4 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1146,6 +1146,7 @@ struct drm_mode_config { struct drm_property *prop_fb_id; struct drm_property *prop_crtc_id; struct drm_property *prop_active;
struct drm_property *prop_mode_id;
/* DVI-I properties */ struct drm_property *dvi_i_subconnector_property;
-- 2.4.1
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi,
On 22 May 2015 at 15:34, Daniel Vetter daniel@ffwll.ch wrote:
On Fri, May 22, 2015 at 01:34:54PM +0100, Daniel Stone wrote:
/* FIXME: Mode prop is missing, which also controls ->enable. */ if (property == config->prop_active) state->active = val;
else if (property == config->prop_mode_id) {struct drm_property_blob *mode =drm_property_lookup_blob(dev, val);ret = drm_atomic_set_mode_prop_for_crtc(state, mode);if (mode)drm_property_unreference_blob(mode);Hm, maybe I need to revisit whether auto-clamping ->active is a good idea. We need it for legacy helpers, but for atomic userspace this code means depending upon whether active or mode_id is first in the prop list it will get clamped or not, which isn't awesome.
Imo that's a good reason to remove the ->active clamping from set_mode_pop_for_crtc. I guess we can keep it for set_mode_for_crtc since that's only used internally and in legacy paths. Perhaps with a comment as to why (and why not in set_mode_prop).
No argument to not touching mode_changed, but I'm a bit confused about this one.
We don't touch ->active when setting a mode (i.e. if active is true and you change MODE_ID without changing the ACTIVE prop, active remains true; if active is false and you change MODE_ID, active remains false, but it gains a mode).
I've been working on the following assumption: - enable is a proxy for having a valid mode (enable == !!MODE_ID) - active cannot be true without enable also being true
Setting MODE_ID to 0 removes the current mode, and when it becomes 0, we can no longer report back a mode that we're scanning out. So how would we have active == true (a particular mode is enabled and being displayed), with no mode? Setting MODE_ID == 0 and ACTIVE == true in the same request is a broken configuration which should be rejected. Setting ACTIVE == true, MODE_ID == 0, MODE_ID == some_mode, is not only pretty pathological but impossible with current libdrm, but you're right that it should be respected.
So, I guess the way forward would be to not clamp either active or enable, check that the dependencies (active -> enable -> MODE_ID) are satisfied in drm_atomic_helper_check, and hope that everyone implementing their own check gets it right too.
Sound good?
Cheers, Daniel
On Mon, May 25, 2015 at 02:06:13PM +0100, Daniel Stone wrote:
On 22 May 2015 at 15:34, Daniel Vetter daniel@ffwll.ch wrote:
On Fri, May 22, 2015 at 01:34:54PM +0100, Daniel Stone wrote:
/* FIXME: Mode prop is missing, which also controls ->enable. */ if (property == config->prop_active) state->active = val;
else if (property == config->prop_mode_id) {struct drm_property_blob *mode =drm_property_lookup_blob(dev, val);ret = drm_atomic_set_mode_prop_for_crtc(state, mode);if (mode)drm_property_unreference_blob(mode);Hm, maybe I need to revisit whether auto-clamping ->active is a good idea. We need it for legacy helpers, but for atomic userspace this code means depending upon whether active or mode_id is first in the prop list it will get clamped or not, which isn't awesome.
Imo that's a good reason to remove the ->active clamping from set_mode_pop_for_crtc. I guess we can keep it for set_mode_for_crtc since that's only used internally and in legacy paths. Perhaps with a comment as to why (and why not in set_mode_prop).
No argument to not touching mode_changed, but I'm a bit confused about this one.
We don't touch ->active when setting a mode (i.e. if active is true and you change MODE_ID without changing the ACTIVE prop, active remains true; if active is false and you change MODE_ID, active remains false, but it gains a mode).
I've been working on the following assumption:
- enable is a proxy for having a valid mode (enable == !!MODE_ID)
- active cannot be true without enable also being true
Setting MODE_ID to 0 removes the current mode, and when it becomes 0, we can no longer report back a mode that we're scanning out. So how would we have active == true (a particular mode is enabled and being displayed), with no mode? Setting MODE_ID == 0 and ACTIVE == true in the same request is a broken configuration which should be rejected. Setting ACTIVE == true, MODE_ID == 0, MODE_ID == some_mode, is not only pretty pathological but impossible with current libdrm, but you're right that it should be respected.
So, I guess the way forward would be to not clamp either active or enable, check that the dependencies (active -> enable -> MODE_ID) are satisfied in drm_atomic_helper_check, and hope that everyone implementing their own check gets it right too.
Just to summarize the irc discussion: - ->enable is a derived state and should == !!MODE_ID - ->active is it's own independent atomic property, and as a rule we shouldn't try to "intelligently" patch up what userspace passes in, but instead just reject invalid configurations. Hence no auto-clamping for active, but proper computation of enable - There's no risk that some drivers will get the active vs. enable checks wrong, they're core drm code: Everything in drm_atomic.c is non-optional (and that's the hunk where you've removed the check). Maybe that was part of the confusion, since you're description above sounds like you put this in the helper library?
-Daniel
On Fri, May 22, 2015 at 01:34:43PM +0100, Daniel Stone wrote:
Hi, Building on the blob-property submission series[0], this provides a MODE_ID property that can be used to set the current mode for a CRTC under atomic modesetting, plus a way for users to create blob properties so they can specify their own modes.
Using this[1] I've been able to validate the atomic userspace API with a ported version of Weston[2]. This is currently undergoing review, but seems pretty OK. Both modesetting for every output together, and per-output async pageflips with events, works as well as the driver does.
Also tested with fbcon and X11, on Broadwell.
A few nitpicks on some patches, all the others merged to topic/drm-misc. -Daniel
Cheers, Daniel
[1]: And a pile of other work in the i915 driver from Maarten/Ander, and my own bits on top; see git://git.collabora.co.uk/git/user/daniels/linux in the wip/4.1.x/blob-modes-i915 branch. [2]: http://lists.freedesktop.org/archives/wayland-devel/2015-May/022106.html
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Daniel Stone -
Daniel Stone -
Daniel Vetter