[PATCH v3 1/4] DSS2: OMAPFB: Add support for switching memory regions

[PATCH v3 1/4] DSS2: OMAPFB: Add support for switching memory regions

[ville.syrjala]

From: Ville Syrj채l채 <ville.syrjala@nokia.com>

Separate the memory region from the framebuffer device a little bit.
It's now possible to select the memory region used by the framebuffer
device using the new mem_idx parameter of omapfb_plane_info. If the
mem_idx is specified it will be interpreted as an index into the
memory regions array, if it's not specified the framebuffer's index is
used instead. So by default each framebuffer keeps using it's own
memory region which preserves backwards compatibility.

This allows cloning the same memory region to several overlays and yet
each overlay can be controlled independently since they can be
associated with separate framebuffer devices.

Signed-off-by: Ville Syrj채l채 <ville.syrjala@nokia.com>
---
Changes since v2:
* Removed the use_count and rely on just counting all active overlays. A bit racy
  but no chance of getting stuck in a state where memory allocation can't be changed.
* s/source_idx/mem_idx as that seems to be a little more self explanatory

 drivers/video/omap2/omapfb/omapfb-ioctl.c |  163 ++++++++++++++++++++-----
 drivers/video/omap2/omapfb/omapfb-main.c  |  184 +++++++++++++++++++----------
 drivers/video/omap2/omapfb/omapfb-sysfs.c |   60 ++++++++--
 drivers/video/omap2/omapfb/omapfb.h       |   38 ++++++-
 include/linux/omapfb.h                    |    5 +-
 5 files changed, 339 insertions(+), 111 deletions(-)

diff --git a/drivers/video/omap2/omapfb/omapfb-ioctl.c b/drivers/video/omap2/omapfb/omapfb-ioctl.c
index 1ffa760..cd00bdc 100644
--- a/drivers/video/omap2/omapfb/omapfb-ioctl.c
+++ b/drivers/video/omap2/omapfb/omapfb-ioctl.c
@@ -34,12 +34,37 @@
 
 #include "omapfb.h"
 
+static u8 get_mem_idx(struct omapfb_info *ofbi)
+{
+	if (ofbi->id = ofbi->region->id)
+		return 0;
+
+	return OMAPFB_MEM_IDX_ENABLED | ofbi->region->id;
+}
+
+static struct omapfb2_mem_region *get_mem_region(struct omapfb_info *ofbi,
+						 u8 mem_idx)
+{
+	struct omapfb2_device *fbdev = ofbi->fbdev;
+
+	if (mem_idx & OMAPFB_MEM_IDX_ENABLED)
+		mem_idx &= OMAPFB_MEM_IDX_MASK;
+	else
+		mem_idx = ofbi->id;
+
+	if (mem_idx >= fbdev->num_fbs)
+		return NULL;
+
+	return omapfb_get_mem_region(&fbdev->regions[mem_idx]);
+}
+
 static int omapfb_setup_plane(struct fb_info *fbi, struct omapfb_plane_info *pi)
 {
 	struct omapfb_info *ofbi = FB2OFB(fbi);
 	struct omapfb2_device *fbdev = ofbi->fbdev;
 	struct omap_overlay *ovl;
-	struct omap_overlay_info info;
+	struct omap_overlay_info old_info;
+	struct omapfb2_mem_region *old_rg, *new_rg;
 	int r = 0;
 
 	DBG("omapfb_setup_plane\n");
@@ -52,57 +77,101 @@ static int omapfb_setup_plane(struct fb_info *fbi, struct omapfb_plane_info *pi)
 	/* XXX uses only the first overlay */
 	ovl = ofbi->overlays[0];
 
-	if (pi->enabled && !ofbi->region.size) {
+	old_rg = omapfb_get_mem_region(ofbi->region);
+	new_rg = get_mem_region(ofbi, pi->mem_idx);
+	if (!new_rg) {
+		r = -EINVAL;
+		goto put_old;
+	}
+
+	if (pi->enabled && !new_rg->size) {
 		/*
 		 * This plane's memory was freed, can't enable it
 		 * until it's reallocated.
 		 */
 		r = -EINVAL;
-		goto out;
+		goto put_new;
 	}
 
-	ovl->get_overlay_info(ovl, &info);
+	ovl->get_overlay_info(ovl, &old_info);
 
-	info.pos_x = pi->pos_x;
-	info.pos_y = pi->pos_y;
-	info.out_width = pi->out_width;
-	info.out_height = pi->out_height;
-	info.enabled = pi->enabled;
+	if (old_rg != new_rg) {
+		ofbi->region = new_rg;
+		set_fb_fix(fbi);
+	}
 
-	r = ovl->set_overlay_info(ovl, &info);
-	if (r)
-		goto out;
+	if (pi->enabled) {
+		struct omap_overlay_info info;
 
-	if (ovl->manager) {
-		r = ovl->manager->apply(ovl->manager);
+		r = omapfb_setup_overlay(fbi, ovl, pi->pos_x, pi->pos_y,
+			pi->out_width, pi->out_height);
 		if (r)
-			goto out;
+			goto undo;
+
+		ovl->get_overlay_info(ovl, &info);
+
+		if (!info.enabled) {
+			info.enabled = pi->enabled;
+			r = ovl->set_overlay_info(ovl, &info);
+			if (r)
+				goto undo;
+		}
+	} else {
+		struct omap_overlay_info info;
+
+		ovl->get_overlay_info(ovl, &info);
+
+		info.enabled = pi->enabled;
+		info.pos_x = pi->pos_x;
+		info.pos_y = pi->pos_y;
+		info.out_width = pi->out_width;
+		info.out_height = pi->out_height;
+
+		r = ovl->set_overlay_info(ovl, &info);
+		if (r)
+			goto undo;
 	}
 
-out:
-	if (r)
-		dev_err(fbdev->dev, "setup_plane failed\n");
+	if (ovl->manager)
+		ovl->manager->apply(ovl->manager);
+
+	omapfb_put_mem_region(new_rg);
+	omapfb_put_mem_region(old_rg);
+
+	return 0;
+
+ undo:
+	if (old_rg != new_rg) {
+		ofbi->region = old_rg;
+		set_fb_fix(fbi);
+	}
+
+	ovl->set_overlay_info(ovl, &old_info);
+ put_new:
+	omapfb_put_mem_region(new_rg);
+ put_old:
+	omapfb_put_mem_region(old_rg);
+ out:
+	dev_err(fbdev->dev, "setup_plane failed\n");
+
 	return r;
 }
 
 static int omapfb_query_plane(struct fb_info *fbi, struct omapfb_plane_info *pi)
 {
 	struct omapfb_info *ofbi = FB2OFB(fbi);
+	struct omap_overlay *ovl = ofbi->overlays[0];
+	struct omap_overlay_info *ovli = &ovl->info;
 
 	if (ofbi->num_overlays != 1) {
 		memset(pi, 0, sizeof(*pi));
 	} else {
-		struct omap_overlay_info *ovli;
-		struct omap_overlay *ovl;
-
-		ovl = ofbi->overlays[0];
-		ovli = &ovl->info;
-
 		pi->pos_x = ovli->pos_x;
 		pi->pos_y = ovli->pos_y;
 		pi->enabled = ovli->enabled;
 		pi->channel_out = 0; /* xxx */
 		pi->mirror = 0;
+		pi->mem_idx = get_mem_idx(ofbi);
 		pi->out_width = ovli->out_width;
 		pi->out_height = ovli->out_height;
 	}
@@ -115,30 +184,57 @@ static int omapfb_setup_mem(struct fb_info *fbi, struct omapfb_mem_info *mi)
 	struct omapfb_info *ofbi = FB2OFB(fbi);
 	struct omapfb2_device *fbdev = ofbi->fbdev;
 	struct omapfb2_mem_region *rg;
-	int r, i;
+	int r = 0;
 	size_t size;
+	int i;
 
 	if (mi->type > OMAPFB_MEMTYPE_MAX)
 		return -EINVAL;
 
 	size = PAGE_ALIGN(mi->size);
 
-	rg = &ofbi->region;
+	rg = ofbi->region;
 
-	for (i = 0; i < ofbi->num_overlays; i++) {
-		if (ofbi->overlays[i]->info.enabled)
-			return -EBUSY;
+	/* FIXME probably should be a rwsem ... */
+	mutex_lock(&rg->mtx);
+	while (rg->ref) {
+		mutex_unlock(&rg->mtx);
+		schedule();
+		mutex_lock(&rg->mtx);
+	}
+
+	if (atomic_read(&rg->map_count)) {
+		r = -EBUSY;
+		goto out;
+	}
+
+	for (i = 0; i < fbdev->num_fbs; i++) {
+		struct omapfb_info *ofbi2 = FB2OFB(fbdev->fbs[i]);
+		int j;
+
+		if (ofbi2->region != rg)
+			continue;
+
+		for (j = 0; j < ofbi2->num_overlays; j++) {
+			if (ofbi2->overlays[j]->info.enabled) {
+				r = -EBUSY;
+				goto out;
+			}
+		}
 	}
 
 	if (rg->size != size || rg->type != mi->type) {
 		r = omapfb_realloc_fbmem(fbi, size, mi->type);
 		if (r) {
 			dev_err(fbdev->dev, "realloc fbmem failed\n");
-			return r;
+			goto out;
 		}
 	}
 
-	return 0;
+ out:
+	mutex_unlock(&rg->mtx);
+
+	return r;
 }
 
 static int omapfb_query_mem(struct fb_info *fbi, struct omapfb_mem_info *mi)
@@ -146,12 +242,15 @@ 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 = &ofbi->region;
 	memset(mi, 0, sizeof(*mi));
 
+	rg = omapfb_get_mem_region(ofbi->region);
+
 	mi->size = rg->size;
 	mi->type = rg->type;
 
+	omapfb_put_mem_region(rg);
+
 	return 0;
 }
 
diff --git a/drivers/video/omap2/omapfb/omapfb-main.c b/drivers/video/omap2/omapfb/omapfb-main.c
index 4a76917..dcd9d49 100644
--- a/drivers/video/omap2/omapfb/omapfb-main.c
+++ b/drivers/video/omap2/omapfb/omapfb-main.c
@@ -156,7 +156,7 @@ static void fill_fb(struct fb_info *fbi)
 
 static unsigned omapfb_get_vrfb_offset(const struct omapfb_info *ofbi, int rot)
 {
-	const struct vrfb *vrfb = &ofbi->region.vrfb;
+	const struct vrfb *vrfb = &ofbi->region->vrfb;
 	unsigned offset;
 
 	switch (rot) {
@@ -184,27 +184,27 @@ static unsigned omapfb_get_vrfb_offset(const struct omapfb_info *ofbi, int rot)
 static u32 omapfb_get_region_rot_paddr(const struct omapfb_info *ofbi, int rot)
 {
 	if (ofbi->rotation_type = OMAP_DSS_ROT_VRFB) {
-		return ofbi->region.vrfb.paddr[rot]
+		return ofbi->region->vrfb.paddr[rot]
 			+ omapfb_get_vrfb_offset(ofbi, rot);
 	} else {
-		return ofbi->region.paddr;
+		return ofbi->region->paddr;
 	}
 }
 
 static u32 omapfb_get_region_paddr(const struct omapfb_info *ofbi)
 {
 	if (ofbi->rotation_type = OMAP_DSS_ROT_VRFB)
-		return ofbi->region.vrfb.paddr[0];
+		return ofbi->region->vrfb.paddr[0];
 	else
-		return ofbi->region.paddr;
+		return ofbi->region->paddr;
 }
 
 static void __iomem *omapfb_get_region_vaddr(const struct omapfb_info *ofbi)
 {
 	if (ofbi->rotation_type = OMAP_DSS_ROT_VRFB)
-		return ofbi->region.vrfb.vaddr[0];
+		return ofbi->region->vrfb.vaddr[0];
 	else
-		return ofbi->region.vaddr;
+		return ofbi->region->vaddr;
 }
 
 static struct omapfb_colormode omapfb_colormodes[] = {
@@ -449,7 +449,7 @@ static int check_vrfb_fb_size(unsigned long region_size,
 static int check_fb_size(const struct omapfb_info *ofbi,
 		struct fb_var_screeninfo *var)
 {
-	unsigned long max_frame_size = ofbi->region.size;
+	unsigned long max_frame_size = ofbi->region->size;
 	int bytespp = var->bits_per_pixel >> 3;
 	unsigned long line_size = var->xres_virtual * bytespp;
 
@@ -496,7 +496,7 @@ static int check_fb_size(const struct omapfb_info *ofbi,
 static int setup_vrfb_rotation(struct fb_info *fbi)
 {
 	struct omapfb_info *ofbi = FB2OFB(fbi);
-	struct omapfb2_mem_region *rg = &ofbi->region;
+	struct omapfb2_mem_region *rg = ofbi->region;
 	struct vrfb *vrfb = &rg->vrfb;
 	struct fb_var_screeninfo *var = &fbi->var;
 	struct fb_fix_screeninfo *fix = &fbi->fix;
@@ -557,9 +557,9 @@ static int setup_vrfb_rotation(struct fb_info *fbi)
 		return r;
 
 	/* used by open/write in fbmem.c */
-	fbi->screen_base = ofbi->region.vrfb.vaddr[0];
+	fbi->screen_base = ofbi->region->vrfb.vaddr[0];
 
-	fix->smem_start = ofbi->region.vrfb.paddr[0];
+	fix->smem_start = ofbi->region->vrfb.paddr[0];
 
 	switch (var->nonstd) {
 	case OMAPFB_COLOR_YUV422:
@@ -598,7 +598,7 @@ void set_fb_fix(struct fb_info *fbi)
 	struct fb_fix_screeninfo *fix = &fbi->fix;
 	struct fb_var_screeninfo *var = &fbi->var;
 	struct omapfb_info *ofbi = FB2OFB(fbi);
-	struct omapfb2_mem_region *rg = &ofbi->region;
+	struct omapfb2_mem_region *rg = ofbi->region;
 
 	DBG("set_fb_fix\n");
 
@@ -667,9 +667,6 @@ int check_fb_var(struct fb_info *fbi, struct fb_var_screeninfo *var)
 
 	DBG("check_fb_var %d\n", ofbi->id);
 
-	if (ofbi->region.size = 0)
-		return 0;
-
 	r = fb_mode_to_dss_mode(var, &mode);
 	if (r) {
 		DBG("cannot convert var to omap dss mode\n");
@@ -689,7 +686,8 @@ int check_fb_var(struct fb_info *fbi, struct fb_var_screeninfo *var)
 	if (check_fb_res_bounds(var))
 		return -EINVAL;
 
-	if (check_fb_size(ofbi, var))
+	/* When no memory is allocated ignore the size check */
+	if (ofbi->region->size != 0 && check_fb_size(ofbi, var))
 		return -EINVAL;
 
 	if (var->xres + var->xoffset > var->xres_virtual)
@@ -821,9 +819,43 @@ static unsigned calc_rotation_offset_vrfb(const struct fb_var_screeninfo *var,
 	return offset;
 }
 
+static void omapfb_calc_addr(const struct omapfb_info *ofbi,
+			     const struct fb_var_screeninfo *var,
+			     const struct fb_fix_screeninfo *fix,
+			     int rotation, u32 *paddr, void __iomem **vaddr)
+{
+	u32 data_start_p;
+	void __iomem *data_start_v;
+	int offset;
+
+	if (ofbi->rotation_type = OMAP_DSS_ROT_VRFB) {
+		data_start_p = omapfb_get_region_rot_paddr(ofbi, rotation);
+		data_start_v = NULL;
+	} else {
+		data_start_p = omapfb_get_region_paddr(ofbi);
+		data_start_v = omapfb_get_region_vaddr(ofbi);
+	}
+
+	if (ofbi->rotation_type = OMAP_DSS_ROT_VRFB)
+		offset = calc_rotation_offset_vrfb(var, fix, rotation);
+	else
+		offset = calc_rotation_offset_dma(var, fix, rotation);
+
+	data_start_p += offset;
+	data_start_v += offset;
+
+	if (offset)
+		DBG("offset %d, %d = %d\n",
+		    var->xoffset, var->yoffset, offset);
+
+	DBG("paddr %x, vaddr %p\n", data_start_p, data_start_v);
+
+	*paddr = data_start_p;
+	*vaddr = data_start_v;
+}
 
 /* setup overlay according to the fb */
-static int omapfb_setup_overlay(struct fb_info *fbi, struct omap_overlay *ovl,
+int omapfb_setup_overlay(struct fb_info *fbi, struct omap_overlay *ovl,
 		u16 posx, u16 posy, u16 outw, u16 outh)
 {
 	int r = 0;
@@ -831,9 +863,8 @@ static int omapfb_setup_overlay(struct fb_info *fbi, struct omap_overlay *ovl,
 	struct fb_var_screeninfo *var = &fbi->var;
 	struct fb_fix_screeninfo *fix = &fbi->fix;
 	enum omap_color_mode mode = 0;
-	int offset;
-	u32 data_start_p;
-	void __iomem *data_start_v;
+	u32 data_start_p = 0;
+	void __iomem *data_start_v = NULL;
 	struct omap_overlay_info info;
 	int xres, yres;
 	int screen_width;
@@ -860,28 +891,9 @@ static int omapfb_setup_overlay(struct fb_info *fbi, struct omap_overlay *ovl,
 		yres = var->yres;
 	}
 
-
-	if (ofbi->rotation_type = OMAP_DSS_ROT_VRFB) {
-		data_start_p = omapfb_get_region_rot_paddr(ofbi, rotation);
-		data_start_v = NULL;
-	} else {
-		data_start_p = omapfb_get_region_paddr(ofbi);
-		data_start_v = omapfb_get_region_vaddr(ofbi);
-	}
-
-	if (ofbi->rotation_type = OMAP_DSS_ROT_VRFB)
-		offset = calc_rotation_offset_vrfb(var, fix, rotation);
-	else
-		offset = calc_rotation_offset_dma(var, fix, rotation);
-
-	data_start_p += offset;
-	data_start_v += offset;
-
-	if (offset)
-		DBG("offset %d, %d = %d\n",
-				var->xoffset, var->yoffset, offset);
-
-	DBG("paddr %x, vaddr %p\n", data_start_p, data_start_v);
+	if (ofbi->region->size)
+		omapfb_calc_addr(ofbi, var, fix, rotation,
+				 &data_start_p, &data_start_v);
 
 	r = fb_mode_to_dss_mode(var, &mode);
 	if (r) {
@@ -958,9 +970,9 @@ int omapfb_apply_changes(struct fb_info *fbi, int init)
 
 		DBG("apply_changes, fb %d, ovl %d\n", ofbi->id, ovl->id);
 
-		if (ofbi->region.size = 0) {
+		if (ofbi->region->size = 0) {
 			/* the fb is not available. disable the overlay */
-			omapfb_overlay_enable(ovl, 0);
+			r = omapfb_overlay_enable(ovl, false);
 			if (!init && ovl->manager)
 				ovl->manager->apply(ovl->manager);
 			continue;
@@ -1006,36 +1018,48 @@ err:
  * DO NOT MODIFY PAR */
 static int omapfb_check_var(struct fb_var_screeninfo *var, struct fb_info *fbi)
 {
+	struct omapfb_info *ofbi = FB2OFB(fbi);
 	int r;
 
 	DBG("check_var(%d)\n", FB2OFB(fbi)->id);
 
+	omapfb_get_mem_region(ofbi->region);
+
 	r = check_fb_var(fbi, var);
 
+	omapfb_put_mem_region(ofbi->region);
+
 	return r;
 }
 
 /* set the video mode according to info->var */
 static int omapfb_set_par(struct fb_info *fbi)
 {
+	struct omapfb_info *ofbi = FB2OFB(fbi);
 	int r;
 
 	DBG("set_par(%d)\n", FB2OFB(fbi)->id);
 
+	omapfb_get_mem_region(ofbi->region);
+
 	set_fb_fix(fbi);
 
 	r = setup_vrfb_rotation(fbi);
 	if (r)
-		return r;
+		goto out;
 
 	r = omapfb_apply_changes(fbi, 0);
 
+ out:
+	omapfb_put_mem_region(ofbi->region);
+
 	return r;
 }
 
 static int omapfb_pan_display(struct fb_var_screeninfo *var,
 		struct fb_info *fbi)
 {
+	struct omapfb_info *ofbi = FB2OFB(fbi);
 	struct fb_var_screeninfo new_var;
 	int r;
 
@@ -1051,23 +1075,31 @@ static int omapfb_pan_display(struct fb_var_screeninfo *var,
 
 	fbi->var = new_var;
 
+	omapfb_get_mem_region(ofbi->region);
+
 	r = omapfb_apply_changes(fbi, 0);
 
+	omapfb_put_mem_region(ofbi->region);
+
 	return r;
 }
 
 static void mmap_user_open(struct vm_area_struct *vma)
 {
-	struct omapfb_info *ofbi = (struct omapfb_info *)vma->vm_private_data;
+	struct omapfb2_mem_region *rg = vma->vm_private_data;
 
-	atomic_inc(&ofbi->map_count);
+	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 omapfb_info *ofbi = (struct omapfb_info *)vma->vm_private_data;
+	struct omapfb2_mem_region *rg = vma->vm_private_data;
 
-	atomic_dec(&ofbi->map_count);
+	omapfb_get_mem_region(rg);
+	atomic_dec(&rg->map_count);
+	omapfb_put_mem_region(rg);
 }
 
 static struct vm_operations_struct mmap_user_ops = {
@@ -1079,9 +1111,11 @@ static int omapfb_mmap(struct fb_info *fbi, struct vm_area_struct *vma)
 {
 	struct omapfb_info *ofbi = FB2OFB(fbi);
 	struct fb_fix_screeninfo *fix = &fbi->fix;
+	struct omapfb2_mem_region *rg;
 	unsigned long off;
 	unsigned long start;
 	u32 len;
+	int r = -EINVAL;
 
 	if (vma->vm_end - vma->vm_start = 0)
 		return 0;
@@ -1089,12 +1123,14 @@ 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);
+
 	start = omapfb_get_region_paddr(ofbi);
 	len = fix->smem_len;
 	if (off >= len)
-		return -EINVAL;
+		goto error;
 	if ((vma->vm_end - vma->vm_start + off) > len)
-		return -EINVAL;
+		goto error;
 
 	off += start;
 
@@ -1104,13 +1140,24 @@ static int omapfb_mmap(struct fb_info *fbi, struct vm_area_struct *vma)
 	vma->vm_flags |= VM_IO | VM_RESERVED;
 	vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot);
 	vma->vm_ops = &mmap_user_ops;
-	vma->vm_private_data = ofbi;
+	vma->vm_private_data = rg;
 	if (io_remap_pfn_range(vma, vma->vm_start, off >> PAGE_SHIFT,
-			     vma->vm_end - vma->vm_start, vma->vm_page_prot))
-		return -EAGAIN;
+			vma->vm_end - vma->vm_start, vma->vm_page_prot)) {
+		r = -EAGAIN;
+		goto error;
+	}
+
 	/* vm_ops.open won't be called for mmap itself. */
-	atomic_inc(&ofbi->map_count);
+	atomic_inc(&rg->map_count);
+
+	omapfb_put_mem_region(rg);
+
 	return 0;
+
+ error:
+	omapfb_put_mem_region(ofbi->region);
+
+	return r;
 }
 
 /* Store a single color palette entry into a pseudo palette or the hardware
@@ -1299,7 +1346,9 @@ static void omapfb_free_fbmem(struct fb_info *fbi)
 	struct omapfb2_device *fbdev = ofbi->fbdev;
 	struct omapfb2_mem_region *rg;
 
-	rg = &ofbi->region;
+	rg = ofbi->region;
+
+	WARN_ON(atomic_read(&rg->map_count));
 
 	if (rg->paddr)
 		if (omap_vram_free(rg->paddr, rg->size))
@@ -1354,8 +1403,15 @@ static int omapfb_alloc_fbmem(struct fb_info *fbi, unsigned long size,
 	void __iomem *vaddr;
 	int r;
 
-	rg = &ofbi->region;
-	memset(rg, 0, sizeof(*rg));
+	rg = ofbi->region;
+
+	rg->paddr = 0;
+	rg->vaddr = NULL;
+	memset(&rg->vrfb, 0, sizeof rg->vrfb);
+	rg->size = 0;
+	rg->type = 0;
+	rg->alloc = false;
+	rg->map = false;
 
 	size = PAGE_ALIGN(size);
 
@@ -1608,7 +1664,7 @@ static int omapfb_allocate_all_fbs(struct omapfb2_device *fbdev)
 	for (i = 0; i < fbdev->num_fbs; i++) {
 		struct omapfb_info *ofbi = FB2OFB(fbdev->fbs[i]);
 		struct omapfb2_mem_region *rg;
-		rg = &ofbi->region;
+		rg = ofbi->region;
 
 		DBG("region%d phys %08x virt %p size=%lu\n",
 				i,
@@ -1625,7 +1681,7 @@ int omapfb_realloc_fbmem(struct fb_info *fbi, unsigned long size, int type)
 	struct omapfb_info *ofbi = FB2OFB(fbi);
 	struct omapfb2_device *fbdev = ofbi->fbdev;
 	struct omap_dss_device *display = fb2display(fbi);
-	struct omapfb2_mem_region *rg = &ofbi->region;
+	struct omapfb2_mem_region *rg = ofbi->region;
 	unsigned long old_size = rg->size;
 	unsigned long old_paddr = rg->paddr;
 	int old_type = rg->type;
@@ -1708,7 +1764,7 @@ static int omapfb_fb_init(struct omapfb2_device *fbdev, struct fb_info *fbi)
 	fbi->flags = FBINFO_FLAG_DEFAULT;
 	fbi->pseudo_palette = fbdev->pseudo_palette;
 
-	if (ofbi->region.size = 0) {
+	if (ofbi->region->size = 0) {
 		clear_fb_info(fbi);
 		return 0;
 	}
@@ -1870,6 +1926,10 @@ static int omapfb_create_framebuffers(struct omapfb2_device *fbdev)
 		ofbi->fbdev = fbdev;
 		ofbi->id = i;
 
+		ofbi->region = &fbdev->regions[i];
+		ofbi->region->id = i;
+		mutex_init(&ofbi->region->mtx);
+
 		/* assign these early, so that fb alloc can use them */
 		ofbi->rotation_type = def_vrfb ? OMAP_DSS_ROT_VRFB :
 			OMAP_DSS_ROT_DMA;
@@ -1941,7 +2001,7 @@ static int omapfb_create_framebuffers(struct omapfb2_device *fbdev)
 		if (ofbi->num_overlays > 0) {
 			struct omap_overlay *ovl = ofbi->overlays[0];
 
-			r = omapfb_overlay_enable(ovl, 1);
+			r = omapfb_overlay_enable(ovl, true);
 
 			if (r) {
 				dev_err(fbdev->dev,
diff --git a/drivers/video/omap2/omapfb/omapfb-sysfs.c b/drivers/video/omap2/omapfb/omapfb-sysfs.c
index 62bb88f..caedbaa 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_mem_region *rg;
 	enum omap_dss_rotation_type rot_type;
 	int r;
 
@@ -63,9 +64,11 @@ static ssize_t store_rotate_type(struct device *dev,
 	if (rot_type = ofbi->rotation_type)
 		goto out;
 
-	if (ofbi->region.size) {
+	rg = omapfb_get_mem_region(ofbi->region);
+
+	if (rg->size) {
 		r = -EBUSY;
-		goto out;
+		goto put_region;
 	}
 
 	ofbi->rotation_type = rot_type;
@@ -74,7 +77,10 @@ static ssize_t store_rotate_type(struct device *dev,
 	 * Since the VRAM for this FB is not allocated at the moment we don't
 	 * need to do any further parameter checking at this point.
 	 */
+put_region:
+	omapfb_put_mem_region(rg);
 out:
+
 	unlock_fb_info(fbi);
 
 	return r ? r : count;
@@ -242,6 +248,7 @@ static ssize_t store_overlays(struct device *dev, struct device_attribute *attr,
 	/* detach unused overlays */
 	for (i = 0; i < ofbi->num_overlays; ++i) {
 		int t, found;
+		struct omapfb2_mem_region *rg;
 
 		ovl = ofbi->overlays[i];
 
@@ -259,7 +266,9 @@ static ssize_t store_overlays(struct device *dev, struct device_attribute *attr,
 
 		DBG("detaching %d\n", ofbi->overlays[i]->id);
 
-		omapfb_overlay_enable(ovl, 0);
+		rg = omapfb_get_mem_region(ofbi->region);
+		r = omapfb_overlay_enable(ovl, false);
+		omapfb_put_mem_region(ofbi->region);
 
 		if (ovl->manager)
 			ovl->manager->apply(ovl->manager);
@@ -402,7 +411,7 @@ static ssize_t show_size(struct device *dev,
 	struct fb_info *fbi = dev_get_drvdata(dev);
 	struct omapfb_info *ofbi = FB2OFB(fbi);
 
-	return snprintf(buf, PAGE_SIZE, "%lu\n", ofbi->region.size);
+	return snprintf(buf, PAGE_SIZE, "%lu\n", ofbi->region->size);
 }
 
 static ssize_t store_size(struct device *dev, struct device_attribute *attr,
@@ -410,6 +419,8 @@ static ssize_t store_size(struct device *dev, struct device_attribute *attr,
 {
 	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;
 	unsigned long size;
 	int r;
 	int i;
@@ -418,15 +429,38 @@ static ssize_t store_size(struct device *dev, struct device_attribute *attr,
 
 	lock_fb_info(fbi);
 
-	for (i = 0; i < ofbi->num_overlays; i++) {
-		if (ofbi->overlays[i]->info.enabled) {
-			r = -EBUSY;
-			goto out;
+	rg = ofbi->region;
+
+	/* FIXME probably should be a rwsem ... */
+	mutex_lock(&rg->mtx);
+	while (rg->ref) {
+		mutex_unlock(&rg->mtx);
+		schedule();
+		mutex_lock(&rg->mtx);
+	}
+
+	if (atomic_read(&rg->map_count)) {
+		r = -EBUSY;
+		goto out;
+	}
+
+	for (i = 0; i < fbdev->num_fbs; i++) {
+		struct omapfb_info *ofbi2 = FB2OFB(fbdev->fbs[i]);
+		int j;
+
+		if (ofbi2->region != rg)
+			continue;
+
+		for (j = 0; j < ofbi2->num_overlays; j++) {
+			if (ofbi2->overlays[j]->info.enabled) {
+				r = -EBUSY;
+				goto out;
+			}
 		}
 	}
 
-	if (size != ofbi->region.size) {
-		r = omapfb_realloc_fbmem(fbi, size, ofbi->region.type);
+	if (size != ofbi->region->size) {
+		r = omapfb_realloc_fbmem(fbi, size, ofbi->region->type);
 		if (r) {
 			dev_err(dev, "realloc fbmem failed\n");
 			goto out;
@@ -435,6 +469,8 @@ static ssize_t store_size(struct device *dev, struct device_attribute *attr,
 
 	r = count;
 out:
+	mutex_unlock(&rg->mtx);
+
 	unlock_fb_info(fbi);
 
 	return r;
@@ -446,7 +482,7 @@ static ssize_t show_phys(struct device *dev,
 	struct fb_info *fbi = dev_get_drvdata(dev);
 	struct omapfb_info *ofbi = FB2OFB(fbi);
 
-	return snprintf(buf, PAGE_SIZE, "%0x\n", ofbi->region.paddr);
+	return snprintf(buf, PAGE_SIZE, "%0x\n", ofbi->region->paddr);
 }
 
 static ssize_t show_virt(struct device *dev,
@@ -455,7 +491,7 @@ static ssize_t show_virt(struct device *dev,
 	struct fb_info *fbi = dev_get_drvdata(dev);
 	struct omapfb_info *ofbi = FB2OFB(fbi);
 
-	return snprintf(buf, PAGE_SIZE, "%p\n", ofbi->region.vaddr);
+	return snprintf(buf, PAGE_SIZE, "%p\n", ofbi->region->vaddr);
 }
 
 static struct device_attribute omapfb_attrs[] = {
diff --git a/drivers/video/omap2/omapfb/omapfb.h b/drivers/video/omap2/omapfb/omapfb.h
index cd54fdb..ce15533 100644
--- a/drivers/video/omap2/omapfb/omapfb.h
+++ b/drivers/video/omap2/omapfb/omapfb.h
@@ -44,6 +44,7 @@ extern unsigned int omapfb_debug;
 #define OMAPFB_MAX_OVL_PER_FB 3
 
 struct omapfb2_mem_region {
+	int             id;
 	u32		paddr;
 	void __iomem	*vaddr;
 	struct vrfb	vrfb;
@@ -51,13 +52,15 @@ struct omapfb2_mem_region {
 	u8		type;		/* OMAPFB_PLANE_MEM_* */
 	bool		alloc;		/* allocated by the driver */
 	bool		map;		/* kernel mapped by the driver */
+	struct mutex    mtx;
+	unsigned int    ref;
+	atomic_t	map_count;
 };
 
 /* appended to fb_info */
 struct omapfb_info {
 	int id;
-	struct omapfb2_mem_region region;
-	atomic_t map_count;
+	struct omapfb2_mem_region *region;
 	int num_overlays;
 	struct omap_overlay *overlays[OMAPFB_MAX_OVL_PER_FB];
 	struct omapfb2_device *fbdev;
@@ -76,6 +79,7 @@ struct omapfb2_device {
 
 	unsigned num_fbs;
 	struct fb_info *fbs[10];
+	struct omapfb2_mem_region regions[10];
 
 	unsigned num_displays;
 	struct omap_dss_device *displays[10];
@@ -117,6 +121,9 @@ int omapfb_update_window(struct fb_info *fbi,
 int dss_mode_to_fb_mode(enum omap_color_mode dssmode,
 			struct fb_var_screeninfo *var);
 
+int omapfb_setup_overlay(struct fb_info *fbi, struct omap_overlay *ovl,
+		u16 posx, u16 posy, u16 outw, u16 outh);
+
 /* find the display connected to this fb, if any */
 static inline struct omap_dss_device *fb2display(struct fb_info *fbi)
 {
@@ -143,13 +150,36 @@ static inline void omapfb_unlock(struct omapfb2_device *fbdev)
 }
 
 static inline int omapfb_overlay_enable(struct omap_overlay *ovl,
-		int enable)
+					int enable)
 {
 	struct omap_overlay_info info;
+	int r;
 
 	ovl->get_overlay_info(ovl, &info);
+	if (info.enabled = enable)
+		return 0;
 	info.enabled = enable;
-	return ovl->set_overlay_info(ovl, &info);
+	r = ovl->set_overlay_info(ovl, &info);
+	if (r)
+		return r;
+
+	return 0;
+}
+
+static inline struct omapfb2_mem_region *
+omapfb_get_mem_region(struct omapfb2_mem_region *rg)
+{
+	mutex_lock(&rg->mtx);
+	rg->ref++;
+	mutex_unlock(&rg->mtx);
+	return rg;
+}
+
+static inline void omapfb_put_mem_region(struct omapfb2_mem_region *rg)
+{
+	mutex_lock(&rg->mtx);
+	rg->ref--;
+	mutex_unlock(&rg->mtx);
 }
 
 #endif
diff --git a/include/linux/omapfb.h b/include/linux/omapfb.h
index 9bdd914..5de473c 100644
--- a/include/linux/omapfb.h
+++ b/include/linux/omapfb.h
@@ -85,6 +85,9 @@
 #define OMAPFB_MEMTYPE_SRAM		1
 #define OMAPFB_MEMTYPE_MAX		1
 
+#define OMAPFB_MEM_IDX_ENABLED	0x8
+#define OMAPFB_MEM_IDX_MASK	0x7
+
 enum omapfb_color_format {
 	OMAPFB_COLOR_RGB565 = 0,
 	OMAPFB_COLOR_YUV422,
@@ -136,7 +139,7 @@ struct omapfb_plane_info {
 	__u8  enabled;
 	__u8  channel_out;
 	__u8  mirror;
-	__u8  reserved1;
+	__u8  mem_idx;
 	__u32 out_width;
 	__u32 out_height;
 	__u32 reserved2[12];
-- 
1.6.4.4

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] DSS2: Check if display supports update mode changes

[ville.syrjala]

From: Ville Syrj채l채 <ville.syrjala@nokia.com>

Check whether the display actually has the set_update_mode() function
before calling it. Only the sysfs codepath was broken, the omapfb ioctl
had the necessary protection.

Signed-off-by: Ville Syrj채l채 <ville.syrjala@nokia.com>
---
 drivers/video/omap2/dss/display.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/video/omap2/dss/display.c b/drivers/video/omap2/dss/display.c
index 6a74ea1..3a8f146 100644
--- a/drivers/video/omap2/dss/display.c
+++ b/drivers/video/omap2/dss/display.c
@@ -82,6 +82,9 @@ static ssize_t display_upd_mode_store(struct device *dev,
 	int val, r;
 	enum omap_dss_update_mode mode;
 
+	if (!dssdev->driver->set_update_mode)
+		return -EINVAL;
+
 	val = simple_strtoul(buf, NULL, 10);
 
 	switch (val) {
-- 
1.6.4.4

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] DSS2: Make wait_for_go() succeed for disabled displays

[ville.syrjala]

From: Ville Syrj채l채 <ville.syrjala@nokia.com>

When the display is not active make the wait_for_go() functions return
immediately.

Signed-off-by: Ville Syrj채l채 <ville.syrjala@nokia.com>
---
 drivers/video/omap2/dss/manager.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/video/omap2/dss/manager.c b/drivers/video/omap2/dss/manager.c
index 913142d..0e7b036 100644
--- a/drivers/video/omap2/dss/manager.c
+++ b/drivers/video/omap2/dss/manager.c
@@ -524,7 +524,7 @@ static int dss_mgr_wait_for_go(struct omap_overlay_manager *mgr)
 	int i;
 	struct omap_dss_device *dssdev = mgr->device;
 
-	if (!dssdev)
+	if (!dssdev || dssdev->state != OMAP_DSS_DISPLAY_ACTIVE)
 		return 0;
 
 	if (dssdev->type = OMAP_DISPLAY_TYPE_VENC) {
@@ -595,11 +595,14 @@ int dss_mgr_wait_for_go_ovl(struct omap_overlay *ovl)
 	int r;
 	int i;
 
-	if (!ovl->manager || !ovl->manager->device)
+	if (!ovl->manager)
 		return 0;
 
 	dssdev = ovl->manager->device;
 
+	if (!dssdev || dssdev->state != OMAP_DSS_DISPLAY_ACTIVE)
+		return 0;
+
 	if (dssdev->type = OMAP_DISPLAY_TYPE_VENC) {
 		irq = DISPC_IRQ_EVSYNC_ODD | DISPC_IRQ_EVSYNC_EVEN;
 		channel = OMAP_DSS_CHANNEL_DIGIT;
-- 
1.6.4.4

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] DSS2: clear spurious SYNC_LOST_DIGIT interrupts

[ville.syrjala]

From: Ville Syrj채l채 <ville.syrjala@nokia.com>

When DSS transitions from off mode to on VENC may generate a spurious
SYNC_LOST_DIGIT error. Just ack it when restoring the context. Also
restore IRQENABLE last to avoid triggering interrupts before the
context is fully restored.

Signed-off-by: Ville Syrj채l채 <ville.syrjala@nokia.com>
---
 drivers/video/omap2/dss/dispc.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index e777e35..b8c1603 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -335,7 +335,7 @@ void dispc_save_context(void)
 void dispc_restore_context(void)
 {
 	RR(SYSCONFIG);
-	RR(IRQENABLE);
+	/*RR(IRQENABLE);*/
 	/*RR(CONTROL);*/
 	RR(CONFIG);
 	RR(DEFAULT_COLOR0);
@@ -472,6 +472,15 @@ void dispc_restore_context(void)
 
 	/* enable last, because LCD & DIGIT enable are here */
 	RR(CONTROL);
+
+	/* clear spurious SYNC_LOST_DIGIT interrupts */
+	dispc_write_reg(DISPC_IRQSTATUS, DISPC_IRQ_SYNC_LOST_DIGIT);
+
+	/*
+	 * enable last so IRQs won't trigger before
+	 * the context is fully restored
+	 */
+	RR(IRQENABLE);
 }
 
 #undef SR
-- 
1.6.4.4

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 4/4] DSS2: clear spurious SYNC_LOST_DIGIT interrupts

[Aguirre, Sergio]

Hi Ville,

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of ville.syrjala@nokia.com
> Sent: Friday, March 05, 2010 7:26 AM
> To: Tomi Valkeinen
> Cc: linux-fbdev@vger.kernel.org; linux-omap@vger.kernel.org; Ville Syrjälä
> Subject: [PATCH 4/4] DSS2: clear spurious SYNC_LOST_DIGIT interrupts
> 
> From: Ville Syrjälä <ville.syrjala@nokia.com>
> 
> When DSS transitions from off mode to on VENC may generate a spurious
> SYNC_LOST_DIGIT error. Just ack it when restoring the context. Also
> restore IRQENABLE last to avoid triggering interrupts before the
> context is fully restored.

(Tomi or anyone can correct me if I'm wrong)

AFAIK, The solution for spurious interrupts is to flush posted bus writes during interrupt handling, which can be achieved by reading the same register just after you have written it.

Tomi, what do you think?

Regards,
Sergio

> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@nokia.com>
> ---
>  drivers/video/omap2/dss/dispc.c |   11 ++++++++++-
>  1 files changed, 10 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/dispc.c
> b/drivers/video/omap2/dss/dispc.c
> index e777e35..b8c1603 100644
> --- a/drivers/video/omap2/dss/dispc.c
> +++ b/drivers/video/omap2/dss/dispc.c
> @@ -335,7 +335,7 @@ void dispc_save_context(void)
>  void dispc_restore_context(void)
>  {
>  	RR(SYSCONFIG);
> -	RR(IRQENABLE);
> +	/*RR(IRQENABLE);*/
>  	/*RR(CONTROL);*/
>  	RR(CONFIG);
>  	RR(DEFAULT_COLOR0);
> @@ -472,6 +472,15 @@ void dispc_restore_context(void)
> 
>  	/* enable last, because LCD & DIGIT enable are here */
>  	RR(CONTROL);
> +
> +	/* clear spurious SYNC_LOST_DIGIT interrupts */
> +	dispc_write_reg(DISPC_IRQSTATUS, DISPC_IRQ_SYNC_LOST_DIGIT);
> +
> +	/*
> +	 * enable last so IRQs won't trigger before
> +	 * the context is fully restored
> +	 */
> +	RR(IRQENABLE);
>  }
> 
>  #undef SR
> --
> 1.6.4.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 4/4] DSS2: clear spurious SYNC_LOST_DIGIT interrupts

[Tomi Valkeinen]

On Fri, 2010-03-05 at 16:19 +0100, ext Aguirre, Sergio wrote:
> Hi Ville,
> 
> > -----Original Message-----
> > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> > owner@vger.kernel.org] On Behalf Of ville.syrjala@nokia.com
> > Sent: Friday, March 05, 2010 7:26 AM
> > To: Tomi Valkeinen
> > Cc: linux-fbdev@vger.kernel.org; linux-omap@vger.kernel.org; Ville Syrj채l채
> > Subject: [PATCH 4/4] DSS2: clear spurious SYNC_LOST_DIGIT interrupts
> > 
> > From: Ville Syrj채l채 <ville.syrjala@nokia.com>
> > 
> > When DSS transitions from off mode to on VENC may generate a spurious
> > SYNC_LOST_DIGIT error. Just ack it when restoring the context. Also
> > restore IRQENABLE last to avoid triggering interrupts before the
> > context is fully restored.
> 
> (Tomi or anyone can correct me if I'm wrong)
> 
> AFAIK, The solution for spurious interrupts is to flush posted bus writes during interrupt handling, which can be achieved by reading the same register just after you have written it.
> 
> Tomi, what do you think?

That's a different spurious interrupt =).

In this case the IRQ mechanism from CPU side is working fine, but VENC
is just spamming the error interrupts, for unknown reason.

 Tomi


--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 4/4] DSS2: clear spurious SYNC_LOST_DIGIT interrupts

[Aguirre, Sergio]

> -----Original Message-----
> From: Tomi Valkeinen [mailto:tomi.valkeinen@nokia.com]
> Sent: Friday, March 05, 2010 9:25 AM
> To: Aguirre, Sergio
> Cc: Syrjala Ville (Nokia-D/Helsinki); linux-fbdev@vger.kernel.org; linux-
> omap@vger.kernel.org
> Subject: RE: [PATCH 4/4] DSS2: clear spurious SYNC_LOST_DIGIT interrupts
> 
> On Fri, 2010-03-05 at 16:19 +0100, ext Aguirre, Sergio wrote:
> > Hi Ville,
> >
> > > -----Original Message-----
> > > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> > > owner@vger.kernel.org] On Behalf Of ville.syrjala@nokia.com
> > > Sent: Friday, March 05, 2010 7:26 AM
> > > To: Tomi Valkeinen
> > > Cc: linux-fbdev@vger.kernel.org; linux-omap@vger.kernel.org; Ville
> Syrjälä
> > > Subject: [PATCH 4/4] DSS2: clear spurious SYNC_LOST_DIGIT interrupts
> > >
> > > From: Ville Syrjälä <ville.syrjala@nokia.com>
> > >
> > > When DSS transitions from off mode to on VENC may generate a spurious
> > > SYNC_LOST_DIGIT error. Just ack it when restoring the context. Also
> > > restore IRQENABLE last to avoid triggering interrupts before the
> > > context is fully restored.
> >
> > (Tomi or anyone can correct me if I'm wrong)
> >
> > AFAIK, The solution for spurious interrupts is to flush posted bus
> writes during interrupt handling, which can be achieved by reading the
> same register just after you have written it.
> >
> > Tomi, what do you think?
> 
> That's a different spurious interrupt =).
> 
> In this case the IRQ mechanism from CPU side is working fine, but VENC
> is just spamming the error interrupts, for unknown reason.

Ohh ok, I see.

Thanks for clarifying. And sorry for the noise.

I'm trying to learn about DSS subsystem, but I'm just a n00b for the moment :)

Regards,
Sergio
> 
>  Tomi
> 

Re: [PATCH v3 1/4] DSS2: OMAPFB: Add support for switching memory

[Tomi Valkeinen]

Hi,

On Fri, 2010-03-05 at 14:26 +0100, Syrjala Ville (Nokia-D/Helsinki)
wrote:
> From: Ville Syrj채l채 <ville.syrjala@nokia.com>
> 
> Separate the memory region from the framebuffer device a little bit.
> It's now possible to select the memory region used by the framebuffer
> device using the new mem_idx parameter of omapfb_plane_info. If the
> mem_idx is specified it will be interpreted as an index into the
> memory regions array, if it's not specified the framebuffer's index is
> used instead. So by default each framebuffer keeps using it's own
> memory region which preserves backwards compatibility.
> 
> This allows cloning the same memory region to several overlays and yet
> each overlay can be controlled independently since they can be
> associated with separate framebuffer devices.

Couple of comments inline. Mostly about making the patch a bit simpler.

> 
> Signed-off-by: Ville Syrj채l채 <ville.syrjala@nokia.com>
> ---
> Changes since v2:
> * Removed the use_count and rely on just counting all active overlays. A bit racy
>   but no chance of getting stuck in a state where memory allocation can't be changed.
> * s/source_idx/mem_idx as that seems to be a little more self explanatory
> 
>  drivers/video/omap2/omapfb/omapfb-ioctl.c |  163 ++++++++++++++++++++-----
>  drivers/video/omap2/omapfb/omapfb-main.c  |  184 +++++++++++++++++++----------
>  drivers/video/omap2/omapfb/omapfb-sysfs.c |   60 ++++++++--
>  drivers/video/omap2/omapfb/omapfb.h       |   38 ++++++-
>  include/linux/omapfb.h                    |    5 +-
>  5 files changed, 339 insertions(+), 111 deletions(-)

<snip>

> +static void omapfb_calc_addr(const struct omapfb_info *ofbi,
> +                            const struct fb_var_screeninfo *var,
> +                            const struct fb_fix_screeninfo *fix,
> +                            int rotation, u32 *paddr, void __iomem **vaddr)
> +{
> +       u32 data_start_p;
> +       void __iomem *data_start_v;
> +       int offset;
> +
> +       if (ofbi->rotation_type = OMAP_DSS_ROT_VRFB) {
> +               data_start_p = omapfb_get_region_rot_paddr(ofbi, rotation);
> +               data_start_v = NULL;
> +       } else {
> +               data_start_p = omapfb_get_region_paddr(ofbi);
> +               data_start_v = omapfb_get_region_vaddr(ofbi);
> +       }
> +
> +       if (ofbi->rotation_type = OMAP_DSS_ROT_VRFB)
> +               offset = calc_rotation_offset_vrfb(var, fix, rotation);
> +       else
> +               offset = calc_rotation_offset_dma(var, fix, rotation);
> +
> +       data_start_p += offset;
> +       data_start_v += offset;
> +
> +       if (offset)
> +               DBG("offset %d, %d = %d\n",
> +                   var->xoffset, var->yoffset, offset);
> +
> +       DBG("paddr %x, vaddr %p\n", data_start_p, data_start_v);
> +
> +       *paddr = data_start_p;
> +       *vaddr = data_start_v;
> +}
> 
>  /* setup overlay according to the fb */
> -static int omapfb_setup_overlay(struct fb_info *fbi, struct omap_overlay *ovl,
> +int omapfb_setup_overlay(struct fb_info *fbi, struct omap_overlay *ovl,
>                 u16 posx, u16 posy, u16 outw, u16 outh)
>  {
>         int r = 0;
> @@ -831,9 +863,8 @@ static int omapfb_setup_overlay(struct fb_info *fbi, struct omap_overlay *ovl,
>         struct fb_var_screeninfo *var = &fbi->var;
>         struct fb_fix_screeninfo *fix = &fbi->fix;
>         enum omap_color_mode mode = 0;
> -       int offset;
> -       u32 data_start_p;
> -       void __iomem *data_start_v;
> +       u32 data_start_p = 0;
> +       void __iomem *data_start_v = NULL;
>         struct omap_overlay_info info;
>         int xres, yres;
>         int screen_width;
> @@ -860,28 +891,9 @@ static int omapfb_setup_overlay(struct fb_info *fbi, struct omap_overlay *ovl,
>                 yres = var->yres;
>         }
> 
> -
> -       if (ofbi->rotation_type = OMAP_DSS_ROT_VRFB) {
> -               data_start_p = omapfb_get_region_rot_paddr(ofbi, rotation);
> -               data_start_v = NULL;
> -       } else {
> -               data_start_p = omapfb_get_region_paddr(ofbi);
> -               data_start_v = omapfb_get_region_vaddr(ofbi);
> -       }
> -
> -       if (ofbi->rotation_type = OMAP_DSS_ROT_VRFB)
> -               offset = calc_rotation_offset_vrfb(var, fix, rotation);
> -       else
> -               offset = calc_rotation_offset_dma(var, fix, rotation);
> -
> -       data_start_p += offset;
> -       data_start_v += offset;
> -
> -       if (offset)
> -               DBG("offset %d, %d = %d\n",
> -                               var->xoffset, var->yoffset, offset);
> -
> -       DBG("paddr %x, vaddr %p\n", data_start_p, data_start_v);
> +       if (ofbi->region->size)
> +               omapfb_calc_addr(ofbi, var, fix, rotation,
> +                                &data_start_p, &data_start_v);
> 
>         r = fb_mode_to_dss_mode(var, &mode);
>         if (r) {

Moving this code to omapfb_calc_addr() could perhaps be a separate
patch? I don't see other changes in there.

>  static struct device_attribute omapfb_attrs[] = {
> diff --git a/drivers/video/omap2/omapfb/omapfb.h b/drivers/video/omap2/omapfb/omapfb.h
> index cd54fdb..ce15533 100644
> --- a/drivers/video/omap2/omapfb/omapfb.h
> +++ b/drivers/video/omap2/omapfb/omapfb.h

>  static inline int omapfb_overlay_enable(struct omap_overlay *ovl,
> -               int enable)
> +                                       int enable)
>  {
>         struct omap_overlay_info info;
> +       int r;
> 
>         ovl->get_overlay_info(ovl, &info);
> +       if (info.enabled = enable)
> +               return 0;
>         info.enabled = enable;
> -       return ovl->set_overlay_info(ovl, &info);
> +       r = ovl->set_overlay_info(ovl, &info);
> +       if (r)
> +               return r;
> +
> +       return 0;
> +}

There are perhaps more changes here than needed. int r is not needed,
and the last lines do not need to be changed. And is if (info.enabled =
enable) required by this patch, or is it just optimization? If
optimization, it could be in separate patch.

>  #endif
> diff --git a/include/linux/omapfb.h b/include/linux/omapfb.h
> index 9bdd914..5de473c 100644
> --- a/include/linux/omapfb.h
> +++ b/include/linux/omapfb.h
> @@ -85,6 +85,9 @@
>  #define OMAPFB_MEMTYPE_SRAM            1
>  #define OMAPFB_MEMTYPE_MAX             1
> 
> +#define OMAPFB_MEM_IDX_ENABLED 0x8
> +#define OMAPFB_MEM_IDX_MASK    0x7

Should enabled be 0x80, and mask 0x7f? There are never that many mem
indexes, but is there any point in limiting the value?

 Tomi


--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/4] DSS2: OMAPFB: Add support for switching memory

[Ville Syrjälä]

On Wed, Mar 17, 2010 at 02:50:45PM +0100, Valkeinen Tomi (Nokia-D/Helsinki) wrote:
> Hi,
> 
> On Fri, 2010-03-05 at 14:26 +0100, Syrjala Ville (Nokia-D/Helsinki)
> wrote:
> > From: Ville Syrjälä <ville.syrjala@nokia.com>
> > 
> > Separate the memory region from the framebuffer device a little bit.
> > It's now possible to select the memory region used by the framebuffer
> > device using the new mem_idx parameter of omapfb_plane_info. If the
> > mem_idx is specified it will be interpreted as an index into the
> > memory regions array, if it's not specified the framebuffer's index is
> > used instead. So by default each framebuffer keeps using it's own
> > memory region which preserves backwards compatibility.
> > 
> > This allows cloning the same memory region to several overlays and yet
> > each overlay can be controlled independently since they can be
> > associated with separate framebuffer devices.
> 
> Couple of comments inline. Mostly about making the patch a bit simpler.
> 
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@nokia.com>
> > ---
> > Changes since v2:
> > * Removed the use_count and rely on just counting all active overlays. A bit racy
> >   but no chance of getting stuck in a state where memory allocation can't be changed.
> > * s/source_idx/mem_idx as that seems to be a little more self explanatory
> > 
> >  drivers/video/omap2/omapfb/omapfb-ioctl.c |  163 ++++++++++++++++++++-----
> >  drivers/video/omap2/omapfb/omapfb-main.c  |  184 +++++++++++++++++++----------
> >  drivers/video/omap2/omapfb/omapfb-sysfs.c |   60 ++++++++--
> >  drivers/video/omap2/omapfb/omapfb.h       |   38 ++++++-
> >  include/linux/omapfb.h                    |    5 +-
> >  5 files changed, 339 insertions(+), 111 deletions(-)
> 
> <snip>
> 
> > +static void omapfb_calc_addr(const struct omapfb_info *ofbi,
> > +                            const struct fb_var_screeninfo *var,
> > +                            const struct fb_fix_screeninfo *fix,
> > +                            int rotation, u32 *paddr, void __iomem **vaddr)
> > +{
> > +       u32 data_start_p;
> > +       void __iomem *data_start_v;
> > +       int offset;
> > +
> > +       if (ofbi->rotation_type = OMAP_DSS_ROT_VRFB) {
> > +               data_start_p = omapfb_get_region_rot_paddr(ofbi, rotation);
> > +               data_start_v = NULL;
> > +       } else {
> > +               data_start_p = omapfb_get_region_paddr(ofbi);
> > +               data_start_v = omapfb_get_region_vaddr(ofbi);
> > +       }
> > +
> > +       if (ofbi->rotation_type = OMAP_DSS_ROT_VRFB)
> > +               offset = calc_rotation_offset_vrfb(var, fix, rotation);
> > +       else
> > +               offset = calc_rotation_offset_dma(var, fix, rotation);
> > +
> > +       data_start_p += offset;
> > +       data_start_v += offset;
> > +
> > +       if (offset)
> > +               DBG("offset %d, %d = %d\n",
> > +                   var->xoffset, var->yoffset, offset);
> > +
> > +       DBG("paddr %x, vaddr %p\n", data_start_p, data_start_v);
> > +
> > +       *paddr = data_start_p;
> > +       *vaddr = data_start_v;
> > +}
> > 
> >  /* setup overlay according to the fb */
> > -static int omapfb_setup_overlay(struct fb_info *fbi, struct omap_overlay *ovl,
> > +int omapfb_setup_overlay(struct fb_info *fbi, struct omap_overlay *ovl,
> >                 u16 posx, u16 posy, u16 outw, u16 outh)
> >  {
> >         int r = 0;
> > @@ -831,9 +863,8 @@ static int omapfb_setup_overlay(struct fb_info *fbi, struct omap_overlay *ovl,
> >         struct fb_var_screeninfo *var = &fbi->var;
> >         struct fb_fix_screeninfo *fix = &fbi->fix;
> >         enum omap_color_mode mode = 0;
> > -       int offset;
> > -       u32 data_start_p;
> > -       void __iomem *data_start_v;
> > +       u32 data_start_p = 0;
> > +       void __iomem *data_start_v = NULL;
> >         struct omap_overlay_info info;
> >         int xres, yres;
> >         int screen_width;
> > @@ -860,28 +891,9 @@ static int omapfb_setup_overlay(struct fb_info *fbi, struct omap_overlay *ovl,
> >                 yres = var->yres;
> >         }
> > 
> > -
> > -       if (ofbi->rotation_type = OMAP_DSS_ROT_VRFB) {
> > -               data_start_p = omapfb_get_region_rot_paddr(ofbi, rotation);
> > -               data_start_v = NULL;
> > -       } else {
> > -               data_start_p = omapfb_get_region_paddr(ofbi);
> > -               data_start_v = omapfb_get_region_vaddr(ofbi);
> > -       }
> > -
> > -       if (ofbi->rotation_type = OMAP_DSS_ROT_VRFB)
> > -               offset = calc_rotation_offset_vrfb(var, fix, rotation);
> > -       else
> > -               offset = calc_rotation_offset_dma(var, fix, rotation);
> > -
> > -       data_start_p += offset;
> > -       data_start_v += offset;
> > -
> > -       if (offset)
> > -               DBG("offset %d, %d = %d\n",
> > -                               var->xoffset, var->yoffset, offset);
> > -
> > -       DBG("paddr %x, vaddr %p\n", data_start_p, data_start_v);
> > +       if (ofbi->region->size)
> > +               omapfb_calc_addr(ofbi, var, fix, rotation,
> > +                                &data_start_p, &data_start_v);
> > 
> >         r = fb_mode_to_dss_mode(var, &mode);
> >         if (r) {
> 
> Moving this code to omapfb_calc_addr() could perhaps be a separate
> patch? I don't see other changes in there.

Ah right. It was leftover from an earlier patch with different logic in
the SETUP_PLANE ioctl. But it's still probable easier on the eyes to
have it in a separate function. I'll split the patch up.

> 
> >  static struct device_attribute omapfb_attrs[] = {
> > diff --git a/drivers/video/omap2/omapfb/omapfb.h b/drivers/video/omap2/omapfb/omapfb.h
> > index cd54fdb..ce15533 100644
> > --- a/drivers/video/omap2/omapfb/omapfb.h
> > +++ b/drivers/video/omap2/omapfb/omapfb.h
> 
> >  static inline int omapfb_overlay_enable(struct omap_overlay *ovl,
> > -               int enable)
> > +                                       int enable)
> >  {
> >         struct omap_overlay_info info;
> > +       int r;
> > 
> >         ovl->get_overlay_info(ovl, &info);
> > +       if (info.enabled = enable)
> > +               return 0;
> >         info.enabled = enable;
> > -       return ovl->set_overlay_info(ovl, &info);
> > +       r = ovl->set_overlay_info(ovl, &info);
> > +       if (r)
> > +               return r;
> > +
> > +       return 0;
> > +}
> 
> There are perhaps more changes here than needed. int r is not needed,
> and the last lines do not need to be changed. And is if (info.enabled =
> enable) required by this patch, or is it just optimization? If
> optimization, it could be in separate patch.

That too was leftover from an earlier version which require more logic
here. I'll split it up as well.

> >  #endif
> > diff --git a/include/linux/omapfb.h b/include/linux/omapfb.h
> > index 9bdd914..5de473c 100644
> > --- a/include/linux/omapfb.h
> > +++ b/include/linux/omapfb.h
> > @@ -85,6 +85,9 @@
> >  #define OMAPFB_MEMTYPE_SRAM            1
> >  #define OMAPFB_MEMTYPE_MAX             1
> > 
> > +#define OMAPFB_MEM_IDX_ENABLED 0x8
> > +#define OMAPFB_MEM_IDX_MASK    0x7
> 
> Should enabled be 0x80, and mask 0x7f? There are never that many mem
> indexes, but is there any point in limiting the value?

Hmm. Right. The original clone_idx enable on rover was 0x4 for some
reason. My idea clearly was to move it to the msb but apparently my
brain skipped a beat there and I forgot to add the 0s. Will fix.

-- 
Ville Syrjälä

Re: [PATCH v3 1/4] DSS2: OMAPFB: Add support for switching memory

[Imre Deak]

Hi,

couple of minor comments inlined.

On Fri, Mar 05, 2010 at 02:26:19PM +0100, Syrjala Ville (Nokia-D/Helsinki) wrote:
> From: Ville Syrjälä <ville.syrjala@nokia.com>
> 
> Separate the memory region from the framebuffer device a little bit.
> It's now possible to select the memory region used by the framebuffer
> device using the new mem_idx parameter of omapfb_plane_info. If the
> mem_idx is specified it will be interpreted as an index into the
> memory regions array, if it's not specified the framebuffer's index is
> used instead. So by default each framebuffer keeps using it's own
> memory region which preserves backwards compatibility.
> 
> This allows cloning the same memory region to several overlays and yet
> each overlay can be controlled independently since they can be
> associated with separate framebuffer devices.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@nokia.com>
> ---
> Changes since v2:
> * Removed the use_count and rely on just counting all active overlays. A bit racy
>   but no chance of getting stuck in a state where memory allocation can't be changed.
> * s/source_idx/mem_idx as that seems to be a little more self explanatory
> [...]
> 
>  drivers/video/omap2/omapfb/omapfb-ioctl.c |  163 ++++++++++++++++++++-----
>  drivers/video/omap2/omapfb/omapfb-main.c  |  184 +++++++++++++++++++----------
>  drivers/video/omap2/omapfb/omapfb-sysfs.c |   60 ++++++++--
>  drivers/video/omap2/omapfb/omapfb.h       |   38 ++++++-
>  include/linux/omapfb.h                    |    5 +-
>  5 files changed, 339 insertions(+), 111 deletions(-)
> 
> diff --git a/drivers/video/omap2/omapfb/omapfb-ioctl.c b/drivers/video/omap2/omapfb/omapfb-ioctl.c
> index 1ffa760..cd00bdc 100644
> --- a/drivers/video/omap2/omapfb/omapfb-ioctl.c
> +++ b/drivers/video/omap2/omapfb/omapfb-ioctl.c
> [...]
>  static int omapfb_query_plane(struct fb_info *fbi, struct omapfb_plane_info *pi)
>  {
>         struct omapfb_info *ofbi = FB2OFB(fbi);
> +       struct omap_overlay *ovl = ofbi->overlays[0];
> +       struct omap_overlay_info *ovli = &ovl->info;
> 
>         if (ofbi->num_overlays != 1) {
>                 memset(pi, 0, sizeof(*pi));
>         } else {
> -               struct omap_overlay_info *ovli;
> -               struct omap_overlay *ovl;
> -
> -               ovl = ofbi->overlays[0];
> -               ovli = &ovl->info;
> -

Is this really necessary?

>                 pi->pos_x = ovli->pos_x;
>                 pi->pos_y = ovli->pos_y;
>                 pi->enabled = ovli->enabled;
>                 pi->channel_out = 0; /* xxx */
>                 pi->mirror = 0;
> +               pi->mem_idx = get_mem_idx(ofbi);
>                 pi->out_width = ovli->out_width;
>                 pi->out_height = ovli->out_height;
>         }
> @@ -115,30 +184,57 @@ static int omapfb_setup_mem(struct fb_info *fbi, struct omapfb_mem_info *mi)
>         struct omapfb_info *ofbi = FB2OFB(fbi);
>         struct omapfb2_device *fbdev = ofbi->fbdev;
>         struct omapfb2_mem_region *rg;
> -       int r, i;
> +       int r = 0;
>         size_t size;
> +       int i;
> 
>         if (mi->type > OMAPFB_MEMTYPE_MAX)
>                 return -EINVAL;
> 
>         size = PAGE_ALIGN(mi->size);
> 
> -       rg = &ofbi->region;
> +       rg = ofbi->region;
> 
> -       for (i = 0; i < ofbi->num_overlays; i++) {
> -               if (ofbi->overlays[i]->info.enabled)
> -                       return -EBUSY;
> +       /* FIXME probably should be a rwsem ... */
> +       mutex_lock(&rg->mtx);
> +       while (rg->ref) {
> +               mutex_unlock(&rg->mtx);
> +               schedule();
> +               mutex_lock(&rg->mtx);
> +       }

Yes, rwsem would mean no unnecessary scheduling and also make things
clearer.

> [...]
>  static int omapfb_query_mem(struct fb_info *fbi, struct omapfb_mem_info *mi)
> @@ -146,12 +242,15 @@ 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 = &ofbi->region;
>         memset(mi, 0, sizeof(*mi));
> 
> +       rg = omapfb_get_mem_region(ofbi->region);

At some other users of region I haven't seen omapfb_get_mem_region,
like store_mirror, store_overlays_rotate.

It wouldn't have been nice to have this patch in smaller chunks. For example
one for converting all region. to region-> and another one for adding the
locking for it, then the rest.

Otherwise it looks ok to me.

--Imre

Re: [PATCH v3 1/4] DSS2: OMAPFB: Add support for switching memory

[Ville Syrjälä]

On Wed, Mar 17, 2010 at 06:34:07PM +0100, Deak Imre (Nokia-D/Helsinki) wrote:
> Hi,
> 
> couple of minor comments inlined.
> 
> On Fri, Mar 05, 2010 at 02:26:19PM +0100, Syrjala Ville (Nokia-D/Helsinki) wrote:
> > From: Ville Syrjälä <ville.syrjala@nokia.com>
> > 
> > Separate the memory region from the framebuffer device a little bit.
> > It's now possible to select the memory region used by the framebuffer
> > device using the new mem_idx parameter of omapfb_plane_info. If the
> > mem_idx is specified it will be interpreted as an index into the
> > memory regions array, if it's not specified the framebuffer's index is
> > used instead. So by default each framebuffer keeps using it's own
> > memory region which preserves backwards compatibility.
> > 
> > This allows cloning the same memory region to several overlays and yet
> > each overlay can be controlled independently since they can be
> > associated with separate framebuffer devices.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@nokia.com>
> > ---
> > Changes since v2:
> > * Removed the use_count and rely on just counting all active overlays. A bit racy
> >   but no chance of getting stuck in a state where memory allocation can't be changed.
> > * s/source_idx/mem_idx as that seems to be a little more self explanatory
> > [...]
> > 
> >  drivers/video/omap2/omapfb/omapfb-ioctl.c |  163 ++++++++++++++++++++-----
> >  drivers/video/omap2/omapfb/omapfb-main.c  |  184 +++++++++++++++++++----------
> >  drivers/video/omap2/omapfb/omapfb-sysfs.c |   60 ++++++++--
> >  drivers/video/omap2/omapfb/omapfb.h       |   38 ++++++-
> >  include/linux/omapfb.h                    |    5 +-
> >  5 files changed, 339 insertions(+), 111 deletions(-)
> > 
> > diff --git a/drivers/video/omap2/omapfb/omapfb-ioctl.c b/drivers/video/omap2/omapfb/omapfb-ioctl.c
> > index 1ffa760..cd00bdc 100644
> > --- a/drivers/video/omap2/omapfb/omapfb-ioctl.c
> > +++ b/drivers/video/omap2/omapfb/omapfb-ioctl.c
> > [...]
> >  static int omapfb_query_plane(struct fb_info *fbi, struct omapfb_plane_info *pi)
> >  {
> >         struct omapfb_info *ofbi = FB2OFB(fbi);
> > +       struct omap_overlay *ovl = ofbi->overlays[0];
> > +       struct omap_overlay_info *ovli = &ovl->info;
> > 
> >         if (ofbi->num_overlays != 1) {
> >                 memset(pi, 0, sizeof(*pi));
> >         } else {
> > -               struct omap_overlay_info *ovli;
> > -               struct omap_overlay *ovl;
> > -
> > -               ovl = ofbi->overlays[0];
> > -               ovli = &ovl->info;
> > -
> 
> Is this really necessary?

Nope. Leftovers from some earlier stuff I think.

> 
> >                 pi->pos_x = ovli->pos_x;
> >                 pi->pos_y = ovli->pos_y;
> >                 pi->enabled = ovli->enabled;
> >                 pi->channel_out = 0; /* xxx */
> >                 pi->mirror = 0;
> > +               pi->mem_idx = get_mem_idx(ofbi);
> >                 pi->out_width = ovli->out_width;
> >                 pi->out_height = ovli->out_height;
> >         }
> > @@ -115,30 +184,57 @@ static int omapfb_setup_mem(struct fb_info *fbi, struct omapfb_mem_info *mi)
> >         struct omapfb_info *ofbi = FB2OFB(fbi);
> >         struct omapfb2_device *fbdev = ofbi->fbdev;
> >         struct omapfb2_mem_region *rg;
> > -       int r, i;
> > +       int r = 0;
> >         size_t size;
> > +       int i;
> > 
> >         if (mi->type > OMAPFB_MEMTYPE_MAX)
> >                 return -EINVAL;
> > 
> >         size = PAGE_ALIGN(mi->size);
> > 
> > -       rg = &ofbi->region;
> > +       rg = ofbi->region;
> > 
> > -       for (i = 0; i < ofbi->num_overlays; i++) {
> > -               if (ofbi->overlays[i]->info.enabled)
> > -                       return -EBUSY;
> > +       /* FIXME probably should be a rwsem ... */
> > +       mutex_lock(&rg->mtx);
> > +       while (rg->ref) {
> > +               mutex_unlock(&rg->mtx);
> > +               schedule();
> > +               mutex_lock(&rg->mtx);
> > +       }
> 
> Yes, rwsem would mean no unnecessary scheduling and also make things
> clearer.

Just tried it and seems to be mostly OK. We get lockdep checking as a
bonus. It didn't like setup_plane taking the same rwsem twice so I
added a check to see if the old and new regions are the same and just
lock once in that case. I thought rwsem was supposed to be OK with
read recursion but perhaps I was mitaken, or perhaps it's just lockdep
that's misbehaving.

> 
> > [...]
> >  static int omapfb_query_mem(struct fb_info *fbi, struct omapfb_mem_info *mi)
> > @@ -146,12 +242,15 @@ 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 = &ofbi->region;
> >         memset(mi, 0, sizeof(*mi));
> > 
> > +       rg = omapfb_get_mem_region(ofbi->region);
> 
> At some other users of region I haven't seen omapfb_get_mem_region,
> like store_mirror, store_overlays_rotate.

Right. I apparently just missed them. I now have a patch that tracks the
lock count too and added a few WARN_ON()s to some of the omapfb-main
functions that assume that the region is locked. That triggered a few
warning during boot which I already knew would not hold the lock since
they were only exectedu during init. But I fixed them anyway to take the
lock and now I can boot without warnings.

> It wouldn't have been nice to have this patch in smaller chunks. For example
> one for converting all region. to region-> and another one for adding the
> locking for it, then the rest.

I've now split it up into smalled chunks and eliminated all the
gratuitous changes. Assuming I can get my emmc working again and no issues
appear during some randr torture tests I will post the new patchset still today.

-- 
Ville Syrjälä

Re: [PATCH v3 1/4] DSS2: OMAPFB: Add support for switching memory

[Imre Deak]

On Wed, Mar 17, 2010 at 09:14:25PM +0100, Syrjala Ville (Nokia-D/Helsinki) wrote:
> On Wed, Mar 17, 2010 at 06:34:07PM +0100, Deak Imre (Nokia-D/Helsinki) wrote:
> > Hi,
> > 
> > couple of minor comments inlined.
> > 
> > On Fri, Mar 05, 2010 at 02:26:19PM +0100, Syrjala Ville (Nokia-D/Helsinki) wrote:
> > [...]
> > > @@ -115,30 +184,57 @@ static int omapfb_setup_mem(struct fb_info *fbi, struct omapfb_mem_info *mi)
> > >         struct omapfb_info *ofbi = FB2OFB(fbi);
> > >         struct omapfb2_device *fbdev = ofbi->fbdev;
> > >         struct omapfb2_mem_region *rg;
> > > -       int r, i;
> > > +       int r = 0;
> > >         size_t size;
> > > +       int i;
> > > 
> > >         if (mi->type > OMAPFB_MEMTYPE_MAX)
> > >                 return -EINVAL;
> > > 
> > >         size = PAGE_ALIGN(mi->size);
> > > 
> > > -       rg = &ofbi->region;
> > > +       rg = ofbi->region;
> > > 
> > > -       for (i = 0; i < ofbi->num_overlays; i++) {
> > > -               if (ofbi->overlays[i]->info.enabled)
> > > -                       return -EBUSY;
> > > +       /* FIXME probably should be a rwsem ... */
> > > +       mutex_lock(&rg->mtx);
> > > +       while (rg->ref) {
> > > +               mutex_unlock(&rg->mtx);
> > > +               schedule();
> > > +               mutex_lock(&rg->mtx);
> > > +       }
> > 
> > Yes, rwsem would mean no unnecessary scheduling and also make things
> > clearer.
> 
> Just tried it and seems to be mostly OK. We get lockdep checking as a
> bonus. It didn't like setup_plane taking the same rwsem twice so I
> added a check to see if the old and new regions are the same and just
> lock once in that case. I thought rwsem was supposed to be OK with
> read recursion but perhaps I was mitaken, or perhaps it's just lockdep
> that's misbehaving.

Ah ok, so it's not so obvious change. Nested read locks could really lead
to a deadlock I think. A read lock will block if there is a write waiter
in the queue to avoid write starvation..

--Imre

Re: [PATCH v3 1/4] DSS2: OMAPFB: Add support for switching memory

[Ville Syrjälä]

On Thu, Mar 18, 2010 at 09:52:39AM +0100, Deak Imre (Nokia-D/Helsinki) wrote:
> On Wed, Mar 17, 2010 at 09:14:25PM +0100, Syrjala Ville (Nokia-D/Helsinki) wrote:
> > On Wed, Mar 17, 2010 at 06:34:07PM +0100, Deak Imre (Nokia-D/Helsinki) wrote:
> > > Hi,
> > > 
> > > couple of minor comments inlined.
> > > 
> > > On Fri, Mar 05, 2010 at 02:26:19PM +0100, Syrjala Ville (Nokia-D/Helsinki) wrote:
> > > [...]
> > > > @@ -115,30 +184,57 @@ static int omapfb_setup_mem(struct fb_info *fbi, struct omapfb_mem_info *mi)
> > > >         struct omapfb_info *ofbi = FB2OFB(fbi);
> > > >         struct omapfb2_device *fbdev = ofbi->fbdev;
> > > >         struct omapfb2_mem_region *rg;
> > > > -       int r, i;
> > > > +       int r = 0;
> > > >         size_t size;
> > > > +       int i;
> > > > 
> > > >         if (mi->type > OMAPFB_MEMTYPE_MAX)
> > > >                 return -EINVAL;
> > > > 
> > > >         size = PAGE_ALIGN(mi->size);
> > > > 
> > > > -       rg = &ofbi->region;
> > > > +       rg = ofbi->region;
> > > > 
> > > > -       for (i = 0; i < ofbi->num_overlays; i++) {
> > > > -               if (ofbi->overlays[i]->info.enabled)
> > > > -                       return -EBUSY;
> > > > +       /* FIXME probably should be a rwsem ... */
> > > > +       mutex_lock(&rg->mtx);
> > > > +       while (rg->ref) {
> > > > +               mutex_unlock(&rg->mtx);
> > > > +               schedule();
> > > > +               mutex_lock(&rg->mtx);
> > > > +       }
> > > 
> > > Yes, rwsem would mean no unnecessary scheduling and also make things
> > > clearer.
> > 
> > Just tried it and seems to be mostly OK. We get lockdep checking as a
> > bonus. It didn't like setup_plane taking the same rwsem twice so I
> > added a check to see if the old and new regions are the same and just
> > lock once in that case. I thought rwsem was supposed to be OK with
> > read recursion but perhaps I was mitaken, or perhaps it's just lockdep
> > that's misbehaving.
> 
> Ah ok, so it's not so obvious change. Nested read locks could really lead
> to a deadlock I think. A read lock will block if there is a write waiter
> in the queue to avoid write starvation..

Yes but I think in out case it should be fine because if we hit this:

 t  thread 1     thread 2
 |
 |  down_read(0)
 |               down_write(1)
 v  down_read(1)

then thread 2 will eventually do a up_write() without taking any
other region rwsem, and thread 1 can then continue.

The other locks we have to worry about are the fb_info mutex which
should always be taken before any region lock, and mmap_sem which in
mmap() is taken before the region lock. I'm hoping there are no
mmap_sem users lurking in the driver that already hold the region
rwsem.

-- 
Ville Syrjälä

Re: [PATCH v3 1/4] DSS2: OMAPFB: Add support for switching memory

[Imre Deak]

On Thu, Mar 18, 2010 at 04:26:04PM +0100, Syrjala Ville (Nokia-D/Helsinki) wrote:
> [...]
> > > Just tried it and seems to be mostly OK. We get lockdep checking as a
> > > bonus. It didn't like setup_plane taking the same rwsem twice so I
> > > added a check to see if the old and new regions are the same and just
> > > lock once in that case. I thought rwsem was supposed to be OK with
> > > read recursion but perhaps I was mitaken, or perhaps it's just lockdep
> > > that's misbehaving.
> > 
> > Ah ok, so it's not so obvious change. Nested read locks could really lead
> > to a deadlock I think. A read lock will block if there is a write waiter
> > in the queue to avoid write starvation..
> 
> Yes but I think in out case it should be fine because if we hit this:
> 
>  t  thread 1     thread 2
>  |
>  |  down_read(0)
>  |               down_write(1)
>  v  down_read(1)
> 
> then thread 2 will eventually do a up_write() without taking any
> other region rwsem, and thread 1 can then continue.

Yes and things will work fine with the extra ordering you added. But
lockdep was right in that without the ordering you can get - the not
too likely - scenario:

t thread 1        thread 2       thread 3      thread 4
| down_read(0)
|                 down_write(0)
|                                down_read(1)
|                                              down_write(1)
| down_read(1)
v                                down_read(0)

--Imre

Re: [PATCH v3 1/4] DSS2: OMAPFB: Add support for switching memory

[Ville Syrjälä]

On Fri, Mar 19, 2010 at 08:46:04AM +0100, Deak Imre (Nokia-D/Helsinki) wrote:
> On Thu, Mar 18, 2010 at 04:26:04PM +0100, Syrjala Ville (Nokia-D/Helsinki) wrote:
> > [...]
> > > > Just tried it and seems to be mostly OK. We get lockdep checking as a
> > > > bonus. It didn't like setup_plane taking the same rwsem twice so I
> > > > added a check to see if the old and new regions are the same and just
> > > > lock once in that case. I thought rwsem was supposed to be OK with
> > > > read recursion but perhaps I was mitaken, or perhaps it's just lockdep
> > > > that's misbehaving.
> > > 
> > > Ah ok, so it's not so obvious change. Nested read locks could really lead
> > > to a deadlock I think. A read lock will block if there is a write waiter
> > > in the queue to avoid write starvation..
> > 
> > Yes but I think in out case it should be fine because if we hit this:
> > 
> >  t  thread 1     thread 2
> >  |
> >  |  down_read(0)
> >  |               down_write(1)
> >  v  down_read(1)
> > 
> > then thread 2 will eventually do a up_write() without taking any
> > other region rwsem, and thread 1 can then continue.
> 
> Yes and things will work fine with the extra ordering you added. But
> lockdep was right in that without the ordering you can get - the not
> too likely - scenario:
> 
> t thread 1        thread 2       thread 3      thread 4
> | down_read(0)
> |                 down_write(0)
> |                                down_read(1)
> |                                              down_write(1)
> | down_read(1)
> v                                down_read(0)

Right. I didn't actually consider the case with so many threads. It's
good that lockdep was smarter than me :)

-- 
Ville Syrjälä