Re: [RFCv1 11/12] drm: Atomic modeset ioctl

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Oct 7, 2013 at 11:05 AM, Ville Syrjälä
<ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> On Mon, Oct 07, 2013 at 10:39:01AM -0400, Rob Clark wrote:
>> On Mon, Oct 7, 2013 at 10:18 AM, Ville Syrjälä
>> <ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> wrote:
>> >> > On Sat, Oct 05, 2013 at 08:45:49PM -0400, Rob Clark wrote:
>> >> >> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>> >> >>
>> >> >> 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älä <ville.syrjala@xxxxxxxxxxxxxxx>
>> >> >> ---
>> >> >>  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 = data;
>> >> >> +     uint32_t __user *objs_ptr = (uint32_t __user *)(unsigned long)(arg->objs_ptr);
>> >> >> +     uint32_t __user *count_props_ptr = (uint32_t __user *)(unsigned long)(arg->count_props_ptr);
>> >> >> +     uint32_t __user *props_ptr = (uint32_t __user *)(unsigned long)(arg->props_ptr);
>> >> >> +     uint64_t __user *prop_values_ptr = (uint64_t __user *)(unsigned long)(arg->prop_values_ptr);
>> >> >> +     uint64_t __user *blob_values_ptr = (uint64_t __user *)(unsigned long)(arg->blob_values_ptr);
>> >> >> +     unsigned int copied_objs = 0;
>> >> >> +     unsigned int copied_props = 0;
>> >> >> +     unsigned int copied_blobs = 0;
>> >> >> +     void *state;
>> >> >> +     int ret = 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.
>> >
>>
>> hmm.. I start to wish we did atomic first, before the fine grained
>> locking re-work ;-)
>
> I don't see it mattering for this particular problem if we just grab
> the big lock anyway.
>
> But as I already stated, I would prefer to solve the plane locking
> before doing atomic. Whether we have per-plane locks or not can
> actually matter for this code, especially when dealing with planes
> that can move between crtcs.

right, and I think the only sane way to deal w/ per-plane locking is
ww_mutex..  but then at least for crtc move case we can just grab both
old and new crtc lock

> And we still have locking issues to solve in the .check+commit steps
> when we have to deal with shared resources across the entire device.
> We can't simply use some small local lock in the .check step since
> something relevant might have changed by the time we reach .commit.
> I guess we could re-check such things at the very start of commit,
> before we've clobbered any state, and then update the global state
> to match if everything looks good, before we again drop the local
> lock.

I suppose an easy thing would be per-dev atomic to generate sequence
#'s for state updates.. then it would be pretty easy to check if there
has been a state update since atomic_begin()..

>>
>> >>
>> >> >> +
>> >> >> +     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.
>> >
>>
>> oh, right.. well I guess it wouldn't be too hard to add the current
>> primary plane fb_id to the existing event, which might be sufficient.
>>
>> At any rate, I don't mind adding new event flags to atomic ioctl once
>> we get atomic in place.
>
> Yeah adding new events isn't that hard. We just need to make sure
> things make sense. The code you posted doesn't :P
>
> You add the event to every object, even though you only wanted one
> event per crtc. The same object could also be listed multiple times in
> the object list, so the code would even add multiple events for the
> same object.

yeah, current atomic patch is almost an afterthought in this series.
I still need to get planes working on msm and find some test code (I
guess you have some somewhere?)..  so other than just rebasing it
enough to compile, I didn't really spend any time on it yet.  Right
now I was more interested in getting folks to have a look at the
atomic funcs and helpers.

BR,
-R

>>
>> BR,
>> -R
>>
>> >>
>> >> 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;
>> >> >> +                     }
>> >> >> +             }
>> >> >> +
>> >> >> +             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
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux