| Message ID | 1347246202-24249-2-git-send-email-rob.clark@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Sun, 9 Sep 2012 22:03:14 -0500 Rob Clark <rob.clark@linaro.org> 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); I think the above is more suited to drm_crtc_helper code. I think the top level callback should contain the arrays and be a single "multi flip" hook (or maybe a check then set double callback). For some drivers that'll end up being a lot simpler, rather than sprinkling atomic handling code in all the set_property callbacks. Having a transactional API just seems a little messier with both the atomic state and per-property state to track and rollback in the case of failure.
On Wed, Sep 12, 2012 at 11:57 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > On Sun, 9 Sep 2012 22:03:14 -0500 > Rob Clark <rob.clark@linaro.org> 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); > > I think the above is more suited to drm_crtc_helper code. I think the > top level callback should contain the arrays and be a single "multi > flip" hook (or maybe a check then set double callback). For some > drivers that'll end up being a lot simpler, rather than sprinkling > atomic handling code in all the set_property callbacks. well, there are a few other places in drm_crtc.c where I want to use the new API, to avoid drivers having to support both atomic API and old set_plane/page_flip stuff.. the transactional API makes that a bit easier, I think.. or at least I don't have to construct an array on the stack. But I'm not entirely convinced that it is a problem.. with drm_{crtc,plane,etc}_state, it is just building up a set of values in a state struct, and that state struct gets atomically committed. > Having a transactional API just seems a little messier with both the > atomic state and per-property state to track and rollback in the case > of failure. For the rollback, I think I'm just going to move the array of property values into drm_{crtc,plane,etc}_state. That way, userspace doesn't see updated property values if they are not committed. It makes the property value rollback automatic. BR, -R > -- > Jesse Barnes, Intel Open Source Technology Center > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Sep 12, 2012 at 12:35:01PM -0500, Rob Clark wrote: > On Wed, Sep 12, 2012 at 11:57 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > On Sun, 9 Sep 2012 22:03:14 -0500 > > Rob Clark <rob.clark@linaro.org> 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); > > > > I think the above is more suited to drm_crtc_helper code. I think the > > top level callback should contain the arrays and be a single "multi > > flip" hook (or maybe a check then set double callback). For some > > drivers that'll end up being a lot simpler, rather than sprinkling > > atomic handling code in all the set_property callbacks. > > well, there are a few other places in drm_crtc.c where I want to use > the new API, to avoid drivers having to support both atomic API and > old set_plane/page_flip stuff.. the transactional API makes that a bit > easier, I think.. or at least I don't have to construct an array on > the stack. Usually you would need to build up the full state anyway before you can tell if it's good or bad. I don't see what some big array would buy here. > > Having a transactional API just seems a little messier with both the > > atomic state and per-property state to track and rollback in the case > > of failure. > > For the rollback, I think I'm just going to move the array of property > values into drm_{crtc,plane,etc}_state. That way, userspace doesn't > see updated property values if they are not committed. It makes the > property value rollback automatic. This was my original idea as well. Separate the current and future states cleanly. At the moment it's all mixed up inside the objects somewhere. I sort of abandoned the idea so that I could avoid touching too much driver code while prototyping the atomic modesetting, but that's certainly the way I would design the system.
On Wed, Sep 12, 2012 at 1:03 PM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Wed, Sep 12, 2012 at 12:35:01PM -0500, Rob Clark wrote: >> On Wed, Sep 12, 2012 at 11:57 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: >> > On Sun, 9 Sep 2012 22:03:14 -0500 >> > Rob Clark <rob.clark@linaro.org> 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); >> > >> > I think the above is more suited to drm_crtc_helper code. I think the >> > top level callback should contain the arrays and be a single "multi >> > flip" hook (or maybe a check then set double callback). For some >> > drivers that'll end up being a lot simpler, rather than sprinkling >> > atomic handling code in all the set_property callbacks. >> >> well, there are a few other places in drm_crtc.c where I want to use >> the new API, to avoid drivers having to support both atomic API and >> old set_plane/page_flip stuff.. the transactional API makes that a bit >> easier, I think.. or at least I don't have to construct an array on >> the stack. > > Usually you would need to build up the full state anyway before you can > tell if it's good or bad. I don't see what some big array would buy here. yup.. this was why I added drm_{crtc,plane,etc}_check_state(), to bring back the common state checking that used to be done in the ioctl fxns >> > Having a transactional API just seems a little messier with both the >> > atomic state and per-property state to track and rollback in the case >> > of failure. >> >> For the rollback, I think I'm just going to move the array of property >> values into drm_{crtc,plane,etc}_state. That way, userspace doesn't >> see updated property values if they are not committed. It makes the >> property value rollback automatic. > > This was my original idea as well. Separate the current and future > states cleanly. At the moment it's all mixed up inside the objects > somewhere. I sort of abandoned the idea so that I could avoid touching > too much driver code while prototyping the atomic modesetting, but > that's certainly the way I would design the system. Yeah, I don't see any other way to do it sanely other than separating out the current/future state. Fortunately for planes, there are only a few drivers that support planes so it isn't too bad. For full modesetting, it gets to be a lot of search and replace. But it makes it so much cleaner so I think it is worth doing. We could probably also simplify a bunch of the crtc helper code that handles reverting to previous state. BR, -R > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, 12 Sep 2012 12:35:01 -0500 Rob Clark <rob.clark@linaro.org> wrote: > On Wed, Sep 12, 2012 at 11:57 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > > On Sun, 9 Sep 2012 22:03:14 -0500 > > Rob Clark <rob.clark@linaro.org> 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); > > > > I think the above is more suited to drm_crtc_helper code. I think the > > top level callback should contain the arrays and be a single "multi > > flip" hook (or maybe a check then set double callback). For some > > drivers that'll end up being a lot simpler, rather than sprinkling > > atomic handling code in all the set_property callbacks. > > well, there are a few other places in drm_crtc.c where I want to use > the new API, to avoid drivers having to support both atomic API and > old set_plane/page_flip stuff.. the transactional API makes that a bit > easier, I think.. or at least I don't have to construct an array on > the stack. > > But I'm not entirely convinced that it is a problem.. with > drm_{crtc,plane,etc}_state, it is just building up a set of values in > a state struct, and that state struct gets atomically committed. Yeah I know it can work this way, it just seemed like the begin, end, and set_property callbacks might be unnecessary if the props were all part of the state. The driver call roll things back (or just not touch hw) if the check or commit fails... I guess ultimately, given the choice, I'd rather have the drivers calling into helper functions where possible, rather than having the core impose a specific set of semi-fine grained hooks. > > Having a transactional API just seems a little messier with both the > > atomic state and per-property state to track and rollback in the case > > of failure. > > For the rollback, I think I'm just going to move the array of property > values into drm_{crtc,plane,etc}_state. That way, userspace doesn't > see updated property values if they are not committed. It makes the > property value rollback automatic. Ok that seems reasonable.
On Wed, Sep 12, 2012 at 2:05 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: > On Wed, 12 Sep 2012 12:35:01 -0500 > Rob Clark <rob.clark@linaro.org> wrote: > >> On Wed, Sep 12, 2012 at 11:57 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote: >> > On Sun, 9 Sep 2012 22:03:14 -0500 >> > Rob Clark <rob.clark@linaro.org> 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); >> > >> > I think the above is more suited to drm_crtc_helper code. I think the >> > top level callback should contain the arrays and be a single "multi >> > flip" hook (or maybe a check then set double callback). For some >> > drivers that'll end up being a lot simpler, rather than sprinkling >> > atomic handling code in all the set_property callbacks. >> >> well, there are a few other places in drm_crtc.c where I want to use >> the new API, to avoid drivers having to support both atomic API and >> old set_plane/page_flip stuff.. the transactional API makes that a bit >> easier, I think.. or at least I don't have to construct an array on >> the stack. >> >> But I'm not entirely convinced that it is a problem.. with >> drm_{crtc,plane,etc}_state, it is just building up a set of values in >> a state struct, and that state struct gets atomically committed. > > Yeah I know it can work this way, it just seemed like the begin, end, > and set_property callbacks might be unnecessary if the props were all > part of the state. The driver call roll things back (or just not touch > hw) if the check or commit fails... > > I guess ultimately, given the choice, I'd rather have the drivers > calling into helper functions where possible, rather than having the > core impose a specific set of semi-fine grained hooks. well, that is is basically what is happening.. for example, the driver's set_property() code would, if the driver doesn't have any custom properties, basically just be: int xyz_set_property(crtc, state, property, val) { return drm_crtc_set_property(crtc, xyz_get_crtc_state(state, crtc->pipe), property, val); } so the driver basically just has to map the generic void *state to the appropriate 'struct drm_crtc_state *', and then call helpers. But the driver is also free to intercept property values, if needed. For example, with the private-plane setup in omapdrm, in the crtc I intercept the fb-id property and also set it on the crtc's private plane. I suppose you could move the for loop iterating over an array of properties into the driver. I'm not really sure what that buys you, since none of this is really applying state to hw at this stage. Plus I think we'd end up needing both fxn ptrs that take a single property plus one that takes an array. The part that is more important to give the driver flexibility is that point where you need to apply the state to the hw, and here the driver has complete control. Although perhaps there is some room for crtc-helpers to plug in below that for the modeset. BR, -R >> > Having a transactional API just seems a little messier with both the >> > atomic state and per-property state to track and rollback in the case >> > of failure. >> >> For the rollback, I think I'm just going to move the array of property >> values into drm_{crtc,plane,etc}_state. That way, userspace doesn't >> see updated property values if they are not committed. It makes the >> property value rollback automatic. > > Ok that seems reasonable. > > -- > Jesse Barnes, Intel Open Source Technology Center > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 7552675..40a3f21 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); + 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..96530af 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -933,6 +933,38 @@ 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. + */ + void *(*atomic_begin)(struct drm_device *dev); + + /** + * Check the state object to see if the requested state is + * physically possible. + */ + int (*atomic_check)(struct drm_device *dev, void *state); + + /** + * Commit the state. This will only be called if atomic_check() + * succeeds. + */ + int (*atomic_commit)(struct drm_device *dev, void *state, + struct drm_pending_vblank_event *event); + + /** + * Release resources associated with the 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); };
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. --- drivers/gpu/drm/drm_crtc.c | 126 +++++++++++++++++++++++++++----------------- include/drm/drmP.h | 32 +++++++++++ include/drm/drm_crtc.h | 8 +-- 3 files changed, 115 insertions(+), 51 deletions(-)