From: Rob Clark rob@ti.com
This is following a bit the approach that Ville is taking for atomic- modeset, in that it is switching over to using properties for everything. The advantage of this approach is that it makes it easier to add new attributes to set as part of a page-flip (and even opens the option to add new object types).
The basic principles are: a) split out object state (in this case, plane and crtc, although I expect more to be added as atomic-modeset is added) into seperate structures that can be atomically commited or rolled back b) expand the object property API (set_property()) to take a state object. The obj->set_property() simply updates the state object without actually applying the changes to the hw. c) after all the property updates are done, the updated state can be checked for correctness and against the hw capabilities, and then either discarded or committed atomically.
Since we need to include properties in the nuclear-pageflip scheme, doing everything via properties avoids updating a bunch of additional driver provided callbacks. Instead we just drop crtc->page_flip() and plane->update_plane(). By splitting out the object's mutable state into drm_{plane,crtc,etc}_state structs (which are wrapped by the individual drivers to add their own hw specific state), we can use some helpers (drm_{plane,crtc,etc}_set_property() and drm_{plane,crtc,etc}_check_state()) to keep core error checking in drm core and avoid pushing the burden of dealing with common properties to the individual drivers.
So far, I've only updated omapdrm to the new APIs, as a proof of concept. Only a few drivers support drm plane, so I expect the updates to convert drm-plane to properties should not be so hard. Possibly for crtc/pageflip we might need to have a transition period where we still support crtc->page_flip() code path until all drivers are updated.
My complete branch is here:
https://github.com/robclark/kernel-omap4/commits/drm_nuclear git://github.com/robclark/kernel-omap4.git drm_nuclear
Rob Clark (9): drm: add atomic fxns drm: add object property type drm: add DRM_MODE_PROP_DYNAMIC property flag drm: convert plane to properties drm: add drm_plane_state drm: convert page_flip to properties drm: add drm_crtc_state drm: nuclear pageflip drm/omap: update for atomic age
drivers/gpu/drm/drm_crtc.c | 769 +++++++++++++++++++++++---------- drivers/gpu/drm/drm_crtc_helper.c | 51 +-- drivers/gpu/drm/drm_drv.c | 1 + drivers/gpu/drm/drm_fb_helper.c | 11 +- drivers/staging/omapdrm/Makefile | 1 + drivers/staging/omapdrm/omap_atomic.c | 270 ++++++++++++ drivers/staging/omapdrm/omap_atomic.h | 52 +++ drivers/staging/omapdrm/omap_crtc.c | 247 +++++------ drivers/staging/omapdrm/omap_drv.c | 5 + drivers/staging/omapdrm/omap_drv.h | 35 +- drivers/staging/omapdrm/omap_fb.c | 44 +- drivers/staging/omapdrm/omap_plane.c | 270 ++++++------ include/drm/drm.h | 2 + include/drm/drmP.h | 32 ++ include/drm/drm_crtc.h | 140 ++++-- include/drm/drm_mode.h | 48 ++ 16 files changed, 1378 insertions(+), 600 deletions(-) create mode 100644 drivers/staging/omapdrm/omap_atomic.c create mode 100644 drivers/staging/omapdrm/omap_atomic.h
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(-)
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); };
On Sun, 9 Sep 2012 22:03:14 -0500 Rob Clark rob.clark@linaro.org wrote:
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:
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.
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
On Wed, Sep 12, 2012 at 12:35:01PM -0500, Rob Clark wrote:
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.
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:
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
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
On Wed, 12 Sep 2012 12:35:01 -0500 Rob Clark rob.clark@linaro.org wrote:
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.
Ok that seems reasonable.
On Wed, Sep 12, 2012 at 2:05 PM, Jesse Barnes jbarnes@virtuousgeek.org wrote:
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
From: Rob Clark rob@ti.com
An object property is an id (idr) for a drm mode object. This will allow a property to be used set/get a framebuffer, CRTC, etc. --- drivers/gpu/drm/drm_crtc.c | 28 ++++++++++++++++++++++++++++ include/drm/drm_crtc.h | 5 +++++ include/drm/drm_mode.h | 1 + 3 files changed, 34 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 40a3f21..12829de 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -2859,6 +2859,23 @@ struct drm_property *drm_property_create_range(struct drm_device *dev, int flags } EXPORT_SYMBOL(drm_property_create_range);
+struct drm_property *drm_property_create_object(struct drm_device *dev, + int flags, const char *name, uint32_t type) +{ + struct drm_property *property; + + flags |= DRM_MODE_PROP_OBJECT; + + property = drm_property_create(dev, flags, name, 1); + if (!property) + return NULL; + + property->values[0] = type; + + return property; +} +EXPORT_SYMBOL(drm_property_create_object); + int drm_property_add_enum(struct drm_property *property, int index, uint64_t value, const char *name) { @@ -3203,6 +3220,9 @@ static bool drm_property_change_is_valid(struct drm_property *property, for (i = 0; i < property->num_values; i++) valid_mask |= (1ULL << property->values[i]); return !(value & ~valid_mask); + } else if (property->flags & DRM_MODE_PROP_OBJECT) { + /* check for valid object or zero happens elsewhere */ + return true; } else { int i; for (i = 0; i < property->num_values; i++) @@ -3280,6 +3300,14 @@ static int drm_mode_set_obj_prop(struct drm_device *dev, struct drm_property *property, uint64_t value) { if (drm_property_change_is_valid(property, value)) { + /* a zero value for an object property translates to null: */ + if ((property->flags & DRM_MODE_PROP_OBJECT) && value) { + struct drm_mode_object *obj = + drm_mode_object_find(dev, value, property->values[0]); + if (!obj) + return -EINVAL; + value = VOID2U64(obj); + } switch (obj->type) { case DRM_MODE_OBJECT_CONNECTOR: return drm_mode_connector_set_obj_prop(obj_to_connector(obj), diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index a609190..92df2f4 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -61,6 +61,9 @@ struct drm_object_properties { uint64_t values[DRM_OBJECT_MAX_PROPERTY]; };
+#define U642VOID(x) ((void *)(unsigned long)(x)) +#define VOID2U64(x) ((uint64_t)(unsigned long)(x)) + /* * Note on terminology: here, for brevity and convenience, we refer to connector * control chips as 'CRTCs'. They can control any type of connector, VGA, LVDS, @@ -955,6 +958,8 @@ struct drm_property *drm_property_create_bitmask(struct drm_device *dev, struct drm_property *drm_property_create_range(struct drm_device *dev, int flags, const char *name, uint64_t min, uint64_t max); +struct drm_property *drm_property_create_object(struct drm_device *dev, + int flags, const char *name, uint32_t type); extern void drm_property_destroy(struct drm_device *dev, struct drm_property *property); extern int drm_property_add_enum(struct drm_property *property, int index, uint64_t value, const char *name); diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h index 5581980..cee4c53 100644 --- a/include/drm/drm_mode.h +++ b/include/drm/drm_mode.h @@ -231,6 +231,7 @@ struct drm_mode_get_connector { #define DRM_MODE_PROP_ENUM (1<<3) /* enumerated type with text strings */ #define DRM_MODE_PROP_BLOB (1<<4) #define DRM_MODE_PROP_BITMASK (1<<5) /* bitmask of enumerated types */ +#define DRM_MODE_PROP_OBJECT (1<<6) /* drm mode object */
struct drm_mode_property_enum { __u64 value;
From: Rob Clark rob@ti.com
This indicates to userspace that the property is something that can be set dynamically without requiring a "test" step to check if the hw is capable. This allows a userspace compositor, such as weston, to avoid an extra ioctl to check whether it needs to fall-back to GPU to composite some surface prior to submission of GPU render commands. --- include/drm/drm_mode.h | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h index cee4c53..8cec2cf 100644 --- a/include/drm/drm_mode.h +++ b/include/drm/drm_mode.h @@ -232,6 +232,15 @@ struct drm_mode_get_connector { #define DRM_MODE_PROP_BLOB (1<<4) #define DRM_MODE_PROP_BITMASK (1<<5) /* bitmask of enumerated types */ #define DRM_MODE_PROP_OBJECT (1<<6) /* drm mode object */ +/* Properties that are not dynamic cannot safely be changed without a + * atomic-modeset / nuclear-pageflip test step. But if userspace is + * only changing dynamic properties, it is garanteed that the change + * will not exceed hw limits, so no test step is required. + * + * Note that fb_id properties are a bit ambiguous.. they of course can + * be changed dynamically, assuming the pixel format does not change. + */ +#define DRM_MODE_PROP_DYNAMIC (1<<24)
struct drm_mode_property_enum { __u64 value;
From: Rob Clark rob@ti.com
Use atomic properties mechanism to set plane attributes. This by itself doesn't accomplish anything, but it avoids having multiple code paths to do the same thing when nuclear-pageflip and atomic- modeset are introduced. --- drivers/gpu/drm/drm_crtc.c | 270 ++++++++++++++++++++++++++------------------ include/drm/drm_crtc.h | 32 ++++-- 2 files changed, 187 insertions(+), 115 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 12829de..e680fbe 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -38,6 +38,10 @@ #include "drm_edid.h" #include "drm_fourcc.h"
+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); + /* Avoid boilerplate. I'm tired of typing. */ #define DRM_ENUM_NAME_FN(fnname, list) \ char *fnname(int val) \ @@ -380,11 +384,19 @@ EXPORT_SYMBOL(drm_framebuffer_cleanup); void drm_framebuffer_remove(struct drm_framebuffer *fb) { struct drm_device *dev = fb->dev; + struct drm_mode_config *config = &dev->mode_config; struct drm_crtc *crtc; struct drm_plane *plane; struct drm_mode_set set; + void *state; int ret;
+ state = dev->driver->atomic_begin(dev); + if (IS_ERR(state)) { + DRM_ERROR("failed to disable crtc and/or plane when fb was deleted\n"); + return; + } + /* remove from any CRTC */ list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { if (crtc->fb == fb) { @@ -401,15 +413,19 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) list_for_each_entry(plane, &dev->mode_config.plane_list, head) { if (plane->fb == fb) { /* should turn off the crtc */ - ret = plane->funcs->disable_plane(plane); - if (ret) - DRM_ERROR("failed to disable plane with busy fb\n"); - /* disconnect the plane from the fb and crtc: */ - plane->fb = NULL; - plane->crtc = NULL; + drm_mode_plane_set_obj_prop(plane, state, config->prop_crtc_id, 0); + drm_mode_plane_set_obj_prop(plane, state, config->prop_fb_id, 0); } }
+ /* just disabling stuff shouldn't fail, hopefully: */ + if(dev->driver->atomic_check(dev, state)) + DRM_ERROR("failed to disable crtc and/or plane when fb was deleted\n"); + else + dev->driver->atomic_commit(dev, state, NULL); + + dev->driver->atomic_end(dev, state); + list_del(&fb->filp_head);
drm_framebuffer_unreference(fb); @@ -661,6 +677,7 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane, const uint32_t *formats, uint32_t format_count, bool priv) { + struct drm_mode_config *config = &dev->mode_config; int ret;
mutex_lock(&dev->mode_config.mutex); @@ -696,6 +713,17 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane *plane, INIT_LIST_HEAD(&plane->head); }
+ drm_object_attach_property(&plane->base, config->prop_fb_id, 0); + drm_object_attach_property(&plane->base, config->prop_crtc_id, 0); + drm_object_attach_property(&plane->base, config->prop_crtc_x, 0); + drm_object_attach_property(&plane->base, config->prop_crtc_y, 0); + drm_object_attach_property(&plane->base, config->prop_crtc_w, 0); + drm_object_attach_property(&plane->base, config->prop_crtc_h, 0); + drm_object_attach_property(&plane->base, config->prop_src_x, 0); + drm_object_attach_property(&plane->base, config->prop_src_y, 0); + drm_object_attach_property(&plane->base, config->prop_src_w, 0); + drm_object_attach_property(&plane->base, config->prop_src_h, 0); + out: mutex_unlock(&dev->mode_config.mutex);
@@ -769,23 +797,89 @@ void drm_mode_destroy(struct drm_device *dev, struct drm_display_mode *mode) } EXPORT_SYMBOL(drm_mode_destroy);
-static int drm_mode_create_standard_connector_properties(struct drm_device *dev) +static int drm_mode_create_standard_properties(struct drm_device *dev) { - struct drm_property *edid; - struct drm_property *dpms; + struct drm_property *prop;
/* * Standard properties (apply to all connectors) */ - edid = drm_property_create(dev, DRM_MODE_PROP_BLOB | + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB | DRM_MODE_PROP_IMMUTABLE, "EDID", 0); - dev->mode_config.edid_property = edid; + if (!prop) + return -ENOMEM; + dev->mode_config.edid_property = prop;
- dpms = drm_property_create_enum(dev, 0, + prop = drm_property_create_enum(dev, 0, "DPMS", drm_dpms_enum_list, ARRAY_SIZE(drm_dpms_enum_list)); - dev->mode_config.dpms_property = dpms; + if (!prop) + return -ENOMEM; + dev->mode_config.dpms_property = prop; + + + /* TODO we need the driver to control which of these are dynamic + * and which are not.. or maybe we should just set all to zero + * and let the individual drivers frob the prop->flags for the + * properties they can support dynamic changes on.. + */ + + prop = drm_property_create_range(dev, DRM_MODE_PROP_DYNAMIC, + "src_x", 0, UINT_MAX); + if (!prop) + return -ENOMEM; + dev->mode_config.prop_src_x = prop; + + prop = drm_property_create_range(dev, DRM_MODE_PROP_DYNAMIC, + "src_y", 0, UINT_MAX); + if (!prop) + return -ENOMEM; + dev->mode_config.prop_src_y = prop; + + prop = drm_property_create_range(dev, 0, "src_w", 0, UINT_MAX); + if (!prop) + return -ENOMEM; + dev->mode_config.prop_src_w = prop; + + prop = drm_property_create_range(dev, 0, "src_h", 0, UINT_MAX); + if (!prop) + return -ENOMEM; + dev->mode_config.prop_src_h = prop; + + prop = drm_property_create_range(dev, DRM_MODE_PROP_DYNAMIC, + "crtc_x", INT_MIN, INT_MAX); + if (!prop) + return -ENOMEM; + dev->mode_config.prop_crtc_x = prop; + + prop = drm_property_create_range(dev, DRM_MODE_PROP_DYNAMIC, + "crtc_y", INT_MIN, INT_MAX); + if (!prop) + return -ENOMEM; + dev->mode_config.prop_crtc_y = prop; + + prop = drm_property_create_range(dev, 0, "crtc_w", 0, INT_MAX); + if (!prop) + return -ENOMEM; + dev->mode_config.prop_crtc_w = prop; + + prop = drm_property_create_range(dev, 0, "crtc_h", 0, INT_MAX); + if (!prop) + return -ENOMEM; + dev->mode_config.prop_crtc_h = prop; + + prop = drm_property_create_object(dev, DRM_MODE_PROP_DYNAMIC, + "fb", DRM_MODE_OBJECT_FB); + if (!prop) + return -ENOMEM; + dev->mode_config.prop_fb_id = prop; + + prop = drm_property_create_object(dev, 0, + "crtc", DRM_MODE_OBJECT_CRTC); + if (!prop) + return -ENOMEM; + dev->mode_config.prop_crtc_id = prop;
return 0; } @@ -1000,7 +1094,7 @@ void drm_mode_config_init(struct drm_device *dev) idr_init(&dev->mode_config.crtc_idr);
mutex_lock(&dev->mode_config.mutex); - drm_mode_create_standard_connector_properties(dev); + drm_mode_create_standard_properties(dev); mutex_unlock(&dev->mode_config.mutex);
/* Just to be sure */ @@ -1747,23 +1841,22 @@ int drm_mode_setplane(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_set_plane *plane_req = data; + struct drm_mode_config *config = &dev->mode_config; struct drm_mode_object *obj; - struct drm_plane *plane; - struct drm_crtc *crtc; - struct drm_framebuffer *fb; + void *state; int ret = 0; - unsigned int fb_width, fb_height; - int i;
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL;
mutex_lock(&dev->mode_config.mutex);
- /* - * First, find the plane, crtc, and fb objects. If not available, - * we don't bother to call the driver. - */ + state = dev->driver->atomic_begin(dev); + if (IS_ERR(state)) { + ret = PTR_ERR(state); + goto out_unlock; + } + obj = drm_mode_object_find(dev, plane_req->plane_id, DRM_MODE_OBJECT_PLANE); if (!obj) { @@ -1772,91 +1865,38 @@ int drm_mode_setplane(struct drm_device *dev, void *data, ret = -ENOENT; goto out; } - plane = obj_to_plane(obj); - - /* No fb means shut it down */ - if (!plane_req->fb_id) { - plane->funcs->disable_plane(plane); - plane->crtc = NULL; - plane->fb = NULL; - goto out; - }
- obj = drm_mode_object_find(dev, plane_req->crtc_id, - DRM_MODE_OBJECT_CRTC); - if (!obj) { - DRM_DEBUG_KMS("Unknown crtc ID %d\n", - plane_req->crtc_id); - ret = -ENOENT; - goto out; - } - crtc = obj_to_crtc(obj); + ret = + drm_mode_set_obj_prop(dev, obj, state, + config->prop_crtc_id, plane_req->crtc_id) || + drm_mode_set_obj_prop(dev, obj, state, + config->prop_fb_id, plane_req->fb_id) || + drm_mode_set_obj_prop(dev, obj, state, + config->prop_crtc_x, *(uint32_t *)&plane_req->crtc_x) || + drm_mode_set_obj_prop(dev, obj, state, + config->prop_crtc_y, *(uint32_t *)&plane_req->crtc_y) || + drm_mode_set_obj_prop(dev, obj, state, + config->prop_crtc_w, plane_req->crtc_w) || + drm_mode_set_obj_prop(dev, obj, state, + config->prop_crtc_h, plane_req->crtc_h) || + drm_mode_set_obj_prop(dev, obj, state, + config->prop_src_w, plane_req->src_w) || + drm_mode_set_obj_prop(dev, obj, state, + config->prop_src_h, plane_req->src_h) || + drm_mode_set_obj_prop(dev, obj, state, + config->prop_src_x, plane_req->src_x) || + drm_mode_set_obj_prop(dev, obj, state, + config->prop_src_y, plane_req->src_y) || + dev->driver->atomic_check(dev, state);
- obj = drm_mode_object_find(dev, plane_req->fb_id, - DRM_MODE_OBJECT_FB); - if (!obj) { - DRM_DEBUG_KMS("Unknown framebuffer ID %d\n", - plane_req->fb_id); - ret = -ENOENT; - goto out; - } - fb = obj_to_fb(obj); - - /* Check whether this plane supports the fb pixel format. */ - for (i = 0; i < plane->format_count; i++) - if (fb->pixel_format == plane->format_types[i]) - break; - if (i == plane->format_count) { - DRM_DEBUG_KMS("Invalid pixel format 0x%08x\n", fb->pixel_format); - ret = -EINVAL; - goto out; - } - - fb_width = fb->width << 16; - fb_height = fb->height << 16; - - /* Make sure source coordinates are inside the fb. */ - if (plane_req->src_w > fb_width || - plane_req->src_x > fb_width - plane_req->src_w || - plane_req->src_h > fb_height || - plane_req->src_y > fb_height - plane_req->src_h) { - DRM_DEBUG_KMS("Invalid source coordinates " - "%u.%06ux%u.%06u+%u.%06u+%u.%06u\n", - plane_req->src_w >> 16, - ((plane_req->src_w & 0xffff) * 15625) >> 10, - plane_req->src_h >> 16, - ((plane_req->src_h & 0xffff) * 15625) >> 10, - plane_req->src_x >> 16, - ((plane_req->src_x & 0xffff) * 15625) >> 10, - plane_req->src_y >> 16, - ((plane_req->src_y & 0xffff) * 15625) >> 10); - ret = -ENOSPC; - goto out; - } - - /* Give drivers some help against integer overflows */ - if (plane_req->crtc_w > INT_MAX || - plane_req->crtc_x > INT_MAX - (int32_t) plane_req->crtc_w || - plane_req->crtc_h > INT_MAX || - plane_req->crtc_y > INT_MAX - (int32_t) plane_req->crtc_h) { - DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n", - plane_req->crtc_w, plane_req->crtc_h, - plane_req->crtc_x, plane_req->crtc_y); - ret = -ERANGE; + if (ret) goto out; - }
- ret = plane->funcs->update_plane(plane, crtc, fb, - plane_req->crtc_x, plane_req->crtc_y, - plane_req->crtc_w, plane_req->crtc_h, - plane_req->src_x, plane_req->src_y, - plane_req->src_w, plane_req->src_h); - if (!ret) { - plane->crtc = crtc; - plane->fb = fb; - } + 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; @@ -3247,7 +3287,7 @@ 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_connector *connector, +int drm_mode_connector_set_obj_prop(struct drm_connector *connector, void *state, struct drm_property *property, uint64_t value) { @@ -3266,8 +3306,9 @@ static int drm_mode_connector_set_obj_prop(struct drm_connector *connector, drm_connector_property_set_value(connector, property, value); return ret; } +EXPORT_SYMBOL(drm_mode_connector_set_obj_prop);
-static int drm_mode_crtc_set_obj_prop(struct drm_crtc *crtc, +int drm_mode_crtc_set_obj_prop(struct drm_crtc *crtc, void *state, struct drm_property *property, uint64_t value) { @@ -3280,8 +3321,9 @@ static int drm_mode_crtc_set_obj_prop(struct drm_crtc *crtc,
return ret; } +EXPORT_SYMBOL(drm_mode_crtc_set_obj_prop);
-static int drm_mode_plane_set_obj_prop(struct drm_plane *plane, +int drm_mode_plane_set_obj_prop(struct drm_plane *plane, void *state, struct drm_property *property, uint64_t value) { @@ -3294,19 +3336,33 @@ static int drm_mode_plane_set_obj_prop(struct drm_plane *plane,
return ret; } +EXPORT_SYMBOL(drm_mode_plane_set_obj_prop);
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)) { +if (!drm_property_change_is_valid(property, value)) { + DRM_DEBUG("invalid value: %s = %llx\n", property->name, value); +} +// TODO signed properties don't really work out so well, thanks +// to uint64_t stuff everywhere.. INT_MIN for crtc_x/crtc_y end +// up being a big positive #.. + if (true || drm_property_change_is_valid(property, value)) { /* a zero value for an object property translates to null: */ if ((property->flags & DRM_MODE_PROP_OBJECT) && value) { struct drm_mode_object *obj = drm_mode_object_find(dev, value, property->values[0]); if (!obj) return -EINVAL; - value = VOID2U64(obj); + switch (obj->type) { + case DRM_MODE_OBJECT_CRTC: + value = VOID2U64(obj_to_crtc(obj)); + break; + case DRM_MODE_OBJECT_FB: + value = VOID2U64(obj_to_fb(obj)); + break; + } } switch (obj->type) { case DRM_MODE_OBJECT_CONNECTOR: diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 92df2f4..546026c 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -621,13 +621,6 @@ struct drm_connector { * @set_property: called when a property is changed */ struct drm_plane_funcs { - int (*update_plane)(struct drm_plane *plane, - struct drm_crtc *crtc, struct drm_framebuffer *fb, - int crtc_x, int crtc_y, - unsigned int crtc_w, unsigned int crtc_h, - uint32_t src_x, uint32_t src_y, - uint32_t src_w, uint32_t src_h); - int (*disable_plane)(struct drm_plane *plane); void (*destroy)(struct drm_plane *plane);
int (*set_property)(struct drm_plane *plane, void *state, @@ -796,8 +789,20 @@ struct drm_mode_config { bool poll_enabled; struct delayed_work output_poll_work;
- /* pointers to standard properties */ + /* just so blob properties can always be in a list: */ struct list_head property_blob_list; + + /* pointers to standard properties */ + struct drm_property *prop_src_x; + struct drm_property *prop_src_y; + struct drm_property *prop_src_w; + struct drm_property *prop_src_h; + struct drm_property *prop_crtc_x; + struct drm_property *prop_crtc_y; + struct drm_property *prop_crtc_w; + struct drm_property *prop_crtc_h; + struct drm_property *prop_fb_id; + struct drm_property *prop_crtc_id; struct drm_property *edid_property; struct drm_property *dpms_property;
@@ -932,6 +937,17 @@ extern int drm_object_property_set_value(struct drm_mode_object *obj, extern int drm_object_property_get_value(struct drm_mode_object *obj, struct drm_property *property, uint64_t *value); + +int drm_mode_connector_set_obj_prop(struct drm_connector *connector, + void *state, struct drm_property *property, + uint64_t value); +int drm_mode_crtc_set_obj_prop(struct drm_crtc *crtc, + void *state, struct drm_property *property, + uint64_t value); +int drm_mode_plane_set_obj_prop(struct drm_plane *plane, + void *state, struct drm_property *property, + uint64_t value); + extern int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb, const struct drm_framebuffer_funcs *funcs);
From: Rob Clark rob@ti.com
Break the mutable state of a plane out into a separate structure. This makes it easier to have some helpers for plane->set_property() and for checking for invalid params. The idea is that individual drivers can wrap the state struct in their own struct which adds driver specific parameters, for easy build-up of state across multiple set_property() calls and for easy atomic commit or roll- back.
The same should be done for CRTC, encoder, and connector, but this patch only includes the first part (plane). --- drivers/gpu/drm/drm_crtc.c | 97 +++++++++++++++++++++++++++++++++++++++++--- include/drm/drm_crtc.h | 38 +++++++++++++---- 2 files changed, 123 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index e680fbe..ce6fa6a 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -411,7 +411,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) }
list_for_each_entry(plane, &dev->mode_config.plane_list, head) { - if (plane->fb == fb) { + if (plane->state->fb == fb) { /* should turn off the crtc */ drm_mode_plane_set_obj_prop(plane, state, config->prop_crtc_id, 0); drm_mode_plane_set_obj_prop(plane, state, config->prop_fb_id, 0); @@ -747,6 +747,93 @@ void drm_plane_cleanup(struct drm_plane *plane) } EXPORT_SYMBOL(drm_plane_cleanup);
+int drm_plane_check_state(struct drm_plane *plane, + struct drm_plane_state *state) +{ + unsigned int fb_width, fb_height; + struct drm_framebuffer *fb = state->fb; + int i; + + fb_width = fb->width << 16; + fb_height = fb->height << 16; + + /* Check whether this plane supports the fb pixel format. */ + for (i = 0; i < plane->format_count; i++) + if (fb->pixel_format == plane->format_types[i]) + break; + if (i == plane->format_count) { + DRM_DEBUG_KMS("Invalid pixel format 0x%08x\n", fb->pixel_format); + return -EINVAL; + } + + /* Make sure source coordinates are inside the fb. */ + if (state->src_w > fb_width || + state->src_x > fb_width - state->src_w || + state->src_h > fb_height || + state->src_y > fb_height - state->src_h) { + DRM_DEBUG_KMS("Invalid source coordinates " + "%u.%06ux%u.%06u+%u.%06u+%u.%06u\n", + state->src_w >> 16, + ((state->src_w & 0xffff) * 15625) >> 10, + state->src_h >> 16, + ((state->src_h & 0xffff) * 15625) >> 10, + state->src_x >> 16, + ((state->src_x & 0xffff) * 15625) >> 10, + state->src_y >> 16, + ((state->src_y & 0xffff) * 15625) >> 10); + return -ENOSPC; + } + + /* Give drivers some help against integer overflows */ + if (state->crtc_w > INT_MAX || + state->crtc_x > INT_MAX - (int32_t) state->crtc_w || + state->crtc_h > INT_MAX || + state->crtc_y > INT_MAX - (int32_t) state->crtc_h) { + DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n", + state->crtc_w, state->crtc_h, + state->crtc_x, state->crtc_y); + return -ERANGE; + } + + return 0; +} +EXPORT_SYMBOL(drm_plane_check_state); + +int drm_plane_set_property(struct drm_plane *plane, + struct drm_plane_state *state, + struct drm_property *property, uint64_t value) +{ + struct drm_device *dev = plane->dev; + struct drm_mode_config *config = &dev->mode_config; + + if (property == config->prop_fb_id) { + state->fb = U642VOID(value); + } else if (property == config->prop_crtc_id) { + state->crtc = U642VOID(value); + } else if (property == config->prop_crtc_x) { + state->crtc_x = *(int32_t *)&value; + } else if (property == config->prop_crtc_y) { + state->crtc_y = *(int32_t *)&value; + } else if (property == config->prop_crtc_w) { + state->crtc_w = value; + } else if (property == config->prop_crtc_h) { + state->crtc_h = value; + } else if (property == config->prop_src_x) { + state->src_x = value; + } else if (property == config->prop_src_y) { + state->src_y = value; + } else if (property == config->prop_src_w) { + state->src_w = value; + } else if (property == config->prop_src_h) { + state->src_h = value; + } else { + return -EINVAL; + } + + return 0; +} +EXPORT_SYMBOL(drm_plane_set_property); + /** * drm_mode_create - create a new display mode * @dev: DRM device @@ -1790,13 +1877,13 @@ int drm_mode_getplane(struct drm_device *dev, void *data, } plane = obj_to_plane(obj);
- if (plane->crtc) - plane_resp->crtc_id = plane->crtc->base.id; + if (plane->state->crtc) + plane_resp->crtc_id = plane->state->crtc->base.id; else plane_resp->crtc_id = 0;
- if (plane->fb) - plane_resp->fb_id = plane->fb->base.id; + if (plane->state->fb) + plane_resp->fb_id = plane->state->fb->base.id; else plane_resp->fb_id = 0;
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 546026c..a2ba164 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -628,6 +628,26 @@ struct drm_plane_funcs { };
/** + * drm_plane_state - mutable plane state + * @crtc: currently bound CRTC + * @fb: currently bound fb + * @enabled: enabled flag + */ +struct drm_plane_state { + struct drm_crtc *crtc; + struct drm_framebuffer *fb; + bool enabled; + + /* Signed dest location allows it to be partially off screen */ + int32_t crtc_x, crtc_y; + uint32_t crtc_w, crtc_h; + + /* Source values are 16.16 fixed point */ + uint32_t src_x, src_y; + uint32_t src_h, src_w; +}; + +/** * drm_plane - central DRM plane control structure * @dev: DRM device this plane belongs to * @head: for list management @@ -635,11 +655,9 @@ struct drm_plane_funcs { * @possible_crtcs: pipes this plane can be bound to * @format_types: array of formats supported by this plane * @format_count: number of formats supported - * @crtc: currently bound CRTC - * @fb: currently bound fb + * @state: the mutable state * @gamma_size: size of gamma table * @gamma_store: gamma correction table - * @enabled: enabled flag * @funcs: helper functions * @helper_private: storage for drver layer * @properties: property tracking for this plane @@ -654,15 +672,16 @@ struct drm_plane { uint32_t *format_types; uint32_t format_count;
- struct drm_crtc *crtc; - struct drm_framebuffer *fb; + /* + * State that can be updated from userspace, and atomically + * commited or rolled back: + */ + struct drm_plane_state *state;
/* CRTC gamma size for reporting to userspace */ uint32_t gamma_size; uint16_t *gamma_store;
- bool enabled; - const struct drm_plane_funcs *funcs; void *helper_private;
@@ -874,6 +893,11 @@ extern int drm_plane_init(struct drm_device *dev, const uint32_t *formats, uint32_t format_count, bool priv); extern void drm_plane_cleanup(struct drm_plane *plane); +extern int drm_plane_check_state(struct drm_plane *plane, + struct drm_plane_state *state); +extern int drm_plane_set_property(struct drm_plane *plane, + struct drm_plane_state *state, + struct drm_property *property, uint64_t value);
extern void drm_encoder_cleanup(struct drm_encoder *encoder);
From: Rob Clark rob@ti.com
Use atomic properties mechanism for CRTC page_flip. This by itself doesn't accomplish anything, but it avoids having multiple code paths to do the same thing when nuclear-pageflip and atomic-modeset are introduced. --- drivers/gpu/drm/drm_crtc.c | 59 +++++++++++++++++--------------------------- include/drm/drm_crtc.h | 13 ---------- 2 files changed, 23 insertions(+), 49 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index ce6fa6a..8be57e4 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -387,9 +387,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) struct drm_mode_config *config = &dev->mode_config; struct drm_crtc *crtc; struct drm_plane *plane; - struct drm_mode_set set; void *state; - int ret;
state = dev->driver->atomic_begin(dev); if (IS_ERR(state)) { @@ -401,12 +399,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { if (crtc->fb == fb) { /* should turn off the crtc */ - memset(&set, 0, sizeof(struct drm_mode_set)); - set.crtc = crtc; - set.fb = NULL; - ret = crtc->funcs->set_config(&set); - if (ret) - DRM_ERROR("failed to reset crtc %p when fb was deleted\n", crtc); + drm_mode_crtc_set_obj_prop(crtc, state, config->prop_crtc_id, 0); } }
@@ -449,6 +442,7 @@ EXPORT_SYMBOL(drm_framebuffer_remove); int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, const struct drm_crtc_funcs *funcs) { + struct drm_mode_config *config = &dev->mode_config; int ret;
crtc->dev = dev; @@ -466,6 +460,10 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, list_add_tail(&crtc->head, &dev->mode_config.crtc_list); dev->mode_config.num_crtc++;
+ drm_object_attach_property(&crtc->base, config->prop_fb_id, 0); + drm_object_attach_property(&crtc->base, config->prop_crtc_x, 0); + drm_object_attach_property(&crtc->base, config->prop_crtc_y, 0); + out: mutex_unlock(&dev->mode_config.mutex);
@@ -3750,12 +3748,12 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { struct drm_mode_crtc_page_flip *page_flip = data; + struct drm_mode_config *config = &dev->mode_config; struct drm_mode_object *obj; struct drm_crtc *crtc; - struct drm_framebuffer *fb; struct drm_pending_vblank_event *e = NULL; unsigned long flags; - int hdisplay, vdisplay; + void *state; int ret = -EINVAL;
if (page_flip->flags & ~DRM_MODE_PAGE_FLIP_FLAGS || @@ -3763,6 +3761,13 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, return -EINVAL;
mutex_lock(&dev->mode_config.mutex); + + state = dev->driver->atomic_begin(dev); + if (IS_ERR(state)) { + ret = PTR_ERR(state); + goto out_unlock; + } + obj = drm_mode_object_find(dev, page_flip->crtc_id, DRM_MODE_OBJECT_CRTC); if (!obj) goto out; @@ -3777,31 +3782,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, goto out; }
- if (crtc->funcs->page_flip == NULL) - goto out; - - obj = drm_mode_object_find(dev, page_flip->fb_id, DRM_MODE_OBJECT_FB); - if (!obj) - goto out; - fb = obj_to_fb(obj); - - hdisplay = crtc->mode.hdisplay; - vdisplay = crtc->mode.vdisplay; - - if (crtc->invert_dimensions) - swap(hdisplay, vdisplay); - - if (hdisplay > fb->width || - vdisplay > fb->height || - crtc->x > fb->width - hdisplay || - crtc->y > fb->height - vdisplay) { - DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d%s.\n", - fb->width, fb->height, hdisplay, vdisplay, crtc->x, crtc->y, - crtc->invert_dimensions ? " (inverted)" : ""); - ret = -ENOSPC; - goto out; - } - if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) { ret = -ENOMEM; spin_lock_irqsave(&dev->event_lock, flags); @@ -3829,7 +3809,12 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, (void (*) (struct drm_pending_event *)) kfree; }
- ret = crtc->funcs->page_flip(crtc, fb, e); + ret = drm_mode_set_obj_prop(dev, obj, state, + config->prop_fb_id, page_flip->fb_id); + if (ret) + goto out; + + ret = dev->driver->atomic_commit(dev, state, e); if (ret) { if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) { spin_lock_irqsave(&dev->event_lock, flags); @@ -3840,6 +3825,8 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, }
out: + dev->driver->atomic_end(dev, state); +out_unlock: mutex_unlock(&dev->mode_config.mutex); return ret; } diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index a2ba164..a0bec04 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -347,19 +347,6 @@ struct drm_crtc_funcs {
int (*set_config)(struct drm_mode_set *set);
- /* - * Flip to the given framebuffer. This implements the page - * flip ioctl described in drm_mode.h, specifically, the - * implementation must return immediately and block all - * rendering to the current fb until the flip has completed. - * If userspace set the event flag in the ioctl, the event - * argument will point to an event to send back when the flip - * completes, otherwise it will be NULL. - */ - int (*page_flip)(struct drm_crtc *crtc, - struct drm_framebuffer *fb, - struct drm_pending_vblank_event *event); - int (*set_property)(struct drm_crtc *crtc, void *state, struct drm_property *property, uint64_t val); };
From: Rob Clark rob@ti.com
Start breaking out the mutable state of the CRTC into it's own structure. Plus add _check_state() and _set_property() helpers. This only moves the state that is related to scanout fb, which is needed for nuclear-pageflip. The rest of the mutable state should be moved from drm_crtc to drm_crtc_state as part of the atomic-modeset implementation. --- drivers/gpu/drm/drm_crtc.c | 70 +++++++++++++++++++++++++++++++------ drivers/gpu/drm/drm_crtc_helper.c | 51 ++++++++++++++------------- drivers/gpu/drm/drm_fb_helper.c | 11 +++--- include/drm/drm_crtc.h | 44 +++++++++++++++++------ 4 files changed, 125 insertions(+), 51 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 8be57e4..0ddd43e 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -397,7 +397,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
/* remove from any CRTC */ list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { - if (crtc->fb == fb) { + if (crtc->state->fb == fb) { /* should turn off the crtc */ drm_mode_crtc_set_obj_prop(crtc, state, config->prop_crtc_id, 0); } @@ -447,7 +447,7 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
crtc->dev = dev; crtc->funcs = funcs; - crtc->invert_dimensions = false; + crtc->state->invert_dimensions = false;
mutex_lock(&dev->mode_config.mutex);
@@ -496,6 +496,54 @@ void drm_crtc_cleanup(struct drm_crtc *crtc) } EXPORT_SYMBOL(drm_crtc_cleanup);
+int drm_crtc_check_state(struct drm_crtc *crtc, + struct drm_crtc_state *state) +{ + struct drm_framebuffer *fb = state->fb; + int hdisplay, vdisplay; + + hdisplay = crtc->mode.hdisplay; + vdisplay = crtc->mode.vdisplay; + + if (state->invert_dimensions) + swap(hdisplay, vdisplay); + + if (hdisplay > fb->width || + vdisplay > fb->height || + state->x > fb->width - hdisplay || + state->y > fb->height - vdisplay) { + DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d%s.\n", + fb->width, fb->height, hdisplay, vdisplay, + state->x, state->y, + state->invert_dimensions ? " (inverted)" : ""); + return -ENOSPC; + } + + return 0; +} +EXPORT_SYMBOL(drm_crtc_check_state); + +int drm_crtc_set_property(struct drm_crtc *crtc, + struct drm_crtc_state *state, + struct drm_property *property, uint64_t value) +{ + struct drm_device *dev = crtc->dev; + struct drm_mode_config *config = &dev->mode_config; + + if (property == config->prop_fb_id) { + state->fb = U642VOID(value); + } else if (property == config->prop_crtc_x) { + state->x = *(int *)&value; + } else if (property == config->prop_crtc_y) { + state->y = *(int32_t *)&value; + } else { + return -EINVAL; + } + + return 0; +} +EXPORT_SYMBOL(drm_crtc_set_property); + /** * drm_mode_probed_add - add a mode to a connector's probed mode list * @connector: connector the new mode @@ -1593,11 +1641,11 @@ int drm_mode_getcrtc(struct drm_device *dev, } crtc = obj_to_crtc(obj);
- crtc_resp->x = crtc->x; - crtc_resp->y = crtc->y; + crtc_resp->x = crtc->state->x; + crtc_resp->y = crtc->state->y; crtc_resp->gamma_size = crtc->gamma_size; - if (crtc->fb) - crtc_resp->fb_id = crtc->fb->base.id; + if (crtc->state->fb) + crtc_resp->fb_id = crtc->state->fb->base.id; else crtc_resp->fb_id = 0;
@@ -2042,12 +2090,12 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, /* If we have a mode we need a framebuffer. */ /* If we pass -1, set the mode with the currently bound fb */ if (crtc_req->fb_id == -1) { - if (!crtc->fb) { + if (!crtc->state->fb) { DRM_DEBUG_KMS("CRTC doesn't have current FB\n"); ret = -EINVAL; goto out; } - fb = crtc->fb; + fb = crtc->state->fb; } else { obj = drm_mode_object_find(dev, crtc_req->fb_id, DRM_MODE_OBJECT_FB); @@ -2077,7 +2125,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, hdisplay = mode->hdisplay; vdisplay = mode->vdisplay;
- if (crtc->invert_dimensions) + if (crtc->state->invert_dimensions) swap(hdisplay, vdisplay);
if (hdisplay > fb->width || @@ -2087,7 +2135,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data, DRM_DEBUG_KMS("Invalid fb size %ux%u for CRTC viewport %ux%u+%d+%d%s.\n", fb->width, fb->height, hdisplay, vdisplay, crtc_req->x, crtc_req->y, - crtc->invert_dimensions ? " (inverted)" : ""); + crtc->state->invert_dimensions ? " (inverted)" : ""); ret = -ENOSPC; goto out; } @@ -3773,7 +3821,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, goto out; crtc = obj_to_crtc(obj);
- if (crtc->fb == NULL) { + if (crtc->state->fb == NULL) { /* The framebuffer is currently unbound, presumably * due to a hotplug event, that userspace has not * yet discovered. diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 3252e70..65ed229 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -266,7 +266,7 @@ void drm_helper_disable_unused_functions(struct drm_device *dev) (*crtc_funcs->disable)(crtc); else (*crtc_funcs->dpms)(crtc, DRM_MODE_DPMS_OFF); - crtc->fb = NULL; + crtc->state->fb = NULL; } } } @@ -363,15 +363,15 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc,
saved_hwmode = crtc->hwmode; saved_mode = crtc->mode; - saved_x = crtc->x; - saved_y = crtc->y; + saved_x = crtc->state->x; + saved_y = crtc->state->y;
/* Update crtc values up front so the driver can rely on them for mode * setting. */ crtc->mode = *mode; - crtc->x = x; - crtc->y = y; + crtc->state->x = x; + crtc->state->y = y;
/* Pass our mode to the connectors and the CRTC to give them a chance to * adjust it according to limitations or connector properties, and also @@ -456,8 +456,8 @@ done: if (!ret) { crtc->hwmode = saved_hwmode; crtc->mode = saved_mode; - crtc->x = saved_x; - crtc->y = saved_y; + crtc->state->x = saved_x; + crtc->state->y = saved_y; }
return ret; @@ -591,29 +591,29 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
save_set.crtc = set->crtc; save_set.mode = &set->crtc->mode; - save_set.x = set->crtc->x; - save_set.y = set->crtc->y; - save_set.fb = set->crtc->fb; + save_set.x = set->crtc->state->x; + save_set.y = set->crtc->state->y; + save_set.fb = set->crtc->state->fb;
/* We should be able to check here if the fb has the same properties * and then just flip_or_move it */ - if (set->crtc->fb != set->fb) { + if (set->crtc->state->fb != set->fb) { /* If we have no fb then treat it as a full mode set */ - if (set->crtc->fb == NULL) { + if (set->crtc->state->fb == NULL) { DRM_DEBUG_KMS("crtc has no fb, full mode set\n"); mode_changed = true; } else if (set->fb == NULL) { mode_changed = true; - } else if (set->fb->depth != set->crtc->fb->depth) { + } else if (set->fb->depth != set->crtc->state->fb->depth) { mode_changed = true; } else if (set->fb->bits_per_pixel != - set->crtc->fb->bits_per_pixel) { + set->crtc->state->fb->bits_per_pixel) { mode_changed = true; } else fb_changed = true; }
- if (set->x != set->crtc->x || set->y != set->crtc->y) + if (set->x != set->crtc->state->x || set->y != set->crtc->state->y) fb_changed = true;
if (set->mode && !drm_mode_equal(set->mode, &set->crtc->mode)) { @@ -704,14 +704,14 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set) DRM_DEBUG_KMS("attempting to set mode from" " userspace\n"); drm_mode_debug_printmodeline(set->mode); - old_fb = set->crtc->fb; - set->crtc->fb = set->fb; + old_fb = set->crtc->state->fb; + set->crtc->state->fb = set->fb; if (!drm_crtc_helper_set_mode(set->crtc, set->mode, set->x, set->y, old_fb)) { DRM_ERROR("failed to set mode on [CRTC:%d]\n", set->crtc->base.id); - set->crtc->fb = old_fb; + set->crtc->state->fb = old_fb; ret = -EINVAL; goto fail; } @@ -724,16 +724,16 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set) } drm_helper_disable_unused_functions(dev); } else if (fb_changed) { - set->crtc->x = set->x; - set->crtc->y = set->y; + set->crtc->state->x = set->x; + set->crtc->state->y = set->y;
- old_fb = set->crtc->fb; - if (set->crtc->fb != set->fb) - set->crtc->fb = set->fb; + old_fb = set->crtc->state->fb; + if (set->crtc->state->fb != set->fb) + set->crtc->state->fb = set->fb; ret = crtc_funcs->mode_set_base(set->crtc, set->x, set->y, old_fb); if (ret != 0) { - set->crtc->fb = old_fb; + set->crtc->state->fb = old_fb; goto fail; } } @@ -888,7 +888,8 @@ int drm_helper_resume_force_mode(struct drm_device *dev) continue;
ret = drm_crtc_helper_set_mode(crtc, &crtc->mode, - crtc->x, crtc->y, crtc->fb); + crtc->state->x, crtc->state->y, + crtc->state->fb);
if (ret == false) DRM_ERROR("failed to set mode on crtc %p\n", crtc); diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index f546d1e..d70b787 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -185,7 +185,7 @@ static struct drm_framebuffer *drm_mode_config_fb(struct drm_crtc *crtc)
list_for_each_entry(c, &dev->mode_config.crtc_list, head) { if (crtc->base.id == c->base.id) - return c->fb; + return c->state->fb; }
return NULL; @@ -214,8 +214,9 @@ int drm_fb_helper_debug_leave(struct fb_info *info) }
drm_fb_helper_restore_lut_atomic(mode_set->crtc); - funcs->mode_set_base_atomic(mode_set->crtc, fb, crtc->x, - crtc->y, LEAVE_ATOMIC_MODE_SET); + funcs->mode_set_base_atomic(mode_set->crtc, fb, + crtc->state->x, crtc->state->y, + LEAVE_ATOMIC_MODE_SET); }
return 0; @@ -1361,9 +1362,9 @@ int drm_fb_helper_hotplug_event(struct drm_fb_helper *fb_helper)
mutex_lock(&dev->mode_config.mutex); list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { - if (crtc->fb) + if (crtc->state->fb) crtcs_bound++; - if (crtc->fb == fb_helper->fb) + if (crtc->state->fb == fb_helper->fb) bound++; }
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index a0bec04..9417aaa 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -352,18 +352,41 @@ struct drm_crtc_funcs { };
/** + * drm_crtc_state - mutable crtc state + * @fb: the framebuffer that the CRTC is currently bound to + * @invert_dimensions: for purposes of error checking crtc vs fb sizes, + * invert the width/height of the crtc. This is used if the driver + * is performing 90 or 270 degree rotated scanout + * @x: x position on screen + * @y: y position on screen + */ +struct drm_crtc_state { + /* + * NOTE: more should move from 'struct drm_crtc' into here as + * part of the atomic-modeset support: + * + enabled + * + mode + * + hwmode + * + framedur_ns + * + linedur_ns + * + pixeldur_ns + * + * For now, I'm just moving what is needed for nuclear-pageflip + */ + struct drm_framebuffer *fb; + bool invert_dimensions; + int x, y; +}; + +/** * drm_crtc - central CRTC control structure * @dev: parent DRM device * @head: list management * @base: base KMS object for ID tracking etc. + * @state: the mutable state * @enabled: is this CRTC enabled? * @mode: current mode timings * @hwmode: mode timings as programmed to hw regs - * @invert_dimensions: for purposes of error checking crtc vs fb sizes, - * invert the width/height of the crtc. This is used if the driver - * is performing 90 or 270 degree rotated scanout - * @x: x position on screen - * @y: y position on screen * @funcs: CRTC control functions * @gamma_size: size of gamma ramp * @gamma_store: gamma ramp values @@ -382,8 +405,7 @@ struct drm_crtc {
struct drm_mode_object base;
- /* framebuffer the connector is currently bound to */ - struct drm_framebuffer *fb; + struct drm_crtc_state *state;
bool enabled;
@@ -395,9 +417,6 @@ struct drm_crtc { */ struct drm_display_mode hwmode;
- bool invert_dimensions; - - int x, y; const struct drm_crtc_funcs *funcs;
/* CRTC gamma size for reporting to userspace */ @@ -858,6 +877,11 @@ extern int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, const struct drm_crtc_funcs *funcs); extern void drm_crtc_cleanup(struct drm_crtc *crtc); +extern int drm_crtc_check_state(struct drm_crtc *crtc, + struct drm_crtc_state *state); +extern int drm_crtc_set_property(struct drm_crtc *crtc, + struct drm_crtc_state *state, + struct drm_property *property, uint64_t value);
extern int drm_connector_init(struct drm_device *dev, struct drm_connector *connector,
From: Rob Clark rob@ti.com
--- drivers/gpu/drm/drm_crtc.c | 147 +++++++++++++++++++++++++++++++++++--------- drivers/gpu/drm/drm_drv.c | 1 + include/drm/drm.h | 2 + include/drm/drm_crtc.h | 2 + include/drm/drm_mode.h | 38 ++++++++++++ 5 files changed, 161 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 0ddd43e..d285b11 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -3792,6 +3792,51 @@ out: return ret; }
+static struct drm_pending_vblank_event *create_vblank_event( + struct drm_device *dev, struct drm_file *file_priv, uint64_t user_data) +{ + struct drm_pending_vblank_event *e = NULL; + unsigned long flags; + + spin_lock_irqsave(&dev->event_lock, flags); + if (file_priv->event_space < sizeof e->event) { + spin_unlock_irqrestore(&dev->event_lock, flags); + goto out; + } + file_priv->event_space -= sizeof e->event; + spin_unlock_irqrestore(&dev->event_lock, flags); + + e = kzalloc(sizeof *e, GFP_KERNEL); + if (e == NULL) { + spin_lock_irqsave(&dev->event_lock, flags); + file_priv->event_space += sizeof e->event; + spin_unlock_irqrestore(&dev->event_lock, flags); + goto out; + } + + e->event.base.type = DRM_EVENT_FLIP_COMPLETE; + e->event.base.length = sizeof e->event; + e->event.user_data = user_data; + e->base.event = &e->event.base; + e->base.file_priv = file_priv; + e->base.destroy = + (void (*) (struct drm_pending_event *)) kfree; + +out: + return e; +} + +static void destroy_vblank_event(struct drm_device *dev, + struct drm_file *file_priv, struct drm_pending_vblank_event *e) +{ + unsigned long flags; + + spin_lock_irqsave(&dev->event_lock, flags); + file_priv->event_space += sizeof e->event; + spin_unlock_irqrestore(&dev->event_lock, flags); + kfree(e); +} + int drm_mode_page_flip_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv) { @@ -3800,7 +3845,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, struct drm_mode_object *obj; struct drm_crtc *crtc; struct drm_pending_vblank_event *e = NULL; - unsigned long flags; void *state; int ret = -EINVAL;
@@ -3831,30 +3875,11 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, }
if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) { - ret = -ENOMEM; - spin_lock_irqsave(&dev->event_lock, flags); - if (file_priv->event_space < sizeof e->event) { - spin_unlock_irqrestore(&dev->event_lock, flags); - goto out; - } - file_priv->event_space -= sizeof e->event; - spin_unlock_irqrestore(&dev->event_lock, flags); - - e = kzalloc(sizeof *e, GFP_KERNEL); - if (e == NULL) { - spin_lock_irqsave(&dev->event_lock, flags); - file_priv->event_space += sizeof e->event; - spin_unlock_irqrestore(&dev->event_lock, flags); + e = create_vblank_event(dev, file_priv, page_flip->user_data); + if (!e) { + ret = -ENOMEM; goto out; } - - e->event.base.type = DRM_EVENT_FLIP_COMPLETE; - e->event.base.length = sizeof e->event; - e->event.user_data = page_flip->user_data; - e->base.event = &e->event.base; - e->base.file_priv = file_priv; - e->base.destroy = - (void (*) (struct drm_pending_event *)) kfree; }
ret = drm_mode_set_obj_prop(dev, obj, state, @@ -3863,15 +3888,79 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, goto out;
ret = dev->driver->atomic_commit(dev, state, e); - if (ret) { - if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) { - spin_lock_irqsave(&dev->event_lock, flags); - file_priv->event_space += sizeof e->event; - spin_unlock_irqrestore(&dev->event_lock, flags); - kfree(e); + if (ret && e) + destroy_vblank_event(dev, file_priv, e); + +out: + dev->driver->atomic_end(dev, state); +out_unlock: + mutex_unlock(&dev->mode_config.mutex); + return ret; +} + +int drm_mode_nuclear_page_flip_ioctl(struct drm_device *dev, + void *data, struct drm_file *file_priv) +{ + struct drm_mode_crtc_nuclear_page_flip *page_flip = data; + struct drm_mode_obj_set_property __user *props = + (struct drm_mode_obj_set_property __user *) + (unsigned long)page_flip->props_ptr; + struct drm_pending_vblank_event *e = NULL; + void *state; + int i, ret; + + if (page_flip->flags & ~DRM_MODE_NUCLEAR_PAGE_FLIP_FLAGS || + page_flip->reserved != 0) + return -EINVAL; + + if (!drm_core_check_feature(dev, DRIVER_MODESET)) + return -EINVAL; + + mutex_lock(&dev->mode_config.mutex); + state = dev->driver->atomic_begin(dev); + if (IS_ERR(state)) { + ret = PTR_ERR(state); + goto out_unlock; + } + + if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT) { + e = create_vblank_event(dev, file_priv, page_flip->user_data); + if (!e) { + ret = -ENOMEM; + goto out; } }
+ for (i = 0; i < page_flip->count_props; i++) { + struct drm_mode_obj_set_property prop; + if (copy_from_user(&prop, &props[i], sizeof(prop))) { + ret = -EFAULT; + goto out; + } + + /* TODO should we enforce that none of the + * properties target objects in chain with + * other crtcs? Or just let the driver deal + * with it? + */ + + ret = drm_mode_set_obj_prop_id(dev, state, + prop.obj_id, prop.obj_type, + prop.prop_id, prop.value); + if (ret) + goto out; + } + + ret = dev->driver->atomic_check(dev, state); + if (ret) + goto out; + + if (!(page_flip->flags & DRM_MODE_TEST_ONLY)) + ret = dev->driver->atomic_commit(dev, state, e); + + if (ret && e) + destroy_vblank_event(dev, file_priv, e); + out: dev->driver->atomic_end(dev, state); out_unlock: diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 9238de4..d700042 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -166,6 +166,7 @@ static struct drm_ioctl_desc drm_ioctls[] = { DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROY_DUMB, drm_mode_destroy_dumb_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_OBJ_GETPROPERTIES, drm_mode_obj_get_properties_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), 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_NUCLEAR_PAGE_FLIP, drm_mode_nuclear_page_flip_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), };
#define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls ) diff --git a/include/drm/drm.h b/include/drm/drm.h index e51035a..dec69fd 100644 --- a/include/drm/drm.h +++ b/include/drm/drm.h @@ -732,6 +732,8 @@ struct drm_prime_handle { #define DRM_IOCTL_MODE_ADDFB2 DRM_IOWR(0xB8, struct drm_mode_fb_cmd2) #define DRM_IOCTL_MODE_OBJ_GETPROPERTIES DRM_IOWR(0xB9, struct drm_mode_obj_get_properties) #define DRM_IOCTL_MODE_OBJ_SETPROPERTY DRM_IOWR(0xBA, struct drm_mode_obj_set_property) +/* placeholder for DRM_IOCTL_ATOMIC_MODE_SET */ +#define DRM_IOCTL_MODE_NUCLEAR_PAGE_FLIP DRM_IOWR(0xBC, struct drm_mode_crtc_nuclear_page_flip)
/** * Device specific ioctls should only be in their respective headers diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 9417aaa..9316fbf 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1080,6 +1080,8 @@ extern bool drm_detect_hdmi_monitor(struct edid *edid); extern bool drm_detect_monitor_audio(struct edid *edid); extern int drm_mode_page_flip_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); +extern int drm_mode_nuclear_page_flip_ioctl(struct drm_device *dev, + void *data, struct drm_file *file_priv); extern struct drm_display_mode *drm_cvt_mode(struct drm_device *dev, int hdisplay, int vdisplay, int vrefresh, bool reduced, bool interlaced, bool margins); diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h index 8cec2cf..32d6f65 100644 --- a/include/drm/drm_mode.h +++ b/include/drm/drm_mode.h @@ -408,7 +408,10 @@ struct drm_mode_crtc_lut { };
#define DRM_MODE_PAGE_FLIP_EVENT 0x01 +#define DRM_MODE_TEST_ONLY 0x02 #define DRM_MODE_PAGE_FLIP_FLAGS DRM_MODE_PAGE_FLIP_EVENT +#define DRM_MODE_NUCLEAR_PAGE_FLIP_FLAGS \ + (DRM_MODE_PAGE_FLIP_EVENT | DRM_MODE_TEST_ONLY)
/* * Request a page flip on the specified crtc. @@ -440,6 +443,41 @@ struct drm_mode_crtc_page_flip { __u64 user_data; };
+/* + * Request a page flip on the crtc specified by 'crtc_id' plus zero or + * more planes attached to this crtc. + * + * This ioctl will ask KMS to schedule a page flip for the specified + * crtc. Once any pending rendering targeting the specified fb(s) (as + * of ioctl time) has completed, the crtc and zero or more planes will + * be reprogrammed to display the new fb(s) after the next vertical + * refresh. The ioctl returns immediately, but subsequent rendering + * to the current fb will block in the execbuffer ioctl until the page + * flip happens. If a page flip is already pending as the ioctl is + * called, EBUSY will be returned. + * + * The ioctl supports the following flags: + * + DRM_MODE_PAGE_FLIP_EVENT, which will request that drm sends back + * a vblank event (see drm.h: struct drm_event_vblank) when the page + * flip is done. The user_data field passed in with this ioctl will + * be returned as the user_data field in the vblank event struct. + * + DRM_MODE_TEST_ONLY, don't actually apply the changes (or generate + * a vblank event) but just test the configuration to see if it is + * possible. + * + * The reserved field must be zero until we figure out something clever + * to use it for. + */ + +struct drm_mode_crtc_nuclear_page_flip { + uint32_t crtc_id; + uint32_t flags; + uint64_t user_data; + uint32_t reserved; + uint32_t count_props; + uint64_t props_ptr; /* ptr to array of drm_mode_obj_set_property */ +}; + /* create a dumb scanout buffer */ struct drm_mode_create_dumb { uint32_t height;
From: Rob Clark rob@ti.com
--- drivers/staging/omapdrm/Makefile | 1 + drivers/staging/omapdrm/omap_atomic.c | 270 +++++++++++++++++++++++++++++++++ drivers/staging/omapdrm/omap_atomic.h | 52 +++++++ drivers/staging/omapdrm/omap_crtc.c | 247 +++++++++++++----------------- drivers/staging/omapdrm/omap_drv.c | 5 + drivers/staging/omapdrm/omap_drv.h | 35 +++-- drivers/staging/omapdrm/omap_fb.c | 44 +++--- drivers/staging/omapdrm/omap_plane.c | 270 +++++++++++++++++---------------- 8 files changed, 616 insertions(+), 308 deletions(-) create mode 100644 drivers/staging/omapdrm/omap_atomic.c create mode 100644 drivers/staging/omapdrm/omap_atomic.h
diff --git a/drivers/staging/omapdrm/Makefile b/drivers/staging/omapdrm/Makefile index d85e058..7d45e4d 100644 --- a/drivers/staging/omapdrm/Makefile +++ b/drivers/staging/omapdrm/Makefile @@ -13,6 +13,7 @@ omapdrm-y := omap_drv.o \ omap_connector.o \ omap_fb.o \ omap_fbdev.o \ + omap_atomic.o \ omap_gem.o \ omap_gem_dmabuf.o \ omap_dmm_tiler.o \ diff --git a/drivers/staging/omapdrm/omap_atomic.c b/drivers/staging/omapdrm/omap_atomic.c new file mode 100644 index 0000000..f078012 --- /dev/null +++ b/drivers/staging/omapdrm/omap_atomic.c @@ -0,0 +1,270 @@ +/* + * drivers/staging/omapdrm/omap_atomic.c + * + * Copyright (C) 2012 Texas Instruments + * Author: Rob Clark rob.clark@linaro.org + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published by + * the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along with + * this program. If not, see http://www.gnu.org/licenses/. + */ + +#include "omap_drv.h" +#include "omap_atomic.h" + +struct omap_atomic_state { + struct drm_device *dev; + + int num_dirty_planes, num_dirty_crtcs; + struct omap_plane_state *plane_state[8]; + struct omap_crtc_state *crtc_state[8]; + + int num_pending_fbs, num_ready_fbs; + struct drm_framebuffer *pending_fbs[8]; + + /* for handling page flips without caring about what + * the callback is called from. Possibly we should just + * make omap_gem always call the cb from the worker so + * we don't have to care about this.. + */ + struct work_struct commit_work; +}; + +static void commit_worker(struct work_struct *work); + +void *omap_atomic_begin(struct drm_device *dev) +{ + struct omap_drm_private *priv = dev->dev_private; + struct omap_atomic_state *omap_state; + + // XXX for page_flip, we probably want to know the crtc/crtc_id here, + // so we can error out at this point if there is still a pending flip + // on this crtc.. + + // XXX we should track that we are in-atomic-update per-crtc, so + // that page-flips on multple crtc's don't step on each other's feet + + WARN_ON(priv->in_atomic_update); + priv->in_atomic_update = true; + + omap_state = kzalloc(sizeof(*omap_state), GFP_KERNEL); + if (!omap_state) { + dev_err(dev->dev, "failed to allocate state\n"); + return ERR_PTR(-ENOMEM); + } + + omap_state->dev = dev; + INIT_WORK(&omap_state->commit_work, commit_worker); + + return omap_state; +} + +int omap_atomic_check(struct drm_device *dev, void *state) +{ + struct omap_atomic_state *omap_state = state; + struct omap_drm_private *priv = dev->dev_private; + int i, ret = 0; + + for (i = 0; (i < ARRAY_SIZE(omap_state->plane_state)) && !ret; i++) + if (omap_state->plane_state[i]) + ret = omap_plane_check_state(priv->planes[i], + omap_state->plane_state[i]); + + for (i = 0; (i < ARRAY_SIZE(omap_state->crtc_state)) && !ret; i++) + if (omap_state->crtc_state[i]) + ret = omap_crtc_check_state(priv->crtcs[i], + omap_state->crtc_state[i]); + + return ret; +} + +static void commit_worker(struct work_struct *work) +{ + struct omap_atomic_state *omap_state = + container_of(work, struct omap_atomic_state, commit_work); + struct drm_device *dev = omap_state->dev; + struct omap_drm_private *priv = dev->dev_private; + int i; + + mutex_lock(&dev->mode_config.mutex); + + for (i = 0; i < ARRAY_SIZE(omap_state->plane_state); i++) + if (omap_state->plane_state[i]) + omap_plane_commit_state(priv->planes[i], + omap_state->plane_state[i]); + + priv->in_atomic_update = false; + + for (i = 0; i < ARRAY_SIZE(omap_state->crtc_state); i++) + if (omap_state->crtc_state[i]) + omap_crtc_commit_state(priv->crtcs[i], + omap_state->crtc_state[i]); + + for (i = 0; i < omap_state->num_pending_fbs; i++) + drm_framebuffer_unreference(omap_state->pending_fbs[i]); + + mutex_unlock(&dev->mode_config.mutex); + + /* omap_plane_commit()/omap_crtc_commit() have taken ownership + * of their respective state objects, so don't need to kfree() + * 'em here + */ + + kfree(omap_state); +} + +static void commit_cb(void *state) +{ + struct omap_atomic_state *omap_state = state; + struct omap_drm_private *priv = omap_state->dev->dev_private; + if (++omap_state->num_ready_fbs == omap_state->num_pending_fbs) + queue_work(priv->wq, &omap_state->commit_work); +} + +int omap_atomic_commit(struct drm_device *dev, void *state, + struct drm_pending_vblank_event *event) +{ + struct omap_atomic_state *omap_state = state; + struct omap_drm_private *priv = omap_state->dev->dev_private; + int i; + + // XXX for synchronous modeset, we should block until fb's are ready, + // rather than complete asynchronously + + if (event) { + /* Stash the event on the first plane that is pending update.. + * it doesn't really matter which one, since they all get + * vblank at the same time. + */ + for (i = 0; i < ARRAY_SIZE(omap_state->plane_state); i++) { + if (omap_state->plane_state[i]) { + priv->event[i] = event; + break; + } + } + + /* I don't think there should be any way that this can happen: */ + WARN_ON(i == ARRAY_SIZE(omap_state->plane_state)); + } + + if (!omap_state->num_pending_fbs) { + queue_work(priv->wq, &omap_state->commit_work); + return 0; + } + + for (i = 0; i < omap_state->num_pending_fbs; i++) { + struct drm_gem_object *bo; + bo = omap_framebuffer_bo(omap_state->pending_fbs[i], 0); + omap_gem_op_async(bo, OMAP_GEM_READ, commit_cb, omap_state); + } + + return 0; +} + +void omap_atomic_end(struct drm_device *dev, void *state) +{ + // XXX maybe we need to refcnt the state for async updates.. + // for now we just kfree() stuff after the (potentially async) + // commit completes rather than here, which means state could + // already be freed by now + // XXX but this means we leak if check fails! don't fail! +} + +struct omap_plane_state *omap_atomic_plane_state(void *state, int id) +{ + struct omap_atomic_state *omap_state = state; + struct omap_drm_private *priv = omap_state->dev->dev_private; + struct omap_plane_state *plane_state = omap_state->plane_state[id]; + int i; + + if (!plane_state) { + struct drm_plane *plane = priv->planes[id]; + + plane_state = kzalloc(sizeof(*plane_state), GFP_KERNEL); + + /* snapshot current state: */ + *plane_state = *to_omap_plane_state(plane->state); + + omap_state->plane_state[id] = plane_state; + } + + /* updating a plane implicitly dirties the crtc: */ + for (i = 0; i < priv->num_crtcs; i++) { + if (priv->crtcs[i] == plane_state->base.crtc) { + omap_atomic_crtc_state(state, i); + break; + } + } + + return plane_state; +} + +struct omap_crtc_state *omap_atomic_crtc_state(void *state, int id) +{ + struct omap_atomic_state *omap_state = state; + struct omap_drm_private *priv = omap_state->dev->dev_private; + struct omap_crtc_state *crtc_state = omap_state->crtc_state[id]; + + if (!crtc_state) { + struct drm_crtc *crtc = priv->crtcs[id]; + + crtc_state = kzalloc(sizeof(*crtc_state), GFP_KERNEL); + + /* snapshot current state: */ + *crtc_state = *to_omap_crtc_state(crtc->state); + + omap_state->crtc_state[id] = crtc_state; + } + + return crtc_state; +} + +/* when fb is changed, that gets recorded in the state, so that pageflips + * can defer until all fb's are ready + */ +void omap_atomic_add_fb(void *state, struct drm_framebuffer *fb) +{ + struct omap_atomic_state *omap_state = state; + drm_framebuffer_reference(fb); + omap_state->pending_fbs[omap_state->num_pending_fbs++] = fb; +} + +/* possibly this could be in drm core? */ +static void send_page_flip_event(struct drm_device *dev, int crtc, + struct drm_pending_vblank_event *event) +{ + unsigned long flags; + struct timeval now; + + DBG("%p", event); + + spin_lock_irqsave(&dev->event_lock, flags); + event->event.sequence = drm_vblank_count_and_time(dev, crtc, &now); + event->event.tv_sec = now.tv_sec; + event->event.tv_usec = now.tv_usec; + list_add_tail(&event->base.link, + &event->base.file_priv->event_list); + wake_up_interruptible(&event->base.file_priv->event_wait); + spin_unlock_irqrestore(&dev->event_lock, flags); +} + +/* called when plane is updated.. so we can keep track of when to send + * page-flip events + */ +void omap_atomic_plane_update(struct drm_device *dev, int id) +{ + struct omap_drm_private *priv = dev->dev_private; + if (priv->event[id]) { + /* wakeup userspace */ + send_page_flip_event(dev, id, priv->event[id]); + priv->event[id] = NULL; + } +} diff --git a/drivers/staging/omapdrm/omap_atomic.h b/drivers/staging/omapdrm/omap_atomic.h new file mode 100644 index 0000000..974eaef --- /dev/null +++ b/drivers/staging/omapdrm/omap_atomic.h @@ -0,0 +1,52 @@ +/* + * drivers/staging/omapdrm/omap_atomic.h + * + * Copyright (C) 2012 Texas Instruments + * Author: Rob Clark rob.clark@linaro.org + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 as published by + * the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along with + * this program. If not, see http://www.gnu.org/licenses/. + */ + +#ifndef __OMAP_ATOMIC_H__ +#define __OMAP_ATOMIC_H__ + +#include "drm_mode.h" +#include "drm_crtc.h" + +struct omap_plane_state { + struct drm_plane_state base; + uint8_t rotation; + uint8_t zorder; + uint8_t enabled; +}; +#define to_omap_plane_state(x) container_of(x, struct omap_plane_state, base) + +struct omap_crtc_state { + struct drm_crtc_state base; +}; +#define to_omap_crtc_state(x) container_of(x, struct omap_crtc_state, base) + +void *omap_atomic_begin(struct drm_device *dev); +int omap_atomic_check(struct drm_device *dev, void *state); +int omap_atomic_commit(struct drm_device *dev, void *state, + struct drm_pending_vblank_event *event); +void omap_atomic_end(struct drm_device *dev, void *state); + +struct omap_plane_state *omap_atomic_plane_state(void *state, int id); +struct omap_crtc_state *omap_atomic_crtc_state(void *state, int id); + +void omap_atomic_add_fb(void *state, struct drm_framebuffer *fb); + +void omap_atomic_plane_update(struct drm_device *dev, int id); + +#endif /* __OMAP_ATOMIC_H__ */ diff --git a/drivers/staging/omapdrm/omap_crtc.c b/drivers/staging/omapdrm/omap_crtc.c index b0d22c3..59fae87 100644 --- a/drivers/staging/omapdrm/omap_crtc.c +++ b/drivers/staging/omapdrm/omap_crtc.c @@ -32,7 +32,6 @@ struct omap_crtc { const char *name; int pipe; enum omap_channel channel; - struct omap_overlay_manager_info info;
struct omap_video_timings timings; bool enabled, timings_valid; @@ -48,25 +47,17 @@ struct omap_crtc {
/* for handling queued and in-progress applies: */ struct work_struct apply_work; - - /* if there is a pending flip, these will be non-null: */ - struct drm_pending_vblank_event *event; - - /* for handling page flips without caring about what - * the callback is called from. Possibly we should just - * make omap_gem always call the cb from the worker so - * we don't have to care about this.. - * - * XXX maybe fold into apply_work?? - */ - struct work_struct page_flip_work; };
+static int omap_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, + struct drm_framebuffer *old_fb); + static void omap_crtc_destroy(struct drm_crtc *crtc) { struct omap_crtc *omap_crtc = to_omap_crtc(crtc); omap_crtc->plane->funcs->destroy(omap_crtc->plane); drm_crtc_cleanup(crtc); + kfree(crtc->state); kfree(omap_crtc); }
@@ -89,7 +80,7 @@ static void omap_crtc_dpms(struct drm_crtc *crtc, int mode) /* and any attached overlay planes: */ for (i = 0; i < priv->num_planes; i++) { struct drm_plane *plane = priv->planes[i]; - if (plane->crtc == crtc) + if (plane->state->crtc == crtc) WARN_ON(omap_plane_dpms(plane, mode)); } } @@ -127,11 +118,7 @@ static int omap_crtc_mode_set(struct drm_crtc *crtc, } }
- return omap_plane_mode_set(omap_crtc->plane, crtc, crtc->fb, - 0, 0, mode->hdisplay, mode->vdisplay, - x << 16, y << 16, - mode->hdisplay << 16, mode->vdisplay << 16, - NULL, NULL); + return omap_crtc_mode_set_base(crtc, x, y, old_fb); }
static void omap_crtc_prepare(struct drm_crtc *crtc) @@ -153,134 +140,83 @@ static int omap_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, { struct omap_crtc *omap_crtc = to_omap_crtc(crtc); struct drm_plane *plane = omap_crtc->plane; - struct drm_display_mode *mode = &crtc->mode; - - return omap_plane_mode_set(plane, crtc, crtc->fb, - 0, 0, mode->hdisplay, mode->vdisplay, - x << 16, y << 16, - mode->hdisplay << 16, mode->vdisplay << 16, - NULL, NULL); -} - -static void omap_crtc_load_lut(struct drm_crtc *crtc) -{ -} - -/* possibly this could be in drm core? */ -static void send_page_flip_event(struct drm_device *dev, int crtc, - struct drm_pending_vblank_event *event) -{ - unsigned long flags; - struct timeval now; - - DBG("%p", event); - - spin_lock_irqsave(&dev->event_lock, flags); - event->event.sequence = drm_vblank_count_and_time(dev, crtc, &now); - event->event.tv_sec = now.tv_sec; - event->event.tv_usec = now.tv_usec; - list_add_tail(&event->base.link, - &event->base.file_priv->event_list); - wake_up_interruptible(&event->base.file_priv->event_wait); - spin_unlock_irqrestore(&dev->event_lock, flags); -} - -static void vblank_cb(void *arg) -{ - struct drm_crtc *crtc = arg; - struct omap_crtc *omap_crtc = to_omap_crtc(crtc); - struct drm_pending_vblank_event *event = omap_crtc->event; - - WARN_ON(!event); - - omap_crtc->event = NULL; - - /* wakeup userspace */ - if (event) - send_page_flip_event(crtc->dev, omap_crtc->pipe, event); -} - -static void page_flip_worker(struct work_struct *work) -{ - struct omap_crtc *omap_crtc = - container_of(work, struct omap_crtc, page_flip_work); - struct drm_crtc *crtc = &omap_crtc->base; struct drm_device *dev = crtc->dev; + struct drm_mode_config *config = &dev->mode_config; struct drm_display_mode *mode = &crtc->mode; - struct drm_gem_object *bo; - - mutex_lock(&dev->mode_config.mutex); - omap_plane_mode_set(omap_crtc->plane, crtc, crtc->fb, - 0, 0, mode->hdisplay, mode->vdisplay, - crtc->x << 16, crtc->y << 16, - mode->hdisplay << 16, mode->vdisplay << 16, - vblank_cb, crtc); - mutex_unlock(&dev->mode_config.mutex); - - bo = omap_framebuffer_bo(crtc->fb, 0); - drm_gem_object_unreference_unlocked(bo); -} - -static void page_flip_cb(void *arg) -{ - struct drm_crtc *crtc = arg; - struct omap_crtc *omap_crtc = to_omap_crtc(crtc); - struct omap_drm_private *priv = crtc->dev->dev_private; - - /* avoid assumptions about what ctxt we are called from: */ - queue_work(priv->wq, &omap_crtc->page_flip_work); + void *state; + int ret; + + /* for now, until property based atomic mode-set: */ + state = omap_atomic_begin(dev); + if (IS_ERR(state)) + return PTR_ERR(state); + + ret = + drm_mode_plane_set_obj_prop(plane, state, + config->prop_crtc_id, VOID2U64(crtc)) || + drm_mode_plane_set_obj_prop(plane, state, + config->prop_fb_id, VOID2U64(crtc->state->fb)) || + drm_mode_plane_set_obj_prop(plane, state, + config->prop_crtc_x, 0) || + drm_mode_plane_set_obj_prop(plane, state, + config->prop_crtc_y, 0) || + drm_mode_plane_set_obj_prop(plane, state, + config->prop_crtc_w, mode->hdisplay) || + drm_mode_plane_set_obj_prop(plane, state, + config->prop_crtc_h, mode->vdisplay) || + drm_mode_plane_set_obj_prop(plane, state, + config->prop_src_w, mode->hdisplay << 16) || + drm_mode_plane_set_obj_prop(plane, state, + config->prop_src_h, mode->vdisplay << 16) || + drm_mode_plane_set_obj_prop(plane, state, + config->prop_src_x, x << 16) || + drm_mode_plane_set_obj_prop(plane, state, + config->prop_src_y, y << 16) || + dev->driver->atomic_check(dev, state); + + if (!ret) + ret = omap_atomic_commit(dev, state, NULL); + + omap_atomic_end(dev, state); + + return ret; }
-static int omap_crtc_page_flip_locked(struct drm_crtc *crtc, - struct drm_framebuffer *fb, - struct drm_pending_vblank_event *event) +static void omap_crtc_load_lut(struct drm_crtc *crtc) { - struct drm_device *dev = crtc->dev; - struct omap_crtc *omap_crtc = to_omap_crtc(crtc); - struct drm_gem_object *bo; - - DBG("%d -> %d (event=%p)", crtc->fb ? crtc->fb->base.id : -1, - fb->base.id, event); - - if (omap_crtc->event) { - dev_err(dev->dev, "already a pending flip\n"); - return -EINVAL; - } - - omap_crtc->event = event; - crtc->fb = fb; - - /* - * Hold a reference temporarily until the crtc is updated - * and takes the reference to the bo. This avoids it - * getting freed from under us: - */ - bo = omap_framebuffer_bo(fb, 0); - drm_gem_object_reference(bo); - - omap_gem_op_async(bo, OMAP_GEM_READ, page_flip_cb, crtc); - - return 0; }
-static int omap_crtc_set_property(struct drm_crtc *crtc, +static int omap_crtc_set_property(struct drm_crtc *crtc, void *state, struct drm_property *property, uint64_t val) { struct omap_crtc *omap_crtc = to_omap_crtc(crtc); struct omap_drm_private *priv = crtc->dev->dev_private; + struct omap_crtc_state *crtc_state = + omap_atomic_crtc_state(state, omap_crtc->pipe); + int ret; + +DBG("*** %s: set_property: %s = %llx", omap_crtc->name, property->name, val); + + ret = drm_crtc_set_property(crtc, &crtc_state->base, property, val); + if (!ret) { + /* we need to set fb property on our private plane too: + */ + struct drm_mode_config *config = &crtc->dev->mode_config; + if (property != config->prop_fb_id) + return ret; + }
if (property == priv->rotation_prop) { - crtc->invert_dimensions = + crtc_state->base.invert_dimensions = !!(val & ((1LL << DRM_ROTATE_90) | (1LL << DRM_ROTATE_270))); }
- return omap_plane_set_property(omap_crtc->plane, property, val); + return omap_plane_set_property(omap_crtc->plane, state, property, val); }
static const struct drm_crtc_funcs omap_crtc_funcs = { .set_config = drm_crtc_helper_set_config, .destroy = omap_crtc_destroy, - .page_flip = omap_crtc_page_flip_locked, .set_property = omap_crtc_set_property, };
@@ -306,6 +242,22 @@ enum omap_channel omap_crtc_channel(struct drm_crtc *crtc) return omap_crtc->channel; }
+int omap_crtc_check_state(struct drm_crtc *crtc, + struct omap_crtc_state *crtc_state) +{ + return drm_crtc_check_state(crtc, &crtc_state->base); +} + +void omap_crtc_commit_state(struct drm_crtc *crtc, + struct omap_crtc_state *crtc_state) +{ + struct omap_crtc *omap_crtc = to_omap_crtc(crtc); + struct omap_crtc_state *old_state = to_omap_crtc_state(crtc->state); + crtc->state = &crtc_state->base; + kfree(old_state); + omap_crtc_apply(crtc, &omap_crtc->apply); +} + static void omap_crtc_irq(struct omap_drm_irq *irq, uint32_t irqstatus) { struct omap_crtc *omap_crtc = @@ -327,6 +279,7 @@ static void apply_worker(struct work_struct *work) container_of(work, struct omap_crtc, apply_work); struct drm_crtc *crtc = &omap_crtc->base; struct drm_device *dev = crtc->dev; + struct omap_drm_private *priv = dev->dev_private; struct omap_drm_apply *apply, *n; bool need_apply;
@@ -345,6 +298,9 @@ static void apply_worker(struct work_struct *work) list_del(&apply->pending_node); }
+ if (priv->in_atomic_update) + goto out; + need_apply = !list_empty(&omap_crtc->queued_applies);
/* then handle the next round of of queued apply's: */ @@ -368,6 +324,8 @@ static void apply_worker(struct work_struct *work) omap_crtc_irq(&omap_crtc->irq, 0); } } + +out: dispc_runtime_put(); mutex_unlock(&dev->mode_config.mutex); } @@ -377,15 +335,19 @@ int omap_crtc_apply(struct drm_crtc *crtc, { struct omap_crtc *omap_crtc = to_omap_crtc(crtc); struct drm_device *dev = crtc->dev; + struct omap_drm_private *priv = dev->dev_private;
WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
/* no need to queue it again if it is already queued: */ - if (apply->queued) - return 0; + if (! apply->queued) { + apply->queued = true; + list_add_tail(&apply->queued_node, + &omap_crtc->queued_applies); + }
- apply->queued = true; - list_add_tail(&apply->queued_node, &omap_crtc->queued_applies); + if (priv->in_atomic_update) + return 0;
/* * If there are no currently pending updates, then go ahead and @@ -404,6 +366,7 @@ static void omap_crtc_pre_apply(struct omap_drm_apply *apply) { struct omap_crtc *omap_crtc = container_of(apply, struct omap_crtc, apply); + struct omap_overlay_manager_info info = {0};
DBG("%s: enabled=%d, timings_valid=%d", omap_crtc->name, omap_crtc->enabled, @@ -414,7 +377,12 @@ static void omap_crtc_pre_apply(struct omap_drm_apply *apply) return; }
- dispc_mgr_setup(omap_crtc->channel, &omap_crtc->info); + info.default_color = 0x00000000; + info.trans_key = 0x00000000; + info.trans_key_type = OMAP_DSS_COLOR_KEY_GFX_DST; + info.trans_enabled = false; + + dispc_mgr_setup(omap_crtc->channel, &info); dispc_mgr_set_timings(omap_crtc->channel, &omap_crtc->timings); dispc_mgr_enable(omap_crtc->channel, true); @@ -437,7 +405,6 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev, { struct drm_crtc *crtc = NULL; struct omap_crtc *omap_crtc; - struct omap_overlay_manager_info *info;
DBG("%s", channel_names[channel]);
@@ -450,7 +417,13 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev,
crtc = &omap_crtc->base;
- INIT_WORK(&omap_crtc->page_flip_work, page_flip_worker); + crtc->state = kzalloc(sizeof(struct omap_crtc_state), GFP_KERNEL); + + if (!crtc->state) { + dev_err(dev->dev, "could not allocate CRTC state\n"); + goto fail; + } + INIT_WORK(&omap_crtc->apply_work, apply_worker);
INIT_LIST_HEAD(&omap_crtc->pending_applies); @@ -464,16 +437,10 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev,
omap_crtc->channel = channel; omap_crtc->plane = plane; - omap_crtc->plane->crtc = crtc; + omap_crtc->plane->state->crtc = crtc; omap_crtc->name = channel_names[channel]; omap_crtc->pipe = id;
- /* TODO: fix hard-coded setup.. add properties! */ - info->default_color = 0x00000000; - info->trans_key = 0x00000000; - info->trans_key_type = OMAP_DSS_COLOR_KEY_GFX_DST; - info->trans_enabled = false; - drm_crtc_init(dev, crtc, &omap_crtc_funcs); drm_crtc_helper_add(crtc, &omap_crtc_helper_funcs);
diff --git a/drivers/staging/omapdrm/omap_drv.c b/drivers/staging/omapdrm/omap_drv.c index 04577bc..e6bf0b7 100644 --- a/drivers/staging/omapdrm/omap_drv.c +++ b/drivers/staging/omapdrm/omap_drv.c @@ -18,6 +18,7 @@ */
#include "omap_drv.h" +#include "omap_atomic.h"
#include "drm_crtc_helper.h" #include "drm_fb_helper.h" @@ -511,6 +512,10 @@ static struct drm_driver omap_drm_driver = { .dumb_create = omap_gem_dumb_create, .dumb_map_offset = omap_gem_dumb_map_offset, .dumb_destroy = omap_gem_dumb_destroy, + .atomic_begin = omap_atomic_begin, + .atomic_check = omap_atomic_check, + .atomic_commit = omap_atomic_commit, + .atomic_end = omap_atomic_end, .ioctls = ioctls, .num_ioctls = DRM_OMAP_NUM_IOCTLS, .fops = &omapdriver_fops, diff --git a/drivers/staging/omapdrm/omap_drv.h b/drivers/staging/omapdrm/omap_drv.h index 6efa51da..88717ca 100644 --- a/drivers/staging/omapdrm/omap_drv.h +++ b/drivers/staging/omapdrm/omap_drv.h @@ -27,6 +27,7 @@ #include <drm/drm_crtc_helper.h> #include <linux/platform_data/omap_drm.h> #include "omap_drm.h" +#include "omap_atomic.h"
/* APIs we need from dispc.. TODO omapdss should export these */ void dispc_clear_irqs(u32 mask); @@ -80,15 +81,6 @@ void hdmi_dump_regs(struct seq_file *s); */ #define MAX_MAPPERS 2
-/* parameters which describe (unrotated) coordinates of scanout within a fb: */ -struct omap_drm_window { - uint32_t rotation; - int32_t crtc_x, crtc_y; /* signed because can be offscreen */ - uint32_t crtc_w, crtc_h; - uint32_t src_x, src_y; - uint32_t src_w, src_h; -}; - /* Once GO bit is set, we can't make further updates to shadowed registers * until the GO bit is cleared. So various parts in the kms code that need * to update shadowed registers queue up a pair of callbacks, pre_apply @@ -147,6 +139,12 @@ struct omap_drm_private { struct list_head irq_list; /* list of omap_drm_irq */ uint32_t vblank_mask; /* irq bits set for userspace vblank */ struct omap_drm_irq error_handler; + + /* atomic: */ + bool in_atomic_update; + + /* pending vblank event per CRTC: */ + struct drm_pending_vblank_event *event[8]; };
/* this should probably be in drm-core to standardize amongst drivers */ @@ -179,6 +177,10 @@ void omap_fbdev_free(struct drm_device *dev);
const struct omap_video_timings *omap_crtc_timings(struct drm_crtc *crtc); enum omap_channel omap_crtc_channel(struct drm_crtc *crtc); +int omap_crtc_check_state(struct drm_crtc *crtc, + struct omap_crtc_state *crtc_state); +void omap_crtc_commit_state(struct drm_crtc *crtc, + struct omap_crtc_state *crtc_state); int omap_crtc_apply(struct drm_crtc *crtc, struct omap_drm_apply *apply); struct drm_crtc *omap_crtc_init(struct drm_device *dev, @@ -187,17 +189,14 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev, struct drm_plane *omap_plane_init(struct drm_device *dev, int plane_id, bool private_plane); int omap_plane_dpms(struct drm_plane *plane, int mode); -int omap_plane_mode_set(struct drm_plane *plane, - struct drm_crtc *crtc, struct drm_framebuffer *fb, - int crtc_x, int crtc_y, - unsigned int crtc_w, unsigned int crtc_h, - uint32_t src_x, uint32_t src_y, - uint32_t src_w, uint32_t src_h, - void (*fxn)(void *), void *arg); void omap_plane_install_properties(struct drm_plane *plane, struct drm_mode_object *obj); -int omap_plane_set_property(struct drm_plane *plane, +int omap_plane_set_property(struct drm_plane *plane, void *state, struct drm_property *property, uint64_t val); +int omap_plane_check_state(struct drm_plane *plane, + struct omap_plane_state *plane_state); +void omap_plane_commit_state(struct drm_plane *plane, + struct omap_plane_state *plane_state);
struct drm_encoder *omap_encoder_init(struct drm_device *dev); struct drm_encoder *omap_connector_attached_encoder( @@ -223,7 +222,7 @@ int omap_framebuffer_replace(struct drm_framebuffer *a, struct drm_framebuffer *b, void *arg, void (*unpin)(void *arg, struct drm_gem_object *bo)); void omap_framebuffer_update_scanout(struct drm_framebuffer *fb, - struct omap_drm_window *win, struct omap_overlay_info *info); + struct drm_plane_state *state, struct omap_overlay_info *info); struct drm_connector *omap_framebuffer_get_next_connector( struct drm_framebuffer *fb, struct drm_connector *from); void omap_framebuffer_flush(struct drm_framebuffer *fb, diff --git a/drivers/staging/omapdrm/omap_fb.c b/drivers/staging/omapdrm/omap_fb.c index 8025670..669afd9 100644 --- a/drivers/staging/omapdrm/omap_fb.c +++ b/drivers/staging/omapdrm/omap_fb.c @@ -154,33 +154,34 @@ static uint32_t get_linear_addr(struct plane *plane, /* update ovl info for scanout, handles cases of multi-planar fb's, etc. */ void omap_framebuffer_update_scanout(struct drm_framebuffer *fb, - struct omap_drm_window *win, struct omap_overlay_info *info) + struct drm_plane_state *state, struct omap_overlay_info *info) { struct omap_framebuffer *omap_fb = to_omap_framebuffer(fb); + struct omap_plane_state *plane_state = to_omap_plane_state(state); const struct format *format = omap_fb->format; struct plane *plane = &omap_fb->planes[0]; uint32_t x, y, orient = 0;
info->color_mode = format->dss_format;
- info->pos_x = win->crtc_x; - info->pos_y = win->crtc_y; - info->out_width = win->crtc_w; - info->out_height = win->crtc_h; - info->width = win->src_w; - info->height = win->src_h; + info->pos_x = state->crtc_x; + info->pos_y = state->crtc_y; + info->out_width = state->crtc_w; + info->out_height = state->crtc_h; + info->width = state->src_w >> 16; + info->height = state->src_h >> 16;
- x = win->src_x; - y = win->src_y; + x = state->src_x >> 16; + y = state->src_y >> 16;
if (omap_gem_flags(plane->bo) & OMAP_BO_TILED) { - uint32_t w = win->src_w; - uint32_t h = win->src_h; + uint32_t w = state->src_w >> 16; + uint32_t h = state->src_h >> 16;
- switch (win->rotation & 0xf) { + switch (plane_state->rotation & 0xf) { default: dev_err(fb->dev->dev, "invalid rotation: %02x", - (uint32_t)win->rotation); + plane_state->rotation); /* fallthru to default to no rotation */ case 0: case BIT(DRM_ROTATE_0): @@ -197,10 +198,10 @@ void omap_framebuffer_update_scanout(struct drm_framebuffer *fb, break; }
- if (win->rotation & BIT(DRM_REFLECT_X)) + if (plane_state->rotation & BIT(DRM_REFLECT_X)) orient ^= MASK_X_INVERT;
- if (win->rotation & BIT(DRM_REFLECT_Y)) + if (plane_state->rotation & BIT(DRM_REFLECT_Y)) orient ^= MASK_Y_INVERT;
/* adjust x,y offset for flip/invert: */ @@ -220,6 +221,9 @@ void omap_framebuffer_update_scanout(struct drm_framebuffer *fb, info->screen_width = plane->pitch; }
+ /* always do the rotation in tiler (or none at all): */ + info->rotation = OMAP_DSS_ROT_0; + /* convert to pixels: */ info->screen_width /= format->planes[0].stride_bpp;
@@ -316,7 +320,7 @@ struct drm_connector *omap_framebuffer_get_next_connector( if (connector != from) { struct drm_encoder *encoder = connector->encoder; struct drm_crtc *crtc = encoder ? encoder->crtc : NULL; - if (crtc && crtc->fb == fb) { + if (crtc && crtc->state->fb == fb) { return connector; } } @@ -342,10 +346,10 @@ void omap_framebuffer_flush(struct drm_framebuffer *fb, * could do the coordinate translation.. */ struct drm_crtc *crtc = connector->encoder->crtc; - int cx = max(0, x - crtc->x); - int cy = max(0, y - crtc->y); - int cw = w + (x - crtc->x) - cx; - int ch = h + (y - crtc->y) - cy; + int cx = max(0, x - crtc->state->x); + int cy = max(0, y - crtc->state->y); + int cw = w + (x - crtc->state->x) - cx; + int ch = h + (y - crtc->state->y) - cy;
omap_connector_flush(connector, cx, cy, cw, ch); } diff --git a/drivers/staging/omapdrm/omap_plane.c b/drivers/staging/omapdrm/omap_plane.c index 61b53f8..47df542 100644 --- a/drivers/staging/omapdrm/omap_plane.c +++ b/drivers/staging/omapdrm/omap_plane.c @@ -32,24 +32,14 @@ * plane funcs */
-struct callback { - void (*fxn)(void *); - void *arg; -}; - #define to_omap_plane(x) container_of(x, struct omap_plane, base)
struct omap_plane { struct drm_plane base; int id; /* TODO rename omap_plane -> omap_plane_id in omapdss so I can use the enum */ const char *name; - struct omap_overlay_info info; struct omap_drm_apply apply;
- /* position/orientation of scanout within the fb: */ - struct omap_drm_window win; - bool enabled; - /* last fb that we pinned: */ struct drm_framebuffer *pinned_fb;
@@ -58,9 +48,6 @@ struct omap_plane {
/* set of bo's pending unpin until next post_apply() */ DECLARE_KFIFO_PTR(unpin_fifo, struct drm_gem_object *); - - // XXX maybe get rid of this and handle vblank in crtc too? - struct callback apply_done_cb; };
static void unpin(void *arg, struct drm_gem_object *bo) @@ -112,24 +99,60 @@ static int update_pin(struct drm_plane *plane, struct drm_framebuffer *fb) return 0; }
+static inline bool is_enabled(struct drm_plane_state *state) +{ +#if 0 + // TODO dpms is immediate, so if it happens after state is snapshot + // as part of atomic update, it ends up getting overriden.. which + // shouldn't be a problem once we have atomic modeset.. + return to_omap_plane_state(state)->enabled && + state->crtc && state->fb; +#else + return state->crtc && state->fb; +#endif +} + +/* TODO get rid of this and convert dispc code to use drm state + * structs directly.. + */ +static void state2info(struct omap_plane_state *plane_state, + struct omap_overlay_info *info) +{ + + memset(info, 0, sizeof(*info)); + + info->global_alpha = 0xff; + /* TODO: we should calculate valid zorder from all the planes: */ + info->zorder = plane_state->zorder; + + /* update scanout: */ + omap_framebuffer_update_scanout(plane_state->base.fb, + &plane_state->base, info); + + DBG("%dx%d -> %dx%d (%d)", info->width, info->height, + info->out_width, info->out_height, info->screen_width); + DBG("%d,%d %08x %08x", info->pos_x, info->pos_y, + info->paddr, info->p_uv_addr); +} + static void omap_plane_pre_apply(struct omap_drm_apply *apply) { struct omap_plane *omap_plane = container_of(apply, struct omap_plane, apply); - struct omap_drm_window *win = &omap_plane->win; struct drm_plane *plane = &omap_plane->base; struct drm_device *dev = plane->dev; - struct omap_overlay_info *info = &omap_plane->info; - struct drm_crtc *crtc = plane->crtc; + struct omap_overlay_info info; + struct drm_crtc *crtc = plane->state->crtc; + struct drm_framebuffer *fb = plane->state->fb; enum omap_channel channel; - bool enabled = omap_plane->enabled && crtc; - bool ilace, replication; + bool enabled = is_enabled(plane->state); + bool replication; int ret;
DBG("%s, enabled=%d", omap_plane->name, enabled);
/* if fb has changed, pin new fb: */ - update_pin(plane, enabled ? plane->fb : NULL); + update_pin(plane, enabled ? fb : NULL);
if (!enabled) { dispc_ovl_enable(omap_plane->id, false); @@ -138,21 +161,13 @@ static void omap_plane_pre_apply(struct omap_drm_apply *apply)
channel = omap_crtc_channel(crtc);
- /* update scanout: */ - omap_framebuffer_update_scanout(plane->fb, win, info); - - DBG("%dx%d -> %dx%d (%d)", info->width, info->height, - info->out_width, info->out_height, - info->screen_width); - DBG("%d,%d %08x %08x", info->pos_x, info->pos_y, - info->paddr, info->p_uv_addr); + state2info(to_omap_plane_state(plane->state), &info);
/* TODO: */ - ilace = false; replication = false;
/* and finally, update omapdss: */ - ret = dispc_ovl_setup(omap_plane->id, channel, info, ilace, + ret = dispc_ovl_setup(omap_plane->id, channel, &info, replication, omap_crtc_timings(crtc)); if (ret) { dev_err(dev->dev, "dispc_ovl_setup failed: %d\n", ret); @@ -168,115 +183,51 @@ static void omap_plane_post_apply(struct omap_drm_apply *apply) struct omap_plane *omap_plane = container_of(apply, struct omap_plane, apply); struct drm_plane *plane = &omap_plane->base; - struct omap_overlay_info *info = &omap_plane->info; struct drm_gem_object *bo = NULL; - struct callback cb; - - cb = omap_plane->apply_done_cb; - omap_plane->apply_done_cb.fxn = NULL;
while (kfifo_get(&omap_plane->unpin_fifo, &bo)) { omap_gem_put_paddr(bo); drm_gem_object_unreference_unlocked(bo); }
- if (cb.fxn) - cb.fxn(cb.arg); + omap_atomic_plane_update(plane->dev, omap_plane->id);
- if (omap_plane->enabled) { - omap_framebuffer_flush(plane->fb, info->pos_x, info->pos_y, - info->out_width, info->out_height); + if (is_enabled(plane->state)) { + struct drm_plane_state *state = plane->state; + omap_framebuffer_flush(plane->state->fb, + state->crtc_x, state->crtc_y, + state->crtc_w, state->crtc_h); } }
static int apply(struct drm_plane *plane) { - if (plane->crtc) { + if (plane->state->crtc) { struct omap_plane *omap_plane = to_omap_plane(plane); - return omap_crtc_apply(plane->crtc, &omap_plane->apply); + return omap_crtc_apply(plane->state->crtc, &omap_plane->apply); } return 0; }
-int omap_plane_mode_set(struct drm_plane *plane, - struct drm_crtc *crtc, struct drm_framebuffer *fb, - int crtc_x, int crtc_y, - unsigned int crtc_w, unsigned int crtc_h, - uint32_t src_x, uint32_t src_y, - uint32_t src_w, uint32_t src_h, - void (*fxn)(void *), void *arg) -{ - struct omap_plane *omap_plane = to_omap_plane(plane); - struct omap_drm_window *win = &omap_plane->win; - - win->crtc_x = crtc_x; - win->crtc_y = crtc_y; - win->crtc_w = crtc_w; - win->crtc_h = crtc_h; - - /* src values are in Q16 fixed point, convert to integer: */ - win->src_x = src_x >> 16; - win->src_y = src_y >> 16; - win->src_w = src_w >> 16; - win->src_h = src_h >> 16; - - if (fxn) { - /* omap_crtc should ensure that a new page flip - * isn't permitted while there is one pending: - */ - BUG_ON(omap_plane->apply_done_cb.fxn); - - omap_plane->apply_done_cb.fxn = fxn; - omap_plane->apply_done_cb.arg = arg; - } - - plane->fb = fb; - plane->crtc = crtc; - - return apply(plane); -} - -static int omap_plane_update(struct drm_plane *plane, - struct drm_crtc *crtc, struct drm_framebuffer *fb, - int crtc_x, int crtc_y, - unsigned int crtc_w, unsigned int crtc_h, - uint32_t src_x, uint32_t src_y, - uint32_t src_w, uint32_t src_h) -{ - struct omap_plane *omap_plane = to_omap_plane(plane); - omap_plane->enabled = true; - return omap_plane_mode_set(plane, crtc, fb, - crtc_x, crtc_y, crtc_w, crtc_h, - src_x, src_y, src_w, src_h, - NULL, NULL); -} - -static int omap_plane_disable(struct drm_plane *plane) -{ - struct omap_plane *omap_plane = to_omap_plane(plane); - omap_plane->win.rotation = BIT(DRM_ROTATE_0); - return omap_plane_dpms(plane, DRM_MODE_DPMS_OFF); -} - static void omap_plane_destroy(struct drm_plane *plane) { struct omap_plane *omap_plane = to_omap_plane(plane); DBG("%s", omap_plane->name); - omap_plane_disable(plane); + omap_plane_dpms(plane, DRM_MODE_DPMS_OFF); drm_plane_cleanup(plane); WARN_ON(!kfifo_is_empty(&omap_plane->unpin_fifo)); kfifo_free(&omap_plane->unpin_fifo); + kfree(plane->state); kfree(omap_plane); }
int omap_plane_dpms(struct drm_plane *plane, int mode) { - struct omap_plane *omap_plane = to_omap_plane(plane); bool enabled = (mode == DRM_MODE_DPMS_ON); int ret = 0;
- if (enabled != omap_plane->enabled) { - omap_plane->enabled = enabled; + if (enabled != is_enabled(plane->state)) { + to_omap_plane_state(plane->state)->enabled = enabled; ret = apply(plane); }
@@ -319,29 +270,95 @@ void omap_plane_install_properties(struct drm_plane *plane, drm_object_attach_property(obj, prop, 0); }
-int omap_plane_set_property(struct drm_plane *plane, +int omap_plane_set_property(struct drm_plane *plane, void *state, struct drm_property *property, uint64_t val) { struct omap_plane *omap_plane = to_omap_plane(plane); struct omap_drm_private *priv = plane->dev->dev_private; - int ret = -EINVAL; + struct omap_plane_state *omap_state = + omap_atomic_plane_state(state, omap_plane->id); + int ret; + +DBG("*** %s: set_property: %s = %llx", omap_plane->name, property->name, val); + + ret = drm_plane_set_property(plane, &omap_state->base, property, val); + if (!ret) { + /* if this property is handled by base, we are nearly done.. + * we just need to register an fb property w/ atomic so that + * commit can be deferred until the fb is ready + */ + struct drm_mode_config *config = &plane->dev->mode_config; + if ((property == config->prop_fb_id) && val) + omap_atomic_add_fb(state, U642VOID(val)); + return ret; + } + + /* if it is not a base plane property, see if it is one of ours: */
if (property == priv->rotation_prop) { DBG("%s: rotation: %02x", omap_plane->name, (uint32_t)val); - omap_plane->win.rotation = val; - ret = apply(plane); + omap_state->rotation = val; } else if (property == priv->zorder_prop) { DBG("%s: zorder: %02x", omap_plane->name, (uint32_t)val); - omap_plane->info.zorder = val; - ret = apply(plane); + omap_state->zorder = val; + } else { + return -EINVAL; }
- return ret; + return 0; +} + +int omap_plane_check_state(struct drm_plane *plane, + struct omap_plane_state *plane_state) +{ + struct omap_plane *omap_plane = to_omap_plane(plane); + struct drm_crtc *crtc = plane_state->base.crtc; + struct omap_overlay_info info; + int ret, x_predecim, y_predecim; + bool replication; + + if (!is_enabled(&plane_state->base)) + return 0; + + ret = drm_plane_check_state(plane, &plane_state->base); + if (ret) + return ret; + + state2info(plane_state, &info); + + /* TODO: */ + replication = false; + + ret = dispc_ovl_check(omap_plane->id, omap_crtc_channel(crtc), + &info, replication, omap_crtc_timings(crtc), + &x_predecim, &y_predecim); + if (ret) { + DBG("%s: dispc_ovl_check failed: %d", omap_plane->name, ret); + return ret; + } + + /* TODO add some properties to set max pre-decimation.. but + * until then, we'd rather fallback to GPU than decimate: + */ + if ((x_predecim > 1) || (y_predecim > 1)) { + DBG("%s: x_predecim=%d, y_predecim=%d", omap_plane->name, + x_predecim, y_predecim); + return -EINVAL; + } + + return 0; +} + +void omap_plane_commit_state(struct drm_plane *plane, + struct omap_plane_state *plane_state) +{ + struct omap_plane_state *old_state = to_omap_plane_state(plane->state); + plane->state = &plane_state->base; + kfree(old_state); + apply(plane); }
static const struct drm_plane_funcs omap_plane_funcs = { - .update_plane = omap_plane_update, - .disable_plane = omap_plane_disable, .destroy = omap_plane_destroy, .set_property = omap_plane_set_property, }; @@ -360,7 +377,6 @@ struct drm_plane *omap_plane_init(struct drm_device *dev, struct omap_drm_private *priv = dev->dev_private; struct drm_plane *plane = NULL; struct omap_plane *omap_plane; - struct omap_overlay_info *info; int ret;
DBG("%s: priv=%d", plane_names[id], private_plane); @@ -385,6 +401,13 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
plane = &omap_plane->base;
+ plane->state = kzalloc(sizeof(struct omap_plane_state), GFP_KERNEL); + + if (!plane->state) { + dev_err(dev->dev, "could not allocate CRTC state\n"); + goto fail; + } + omap_plane->apply.pre_apply = omap_plane_pre_apply; omap_plane->apply.post_apply = omap_plane_post_apply;
@@ -393,24 +416,11 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
omap_plane_install_properties(plane, &plane->base);
- /* get our starting configuration, set defaults for parameters - * we don't currently use, etc: - */ - info = &omap_plane->info; - info->rotation_type = OMAP_DSS_ROT_DMA; - info->rotation = OMAP_DSS_ROT_0; - info->global_alpha = 0xff; - info->mirror = 0; - /* Set defaults depending on whether we are a CRTC or overlay * layer. - * TODO add ioctl to give userspace an API to change this.. this - * will come in a subsequent patch. */ - if (private_plane) - omap_plane->info.zorder = 0; - else - omap_plane->info.zorder = id; + if (!private_plane) + to_omap_plane_state(plane->state)->zorder = id;
return plane;
On Sun, Sep 9, 2012 at 10:03 PM, Rob Clark rob.clark@linaro.org wrote:
oh, and for those wondering what the hell this is all about, nuclear-pageflip is a pageflip that atomically updates the CRTC layer and zero or more attached plane layers, optionally changing various properties such as z-order (or even blending modes, etc) atomically. It includes the option for a test step so userspace compositor can test a proposed configuration (if any properties not marked as 'dynamic' are updated). This intended to allow, for example, weston compositor to synchronize updates to plane (sprite) layers with CRTC layer.
BR, -R
On Sun, 9 Sep 2012 22:19:59 -0500 Rob Clark rob@ti.com wrote:
Can we please name this something different? I complained about this in IRC, but I don't know if you hang around in #intel-gfx.
Some suggestions: multiplane pageflip muliplane atomic pageflip atomic multiplane pageflip atomic multiflip pageflip of atomic and multiplane nature
Nuclear is not at all descriptive and requires as your response shows :-)
On Tue, Sep 11, 2012 at 4:15 PM, Ben Widawsky ben@bwidawsk.net wrote:
fair enough.. I was originally calling it atomic-pageflip until someone (I don't remember who) started calling it nuclear-pageflip to differentiate from atomic-modeset. But IMO we could just call the two ioclts atomic-modeset and atomic-pageflip. (Or even modeset2 and pageflip2, but that seems much more boring)
BR, -R
On Tue, Sep 11, 2012 at 05:07:49PM -0500, Rob Clark wrote:
My plan has been to use the same ioctl for both uses. They'll need nearly identical handling anyway on the ioctl level. I have something semi-working currently, but I need to clean it up a bit before I can show it to anyone.
On Wed, Sep 12, 2012 at 3:59 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
I do think the atomic-pageflip ioctl really should take the crtc-id.. so probably should be two ioctls, but nearly identical
BR, -R
On Wed, Sep 12, 2012 at 07:30:18AM -0500, Rob Clark wrote:
But then you can't support atomic pageflips across multiple crtcs even if the hardware would allow it. I would hate to add such limitation to the API. I can immediately think of a use case; combining several smaller displays to form a single larger display.
With a unified API you could just add some kind of flag that tells the kernel that user space really wants an atomic update, and if the driver/hardware can't do it, it can return an error.
On Wed, Sep 12, 2012 at 9:23 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
well, that is really only a problem for X11.. and atomic flip across multiple crtc's is a potential mess from performance standpoint unless your displays are vsync'd lock.
weston already renders each output individually, rather than spanning a single fb across multiple displays like x11 does. So this problem really doesn't exist for weston.
BR, -R
On Wed, Sep 12, 2012 at 09:28:43AM -0500, Rob Clark wrote:
It won't be truly atomic unless they are vsync locked. But anyways more buffers can be used to solve the performance problem. But that's a separate issue and in that case it doesn't really matter whether you issue separate ioctls for each crtc.
It does if you want to make sure the user sees the flip on both displays at the same time.
On Wed, Sep 12, 2012 at 9:34 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
that was basically my thinking.. separate ioctls for each crtc. The way my branch works currently, you can do this. A page-flip on crtc #2 won't care that there is still a flip pending on crtc #1.
I guess that doesn't strictly guarantee that the two pageflips happen at the exact same time, but unless you have some way to vsync lock the two displays, I don't think that is possible anyways. So I'm not really sure it is worth over-complicating the ioctl to support two crtc's. The error checking in case a vsync is still pending is much easier in the driver if you know the crtc up-front at the atomic_begin() point. Which is why I prefer to pass the crtc_id as a field in the ioctl.
BR, -R
On Wed, Sep 12, 2012 at 09:42:27AM -0500, Rob Clark wrote:
Sure you need hardware to keep the pipes in sync.
Doing such checks in atomic_begin() is way too early. Unless you want to block/return immediately if there's a pending flip.
I want to allow user space to force feed the driver with flips at speeds greater than the display refresh. The last frame to finish rendering before the vblank is the one that should end up on the screen. That way you can do tear-free triple buffering without throttling to screen refresh, which is great for benchmarks. It's also a nice way to support cases where you want to throttle to a 60 Hz display, but you still want to clone the content to say a 24 or 30 Hz display.
On Wed, Sep 12, 2012 at 10:12 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
I want to return -EBUSY immediately if there is a pending flip.
Hmm, I still think that userspace should handle this.
Keeping the existing semantics of -EBUSY when there is a pending flip makes it easier to implement legacy page_flip on top of the atomic APIs so the driver doesn't have to care about the difference. I don't really see the problem w/ userspace dropping frames (depending on egl swap-interval) if there is still a pending page-flip.
BR, -R
On Wed, Sep 12, 2012 at 10:23:48AM -0500, Rob Clark wrote:
It will have to drop the most recent frames, whereas the kernel implementation can drop the older frames, while keeping the latest one. This will help minimize latency.
I know OMAP isn't really well suited for this due to the GO bit semantics, but I don't want to punish all hardware through the API design.
On Wed, Sep 12, 2012 at 10:32 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
Hmm, ok.. that is a good point. Well, I suppose we could pass page_flip->flags in to atomic_begin(), and add a flag to specify drop-behavior if you are trying to flip faster than vsync. Probably also need to add a driver cap to indicate whether or not the hw is capable of this.
But I think we could still do this w/ one ioctl per crtc for atomic-pageflip.
yeah.. well adding a cap plus flag, and passing the flags into atomic_begin() seems like it should do the job.
(on o4, we could actually remap buffers in dmm/tiler synchronized w/ scanout.. or even not sync'd w/ scanout if you want tearing. It is a slightly round-about way to do flipping, but I suppose in theory we could support that on hw newer than o3.)
BR, -R
On Wed, Sep 12, 2012 at 10:48:16AM -0500, Rob Clark wrote:
You could implement the "drop most recent" behaviour without issuing the ioctl at all. You just check whether you already received the completion event for the previous flip.
BTW there's a one extra complication in the "drop oldest" behaviour. The completion event should somehow carry information telling which fbs became idle when the flip completed.
But I think we could still do this w/ one ioctl per crtc for atomic-pageflip.
We could, if we want to sacrifice the synced multi display case. I just think it might be a real use case at some point. IVI feels like the most likely short term cadidate to me, but perhaps someone would finally introduce some new style phone/tablet thingy. I have seen concepts/prototypes of such multi display gadgets in the past, but the industry apparently got a bit stuck on the rectangular slab with touchscreen on one side design.
On Wed, Sep 12, 2012 at 12:27 PM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
I could be wrong, but I think IVI the screens would normally be too far apart to matter?
Anyways, it is really only a problem if you can't do two ioctl()s within one vblank period. If it actually turns out to be a real problem, we could always add later an ioctl that takes an array of 'struct drm_mode_crtc_atomic_page_flip's. I'm not sure if this is really useful or not.. but maybe I'm thinking too much about how weston does it's rendering of different output's independently.
I wonder.. if you have some special hw setup where you can keep the multiple display's in sync-lock, maybe a special virtual crtc would work. That way userspace it appears as a single display. I'm not really sure how crazy that would be to implement. But I think in the common case, where the displays are not vsync locked, treating the page flips of different crtc's independently, and rendering the contents for different outputs independently (like weston) makes the most sense.
BR, -R
On Wed, Sep 12, 2012 at 01:00:19PM -0500, Clark, Rob wrote:
I was thinking of something like a display on the dash that normally sits low with only a small sliver visible, and extends upwards when you fire up a movie player for example. Internally it could be made up of two displays for power savings purposes.
Well exactly that's the problem this whole atomic pageflip stuff is trying to tackle, no? ;)
I'm just now thinking of surfaceflinger's way of doing things, with its prepare and commit phases. If you need to issue two ioctls to handle cloned displays, then you can end up in a somewhat funky situation.
Let's say you have a video overlay in use (one each display naturally), and you increase the downscaling factor enough so that you now have enough memory bandwith to support only one overlay. With independent check ioctls for each display, you never have the full device state available in the kernel, so each check succeeds during the prepare phase. So you decide that you can keep using the video overlays.
You then proceed to commit the state, but after the first display has been commited you get an error when trying to commit the second one. What can you do now? The only option is to keep displaying the old frame on the other displays for some time longer, and then on the next frame you can switch to GPU composition. But on the next frame you perhaps no longer need to use GPU composition, but since you can't verify that in the prepare phase, you have no other option but to use GPU composition.
So when you run into a configuration that can be supported only partially, you get animation stalls on some displays due to skipped frames, and you always have to fall back to GPU composition for the next frame.
If on the other hand you would check the whole state in one ioctl, you'd get the error in the first prepare phase, and could fall back to GPU composition immediately.
Am I too much of a perfectionst in considering such things? I don't think so, but perhaps other people disagree.
Sure, add more abstraction layers and you can solve any problem :)
My API design doesn't preclude that at all. In light of my android tale above how would weston decide whether it can use overlays in such a case?
On Wed, Sep 12, 2012 at 1:58 PM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
Ok, if you have a case where the state of the two crtc's are not actually independent, then I think you have a valid point.
I'm not quite sure what userspace would do about it, though.. for the general case where vsync isn't locked, and you can't even necessarily assume vsync period is the same, then I don't really think you want to couple rendering to the displays. OTOH, the 'test' step can come independent of vblank, so maybe you'd want to do the 'test' step together, and then the flip parts independently. Hmm...
well, not really.. but my point was this sort of setup would be a somewhat custom hardware setup, so maybe some hack in that case isn't such a bad idea. I dunno..
From userspace API, I guess something like:
struct drm_mode_crtc_atomic_page_flip { uint32_t flags; uint32_t count_crtcs; uint64_t crtc_ids_ptr; /* array of uint32_t */ uint64_t count_props_ptr; /* array of uint32_t, # of prop's per crtc */ uint64_t props_ptr; /* ptr to array of drm_mode_obj_set_property */ uint64_t user_data; };
Or maybe just have something similar to my current ioctl, and then a drm_mode_crtc_multi_atomic_page_flip which takes an array of drm_mode_crtc_atomic_page_flip.
Or possibly just use atomic_mode_set ioctl for the test step, and atomic_page_flip per crtc for the page-flip step. The test step can be done independently of vblank, so it doesn't really matter if there is a vblank pending or not.
BR, -R
On Wed, Sep 12, 2012 at 02:40:56PM -0500, Rob Clark wrote:
I would say this is going to be the most common use case if you consider just the number of shipping devices. It's pretty much what every Android phone/tablet with a HDMI port has to do.
The solution we came up when working with Medfield was to throttle to the internal display (usually ~60Hz), and let the external display drop however many frames it needs. But I still wanted the external display to always show the most recent frame possible, hence I came up with drm_flip which supports that just fine. So we used three buffers and just issued flips at the rate of the internal display, and the external display with a 30Hz or 24Hz refresh rate ended up dropping some of the frames. It did look fairly good actually.
As a proof of the drm_flip implementation, I also did a quick hack where I bumped the number of buffers up to ~15 or so, and then removed the display related throttling completely (apart from preventing the GPU from writing to an active scanout buffer). This allowed the app and compositor to render at ~400 fps, and each displays would end up displaying 60 or 30 of those frames each second without tearing. Now, the only reason I needed so many buffers was that Android's buffer management rotates buffers in strict order, and I would've needed to modify that code to do proper triple buffering.
I had an idea about optimizing the check+commit by having check save the checked state, and return a cookie of some sort. Then the commit phase could simply pass in that same cookie, and the kernel wouldn't need to re-check the state on commit. Obviously if any other mode setting operation would happen in between the check and commit steps, the cookie would need to be invalidated. This wouldn't work with the single check with multiple commits. But I'm not sure such an optimization would be worthwile.
At least Medfield has support for such setups out of the box as it were. You can hook up two DSI displays to it, and it can run the two pipes in sync.
As a less relevant example I give you the Matrox G550 :) where you actually have to sync the CRTCs if you want to drive both DVI outputs at the same time. But I'll concede that the Matrox is a special case since other independent dual output configurations on this chip don't require such magic.
Starting to look much like my drm_mode_atomic struct :)
Let's compare:
struct drm_mode_atomic { __u32 flags; __u32 count_objs; __u64 objs_ptr; __u64 count_props_ptr; __u64 props_ptr; __u64 prop_values_ptr; __u64 blob_values_ptr; };
One differences seem to be that you have a mix of SOA and AOS concepts in yours, whereas mine is pure SOA.
Also your struct has a bunch of redundant data since drm_mode_obj_set_property keeps repeating the object ID needlessly, and crtc_ids_ptr is also just repeating information already present in drm_mode_obj_set_property. drm_mode_obj_set_property further forces you to pass in the object type, which is pretty much useless.
Your version can't pass in blobs, which is going to be a problem when you want to push in gamma tables/ramps and whatnot. Also I've been considering that for atomic modesetting I'd pass display modes as blobs too, since the display mode object IDs are currently hidden from user space, and also the IDs are a bit volatile since the modes can be shuffled around during EDID probing. I suppose one could try to fix these display mode ID issues, and when you need to push in gamma tables or other heavy weight data, you invent new types of drm objects to describe them, and you pass them by ID as well.
On Thu, Sep 13, 2012 at 3:40 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
bleh, surfaceflinger kinda sucks then..
an interesting idea.. I'd rather try and keep it simple first. OTOH, if done right, the tracking and invalidating could pretty much be done in drm core rather than the driver (which seems like a good thing, when something that is easy to screw up and not really too much hw related can be done once in the core, rather than N buggy times in N different times in each driver)
well, you do miss userdata, I think
One differences seem to be that you have a mix of SOA and AOS concepts in yours, whereas mine is pure SOA.
well, I was trying to re-use drm_mode_obj_set_property because that seemed to keep the code simple, and makes it more similar to existing set-property stuff. OTOH, maybe that doesn't really matter because userspace would normally be going through libdrm and not seeing the ioctl structs directly, so we could make the ioctl structs as ugly as we want.
I'm a bit on the fence about how the pageflip ioctl should look, in particular about pageflip on multiple CRTCs. I'd like to know the involved CRTC(s) upfront, as that seems to make error checking easier on the driver (in case of already pending flip). Although I'll think a bit about alternatives.
Also, if you pageflip on multiple CRTC's, should the be multiple vblank events, and multiple userdata's? I seem to remember our android guys had some performance issues in this area too. Well, really I think one pageflip across multiple unsync'd displays is really just a horrible idea, but I guess it is best to try and accommodate surfaceflinger's brokenness as best as possible.
Really the atomic-pageflip ioctl is pretty much a small part of the entire patchset, so I'm not really too much against changing it. Most of the point of the patchset was to try to enhance properties to do what we need, and come up w/ a sane way to decouple state from the various kms objects to easily handle atomic commit or rollback.
well, not completely, the object-id/type could also be a plane attached to a crtc..
hmm, ok, I was wondering why you were supporting blobs, when existing setprop APIs where not. Well, maybe it wouldn't be a bad idea to start by making existing setprop stuff support blobs.
BR, -R
On Thu, Sep 13, 2012 at 08:39:54AM -0500, Rob Clark wrote:
Why? This use case is not enforced by surfaceflinger, it's just the use case most devices would have.
I don't think there's anything wrong with the way surfaceflinger is designed with the prepare and commit phases. How else would you do it?
Sure, because I didn't add the event stuff yet.
I sort of modelled mine after the way things are done by the various getter ioctls. I don't like messing up my brain by mixing SOA and AOS in the same place.
I don't see much point in iterating that information directly in begin(). You anyway get the same information during the set() operations. I suppose you could save some effort by avoiding some state allocation in begin(). But really, this seems like optimizing for the an uncommon error case. You should not encounter it in practice unless your user space is doing something silly.
Also, if you pageflip on multiple CRTC's, should the be multiple vblank events, and multiple userdata's?
That's a bit of an open question. I was considering several options:
1) One event after the full operation is complete Not a good idea when you want to throttle based on a fast refreshing display but the system has also slow refreshing displays.
2) One event per pipe Seems reasonable. We could use a bitmask so that the user can ask for the event to be delivered only for specific pipes. I'm not sure whether multiple user_datas would be better, or just the one with user space taking care of refcounting it in case it's a pointer to some object.
3) One event per scanout engine Not sure this makes sense since the idea is anyway to do things atomically within the pipe.
I agree 100% with this goal. Once the state would be split cleanly, we could do stuff like keep the user and fbcon states totally separate. Restoring the fbcon state would then trivial. Currently the fbcon state as such doesn't really exist. IIRC you had some patches to reset various bits of state at certain points just to work around this issue.
Sure, but even that gets repeated needlessly for every prop.
I wouldn't care about the existing APIs. Why would anyone use them when the atomic API provides the same capability and more?
On Thu, Sep 13, 2012 at 9:29 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
well, maybe I misunderstood how surfaceflinger works, but it sounded like it has one prepare/commit phase across outputs, vs what weston compositor does where each output is rendered and flipped independently at the rate of that particular output. If the two outputs just happen to be vsync aligned, you would end up flipping at the same time, but if the are not locked you don't have any artificial constraint in the rendering/flipping.
note that the test phase doesn't need vblank events, and also shouldn't -EBUSY if there is still a pending flip[*], so I'd propose that however we go about pageflip (one super-ioctl, or one per crtc), we could use the atomic-modeset ioctl for the test step
[*] this is one thing not correct still in my current patches.. the driver doesn't know the difference between test and real-flip at the point where it checks if there is a pending flip.
fair enough.. anyways, that was just a quick proposal in an email thread, we can tweak how the data is layed out
well, it was more about trying to make the error checking easy to get right in the drivers. But maybe it is easy enough to keep track of the involved crtc(s) and do the check in the atomic_test() step.
the thing I like about one ioctl per crtc is that it avoids this whole question..
And, I think as long as you have to update multiple different scanout address registers, there is always going to be a race in multi-crtc flipping. Having a single ioctl does make the race smaller. I'm not sure how important that point is.
well, if you aren't vsync locked, it isn't going to be atomic anyways.. and not vsync locked seems like the common case to me.
one minor point: I was thinking the same thing but Daniel pointed out that we still have to deal w/ the potential case that the user unplugged one display and plugged a different display between leaving fbcon and restoring fbcon. So we always have to deal with the potential for full modeset. But maybe that is ok, we perhaps could just let the atomic_test() step fail in this case and then trigger full modeset? I'm not quite sure, I haven't thought through this fully, but it does seem like there is potential to simplify things. At any rate, it isn't so important for the initial patchset, but it is worth thinking about when we get to the atomic-modeset part and figuring out how helpers, etc, fit in to this.
well, for libdrm API, what made sense to me is just an array of property type+id+value.. libdrm *could* support this by sorting the props and flattening things out. It would make the amount of data that is copy_from_user()'d a bit more compact. But I am not sure that this is really worth worrying about. Especially if you are just passing the values that have changed since last flip. I'd lean towards keeping it simple rather than optimizing out a few bytes of copy_from_user().
well, I guess there is no strong need to.. but it just seemed strange to require using the new API to set a single property. Anyways, it could be that it is not worth worrying about.
Fwiw the way drm_ioctl() works, it should be possible to add new fields to an ioctl struct at the end, as long as zero has a sensible / backwards-compatible meaning. Ie. old userspace w/ smaller struct on new kernel, the driver will just see the new fields memset to zero. New userspace on old kernel, drm_property_change_is_valid() would fail.
BR, -R
On Thu, Sep 13, 2012 at 11:35:59AM -0500, Rob Clark wrote:
OK so it's purely a pull based model, whereas surfaceflinger is more push based.
I suppose it might be possible to make surfaceflinger support a pull model by driving the compositor loop through a combined signal from multiple outputs. But IIRC it did have some timing related code in there somewhere, so it might not be happy about it. It might also affect the clients' rendering speed since the compositor would be pulling their buffers from queue at non-constant speed. I don't remember the details of the buffer management very well, so I can't be sure though. But I probably wouldn't bother trying this, since the straightforward approach is so simple, and the results are reasonably good.
The pull model does seem more flexible. But it does require a bit of extra complexity in the compositor to avoid compositing the same scene multiple times needlessly when multiple cloned displays are involved. I suppose ideally you'd want to recompose for each display to minimize visible latency, but from power usage POV it may not be a good idea.
Right. Personally I'm not a fan of the EBUSY behaviour at all. Seems a bit pointless since user space can take care of it via the event mechanism. But I suppose you want it for omap so that you can avoid having to write software workarounds to overcome the GO bit limitations.
Yeah that seems reasonable. If we do that, then it doesn't matter that much whether we have commit per pipe or not, except for the synced displays case, which I'd still like to support if possible. At least someone explicitly wanted such a feature in Medfield. I assume they had a fairly compelling use case because they were able to convince the hardware designers of the need.
Sure. The layout isn't that important to me either. All the ioctl structures are already ugly so we can't make the situation much worse :)
Which race?
One reason why I considered the one event per scanout engine is that some overlay implementations can have their own "vblank/flip done" interrupt independent from the whole pipe. Such an interrupt would fire as soon as the scan has passed over the overlay's visible region, even if that happens to be well inside the crtc's active video region. It could allow the rendering for the next frame to begin slightly earlier when there's a shortage of buffers.
Yeah, the displays changing does pose a problem. I think ideally you'd take the saved state, but instead of commiting it as is, you'd adjust it to accomodate the new display layout.
Did you check the libdrm API I have? It frees the client from having to collect any property arrays because that could turn into a realloc() fest quite quickly. And the implementation itself is hidden inside libdrm so if the linked list based implementation turns out insufficient it's possible to change the data structrures.
It keeps the client code much simpler.
Really? Isn't the struct size encoded into the ioctl ID itself?
On Fri, Sep 14, 2012 at 7:50 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
[snip]
As I understood, at least in older versions android versions, rendering was based on a timer as there was no vblank event to userspace on most SoC platforms (which sounds strange, but so far most SoC's are using fbdev and/or crazy hacks rather than drm/kms)
not sure if the timer is still there.. but I hope it goes away, it is really a horrible way to keep track of vsync
fwiw, weston is already being pretty clever about keeping track of damage and minimizing the area of the screen that must be re-rendered. I'm not sure if SF does anything like this.
I the main issue is disconnecting an overlay from one crtc and connecting to another.. I would expect that any hw which can connect an ovl to more than one possible crtc would have the same limit (ie. have to wait until scanout on previous crtc completes), so I think EBUSY is a good way to indicate to userspace that the requested configuration is not possible *now* but would be possible in the future.
heheh, well, it wouldn't be the first time I've seen support for crazy features in hw :-P
but anyways, I guess I'd be interested in hearing more opinions on whether it is worth worrying about
ie. if you set REG_CRTC1_ADDR just immediately before vblank and REG_CRTC2_ADDR just after
hmm, ok, so even more than one event per CRTC, but one per CRTC+plane.. well, we can use this on omap, although I'm not quite sure the tradeoff (slighly less latency to release the buffer, vs more IRQ's and waking up the CPU more..).
I guess it isn't too critical now, we can always add a new flag later.
actually I didn't even realize you had a libdrm branch until now.. I'll have a look
yes, but the dispatch code in drm_ioctl() only considers the ioctl # bitfield in figuring out which handler to dispatch the ioctl to. It uses the size field in the copy_{to,from}_user(). Thanks to the size encoded in the ioctl # we can deal w/ new fields added at the end.
(well, slight complication that older kernels didn't zero out the excess ioctl size if userspace passes a smaller struct. But that should only be a problem if we made ioctl structs *smaller*, so it should be ok)
BR, -R
On Fri, Sep 14, 2012 at 08:25:53AM -0500, Rob Clark wrote:
I've only looked at ICS in any detail. At least there we used the page flip event from one display to set the pace of the compositor loop. IIRC JB is supposed to have some vsync related changes, but I haven't looked at the code.
IIRC it can do that, but the EGL implementation needs to support EGL_BUFFER_PRESERVED.
I suppose the best way to implement EGL_BUFFER_PRESERVED with page flips would be to schedule the flip and immediately perform a blit from the new front buffer to the new back buffer. Well, unless the hardware has some more clever mechanism for it.
Does weston depend on preserved flips too, or can it even track damage independently for each buffer?
Intel HW can do the transition automagically, but if you try to combine it with other page flips, the driver would have to perform some gynmastics to make things appear atomic. Of course if you'd try to swap overlay A from pipe 1 to pipe 2, and overlay B from pipe 2 to pipe 1 at the same time, there's just no way to do that without sacrificing atomicity on one of the pipes.
So even with such HW, it's probably easier to forget about the feature, and require user space to perform the disable+enable sequence in two steps.
Well, with unsynced crtcs I wouldn't call that any kind of meaningful race. The same problem after all exists even with a single crtc. You either make the deadline and write the register before vblank, or you don't make it and end up with a repeated frame.
On Fri, Sep 14, 2012 at 8:58 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
well, weston knows how many buffers are at play. So it takes the union of the damage from the last time the buffer was used (well, currently it assumes only double buffered) and the new damage. This way it avoids need for the gl driver, which doesn't know as well what is going on as the app, from needing to do a back-blit. It can do this because w/ drm/gbm egl winsys, eglSwapBuffers() doesn't actually swap the buffers on the display and weston is in charge of which buffer is displayed or rendered. Weston explicitly calls page flip ioctl. The good news being that it can atomically flip overlay layers at the same time once the new ioctl is in place.
Maybe it is useful to look at http://github.com/robclark/kmscube .. it doesn't actually use planes, but shows the interaction of egl and kms. Maybe I should enhance it w/ multiple rotating cubes on different overlays. ;-)
true, but I don't want to block the disable until vblank w/ atomic-pageflip, and if userspace re-enables the plane on a different crtc before the next vblank, it would be useful for the driver to have a way to say 'try again later'.
And if we do support multiple crtc's w/ pageflip, I'm not sure if there is a good way to enforce two-steps. Having a standardized way to tell userspace to try later seems like a good thing.
I meant w/ sync'd crtc's, there is still no 100% guarantee that the two flip at the same time. With unsync'd crtc's there is no point for the single ioctl.
BR, -R
On Fri, Sep 14, 2012 at 09:45:18AM -0500, Rob Clark wrote:
With more buffer it'll get a bit more complicate as it needs to keep accumulating the damage for all buffers. But it should still be fairly trivial when you're in full control of the buffers.
Yeah, with EGL in the mix, as can be the case with Android, the layering can start to work against you a little bit. Well, it's not too bad based on my experience though.
When doing the Medfield work, I had a test case which utilized the video overlays and the primary plane. What it did was draw ugly colored rectangles on the primary layer, and positioned the overlays to cover those up exactly. When the atomic page flip system was operating as intended you could only see a solid color on the screen. It randomly changed the position and size of the rectangles and overlays. I really need to dig that up modify it to work with my current code.
Yeah, trying to do it all asynchronously in the kernel would perhaps be too much work for little gain.
I wonder if you've though about omap's FIFO merge. It can cause similar issues, that is some operations may need two vblanks to complete. And it looks like I'll get to worry about this stuff too since there are some watermark related wait_for_vblank() workarounds in the IVB sprite code, sigh.
Sure, for that it seems reasonable.
Sure there is. That's what the vblank evade stuff gives you. I just happen to need it even when using just one crtc because the hardware doesn't have the necessary mechanism to flip several planes atomically.
On Fri, Sep 14, 2012 at 10:48 AM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
well, just track previous damage per buffer.. but yeah, slightly more complicated
it would be nice to resurrect this using drm/gbm stuff so we can use it as test code on multiple platform. (I've tested kmscube on i915 and omap)
yeah, FIFO merge is a nice big headache.. and not really ideal for latency unless you have some advanced warning to disable FIFO merge before userspace wants to switch on an extra overlay.
I think the best way to deal is just start switching off FIFO merge when userspace first does test w/ overlay, but return EBUSY. It means we'll use the gpu for rendering for one frame, but I think that is better than blocking the compositor for a vblank or two. Thou shalt not block the compositor.
hmm, I guess I don't quite follow then. But I guess I don't know the intel hw well enough. It seemed like you weren't atomically updating scanout registers.
But anyways, I think it is probably ok to not need the crtc up-front. We can catch issues w/ pending vblank at the atomic_test() stage. Still not sure what to do about userdata. Although I suppose we could make userdata a property attached to crtc and/or plane and that gives userspace plenty of flexibility about how many events it wants back. (Ie. no event if userdata==0.. or maybe separate send-event property.)
BR, -R
On Fri, Sep 14, 2012 at 11:29:04AM -0500, Rob Clark wrote:
Yeah, I suppose it could be handled through another property as well. Perhaps some kind of "LOW_POWER_OPTIMIZATIONS" property that you'd disable one vblank in advance. But then it's starting to be a bit hardware specific ie. you more or less have to know the circumstances when the property must be disabled. On omap it would be when more than one plane is used, on IVB it appears that you need it when you want to enable scaling.
I'm not too thrilled about the idea that the test phase would actually touch the hardware. What happens if you do multiple test steps between commits? But I can't immediately think of a good solution that would avoid the need for hardware specific knowledge.
I guarantee the atomicity by making sure I'm not too close to the start of vblank when I write the registers. It's a very generic solution that will work on any hardware with double buffered registers that get flipped on vblank. Even if some of the registers would get flipped at slightly different times (eg. plane A flips at vbl_start+1, plane B at vbl_start+10) you could still use this method by extending the range of scanlines to be avoided.
I suppose the most difficult bit is determining how long you need to write all the necessary registers. You want to make it long enough to guarantee atomic operation, but short enough to avoid blocking needlessly. Currently I just have a hardcoded number there. If the driver is going to be deployed on a specific device, it's easy enough to measure it and tweak the number, but it would be nice to have the driver calibrate itself. Or you just have a sysfs knob so that it could tweaked more easily for specific systems by non-developers.
I think that if the event carries the crtc id, then the user_data could be shared across crtcs. User space can ref count stuff if necessary. When done this way you can think if the user_data as a cookie that identifies the flip event(s) originating from a specific flip request.
Hmm. That's an interesting idea. I'll have to give it some thought.
On Fri, Sep 14, 2012 at 12:02 PM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
yeah, that is the problem..
But then again, I suppose we could make fifo-merge disabled by default and explicitly controlled by userspace via property. A generic userspace, and the property would never be set. A userspace a bit more optimized/customized for the hw, however, is in a better position to know if there is likely to be changes in the next frame (ie. based on user input, etc) and could make perhaps some more intelligent decisions about when to enable it.
ahh, ok, double-buffered.. well, if they are double buffered you should be able to tolerate two ioctl() calls, because you have a relatively large window to update all the registers ;-)
It does give userspace plenty of control about how it want's it's events, which is nice.. although I'm not quite sure yet about the implementation. The point at which you create the vblank evt, you want to know the 'struct drm_file *'.. and it seems a bit strange to have to pass that in to set_property() ;-)
BR, -R
On Fri, Sep 14, 2012 at 12:34:59PM -0500, Rob Clark wrote:
Hey, if "should" would be good enough, there would be no need for an atomic page flip ioctl.
And somehwat ironically, if I didn't have double buffered registers, I'd just write the lot of them from the vblank irq handler, which would be simpler in some sense. Well, to tell the truth, not all registers in Intel HW are double buffered. Gamma tables/ramps for example are single buffered, and if we actually start to care about accurate color reproduction we may need to mix the two approaches. The other approach would be to reject changes to features backed by single buffered registers while the relevant piece of hardware is enabled.
On Fri, Sep 14, 2012 at 1:23 PM, Ville Syrjälä syrjala@sci.fi wrote:
Well, true.. but there is a bit of a difference of scale.. I mean flipping multiple layers on a single CRTC that is not vblank sync'd with another CRTC should be a common case, and you can get incorrect results on the screen, so an "it *should* work" solution is less acceptable.
vsync locked crtc's seem like it would be less common, and less likely to be noticed if once in a while the flip is off by a frame. So it seems like a less urgent issue to solve.
Just playing devil's advocate here ;-)
BR, -R
On Thu, Sep 13, 2012 at 11:35 AM, Rob Clark rob.clark@linaro.org wrote:
actually, I think I take this back.. one thing that was discussed on IRC, but didn't make it to this email thread is the behavior of non-specified properties. What I am thinking:
modeset: unspecified properties revert to default pageflip: unspecified properties preserve current value
So I definitely do think there should be two ioctls, and that test for pageflip should go via atomic-pageflip ioctl to be consistent w/ the preserve-current-values approach. Instead I'll just move the is-there-a-pending-vblank to the top of atomic_commit() so it doesn't get in the way if you try to test for frame n+1 while waiting for vblank from frame n.
BR. -R
On Fri, Sep 14, 2012 at 09:12:35PM -0500, Rob Clark wrote:
Why on earth would you want to revert stuff to default? That's only going to make the code more complex.
On Sat, Sep 15, 2012 at 9:56 AM, Ville Syrjälä syrjala@sci.fi wrote:
well, you need to do it *somewhere*.. possibly it can be on drm file close or dropmaster. But modeset seems like a sensible place. I really hate the v4l2 approach of preserving settings for the next process that opens the device.
BR, -R
On Sat, Sep 15, 2012 at 11:07:02AM -0500, Rob Clark wrote:
Ah so it's the same workaround for lack of proper state management. Each master should just have its own state. Or if that's too much to ask, at least the reset could be done only when the master changes.
If you do it at modeset time, which props do you reset anyway? All of them for the whole device? Just the ones related to the CRTCs undergoing the modeset? What if there's some conflict between the default values on that CRTC and the current values on another CRTC? What about properties for planes that can move across CRTCs? This kind of partial state reset opens up a lot of open questions, so a full reset at master switch seems a lot more sensible.
On Sat, Sep 15, 2012 at 12:04 PM, Ville Syrjälä syrjala@sci.fi wrote:
Well, if you reset *all* properties on modeset, then crtcs's that aren't set in the modeset go off.. atomic-modeset is userspace saying "here is the entire config I want.. go make it happen". But I guess it does get a bit easier to implement legacy setcrtc on top of the new mechanism if untouched properties preserve their value.
I could live w/ just reset on master change.. that meets my minimum requirement of not carrying state between different processes using the device. Having a flag indicating 'reset untouched properties' would be useful if the default behavior is to preserve.
I still think setcrtc and pageflip shouldn't be mashed into a single ioctl :-)
BR, -R
On Sat, Sep 15, 2012 at 12:15:59PM -0500, Rob Clark wrote:
Yeah. I don't see much point in maintaining the state stometimes, but sometimes not. Either do or do not.
Having a flag indicating 'reset untouched properties' would be useful if the default behavior is to preserve.
Perhaps. Or perhaps some way to query the default values of properties.
I still think setcrtc and pageflip shouldn't be mashed into a single ioctl :-)
So instead of one ioctl w/ async flag, you want one sync ioctl and one async ioctl. Sure, why not. Both would en up doing much of the same things when collecting and verifying the state, but sharing the code is easy anyway.
But I'm actually not sure what everyone wants from the sync ioctl, especially when you use it for somthing that doesn't involve changing the timings. Should it behave like the current setcrtc, setplane etc. where the ioctl is free to execute asynchronously, but without any way to get a completion event? Or should it always block until the operation is truly complete?
On Sat, Sep 15, 2012 at 2:08 PM, Ville Syrjälä syrjala@sci.fi wrote:
Being able to query default values is probably useful. But we might want the flag anyways to make life easier on userspace. But I guess these can be added over time if needed, they shouldn't block getting the basic multi-plane page-flip in place.
yup, I see both using a lot of the same code.. all the stuff about splitting out the object state is applicable to both. The property enhancements to support object properties, signed properties, etc, this all applies for both.
I'm thinking of the sync ioctl in particular for things that change timings or light up a new display. Stuff like adding a new plane, flipping/moving an existing plane or crtc, etc.. these can all be done via the async ioctl. The crtc property on a plane, for example, would probably not have the 'dynamic' flag set[*], so that would require a test step.
[*] in my current patchset, the dynamic flags are set in core, but this really isn't the way it should be, the driver should be in control of which properties have the 'dynamic' flag set, but I needed something temporary to get things up and running.
BR, -R
On Wed, 12 Sep 2012 21:58:31 +0300 Ville Syrjälä ville.syrjala@linux.intel.com wrote:
I don't think there's any harm in having multiple ioctls for different things.
I was initially hoping the nuclear page flip would be very simple. Intended for simply updating buffers of several planes associated with a single display. That makes the inner loop of something like Wayland or SF simple, but obviously doesn't cover every case (in fact I had avoided dealing with moving planes initially).
Rob's patchset goes further than that, but obviously not as far as you propose.
OTOH, keeping things really simple and not very featureful means there are fewer points of failure, which is what I think callers would expect from a flip API...
So where does that leave us? I'd propose we have a very simple, stripped down, single crtc flip ioctl, along with a big atomic mode set ioctl, and then perhaps a fancier multi-crtc flip ioctl.
On Fri, Sep 14, 2012 at 4:14 PM, Jesse Barnes jbarnes@virtuousgeek.org wrote:
well, I do think it is quite useful to be able to enable/disable planes as part as part of the flip.. you really don't want to have to block the compositor for a synchronous operation to enable/disable a plane.. even if you have to return -EBUSY for a transition (ie. if a plane is still pending vblank on other crtc to switch over, etc)
I am on the fence about multi-crtc flip. Although the everything-is-a-property approach does, I suppose, means we could support it with one ioctl. Maybe we should start without. If later we want to support multi-crtc flip, we can add a driver cap to give userspace an idea about what it could expect to work.
What I am leaning towards now is an ioctl a bit more like Ville's atomic-modeset ioctl, but with a single user_data, and userspace gets a single event back (if requested). If userspace wants an event per CRTC it should do multiple ioctl calls, one per CRTC.
BR, -R
On Fri, Sep 14, 2012 at 5:14 PM, Jesse Barnes jbarnes@virtuousgeek.org wrote:
I think (hope) the consensus coming out of this thread is something along these lines:
- We use properties for specifying what to change to be future compatible with new crtc features, but also to allow exposing hw-specific properties and tie them into the atomicity of the pageflip. The KMS overlays are a lowest-common denominator for all the various overlay types out there and it should be possible to write a piece of chipset specific compositor code to use features that can't be expressed through KMS overlays.
- We have two types of properties: dynamic and non-dynamic ones. Dynamic properties can always be changed in the next frame (fb bos, hw cursor position, overlay position, for example), non-dynamic properties typically involve changing the way bandwidth are allocated and changing them may fail.
- We need a test ioctl that can verify whether changing non-dynamic properties will work. Using the atomic modeset for that with a test-only flag seems like a good option since that already has the logic to analyze bandwidth allocation across all crtcs. On the other hand, it may make more sense to use the multiflip ioctl as well here. What we need to check is whether the change made by a multifflip is possible, so it seems natural to communicate that change to the kernel using the same ioctl and data structs as the multiflip itself. The bandwidth calculation is a global decision and involves all crtcs and the current state, so the kernel can decide just fine if a multiflip is possible or not, based on the current state and the requested multiflip.
- Atomic multiflip for one crtc is essential for avoiding flicker and artifacts, but ill-defined for multiple crtcs simultaneously and even in the genlock case, the failure mode is hardly noticable (one crtc may drop a frame in case the compositor is racing with vsync, in which case multiflip just means both crtcs drop a frame). For flipping multiple fbs and planes, on one crtc, however, atomicity means that we can combine gpu rendering and overlays in a reliable way, without having to worry about flicker when sprites turn on a frame later after we've already erased the surface contents from the main fb. We need to be able to render the scene graph split across various planes at certain positions and know for certain that when we flip, that's the configuration that ends up on the output.
- Pageflip events can be controlled by a flag (as for the current pageflip ioctl) or perhaps disabled by setting user_data to 0, but the user data is passed in with each nuclear pageflip ioctl and each ioctl generates one event (if requested) which returns the user data that was passed in at ioctl time. This is how it currently works, the event mechanism is already in place, I see no reason to change this behaviour. Surely, we're not concerned about 8 extra bytes in the ioctl struct? The atomic modeset event (in test mode or not) never generates an event, so there's no need for user data there.
- Pageflip for multiple crtc may be useful in case of gen-locked crtc, but it is a corner case and not likely to be present or relevant in mainstream hw. With the properties being an extensible mechanism, we could probably expose gen-locked crtcs through the properties or such and in worst case make a new ioctl as Jesses suggests.
Kristian
On Fri, Sep 14, 2012 at 05:46:35PM -0400, Kristian Høgsberg wrote:
Properties are good. Check.
There's just no way to make such a general split. The simple fact is that even moving an overlay can fail due to timing/bandwith related constraints.
Ie. multiflip and atomic modeset need exactly the same thing here.
Sorry I don't follow. With two ioctls in the genlocked case, one crtc could drop, and the other might not. That is going to be a problem if both crtcs handle parts of the same physical display. Apart from the possible IVI and phone/tablet/gadget uses, I can imagine this being useful for large advertisement/presentation/simulation displays too.
Also allowing multi crtc flips in the non-genlocked case makes cloned displays trivial to implement. This is especially useful if the system is push based like surfaceflinger.
Sure, that's the main goal of this work.
There's no reason why you couldn't send the event in the blocking modeset case too. Also it would open the door for asynchronous modeset, if someone has the cojones to implement it.
I've already provided many ideas where it could be used, and I don't even consider myself a very imaginative person.
I don't see the point of forcing everyone with such a setup to add hacks in order to work around artificial restrictions imposed by the API. Do we want to make a system that people *want* to use, or one they *have* to use.
Well, I just don't see the point of going about it in such a roundabout way.
My current prototype code basically handles this case already, except that I added an artifical restriction to avoid the async apply code path in the multi-crtc case since some people were suggesting that. With just a few lines of code change I will lift that restriction and it'll work just fine.
I honestly do not see why some people want this restriction. From where I'm standing it doesn't make the code any less complex. What is the benefit you're trying to extract from the restriction?
And even if someone thinks that they can't implement the multi crtc case, then they're free to return an error from the check ioctl. So there's no harm in allowing it.
So I propose that we have: - One ioctl that takes an arbitrary number of obj/prop/value tuples - A flag to specify "test only" mode - A flag to demand asynchronous operation - Flags to request completion events for each crtc. A bitmask of crtc index will do. Each event will contain the relevant crtc ID. - user_data is passed to the ioctl, and included in the events originating from that operation.
From my POV the only significant API issues left are:
- Truly useful error reporting. Perhaps there is no nice way to do it. - Returning a list of retired FBs in the events
On Sat, Sep 15, 2012 at 9:53 AM, Ville Syrjälä syrjala@sci.fi wrote:
fwiw, the driver should indicate this by setting a flag on the property, this way userspace knows what can be changed dynamically and what can not.
well, to be fair, if we convert everything to properties, maybe drm/kms only needs one ioctl for everything :-P
But different ioctls are cheap.. I don't think it hurts to have two instead of one. I really don't see modeset and pageflip as the same thing. Maybe from the front-end of the video pipe, they are. But from encoder and back they are different. Modeset can take many vblank cycles to complete. And is infrequent. Introducing a state-machine to try to make this asynchronous is just adding a lot of complexity and potential fail for not really much gain.
Even flip on multiple crtcs introduces some new edge cases, like moving a plane from one crtc to another. If this is split into two ioctl calls, then the test on the 2nd crtc can -EBUSY because the plane is still pending disconnect from the first. But the test on the 1st crtc can succeed. I can see the usefulness of flip on multi-crtc... but since it isn't nearly as useful/important as flipping multiple planes on a single crtc, I don't see the harm in starting simple and adding this later.
BR, -R
On Sat, Sep 15, 2012 at 11:05:29AM -0500, Rob Clark wrote:
OK maybe user space could notice that all of the properties it's going to manipulate have that flag in the correct position. User space could then skip the check ioctl, and proceed straight to the commit phase with the nice feeling that it should not fail. But that's just an optimization.
Or are you actually suggesting that changing any property with the flag in the wrong position would require a full modeset, ie. force you to take the blocking code path? That just won't fly.
Sure, why not ;) Well, we still have all the enumerations stuff and whatnot. I see no point in changing those when they work adequately. But actually setting the state of the hardware can be handled through a single ioctl.
I don't entirely agree with the infrequent part. Fullscreen video on external displays is one case where you really may want to change the mode quite often. Or you may not want to change the actual timings on the display, but you still want to change the resolution of the CRTC, and let a panel fitter handle the difference in input and output resolutions. But thanks to the way kms is designed those two things are both linked to the display mode of the CRTC, so you still need a modeset to handle it.
Forcing you to rewrite user space multiple times. And keeping all the old codepaths around in both kernel and user space side to maintain ABI compatibility both ways. Also the speed at which these things trickle through to the actual users is very slow, so adding new ioctls every six months to handle overlapping tasks is not sensible IMHO.
My _point_ is that there is no need to hardcode these restrictions into the API. We already know what kind of API will handle all these cases, so why can't we just go with that API from the very beginning?
On Sat, Sep 15, 2012 at 1:00 PM, Ville Syrjälä syrjala@sci.fi wrote:
no, no.. the flag would just be a hint to userspace that it was a property that could be safely changed without a test step. Otherwise, userspace should do the test call first to confirm that the hw could handle the new property values.
well, possibly some properties could be added to circumvent that.. I was more thinking of actual timing changes
well, I'm not quite advocating a new ioctl every 6 months.. but what I meant is that the cost isn't high to have two ioctls, one for async pageflip, and one for modeset.. and the sync modeset ioctl is required if you are changing timings or lighting up a new display.
I think for pageflip, we could have just one ioctl, with a single user-data. And later add flags to indicate when the event should be sent back (once, once per crtc, etc). Sorting out exactly what we want for multi-crtc flips shouldn't block getting the basics in place for single crtc flip.
well, I don't think we necessarily need to change the ioctl to support flip on multiple crtc's.. it should be more a matter of adding a cap to indicate to userspace that you can change multiple crtc's on a single pageflip, and maybe defining some new flags.
BR, -R
dri-devel@lists.freedesktop.org
-
Ben Widawsky -
Clark, Rob -
Jesse Barnes -
Kristian Høgsberg -
Rob Clark -
Rob Clark
-
Ville Syrjälä -
Ville Syrjälä