[PATCH 0/3] FB: Add window positioning support

[PATCH 0/3] FB: Add window positioning support

[Ajay Kumar]

This patchset adds support for positioning the overlay windows on the LCD.

These patches are based on the discussion about adding features to fbdev at:
http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg06292.html

To Florian:
  	include: fb: Add definiton for window positioning structure
  	video: s3c-fb: Modify s3c-fb driver to support window positioning

To Kukjin,
  	ARM: SAMSUNG: Embed window positioning data structure in s3c-fb plat
  		      data

 arch/arm/plat-samsung/include/plat/fb.h |    3 ++
 drivers/video/s3c-fb.c                  |   37 ++++++++++++++++++++++++++----
 include/linux/fb.h                      |    7 ++++++
 3 files changed, 42 insertions(+), 5 deletions(-)

[PATCH 1/3] include: fb: Add definiton for window positioning structure

[Ajay Kumar]

This patch adds a data structure definiton to hold framebuffer windows/planes.
An ioctl number is also added to provide user access
to change window position dynamically.

Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
Signed-off-by: Banajit Goswami <banajit.g@samsung.com>
Suggested-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 include/linux/fb.h |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/include/linux/fb.h b/include/linux/fb.h
index 1d6836c..2141941 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -39,6 +39,7 @@
 #define FBIOPUT_MODEINFO        0x4617
 #define FBIOGET_DISPINFO        0x4618
 #define FBIO_WAITFORVSYNC	_IOW('F', 0x20, __u32)
+#define FBIOPOS_OVERLAY_WIN	_IOW('F', 0x21, struct fb_overlay_win_pos)
 
 #define FB_TYPE_PACKED_PIXELS		0	/* Packed Pixels	*/
 #define FB_TYPE_PLANES			1	/* Non interleaved planes */
@@ -366,6 +367,12 @@ struct fb_image {
 	struct fb_cmap cmap;	/* color map info */
 };
 
+/* Window overlaying */
+struct fb_overlay_win_pos {
+	__u32 win_pos_x;	/* x-offset from LCD(0,0) where window starts */
+	__u32 win_pos_y;	/* y-offset from LCD(0,0) where window starts */
+};
+
 /*
  * hardware cursor control
  */
-- 
1.7.0.4

[PATCH 3/3] video: s3c-fb: Modify s3c-fb driver to support window

[Ajay Kumar]

Positions the framebuffer window during driver initialization,
using the platform data.It also adds an ioctl definition
to change window position dynamically.

Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
Signed-off-by: Banajit Goswami <banajit.g@samsung.com>
Suggested-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/video/s3c-fb.c |   37 ++++++++++++++++++++++++++++++++-----
 1 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
index 0fda252..b087a12 100644
--- a/drivers/video/s3c-fb.c
+++ b/drivers/video/s3c-fb.c
@@ -441,6 +441,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 fb_overlay_win_pos *winpos = &win->windata->winpos;
 	struct s3c_fb *sfb = win->parent;
 	void __iomem *regs = sfb->regs;
 	void __iomem *buf = regs;
@@ -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));
 
@@ -998,9 +1000,11 @@ static int s3c_fb_ioctl(struct fb_info *info, unsigned int cmd,
 			unsigned long arg)
 {
 	struct s3c_fb_win *win = info->par;
+	struct fb_overlay_win_pos *winpos = &win->windata->winpos;
 	struct s3c_fb *sfb = win->parent;
-	int ret;
+	int ret = 0;
 	u32 crtc;
+	u32 data;
 
 	switch (cmd) {
 	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 FBIOPOS_OVERLAY_WIN:
+		if (copy_from_user(winpos, (u32 __user *)arg,
+					sizeof(struct fb_overlay_win_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);
+		break;
 	default:
 		ret = -ENOTTY;
 	}
-- 
1.7.0.4

[PATCH 2/3] ARM: SAMSUNG: Embed window positioning data structure in

[Ajay Kumar]

This patch adds a new field in s3c_fb_pd_win structure,
thereby allowing the user to place window at the desired position.

Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
Signed-off-by: Banajit Goswami <banajit.g@samsung.com>
Suggested-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 arch/arm/plat-samsung/include/plat/fb.h |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-samsung/include/plat/fb.h b/arch/arm/plat-samsung/include/plat/fb.h
index 0fedf47..920decd 100644
--- a/arch/arm/plat-samsung/include/plat/fb.h
+++ b/arch/arm/plat-samsung/include/plat/fb.h
@@ -27,6 +27,7 @@
  * @win_mode: The display parameters to initialise (not for window 0)
  * @virtual_x: The virtual X size.
  * @virtual_y: The virtual Y size.
+ * @winpos: Position of overlayed window w.r.t the LCD screen.
  */
 struct s3c_fb_pd_win {
 	struct fb_videomode	win_mode;
@@ -35,6 +36,8 @@ struct s3c_fb_pd_win {
 	unsigned short		max_bpp;
 	unsigned short		virtual_x;
 	unsigned short		virtual_y;
+
+	struct fb_overlay_win_pos	winpos;
 };
 
 /**
-- 
1.7.0.4

Re: [PATCH 1/3] include: fb: Add definiton for window positioning

[Tomi Valkeinen]

On Tue, 2011-09-20 at 11:30 -0400, Ajay Kumar wrote:
> This patch adds a data structure definiton to hold framebuffer windows/planes.
> An ioctl number is also added to provide user access
> to change window position dynamically.
> 
> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> Signed-off-by: Banajit Goswami <banajit.g@samsung.com>
> Suggested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  include/linux/fb.h |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index 1d6836c..2141941 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -39,6 +39,7 @@
>  #define FBIOPUT_MODEINFO        0x4617
>  #define FBIOGET_DISPINFO        0x4618
>  #define FBIO_WAITFORVSYNC	_IOW('F', 0x20, __u32)
> +#define FBIOPOS_OVERLAY_WIN	_IOW('F', 0x21, struct fb_overlay_win_pos)
>  
>  #define FB_TYPE_PACKED_PIXELS		0	/* Packed Pixels	*/
>  #define FB_TYPE_PLANES			1	/* Non interleaved planes */
> @@ -366,6 +367,12 @@ struct fb_image {
>  	struct fb_cmap cmap;	/* color map info */
>  };
>  
> +/* Window overlaying */
> +struct fb_overlay_win_pos {
> +	__u32 win_pos_x;	/* x-offset from LCD(0,0) where window starts */
> +	__u32 win_pos_y;	/* y-offset from LCD(0,0) where window starts */
> +};

Shouldn't this also include the window size (in case scaling is
supported)?

This also won't work for setups where the same framebuffer is used by
multiple overlays. For example, this is the case on OMAP when the same
content is cloned to, say, LCD and TV, each of which is showing an
overlay.

 Tomi

Re: [PATCH 1/3] include: fb: Add definiton for window positioning

[Baruch Siach]

Hi Ajay,

On Tue, Sep 20, 2011 at 11:30:39AM -0400, Ajay Kumar wrote:
> This patch adds a data structure definiton to hold framebuffer windows/planes.
> An ioctl number is also added to provide user access
> to change window position dynamically.

[snip]

> +/* Window overlaying */
> +struct fb_overlay_win_pos {
> +	__u32 win_pos_x;	/* x-offset from LCD(0,0) where window starts */
> +	__u32 win_pos_y;	/* y-offset from LCD(0,0) where window starts */
> +};

Why not allow negative offsets where the left or upper part of the framebuffer 
is hidden?

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCH 1/3] include: fb: Add definiton for window positioning structure

[Ajay kumar]

Hi Tomi,

On Tue, Sep 20, 2011 at 4:40 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Tue, 2011-09-20 at 11:30 -0400, Ajay Kumar wrote:
>> This patch adds a data structure definiton to hold framebuffer windows/planes.
>> An ioctl number is also added to provide user access
>> to change window position dynamically.
>>
>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
>> Signed-off-by: Banajit Goswami <banajit.g@samsung.com>
>> Suggested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>  include/linux/fb.h |    7 +++++++
>>  1 files changed, 7 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/linux/fb.h b/include/linux/fb.h
>> index 1d6836c..2141941 100644
>> --- a/include/linux/fb.h
>> +++ b/include/linux/fb.h
>> @@ -39,6 +39,7 @@
>>  #define FBIOPUT_MODEINFO        0x4617
>>  #define FBIOGET_DISPINFO        0x4618
>>  #define FBIO_WAITFORVSYNC    _IOW('F', 0x20, __u32)
>> +#define FBIOPOS_OVERLAY_WIN  _IOW('F', 0x21, struct fb_overlay_win_pos)
>>
>>  #define FB_TYPE_PACKED_PIXELS                0       /* Packed Pixels        */
>>  #define FB_TYPE_PLANES                       1       /* Non interleaved planes */
>> @@ -366,6 +367,12 @@ struct fb_image {
>>       struct fb_cmap cmap;    /* color map info */
>>  };
>>
>> +/* Window overlaying */
>> +struct fb_overlay_win_pos {
>> +     __u32 win_pos_x;        /* x-offset from LCD(0,0) where window starts */
>> +     __u32 win_pos_y;        /* y-offset from LCD(0,0) where window starts */
>> +};
>
> Shouldn't this also include the window size (in case scaling is
> supported)?

The "xres" and "yres" fields in fb_var_screeninfo are being used to
represent the size of the window (visible resolution). So we have,

win_pos_x: x-offset from LCD(0,0) where window starts.
win_pos_y: y-offset from LCD(0,0) where window starts.
(win_pos_x + xres) : x-offset from LCD(0,0) where window ends.
(win_pos_y + yres) : y-offset from LCD(0,0) where window ends.

> This also won't work for setups where the same framebuffer is used by
> multiple overlays. For example, this is the case on OMAP when the same
> content is cloned to, say, LCD and TV, each of which is showing an
> overlay.

These x and y position are used to configure the display controller
(for LCD only) and not to alter the data in physical buffer
(framebuffer). Could you elaborate the above use case you have
mentioned and how adding the x and y offsets would not meet that
requirement.

>  Tomi
>

Ajay

Re: [PATCH 1/3] include: fb: Add definiton for window positioning structure

[Ajay kumar]

Hi Baruch,

On Tue, Sep 20, 2011 at 4:54 PM, Baruch Siach <baruch@tkos.co.il> wrote:
> Hi Ajay,
>
> On Tue, Sep 20, 2011 at 11:30:39AM -0400, Ajay Kumar wrote:
>> This patch adds a data structure definiton to hold framebuffer windows/planes.
>> An ioctl number is also added to provide user access
>> to change window position dynamically.
>
> [snip]
>
>> +/* Window overlaying */
>> +struct fb_overlay_win_pos {
>> +     __u32 win_pos_x;        /* x-offset from LCD(0,0) where window starts */
>> +     __u32 win_pos_y;        /* y-offset from LCD(0,0) where window starts */
>> +};
>
> Why not allow negative offsets where the left or upper part of the framebuffer
> is hidden?

Thanks for pointing it out. Are there drivers which place the overlay
windows such that some part of the window is hidden from being
displayed on the screen?

> baruch
>
> --
>                                                     ~. .~   Tk Open Systems
> =}------------------------------------------------ooO--U--Ooo------------{>   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
> --
> 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
>

Ajay

Re: [PATCH 1/3] include: fb: Add definiton for window positioning

[Tomi Valkeinen]

On Tue, 2011-09-20 at 20:16 +0530, Ajay kumar wrote:
> Hi Tomi,
> 
> On Tue, Sep 20, 2011 at 4:40 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Tue, 2011-09-20 at 11:30 -0400, Ajay Kumar wrote:
> >> This patch adds a data structure definiton to hold framebuffer windows/planes.
> >> An ioctl number is also added to provide user access
> >> to change window position dynamically.
> >>
> >> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> >> Signed-off-by: Banajit Goswami <banajit.g@samsung.com>
> >> Suggested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >> ---
> >>  include/linux/fb.h |    7 +++++++
> >>  1 files changed, 7 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/include/linux/fb.h b/include/linux/fb.h
> >> index 1d6836c..2141941 100644
> >> --- a/include/linux/fb.h
> >> +++ b/include/linux/fb.h
> >> @@ -39,6 +39,7 @@
> >>  #define FBIOPUT_MODEINFO        0x4617
> >>  #define FBIOGET_DISPINFO        0x4618
> >>  #define FBIO_WAITFORVSYNC    _IOW('F', 0x20, __u32)
> >> +#define FBIOPOS_OVERLAY_WIN  _IOW('F', 0x21, struct fb_overlay_win_pos)
> >>
> >>  #define FB_TYPE_PACKED_PIXELS                0       /* Packed Pixels        */
> >>  #define FB_TYPE_PLANES                       1       /* Non interleaved planes */
> >> @@ -366,6 +367,12 @@ struct fb_image {
> >>       struct fb_cmap cmap;    /* color map info */
> >>  };
> >>
> >> +/* Window overlaying */
> >> +struct fb_overlay_win_pos {
> >> +     __u32 win_pos_x;        /* x-offset from LCD(0,0) where window starts */
> >> +     __u32 win_pos_y;        /* y-offset from LCD(0,0) where window starts */
> >> +};
> >
> > Shouldn't this also include the window size (in case scaling is
> > supported)?
> 
> The "xres" and "yres" fields in fb_var_screeninfo are being used to
> represent the size of the window (visible resolution). So we have,
> 
> win_pos_x: x-offset from LCD(0,0) where window starts.
> win_pos_y: y-offset from LCD(0,0) where window starts.
> (win_pos_x + xres) : x-offset from LCD(0,0) where window ends.
> (win_pos_y + yres) : y-offset from LCD(0,0) where window ends.

Sure, but the xres and yres tell the _input_ resolution, i.e. how many
pixels are read from the memory. What is missing is the _output_
resolution, which is the size of the window. These are not necessarily
the same, if the system supports scaling.

> > This also won't work for setups where the same framebuffer is used by
> > multiple overlays. For example, this is the case on OMAP when the same
> > content is cloned to, say, LCD and TV, each of which is showing an
> > overlay.
> 
> These x and y position are used to configure the display controller
> (for LCD only) and not to alter the data in physical buffer
> (framebuffer). Could you elaborate the above use case you have
> mentioned and how adding the x and y offsets would not meet that
> requirement.

Nothing wrong with adding x/y offsets, but the problem is in configuring
the two overlays. If the framebuffer data is used by two overlays, each
overlay should be configured separately. And your ioctl does not have
any way to define which overlay is being affected.

Of course, if we specify that a single framebuffer will ever go only to
one output, the problem disappears.

However, even if we specify so, this will make the fbdev a bit weird:
what is x/yres after this patch? In the current fbdev x/yres is the size
of the output, and x/yres are part of video timings. After this patch
this is no longer the case: x/yres will be the size of the overlay. But
the old code will still use x/yres as part of video timings, making
things confusing.

And generally I can't really make my mind about adding these more
complex features. On one hand it would be very nice to have fbdev
supporting overlays and whatnot, but on the other hand, I can't figure
out how to add them properly.

 Tomi

Re: [PATCH 1/3] include: fb: Add definiton for window positioning

[Florian Tobias Schandinat]

On 09/20/2011 03:39 PM, Tomi Valkeinen wrote:
> On Tue, 2011-09-20 at 20:16 +0530, Ajay kumar wrote:
>> Hi Tomi,
>>
>> On Tue, Sep 20, 2011 at 4:40 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>> On Tue, 2011-09-20 at 11:30 -0400, Ajay Kumar wrote:
>>>> This patch adds a data structure definiton to hold framebuffer windows/planes.
>>>> An ioctl number is also added to provide user access
>>>> to change window position dynamically.

Ajay, do you need this urgently or can we delay this one merge window? I don't
think that a week or so is enough to get a consistent API that gets everything
right. So if you have a pressing need to have it within the 3.2 kernel I'd
prefer to do it only for your driver now and adjust it when we get the thing
done, probably in 3.3.

>>>>
>>>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
>>>> Signed-off-by: Banajit Goswami <banajit.g@samsung.com>
>>>> Suggested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>> ---
>>>>  include/linux/fb.h |    7 +++++++
>>>>  1 files changed, 7 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/include/linux/fb.h b/include/linux/fb.h
>>>> index 1d6836c..2141941 100644
>>>> --- a/include/linux/fb.h
>>>> +++ b/include/linux/fb.h
>>>> @@ -39,6 +39,7 @@
>>>>  #define FBIOPUT_MODEINFO        0x4617
>>>>  #define FBIOGET_DISPINFO        0x4618
>>>>  #define FBIO_WAITFORVSYNC    _IOW('F', 0x20, __u32)
>>>> +#define FBIOPOS_OVERLAY_WIN  _IOW('F', 0x21, struct fb_overlay_win_pos)
>>>>
>>>>  #define FB_TYPE_PACKED_PIXELS                0       /* Packed Pixels        */
>>>>  #define FB_TYPE_PLANES                       1       /* Non interleaved planes */
>>>> @@ -366,6 +367,12 @@ struct fb_image {
>>>>       struct fb_cmap cmap;    /* color map info */
>>>>  };
>>>>
>>>> +/* Window overlaying */
>>>> +struct fb_overlay_win_pos {
>>>> +     __u32 win_pos_x;        /* x-offset from LCD(0,0) where window starts */
>>>> +     __u32 win_pos_y;        /* y-offset from LCD(0,0) where window starts */
>>>> +};
>>>
>>> Shouldn't this also include the window size (in case scaling is
>>> supported)?
>>
>> The "xres" and "yres" fields in fb_var_screeninfo are being used to
>> represent the size of the window (visible resolution). So we have,
>>
>> win_pos_x: x-offset from LCD(0,0) where window starts.
>> win_pos_y: y-offset from LCD(0,0) where window starts.
>> (win_pos_x + xres) : x-offset from LCD(0,0) where window ends.
>> (win_pos_y + yres) : y-offset from LCD(0,0) where window ends.
> 
> Sure, but the xres and yres tell the _input_ resolution, i.e. how many
> pixels are read from the memory. What is missing is the _output_
> resolution, which is the size of the window. These are not necessarily
> the same, if the system supports scaling.

I agree, scaling is an issue that should get solved on the way. So adding
u32 width, height;
with an initial/special value of 0 which means just take what the source
width/height is.

>>> This also won't work for setups where the same framebuffer is used by
>>> multiple overlays. For example, this is the case on OMAP when the same
>>> content is cloned to, say, LCD and TV, each of which is showing an
>>> overlay.
>>
>> These x and y position are used to configure the display controller
>> (for LCD only) and not to alter the data in physical buffer
>> (framebuffer). Could you elaborate the above use case you have
>> mentioned and how adding the x and y offsets would not meet that
>> requirement.
> 
> Nothing wrong with adding x/y offsets, but the problem is in configuring
> the two overlays. If the framebuffer data is used by two overlays, each
> overlay should be configured separately. And your ioctl does not have
> any way to define which overlay is being affected.

Did you have a look at the (existing) API [1] Laurent proposed for discovering
the internal connections between the framebuffers (or with any other devices)?
If you agree that it'd be a good idea to use it I feel that we should make the
windowing API more compatible with it. So basically what we want to have as a
window is one or more sunk pads so the pad-index should be also part of the
interface. I'm still confused with how OMAP works when it does not have a "root"
window/framebuffer. Normally I feel that the window position should be a
property of the parent window as this is what the position is relative too. But
if the parent is no framebuffer, should we also include the entity into the
interface to allow configuring things that are nor even framebuffers?
Also I think we need a z-index in case overlays overlap (might happen or?) and
enforcing that all z-indexes are different for the same entity.

> Of course, if we specify that a single framebuffer will ever go only to
> one output, the problem disappears.
> 
> However, even if we specify so, this will make the fbdev a bit weird:
> what is x/yres after this patch? In the current fbdev x/yres is the size
> of the output, and x/yres are part of video timings. After this patch
> this is no longer the case: x/yres will be the size of the overlay. But
> the old code will still use x/yres as part of video timings, making
> things confusing.

As I see it xres/yres (together with xoffset/yoffset) is always the visible part
of the framebuffer. Typically that's also part of the timings as they define
what is visible. With the introduction of overlays (and maybe even for some
hardware anyway) it is no longer always true to have any timings at all. So on
all framebuffer that do not have physical timings the timing interpretation is
useless anyway (I'm thinking about adding a FB_CAP_NOTIMING) and what remains is
the interpretation of xres/yres as visible screen region.

> And generally I can't really make my mind about adding these more
> complex features. On one hand it would be very nice to have fbdev
> supporting overlays and whatnot, but on the other hand, I can't figure
> out how to add them properly.

I don't see it as adding new features, rather unifying what is already there for
easier use. Sure it should be done in a consistent way.


Best regards,

Florian Tobias Schandinat


[1] http://linuxtv.org/downloads/v4l-dvb-apis/media_common.html

Re: [PATCH 1/3] include: fb: Add definiton for window positioning

[Baruch Siach]

Hi Ajay,

On Tue, Sep 20, 2011 at 08:56:57PM +0530, Ajay kumar wrote:
> Hi Baruch,
> On Tue, Sep 20, 2011 at 4:54 PM, Baruch Siach <baruch@tkos.co.il> wrote:
> > Hi Ajay,
> >
> > On Tue, Sep 20, 2011 at 11:30:39AM -0400, Ajay Kumar wrote:
> >> This patch adds a data structure definiton to hold framebuffer windows/planes.
> >> An ioctl number is also added to provide user access
> >> to change window position dynamically.
> >
> > [snip]
> >
> >> +/* Window overlaying */
> >> +struct fb_overlay_win_pos {
> >> +     __u32 win_pos_x;        /* x-offset from LCD(0,0) where window starts */
> >> +     __u32 win_pos_y;        /* y-offset from LCD(0,0) where window starts */
> >> +};
> >
> > Why not allow negative offsets where the left or upper part of the framebuffer
> > is hidden?
> 
> Thanks for pointing it out. Are there drivers which place the overlay
> windows such that some part of the window is hidden from being
> displayed on the screen?

I don't know. However, since this is new userspace ABI which should stay 
compatible forever, we should make sure to do it right. Using __s32 instead of 
__u32 won't limit us in the future.

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{   - baruch@tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCH 1/3] include: fb: Add definiton for window positioning

[Tomi Valkeinen]

On Tue, 2011-09-20 at 16:55 +0000, Florian Tobias Schandinat wrote:

> Did you have a look at the (existing) API [1] Laurent proposed for discovering
> the internal connections between the framebuffers (or with any other devices)?

I know the basics of media controller, but I haven't really looked at
the code.

> If you agree that it'd be a good idea to use it I feel that we should make the
> windowing API more compatible with it. So basically what we want to have as a

I can't say if MC should be used for fb or not, but I think something
similar should be used if we want to get overlays etc. supported. But
even with MC (or something else) there, I'm not quite sure how they fit
into the fb model.

I think the core problem here is that in a more complex setup (at least
as I see it for OMAP) the fb should be just a framebuffer in memory,
without any direct relation to hardware. The hardware part (an overlay,
or whichever is the corresponding element) will then use the framebuffer
as the pixel source.

However, the current fb model combines those two things into one. If we
manage to separate them, and add MC or similar, I think it'll work.

> window is one or more sunk pads so the pad-index should be also part of the
> interface. I'm still confused with how OMAP works when it does not have a "root"

Do you mean how the hardware works, or how I've designed omapfb driver?

> window/framebuffer. Normally I feel that the window position should be a
> property of the parent window as this is what the position is relative too. But
> if the parent is no framebuffer, should we also include the entity into the
> interface to allow configuring things that are nor even framebuffers?

Right, I think you're pondering the core problem here =).

On OMAP we have the display (the whole area shown on the panel/tv),
which has video timings and things like that. But no pixel data as such
(a configurable background color is there, though).

And then we have the overlays, which are somewhere on the display, and
the overlays get pixel data from memory (framebuffers).

So in a way we have a contentless root window, but presenting that with
an fb device doesn't feel right. And the fb device would logically be
fb0, and if it couldn't show any content it couldn't be used as default
framebuffer.

> Also I think we need a z-index in case overlays overlap (might happen or?) and
> enforcing that all z-indexes are different for the same entity.

Yes, free z-order is supported in OMAP4. Previous OMAPs had fixed
z-order, although in certain configuration (enable/disable alpha
blending) the fixed z-order does change...

> As I see it xres/yres (together with xoffset/yoffset) is always the visible part
> of the framebuffer. Typically that's also part of the timings as they define
> what is visible. With the introduction of overlays (and maybe even for some
> hardware anyway) it is no longer always true to have any timings at all. So on
> all framebuffer that do not have physical timings the timing interpretation is
> useless anyway (I'm thinking about adding a FB_CAP_NOTIMING) and what remains is
> the interpretation of xres/yres as visible screen region.

For a system where there's always a root window for every output, plus
variable size overlays, FB_CAP_NOTIMING makes sense. But it would still
leave the problem if there's no root window. How to change timings on a
system like OMAP?

 Tomi

Re: [PATCH 1/3] include: fb: Add definiton for window positioning

[Tomi Valkeinen]

On Tue, 2011-09-20 at 20:08 +0300, Baruch Siach wrote:
> Hi Ajay,
> 
> On Tue, Sep 20, 2011 at 08:56:57PM +0530, Ajay kumar wrote:
> > Hi Baruch,
> > On Tue, Sep 20, 2011 at 4:54 PM, Baruch Siach <baruch@tkos.co.il> wrote:
> > > Hi Ajay,
> > >
> > > On Tue, Sep 20, 2011 at 11:30:39AM -0400, Ajay Kumar wrote:
> > >> This patch adds a data structure definiton to hold framebuffer windows/planes.
> > >> An ioctl number is also added to provide user access
> > >> to change window position dynamically.
> > >
> > > [snip]
> > >
> > >> +/* Window overlaying */
> > >> +struct fb_overlay_win_pos {
> > >> +     __u32 win_pos_x;        /* x-offset from LCD(0,0) where window starts */
> > >> +     __u32 win_pos_y;        /* y-offset from LCD(0,0) where window starts */
> > >> +};
> > >
> > > Why not allow negative offsets where the left or upper part of the framebuffer
> > > is hidden?
> > 
> > Thanks for pointing it out. Are there drivers which place the overlay
> > windows such that some part of the window is hidden from being
> > displayed on the screen?
> 
> I don't know. However, since this is new userspace ABI which should stay 
> compatible forever, we should make sure to do it right. Using __s32 instead of 
> __u32 won't limit us in the future.

OMAP HW doesn't allow "funny" things like overlay being outside the
visible area, i.e. negative position or size larger than the display.
And my guess is that hardware rarely allow things like that, as it would
just complicate the hardware without any gain.

Out-of-display-overlays can of course be emulated by the software. But
I'm not sure if it's good to add the complexity in the driver layer, as
it could as well be handled in the userspace.

Then again, a signed value would be future safer ("just in case"), and
if the driver can just reject values it doesn't want to support, there's
no real harm there either.

 Tomi

Re: [PATCH 1/3] include: fb: Add definiton for window positioning structure

[Ajay kumar]

Hi Florian,

On Tue, Sep 20, 2011 at 10:25 PM, Florian Tobias Schandinat
<FlorianSchandinat@gmx.de> wrote:
> On 09/20/2011 03:39 PM, Tomi Valkeinen wrote:
>> On Tue, 2011-09-20 at 20:16 +0530, Ajay kumar wrote:
>>> Hi Tomi,
>>>
>>> On Tue, Sep 20, 2011 at 4:40 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>>> On Tue, 2011-09-20 at 11:30 -0400, Ajay Kumar wrote:
>>>>> This patch adds a data structure definiton to hold framebuffer windows/planes.
>>>>> An ioctl number is also added to provide user access
>>>>> to change window position dynamically.
>
> Ajay, do you need this urgently or can we delay this one merge window? I don't
> think that a week or so is enough to get a consistent API that gets everything
> right. So if you have a pressing need to have it within the 3.2 kernel I'd
> prefer to do it only for your driver now and adjust it when we get the thing
> done, probably in 3.3.

No. I am not in a hurry, and I do not have any issue even if it takes
more time to get a consistent API.

>>>>>
>>>>> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
>>>>> Signed-off-by: Banajit Goswami <banajit.g@samsung.com>
>>>>> Suggested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>>> ---
>>>>>  include/linux/fb.h |    7 +++++++
>>>>>  1 files changed, 7 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/fb.h b/include/linux/fb.h
>>>>> index 1d6836c..2141941 100644
>>>>> --- a/include/linux/fb.h
>>>>> +++ b/include/linux/fb.h
>>>>> @@ -39,6 +39,7 @@
>>>>>  #define FBIOPUT_MODEINFO        0x4617
>>>>>  #define FBIOGET_DISPINFO        0x4618
>>>>>  #define FBIO_WAITFORVSYNC    _IOW('F', 0x20, __u32)
>>>>> +#define FBIOPOS_OVERLAY_WIN  _IOW('F', 0x21, struct fb_overlay_win_pos)
>>>>>
>>>>>  #define FB_TYPE_PACKED_PIXELS                0       /* Packed Pixels        */
>>>>>  #define FB_TYPE_PLANES                       1       /* Non interleaved planes */
>>>>> @@ -366,6 +367,12 @@ struct fb_image {
>>>>>       struct fb_cmap cmap;    /* color map info */
>>>>>  };
>>>>>
>>>>> +/* Window overlaying */
>>>>> +struct fb_overlay_win_pos {
>>>>> +     __u32 win_pos_x;        /* x-offset from LCD(0,0) where window starts */
>>>>> +     __u32 win_pos_y;        /* y-offset from LCD(0,0) where window starts */
>>>>> +};
>>>>
>>>> Shouldn't this also include the window size (in case scaling is
>>>> supported)?
>>>
>>> The "xres" and "yres" fields in fb_var_screeninfo are being used to
>>> represent the size of the window (visible resolution). So we have,
>>>
>>> win_pos_x: x-offset from LCD(0,0) where window starts.
>>> win_pos_y: y-offset from LCD(0,0) where window starts.
>>> (win_pos_x + xres) : x-offset from LCD(0,0) where window ends.
>>> (win_pos_y + yres) : y-offset from LCD(0,0) where window ends.
>>
>> Sure, but the xres and yres tell the _input_ resolution, i.e. how many
>> pixels are read from the memory. What is missing is the _output_
>> resolution, which is the size of the window. These are not necessarily
>> the same, if the system supports scaling.
>
> I agree, scaling is an issue that should get solved on the way. So adding
> u32 width, height;
> with an initial/special value of 0 which means just take what the source
> width/height is.

Do you mean to say the "width" and the "height" fields which you are
suggesting, will represent the "output resolution" which OMAP needs?

>>>> This also won't work for setups where the same framebuffer is used by
>>>> multiple overlays. For example, this is the case on OMAP when the same
>>>> content is cloned to, say, LCD and TV, each of which is showing an
>>>> overlay.
>>>
>>> These x and y position are used to configure the display controller
>>> (for LCD only) and not to alter the data in physical buffer
>>> (framebuffer). Could you elaborate the above use case you have
>>> mentioned and how adding the x and y offsets would not meet that
>>> requirement.
>>
>> Nothing wrong with adding x/y offsets, but the problem is in configuring
>> the two overlays. If the framebuffer data is used by two overlays, each
>> overlay should be configured separately. And your ioctl does not have
>> any way to define which overlay is being affected.
>
> Did you have a look at the (existing) API [1] Laurent proposed for discovering
> the internal connections between the framebuffers (or with any other devices)?
> If you agree that it'd be a good idea to use it I feel that we should make the
> windowing API more compatible with it. So basically what we want to have as a
> window is one or more sunk pads so the pad-index should be also part of the
> interface. I'm still confused with how OMAP works when it does not have a "root"
> window/framebuffer. Normally I feel that the window position should be a
> property of the parent window as this is what the position is relative too. But
> if the parent is no framebuffer, should we also include the entity into the
> interface to allow configuring things that are nor even framebuffers?
> Also I think we need a z-index in case overlays overlap (might happen or?) and
> enforcing that all z-indexes are different for the same entity.
>
>> Of course, if we specify that a single framebuffer will ever go only to
>> one output, the problem disappears.
>>
>> However, even if we specify so, this will make the fbdev a bit weird:
>> what is x/yres after this patch? In the current fbdev x/yres is the size
>> of the output, and x/yres are part of video timings. After this patch
>> this is no longer the case: x/yres will be the size of the overlay. But
>> the old code will still use x/yres as part of video timings, making
>> things confusing.
>
> As I see it xres/yres (together with xoffset/yoffset) is always the visible part
> of the framebuffer. Typically that's also part of the timings as they define
> what is visible. With the introduction of overlays (and maybe even for some
> hardware anyway) it is no longer always true to have any timings at all. So on
> all framebuffer that do not have physical timings the timing interpretation is
> useless anyway (I'm thinking about adding a FB_CAP_NOTIMING) and what remains is
> the interpretation of xres/yres as visible screen region.
>
>> And generally I can't really make my mind about adding these more
>> complex features. On one hand it would be very nice to have fbdev
>> supporting overlays and whatnot, but on the other hand, I can't figure
>> out how to add them properly.
>
> I don't see it as adding new features, rather unifying what is already there for
> easier use. Sure it should be done in a consistent way.
>
>
> Best regards,
>
> Florian Tobias Schandinat
>

Ajay

> [1] http://linuxtv.org/downloads/v4l-dvb-apis/media_common.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 1/3] include: fb: Add definiton for window positioning structure

[Ajay kumar]

On Wed, Sep 21, 2011 at 11:55 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Tue, 2011-09-20 at 20:08 +0300, Baruch Siach wrote:
>> Hi Ajay,
>>
>> On Tue, Sep 20, 2011 at 08:56:57PM +0530, Ajay kumar wrote:
>> > Hi Baruch,
>> > On Tue, Sep 20, 2011 at 4:54 PM, Baruch Siach <baruch@tkos.co.il> wrote:
>> > > Hi Ajay,
>> > >
>> > > On Tue, Sep 20, 2011 at 11:30:39AM -0400, Ajay Kumar wrote:
>> > >> This patch adds a data structure definiton to hold framebuffer windows/planes.
>> > >> An ioctl number is also added to provide user access
>> > >> to change window position dynamically.
>> > >
>> > > [snip]
>> > >
>> > >> +/* Window overlaying */
>> > >> +struct fb_overlay_win_pos {
>> > >> +     __u32 win_pos_x;        /* x-offset from LCD(0,0) where window starts */
>> > >> +     __u32 win_pos_y;        /* y-offset from LCD(0,0) where window starts */
>> > >> +};
>> > >
>> > > Why not allow negative offsets where the left or upper part of the framebuffer
>> > > is hidden?
>> >
>> > Thanks for pointing it out. Are there drivers which place the overlay
>> > windows such that some part of the window is hidden from being
>> > displayed on the screen?
>>
>> I don't know. However, since this is new userspace ABI which should stay
>> compatible forever, we should make sure to do it right. Using __s32 instead of
>> __u32 won't limit us in the future.
>
> OMAP HW doesn't allow "funny" things like overlay being outside the
> visible area, i.e. negative position or size larger than the display.
> And my guess is that hardware rarely allow things like that, as it would
> just complicate the hardware without any gain.
>
> Out-of-display-overlays can of course be emulated by the software. But
> I'm not sure if it's good to add the complexity in the driver layer, as
> it could as well be handled in the userspace.
>
> Then again, a signed value would be future safer ("just in case"), and
> if the driver can just reject values it doesn't want to support, there's
> no real harm there either.

OK. I will consider this and modify it in the next version of patches.

Ajay