Kristian Høgsberg wrote:
> This patch adds a vblank synced pageflip ioctl for to the modesetting
> family of ioctls. The ioctl takes a crtc and an fb and schedules a
> pageflip to the new fb at the next coming vertical blank event. This
> feature lets userspace implement tear-free updating of the screen contents
> with hw-guaranteed low latency page flipping.
>
> The ioctl is asynchronous in that it returns immediately and then later
> notifies the client by making an event available for reading on the drm fd.
> This lets applications add the drm fd to their main loop and handle other
> tasks while waiting for the flip to happen. The event includes the time
> of the flip, the frame counter and a 64 bit opaque token provided by
> user space in the ioctl.
>
> Based on work and suggestions from
> Jesse Barnes <jbar...@virtuousgeek.org>,
> Jakob Bornecrantz <wallbra...@gmail.com>,
> Chris Wilson <ch...@chris-wilson.co.uk>
>
> Signed-off-by: Kristian Høgsberg <k...@redhat.com>
> Signed-off-by: Jesse Barnes <jbar...@virtuousgeek.org>
> ---
>
> Ok, another version of this patch. This one has fixes to work with radeon
> kms plus a missing list head init that would cause an oops in the case
> where we schedule a flip before the previous one has been queued.
>
> I'm now ready to propose this patch for the 2.6.32 merge window.
>
>
Hi!
First a general question: There is some hardware (for example Unichromes
and Chrome9) that can schedule
page-flips in the command stream after optional vblank barriers. For
this kind of hardware the pageflip would be a matter of scheduling the
flip and fence the old and new buffers. No need for delayed unpins and
explicit user-space notifications, although the workqueue could be handy
to avoid command processing stalls in the vblank barriers. How would
such a scheme fit into the framework below?
> Kristian
>
> drivers/gpu/drm/drm_crtc.c | 170
> ++++++++++++++++++++++++++++++-
> drivers/gpu/drm/drm_crtc_helper.c | 12 ++
> drivers/gpu/drm/drm_drv.c | 1 +
> drivers/gpu/drm/drm_fops.c | 68 ++++++++++++-
> drivers/gpu/drm/drm_irq.c | 43 ++++++++
> drivers/gpu/drm/i915/i915_drv.c | 1 +
> drivers/gpu/drm/i915/intel_display.c | 24 +++--
> drivers/gpu/drm/radeon/radeon_display.c | 3 +-
> include/drm/drm.h | 25 +++++
> include/drm/drmP.h | 32 ++++++
> include/drm/drm_crtc.h | 27 +++++
> include/drm/drm_crtc_helper.h | 4 +
> include/drm/drm_mode.h | 16 +++
> 13 files changed, 414 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 8fab789..0906cb3 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -34,6 +34,8 @@
> #include "drmP.h"
> #include "drm_crtc.h"
>
> +#undef set_base
> +
> struct drm_prop_enum_list {
> int type;
> char *name;
> @@ -342,6 +344,34 @@ void drm_framebuffer_cleanup(struct drm_framebuffer *fb)
> EXPORT_SYMBOL(drm_framebuffer_cleanup);
>
> /**
> + * drm_crtc_async_flip - do a set_base call from a work queue
> + * @work: work struct
> + *
> + * Called when a set_base call is queued by the page flip code. This
> + * allows the flip ioctl itself to return immediately and allow userspace
> + * to continue working.
> + */
> +static void drm_crtc_async_flip(struct work_struct *work)
> +{
> + struct drm_crtc *crtc = container_of(work, struct drm_crtc, async_flip);
> + struct drm_device *dev = crtc->dev;
> + struct drm_pending_flip *pending;
> +
> + BUG_ON(crtc->pending_flip == NULL);
> +
> + mutex_lock(&dev->struct_mutex);
>
Here the struct mutex is taken before a call to a function that doesn't
even have dev as an argument.
Exactly what in the generic modesetting code is protected by the struct
mutex here?
If the driver needs the struct mutex to protect internal data
structures, let the driver take care of the locking. If the struct mutex
protects the dev->flip_list, then we should take it around the
manipulation of that list only. I'd hate to see the not-too-careful use
of struct_mutex in some drivers leak out to generic code.
> + crtc->funcs->set_base(crtc, crtc->x, crtc->y, NULL);
> +
> + pending = crtc->pending_flip;
> + crtc->pending_flip = NULL;
> +
> + pending->frame = drm_vblank_count(dev, crtc->pipe);
> + list_add_tail(&pending->link, &dev->flip_list);
> +
> + mutex_unlock(&dev->struct_mutex);
> +}
> +
> +/**
> * drm_crtc_init - Initialise a new CRTC object
> * @dev: DRM device
> * @crtc: CRTC object to init
> @@ -352,17 +382,19 @@ EXPORT_SYMBOL(drm_framebuffer_cleanup);
> *
> * Inits a new object created as base part of an driver crtc object.
> */
> -void drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
> +void drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, int pipe,
> const struct drm_crtc_funcs *funcs)
> {
> crtc->dev = dev;
> crtc->funcs = funcs;
> + crtc->pipe = pipe;
>
> mutex_lock(&dev->mode_config.mutex);
> drm_mode_object_get(dev, &crtc->base, DRM_MODE_OBJECT_CRTC);
>
> list_add_tail(&crtc->head, &dev->mode_config.crtc_list);
> dev->mode_config.num_crtc++;
> + INIT_WORK(&crtc->async_flip, drm_crtc_async_flip);
> mutex_unlock(&dev->mode_config.mutex);
> }
> EXPORT_SYMBOL(drm_crtc_init);
> @@ -381,6 +413,9 @@ void drm_crtc_cleanup(struct drm_crtc *crtc)
> {
> struct drm_device *dev = crtc->dev;
>
> + mutex_lock(&dev->mode_config.mutex);
> + flush_work(&crtc->async_flip);
> +
> if (crtc->gamma_store) {
> kfree(crtc->gamma_store);
> crtc->gamma_store = NULL;
> @@ -388,6 +423,7 @@ void drm_crtc_cleanup(struct drm_crtc *crtc)
>
> drm_mode_object_put(dev, &crtc->base);
> list_del(&crtc->head);
> + mutex_unlock(&dev->mode_config.mutex);
> dev->mode_config.num_crtc--;
> }
> EXPORT_SYMBOL(drm_crtc_cleanup);
> @@ -2452,3 +2488,135 @@ out:
> mutex_unlock(&dev->mode_config.mutex);
> return ret;
> }
> +
> +/**
> + * drm_mode_page_flip_ioctl - page flip ioctl
> + * @dev: DRM device
> + * @data: ioctl args
> + * @file_priv: file private data
> + *
> + * The page flip ioctl replaces the current front buffer with a new
> + * one, using the CRTC's set_base function, which should just update
> + * the front buffer base pointer. It's up to set_base to make
> + * sure the update doesn't result in tearing (on some hardware the
> + * base register is double buffered, so this is easy).
> + *
> + * Note that this covers just the simple case of flipping the front
> + * buffer immediately. Interval handling and interlaced modes have to
> + * be handled by userspace, or with new ioctls.
> + */
> +int drm_mode_page_flip_ioctl(struct drm_device *dev, void *data,
> + struct drm_file *file_priv)
> +{
> + struct drm_pending_flip *pending;
> + struct drm_mode_page_flip *flip_data = data;
> + struct drm_mode_object *drm_obj, *fb_obj;
> + struct drm_crtc *crtc;
> + int ret = 0;
> +
> + if (!(drm_core_check_feature(dev, DRIVER_MODESET)))
> + return -ENODEV;
> +
> + /*
> + * Reject unknown flags so future userspace knows what we (don't)
> + * support
> + */
> + if (flip_data->flags & (~DRM_MODE_PAGE_FLIP_FLAGS_MASK)) {
> + DRM_DEBUG("bad page flip flags\n");
> + return -EINVAL;
> + }
> +
> + pending = kzalloc(sizeof *pending, GFP_KERNEL);
> + if (pending == NULL)
> + return -ENOMEM;
> +
> + mutex_lock(&dev->struct_mutex);
> +
> + fb_obj = drm_mode_object_find(dev, flip_data->fb_id,
> + DRM_MODE_OBJECT_FB);
> + if (!fb_obj) {
> + DRM_DEBUG("unknown fb %d\n", flip_data->fb_id);
> + ret = -ENOENT;
> + goto out_unlock;
> + }
> +
> + drm_obj = drm_mode_object_find(dev, flip_data->crtc_id,
> + DRM_MODE_OBJECT_CRTC);
> + if (!drm_obj) {
> + DRM_DEBUG("unknown crtc %d\n", flip_data->crtc_id);
> + ret = -ENOENT;
> + goto out_unlock;
> + }
> + crtc = obj_to_crtc(drm_obj);
> + if (!crtc->enabled) {
> + DRM_DEBUG("crtc %d not enabled\n", flip_data->crtc_id);
> + ret = -EINVAL;
> + goto out_unlock;
> + }
> +
> + if (crtc->fb->funcs->unpin == NULL) {
> + DRM_DEBUG("fb for crtc %d does not support delayed unpin\n",
> + flip_data->crtc_id);
> + ret = -ENODEV;
> + goto out_unlock;
> + }
> +
> + pending->crtc = crtc;
> + pending->old_fb = crtc->fb;
> + pending->pipe = crtc->pipe;
> + INIT_LIST_HEAD(&pending->link);
> + pending->event.base.type = DRM_EVENT_MODE_PAGE_FLIP;
> + pending->event.base.length = sizeof pending->event;
> + pending->event.user_data = flip_data->user_data;
> + pending->pending_event.event = &pending->event.base;
> + pending->pending_event.file_priv = file_priv;
> + pending->pending_event.destroy =
> + (void (*) (struct drm_pending_event *)) kfree;
> +
> + /* Get vblank ref for completion handling */
> + ret = drm_vblank_get(dev, crtc->pipe);
> + if (ret) {
> + DRM_DEBUG("failed to take vblank ref\n");
> + goto out_unlock;
> + }
> +
> + /*
> + * The set_base call will change the domain on the new fb,
> + * which will force the rendering to finish and block the
> + * ioctl. We need to do this last part from a work queue, to
> + * avoid blocking userspace here.
> + */
> + crtc->fb = obj_to_fb(fb_obj);
> +
> + if (crtc->pending_flip != NULL) {
> + struct drm_pending_flip *old_flip;
> +
> + /* We have an outstanding flip request for this crtc/pipe.
> + * In order to satisfy the user we can either queue the requests
> + * and apply them on sequential vblanks, or we can drop old
> + * requests.
> + *
> + * Here we choose to discard the previous request for
> + * simplicity. Note that since we have not yet applied the
> + * previous flip, we need to preserve the original (i.e. still
> + * current) fb.
> + */
> +
> + old_flip = crtc->pending_flip;
> + pending->old_fb = old_flip->old_fb;
> + old_flip->old_fb = NULL;
> + drm_finish_pending_flip (dev, old_flip, 0);
> + } else
> + schedule_work(&crtc->async_flip);
> + crtc->pending_flip = pending;
> +
> + mutex_unlock(&dev->struct_mutex);
> +
> + return 0;
> +
> +out_unlock:
> + mutex_unlock(&dev->struct_mutex);
> + kfree(pending);
> +
> + return ret;
> +}
> diff --git a/drivers/gpu/drm/drm_crtc_helper.c
> b/drivers/gpu/drm/drm_crtc_helper.c
> index 3da9cfa..5a26bab 100644
> --- a/drivers/gpu/drm/drm_crtc_helper.c
> +++ b/drivers/gpu/drm/drm_crtc_helper.c
> @@ -868,8 +868,10 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set)
> old_fb = set->crtc->fb;
> if (set->crtc->fb != set->fb)
> set->crtc->fb = set->fb;
> + mutex_lock(&dev->struct_mutex);
>
dev->struct_mutex again!
> ret = crtc_funcs->mode_set_base(set->crtc,
> set->x, set->y, old_fb);
> + mutex_unlock(&dev->struct_mutex);
> if (ret != 0)
> goto fail_set_mode;
> }
> @@ -1095,3 +1097,13 @@ int drm_helper_resume_force_mode(struct drm_device
> *dev)
> return 0;
> }
> EXPORT_SYMBOL(drm_helper_resume_force_mode);
> +
> +int
> +drm_crtc_helper_set_base(struct drm_crtc *crtc, int x, int y,
> + struct drm_framebuffer *old_fb)
> +{
> + struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
> +
> + return crtc_funcs->mode_set_base(crtc, x, y, old_fb);
> +}
> +EXPORT_SYMBOL(drm_crtc_helper_set_base);
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index b39d7bf..c66c993 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -145,6 +145,7 @@ static struct drm_ioctl_desc drm_ioctls[] = {
> DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETFB, drm_mode_getfb,
> DRM_MASTER|DRM_CONTROL_ALLOW),
> DRM_IOCTL_DEF(DRM_IOCTL_MODE_ADDFB, drm_mode_addfb,
> DRM_MASTER|DRM_CONTROL_ALLOW),
> DRM_IOCTL_DEF(DRM_IOCTL_MODE_RMFB, drm_mode_rmfb,
> DRM_MASTER|DRM_CONTROL_ALLOW),
> + DRM_IOCTL_DEF(DRM_IOCTL_MODE_PAGE_FLIP, drm_mode_page_flip_ioctl,
> DRM_MASTER|DRM_CONTROL_ALLOW),
> };
>
> #define DRM_CORE_IOCTL_COUNT ARRAY_SIZE( drm_ioctls )
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index 251bc0e..dcd9c66 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -257,6 +257,8 @@ static int drm_open_helper(struct inode *inode, struct
> file *filp,
>
> INIT_LIST_HEAD(&priv->lhead);
> INIT_LIST_HEAD(&priv->fbs);
> + INIT_LIST_HEAD(&priv->event_list);
> + init_waitqueue_head(&priv->event_wait);
>
> if (dev->driver->driver_features & DRIVER_GEM)
> drm_gem_open(dev, priv);
> @@ -429,6 +431,9 @@ int drm_release(struct inode *inode, struct file *filp)
> {
> struct drm_file *file_priv = filp->private_data;
> struct drm_device *dev = file_priv->minor->dev;
> + struct drm_pending_flip *f, *ft;
> + struct drm_pending_event *e, *et;
> +
> int retcode = 0;
>
> lock_kernel();
> @@ -451,6 +456,19 @@ int drm_release(struct inode *inode, struct file *filp)
> if (file_priv->minor->master)
> drm_master_release(dev, filp);
>
> + mutex_lock(&dev->struct_mutex);
> +
> + /* Remove pending flips */
> + list_for_each_entry_safe(f, ft, &dev->flip_list, link)
> + if (f->pending_event.file_priv == file_priv)
> + drm_finish_pending_flip(dev, f, 0);
> +
> + /* Remove unconsumed events */
> + list_for_each_entry_safe(e, et, &file_priv->event_list, link)
> + e->destroy(e);
> +
> + mutex_unlock(&dev->struct_mutex);
> +
> if (dev->driver->driver_features & DRIVER_GEM)
> drm_gem_release(dev, file_priv);
>
> @@ -544,9 +562,55 @@ int drm_release(struct inode *inode, struct file *filp)
> }
> EXPORT_SYMBOL(drm_release);
>
> -/** No-op. */
> +ssize_t drm_read(struct file *filp, char __user *buffer,
> + size_t count, loff_t *offset)
>
The usage of drm_read here will clash with TTM bo read / write, which
suggests perhaps moving TTM bo read / write to a character device of its
own. That would affect Radeon to some extent if there is a wish to use
consistently implemented read / write to Radeon buffer objects.
> +{
> + struct drm_file *file_priv = filp->private_data;
> + struct drm_device *dev = file_priv->minor->dev;
> + struct drm_pending_event *event;
> + ssize_t total, ret;
> +
> + ret = wait_event_interruptible(file_priv->event_wait,
> + !list_empty(&file_priv->event_list));
> + if (ret < 0)
> + return ret;
> +
> + total = 0;
> + while (!list_empty(&file_priv->event_list)) {
> + mutex_lock(&dev->struct_mutex);
> + event = list_first_entry(&file_priv->event_list,
> + struct drm_pending_event, link);
> + if (total + event->event->length > count) {
> + mutex_unlock(&dev->struct_mutex);
> + break;
> + }
> + list_del(&event->link);
> + mutex_unlock(&dev->struct_mutex);
> +
> + if (copy_to_user(buffer + total,
> + event->event, event->event->length)) {
> + total = -EFAULT;
> + break;
> + }
> +
> + total += event->event->length;
> + event->destroy(event);
> + }
> +
> + return total;
> +}
> +EXPORT_SYMBOL(drm_read);
> +
> unsigned int drm_poll(struct file *filp, struct poll_table_struct *wait)
> {
>
A couple of years ago, any attempt to return anything else than 0 from
drm poll resulted in an X server error.
http://freedesktop.org/bugzilla/show_bug.cgi?id=1505. The fix mentioned
in the bug was actually to return 0 from drm poll, and a comment about
this is still present in drm.git. The above breaks drm for old X servers
and all drivers, which I think is against drm policy?
/Thomas
------------------------------------------------------------------------------
Let Crystal Reports handle the reporting - Free Crystal Reports 2008 30-Day
trial. Simplify your report design, integration and deployment - and focus on
what you do best, core application coding. Discover what's new with
Crystal Reports now. http://p.sf.net/sfu/bobj-july
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
