From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [RFCv1 11/12] drm: Atomic modeset ioctl Date: Mon, 7 Oct 2013 17:18:02 +0300 Message-ID: <20131007141802.GL9395@intel.com> References: <1381020350-1125-1-git-send-email-robdclark@gmail.com> <1381020350-1125-12-git-send-email-robdclark@gmail.com> <20131007132812.GG9395@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 48EC4E627D for ; Mon, 7 Oct 2013 07:18:06 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Rob Clark Cc: "dri-devel@lists.freedesktop.org" List-Id: dri-devel@lists.freedesktop.org On Mon, Oct 07, 2013 at 09:55:47AM -0400, Rob Clark wrote: > On Mon, Oct 7, 2013 at 9:28 AM, Ville Syrj=E4l=E4 > wrote: > > On Sat, Oct 05, 2013 at 08:45:49PM -0400, Rob Clark wrote: > >> From: Ville Syrj=E4l=E4 > >> > >> The atomic modeset ioctl cna be used to push any number of new values > >> for object properties. The driver can then check the full device > >> configuration as single unit, and try to apply the changes atomically. > >> > >> The ioctl simply takes a list of object IDs and property IDs and their > >> values. For setting values to blob properties, the property value > >> indicates the length of the data, and the actual data is passed via > >> another blob pointer. > >> > >> The caller can demand non-blocking operation from the ioctl, and if the > >> driver can't satisfy that requirement an error will be returned. > >> > >> The caller can also request to receive asynchronous completion events > >> after the operation has reached the hardware. An event is sent for each > >> object specified by the caller, whether or not the actual state of > >> that object changed. Each event also carries a framebuffer ID, which > >> indicates to user space that the specified object is no longer > >> accessing that framebuffer. > >> > >> TODO: detailed error reporting? > >> > >> v1: original > >> v2: rebase on uapi changes, and drm state structs.. -- Rob Clark > >> v3: rebase, missing padding in drm_event_atomic.. -- Rob Clark > >> > >> Signed-off-by: Ville Syrj=E4l=E4 > >> --- > >> drivers/gpu/drm/drm_crtc.c | 159 +++++++++++++++++++++++++++++++++++= +++++++++ > >> drivers/gpu/drm/drm_drv.c | 1 + > >> include/drm/drmP.h | 6 ++ > >> include/drm/drm_crtc.h | 2 + > >> include/uapi/drm/drm.h | 12 ++++ > >> include/uapi/drm/drm_mode.h | 16 +++++ > >> 6 files changed, 196 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > >> index 09396a9..910e5c6 100644 > >> --- a/drivers/gpu/drm/drm_crtc.c > >> +++ b/drivers/gpu/drm/drm_crtc.c > >> @@ -4338,3 +4338,162 @@ void drm_mode_config_cleanup(struct drm_device= *dev) > >> idr_destroy(&dev->mode_config.crtc_idr); > >> } > >> EXPORT_SYMBOL(drm_mode_config_cleanup); > >> + > >> +int drm_mode_atomic_ioctl(struct drm_device *dev, > >> + void *data, struct drm_file *file_priv) > >> +{ > >> + struct drm_mode_atomic *arg =3D data; > >> + uint32_t __user *objs_ptr =3D (uint32_t __user *)(unsigned long)= (arg->objs_ptr); > >> + uint32_t __user *count_props_ptr =3D (uint32_t __user *)(unsigne= d long)(arg->count_props_ptr); > >> + uint32_t __user *props_ptr =3D (uint32_t __user *)(unsigned long= )(arg->props_ptr); > >> + uint64_t __user *prop_values_ptr =3D (uint64_t __user *)(unsigne= d long)(arg->prop_values_ptr); > >> + uint64_t __user *blob_values_ptr =3D (uint64_t __user *)(unsigne= d long)(arg->blob_values_ptr); > >> + unsigned int copied_objs =3D 0; > >> + unsigned int copied_props =3D 0; > >> + unsigned int copied_blobs =3D 0; > >> + void *state; > >> + int ret =3D 0; > >> + unsigned int i, j; > >> + > >> + if (arg->flags & ~(DRM_MODE_ATOMIC_TEST_ONLY | > >> + DRM_MODE_ATOMIC_EVENT | DRM_MODE_ATOMIC_NONBLOCK= )) > >> + return -EINVAL; > >> + > >> + /* can't test and expect an event at the same time. */ > >> + if ((arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) && > >> + (arg->flags & DRM_MODE_ATOMIC_EVENT)) > >> + return -EINVAL; > >> + > >> + mutex_lock(&dev->mode_config.mutex); > > > > I'm pretty sure I had a version w/ fixed locking... > > > > git://gitorious.org/vsyrjala/linux.git rebased_drm_atomic_24 > = > oh, hmm.. actually the locking should no longer be in the ioctl fxn, > but should be pushed down to the commit. looks like I missed this. I > need to dig up some actual test code for atomic ioctl ;-) It can't be in commit. At the very least it has to be around check+commit, and also you need to hold some locks when you're copying the current config over to your temp storage. That's assuming you store _everything_ in the temp storage and so it doesn't matter if the current state can change between the copy and check+commit. > = > >> + > >> + state =3D dev->driver->atomic_begin(dev, arg->flags); > >> + if (IS_ERR(state)) { > >> + ret =3D PTR_ERR(state); > >> + goto unlock; > >> + } > >> + > >> + for (i =3D 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 =3D -EFAULT; > >> + goto out; > >> + } > >> + > >> + obj =3D drm_mode_object_find(dev, obj_id, DRM_MODE_OBJEC= T_ANY); > >> + if (!obj || !obj->properties) { > >> + ret =3D -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. > = > BR, > -R > = >> + struct drm_pending_vblank_event *e =3D > >> + create_vblank_event(dev, file_priv, arg-= >user_data); > >> + if (!e) { > >> + ret =3D -ENOMEM; > >> + goto out; > >> + } > >> + ret =3D dev->driver->atomic_set_event(dev, state= , obj, e); > >> + if (ret) { > >> + destroy_vblank_event(dev, file_priv, e); > >> + goto out; > >> + } > >> + } > >> + > >> + if (get_user(count_props, count_props_ptr + copied_objs)= ) { > >> + ret =3D -EFAULT; > >> + goto out; > >> + } > >> + > >> + copied_objs++; > >> + > >> + for (j =3D 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 =3D NULL; > >> + > >> + if (get_user(prop_id, props_ptr + copied_props))= { > >> + ret =3D -EFAULT; > >> + goto out; > >> + } > >> + > >> + if (!object_has_prop(obj, prop_id)) { > >> + ret =3D -EINVAL; > >> + goto out; > >> + } > >> + > >> + prop_obj =3D drm_mode_object_find(dev, prop_id, > >> + DRM_MODE_OBJECT_PROPERTY); > >> + if (!prop_obj) { > >> + ret =3D -ENOENT; > >> + goto out; > >> + } > >> + prop =3D obj_to_property(prop_obj); > >> + > >> + if (get_user(prop_value, prop_values_ptr + copie= d_props)) { > >> + ret =3D -EFAULT; > >> + goto out; > >> + } > >> + > >> + if (!drm_property_change_is_valid(prop, prop_val= ue)) { > >> + ret =3D -EINVAL; > >> + goto out; > >> + } > >> + > >> + if ((prop->flags & DRM_MODE_PROP_BLOB) && prop_v= alue) { > >> + uint64_t blob_ptr; > >> + > >> + if (get_user(blob_ptr, blob_values_ptr += copied_blobs)) { > >> + ret =3D -EFAULT; > >> + goto out; > >> + } > >> + > >> + blob_data =3D kmalloc(prop_value, GFP_KE= RNEL); > >> + if (!blob_data) { > >> + ret =3D -ENOMEM; > >> + goto out; > >> + } > >> + > >> + if (copy_from_user(blob_data, (void __us= er *)(unsigned long)blob_ptr, prop_value)) { > >> + kfree(blob_data); > >> + ret =3D -EFAULT; > >> + goto out; > >> + } > >> + } > >> + > >> + /* User space sends the blob pointer even if we > >> + * don't use it (length=3D=3D0). > >> + */ > >> + if (prop->flags & DRM_MODE_PROP_BLOB) > >> + copied_blobs++; > >> + > >> + /* The driver will be in charge of blob_data fro= m now on. */ > >> + ret =3D drm_mode_set_obj_prop(obj, state, prop, > >> + prop_value, blob_data); > >> + if (ret) > >> + goto out; > >> + > >> + copied_props++; > >> + } > >> + } > >> + > >> + ret =3D dev->driver->atomic_check(dev, state); > >> + if (ret) > >> + goto out; > >> + > >> + if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) > >> + goto out; > >> + > >> + ret =3D 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[] = =3D { > >> 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_p= roperty_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), > >> DRM_IOCTL_DEF(DRM_IOCTL_MODE_CURSOR2, drm_mode_cursor2_ioctl, DR= M_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(str= uct drm_device *dev, void *data, > >> struct drm_file *file_priv); > >> extern int drm_mode_obj_set_property_ioctl(struct drm_device *dev, vo= id *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 dr= m_mode_obj_get_properties) > >> #define DRM_IOCTL_MODE_OBJ_SETPROPERTY DRM_IOWR(0xBA, struct dr= m_mode_obj_set_property) > >> #define DRM_IOCTL_MODE_CURSOR2 DRM_IOWR(0xBB, struct dr= m_mode_cursor2) > >> +#define DRM_IOCTL_MODE_ATOMIC DRM_IOWR(0xBC, struct dr= m_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=E4l=E4 > > Intel OTC -- = Ville Syrj=E4l=E4 Intel OTC