[PATCH v2 0/5] omapdrm: fences and zpos

[PATCH v2 0/5] omapdrm: fences and zpos

[Laurent Pinchart]

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(-)

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/5] drm: omapdrm: Handle events when enabling/disabling CRTCs

[Laurent Pinchart]

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);
 }
 
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/5] drm: omapdrm: Use DRM core's atomic commit helper

[Laurent Pinchart]

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;
 };
 
 
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/5] drm: omapdrm: Remove legacy buffer synchronization support

[Laurent Pinchart]

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
 
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 4/5] drm: omapdrm: Store the Z order in the plane state zpos field

[Laurent Pinchart]

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, &copy->base);
-
-	return &copy->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,
 };
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 5/5] drm: omapdrm: Add zpos property

[Laurent Pinchart]

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;
 
-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/5] drm: omapdrm: Remove legacy buffer synchronization support

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 3/5] drm: omapdrm: Remove legacy buffer synchronization support

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/5] drm: omapdrm: Add zpos property

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 1145 bytes --]



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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/5] drm: omapdrm: Store the Z order in the plane state zpos field

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 792 bytes --]

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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/5] drm: omapdrm: Handle events when enabling/disabling CRTCs

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 634 bytes --]

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



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/5] drm: omapdrm: Add zpos property

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/5] drm: omapdrm: Store the Z order in the plane state zpos field

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/5] drm: omapdrm: Add zpos property

[Laurent Pinchart]

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 ?

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 5/5] drm: omapdrm: Add zpos property

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 2125 bytes --]

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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/5] drm: omapdrm: Store the Z order in the plane state zpos field

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 1852 bytes --]

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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/5] drm: omapdrm: Use DRM core's atomic commit helper

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 524 bytes --]

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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel