video: s3c-fb: Add window positioning support

-----Original Message-----
From: Ajay Kumar [mailto:[email protected]]
Sent: Friday, August 26, 2011 4:52 AM
To: [email protected]; [email protected]; linux- [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: [PATCH 1/2] ARM: SAMSUNG: Add Window Positioning Support for s3c- fb driver

This patch:
--adds a data-structure to hold the current position of windows. --adds an ioctl number to support dynamic positioning the windows.

Signed-off-by Ajay Kumar <[email protected]>

Signed-off-by Banajit Goswami <[email protected]>

---
 arch/arm/plat-samsung/include/plat/fb.h |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

arch/arm/plat- renamed from arch/arm/plat-samsung/include/plat/fb.h +0 -0

samsung/include/plat/fb.h
index bd79c0a..77ed75c 100644

arch/arm/plat-samsung/include/plat/fb.h +1 -0

@@ -22,6 +22,18 @@
  */
 #define S3C_FB_MAX_WIN	(5)
 
+/* struct s3cfb_window_pos

arch/arm/plat-samsung/include/plat/fb.h +3 -1

+
 /**
  * struct s3c_fb_pd_win - per window setup data
  * @win_mode: The display parameters to initialise (not for window 0)

@@ -35,6 +47,8 @@ struct s3c_fb_pd_win {
 	unsigned short		max_bpp;
 	unsigned short		virtual_x;
 	unsigned short		virtual_y;
+
+	struct s3cfb_window_pos		winpos;
 };
 
 /**
--

1.7.0.4

-----Original Message-----
From: Ajay Kumar [mailto:[email protected]]
Sent: Friday, August 26, 2011 4:52 AM
To: [email protected]; [email protected]; linux- [email protected]
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: [PATCH 2/2] video: s3c-fb: Modify s3c-fb driver to support window positioning

This patch modifies the existing s3c-fb driver to provide flexibility to the user to reposition the framebuffer windows.

Signed-off-by Ajay Kumar <[email protected]>

Signed-off-by Banajit Goswami <[email protected]>

---
 drivers/video/s3c-fb.c |   37 ++++++++++++++++++++++++++++++++-----
 1 files changed, 32 insertions(+), 5 deletions(-)

drivers/video/s3c-fb.c +9 -5

index 0fda252..41179d7 100644
@@ -442,6 +442,7 @@ static int s3c_fb_set_par(struct fb_info *info)
 	struct fb_var_screeninfo *var = &info->var;
 	struct s3c_fb_win *win = info->par;
 	struct s3c_fb *sfb = win->parent;
+	struct s3cfb_window_pos *winpos = &win->windata->winpos;
 	void __iomem *regs = sfb->regs;
 	void __iomem *buf = regs;
 	int win_no = win->index;

@@ -539,12 +540,13 @@ static int s3c_fb_set_par(struct fb_info *info)
 
 	/* write 'OSD' registers to control position of framebuffer */
 
-	data = VIDOSDxA_TOPLEFT_X(0) | VIDOSDxA_TOPLEFT_Y(0);
+	data = VIDOSDxA_TOPLEFT_X(winpos->win_pos_x) |
+	       VIDOSDxA_TOPLEFT_Y(winpos->win_pos_y);
 	writel(data, regs + VIDOSD_A(win_no, sfb->variant));
 
-	data = VIDOSDxB_BOTRIGHT_X(s3c_fb_align_word(var->bits_per_pixel,
-						     var->xres - 1)) |
-	       VIDOSDxB_BOTRIGHT_Y(var->yres - 1);
+	data = VIDOSDxB_BOTRIGHT_X((s3c_fb_align_word(var->bits_per_pixel,
+	       (winpos->win_pos_x + var->xres - 1)))) |
+	       VIDOSDxB_BOTRIGHT_Y((winpos->win_pos_y + var->yres - 1));
 
 	writel(data, regs + VIDOSD_B(win_no, sfb->variant));
 

@@ -999,8 +1001,10 @@ static int s3c_fb_ioctl(struct fb_info *info,
 unsigned int cmd,
 {
 	struct s3c_fb_win *win = info->par;
 	struct s3c_fb *sfb = win->parent;
-	int ret;
+	struct s3cfb_window_pos *winpos = &win->windata->winpos;
+	int ret = 0;
 	u32 crtc;
+	u32 data;
 
 	switch (cmd) {

drivers/video/s3c-fb.c +13 -0

 	case FBIO_WAITFORVSYNC:

@@ -1011,6 +1015,29 @@ static int s3c_fb_ioctl(struct fb_info *info,
 unsigned int cmd,
 
 		ret = s3c_fb_wait_for_vsync(sfb, crtc);
 		break;
+	case S3CFB_WIN_POSITION:
+		if (copy_from_user(winpos, (u32 __user *)arg,
+					sizeof(struct s3cfb_window_pos))) {
+			ret = -EFAULT;
+			break;
+		}
+
+		shadow_protect_win(win, 1);
+
+		/* write 'OSD' registers to set position of the window */
+		data = VIDOSDxA_TOPLEFT_X(winpos->win_pos_x) |
+		       VIDOSDxA_TOPLEFT_Y(winpos->win_pos_y);
+		writel(data, sfb->regs + VIDOSD_A(win->index, sfb->variant));

drivers/video/s3c-fb.c +0 -0

 	default:
 		ret = -ENOTTY;

}
--
1.7.0.4

Hi, Ajay.

Quoted 89 lines

-----Original Message-----
From: Ajay Kumar [mailto:ajaykumar.rs at samsung.com]
Sent: Friday, August 26, 2011 4:52 AM
To: linux-samsung-soc at vger.kernel.org; linux-fbdev at vger.kernel.org; linux- arm-kernel at lists.infradead.org
Cc: FlorianSchandinat at gmx.de; lethal at linux-sh.org; jg1.han at samsung.com; m.szyprowski at samsung.com; ben-linux at fluff.org; banajit.g at samsung.com
Subject: [PATCH 2/2] video: s3c-fb: Modify s3c-fb driver to support window positioning

This patch modifies the existing s3c-fb driver to provide flexibility to the user to reposition the framebuffer windows.

Signed-off-by Ajay Kumar <[email protected]>

Signed-off-by Banajit Goswami <[email protected]>

---
?drivers/video/s3c-fb.c | ? 37 ++++++++++++++++++++++++++++++++----- ?1 files changed, 32 insertions(+), 5 deletions(-)

drivers/video/s3c-fb.c +1 -0

index 0fda252..41179d7 100644
@@ -442,6 +442,7 @@ static int s3c_fb_set_par(struct fb_info *info)
 ? ? ? struct fb_var_screeninfo *var = &info->var;
 ? ? ? struct s3c_fb_win *win = info->par;
 ? ? ? struct s3c_fb *sfb = win->parent;
+ ? ? struct s3cfb_window_pos *winpos = &win->windata->winpos;
 ? ? ? void __iomem *regs = sfb->regs;
 ? ? ? void __iomem *buf = regs;
 ? ? ? int win_no = win->index;

@@ -539,12 +540,13 @@ static int s3c_fb_set_par(struct fb_info *info)
 

? ? ? /* write 'OSD' registers to control position of framebuffer */

- ? ? data = VIDOSDxA_TOPLEFT_X(0) | VIDOSDxA_TOPLEFT_Y(0);
+ ? ? data = VIDOSDxA_TOPLEFT_X(winpos->win_pos_x) |
+ ? ? ? ? ? ?VIDOSDxA_TOPLEFT_Y(winpos->win_pos_y); ? ? ? writel(data, regs + VIDOSD_A(win_no, sfb->variant));

- ? ? data = VIDOSDxB_BOTRIGHT_X(s3c_fb_align_word(var->bits_per_pixel,
- ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?var->xres - 1)) |
- ? ? ? ? ? ?VIDOSDxB_BOTRIGHT_Y(var->yres - 1);
+ ? ? data = VIDOSDxB_BOTRIGHT_X((s3c_fb_align_word(var->bits_per_pixel,
+ ? ? ? ? ? ?(winpos->win_pos_x + var->xres - 1)))) |
+ ? ? ? ? ? ?VIDOSDxB_BOTRIGHT_Y((winpos->win_pos_y + var->yres - 1));

? ? ? writel(data, regs + VIDOSD_B(win_no, sfb->variant));

drivers/video/s3c-fb.c +3 -1

@@ -999,8 +1001,10 @@ static int s3c_fb_ioctl(struct fb_info *info,
 unsigned int cmd,
 ?{
 ? ? ? struct s3c_fb_win *win = info->par;
 ? ? ? struct s3c_fb *sfb = win->parent;
- ? ? int ret;
+ ? ? struct s3cfb_window_pos *winpos = &win->windata->winpos;
+ ? ? int ret = 0;
 ? ? ? u32 crtc;
+ ? ? u32 data;
 
 ? ? ? switch (cmd) {

? ? ? case FBIO_WAITFORVSYNC:

drivers/video/s3c-fb.c +22 -0

@@ -1011,6 +1015,29 @@ static int s3c_fb_ioctl(struct fb_info *info,
 unsigned int cmd,
 
 ? ? ? ? ? ? ? ret = s3c_fb_wait_for_vsync(sfb, crtc);
 ? ? ? ? ? ? ? break;
+ ? ? case S3CFB_WIN_POSITION:
+ ? ? ? ? ? ? if (copy_from_user(winpos, (u32 __user *)arg,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? sizeof(struct s3cfb_window_pos))) {
+ ? ? ? ? ? ? ? ? ? ? ret = -EFAULT;
+ ? ? ? ? ? ? ? ? ? ? break;
+ ? ? ? ? ? ? }
+
+ ? ? ? ? ? ? shadow_protect_win(win, 1);
+
+ ? ? ? ? ? ? /* write 'OSD' registers to set position of the window */
+ ? ? ? ? ? ? data = VIDOSDxA_TOPLEFT_X(winpos->win_pos_x) |
+ ? ? ? ? ? ? ? ? ? ?VIDOSDxA_TOPLEFT_Y(winpos->win_pos_y);
+ ? ? ? ? ? ? writel(data, sfb->regs + VIDOSD_A(win->index, sfb->variant));
+
+ ? ? ? ? ? ? data = VIDOSDxB_BOTRIGHT_X(
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? s3c_fb_align_word(info->var.bits_per_pixel,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? (winpos->win_pos_x + info->var.xres - 1)));
+ ? ? ? ? ? ? data |= VIDOSDxB_BOTRIGHT_Y(winpos->win_pos_y +
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? info->var.yres - 1);
+ ? ? ? ? ? ? writel(data, sfb->regs + VIDOSD_B(win->index, sfb->variant));
+
+ ? ? ? ? ? ? shadow_protect_win(win, 0);

drivers/video/s3c-fb.c static int s3c_fb_ioctl(struct fb_info *info, · line 1041

Quoted 1 line

drivers/video/s3c-fb.c +1 -0

+ ? ? ? ? ? ? break;

Can you move this to separate function?
In my opinion, it would be better for readability and consistency. Also, FBIO_WAITFORVSYNC has been implemented in this way.
? ? ? ? ? ? ? ?ret = s3c_fb_wait_for_vsync(sfb, crtc);
How about adding a function 's3c_fb_set_window_position' for this as belows? +int s3c_fb_set_window_position(struct fb_info *info,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct s3cfb_window_pos winpos)

? ? ? ? ? ? ? ?........
+ ? ? ? case S3CFB_WIN_POSITION:
+ ? ? ? ? ? ? ? if (copy_from_user(winpos, (u32 __user *)arg,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? sizeof(struct s3cfb_window_pos))) {
+ ? ? ? ? ? ? ? ? ? ? ? ret = -EFAULT;
+ ? ? ? ? ? ? ? ? ? ? ? break;
+ ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? ? ret = s3c_fb_set_window_position(info, winpos);
+ ? ? ? ? ? ? ? break;

Quoted 5 lines

? ? ? default:
? ? ? ? ? ? ? ret = -ENOTTY;
? ? ? }
--
1.7.0.4

Just as a note, there are many drivers like mx3fb.c, au1200fb.c and OMAP seem to be doing window/plane positioning in their driver code. Is it possible to have this window positioning support at a common place?

For instance, we can have a common struture and ioctl number in include/linux/fb.h as below:

	#define FBIOPOS_OVERLAY_WIN    _IOW('F', 0x21, struct fb_overlay_win_pos)

	struct fb_overlay_win_pos {
		__u32 win_pos_x;  /* x-offset of window from LCD(0,0) */
		__u32 win_pos_y;  /* y-offset of window from LCD(0,0) */
	};

where LCD(0,0) means the first pixel of the LCD screen. Individual drivers can have implementation for this ioctl.

To Kukjin Kim,
[PATCH 1/2] ARM: SAMSUNG: Add Window Positioning Support for s3c-fb driver

To Paul Mundt, Florian Tobias Schandinat
[PATCH 2/2] video: s3c-fb: Modify s3c-fb driver to support window positioning

 arch/arm/plat-samsung/include/plat/fb.h |   14 +++++++++++
 drivers/video/s3c-fb.c                  |   37 ++++++++++++++++++++++++++----
 2 files changed, 46 insertions(+), 5 deletions(-)

Hi all,

Quoted 3 lines · Ajay Kumar

Just as a note, there are many drivers like mx3fb.c, au1200fb.c and OMAP seem to be doing window/plane positioning in their driver code. Is it possible to have this window positioning support at a common place?

Good point. Congratulations for figuring out that I like to standardize things. But I think your suggestion is far from being enough to be useful for userspace (which is our goal so that applications can be reused along drivers and don't need to know about individual drivers).

So let me at first summarize how I understand you implemented those things after having a brief look at some of the drivers:
Windows are rectangular screen areas whose pixel data come from other locations. The other locations are accessible via other framebuffer devices (e.g. fb1). So in this area the data of fb1 is shown and not the data of fb0 that would be normally shown.

So in addition to your proposed positioning I think we should also have the following to give userspace a useful set of functionality:

- a way to discover how the screen is composited (how many windows are there, how they are stacked and how to access those)

- a way to enable/disable windows (make them (in)visible)

- reporting and selecting how the window content can be mixed with the root screen (overwrite, source or destination color keying)

- things like window size and color format could be handled by the usual fb API used on the window. However there might be restrictions which cause them to be not 100% API compatible (for example when changing the color format if the windows are required to have the same format as the root screen)

- do we need to worry about hardware (up/down) scaling of the window content?

So is that what you need for a standardized window implementation? Any additional things that were useful/needed in this context? Would you consider adding support for this API in your drivers? (as standardizing wouldn't be useful if nobody would implement it)

Hi all,

Quoted 3 lines · Ajay Kumar

Just as a note, there are many drivers like mx3fb.c, au1200fb.c and OMAP seem to be doing window/plane positioning in their driver code. Is it possible to have this window positioning support at a common place?

Good point. Congratulations for figuring out that I like to standardize things. But I think your suggestion is far from being enough to be useful for userspace (which is our goal so that applications can be reused along drivers and don't need to know about individual drivers).

So let me at first summarize how I understand you implemented those things after having a brief look at some of the drivers:
Windows are rectangular screen areas whose pixel data come from other locations. The other locations are accessible via other framebuffer devices (e.g. fb1). So in this area the data of fb1 is shown and not the data of fb0 that would be normally shown.

So in addition to your proposed positioning I think we should also have the following to give userspace a useful set of functionality:

- a way to discover how the screen is composited (how many windows are there, how they are stacked and how to access those)

- a way to enable/disable windows (make them (in)visible)

- reporting and selecting how the window content can be mixed with the root screen (overwrite, source or destination color keying)

- things like window size and color format could be handled by the usual fb API used on the window. However there might be restrictions which cause them to be not 100% API compatible (for example when changing the color format if the windows are required to have the same format as the root screen)

- do we need to worry about hardware (up/down) scaling of the window content?

So is that what you need for a standardized window implementation? Any additional things that were useful/needed in this context? Would you consider adding support for this API in your drivers? (as standardizing wouldn't be useful if nobody would implement it)

Best regards,

Florian Tobias Schandinat

Hi all,

Quoted 3 lines · Ajay Kumar

Just as a note, there are many drivers like mx3fb.c, au1200fb.c and OMAP seem to be doing window/plane positioning in their driver code. Is it possible to have this window positioning support at a common place?

Good point. Congratulations for figuring out that I like to standardize things. But I think your suggestion is far from being enough to be useful for userspace (which is our goal so that applications can be reused along drivers and don't need to know about individual drivers).

So let me at first summarize how I understand you implemented those things after having a brief look at some of the drivers:
Windows are rectangular screen areas whose pixel data come from other locations. The other locations are accessible via other framebuffer devices (e.g. fb1). So in this area the data of fb1 is shown and not the data of fb0 that would be normally shown.

So in addition to your proposed positioning I think we should also have the following to give userspace a useful set of functionality:

- a way to discover how the screen is composited (how many windows are there, how they are stacked and how to access those)

- a way to enable/disable windows (make them (in)visible)

- reporting and selecting how the window content can be mixed with the root screen (overwrite, source or destination color keying)

- things like window size and color format could be handled by the usual fb API used on the window. However there might be restrictions which cause them to be not 100% API compatible (for example when changing the color format if the windows are required to have the same format as the root screen)

- do we need to worry about hardware (up/down) scaling of the window content?

So is that what you need for a standardized window implementation? Any additional things that were useful/needed in this context? Would you consider adding support for this API in your drivers? (as standardizing wouldn't be useful if nobody would implement it)

Hi Florian,

Quoted 11 lines · On Thursday 01 September 2011 18:45:18 Florian Tobias Schandinat wrote:

Hi all,

Quoted 3 lines · Ajay Kumar

Just as a note, there are many drivers like mx3fb.c, au1200fb.c and OMAP seem to be doing window/plane positioning in their driver code. Is it possible to have this window positioning support at a common place?

Good point. Congratulations for figuring out that I like to standardize things. But I think your suggestion is far from being enough to be useful for userspace (which is our goal so that applications can be reused along drivers and don't need to know about individual drivers).

Beside standardizing things, do you also like to take them one level higher to solve challenging issues ? I know the answer must be yes :-)

The problem at hand here is something we have solved in V4L2 (theoretically only for part of it) with the media controller API, the V4L2 subdevs and their pad-level format API.

In a nutshell, the media controller lets drivers model hardware as a graph of buliding blocks connected through their pads and expose that description to userspace applications. In V4L2 most of those blocks are V4L2 subdevs, which are abstract building blocks that implement sets of standard operations. Those operations are exposed to userspace through the V4L2 subdevs pad-level format API, allowing application to configure sizes and selection rectangles at all pads in the graph. Selection rectangles can be used to configure cropping and composing, which is exactly what the window positioning API needs to do.

Instead of creating a new fbdev-specific API to do the same, shouldn't we try to join forces ?

Quoted 29 lines · Laurent Pinchart

Quoted 10 lines · On Thursday 01 September 2011 18:45:18 Florian Tobias Schandinat wrote:

Quoted 4 lines · Ajay Kumar

Just as a note, there are many drivers like mx3fb.c, au1200fb.c and OMAP seem to be doing window/plane positioning in their driver code. Is it possible to have this window positioning support at a common place?

Good point. Congratulations for figuring out that I like to standardize things. But I think your suggestion is far from being enough to be useful for userspace (which is our goal so that applications can be reused along drivers and don't need to know about individual drivers).

Beside standardizing things, do you also like to take them one level higher to solve challenging issues ? I know the answer must be yes :-)

The problem at hand here is something we have solved in V4L2 (theoretically only for part of it) with the media controller API, the V4L2 subdevs and their pad-level format API.

In a nutshell, the media controller lets drivers model hardware as a graph of buliding blocks connected through their pads and expose that description to userspace applications. In V4L2 most of those blocks are V4L2 subdevs, which are abstract building blocks that implement sets of standard operations. Those operations are exposed to userspace through the V4L2 subdevs pad-level format API, allowing application to configure sizes and selection rectangles at all pads in the graph. Selection rectangles can be used to configure cropping and composing, which is exactly what the window positioning API needs to do.

Quoted 2 lines

Instead of creating a new fbdev-specific API to do the same, shouldn't we try to join forces ?

Okay, thanks for the pointer. After having a look at your API I understand that it would solve the problem to discover how many windows (in this case) are there and how they can be accessed. It looks fine for this purpose, powerful enough and not too complex. So if I get it correct we still need at least a way to configure the position of the windows/overlays/sink pads similar to what Ajay proposed.

Additionally a way to get and/or set the z-position of the overlays if multiple overlays overlap and set/get how the overlays work (overdraw, constant alpha, source/destination color keying). Normally I'd consider these link properties but I think implementing them as properties of the source framebuffer or sink pad would work as well.
Is this correct or did I miss something?