On Thu, Oct 11, 2012 at 2:26 PM, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Rob,
On Wednesday 12 September 2012 22:49:47 Rob Clark wrote:
From: Rob Clark rob@ti.com
The 'atomic' mechanism allows for multiple properties to be updated, checked, and commited atomically. This will be the basis of atomic- modeset and nuclear-pageflip.
The basic flow is:
state = dev->atomic_begin(); for (... one or more ...) obj->set_property(obj, state, prop, value); if (dev->atomic_check(state)) dev->atomic_commit(state, event); dev->atomic_end(state);
The split of check and commit steps is to allow for ioctls with a test-only flag (which would skip the commit step).
The atomic functions are mandatory, as they will end up getting called from enough places that it is easier not to have to bother with if-null checks everywhere.
TODO:
- add some stub atomic functions that can be used by drivers until they are updated to support atomic operations natively
- roll-back previous property values if check/commit fails, so userspace doesn't see incorrect values.
I have to confess that I have trouble understanding how the various pieces fit together. The call stacks are quite deep, and pointers to objects are passed around in a way to makes it difficult to understand what objects are accessed without going through the whole stack. Reviewing your proposal would be easier with proper documentation, including a functional overview of the atomic properties architecture. Renaming some methods would also be worth it: drm_connector_property_set_value() and drm_mode_connector_set_obj_prop() are confusing, and I would need pretty long to get used to them without having to look at the kerneldoc all the time.
Yeah, the drm_connector_property_* stuff is a bit of confusing legacy.. let's just get rid of it. I sent a patchset to do this, and I'll rebase next round of the atomic pagefip series on top.
Speaking of kerneldoc, there's none :-) Library functions need to be documented, especially with such a complex code flow.
Let me make an attempt to explain the code flow via email.. admittedly I tend to understand things better from code so maybe writing docs isn't my best strength. But if this makes sense then I'll fold it into the docs. If not let me know and I'll keep trying until it does make sense.
First, I should note, that I don't expect it will be possible to update all drivers in one go, so there will be a transition period with some drivers working the old way, and some supporting the "atomic" way. For old drivers, the .set_property() fxns would continue to function the way they do today (touching hw immediately, most likely). They will have the additional state parameter added, but will just ignore this. I'm going to add back in the .page_flip(), .set_plane(), .disable_plane(). For drivers that implement these fxn ptrs, instead of the atomic fxns, they won't see common crtc/plane attributes set via property. For old drivers, userspace will not see properties for common crtc/plane attributes (fb_id, crtc_x/y/w/h, etc). That isn't the case for the last version of the series I sent, but I think it is the only sane way to avoid having to port all drivers to "atomic" at once.
---
For drivers that have been atomic-ified, they no longer need to implement .page_flip(), .set_plane(), .disable_plane(). Instead they should implement the atomic fxns, and their various .set_property() functions would not actually touch the hw, but instead update state objects. State objects should hold all the state which could potentially be applied to the hw. The .set_property() functions should not actually update the hw, because it might be the users intention to only test a new configuration.
In drm_crtc / drm_plane, the userspace visible attributes are split out into separate drm_crtc_state / drm_plane_state structs. (Note that this is only partially done for crtc, only the page-flip related attributes are split out. I wanted to do this first, and then add the modeset related attributes in a later patch to split things up into smaller pieces.) The intention is that drivers can wrap the state structs, and add whatever other information they need. For example:
struct omap_plane_state { struct drm_plane_state base; uint8_t rotation; uint8_t zorder; uint8_t enabled; };
It doesn't strictly need to be just property values. If driver needs to calculate some clock settings, fifo thresholds, or other internal state, based the external state set from userspace, it could stuff that stuff into it's state structs as well if it wanted. But at a minimum the state objects should encapsulate the userspace visible state.
For atomic-ified drivers, all updates, whether it be userspace setproperty ioctl updating a single property, or legacy setplane or pageflip ioctls, or new atomic ioctl(s), all go through the same path from the driver's perspective:
state = dev->atomic_begin(); for (... one or more ...) obj->set_property(obj, state, prop, value); if (dev->atomic_check(state)) dev->atomic_commit(state, event); dev->atomic_end(state);
The global driver state token/object returned from .atomic_begin() is opaque from drm core perspective. It somehow encapsulates the state of all crtc/plane/etc. Inside the driver's different .set_property() fxns, this global state object gets mapped to per crtc/plane state objects. Core drm helpers for dealing with common attributes (fb_id, crtc_x/y/w/h, etc) are provided. (ie. drm_plane_set_property(), drm_plane_check_state(), etc.) This is to avoid forcing each driver to duplicate code for setting common properties and sanity checking.
After all the property update have been passed to the driver via .set_property() calls, .atomic_check() is called. This should check all the modified crtc/plane's (using drm_{plane,crtc}_check_state() for common check), and do any needed global sanity checks for the hw (ie. can this combination of planes meet hw bandwidth limitations, etc).
For ioctls with a 'test' flag, the .atomic_commit() step might be skipped if userspace is only testing the new configuration. In either case, .atomic_commit() is only called if .atomic_check() succeeds, so at this point it is already known that the new configuration is "good", so .atomic_commit() should really only fail for catastrophic things (unable to allocate memory, hardware is on fire, etc).
The .atomic_end() is called at the end if the driver needs to do some cleanup.
Before your patches the code currently called the drivers set_property methods to apply the state to the hardware, and then, if those calls were successful, called drm_object_property_set_value() to store the property values permanently.
Your patches modify that behaviour and changes the meaning of set_property. The method now just updates the new state object without touching the hardware. However, you still call drm_object_property_set_value() which stores the new property value in propvals (struct drm_object_property_values).
The property values array is moved into the state object. So the userspace visible property values only change as a result of .atomic_commit(). This way, each driver does not have to care about rollback or knowing when to update the userspace visible property values. It is all automatic, based on the current state object.
One of my initial concerns about using properties for everything is that I didn't want to make each driver have to deal w/ the "paperwork" of keeping userspace visible property values in sync with current hw state. Splitting the prop vals out into the state objects makes that problem go away.
Rolling back the values is a possible solution. As the mode_config.mutex is taken around all accesses to propvals, no other thread will be able to access incorrect values before they're rolled back. However, I wonder whether we couldn't come up with a more simple solution.
yup, putting prop vals into the state struct is that more simple solution :-P
First of all, do we really need dynamic states ? All accesses to properties are serialized using a single common mutex. We could just have two static states (current and new) instead of allocating the state at runtime.
The driver is free to implement things this way, since it is the one allocating state structs. I happened to pick dynamic allocation in omapdrm, but nothing in the API forces this.
Then, do we really need to parse the properties and compute the state values so early ? We're storing the properties values in two separate locations, in the propvals structure as "raw" unprocessed values, and also in the state structures as processed values. Is that really necessary ? Wouldn't it be simpler to first validate all the new properties and then compute the state values at commit time ?
the 'raw' propvals array is really just intended for getproperty ioclt, so that we aren't needing to have .get_property() fxn ptrs in the drivers.
I think you *somehow* need to parse the property values in order to validate the state. I guess the other alternative is ditch the .set_property() and instead update all the propval's and then have a .one_or_more_of_your_properties_has_changed() fxns to call in the driver. This seems more awkward for me, because then I have to go figure out in the driver *which* properties have changed. So I don't really see that as an improvement.
I can't really pinpoint the exact reason, but I feel like this is all getting a bit too messy for my taste. That might be partly caused by me being familiar with the V4L2 control framework, which solves some of the same issues (such as modifying several controls atomically) in a (in my opinion) cleaner way. The problem at hand in DRM might very well be more complex though.
Could you please include more documentation in the next version of this RFC ? I'd like text documentation that explains the design principles and how the pieces fit together (including code flows) (in plain text or DocBook, it doesn't matter much at this point), and kerneldoc for the exported functions.
Yeah, I knew you would ask for that. If my attempt at explaining this makes things clearer to you (and hopefully not more confusing), then I'll somehow re-work it into some text for the docbook
BR, -R
drivers/gpu/drm/drm_crtc.c | 126 +++++++++++++++++++++++++---------------- include/drm/drmP.h | 52 ++++++++++++++++++ include/drm/drm_crtc.h | 8 +-- 3 files changed, 135 insertions(+), 51 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 7552675..3a087cf 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3227,12 +3227,11 @@ int drm_mode_connector_property_set_ioctl(struct drm_device *dev, return drm_mode_obj_set_property_ioctl(dev, &obj_set_prop, file_priv); }
-static int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj,
struct drm_property *property,+static int drm_mode_connector_set_obj_prop(struct drm_connector *connector,
void *state, struct drm_property *property, uint64_t value){ int ret = -EINVAL;
struct drm_connector *connector = obj_to_connector(obj); /* Do DPMS ourselves */ if (property == connector->dev->mode_config.dpms_property) {@@ -3240,7 +3239,7 @@ static int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj, (*connector->funcs->dpms)(connector, (int)value); ret = 0; } else if (connector->funcs->set_property)
ret = connector->funcs->set_property(connector, property, value);
ret = connector->funcs->set_property(connector, state, property,value);
/* store the property value if successful */ if (!ret)@@ -3248,36 +3247,87 @@ static int drm_mode_connector_set_obj_prop(struct drm_mode_object *obj, return ret; }
-static int drm_mode_crtc_set_obj_prop(struct drm_mode_object *obj,
struct drm_property *property,+static int drm_mode_crtc_set_obj_prop(struct drm_crtc *crtc,
void *state, struct drm_property *property, uint64_t value){ int ret = -EINVAL;
struct drm_crtc *crtc = obj_to_crtc(obj); if (crtc->funcs->set_property)ret = crtc->funcs->set_property(crtc, property, value);
ret = crtc->funcs->set_property(crtc, state, property, value); if (!ret)
drm_object_property_set_value(obj, property, value);
drm_object_property_set_value(&crtc->base, property, value); return ret;}
-static int drm_mode_plane_set_obj_prop(struct drm_mode_object *obj,
struct drm_property *property,+static int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
void *state, struct drm_property *property, uint64_t value){ int ret = -EINVAL;
struct drm_plane *plane = obj_to_plane(obj); if (plane->funcs->set_property)ret = plane->funcs->set_property(plane, property, value);
ret = plane->funcs->set_property(plane, state, property, value); if (!ret)
drm_object_property_set_value(obj, property, value);
drm_object_property_set_value(&plane->base, property, value); return ret;}
+static int drm_mode_set_obj_prop(struct drm_device *dev,
struct drm_mode_object *obj, void *state,struct drm_property *property, uint64_t value)+{
if (drm_property_change_is_valid(property, value)) {switch (obj->type) {case DRM_MODE_OBJECT_CONNECTOR:return drm_mode_connector_set_obj_prop(obj_to_connector(obj),state, property, value);case DRM_MODE_OBJECT_CRTC:return drm_mode_crtc_set_obj_prop(obj_to_crtc(obj),state, property, value);case DRM_MODE_OBJECT_PLANE:return drm_mode_plane_set_obj_prop(obj_to_plane(obj),state, property, value);}}return -EINVAL;+}
+/* call with mode_config mutex held */ +static int drm_mode_set_obj_prop_id(struct drm_device *dev, void *state,
uint32_t obj_id, uint32_t obj_type,uint32_t prop_id, uint64_t value)+{
struct drm_mode_object *arg_obj;struct drm_mode_object *prop_obj;struct drm_property *property;int i;arg_obj = drm_mode_object_find(dev, obj_id, obj_type);if (!arg_obj)return -EINVAL;if (!arg_obj->properties)return -EINVAL;for (i = 0; i < arg_obj->properties->count; i++)if (arg_obj->properties->ids[i] == prop_id)break;if (i == arg_obj->properties->count)return -EINVAL;prop_obj = drm_mode_object_find(dev, prop_id,DRM_MODE_OBJECT_PROPERTY);if (!prop_obj)return -EINVAL;property = obj_to_property(prop_obj);return drm_mode_set_obj_prop(dev, arg_obj, state, property, value);+}
int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { @@ -3338,53 +3388,35 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_obj_set_property *arg = data;
struct drm_mode_object *arg_obj;struct drm_mode_object *prop_obj;struct drm_property *property;
void *state = NULL; int ret = -EINVAL;
int i; if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL; mutex_lock(&dev->mode_config.mutex);arg_obj = drm_mode_object_find(dev, arg->obj_id, arg->obj_type);if (!arg_obj)goto out;if (!arg_obj->properties)goto out;for (i = 0; i < arg_obj->properties->count; i++)if (arg_obj->properties->ids[i] == arg->prop_id)break;
state = dev->driver->atomic_begin(dev, NULL);if (IS_ERR(state)) {ret = PTR_ERR(state);goto out_unlock;}
if (i == arg_obj->properties->count)
ret = drm_mode_set_obj_prop_id(dev, state,arg->obj_id, arg->obj_type,arg->prop_id, arg->value);if (ret) goto out;
prop_obj = drm_mode_object_find(dev, arg->prop_id,DRM_MODE_OBJECT_PROPERTY);if (!prop_obj)goto out;property = obj_to_property(prop_obj);if (!drm_property_change_is_valid(property, arg->value))
ret = dev->driver->atomic_check(dev, state);if (ret) goto out;
switch (arg_obj->type) {case DRM_MODE_OBJECT_CONNECTOR:ret = drm_mode_connector_set_obj_prop(arg_obj, property,arg->value);break;case DRM_MODE_OBJECT_CRTC:ret = drm_mode_crtc_set_obj_prop(arg_obj, property, arg->value);break;case DRM_MODE_OBJECT_PLANE:ret = drm_mode_plane_set_obj_prop(arg_obj, property, arg->value);break;}
ret = dev->driver->atomic_commit(dev, state, NULL);out:
dev->driver->atomic_end(dev, state);+out_unlock: mutex_unlock(&dev->mode_config.mutex); return ret; } diff --git a/include/drm/drmP.h b/include/drm/drmP.h index d6b67bb..f719c4d 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -933,6 +933,58 @@ struct drm_driver { struct drm_device *dev, uint32_t handle);
/** Atomic functions:*//*** Begin a sequence of atomic property sets. Returns a driver* private state object that is passed back into the various* object's set_property() fxns, and into the remainder of the* atomic funcs. The state object should accumulate the changes* from one o more set_property()'s. At the end, the state can* be checked, and optionally committed.** \param dev dev DRM device handle.* \param crtc for asynchronous page-flip operations, the crtc* that is being updated. (The driver should return -EBUSY if* a page-flip is still pending.) Otherwise, NULL.* \returns a driver private state object, which is passed* back in to the various other atomic fxns*/void *(*atomic_begin)(struct drm_device *dev, struct drm_crtc *crtc);/*** Check the state object to see if the requested state is* physically possible.** \param dev dev DRM device handle.* \param state the driver private state object*/int (*atomic_check)(struct drm_device *dev, void *state);/*** Commit the state. This will only be called if atomic_check()* succeeds.** \param dev dev DRM device handle.* \param state the driver private state object* \param event for asynchronous page-flip operations, the* userspace has requested an event to be sent when the* page-flip completes, or NULL. Will always be NULL for* non-page-flip operations*/int (*atomic_commit)(struct drm_device *dev, void *state,struct drm_pending_vblank_event *event);/*** Release resources associated with the state object.** \param dev dev DRM device handle.* \param state the driver private state object*/void (*atomic_end)(struct drm_device *dev, void *state);/* Driver private ops for this object */ const struct vm_operations_struct *gem_vm_ops;diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 1422b36..a609190 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -357,7 +357,7 @@ struct drm_crtc_funcs { struct drm_framebuffer *fb, struct drm_pending_vblank_event *event);
int (*set_property)(struct drm_crtc *crtc,
int (*set_property)(struct drm_crtc *crtc, void *state, struct drm_property *property, uint64_t val);};
@@ -455,8 +455,8 @@ struct drm_connector_funcs { enum drm_connector_status (*detect)(struct drm_connector *connector, bool force); int (*fill_modes)(struct drm_connector *connector, uint32_t max_width, uint32_t max_height); - int (*set_property)(struct drm_connector *connector, struct drm_property *property, - uint64_t val);
int (*set_property)(struct drm_connector *connector, void *state,struct drm_property *property, uint64_t val); void (*destroy)(struct drm_connector *connector); void (*force)(struct drm_connector *connector);}; @@ -627,7 +627,7 @@ struct drm_plane_funcs { int (*disable_plane)(struct drm_plane *plane); void (*destroy)(struct drm_plane *plane);
int (*set_property)(struct drm_plane *plane,
int (*set_property)(struct drm_plane *plane, void *state, struct drm_property *property, uint64_t val);};
-- Regards,
Laurent Pinchart
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel