[PATCH 1/5] OMAPFB: remove exported udpate window

[PATCH 1/5] OMAPFB: remove exported udpate window

[Tomi Valkeinen]

omapfb contains an exported omapfb_update_window function, which, at
some point in history, was used by a closed source SGX driver. This was
a hack even then, and should not be needed anymore. So remove it.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/omapfb/omapfb-ioctl.c |   27 +++------------------------
 1 file changed, 3 insertions(+), 24 deletions(-)

diff --git a/drivers/video/omap2/omapfb/omapfb-ioctl.c b/drivers/video/omap2/omapfb/omapfb-ioctl.c
index 8b1e9e3..94de47e 100644
--- a/drivers/video/omap2/omapfb/omapfb-ioctl.c
+++ b/drivers/video/omap2/omapfb/omapfb-ioctl.c
@@ -279,7 +279,7 @@ static int omapfb_query_mem(struct fb_info *fbi, struct omapfb_mem_info *mi)
 	return 0;
 }
 
-static int omapfb_update_window_nolock(struct fb_info *fbi,
+static int omapfb_update_window(struct fb_info *fbi,
 		u32 x, u32 y, u32 w, u32 h)
 {
 	struct omap_dss_device *display = fb2display(fbi);
@@ -299,27 +299,6 @@ static int omapfb_update_window_nolock(struct fb_info *fbi,
 	return display->driver->update(display, x, y, w, h);
 }
 
-/* This function is exported for SGX driver use */
-int omapfb_update_window(struct fb_info *fbi,
-		u32 x, u32 y, u32 w, u32 h)
-{
-	struct omapfb_info *ofbi = FB2OFB(fbi);
-	struct omapfb2_device *fbdev = ofbi->fbdev;
-	int r;
-
-	if (!lock_fb_info(fbi))
-		return -ENODEV;
-	omapfb_lock(fbdev);
-
-	r = omapfb_update_window_nolock(fbi, x, y, w, h);
-
-	omapfb_unlock(fbdev);
-	unlock_fb_info(fbi);
-
-	return r;
-}
-EXPORT_SYMBOL(omapfb_update_window);
-
 int omapfb_set_update_mode(struct fb_info *fbi,
 				   enum omapfb_update_mode mode)
 {
@@ -646,7 +625,7 @@ int omapfb_ioctl(struct fb_info *fbi, unsigned int cmd, unsigned long arg)
 			break;
 		}
 
-		r = omapfb_update_window_nolock(fbi, p.uwnd_o.x, p.uwnd_o.y,
+		r = omapfb_update_window(fbi, p.uwnd_o.x, p.uwnd_o.y,
 				p.uwnd_o.width, p.uwnd_o.height);
 		break;
 
@@ -663,7 +642,7 @@ int omapfb_ioctl(struct fb_info *fbi, unsigned int cmd, unsigned long arg)
 			break;
 		}
 
-		r = omapfb_update_window_nolock(fbi, p.uwnd.x, p.uwnd.y,
+		r = omapfb_update_window(fbi, p.uwnd.x, p.uwnd.y,
 				p.uwnd.width, p.uwnd.height);
 		break;
 
-- 
1.7.10.4

[PATCH 2/5] OMAPFB: simplify locking

[Tomi Valkeinen]

Kernel lock verification code has lately detected possible circular
locking in omapfb. The exact problem is unclear, but omapfb's current
locking seems to be overly complex.

This patch simplifies the locking in the following ways:

- Remove explicit omapfb mem region locking. I couldn't figure out the
  need for this, as long as we take care to take omapfb lock.

- Get omapfb lock always, even if the operation is possibly only related
  to one fb_info. Better safe than sorry, and normally there's only one
  user for the fb so this shouldn't matter.

- Make sure fb_info lock is taken first, then omapfb lock.

With this patch the warnings about possible circular locking does not
happen anymore.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/video/omap2/omapfb/omapfb-ioctl.c |   65 +++-------------------
 drivers/video/omap2/omapfb/omapfb-main.c  |   40 ++++----------
 drivers/video/omap2/omapfb/omapfb-sysfs.c |   84 +++++++++++++++++++----------
 drivers/video/omap2/omapfb/omapfb.h       |   19 -------
 4 files changed, 73 insertions(+), 135 deletions(-)

diff --git a/drivers/video/omap2/omapfb/omapfb-ioctl.c b/drivers/video/omap2/omapfb/omapfb-ioctl.c
index 94de47e..ae7aac7 100644
--- a/drivers/video/omap2/omapfb/omapfb-ioctl.c
+++ b/drivers/video/omap2/omapfb/omapfb-ioctl.c
@@ -85,16 +85,6 @@ static int omapfb_setup_plane(struct fb_info *fbi, struct omapfb_plane_info *pi)
 		goto out;
 	}
 
-	/* Take the locks in a specific order to keep lockdep happy */
-	if (old_rg->id < new_rg->id) {
-		omapfb_get_mem_region(old_rg);
-		omapfb_get_mem_region(new_rg);
-	} else if (new_rg->id < old_rg->id) {
-		omapfb_get_mem_region(new_rg);
-		omapfb_get_mem_region(old_rg);
-	} else
-		omapfb_get_mem_region(old_rg);
-
 	if (pi->enabled && !new_rg->size) {
 		/*
 		 * This plane's memory was freed, can't enable it
@@ -146,16 +136,6 @@ static int omapfb_setup_plane(struct fb_info *fbi, struct omapfb_plane_info *pi)
 			goto undo;
 	}
 
-	/* Release the locks in a specific order to keep lockdep happy */
-	if (old_rg->id > new_rg->id) {
-		omapfb_put_mem_region(old_rg);
-		omapfb_put_mem_region(new_rg);
-	} else if (new_rg->id > old_rg->id) {
-		omapfb_put_mem_region(new_rg);
-		omapfb_put_mem_region(old_rg);
-	} else
-		omapfb_put_mem_region(old_rg);
-
 	return 0;
 
  undo:
@@ -166,15 +146,6 @@ static int omapfb_setup_plane(struct fb_info *fbi, struct omapfb_plane_info *pi)
 
 	ovl->set_overlay_info(ovl, &old_info);
  put_mem:
-	/* Release the locks in a specific order to keep lockdep happy */
-	if (old_rg->id > new_rg->id) {
-		omapfb_put_mem_region(old_rg);
-		omapfb_put_mem_region(new_rg);
-	} else if (new_rg->id > old_rg->id) {
-		omapfb_put_mem_region(new_rg);
-		omapfb_put_mem_region(old_rg);
-	} else
-		omapfb_put_mem_region(old_rg);
  out:
 	dev_err(fbdev->dev, "setup_plane failed\n");
 
@@ -222,9 +193,6 @@ static int omapfb_setup_mem(struct fb_info *fbi, struct omapfb_mem_info *mi)
 
 	rg = ofbi->region;
 
-	down_write_nested(&rg->lock, rg->id);
-	atomic_inc(&rg->lock_count);
-
 	if (rg->size = size && rg->type = mi->type)
 		goto out;
 
@@ -257,9 +225,6 @@ static int omapfb_setup_mem(struct fb_info *fbi, struct omapfb_mem_info *mi)
 	}
 
  out:
-	atomic_dec(&rg->lock_count);
-	up_write(&rg->lock);
-
 	return r;
 }
 
@@ -268,14 +233,12 @@ static int omapfb_query_mem(struct fb_info *fbi, struct omapfb_mem_info *mi)
 	struct omapfb_info *ofbi = FB2OFB(fbi);
 	struct omapfb2_mem_region *rg;
 
-	rg = omapfb_get_mem_region(ofbi->region);
+	rg = ofbi->region;
 	memset(mi, 0, sizeof(*mi));
 
 	mi->size = rg->size;
 	mi->type = rg->type;
 
-	omapfb_put_mem_region(rg);
-
 	return 0;
 }
 
@@ -314,14 +277,10 @@ int omapfb_set_update_mode(struct fb_info *fbi,
 	if (mode != OMAPFB_AUTO_UPDATE && mode != OMAPFB_MANUAL_UPDATE)
 		return -EINVAL;
 
-	omapfb_lock(fbdev);
-
 	d = get_display_data(fbdev, display);
 
-	if (d->update_mode = mode) {
-		omapfb_unlock(fbdev);
+	if (d->update_mode = mode)
 		return 0;
-	}
 
 	r = 0;
 
@@ -337,8 +296,6 @@ int omapfb_set_update_mode(struct fb_info *fbi,
 			r = -EINVAL;
 	}
 
-	omapfb_unlock(fbdev);
-
 	return r;
 }
 
@@ -353,14 +310,10 @@ int omapfb_get_update_mode(struct fb_info *fbi,
 	if (!display)
 		return -EINVAL;
 
-	omapfb_lock(fbdev);
-
 	d = get_display_data(fbdev, display);
 
 	*mode = d->update_mode;
 
-	omapfb_unlock(fbdev);
-
 	return 0;
 }
 
@@ -420,13 +373,10 @@ static int omapfb_set_color_key(struct fb_info *fbi,
 		struct omapfb_color_key *ck)
 {
 	struct omapfb_info *ofbi = FB2OFB(fbi);
-	struct omapfb2_device *fbdev = ofbi->fbdev;
 	int r;
 	int i;
 	struct omap_overlay_manager *mgr = NULL;
 
-	omapfb_lock(fbdev);
-
 	for (i = 0; i < ofbi->num_overlays; i++) {
 		if (ofbi->overlays[i]->manager) {
 			mgr = ofbi->overlays[i]->manager;
@@ -441,8 +391,6 @@ static int omapfb_set_color_key(struct fb_info *fbi,
 
 	r = _omapfb_set_color_key(mgr, ck);
 err:
-	omapfb_unlock(fbdev);
-
 	return r;
 }
 
@@ -450,13 +398,10 @@ static int omapfb_get_color_key(struct fb_info *fbi,
 		struct omapfb_color_key *ck)
 {
 	struct omapfb_info *ofbi = FB2OFB(fbi);
-	struct omapfb2_device *fbdev = ofbi->fbdev;
 	struct omap_overlay_manager *mgr = NULL;
 	int r = 0;
 	int i;
 
-	omapfb_lock(fbdev);
-
 	for (i = 0; i < ofbi->num_overlays; i++) {
 		if (ofbi->overlays[i]->manager) {
 			mgr = ofbi->overlays[i]->manager;
@@ -471,8 +416,6 @@ static int omapfb_get_color_key(struct fb_info *fbi,
 
 	*ck = omapfb_color_keys[mgr->id];
 err:
-	omapfb_unlock(fbdev);
-
 	return r;
 }
 
@@ -599,6 +542,8 @@ int omapfb_ioctl(struct fb_info *fbi, unsigned int cmd, unsigned long arg)
 
 	int r = 0;
 
+	omapfb_lock(fbdev);
+
 	switch (cmd) {
 	case OMAPFB_SYNC_GFX:
 		DBG("ioctl SYNC_GFX\n");
@@ -904,6 +849,8 @@ int omapfb_ioctl(struct fb_info *fbi, unsigned int cmd, unsigned long arg)
 		r = -EINVAL;
 	}
 
+	omapfb_unlock(fbdev);
+
 	if (r < 0)
 		DBG("ioctl failed: %d\n", r);
 
diff --git a/drivers/video/omap2/omapfb/omapfb-main.c b/drivers/video/omap2/omapfb/omapfb-main.c
index 1a69d7c..28b2a21 100644
--- a/drivers/video/omap2/omapfb/omapfb-main.c
+++ b/drivers/video/omap2/omapfb/omapfb-main.c
@@ -672,8 +672,6 @@ int check_fb_var(struct fb_info *fbi, struct fb_var_screeninfo *var)
 
 	DBG("check_fb_var %d\n", ofbi->id);
 
-	WARN_ON(!atomic_read(&ofbi->region->lock_count));
-
 	r = fb_mode_to_dss_mode(var, &mode);
 	if (r) {
 		DBG("cannot convert var to omap dss mode\n");
@@ -855,8 +853,6 @@ int omapfb_setup_overlay(struct fb_info *fbi, struct omap_overlay *ovl,
 	int rotation = var->rotate;
 	int i;
 
-	WARN_ON(!atomic_read(&ofbi->region->lock_count));
-
 	for (i = 0; i < ofbi->num_overlays; i++) {
 		if (ovl != ofbi->overlays[i])
 			continue;
@@ -948,8 +944,6 @@ int omapfb_apply_changes(struct fb_info *fbi, int init)
 		fill_fb(fbi);
 #endif
 
-	WARN_ON(!atomic_read(&ofbi->region->lock_count));
-
 	for (i = 0; i < ofbi->num_overlays; i++) {
 		ovl = ofbi->overlays[i];
 
@@ -1008,15 +1002,16 @@ err:
 static int omapfb_check_var(struct fb_var_screeninfo *var, struct fb_info *fbi)
 {
 	struct omapfb_info *ofbi = FB2OFB(fbi);
+	struct omapfb2_device *fbdev = ofbi->fbdev;
 	int r;
 
 	DBG("check_var(%d)\n", FB2OFB(fbi)->id);
 
-	omapfb_get_mem_region(ofbi->region);
+	omapfb_lock(fbdev);
 
 	r = check_fb_var(fbi, var);
 
-	omapfb_put_mem_region(ofbi->region);
+	omapfb_unlock(fbdev);
 
 	return r;
 }
@@ -1025,11 +1020,12 @@ static int omapfb_check_var(struct fb_var_screeninfo *var, struct fb_info *fbi)
 static int omapfb_set_par(struct fb_info *fbi)
 {
 	struct omapfb_info *ofbi = FB2OFB(fbi);
+	struct omapfb2_device *fbdev = ofbi->fbdev;
 	int r;
 
 	DBG("set_par(%d)\n", FB2OFB(fbi)->id);
 
-	omapfb_get_mem_region(ofbi->region);
+	omapfb_lock(fbdev);
 
 	set_fb_fix(fbi);
 
@@ -1040,7 +1036,7 @@ static int omapfb_set_par(struct fb_info *fbi)
 	r = omapfb_apply_changes(fbi, 0);
 
  out:
-	omapfb_put_mem_region(ofbi->region);
+	omapfb_unlock(fbdev);
 
 	return r;
 }
@@ -1049,6 +1045,7 @@ static int omapfb_pan_display(struct fb_var_screeninfo *var,
 		struct fb_info *fbi)
 {
 	struct omapfb_info *ofbi = FB2OFB(fbi);
+	struct omapfb2_device *fbdev = ofbi->fbdev;
 	struct fb_var_screeninfo new_var;
 	int r;
 
@@ -1064,11 +1061,11 @@ static int omapfb_pan_display(struct fb_var_screeninfo *var,
 
 	fbi->var = new_var;
 
-	omapfb_get_mem_region(ofbi->region);
+	omapfb_lock(fbdev);
 
 	r = omapfb_apply_changes(fbi, 0);
 
-	omapfb_put_mem_region(ofbi->region);
+	omapfb_unlock(fbdev);
 
 	return r;
 }
@@ -1077,18 +1074,14 @@ static void mmap_user_open(struct vm_area_struct *vma)
 {
 	struct omapfb2_mem_region *rg = vma->vm_private_data;
 
-	omapfb_get_mem_region(rg);
 	atomic_inc(&rg->map_count);
-	omapfb_put_mem_region(rg);
 }
 
 static void mmap_user_close(struct vm_area_struct *vma)
 {
 	struct omapfb2_mem_region *rg = vma->vm_private_data;
 
-	omapfb_get_mem_region(rg);
 	atomic_dec(&rg->map_count);
-	omapfb_put_mem_region(rg);
 }
 
 static struct vm_operations_struct mmap_user_ops = {
@@ -1112,7 +1105,7 @@ static int omapfb_mmap(struct fb_info *fbi, struct vm_area_struct *vma)
 		return -EINVAL;
 	off = vma->vm_pgoff << PAGE_SHIFT;
 
-	rg = omapfb_get_mem_region(ofbi->region);
+	rg = ofbi->region;
 
 	start = omapfb_get_region_paddr(ofbi);
 	len = fix->smem_len;
@@ -1140,13 +1133,9 @@ static int omapfb_mmap(struct fb_info *fbi, struct vm_area_struct *vma)
 	/* vm_ops.open won't be called for mmap itself. */
 	atomic_inc(&rg->map_count);
 
-	omapfb_put_mem_region(rg);
-
 	return 0;
 
  error:
-	omapfb_put_mem_region(ofbi->region);
-
 	return r;
 }
 
@@ -1918,7 +1907,6 @@ static int omapfb_create_framebuffers(struct omapfb2_device *fbdev)
 
 		ofbi->region = &fbdev->regions[i];
 		ofbi->region->id = i;
-		init_rwsem(&ofbi->region->lock);
 
 		/* assign these early, so that fb alloc can use them */
 		ofbi->rotation_type = def_vrfb ? OMAP_DSS_ROT_VRFB :
@@ -1950,12 +1938,8 @@ static int omapfb_create_framebuffers(struct omapfb2_device *fbdev)
 	/* setup fb_infos */
 	for (i = 0; i < fbdev->num_fbs; i++) {
 		struct fb_info *fbi = fbdev->fbs[i];
-		struct omapfb_info *ofbi = FB2OFB(fbi);
 
-		omapfb_get_mem_region(ofbi->region);
 		r = omapfb_fb_init(fbdev, fbi);
-		omapfb_put_mem_region(ofbi->region);
-
 		if (r) {
 			dev_err(fbdev->dev, "failed to setup fb_info\n");
 			return r;
@@ -1987,12 +1971,8 @@ static int omapfb_create_framebuffers(struct omapfb2_device *fbdev)
 
 	for (i = 0; i < fbdev->num_fbs; i++) {
 		struct fb_info *fbi = fbdev->fbs[i];
-		struct omapfb_info *ofbi = FB2OFB(fbi);
 
-		omapfb_get_mem_region(ofbi->region);
 		r = omapfb_apply_changes(fbi, 1);
-		omapfb_put_mem_region(ofbi->region);
-
 		if (r) {
 			dev_err(fbdev->dev, "failed to change mode\n");
 			return r;
diff --git a/drivers/video/omap2/omapfb/omapfb-sysfs.c b/drivers/video/omap2/omapfb/omapfb-sysfs.c
index 17aa174..6614462 100644
--- a/drivers/video/omap2/omapfb/omapfb-sysfs.c
+++ b/drivers/video/omap2/omapfb/omapfb-sysfs.c
@@ -49,6 +49,7 @@ static ssize_t store_rotate_type(struct device *dev,
 {
 	struct fb_info *fbi = dev_get_drvdata(dev);
 	struct omapfb_info *ofbi = FB2OFB(fbi);
+	struct omapfb2_device *fbdev = ofbi->fbdev;
 	struct omapfb2_mem_region *rg;
 	int rot_type;
 	int r;
@@ -62,12 +63,13 @@ static ssize_t store_rotate_type(struct device *dev,
 
 	if (!lock_fb_info(fbi))
 		return -ENODEV;
+	omapfb_lock(fbdev);
 
 	r = 0;
 	if (rot_type = ofbi->rotation_type)
 		goto out;
 
-	rg = omapfb_get_mem_region(ofbi->region);
+	rg = ofbi->region;
 
 	if (rg->size) {
 		r = -EBUSY;
@@ -81,8 +83,8 @@ static ssize_t store_rotate_type(struct device *dev,
 	 * need to do any further parameter checking at this point.
 	 */
 put_region:
-	omapfb_put_mem_region(rg);
 out:
+	omapfb_unlock(fbdev);
 	unlock_fb_info(fbi);
 
 	return r ? r : count;
@@ -104,6 +106,7 @@ static ssize_t store_mirror(struct device *dev,
 {
 	struct fb_info *fbi = dev_get_drvdata(dev);
 	struct omapfb_info *ofbi = FB2OFB(fbi);
+	struct omapfb2_device *fbdev = ofbi->fbdev;
 	bool mirror;
 	int r;
 	struct fb_var_screeninfo new_var;
@@ -114,11 +117,10 @@ static ssize_t store_mirror(struct device *dev,
 
 	if (!lock_fb_info(fbi))
 		return -ENODEV;
+	omapfb_lock(fbdev);
 
 	ofbi->mirror = mirror;
 
-	omapfb_get_mem_region(ofbi->region);
-
 	memcpy(&new_var, &fbi->var, sizeof(new_var));
 	r = check_fb_var(fbi, &new_var);
 	if (r)
@@ -133,8 +135,7 @@ static ssize_t store_mirror(struct device *dev,
 
 	r = count;
 out:
-	omapfb_put_mem_region(ofbi->region);
-
+	omapfb_unlock(fbdev);
 	unlock_fb_info(fbi);
 
 	return r;
@@ -273,15 +274,11 @@ static ssize_t store_overlays(struct device *dev, struct device_attribute *attr,
 
 		DBG("detaching %d\n", ofbi->overlays[i]->id);
 
-		omapfb_get_mem_region(ofbi->region);
-
 		omapfb_overlay_enable(ovl, 0);
 
 		if (ovl->manager)
 			ovl->manager->apply(ovl->manager);
 
-		omapfb_put_mem_region(ofbi->region);
-
 		for (t = i + 1; t < ofbi->num_overlays; t++) {
 			ofbi->rotation[t-1] = ofbi->rotation[t];
 			ofbi->overlays[t-1] = ofbi->overlays[t];
@@ -314,12 +311,8 @@ static ssize_t store_overlays(struct device *dev, struct device_attribute *attr,
 	}
 
 	if (added) {
-		omapfb_get_mem_region(ofbi->region);
-
 		r = omapfb_apply_changes(fbi, 0);
 
-		omapfb_put_mem_region(ofbi->region);
-
 		if (r)
 			goto out;
 	}
@@ -337,11 +330,13 @@ static ssize_t show_overlays_rotate(struct device *dev,
 {
 	struct fb_info *fbi = dev_get_drvdata(dev);
 	struct omapfb_info *ofbi = FB2OFB(fbi);
+	struct omapfb2_device *fbdev = ofbi->fbdev;
 	ssize_t l = 0;
 	int t;
 
 	if (!lock_fb_info(fbi))
 		return -ENODEV;
+	omapfb_lock(fbdev);
 
 	for (t = 0; t < ofbi->num_overlays; t++) {
 		l += snprintf(buf + l, PAGE_SIZE - l, "%s%d",
@@ -350,6 +345,7 @@ static ssize_t show_overlays_rotate(struct device *dev,
 
 	l += snprintf(buf + l, PAGE_SIZE - l, "\n");
 
+	omapfb_unlock(fbdev);
 	unlock_fb_info(fbi);
 
 	return l;
@@ -360,6 +356,7 @@ static ssize_t store_overlays_rotate(struct device *dev,
 {
 	struct fb_info *fbi = dev_get_drvdata(dev);
 	struct omapfb_info *ofbi = FB2OFB(fbi);
+	struct omapfb2_device *fbdev = ofbi->fbdev;
 	int num_ovls = 0, r, i;
 	int len;
 	bool changed = false;
@@ -371,6 +368,7 @@ static ssize_t store_overlays_rotate(struct device *dev,
 
 	if (!lock_fb_info(fbi))
 		return -ENODEV;
+	omapfb_lock(fbdev);
 
 	if (len > 0) {
 		char *p = (char *)buf;
@@ -407,12 +405,7 @@ static ssize_t store_overlays_rotate(struct device *dev,
 		for (i = 0; i < num_ovls; ++i)
 			ofbi->rotation[i] = rotation[i];
 
-		omapfb_get_mem_region(ofbi->region);
-
 		r = omapfb_apply_changes(fbi, 0);
-
-		omapfb_put_mem_region(ofbi->region);
-
 		if (r)
 			goto out;
 
@@ -421,6 +414,7 @@ static ssize_t store_overlays_rotate(struct device *dev,
 
 	r = count;
 out:
+	omapfb_unlock(fbdev);
 	unlock_fb_info(fbi);
 
 	return r;
@@ -431,8 +425,19 @@ static ssize_t show_size(struct device *dev,
 {
 	struct fb_info *fbi = dev_get_drvdata(dev);
 	struct omapfb_info *ofbi = FB2OFB(fbi);
+	struct omapfb2_device *fbdev = ofbi->fbdev;
+	int r;
+
+	if (!lock_fb_info(fbi))
+		return -ENODEV;
+	omapfb_lock(fbdev);
+
+	r = snprintf(buf, PAGE_SIZE, "%lu\n", ofbi->region->size);
 
-	return snprintf(buf, PAGE_SIZE, "%lu\n", ofbi->region->size);
+	omapfb_unlock(fbdev);
+	unlock_fb_info(fbi);
+
+	return r;
 }
 
 static ssize_t store_size(struct device *dev, struct device_attribute *attr,
@@ -454,12 +459,10 @@ static ssize_t store_size(struct device *dev, struct device_attribute *attr,
 
 	if (!lock_fb_info(fbi))
 		return -ENODEV;
+	omapfb_lock(fbdev);
 
 	rg = ofbi->region;
 
-	down_write_nested(&rg->lock, rg->id);
-	atomic_inc(&rg->lock_count);
-
 	if (atomic_read(&rg->map_count)) {
 		r = -EBUSY;
 		goto out;
@@ -492,9 +495,7 @@ static ssize_t store_size(struct device *dev, struct device_attribute *attr,
 
 	r = count;
 out:
-	atomic_dec(&rg->lock_count);
-	up_write(&rg->lock);
-
+	omapfb_unlock(fbdev);
 	unlock_fb_info(fbi);
 
 	return r;
@@ -505,8 +506,19 @@ static ssize_t show_phys(struct device *dev,
 {
 	struct fb_info *fbi = dev_get_drvdata(dev);
 	struct omapfb_info *ofbi = FB2OFB(fbi);
+	struct omapfb2_device *fbdev = ofbi->fbdev;
+	int r;
 
-	return snprintf(buf, PAGE_SIZE, "%0x\n", ofbi->region->paddr);
+	if (!lock_fb_info(fbi))
+		return -ENODEV;
+	omapfb_lock(fbdev);
+
+	r = snprintf(buf, PAGE_SIZE, "%0x\n", ofbi->region->paddr);
+
+	omapfb_unlock(fbdev);
+	unlock_fb_info(fbi);
+
+	return r;
 }
 
 static ssize_t show_virt(struct device *dev,
@@ -522,11 +534,20 @@ static ssize_t show_upd_mode(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct fb_info *fbi = dev_get_drvdata(dev);
+	struct omapfb_info *ofbi = FB2OFB(fbi);
+	struct omapfb2_device *fbdev = ofbi->fbdev;
 	enum omapfb_update_mode mode;
 	int r;
 
+	if (!lock_fb_info(fbi))
+		return -ENODEV;
+	omapfb_lock(fbdev);
+
 	r = omapfb_get_update_mode(fbi, &mode);
 
+	omapfb_unlock(fbdev);
+	unlock_fb_info(fbi);
+
 	if (r)
 		return r;
 
@@ -537,6 +558,8 @@ static ssize_t store_upd_mode(struct device *dev, struct device_attribute *attr,
 		const char *buf, size_t count)
 {
 	struct fb_info *fbi = dev_get_drvdata(dev);
+	struct omapfb_info *ofbi = FB2OFB(fbi);
+	struct omapfb2_device *fbdev = ofbi->fbdev;
 	unsigned mode;
 	int r;
 
@@ -544,10 +567,17 @@ static ssize_t store_upd_mode(struct device *dev, struct device_attribute *attr,
 	if (r)
 		return r;
 
+	if (!lock_fb_info(fbi))
+		return -ENODEV;
+	omapfb_lock(fbdev);
+
 	r = omapfb_set_update_mode(fbi, mode);
 	if (r)
 		return r;
 
+	omapfb_unlock(fbdev);
+	unlock_fb_info(fbi);
+
 	return count;
 }
 
diff --git a/drivers/video/omap2/omapfb/omapfb.h b/drivers/video/omap2/omapfb/omapfb.h
index 5f72bf9..71cd8ba 100644
--- a/drivers/video/omap2/omapfb/omapfb.h
+++ b/drivers/video/omap2/omapfb/omapfb.h
@@ -62,8 +62,6 @@ struct omapfb2_mem_region {
 	bool		alloc;		/* allocated by the driver */
 	bool		map;		/* kernel mapped by the driver */
 	atomic_t	map_count;
-	struct rw_semaphore lock;
-	atomic_t	lock_count;
 };
 
 /* appended to fb_info */
@@ -129,9 +127,6 @@ void omapfb_remove_sysfs(struct omapfb2_device *fbdev);
 
 int omapfb_ioctl(struct fb_info *fbi, unsigned int cmd, unsigned long arg);
 
-int omapfb_update_window(struct fb_info *fbi,
-		u32 x, u32 y, u32 w, u32 h);
-
 int dss_mode_to_fb_mode(enum omap_color_mode dssmode,
 			struct fb_var_screeninfo *var);
 
@@ -194,18 +189,4 @@ static inline int omapfb_overlay_enable(struct omap_overlay *ovl,
 		return ovl->disable(ovl);
 }
 
-static inline struct omapfb2_mem_region *
-omapfb_get_mem_region(struct omapfb2_mem_region *rg)
-{
-	down_read_nested(&rg->lock, rg->id);
-	atomic_inc(&rg->lock_count);
-	return rg;
-}
-
-static inline void omapfb_put_mem_region(struct omapfb2_mem_region *rg)
-{
-	atomic_dec(&rg->lock_count);
-	up_read(&rg->lock);
-}
-
 #endif
-- 
1.7.10.4

Re: [PATCH 2/5] OMAPFB: simplify locking

[Ville Syrjälä]

On Fri, Dec 07, 2012 at 01:55:06PM +0200, Tomi Valkeinen wrote:
> Kernel lock verification code has lately detected possible circular
> locking in omapfb. The exact problem is unclear, but omapfb's current
> locking seems to be overly complex.
> 
> This patch simplifies the locking in the following ways:
> 
> - Remove explicit omapfb mem region locking. I couldn't figure out the
>   need for this, as long as we take care to take omapfb lock.

I suppose the idea with that was that you wouldn't need the global
omapfb lock, and also it was an rwsem so it allowed parallel access to
the mem regions, unless the region size was being changed, in which
case it took the write lock. I can't really remember what the reason
for using an rwsem was, but I suppose there was one at the time.

I think the only correctness issue with your patch is that you're
opening up a race between omapfb_mmap and
omapfb_setup_mem/store_size.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/5] OMAPFB: simplify locking

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1537 bytes --]

On 2012-12-07 14:53, Ville Syrjälä wrote:
> On Fri, Dec 07, 2012 at 01:55:06PM +0200, Tomi Valkeinen wrote:
>> Kernel lock verification code has lately detected possible circular
>> locking in omapfb. The exact problem is unclear, but omapfb's current
>> locking seems to be overly complex.
>>
>> This patch simplifies the locking in the following ways:
>>
>> - Remove explicit omapfb mem region locking. I couldn't figure out the
>>   need for this, as long as we take care to take omapfb lock.
> 
> I suppose the idea with that was that you wouldn't need the global
> omapfb lock, and also it was an rwsem so it allowed parallel access to
> the mem regions, unless the region size was being changed, in which
> case it took the write lock. I can't really remember what the reason
> for using an rwsem was, but I suppose there was one at the time.

Right. Yes, I have no recollection either of the possible reason for it
=). Did we have multiple concurrerent users for the fbs? It still sounds
like a useless optimization, as all the region locks were only held for
a short time, as far as I saw.

It could also be that we're missing something from the mainline kernel,
which we had in the Nokia kernel, and which would explain the need for
region locks.

> I think the only correctness issue with your patch is that you're
> opening up a race between omapfb_mmap and
> omapfb_setup_mem/store_size.

Good point. I think this can be fixed by taking fb_info->mm_lock in
omapfb_setup_mem & co.

 Tomi



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

Re: [PATCH 2/5] OMAPFB: simplify locking

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1944 bytes --]

On 2012-12-07 15:42, Tomi Valkeinen wrote:
> On 2012-12-07 14:53, Ville Syrjälä wrote:
>> On Fri, Dec 07, 2012 at 01:55:06PM +0200, Tomi Valkeinen wrote:
>>> Kernel lock verification code has lately detected possible circular
>>> locking in omapfb. The exact problem is unclear, but omapfb's current
>>> locking seems to be overly complex.
>>>
>>> This patch simplifies the locking in the following ways:
>>>
>>> - Remove explicit omapfb mem region locking. I couldn't figure out the
>>>   need for this, as long as we take care to take omapfb lock.
>>
>> I suppose the idea with that was that you wouldn't need the global
>> omapfb lock, and also it was an rwsem so it allowed parallel access to
>> the mem regions, unless the region size was being changed, in which
>> case it took the write lock. I can't really remember what the reason
>> for using an rwsem was, but I suppose there was one at the time.
> 
> Right. Yes, I have no recollection either of the possible reason for it
> =). Did we have multiple concurrerent users for the fbs? It still sounds
> like a useless optimization, as all the region locks were only held for
> a short time, as far as I saw.
> 
> It could also be that we're missing something from the mainline kernel,
> which we had in the Nokia kernel, and which would explain the need for
> region locks.
> 
>> I think the only correctness issue with your patch is that you're
>> opening up a race between omapfb_mmap and
>> omapfb_setup_mem/store_size.
> 
> Good point. I think this can be fixed by taking fb_info->mm_lock in
> omapfb_setup_mem & co.

Well... Adding using of fb_info->mm_lock to omapfb_setup_mem again
causes possible circular locking warnings. And I still can't figure out
the exact reason. Perhaps the region locking was not involved in this
warning at all, but the problem is elsewhere.

(I hope the circular locking detection is correct =).

 Tomi



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

Re: [PATCH 2/5] OMAPFB: simplify locking

[Ville Syrjälä]

On Fri, Dec 07, 2012 at 03:42:10PM +0200, Tomi Valkeinen wrote:
> On 2012-12-07 14:53, Ville Syrjälä wrote:
> > On Fri, Dec 07, 2012 at 01:55:06PM +0200, Tomi Valkeinen wrote:
> >> Kernel lock verification code has lately detected possible circular
> >> locking in omapfb. The exact problem is unclear, but omapfb's current
> >> locking seems to be overly complex.
> >>
> >> This patch simplifies the locking in the following ways:
> >>
> >> - Remove explicit omapfb mem region locking. I couldn't figure out the
> >>   need for this, as long as we take care to take omapfb lock.
> > 
> > I suppose the idea with that was that you wouldn't need the global
> > omapfb lock, and also it was an rwsem so it allowed parallel access to
> > the mem regions, unless the region size was being changed, in which
> > case it took the write lock. I can't really remember what the reason
> > for using an rwsem was, but I suppose there was one at the time.
> 
> Right. Yes, I have no recollection either of the possible reason for it
> =). Did we have multiple concurrerent users for the fbs? It still sounds
> like a useless optimization, as all the region locks were only held for
> a short time, as far as I saw.
> 
> It could also be that we're missing something from the mainline kernel,
> which we had in the Nokia kernel, and which would explain the need for
> region locks.

Yeah, perhaps there was some use for it at the time. Possibly the
zero copy video playback benefited from it in the beginning. But
IIRC almost everything was handled via the X server in the end,
so I don't think there's any compelling reason for keeping the
rwsem.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 2/5] OMAPFB: simplify locking

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 983 bytes --]

On 2012-12-07 13:55, Tomi Valkeinen wrote:
> Kernel lock verification code has lately detected possible circular
> locking in omapfb. The exact problem is unclear, but omapfb's current
> locking seems to be overly complex.
> 
> This patch simplifies the locking in the following ways:
> 
> - Remove explicit omapfb mem region locking. I couldn't figure out the
>   need for this, as long as we take care to take omapfb lock.
> 
> - Get omapfb lock always, even if the operation is possibly only related
>   to one fb_info. Better safe than sorry, and normally there's only one
>   user for the fb so this shouldn't matter.
> 
> - Make sure fb_info lock is taken first, then omapfb lock.
> 
> With this patch the warnings about possible circular locking does not
> happen anymore.

I'm dropping this patch, for some reason it causes huge latencies with
two processes using separate fbs. I guess there's need for the more
precise locking, after all.

 Tomi



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

[PATCH 3/5] OMAPFB: remove warning when trying to alloc at certain paddress

[Tomi Valkeinen]

omapfb gives a WARN_ONCE if a predefined physical address is given for
allocating the framebuffer memory, as this is not currently supported.

However, the same warning happens if omapfb fails to allocate memory
during runtime, as when the allocation has failed, omapfb tries to
re-allocate the old memory with the physical address of the old memory
area.

Remove the warning from omapfb_alloc_fbmem, as it serves no purpose on
the failure case above, and move it to omapfb_parse_vram_param, so that
we only warn if physical address is given via omapfb module parameters.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/omapfb/omapfb-main.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/video/omap2/omapfb/omapfb-main.c b/drivers/video/omap2/omapfb/omapfb-main.c
index 28b2a21..1df973e 100644
--- a/drivers/video/omap2/omapfb/omapfb-main.c
+++ b/drivers/video/omap2/omapfb/omapfb-main.c
@@ -1391,9 +1391,6 @@ static int omapfb_alloc_fbmem(struct fb_info *fbi, unsigned long size,
 
 	size = PAGE_ALIGN(size);
 
-	WARN_ONCE(paddr,
-		"reserving memory at predefined address not supported\n");
-
 	dma_set_attr(DMA_ATTR_WRITE_COMBINE, &attrs);
 
 	if (ofbi->rotation_type = OMAP_DSS_ROT_VRFB)
@@ -1521,6 +1518,9 @@ static int omapfb_parse_vram_param(const char *param, int max_entries,
 
 		}
 
+		WARN_ONCE(paddr,
+			"reserving memory at predefined address not supported\n");
+
 		paddrs[fbnum] = paddr;
 		sizes[fbnum] = size;
 
-- 
1.7.10.4

[PATCH 4/5] OMAPDSS: manage output-dssdev connection in output drivers

[Tomi Valkeinen]

We currently attach an output to a dssdev in the initialization code for
dssdevices in display.c. This works, but doesn't quite make sense: an
output entity represents (surprisingly) an output of DSS, which is
managed by an output driver. The output driver also handles adding new
dssdev's for that particular output.

It makes more sense to make the output-dssdev connection in the output
driver. This is also in line with common display framework.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/dss/display.c |   12 ------------
 drivers/video/omap2/dss/dpi.c     |    9 +++++++++
 drivers/video/omap2/dss/dsi.c     |   10 ++++++++++
 drivers/video/omap2/dss/dss.h     |    1 -
 drivers/video/omap2/dss/hdmi.c    |    9 +++++++++
 drivers/video/omap2/dss/output.c  |   33 ---------------------------------
 drivers/video/omap2/dss/rfbi.c    |    9 +++++++++
 drivers/video/omap2/dss/sdi.c     |    9 +++++++++
 drivers/video/omap2/dss/venc.c    |    9 +++++++++
 9 files changed, 55 insertions(+), 46 deletions(-)

diff --git a/drivers/video/omap2/dss/display.c b/drivers/video/omap2/dss/display.c
index 008e9ee..05f21b6 100644
--- a/drivers/video/omap2/dss/display.c
+++ b/drivers/video/omap2/dss/display.c
@@ -79,17 +79,8 @@ EXPORT_SYMBOL(omapdss_default_get_timings);
 int dss_init_device(struct platform_device *pdev,
 		struct omap_dss_device *dssdev)
 {
-	struct omap_dss_output *out;
 	int r;
 
-	out = omapdss_get_output_from_dssdev(dssdev);
-
-	r = omapdss_output_set_device(out, dssdev);
-	if (r) {
-		DSSERR("failed to connect output to new device\n");
-		return r;
-	}
-
 	r = display_init_sysfs(pdev, dssdev);
 	if (r) {
 		omapdss_output_unset_device(dssdev->output);
@@ -103,9 +94,6 @@ void dss_uninit_device(struct platform_device *pdev,
 		struct omap_dss_device *dssdev)
 {
 	display_uninit_sysfs(pdev, dssdev);
-
-	if (dssdev->output)
-		omapdss_output_unset_device(dssdev->output);
 }
 
 static int dss_suspend_device(struct device *dev, void *data)
diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index c109fa6..d5bc47a 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -477,9 +477,18 @@ static void __init dpi_probe_pdata(struct platform_device *dpidev)
 		return;
 	}
 
+	r = omapdss_output_set_device(&dpi.output, dssdev);
+	if (r) {
+		DSSERR("failed to connect output to new device: %s\n",
+				dssdev->name);
+		dss_put_device(dssdev);
+		return;
+	}
+
 	r = dss_add_device(dssdev);
 	if (r) {
 		DSSERR("device %s register failed: %d\n", dssdev->name, r);
+		omapdss_output_unset_device(&dpi.output);
 		dss_put_device(dssdev);
 		return;
 	}
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index cf32dc7..db9663d 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -5146,6 +5146,7 @@ static struct omap_dss_device * __init dsi_find_dssdev(struct platform_device *p
 
 static void __init dsi_probe_pdata(struct platform_device *dsidev)
 {
+	struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
 	struct omap_dss_device *plat_dssdev;
 	struct omap_dss_device *dssdev;
 	int r;
@@ -5168,9 +5169,18 @@ static void __init dsi_probe_pdata(struct platform_device *dsidev)
 		return;
 	}
 
+	r = omapdss_output_set_device(&dsi->output, dssdev);
+	if (r) {
+		DSSERR("failed to connect output to new device: %s\n",
+				dssdev->name);
+		dss_put_device(dssdev);
+		return;
+	}
+
 	r = dss_add_device(dssdev);
 	if (r) {
 		DSSERR("device %s register failed: %d\n", dssdev->name, r);
+		omapdss_output_unset_device(&dsi->output);
 		dss_put_device(dssdev);
 		return;
 	}
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index 4d6f325..bdf8431 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -212,7 +212,6 @@ int dss_ovl_unset_manager(struct omap_overlay *ovl);
 /* output */
 void dss_register_output(struct omap_dss_output *out);
 void dss_unregister_output(struct omap_dss_output *out);
-struct omap_dss_output *omapdss_get_output_from_dssdev(struct omap_dss_device *dssdev);
 
 /* display */
 int dss_suspend_all_devices(void);
diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
index 24a2eef..769d082 100644
--- a/drivers/video/omap2/dss/hdmi.c
+++ b/drivers/video/omap2/dss/hdmi.c
@@ -1026,9 +1026,18 @@ static void __init hdmi_probe_pdata(struct platform_device *pdev)
 		return;
 	}
 
+	r = omapdss_output_set_device(&hdmi.output, dssdev);
+	if (r) {
+		DSSERR("failed to connect output to new device: %s\n",
+				dssdev->name);
+		dss_put_device(dssdev);
+		return;
+	}
+
 	r = dss_add_device(dssdev);
 	if (r) {
 		DSSERR("device %s register failed: %d\n", dssdev->name, r);
+		omapdss_output_unset_device(&hdmi.output);
 		hdmi_uninit_display(dssdev);
 		dss_put_device(dssdev);
 		return;
diff --git a/drivers/video/omap2/dss/output.c b/drivers/video/omap2/dss/output.c
index 813f266..1a84b79 100644
--- a/drivers/video/omap2/dss/output.c
+++ b/drivers/video/omap2/dss/output.c
@@ -113,36 +113,3 @@ struct omap_dss_output *omap_dss_get_output(enum omap_dss_output_id id)
 
 	return NULL;
 }
-
-struct omap_dss_output *omapdss_get_output_from_dssdev(struct omap_dss_device *dssdev)
-{
-	struct omap_dss_output *out = NULL;
-	enum omap_dss_output_id id;
-
-	switch (dssdev->type) {
-	case OMAP_DISPLAY_TYPE_DPI:
-		out = omap_dss_get_output(OMAP_DSS_OUTPUT_DPI);
-		break;
-	case OMAP_DISPLAY_TYPE_DBI:
-		out = omap_dss_get_output(OMAP_DSS_OUTPUT_DBI);
-		break;
-	case OMAP_DISPLAY_TYPE_SDI:
-		out = omap_dss_get_output(OMAP_DSS_OUTPUT_SDI);
-		break;
-	case OMAP_DISPLAY_TYPE_VENC:
-		out = omap_dss_get_output(OMAP_DSS_OUTPUT_VENC);
-		break;
-	case OMAP_DISPLAY_TYPE_HDMI:
-		out = omap_dss_get_output(OMAP_DSS_OUTPUT_HDMI);
-		break;
-	case OMAP_DISPLAY_TYPE_DSI:
-		id = dssdev->phy.dsi.module = 0 ? OMAP_DSS_OUTPUT_DSI1 :
-					OMAP_DSS_OUTPUT_DSI2;
-		out = omap_dss_get_output(id);
-		break;
-	default:
-		break;
-	}
-
-	return out;
-}
diff --git a/drivers/video/omap2/dss/rfbi.c b/drivers/video/omap2/dss/rfbi.c
index 7bfeb13..ec9fde5 100644
--- a/drivers/video/omap2/dss/rfbi.c
+++ b/drivers/video/omap2/dss/rfbi.c
@@ -999,9 +999,18 @@ static void __init rfbi_probe_pdata(struct platform_device *rfbidev)
 		return;
 	}
 
+	r = omapdss_output_set_device(&rfbi.output, dssdev);
+	if (r) {
+		DSSERR("failed to connect output to new device: %s\n",
+				dssdev->name);
+		dss_put_device(dssdev);
+		return;
+	}
+
 	r = dss_add_device(dssdev);
 	if (r) {
 		DSSERR("device %s register failed: %d\n", dssdev->name, r);
+		omapdss_output_unset_device(&rfbi.output);
 		dss_put_device(dssdev);
 		return;
 	}
diff --git a/drivers/video/omap2/dss/sdi.c b/drivers/video/omap2/dss/sdi.c
index 882ce89..62b5374 100644
--- a/drivers/video/omap2/dss/sdi.c
+++ b/drivers/video/omap2/dss/sdi.c
@@ -254,9 +254,18 @@ static void __init sdi_probe_pdata(struct platform_device *sdidev)
 		return;
 	}
 
+	r = omapdss_output_set_device(&sdi.output, dssdev);
+	if (r) {
+		DSSERR("failed to connect output to new device: %s\n",
+				dssdev->name);
+		dss_put_device(dssdev);
+		return;
+	}
+
 	r = dss_add_device(dssdev);
 	if (r) {
 		DSSERR("device %s register failed: %d\n", dssdev->name, r);
+		omapdss_output_unset_device(&sdi.output);
 		dss_put_device(dssdev);
 		return;
 	}
diff --git a/drivers/video/omap2/dss/venc.c b/drivers/video/omap2/dss/venc.c
index e8fddc9..006caf3 100644
--- a/drivers/video/omap2/dss/venc.c
+++ b/drivers/video/omap2/dss/venc.c
@@ -795,9 +795,18 @@ static void __init venc_probe_pdata(struct platform_device *vencdev)
 		return;
 	}
 
+	r = omapdss_output_set_device(&venc.output, dssdev);
+	if (r) {
+		DSSERR("failed to connect output to new device: %s\n",
+				dssdev->name);
+		dss_put_device(dssdev);
+		return;
+	}
+
 	r = dss_add_device(dssdev);
 	if (r) {
 		DSSERR("device %s register failed: %d\n", dssdev->name, r);
+		omapdss_output_unset_device(&venc.output);
 		dss_put_device(dssdev);
 		return;
 	}
-- 
1.7.10.4

[PATCH 5/5] OMAPFB: connect ovl managers to all dssdevs

[Tomi Valkeinen]

Commit 5d89bcc341771d95e3a2996218e5949a6627f59e (OMAPDSS: remove initial
display code from omapdss) moved setting up the initial overlay, overlay
manager, output and display connections from omapdss to omapfb.

However, currently omapfb only handles the connection related to the
default display, which means that no overlay managers are connected to
other displays.

This patch changes omapfb to go through all dssdevs, and connect an
overlay manager to them.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/omap2/omapfb/omapfb-main.c |   38 +++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/video/omap2/omapfb/omapfb-main.c b/drivers/video/omap2/omapfb/omapfb-main.c
index 1df973e..24739fc 100644
--- a/drivers/video/omap2/omapfb/omapfb-main.c
+++ b/drivers/video/omap2/omapfb/omapfb-main.c
@@ -2353,27 +2353,37 @@ static int omapfb_init_display(struct omapfb2_device *fbdev,
 }
 
 static int omapfb_init_connections(struct omapfb2_device *fbdev,
-		struct omap_dss_device *dssdev)
+		struct omap_dss_device *def_dssdev)
 {
 	int i, r;
-	struct omap_overlay_manager *mgr = NULL;
+	struct omap_overlay_manager *mgr;
 
-	for (i = 0; i < fbdev->num_managers; i++) {
-		mgr = fbdev->managers[i];
-
-		if (dssdev->channel = mgr->id)
-			break;
+	if (!def_dssdev->output) {
+		dev_err(fbdev->dev, "no output for the default display\n");
+		return -EINVAL;
 	}
 
-	if (i = fbdev->num_managers)
-		return -ENODEV;
+	for (i = 0; i < fbdev->num_displays; ++i) {
+		struct omap_dss_device *dssdev = fbdev->displays[i].dssdev;
+		struct omap_dss_output *out = dssdev->output;
 
-	if (mgr->output)
-		mgr->unset_output(mgr);
+		mgr = omap_dss_get_overlay_manager(dssdev->channel);
 
-	r = mgr->set_output(mgr, dssdev->output);
-	if (r)
-		return r;
+		if (!mgr || !out)
+			continue;
+
+		if (mgr->output)
+			mgr->unset_output(mgr);
+
+		mgr->set_output(mgr, out);
+	}
+
+	mgr = def_dssdev->output->manager;
+
+	if (!mgr) {
+		dev_err(fbdev->dev, "no ovl manager for the default display\n");
+		return -EINVAL;
+	}
 
 	for (i = 0; i < fbdev->num_overlays; i++) {
 		struct omap_overlay *ovl = fbdev->overlays[i];
-- 
1.7.10.4

Re: [PATCH 5/5] OMAPFB: connect ovl managers to all dssdevs

[Archit Taneja]

Hi,

On Friday 07 December 2012 05:25 PM, Tomi Valkeinen wrote:
> Commit 5d89bcc341771d95e3a2996218e5949a6627f59e (OMAPDSS: remove initial
> display code from omapdss) moved setting up the initial overlay, overlay
> manager, output and display connections from omapdss to omapfb.
>
> However, currently omapfb only handles the connection related to the
> default display, which means that no overlay managers are connected to
> other displays.
>
> This patch changes omapfb to go through all dssdevs, and connect an
> overlay manager to them.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>   drivers/video/omap2/omapfb/omapfb-main.c |   38 +++++++++++++++++++-----------
>   1 file changed, 24 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/video/omap2/omapfb/omapfb-main.c b/drivers/video/omap2/omapfb/omapfb-main.c
> index 1df973e..24739fc 100644
> --- a/drivers/video/omap2/omapfb/omapfb-main.c
> +++ b/drivers/video/omap2/omapfb/omapfb-main.c
> @@ -2353,27 +2353,37 @@ static int omapfb_init_display(struct omapfb2_device *fbdev,
>   }
>
>   static int omapfb_init_connections(struct omapfb2_device *fbdev,
> -		struct omap_dss_device *dssdev)
> +		struct omap_dss_device *def_dssdev)
>   {
>   	int i, r;
> -	struct omap_overlay_manager *mgr = NULL;
> +	struct omap_overlay_manager *mgr;
>
> -	for (i = 0; i < fbdev->num_managers; i++) {
> -		mgr = fbdev->managers[i];
> -
> -		if (dssdev->channel = mgr->id)
> -			break;
> +	if (!def_dssdev->output) {
> +		dev_err(fbdev->dev, "no output for the default display\n");
> +		return -EINVAL;
>   	}
>
> -	if (i = fbdev->num_managers)
> -		return -ENODEV;
> +	for (i = 0; i < fbdev->num_displays; ++i) {
> +		struct omap_dss_device *dssdev = fbdev->displays[i].dssdev;
> +		struct omap_dss_output *out = dssdev->output;
>
> -	if (mgr->output)
> -		mgr->unset_output(mgr);
> +		mgr = omap_dss_get_overlay_manager(dssdev->channel);

This dssdev->channel reference is something we would want to get rid of 
eventually, right?

At the point omapfb_init_connections() is called, we would have all the 
omap_dss_devices registered, right? So at this point, omapfb will have 
an overall view of how the panels need to be connected to DSS.

I think we can try to find a manager here for dssdev rather than using 
dssdev->channel directly. The dssdev's output could connect to a few 
managers. We would want to chose managers for each dssdev output in such 
a way that all outputs have a manager. I guess there would be multiple 
combinations for this, but it would be okay to pick any one of them.

I think we would need some recursive or backtracking sort of approach to 
get a desired combination. We can figure about how to make it work 
later, but do you agree if this is a right way to get rid of 
dssdev->channel?

Also, we would need to do this for omapdrm separately using it's own 
encoder/connector entities.

Archit

>
> -	r = mgr->set_output(mgr, dssdev->output);
> -	if (r)
> -		return r;
> +		if (!mgr || !out)
> +			continue;
> +
> +		if (mgr->output)
> +			mgr->unset_output(mgr);
> +
> +		mgr->set_output(mgr, out);
> +	}
> +
> +	mgr = def_dssdev->output->manager;
> +
> +	if (!mgr) {
> +		dev_err(fbdev->dev, "no ovl manager for the default display\n");
> +		return -EINVAL;
> +	}
>
>   	for (i = 0; i < fbdev->num_overlays; i++) {
>   		struct omap_overlay *ovl = fbdev->overlays[i];
>

Re: [PATCH 5/5] OMAPFB: connect ovl managers to all dssdevs

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 5584 bytes --]

On 2012-12-10 09:34, Archit Taneja wrote:
> Hi,
> 
> On Friday 07 December 2012 05:25 PM, Tomi Valkeinen wrote:
>> Commit 5d89bcc341771d95e3a2996218e5949a6627f59e (OMAPDSS: remove initial
>> display code from omapdss) moved setting up the initial overlay, overlay
>> manager, output and display connections from omapdss to omapfb.
>>
>> However, currently omapfb only handles the connection related to the
>> default display, which means that no overlay managers are connected to
>> other displays.
>>
>> This patch changes omapfb to go through all dssdevs, and connect an
>> overlay manager to them.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>   drivers/video/omap2/omapfb/omapfb-main.c |   38
>> +++++++++++++++++++-----------
>>   1 file changed, 24 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/video/omap2/omapfb/omapfb-main.c
>> b/drivers/video/omap2/omapfb/omapfb-main.c
>> index 1df973e..24739fc 100644
>> --- a/drivers/video/omap2/omapfb/omapfb-main.c
>> +++ b/drivers/video/omap2/omapfb/omapfb-main.c
>> @@ -2353,27 +2353,37 @@ static int omapfb_init_display(struct
>> omapfb2_device *fbdev,
>>   }
>>
>>   static int omapfb_init_connections(struct omapfb2_device *fbdev,
>> -        struct omap_dss_device *dssdev)
>> +        struct omap_dss_device *def_dssdev)
>>   {
>>       int i, r;
>> -    struct omap_overlay_manager *mgr = NULL;
>> +    struct omap_overlay_manager *mgr;
>>
>> -    for (i = 0; i < fbdev->num_managers; i++) {
>> -        mgr = fbdev->managers[i];
>> -
>> -        if (dssdev->channel == mgr->id)
>> -            break;
>> +    if (!def_dssdev->output) {
>> +        dev_err(fbdev->dev, "no output for the default display\n");
>> +        return -EINVAL;
>>       }
>>
>> -    if (i == fbdev->num_managers)
>> -        return -ENODEV;
>> +    for (i = 0; i < fbdev->num_displays; ++i) {
>> +        struct omap_dss_device *dssdev = fbdev->displays[i].dssdev;
>> +        struct omap_dss_output *out = dssdev->output;
>>
>> -    if (mgr->output)
>> -        mgr->unset_output(mgr);
>> +        mgr = omap_dss_get_overlay_manager(dssdev->channel);
> 
> This dssdev->channel reference is something we would want to get rid of
> eventually, right?

Yes.

> At the point omapfb_init_connections() is called, we would have all the
> omap_dss_devices registered, right? So at this point, omapfb will have
> an overall view of how the panels need to be connected to DSS.
> 
> I think we can try to find a manager here for dssdev rather than using
> dssdev->channel directly. The dssdev's output could connect to a few
> managers. We would want to chose managers for each dssdev output in such
> a way that all outputs have a manager. I guess there would be multiple
> combinations for this, but it would be okay to pick any one of them.
> 
> I think we would need some recursive or backtracking sort of approach to
> get a desired combination. We can figure about how to make it work
> later, but do you agree if this is a right way to get rid of
> dssdev->channel?

Yes, I think that's a sensible approach. The thing I don't like about it
is that it requires omapfb/omapdrm to create the mgr-output connections.
They shouldn't really be interested in that. All they want is a display
device that works, and a way to get the mgr used for the display.

Well, I think we can hide the implementation inside omapdss, so that
omapfb/omapdrm will just need to call one omapdss func when they are
started, which will connect the mgrs to outputs.

A downside with setting up the mgr-output links at one go, when
omapfb/omapdrm starts, is that it creates a strict load order
restriction between omapdss, panels and omapfb/omapdrm. The drivers will
need to be loaded in that order, or things won't work.

Another option would be to pass information about mgr-output links from
the board files (or DT data) to the omapdss driver, so that omapdss
could setup them at probe time. With this option omapfb/omapdrm doesn't
need to care about this, and it doesn't create load order restriction.
But mgr-output links are something that I'd really like to handle inside
the drivers, not something that needs to be passed via platform data.

Third option, which is the best, but also something I have no idea how
to implement, would be to create the mgr-output links dynamically when
needed. The problem here is, of course, that a mgr could be allocated to
an output, only to be later found out that that particular mgr is needed
for another output.

But this is something we could study a bit: can we create such mgr
allocation system, that no matter what outputs the board uses, it'll
just work.

So, for example, on omap4, LCD2 mgr can be used for DPI, DSI2, DSI1, and
RFBI. LCD1 can be used for RFBI and DSI1. If we know that DSI1 and RFBI
cannot be used at the same time, we're free to give LCD1 to either one.
And if we know that DPI and DSI2 cannot be used at the same time, we're
also free to give LCD2 to either one. And if that's the case, there are
no conflicts.

I don't know if we can find such allocation for all current omaps, and I
know it's slightly risky as the next omap could have limitations even if
current ones do not.

> Also, we would need to do this for omapdrm separately using it's own
> encoder/connector entities.

Yep. That's also a negative side: both omapfb/omapdrm will need to
implement the same stuff, even if neither of them are really interested
in that stuff.

 Tomi



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

Re: [PATCH 5/5] OMAPFB: connect ovl managers to all dssdevs

[Archit Taneja]

On Monday 10 December 2012 01:33 PM, Tomi Valkeinen wrote:
> On 2012-12-10 09:34, Archit Taneja wrote:
>> Hi,
>>
>> On Friday 07 December 2012 05:25 PM, Tomi Valkeinen wrote:
>>> Commit 5d89bcc341771d95e3a2996218e5949a6627f59e (OMAPDSS: remove initial
>>> display code from omapdss) moved setting up the initial overlay, overlay
>>> manager, output and display connections from omapdss to omapfb.
>>>
>>> However, currently omapfb only handles the connection related to the
>>> default display, which means that no overlay managers are connected to
>>> other displays.
>>>
>>> This patch changes omapfb to go through all dssdevs, and connect an
>>> overlay manager to them.
>>>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>> ---
>>>    drivers/video/omap2/omapfb/omapfb-main.c |   38
>>> +++++++++++++++++++-----------
>>>    1 file changed, 24 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/video/omap2/omapfb/omapfb-main.c
>>> b/drivers/video/omap2/omapfb/omapfb-main.c
>>> index 1df973e..24739fc 100644
>>> --- a/drivers/video/omap2/omapfb/omapfb-main.c
>>> +++ b/drivers/video/omap2/omapfb/omapfb-main.c
>>> @@ -2353,27 +2353,37 @@ static int omapfb_init_display(struct
>>> omapfb2_device *fbdev,
>>>    }
>>>
>>>    static int omapfb_init_connections(struct omapfb2_device *fbdev,
>>> -        struct omap_dss_device *dssdev)
>>> +        struct omap_dss_device *def_dssdev)
>>>    {
>>>        int i, r;
>>> -    struct omap_overlay_manager *mgr = NULL;
>>> +    struct omap_overlay_manager *mgr;
>>>
>>> -    for (i = 0; i < fbdev->num_managers; i++) {
>>> -        mgr = fbdev->managers[i];
>>> -
>>> -        if (dssdev->channel = mgr->id)
>>> -            break;
>>> +    if (!def_dssdev->output) {
>>> +        dev_err(fbdev->dev, "no output for the default display\n");
>>> +        return -EINVAL;
>>>        }
>>>
>>> -    if (i = fbdev->num_managers)
>>> -        return -ENODEV;
>>> +    for (i = 0; i < fbdev->num_displays; ++i) {
>>> +        struct omap_dss_device *dssdev = fbdev->displays[i].dssdev;
>>> +        struct omap_dss_output *out = dssdev->output;
>>>
>>> -    if (mgr->output)
>>> -        mgr->unset_output(mgr);
>>> +        mgr = omap_dss_get_overlay_manager(dssdev->channel);
>>
>> This dssdev->channel reference is something we would want to get rid of
>> eventually, right?
>
> Yes.
>
>> At the point omapfb_init_connections() is called, we would have all the
>> omap_dss_devices registered, right? So at this point, omapfb will have
>> an overall view of how the panels need to be connected to DSS.
>>
>> I think we can try to find a manager here for dssdev rather than using
>> dssdev->channel directly. The dssdev's output could connect to a few
>> managers. We would want to chose managers for each dssdev output in such
>> a way that all outputs have a manager. I guess there would be multiple
>> combinations for this, but it would be okay to pick any one of them.
>>
>> I think we would need some recursive or backtracking sort of approach to
>> get a desired combination. We can figure about how to make it work
>> later, but do you agree if this is a right way to get rid of
>> dssdev->channel?
>
> Yes, I think that's a sensible approach. The thing I don't like about it
> is that it requires omapfb/omapdrm to create the mgr-output connections.
> They shouldn't really be interested in that. All they want is a display
> device that works, and a way to get the mgr used for the display.
>
> Well, I think we can hide the implementation inside omapdss, so that
> omapfb/omapdrm will just need to call one omapdss func when they are
> started, which will connect the mgrs to outputs.
>
> A downside with setting up the mgr-output links at one go, when
> omapfb/omapdrm starts, is that it creates a strict load order
> restriction between omapdss, panels and omapfb/omapdrm. The drivers will
> need to be loaded in that order, or things won't work.

Yes, that's true.

>
> Another option would be to pass information about mgr-output links from
> the board files (or DT data) to the omapdss driver, so that omapdss
> could setup them at probe time. With this option omapfb/omapdrm doesn't
> need to care about this, and it doesn't create load order restriction.
> But mgr-output links are something that I'd really like to handle inside
> the drivers, not something that needs to be passed via platform data.

This would definitely make things simpler, but if this parameter is put 
in a panel's DT, it would become omap specific. We could add this info 
to the DT corresponding to omapdss.

>
> Third option, which is the best, but also something I have no idea how
> to implement, would be to create the mgr-output links dynamically when
> needed. The problem here is, of course, that a mgr could be allocated to
> an output, only to be later found out that that particular mgr is needed
> for another output.
>
> But this is something we could study a bit: can we create such mgr
> allocation system, that no matter what outputs the board uses, it'll
> just work.

Yes, that would be quite useful. But I think we'll hit situations where 
it is sort of impossible to prevent the above situation.

When an output needs a manager. We could study the current state of the 
system by splitting managers into 2 sets:

A: managers which already have outputs connected to them
B: managers which don't have an output, but might get connected to one 
in the future.

managers in A are lost, and we can't detach them, we would need to at 
least disable/reenable the panel with a new manager connected to the output.

we need to find one from B such that maximum outputs(or some other 
weightage factor) will still be supported after this new link is made.

The system will initially have all managers in B, but eventually 
managers will move to A. We need to move one manager from B to A for 
every mgr-output link.

I guess I just described the problem in a more mathematical way, without 
providing any solution :), but it does look like an optimisation problem.

>
> So, for example, on omap4, LCD2 mgr can be used for DPI, DSI2, DSI1, and
> RFBI. LCD1 can be used for RFBI and DSI1. If we know that DSI1 and RFBI
> cannot be used at the same time, we're free to give LCD1 to either one.

But they can be used together, can't they? LCD1->DSI1 and LCD2->RFBI. 
Are you creating this constraint by assuming what the board is like? Or 
is this a constraint of OMAP DSS HW?


> And if we know that DPI and DSI2 cannot be used at the same time, we're
> also free to give LCD2 to either one. And if that's the case, there are
> no conflicts.

This is also possible at the same time: TV->DPI and LCD2->DSI2

>
> I don't know if we can find such allocation for all current omaps, and I
> know it's slightly risky as the next omap could have limitations even if
> current ones do not.

I don't understand the example so well, but I get your point of taking 
advantage of such limitations.

>
>> Also, we would need to do this for omapdrm separately using it's own
>> encoder/connector entities.
>
> Yep. That's also a negative side: both omapfb/omapdrm will need to
> implement the same stuff, even if neither of them are really interested
> in that stuff.

Yes. I wonder if crtcs, encoders and connectors already have some sort 
of helpers for this?

Archit

Re: [PATCH 5/5] OMAPFB: connect ovl managers to all dssdevs

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 4743 bytes --]

On 2012-12-10 11:54, Archit Taneja wrote:
> On Monday 10 December 2012 01:33 PM, Tomi Valkeinen wrote:

>> Another option would be to pass information about mgr-output links from
>> the board files (or DT data) to the omapdss driver, so that omapdss
>> could setup them at probe time. With this option omapfb/omapdrm doesn't
>> need to care about this, and it doesn't create load order restriction.
>> But mgr-output links are something that I'd really like to handle inside
>> the drivers, not something that needs to be passed via platform data.
> 
> This would definitely make things simpler, but if this parameter is put
> in a panel's DT, it would become omap specific. We could add this info
> to the DT corresponding to omapdss.

Yes, I meant it should be omapdss platform data. Nothing related to panels.

>> Third option, which is the best, but also something I have no idea how
>> to implement, would be to create the mgr-output links dynamically when
>> needed. The problem here is, of course, that a mgr could be allocated to
>> an output, only to be later found out that that particular mgr is needed
>> for another output.
>>
>> But this is something we could study a bit: can we create such mgr
>> allocation system, that no matter what outputs the board uses, it'll
>> just work.
> 
> Yes, that would be quite useful. But I think we'll hit situations where
> it is sort of impossible to prevent the above situation.
> 
> When an output needs a manager. We could study the current state of the
> system by splitting managers into 2 sets:
> 
> A: managers which already have outputs connected to them
> B: managers which don't have an output, but might get connected to one
> in the future.
> 
> managers in A are lost, and we can't detach them, we would need to at
> least disable/reenable the panel with a new manager connected to the
> output.
> 
> we need to find one from B such that maximum outputs(or some other
> weightage factor) will still be supported after this new link is made.
> 
> The system will initially have all managers in B, but eventually
> managers will move to A. We need to move one manager from B to A for
> every mgr-output link.
> 
> I guess I just described the problem in a more mathematical way, without
> providing any solution :), but it does look like an optimisation problem.

Well, optimization problem sounds like something that can always be
solved. But in this case the driver may need to predict what outputs
will be used, which is of course impossible.

>> So, for example, on omap4, LCD2 mgr can be used for DPI, DSI2, DSI1, and
>> RFBI. LCD1 can be used for RFBI and DSI1. If we know that DSI1 and RFBI
>> cannot be used at the same time, we're free to give LCD1 to either one.
> 
> But they can be used together, can't they? LCD1->DSI1 and LCD2->RFBI.
> Are you creating this constraint by assuming what the board is like? Or
> is this a constraint of OMAP DSS HW?

I didn't check if they can be used together or not, I was just guessing.
At least on OMAP3 DSI and RFBI shared the same pins, so they could not
be used at the same time.

Perhaps we should implement a mixed approach: the driver presumes
certain things, like "if DSI is used, RFBI is not used", based on the
knowledge of what kind of boards there currently are. This would allow
us to manage the mgr->output connections in the driver for, say, 95% of
the cases. Then we'd also have the platform data parameters for omapdss,
which could be used in the weird cases.

>> And if we know that DPI and DSI2 cannot be used at the same time, we're
>> also free to give LCD2 to either one. And if that's the case, there are
>> no conflicts.
> 
> This is also possible at the same time: TV->DPI and LCD2->DSI2

True. I was just again guessing. On OMAP3 DPI and DSI shared the same pins.

Thinking about this, OMAP4 does have separate pins for DSI, doesn't it?
So my guesses don't hold.

>> I don't know if we can find such allocation for all current omaps, and I
>> know it's slightly risky as the next omap could have limitations even if
>> current ones do not.
> 
> I don't understand the example so well, but I get your point of taking
> advantage of such limitations.
> 
>>
>>> Also, we would need to do this for omapdrm separately using it's own
>>> encoder/connector entities.
>>
>> Yep. That's also a negative side: both omapfb/omapdrm will need to
>> implement the same stuff, even if neither of them are really interested
>> in that stuff.
> 
> Yes. I wonder if crtcs, encoders and connectors already have some sort
> of helpers for this?

Probably nothing that helps us, as this is OMAP HW restriction.

 Tomi



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

Re: [PATCH 5/5] OMAPFB: connect ovl managers to all dssdevs

[Archit Taneja]

On Monday 10 December 2012 03:37 PM, Tomi Valkeinen wrote:
> On 2012-12-10 11:54, Archit Taneja wrote:
>> On Monday 10 December 2012 01:33 PM, Tomi Valkeinen wrote:
>
>>> Another option would be to pass information about mgr-output links from
>>> the board files (or DT data) to the omapdss driver, so that omapdss
>>> could setup them at probe time. With this option omapfb/omapdrm doesn't
>>> need to care about this, and it doesn't create load order restriction.
>>> But mgr-output links are something that I'd really like to handle inside
>>> the drivers, not something that needs to be passed via platform data.
>>
>> This would definitely make things simpler, but if this parameter is put
>> in a panel's DT, it would become omap specific. We could add this info
>> to the DT corresponding to omapdss.
>
> Yes, I meant it should be omapdss platform data. Nothing related to panels.

I think this is the easiest way out. We can have one parameter per 
output in DT. If we do come up with a way to implement the 3rd option 
below, we can always ignore those DT fields(assuming our implementation 
to give the same result). So in this way, we would just be deprecating a 
DT field in the future, and calculating it ourselves.

>
>>> Third option, which is the best, but also something I have no idea how
>>> to implement, would be to create the mgr-output links dynamically when
>>> needed. The problem here is, of course, that a mgr could be allocated to
>>> an output, only to be later found out that that particular mgr is needed
>>> for another output.
>>>
>>> But this is something we could study a bit: can we create such mgr
>>> allocation system, that no matter what outputs the board uses, it'll
>>> just work.
>>
>> Yes, that would be quite useful. But I think we'll hit situations where
>> it is sort of impossible to prevent the above situation.
>>
>> When an output needs a manager. We could study the current state of the
>> system by splitting managers into 2 sets:
>>
>> A: managers which already have outputs connected to them
>> B: managers which don't have an output, but might get connected to one
>> in the future.
>>
>> managers in A are lost, and we can't detach them, we would need to at
>> least disable/reenable the panel with a new manager connected to the
>> output.
>>
>> we need to find one from B such that maximum outputs(or some other
>> weightage factor) will still be supported after this new link is made.
>>
>> The system will initially have all managers in B, but eventually
>> managers will move to A. We need to move one manager from B to A for
>> every mgr-output link.
>>
>> I guess I just described the problem in a more mathematical way, without
>> providing any solution :), but it does look like an optimisation problem.
>
> Well, optimization problem sounds like something that can always be
> solved. But in this case the driver may need to predict what outputs
> will be used, which is of course impossible.
>
>>> So, for example, on omap4, LCD2 mgr can be used for DPI, DSI2, DSI1, and
>>> RFBI. LCD1 can be used for RFBI and DSI1. If we know that DSI1 and RFBI
>>> cannot be used at the same time, we're free to give LCD1 to either one.
>>
>> But they can be used together, can't they? LCD1->DSI1 and LCD2->RFBI.
>> Are you creating this constraint by assuming what the board is like? Or
>> is this a constraint of OMAP DSS HW?
>
> I didn't check if they can be used together or not, I was just guessing.
> At least on OMAP3 DSI and RFBI shared the same pins, so they could not
> be used at the same time.
>
> Perhaps we should implement a mixed approach: the driver presumes
> certain things, like "if DSI is used, RFBI is not used", based on the
> knowledge of what kind of boards there currently are. This would allow
> us to manage the mgr->output connections in the driver for, say, 95% of
> the cases. Then we'd also have the platform data parameters for omapdss,
> which could be used in the weird cases.

Let's just have platform data parameters :)

>
>>> And if we know that DPI and DSI2 cannot be used at the same time, we're
>>> also free to give LCD2 to either one. And if that's the case, there are
>>> no conflicts.
>>
>> This is also possible at the same time: TV->DPI and LCD2->DSI2
>
> True. I was just again guessing. On OMAP3 DPI and DSI shared the same pins.
>
> Thinking about this, OMAP4 does have separate pins for DSI, doesn't it?
> So my guesses don't hold.
>
>>> I don't know if we can find such allocation for all current omaps, and I
>>> know it's slightly risky as the next omap could have limitations even if
>>> current ones do not.
>>
>> I don't understand the example so well, but I get your point of taking
>> advantage of such limitations.
>>
>>>
>>>> Also, we would need to do this for omapdrm separately using it's own
>>>> encoder/connector entities.
>>>
>>> Yep. That's also a negative side: both omapfb/omapdrm will need to
>>> implement the same stuff, even if neither of them are really interested
>>> in that stuff.
>>
>> Yes. I wonder if crtcs, encoders and connectors already have some sort
>> of helpers for this?
>
> Probably nothing that helps us, as this is OMAP HW restriction.

If we do use DT/platform data, would we need to parse it in omapdrm to 
establish drm entities? Or do we rely on omapdss to parse the DT data 
and give the links to omapdrm?

Archit

Re: [PATCH 5/5] OMAPFB: connect ovl managers to all dssdevs

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2485 bytes --]

On 2012-12-10 12:53, Archit Taneja wrote:
> On Monday 10 December 2012 03:37 PM, Tomi Valkeinen wrote:
>> On 2012-12-10 11:54, Archit Taneja wrote:
>>> On Monday 10 December 2012 01:33 PM, Tomi Valkeinen wrote:
>>
>>>> Another option would be to pass information about mgr-output links from
>>>> the board files (or DT data) to the omapdss driver, so that omapdss
>>>> could setup them at probe time. With this option omapfb/omapdrm doesn't
>>>> need to care about this, and it doesn't create load order restriction.
>>>> But mgr-output links are something that I'd really like to handle
>>>> inside
>>>> the drivers, not something that needs to be passed via platform data.
>>>
>>> This would definitely make things simpler, but if this parameter is put
>>> in a panel's DT, it would become omap specific. We could add this info
>>> to the DT corresponding to omapdss.
>>
>> Yes, I meant it should be omapdss platform data. Nothing related to
>> panels.
> 
> I think this is the easiest way out. We can have one parameter per
> output in DT. If we do come up with a way to implement the 3rd option
> below, we can always ignore those DT fields(assuming our implementation
> to give the same result). So in this way, we would just be deprecating a
> DT field in the future, and calculating it ourselves.

I would rather go the other way around: calculate it ourselves
(presuming it can be done for the current boards), and add the DT field
later if we come up with boards that won't work with the dynamic approach.

The reasons I'm not too happy with having the parameter in the DT data
is that:

- DT should be about describing the hardware connections between
components, but in this case it's dynamically configurable connection.

- We need to have the DT parameter for all cases, even if in 95% of
cases it's not really needed.

- We may never need the parameter, if we never get boards that require
funny setup.

> If we do use DT/platform data, would we need to parse it in omapdrm to
> establish drm entities? Or do we rely on omapdss to parse the DT data
> and give the links to omapdrm?

I think we should parse it in omapdss and setup the connections at
omapdss's probe. Then when omapfb/omapdrm starts, they can get
information about the connections from omapdss, and setup their entities
as they want.

So omapdss would setup the mgr->output->panel links, and they would be
ready for omapfb/omapdrm to use.

 Tomi



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