[RFCv1 11/12] drm: Atomic modeset ioctl
Matt Roper
matthew.d.roper at intel.com
Tue Oct 8 20:35:16 CEST 2013
On Mon, Oct 07, 2013 at 05:18:02PM +0300, Ville Syrjälä wrote:
> On Mon, Oct 07, 2013 at 09:55:47AM -0400, Rob Clark wrote:
> > On Mon, Oct 7, 2013 at 9:28 AM, Ville Syrjälä
> > <ville.syrjala at linux.intel.com> wrote:
> > > On Sat, Oct 05, 2013 at 08:45:49PM -0400, Rob Clark wrote:
...
> > >> +
> > >> + state = dev->driver->atomic_begin(dev, arg->flags);
> > >> + if (IS_ERR(state)) {
> > >> + ret = PTR_ERR(state);
> > >> + goto unlock;
> > >> + }
> > >> +
> > >> + for (i = 0; i < arg->count_objs; i++) {
> > >> + uint32_t obj_id, count_props;
> > >> + struct drm_mode_object *obj;
> > >> +
> > >> + if (get_user(obj_id, objs_ptr + copied_objs)) {
> > >> + ret = -EFAULT;
> > >> + goto out;
> > >> + }
> > >> +
> > >> + obj = drm_mode_object_find(dev, obj_id, DRM_MODE_OBJECT_ANY);
> > >> + if (!obj || !obj->properties) {
> > >> + ret = -ENOENT;
> > >> + goto out;
> > >> + }
> > >> +
> > >> + if (arg->flags & DRM_MODE_ATOMIC_EVENT) {
> > >> + // XXX create atomic event instead..
> > >
> > > Some people are apparently more comfortable with a per-crtc event
> > > rather than the per-obj events I added. So I think we might want
> > > both in the end.
> >
> > yeah, sorting out events is a bit 'TODO' at the moment. I do kind of
> > like per-crtc event.. it seems easier to implement and I'm not really
> > sure what finer event granularity buys us.
>
> Mainly it gets you the old fb. If you limit youself to < vrefresh it's
> not a big deal, but going faster than that and you start to want that
> information.
Are there cases where userspace would really need this fb information
returned? I'm having trouble envisioning a case where userspace
wouldn't have its own bookkeeping to know which fb's are currently
displayed on each plane and which fb's are pending.
If you're running a compositor like Weston, the redraw of a display is
generally tied to completion events, so there's no real desire/potential
for > vrefresh submission in most cases. In this model, having a single
per-crtc "I'm done updating everything" event is easier to work with
than an event per object where you have to do some additional counting
to make sure you consume the right number of events before kicking off
the repaint.
>
> >
> > BR,
> > -R
> >
> >> + struct drm_pending_vblank_event *e =
> > >> + create_vblank_event(dev, file_priv, arg->user_data);
> > >> + if (!e) {
> > >> + ret = -ENOMEM;
> > >> + goto out;
> > >> + }
> > >> + ret = dev->driver->atomic_set_event(dev, state, obj, e);
> > >> + if (ret) {
> > >> + destroy_vblank_event(dev, file_priv, e);
> > >> + goto out;
> > >> + }
> > >> + }
Did you mean to use the drm_pending_atomic_event/drm_event_atomic you
declare farther down? It doesn't look like anything is actually using
them yet in this patch series.
> > >> +
> > >> + if (get_user(count_props, count_props_ptr + copied_objs)) {
> > >> + ret = -EFAULT;
> > >> + goto out;
> > >> + }
> > >> +
> > >> + copied_objs++;
> > >> +
> > >> + for (j = 0; j < count_props; j++) {
> > >> + uint32_t prop_id;
> > >> + uint64_t prop_value;
> > >> + struct drm_mode_object *prop_obj;
> > >> + struct drm_property *prop;
> > >> + void *blob_data = NULL;
> > >> +
> > >> + if (get_user(prop_id, props_ptr + copied_props)) {
> > >> + ret = -EFAULT;
> > >> + goto out;
> > >> + }
> > >> +
> > >> + if (!object_has_prop(obj, prop_id)) {
> > >> + ret = -EINVAL;
> > >> + goto out;
> > >> + }
> > >> +
> > >> + prop_obj = drm_mode_object_find(dev, prop_id,
> > >> + DRM_MODE_OBJECT_PROPERTY);
> > >> + if (!prop_obj) {
> > >> + ret = -ENOENT;
> > >> + goto out;
> > >> + }
> > >> + prop = obj_to_property(prop_obj);
> > >> +
> > >> + if (get_user(prop_value, prop_values_ptr + copied_props)) {
> > >> + ret = -EFAULT;
> > >> + goto out;
> > >> + }
> > >> +
> > >> + if (!drm_property_change_is_valid(prop, prop_value)) {
> > >> + ret = -EINVAL;
> > >> + goto out;
> > >> + }
> > >> +
> > >> + if ((prop->flags & DRM_MODE_PROP_BLOB) && prop_value) {
> > >> + uint64_t blob_ptr;
> > >> +
> > >> + if (get_user(blob_ptr, blob_values_ptr + copied_blobs)) {
> > >> + ret = -EFAULT;
> > >> + goto out;
> > >> + }
> > >> +
> > >> + blob_data = kmalloc(prop_value, GFP_KERNEL);
> > >> + if (!blob_data) {
> > >> + ret = -ENOMEM;
> > >> + goto out;
> > >> + }
> > >> +
> > >> + if (copy_from_user(blob_data, (void __user *)(unsigned long)blob_ptr, prop_value)) {
> > >> + kfree(blob_data);
> > >> + ret = -EFAULT;
> > >> + goto out;
> > >> + }
> > >> + }
> > >> +
> > >> + /* User space sends the blob pointer even if we
> > >> + * don't use it (length==0).
> > >> + */
> > >> + if (prop->flags & DRM_MODE_PROP_BLOB)
> > >> + copied_blobs++;
> > >> +
> > >> + /* The driver will be in charge of blob_data from now on. */
> > >> + ret = drm_mode_set_obj_prop(obj, state, prop,
> > >> + prop_value, blob_data);
> > >> + if (ret)
> > >> + goto out;
> > >> +
> > >> + copied_props++;
> > >> + }
> > >> + }
> > >> +
> > >> + ret = dev->driver->atomic_check(dev, state);
> > >> + if (ret)
> > >> + goto out;
> > >> +
> > >> + if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
> > >> + goto out;
> > >> +
> > >> + ret = dev->driver->atomic_commit(dev, state);
> > >> +
> > >> + out:
> > >> + dev->driver->atomic_end(dev, state);
> > >> + unlock:
> > >> + mutex_unlock(&dev->mode_config.mutex);
> > >> +
> > >> + return ret;
> > >> +}
> > >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > >> index e572dd2..43e04ae 100644
> > >> --- a/drivers/gpu/drm/drm_drv.c
> > >> +++ b/drivers/gpu/drm/drm_drv.c
> > >> @@ -166,6 +166,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = {
> > >> DRM_IOCTL_DEF(DRM_IOCTL_MODE_OBJ_GETPROPERTIES, drm_mode_obj_get_properties_ioctl, 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_CURSOR2, drm_mode_cursor2_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> > >> + DRM_IOCTL_DEF(DRM_IOCTL_MODE_ATOMIC, drm_mode_atomic_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
> > >> };
> > >>
> > >> #define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls )
> > >> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> > >> index 4c3de22..f282733 100644
> > >> --- a/include/drm/drmP.h
> > >> +++ b/include/drm/drmP.h
> > >> @@ -1158,6 +1158,12 @@ struct drm_pending_vblank_event {
> > >> struct drm_event_vblank event;
> > >> };
> > >>
> > >> +struct drm_pending_atomic_event {
> > >> + struct drm_pending_event base;
> > >> + int pipe;
> > >> + struct drm_event_atomic event;
> > >> +};
> > >> +
> > >> /**
> > >> * DRM device structure. This structure represent a complete card that
> > >> * may contain multiple heads.
> > >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > >> index 86a5a00..fa3956e 100644
> > >> --- a/include/drm/drm_crtc.h
> > >> +++ b/include/drm/drm_crtc.h
> > >> @@ -1313,6 +1313,8 @@ extern int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data,
> > >> struct drm_file *file_priv);
> > >> extern int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data,
> > >> struct drm_file *file_priv);
> > >> +extern int drm_mode_atomic_ioctl(struct drm_device *dev,
> > >> + void *data, struct drm_file *file_priv);
> > >>
> > >> extern void drm_fb_get_bpp_depth(uint32_t format, unsigned int *depth,
> > >> int *bpp);
> > >> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> > >> index ece8678..efb1461 100644
> > >> --- a/include/uapi/drm/drm.h
> > >> +++ b/include/uapi/drm/drm.h
> > >> @@ -733,6 +733,7 @@ struct drm_prime_handle {
> > >> #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)
> > >> #define DRM_IOCTL_MODE_CURSOR2 DRM_IOWR(0xBB, struct drm_mode_cursor2)
> > >> +#define DRM_IOCTL_MODE_ATOMIC DRM_IOWR(0xBC, struct drm_mode_atomic)
> > >>
> > >> /**
> > >> * Device specific ioctls should only be in their respective headers
> > >> @@ -774,6 +775,17 @@ struct drm_event_vblank {
> > >> __u32 reserved;
> > >> };
> > >>
> > >> +struct drm_event_atomic {
> > >> + struct drm_event base;
> > >> + __u64 user_data;
> > >> + __u32 tv_sec;
> > >> + __u32 tv_usec;
> > >> + __u32 sequence;
> > >> + __u32 obj_id;
> > >> + __u32 old_fb_id;
> > >> + __u32 reserved;
> > >> +};
> > >> +
> > >> #define DRM_CAP_DUMB_BUFFER 0x1
> > >> #define DRM_CAP_VBLANK_HIGH_CRTC 0x2
> > >> #define DRM_CAP_DUMB_PREFERRED_DEPTH 0x3
> > >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> > >> index 6d4f089..03a473c 100644
> > >> --- a/include/uapi/drm/drm_mode.h
> > >> +++ b/include/uapi/drm/drm_mode.h
> > >> @@ -489,4 +489,20 @@ struct drm_mode_destroy_dumb {
> > >> uint32_t handle;
> > >> };
> > >>
> > >> +#define DRM_MODE_ATOMIC_TEST_ONLY (1<<0)
> > >> +#define DRM_MODE_ATOMIC_EVENT (1<<1)
> > >> +#define DRM_MODE_ATOMIC_NONBLOCK (1<<2)
> > >> +
> > >> +/* FIXME come up with some sane error reporting mechanism? */
> > >> +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;
> > >> + __u64 user_data;
> > >> +};
> > >> +
> > >> #endif
> > >> --
> > >> 1.8.3.1
> > >
> > > --
> > > Ville Syrjälä
> > > Intel OTC
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Matt Roper
Intel Corporation
Embedded Media & Graphics Driver Group
(916) 356-2795
More information about the dri-devel
mailing list