Hello,
This patch series contains two completely unrelated features that just happen to have been developed one right after the other.
Patches 1/5 to 3/5 implement explicit fences support. The first patch fixes event handling in the omapdrm driver as required by the atomic commit helper (and the DRM atomic update API). The second patch replace the hand-rolled atomic commit handler with the DRM core atomic commit helper, bringing fences support as a bonus. The third patch then removes the omapdrm custom sync support that has no user in the mainline kernel.
Patches 4/5 and 5/5 clean up zpos handling by switching to the standard zpos property implemented in the DRM core. The omapdrm-specific zorder property is kept for backward compatibility as an alias and should be removed later after a long enough grace period for userspace to be updated.
Compared to v1, the patches have been rebased on top of Dave's drm-next branch which now contains all the dependencies.
On the testing side, I've ran kmstest --flip for 250 frames in a loop for half an hour, with and without CPU load (with 'stress -c 4'), on both PandaBoard and AM572x EVM, and couldn't reproduce any of the issues reported against v1.
As fence support is implemented completely inside the atomic commit helper I have only tested it lightly using the sw-sync driver. No issue has been noticed.
Laurent Pinchart (5): drm: omapdrm: Handle events when enabling/disabling CRTCs drm: omapdrm: Use DRM core's atomic commit helper drm: omapdrm: Remove legacy buffer synchronization support drm: omapdrm: Store the Z order in the plane state zpos field drm: omapdrm: Add zpos property
drivers/gpu/drm/omapdrm/omap_crtc.c | 30 +++-- drivers/gpu/drm/omapdrm/omap_drv.c | 162 ++++---------------------- drivers/gpu/drm/omapdrm/omap_drv.h | 14 +-- drivers/gpu/drm/omapdrm/omap_gem.c | 214 ----------------------------------- drivers/gpu/drm/omapdrm/omap_plane.c | 74 ++---------- include/uapi/drm/omap_drm.h | 4 +- 6 files changed, 55 insertions(+), 443 deletions(-)
The driver currently handles vblank events only when updating planes on an already enabled CRTC. The atomic update API however allows requesting an event when enabling or disabling a CRTC. This currently leads to event objects being leaked in the kernel and to events not being sent out. Fix it.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/omapdrm/omap_crtc.c | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c index dccd03726796..dd0ef40ca469 100644 --- a/drivers/gpu/drm/omapdrm/omap_crtc.c +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c @@ -343,6 +343,19 @@ static void omap_crtc_destroy(struct drm_crtc *crtc) kfree(omap_crtc); }
+static void omap_crtc_arm_event(struct drm_crtc *crtc) +{ + struct omap_crtc *omap_crtc = to_omap_crtc(crtc); + + WARN_ON(omap_crtc->pending); + omap_crtc->pending = true; + + if (crtc->state->event) { + omap_crtc->event = crtc->state->event; + crtc->state->event = NULL; + } +} + static void omap_crtc_enable(struct drm_crtc *crtc) { struct omap_crtc *omap_crtc = to_omap_crtc(crtc); @@ -355,8 +368,7 @@ static void omap_crtc_enable(struct drm_crtc *crtc) ret = drm_crtc_vblank_get(crtc); WARN_ON(ret != 0);
- WARN_ON(omap_crtc->pending); - omap_crtc->pending = true; + omap_crtc_arm_event(crtc); spin_unlock_irq(&crtc->dev->event_lock); }
@@ -366,6 +378,13 @@ static void omap_crtc_disable(struct drm_crtc *crtc)
DBG("%s", omap_crtc->name);
+ spin_lock_irq(&crtc->dev->event_lock); + if (crtc->state->event) { + drm_crtc_send_vblank_event(crtc, crtc->state->event); + crtc->state->event = NULL; + } + spin_unlock_irq(&crtc->dev->event_lock); + drm_crtc_vblank_off(crtc); }
@@ -473,12 +492,7 @@ static void omap_crtc_atomic_flush(struct drm_crtc *crtc,
spin_lock_irq(&crtc->dev->event_lock); priv->dispc_ops->mgr_go(omap_crtc->channel); - - WARN_ON(omap_crtc->pending); - omap_crtc->pending = true; - - if (crtc->state->event) - omap_crtc->event = crtc->state->event; + omap_crtc_arm_event(crtc); spin_unlock_irq(&crtc->dev->event_lock); }
On 15/04/17 12:16, Laurent Pinchart wrote:
The driver currently handles vblank events only when updating planes on an already enabled CRTC. The atomic update API however allows requesting an event when enabling or disabling a CRTC. This currently leads to event objects being leaked in the kernel and to events not being sent out. Fix it.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
drivers/gpu/drm/omapdrm/omap_crtc.c | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-)
Reviewed-by: Tomi Valkeinen tomi.valkeinen@ti.com
Tomi
The DRM core atomic helper now supports asynchronous commits natively. The custom omapdrm implementation isn't needed anymore, remove it.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/omapdrm/omap_drv.c | 112 +++++-------------------------------- drivers/gpu/drm/omapdrm/omap_drv.h | 9 +-- 2 files changed, 15 insertions(+), 106 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index e1f47f0b3ccf..66a541c28063 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -17,8 +17,6 @@ * this program. If not, see http://www.gnu.org/licenses/. */
-#include <linux/wait.h> - #include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_crtc_helper.h> @@ -54,13 +52,6 @@ static void omap_fb_output_poll_changed(struct drm_device *dev) drm_fb_helper_hotplug_event(priv->fbdev); }
-struct omap_atomic_state_commit { - struct work_struct work; - struct drm_device *dev; - struct drm_atomic_state *state; - u32 crtcs; -}; - static void omap_atomic_wait_for_completion(struct drm_device *dev, struct drm_atomic_state *old_state) { @@ -81,15 +72,14 @@ static void omap_atomic_wait_for_completion(struct drm_device *dev, } }
-static void omap_atomic_complete(struct omap_atomic_state_commit *commit) +static void omap_atomic_commit_tail(struct drm_atomic_state *old_state) { - struct drm_device *dev = commit->dev; + struct drm_device *dev = old_state->dev; struct omap_drm_private *priv = dev->dev_private; - struct drm_atomic_state *old_state = commit->state;
- /* Apply the atomic update. */ priv->dispc_ops->runtime_get();
+ /* Apply the atomic update. */ drm_atomic_helper_commit_modeset_disables(dev, old_state);
/* With the current dss dispc implementation we have to enable @@ -108,101 +98,28 @@ static void omap_atomic_complete(struct omap_atomic_state_commit *commit)
drm_atomic_helper_commit_planes(dev, old_state, 0);
+ drm_atomic_helper_commit_hw_done(old_state); + + /* + * Wait for completion of the page flips to ensure that old buffers + * can't be touched by the hardware anymore before cleaning up planes. + */ omap_atomic_wait_for_completion(dev, old_state);
drm_atomic_helper_cleanup_planes(dev, old_state);
priv->dispc_ops->runtime_put(); - - drm_atomic_state_put(old_state); - - /* Complete the commit, wake up any waiter. */ - spin_lock(&priv->commit.lock); - priv->commit.pending &= ~commit->crtcs; - spin_unlock(&priv->commit.lock); - - wake_up_all(&priv->commit.wait); - - kfree(commit); -} - -static void omap_atomic_work(struct work_struct *work) -{ - struct omap_atomic_state_commit *commit = - container_of(work, struct omap_atomic_state_commit, work); - - omap_atomic_complete(commit); }
-static bool omap_atomic_is_pending(struct omap_drm_private *priv, - struct omap_atomic_state_commit *commit) -{ - bool pending; - - spin_lock(&priv->commit.lock); - pending = priv->commit.pending & commit->crtcs; - spin_unlock(&priv->commit.lock); - - return pending; -} - -static int omap_atomic_commit(struct drm_device *dev, - struct drm_atomic_state *state, bool nonblock) -{ - struct omap_drm_private *priv = dev->dev_private; - struct omap_atomic_state_commit *commit; - struct drm_crtc *crtc; - struct drm_crtc_state *crtc_state; - int i, ret; - - ret = drm_atomic_helper_prepare_planes(dev, state); - if (ret) - return ret; - - /* Allocate the commit object. */ - commit = kzalloc(sizeof(*commit), GFP_KERNEL); - if (commit == NULL) { - ret = -ENOMEM; - goto error; - } - - INIT_WORK(&commit->work, omap_atomic_work); - commit->dev = dev; - commit->state = state; - - /* Wait until all affected CRTCs have completed previous commits and - * mark them as pending. - */ - for_each_crtc_in_state(state, crtc, crtc_state, i) - commit->crtcs |= drm_crtc_mask(crtc); - - wait_event(priv->commit.wait, !omap_atomic_is_pending(priv, commit)); - - spin_lock(&priv->commit.lock); - priv->commit.pending |= commit->crtcs; - spin_unlock(&priv->commit.lock); - - /* Swap the state, this is the point of no return. */ - drm_atomic_helper_swap_state(state, true); - - drm_atomic_state_get(state); - if (nonblock) - schedule_work(&commit->work); - else - omap_atomic_complete(commit); - - return 0; - -error: - drm_atomic_helper_cleanup_planes(dev, state); - return ret; -} +static const struct drm_mode_config_helper_funcs omap_mode_config_helper_funcs = { + .atomic_commit_tail = omap_atomic_commit_tail, +};
static const struct drm_mode_config_funcs omap_mode_config_funcs = { .fb_create = omap_framebuffer_create, .output_poll_changed = omap_fb_output_poll_changed, .atomic_check = drm_atomic_helper_check, - .atomic_commit = omap_atomic_commit, + .atomic_commit = drm_atomic_helper_commit, };
static int get_connector_type(struct omap_dss_device *dssdev) @@ -385,6 +302,7 @@ static int omap_modeset_init(struct drm_device *dev) dev->mode_config.max_height = 2048;
dev->mode_config.funcs = &omap_mode_config_funcs; + dev->mode_config.helper_private = &omap_mode_config_helper_funcs;
drm_mode_config_reset(dev);
@@ -674,8 +592,6 @@ static int pdev_probe(struct platform_device *pdev) priv->omaprev = pdata->omaprev; priv->wq = alloc_ordered_workqueue("omapdrm", 0);
- init_waitqueue_head(&priv->commit.wait); - spin_lock_init(&priv->commit.lock); spin_lock_init(&priv->list_lock); INIT_LIST_HEAD(&priv->obj_list);
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h index 7a4c57eb6536..54e6055ea1d3 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.h +++ b/drivers/gpu/drm/omapdrm/omap_drv.h @@ -23,7 +23,7 @@ #include <linux/module.h> #include <linux/platform_data/omap_drm.h> #include <linux/types.h> -#include <linux/wait.h> +#include <linux/workqueue.h>
#include <drm/drmP.h> #include <drm/drm_crtc_helper.h> @@ -93,13 +93,6 @@ struct omap_drm_private { spinlock_t wait_lock; /* protects the wait_list */ struct list_head wait_list; /* list of omap_irq_wait */ uint32_t irq_mask; /* enabled irqs in addition to wait_list */ - - /* atomic commit */ - struct { - wait_queue_head_t wait; - u32 pending; - spinlock_t lock; /* Protects commit.pending */ - } commit; };
On 15/04/17 12:16, Laurent Pinchart wrote:
The DRM core atomic helper now supports asynchronous commits natively. The custom omapdrm implementation isn't needed anymore, remove it.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
drivers/gpu/drm/omapdrm/omap_drv.c | 112 +++++-------------------------------- drivers/gpu/drm/omapdrm/omap_drv.h | 9 +-- 2 files changed, 15 insertions(+), 106 deletions(-)
Reviewed-by: Tomi Valkeinen tomi.valkeinen@ti.com
Tomi
The omapdrm driver uses a custom API to synchronize with the SGX GPU. This is unusable as such in the mainline kernel as the API is only partially implemented and requires additional out-of-tree patches. Furthermore, as no SGX driver is available in the mainline kernel, the API can't be considered as a stable mainline API.
Now that the driver supports synchronization through fences, remove legacy buffer synchronization support. The two userspace ioctls are turned into no-ops to avoid breaking userspace and will be removed in the future.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/omapdrm/omap_drv.c | 50 +-------- drivers/gpu/drm/omapdrm/omap_drv.h | 5 - drivers/gpu/drm/omapdrm/omap_gem.c | 214 ------------------------------------- include/uapi/drm/omap_drm.h | 4 +- 4 files changed, 7 insertions(+), 266 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 66a541c28063..f5e71fe5f770 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -365,51 +365,11 @@ static int ioctl_gem_new(struct drm_device *dev, void *data, &args->handle); }
-static int ioctl_gem_cpu_prep(struct drm_device *dev, void *data, +static int ioctl_gem_cpu_prep_fini(struct drm_device *dev, void *data, struct drm_file *file_priv) { - struct drm_omap_gem_cpu_prep *args = data; - struct drm_gem_object *obj; - int ret; - - VERB("%p:%p: handle=%d, op=%x", dev, file_priv, args->handle, args->op); - - obj = drm_gem_object_lookup(file_priv, args->handle); - if (!obj) - return -ENOENT; - - ret = omap_gem_op_sync(obj, args->op); - - if (!ret) - ret = omap_gem_op_start(obj, args->op); - - drm_gem_object_unreference_unlocked(obj); - - return ret; -} - -static int ioctl_gem_cpu_fini(struct drm_device *dev, void *data, - struct drm_file *file_priv) -{ - struct drm_omap_gem_cpu_fini *args = data; - struct drm_gem_object *obj; - int ret; - - VERB("%p:%p: handle=%d", dev, file_priv, args->handle); - - obj = drm_gem_object_lookup(file_priv, args->handle); - if (!obj) - return -ENOENT; - - /* XXX flushy, flushy */ - ret = 0; - - if (!ret) - ret = omap_gem_op_finish(obj, args->op); - - drm_gem_object_unreference_unlocked(obj); - - return ret; + /* Deprecated, to be removed. */ + return 0; }
static int ioctl_gem_info(struct drm_device *dev, void *data, @@ -440,9 +400,9 @@ static const struct drm_ioctl_desc ioctls[DRM_COMMAND_END - DRM_COMMAND_BASE] = DRM_AUTH | DRM_MASTER | DRM_ROOT_ONLY), DRM_IOCTL_DEF_DRV(OMAP_GEM_NEW, ioctl_gem_new, DRM_AUTH | DRM_RENDER_ALLOW), - DRM_IOCTL_DEF_DRV(OMAP_GEM_CPU_PREP, ioctl_gem_cpu_prep, + DRM_IOCTL_DEF_DRV(OMAP_GEM_CPU_PREP, ioctl_gem_cpu_prep_fini, DRM_AUTH | DRM_RENDER_ALLOW), - DRM_IOCTL_DEF_DRV(OMAP_GEM_CPU_FINI, ioctl_gem_cpu_fini, + DRM_IOCTL_DEF_DRV(OMAP_GEM_CPU_FINI, ioctl_gem_cpu_prep_fini, DRM_AUTH | DRM_RENDER_ALLOW), DRM_IOCTL_DEF_DRV(OMAP_GEM_INFO, ioctl_gem_info, DRM_AUTH | DRM_RENDER_ALLOW), diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h index 54e6055ea1d3..16aa43c6fbc2 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.h +++ b/drivers/gpu/drm/omapdrm/omap_drv.h @@ -184,11 +184,6 @@ int omap_gem_mmap(struct file *filp, struct vm_area_struct *vma); int omap_gem_mmap_obj(struct drm_gem_object *obj, struct vm_area_struct *vma); int omap_gem_fault(struct vm_fault *vmf); -int omap_gem_op_start(struct drm_gem_object *obj, enum omap_gem_op op); -int omap_gem_op_finish(struct drm_gem_object *obj, enum omap_gem_op op); -int omap_gem_op_sync(struct drm_gem_object *obj, enum omap_gem_op op); -int omap_gem_op_async(struct drm_gem_object *obj, enum omap_gem_op op, - void (*fxn)(void *arg), void *arg); int omap_gem_roll(struct drm_gem_object *obj, uint32_t roll); void omap_gem_cpu_sync(struct drm_gem_object *obj, int pgoff); void omap_gem_dma_sync(struct drm_gem_object *obj, diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c index 68a75b829b71..4bb52a5f5939 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem.c +++ b/drivers/gpu/drm/omapdrm/omap_gem.c @@ -101,19 +101,6 @@ struct omap_gem_object { * Virtual address, if mapped. */ void *vaddr; - - /** - * sync-object allocated on demand (if needed) - * - * Per-buffer sync-object for tracking pending and completed hw/dma - * read and write operations. - */ - struct { - uint32_t write_pending; - uint32_t write_complete; - uint32_t read_pending; - uint32_t read_complete; - } *sync; };
#define to_omap_bo(x) container_of(x, struct omap_gem_object, base) @@ -1071,205 +1058,6 @@ void omap_gem_describe_objects(struct list_head *list, struct seq_file *m) #endif
/* ----------------------------------------------------------------------------- - * Buffer Synchronization - */ - -static DEFINE_SPINLOCK(sync_lock); - -struct omap_gem_sync_waiter { - struct list_head list; - struct omap_gem_object *omap_obj; - enum omap_gem_op op; - uint32_t read_target, write_target; - /* notify called w/ sync_lock held */ - void (*notify)(void *arg); - void *arg; -}; - -/* list of omap_gem_sync_waiter.. the notify fxn gets called back when - * the read and/or write target count is achieved which can call a user - * callback (ex. to kick 3d and/or 2d), wakeup blocked task (prep for - * cpu access), etc. - */ -static LIST_HEAD(waiters); - -static inline bool is_waiting(struct omap_gem_sync_waiter *waiter) -{ - struct omap_gem_object *omap_obj = waiter->omap_obj; - if ((waiter->op & OMAP_GEM_READ) && - (omap_obj->sync->write_complete < waiter->write_target)) - return true; - if ((waiter->op & OMAP_GEM_WRITE) && - (omap_obj->sync->read_complete < waiter->read_target)) - return true; - return false; -} - -/* macro for sync debug.. */ -#define SYNCDBG 0 -#define SYNC(fmt, ...) do { if (SYNCDBG) \ - pr_err("%s:%d: " fmt "\n", __func__, __LINE__, ##__VA_ARGS__); \ - } while (0) - - -static void sync_op_update(void) -{ - struct omap_gem_sync_waiter *waiter, *n; - list_for_each_entry_safe(waiter, n, &waiters, list) { - if (!is_waiting(waiter)) { - list_del(&waiter->list); - SYNC("notify: %p", waiter); - waiter->notify(waiter->arg); - kfree(waiter); - } - } -} - -static inline int sync_op(struct drm_gem_object *obj, - enum omap_gem_op op, bool start) -{ - struct omap_gem_object *omap_obj = to_omap_bo(obj); - int ret = 0; - - spin_lock(&sync_lock); - - if (!omap_obj->sync) { - omap_obj->sync = kzalloc(sizeof(*omap_obj->sync), GFP_ATOMIC); - if (!omap_obj->sync) { - ret = -ENOMEM; - goto unlock; - } - } - - if (start) { - if (op & OMAP_GEM_READ) - omap_obj->sync->read_pending++; - if (op & OMAP_GEM_WRITE) - omap_obj->sync->write_pending++; - } else { - if (op & OMAP_GEM_READ) - omap_obj->sync->read_complete++; - if (op & OMAP_GEM_WRITE) - omap_obj->sync->write_complete++; - sync_op_update(); - } - -unlock: - spin_unlock(&sync_lock); - - return ret; -} - -/* mark the start of read and/or write operation */ -int omap_gem_op_start(struct drm_gem_object *obj, enum omap_gem_op op) -{ - return sync_op(obj, op, true); -} - -int omap_gem_op_finish(struct drm_gem_object *obj, enum omap_gem_op op) -{ - return sync_op(obj, op, false); -} - -static DECLARE_WAIT_QUEUE_HEAD(sync_event); - -static void sync_notify(void *arg) -{ - struct task_struct **waiter_task = arg; - *waiter_task = NULL; - wake_up_all(&sync_event); -} - -int omap_gem_op_sync(struct drm_gem_object *obj, enum omap_gem_op op) -{ - struct omap_gem_object *omap_obj = to_omap_bo(obj); - int ret = 0; - if (omap_obj->sync) { - struct task_struct *waiter_task = current; - struct omap_gem_sync_waiter *waiter = - kzalloc(sizeof(*waiter), GFP_KERNEL); - - if (!waiter) - return -ENOMEM; - - waiter->omap_obj = omap_obj; - waiter->op = op; - waiter->read_target = omap_obj->sync->read_pending; - waiter->write_target = omap_obj->sync->write_pending; - waiter->notify = sync_notify; - waiter->arg = &waiter_task; - - spin_lock(&sync_lock); - if (is_waiting(waiter)) { - SYNC("waited: %p", waiter); - list_add_tail(&waiter->list, &waiters); - spin_unlock(&sync_lock); - ret = wait_event_interruptible(sync_event, - (waiter_task == NULL)); - spin_lock(&sync_lock); - if (waiter_task) { - SYNC("interrupted: %p", waiter); - /* we were interrupted */ - list_del(&waiter->list); - waiter_task = NULL; - } else { - /* freed in sync_op_update() */ - waiter = NULL; - } - } - spin_unlock(&sync_lock); - kfree(waiter); - } - return ret; -} - -/* call fxn(arg), either synchronously or asynchronously if the op - * is currently blocked.. fxn() can be called from any context - * - * (TODO for now fxn is called back from whichever context calls - * omap_gem_op_finish().. but this could be better defined later - * if needed) - * - * TODO more code in common w/ _sync().. - */ -int omap_gem_op_async(struct drm_gem_object *obj, enum omap_gem_op op, - void (*fxn)(void *arg), void *arg) -{ - struct omap_gem_object *omap_obj = to_omap_bo(obj); - if (omap_obj->sync) { - struct omap_gem_sync_waiter *waiter = - kzalloc(sizeof(*waiter), GFP_ATOMIC); - - if (!waiter) - return -ENOMEM; - - waiter->omap_obj = omap_obj; - waiter->op = op; - waiter->read_target = omap_obj->sync->read_pending; - waiter->write_target = omap_obj->sync->write_pending; - waiter->notify = fxn; - waiter->arg = arg; - - spin_lock(&sync_lock); - if (is_waiting(waiter)) { - SYNC("waited: %p", waiter); - list_add_tail(&waiter->list, &waiters); - spin_unlock(&sync_lock); - return 0; - } - - spin_unlock(&sync_lock); - - kfree(waiter); - } - - /* no waiting.. */ - fxn(arg); - - return 0; -} - -/* ----------------------------------------------------------------------------- * Constructor & Destructor */
@@ -1308,8 +1096,6 @@ void omap_gem_free_object(struct drm_gem_object *obj) drm_prime_gem_destroy(obj, omap_obj->sgt); }
- kfree(omap_obj->sync); - drm_gem_object_release(obj);
kfree(omap_obj); diff --git a/include/uapi/drm/omap_drm.h b/include/uapi/drm/omap_drm.h index 7fb97863c945..fd5e3ea53f2b 100644 --- a/include/uapi/drm/omap_drm.h +++ b/include/uapi/drm/omap_drm.h @@ -106,8 +106,8 @@ struct drm_omap_gem_info { #define DRM_OMAP_GET_PARAM 0x00 #define DRM_OMAP_SET_PARAM 0x01 #define DRM_OMAP_GEM_NEW 0x03 -#define DRM_OMAP_GEM_CPU_PREP 0x04 -#define DRM_OMAP_GEM_CPU_FINI 0x05 +#define DRM_OMAP_GEM_CPU_PREP 0x04 /* Deprecated, to be removed */ +#define DRM_OMAP_GEM_CPU_FINI 0x05 /* Deprecated, to be removed */ #define DRM_OMAP_GEM_INFO 0x06 #define DRM_OMAP_NUM_IOCTLS 0x07
On Sat, Apr 15, 2017 at 11:16 AM, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
DRM_IOCTL_DEF_DRV(OMAP_GEM_CPU_PREP, ioctl_gem_cpu_prep,
DRM_IOCTL_DEF_DRV(OMAP_GEM_CPU_PREP, ioctl_gem_cpu_prep_fini, DRM_AUTH | DRM_RENDER_ALLOW),
DRM_IOCTL_DEF_DRV(OMAP_GEM_CPU_FINI, ioctl_gem_cpu_fini,
DRM_IOCTL_DEF_DRV(OMAP_GEM_CPU_FINI, ioctl_gem_cpu_prep_fini, DRM_AUTH | DRM_RENDER_ALLOW),
drm_noop and drm_invalid_op give you 2 default options for this, for even less code. -Daniel
Hi Daniel,
On Tuesday 18 Apr 2017 08:20:29 Daniel Vetter wrote:
On Sat, Apr 15, 2017 at 11:16 AM, Laurent Pinchart wrote:
DRM_IOCTL_DEF_DRV(OMAP_GEM_CPU_PREP, ioctl_gem_cpu_prep,
DRM_IOCTL_DEF_DRV(OMAP_GEM_CPU_PREP, ioctl_gem_cpu_prep_fini, DRM_AUTH | DRM_RENDER_ALLOW),
DRM_IOCTL_DEF_DRV(OMAP_GEM_CPU_FINI, ioctl_gem_cpu_fini,
DRM_IOCTL_DEF_DRV(OMAP_GEM_CPU_FINI, ioctl_gem_cpu_prep_fini, DRM_AUTH | DRM_RENDER_ALLOW),drm_noop and drm_invalid_op give you 2 default options for this, for even less code.
Thank you for the tip, I didn't know about that.
The DRM core implements a standard "zpos" property to control planes ordering. The omapdrm driver implements a similar property named "zorder". Although we can't switch to DRM core handling of the "zpos" property for backward compatibility reasons, we can store the zorder value in the drm_plane_state zpos field, saving us from having to implement custom plane state handling.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/omapdrm/omap_plane.c | 73 +++++------------------------------- 1 file changed, 10 insertions(+), 63 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c index 9168154d749e..521dd2ea519a 100644 --- a/drivers/gpu/drm/omapdrm/omap_plane.c +++ b/drivers/gpu/drm/omapdrm/omap_plane.c @@ -39,18 +39,6 @@ struct omap_plane { uint32_t formats[32]; };
-struct omap_plane_state { - struct drm_plane_state base; - - unsigned int zorder; -}; - -static inline struct omap_plane_state * -to_omap_plane_state(struct drm_plane_state *state) -{ - return container_of(state, struct omap_plane_state, base); -} - static int omap_plane_prepare_fb(struct drm_plane *plane, struct drm_plane_state *new_state) { @@ -73,7 +61,6 @@ static void omap_plane_atomic_update(struct drm_plane *plane, struct omap_drm_private *priv = plane->dev->dev_private; struct omap_plane *omap_plane = to_omap_plane(plane); struct drm_plane_state *state = plane->state; - struct omap_plane_state *omap_state = to_omap_plane_state(state); struct omap_overlay_info info; struct omap_drm_window win; int ret; @@ -85,7 +72,7 @@ static void omap_plane_atomic_update(struct drm_plane *plane, info.rotation = OMAP_DSS_ROT_0; info.global_alpha = 0xff; info.mirror = 0; - info.zorder = omap_state->zorder; + info.zorder = state->zpos;
memset(&win, 0, sizeof(win)); win.rotation = state->rotation; @@ -138,11 +125,10 @@ static void omap_plane_atomic_disable(struct drm_plane *plane, struct drm_plane_state *old_state) { struct omap_drm_private *priv = plane->dev->dev_private; - struct omap_plane_state *omap_state = to_omap_plane_state(plane->state); struct omap_plane *omap_plane = to_omap_plane(plane);
plane->state->rotation = DRM_ROTATE_0; - omap_state->zorder = plane->type == DRM_PLANE_TYPE_PRIMARY + plane->state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY ? 0 : omap_plane->id;
priv->dispc_ops->ovl_enable(omap_plane->id, false); @@ -227,56 +213,20 @@ void omap_plane_install_properties(struct drm_plane *plane, drm_object_attach_property(obj, priv->zorder_prop, 0); }
-static struct drm_plane_state * -omap_plane_atomic_duplicate_state(struct drm_plane *plane) -{ - struct omap_plane_state *state; - struct omap_plane_state *copy; - - if (WARN_ON(!plane->state)) - return NULL; - - state = to_omap_plane_state(plane->state); - copy = kmemdup(state, sizeof(*state), GFP_KERNEL); - if (copy == NULL) - return NULL; - - __drm_atomic_helper_plane_duplicate_state(plane, ©->base); - - return ©->base; -} - -static void omap_plane_atomic_destroy_state(struct drm_plane *plane, - struct drm_plane_state *state) -{ - __drm_atomic_helper_plane_destroy_state(state); - kfree(to_omap_plane_state(state)); -} - static void omap_plane_reset(struct drm_plane *plane) { struct omap_plane *omap_plane = to_omap_plane(plane); - struct omap_plane_state *omap_state; - - if (plane->state) { - omap_plane_atomic_destroy_state(plane, plane->state); - plane->state = NULL; - }
- omap_state = kzalloc(sizeof(*omap_state), GFP_KERNEL); - if (omap_state == NULL) + drm_atomic_helper_plane_reset(plane); + if (!plane->state) return;
/* - * Set defaults depending on whether we are a primary or overlay + * Set the zpos default depending on whether we are a primary or overlay * plane. */ - omap_state->zorder = plane->type == DRM_PLANE_TYPE_PRIMARY + plane->state->zpos = plane->type == DRM_PLANE_TYPE_PRIMARY ? 0 : omap_plane->id; - omap_state->base.rotation = DRM_ROTATE_0; - - plane->state = &omap_state->base; - plane->state->plane = plane; }
static int omap_plane_atomic_set_property(struct drm_plane *plane, @@ -285,10 +235,9 @@ static int omap_plane_atomic_set_property(struct drm_plane *plane, uint64_t val) { struct omap_drm_private *priv = plane->dev->dev_private; - struct omap_plane_state *omap_state = to_omap_plane_state(state);
if (property == priv->zorder_prop) - omap_state->zorder = val; + state->zpos = val; else return -EINVAL;
@@ -301,11 +250,9 @@ static int omap_plane_atomic_get_property(struct drm_plane *plane, uint64_t *val) { struct omap_drm_private *priv = plane->dev->dev_private; - const struct omap_plane_state *omap_state = - container_of(state, const struct omap_plane_state, base);
if (property == priv->zorder_prop) - *val = omap_state->zorder; + *val = state->zpos; else return -EINVAL;
@@ -318,8 +265,8 @@ static const struct drm_plane_funcs omap_plane_funcs = { .reset = omap_plane_reset, .destroy = omap_plane_destroy, .set_property = drm_atomic_helper_plane_set_property, - .atomic_duplicate_state = omap_plane_atomic_duplicate_state, - .atomic_destroy_state = omap_plane_atomic_destroy_state, + .atomic_duplicate_state = drm_atomic_helper_plane_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_plane_destroy_state, .atomic_set_property = omap_plane_atomic_set_property, .atomic_get_property = omap_plane_atomic_get_property, };
On 15/04/17 12:16, Laurent Pinchart wrote:
The DRM core implements a standard "zpos" property to control planes ordering. The omapdrm driver implements a similar property named "zorder". Although we can't switch to DRM core handling of the "zpos" property for backward compatibility reasons, we can store the zorder value in the drm_plane_state zpos field, saving us from having to implement custom plane state handling.
I'm fine with the zpos change, but I'd like to keep the omap_plane_state around. It'll be empty after this patch, but we have a bunch of properties we need to add. Some can be added as common DRM properties, but perhaps not all. It's so much easier to handle those patches if the plumbing is there already, instead of adding it back.
Tomi
Hi Tomi,
On Monday 24 Apr 2017 12:40:10 Tomi Valkeinen wrote:
On 15/04/17 12:16, Laurent Pinchart wrote:
The DRM core implements a standard "zpos" property to control planes ordering. The omapdrm driver implements a similar property named "zorder". Although we can't switch to DRM core handling of the "zpos" property for backward compatibility reasons, we can store the zorder value in the drm_plane_state zpos field, saving us from having to implement custom plane state handling.
I'm fine with the zpos change, but I'd like to keep the omap_plane_state around. It'll be empty after this patch, but we have a bunch of properties we need to add. Some can be added as common DRM properties, but perhaps not all. It's so much easier to handle those patches if the plumbing is there already, instead of adding it back.
How about reverting the needed parts of this patch at that time then ? I think it would be better than keeping useless code around until a hypothetical time when it will be needed.
On 24/04/17 17:00, Laurent Pinchart wrote:
Hi Tomi,
On Monday 24 Apr 2017 12:40:10 Tomi Valkeinen wrote:
On 15/04/17 12:16, Laurent Pinchart wrote:
The DRM core implements a standard "zpos" property to control planes ordering. The omapdrm driver implements a similar property named "zorder". Although we can't switch to DRM core handling of the "zpos" property for backward compatibility reasons, we can store the zorder value in the drm_plane_state zpos field, saving us from having to implement custom plane state handling.
I'm fine with the zpos change, but I'd like to keep the omap_plane_state around. It'll be empty after this patch, but we have a bunch of properties we need to add. Some can be added as common DRM properties, but perhaps not all. It's so much easier to handle those patches if the plumbing is there already, instead of adding it back.
How about reverting the needed parts of this patch at that time then ? I think it would be better than keeping useless code around until a hypothetical time when it will be needed.
I kind of agree, but based on my experience, I disagree =).
We have a bunch of HW properties for both crtcs and planes which are not supported in mainline. I have patches for those. It's difficult to upstream them, because the aim should be to get common properties. Creating common properties is Hard. So I need to carry those patches, maybe for a long time, which means rebasing, which means gazillion conflicts if the mainline version doesn't have the omap custom state for crtc and plane.
In the minimum, the change for the zorder and the change that removes the omap_plane_state should be separate. Then the removal patch can be reverted, and the amount of conflicts drops to half a gazillion.
Tomi
Create a standard zpos property for every plane as an alias to the omapdrm-specific zorder property. Unlike the zorder property that has to be instantiated for both planes and CRTCs due to backward compatibility, the zpos property is only instantiated for planes. When userspace will have switched to the zpos property the zorder property will be removed.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- drivers/gpu/drm/omapdrm/omap_plane.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c index 521dd2ea519a..871a89b87e72 100644 --- a/drivers/gpu/drm/omapdrm/omap_plane.c +++ b/drivers/gpu/drm/omapdrm/omap_plane.c @@ -324,6 +324,7 @@ struct drm_plane *omap_plane_init(struct drm_device *dev, drm_plane_helper_add(plane, &omap_plane_helper_funcs);
omap_plane_install_properties(plane, &plane->base); + drm_plane_create_zpos_property(plane, 0, 0, 3);
return plane;
On 15/04/17 12:16, Laurent Pinchart wrote:
Create a standard zpos property for every plane as an alias to the omapdrm-specific zorder property. Unlike the zorder property that has to be instantiated for both planes and CRTCs due to backward compatibility, the zpos property is only instantiated for planes. When userspace will have switched to the zpos property the zorder property will be removed.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
drivers/gpu/drm/omapdrm/omap_plane.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c index 521dd2ea519a..871a89b87e72 100644 --- a/drivers/gpu/drm/omapdrm/omap_plane.c +++ b/drivers/gpu/drm/omapdrm/omap_plane.c @@ -324,6 +324,7 @@ struct drm_plane *omap_plane_init(struct drm_device *dev, drm_plane_helper_add(plane, &omap_plane_helper_funcs);
omap_plane_install_properties(plane, &plane->base);
- drm_plane_create_zpos_property(plane, 0, 0, 3);
I think this should use get_num_ovls() to get the max value.
Tomi
Hi Tomi,
On Monday 24 Apr 2017 12:37:24 Tomi Valkeinen wrote:
On 15/04/17 12:16, Laurent Pinchart wrote:
Create a standard zpos property for every plane as an alias to the omapdrm-specific zorder property. Unlike the zorder property that has to be instantiated for both planes and CRTCs due to backward compatibility, the zpos property is only instantiated for planes. When userspace will have switched to the zpos property the zorder property will be removed.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
drivers/gpu/drm/omapdrm/omap_plane.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c index 521dd2ea519a..871a89b87e72 100644 --- a/drivers/gpu/drm/omapdrm/omap_plane.c +++ b/drivers/gpu/drm/omapdrm/omap_plane.c @@ -324,6 +324,7 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,> drm_plane_helper_add(plane, &omap_plane_helper_funcs);
omap_plane_install_properties(plane, &plane->base);
- drm_plane_create_zpos_property(plane, 0, 0, 3);
I think this should use get_num_ovls() to get the max value.
That's fine with me, but note that the code currently hardcodes the value to 3 for the zorder property. I can submit an addition patch on top of this to change both if you think it would be better.
Hi Tomi,
On Monday 24 Apr 2017 17:00:52 Laurent Pinchart wrote:
On Monday 24 Apr 2017 12:37:24 Tomi Valkeinen wrote:
On 15/04/17 12:16, Laurent Pinchart wrote:
Create a standard zpos property for every plane as an alias to the omapdrm-specific zorder property. Unlike the zorder property that has to be instantiated for both planes and CRTCs due to backward compatibility, the zpos property is only instantiated for planes. When userspace will have switched to the zpos property the zorder property will be removed.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
drivers/gpu/drm/omapdrm/omap_plane.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c index 521dd2ea519a..871a89b87e72 100644 --- a/drivers/gpu/drm/omapdrm/omap_plane.c +++ b/drivers/gpu/drm/omapdrm/omap_plane.c @@ -324,6 +324,7 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
drm_plane_helper_add(plane, &omap_plane_helper_funcs);
omap_plane_install_properties(plane, &plane->base);
- drm_plane_create_zpos_property(plane, 0, 0, 3);
I think this should use get_num_ovls() to get the max value.
That's fine with me, but note that the code currently hardcodes the value to 3 for the zorder property. I can submit an addition patch on top of this to change both if you think it would be better.
And should it be get_num_ovls() - 1 ? The zorder register field is two bits wide, and we have up to 4 overlays on OMAP4. This will change the maximum value of the property from 3 to 2 on OMAP3. Do you think that could cause issues ?
On 24/04/17 17:05, Laurent Pinchart wrote:
Hi Tomi,
On Monday 24 Apr 2017 17:00:52 Laurent Pinchart wrote:
On Monday 24 Apr 2017 12:37:24 Tomi Valkeinen wrote:
On 15/04/17 12:16, Laurent Pinchart wrote:
Create a standard zpos property for every plane as an alias to the omapdrm-specific zorder property. Unlike the zorder property that has to be instantiated for both planes and CRTCs due to backward compatibility, the zpos property is only instantiated for planes. When userspace will have switched to the zpos property the zorder property will be removed.
Signed-off-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
drivers/gpu/drm/omapdrm/omap_plane.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c index 521dd2ea519a..871a89b87e72 100644 --- a/drivers/gpu/drm/omapdrm/omap_plane.c +++ b/drivers/gpu/drm/omapdrm/omap_plane.c @@ -324,6 +324,7 @@ struct drm_plane *omap_plane_init(struct drm_device *dev,
drm_plane_helper_add(plane, &omap_plane_helper_funcs);
omap_plane_install_properties(plane, &plane->base);
- drm_plane_create_zpos_property(plane, 0, 0, 3);
I think this should use get_num_ovls() to get the max value.
That's fine with me, but note that the code currently hardcodes the value to 3 for the zorder property. I can submit an addition patch on top of this to change both if you think it would be better.
Ah, right, we have it already at 3 currently...
And should it be get_num_ovls() - 1 ? The zorder register field is two bits wide, and we have up to 4 overlays on OMAP4. This will change the maximum value of the property from 3 to 2 on OMAP3. Do you think that could cause issues ?
Yes, num_ovls - 1. On OMAP2/3 we can't even set the zorder, so it doesn't matter. Of course, we shouldn't even have the property for OMAP2/3... Or maybe we could have it, but as read-only.
Tomi
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Laurent Pinchart -
Tomi Valkeinen