* [PATCH 1/2] drivers/video: fix ioctls in the Freescale DIU framebuffer driver
From: Timur Tabi @ 2011-09-21 16:54 UTC (permalink / raw)
To: linux-fbdev
Use the _IOx macros to define the ioctl commands, instead of hard-coded
numbers. Unfortunately, the original definitions of MFB_SET_PIXFMT and
MFB_GET_PIXFMT used the wrong value for the size, so this will break
binary compatibility with older applications.
Also remove the FBIOGET_GWINFO and FBIOPUT_GWINFO ioctls. FBIOPUT_GWINFO
was never implemented, and FBIOGET_GWINFO was never used by any
application.
Signed-off-by: Timur Tabi <timur@freescale.com>
---
drivers/video/fsl-diu-fb.c | 8 --------
include/linux/fsl-diu-fb.h | 20 ++++++++------------
2 files changed, 8 insertions(+), 20 deletions(-)
diff --git a/drivers/video/fsl-diu-fb.c b/drivers/video/fsl-diu-fb.c
index 5137fbb..e5905b7 100644
--- a/drivers/video/fsl-diu-fb.c
+++ b/drivers/video/fsl-diu-fb.c
@@ -1030,14 +1030,6 @@ static int fsl_diu_ioctl(struct fb_info *info, unsigned int cmd,
ad->ckmin_b = ck.blue_min;
}
break;
- case FBIOGET_GWINFO:
- if (mfbi->type = MFB_TYPE_OFF)
- return -ENODEV;
- /* get graphic window information */
- if (copy_to_user(buf, ad, sizeof(*ad)))
- return -EFAULT;
- break;
-
default:
dev_err(info->dev, "unknown ioctl command (0x%08X)\n", cmd);
return -ENOIOCTLCMD;
diff --git a/include/linux/fsl-diu-fb.h b/include/linux/fsl-diu-fb.h
index df23f59..5efb407 100644
--- a/include/linux/fsl-diu-fb.h
+++ b/include/linux/fsl-diu-fb.h
@@ -33,22 +33,18 @@ struct mfb_chroma_key {
};
struct aoi_display_offset {
- int x_aoi_d;
- int y_aoi_d;
+ __s32 x_aoi_d;
+ __s32 y_aoi_d;
};
#define MFB_SET_CHROMA_KEY _IOW('M', 1, struct mfb_chroma_key)
#define MFB_SET_BRIGHTNESS _IOW('M', 3, __u8)
-
-#define MFB_SET_ALPHA 0x80014d00
-#define MFB_GET_ALPHA 0x40014d00
-#define MFB_SET_AOID 0x80084d04
-#define MFB_GET_AOID 0x40084d04
-#define MFB_SET_PIXFMT 0x80014d08
-#define MFB_GET_PIXFMT 0x40014d08
-
-#define FBIOGET_GWINFO 0x46E0
-#define FBIOPUT_GWINFO 0x46E1
+#define MFB_SET_ALPHA _IOW('M', 0, __u8)
+#define MFB_GET_ALPHA _IOR('M', 0, __u8)
+#define MFB_SET_AOID _IOW('M', 4, struct aoi_display_offset)
+#define MFB_GET_AOID _IOR('M', 4, struct aoi_display_offset)
+#define MFB_SET_PIXFMT _IOW('M', 8, __u32)
+#define MFB_GET_PIXFMT _IOR('M', 8, __u32)
#ifdef __KERNEL__
#include <linux/spinlock.h>
--
1.7.3.4
^ permalink raw reply related
* Re: Proposal for a low-level Linux display framework
From: Heiko Stübner @ 2011-09-21 13:26 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: linux-fbdev, linux-kernel, dri-devel, linaro-dev, Clark, Rob,
Archit Taneja, Nishant Kamat
In-Reply-To: <1316088425.11294.78.camel@lappyti>
Hi,
Am Donnerstag, 15. September 2011, 14:07:05 schrieb Tomi Valkeinen:
> Now, I'm quite sure the above framework could work quite well with any
> OMAP like hardware, with unified memory (i.e. the video buffers are in
> SDRAM) and 3D chips and similar components are separate. But what I'm
> not sure is how desktop world's gfx cards change things. Most probably
> all the above components can be found from there also in some form, but
> are there some interdependencies between 3D/buffer management/something
> else and the video output side?
If I have read your proposal right, it could also help in better supporting
epaper-displays.
The current drivers each (re-)implement the deferredIo logic needed to drive
such systems (catching and combining updates to the framebuffer) and combine
this with the actual display driver which issues specific commands to the
display hardware. Also a board-specific driver is needed to implement the
actual transport to the display which seems be done via this i80 command
protocol over GPIOs or the LCD controllers of SoCs.
If one were to split this it could be realised like
---------------- --------------- -------------
| deferredIOFb | - | DisplayCtrl | - | Transport |
---------------- --------------- -------------
An interesting tidbit is that on the ereaders I'm working on the LCD
controller should be able to do some sort of pseudo DMA operation.
The normal way to transmit data to epaper displays is:
- issue update command with dimension of the regions to update
- read relevant pixeldata from fb and write it to the i80 in a loop
- issue stop-update command
On the S3C2416 it could go like:
- issue update command
- describe to the lcd controller the source memory region
- let the lcd controller transmit exactly one frame
- issue stop update command
So the transport would need to get the memory addresses of the region to
update from the framebuffer driver.
Also this system could possibly use some of the drawing routines from the
S3C2416 SoC.
Essentially needed would be a s3c-fb with deferredio-addon and the lcd
controller separated from it. I'm still not sure how this all could fit
together but I would guess separating framebuffer and display control would
help.
Heiko
^ permalink raw reply
* [PATCH 49/57] video: irq: Remove IRQF_DISABLED
From: Yong Zhang @ 2011-09-21 9:28 UTC (permalink / raw)
To: linux-arch, linux-kernel
Cc: linux-fbdev, Paul Gortmaker, Laurent Pinchart, Daniel Walker,
Kukjin Kim, Florian Tobias Schandinat, Tomi Valkeinen,
Archit Taneja, David Brown, Anatolij Gustschin, cbe-oss-dev,
Wan ZongShun, linux-arm-msm, yong.zhang0, Ben Dooks, tglx,
linux-omap, linux-arm-kernel, Geoff Levand, Jiri Kosina,
Bryan Huntsman, Andrew Morton, linuxppc-dev
In-Reply-To: <1316597339-29861-1-git-send-email-yong.zhang0@gmail.com>
Since commit [c58543c8: genirq: Run irq handlers with interrupts disabled],
We run all interrupt handlers with interrupts disabled
and we even check and yell when an interrupt handler
returns with interrupts enabled (see commit [b738a50a:
genirq: Warn when handler enables interrupts]).
So now this flag is a NOOP and can be removed.
Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
---
drivers/video/au1200fb.c | 2 +-
drivers/video/bf54x-lq043fb.c | 2 +-
drivers/video/bfin-lq035q1-fb.c | 2 +-
drivers/video/bfin-t350mcqb-fb.c | 2 +-
drivers/video/bfin_adv7393fb.c | 2 +-
drivers/video/mb862xx/mb862xxfbdrv.c | 4 ++--
drivers/video/msm/mddi.c | 2 +-
drivers/video/msm/mdp.c | 2 +-
drivers/video/nuc900fb.c | 2 +-
drivers/video/omap2/displays/panel-taal.c | 2 +-
drivers/video/ps3fb.c | 2 +-
drivers/video/pxa3xx-gcu.c | 2 +-
drivers/video/pxafb.c | 2 +-
drivers/video/s3c2410fb.c | 2 +-
drivers/video/sa1100fb.c | 3 +--
drivers/video/sh_mobile_lcdcfb.c | 2 +-
drivers/video/tmiofb.c | 2 +-
drivers/video/vt8500lcdfb.c | 2 +-
18 files changed, 19 insertions(+), 20 deletions(-)
diff --git a/drivers/video/au1200fb.c b/drivers/video/au1200fb.c
index a19a40e..7200559 100644
--- a/drivers/video/au1200fb.c
+++ b/drivers/video/au1200fb.c
@@ -1673,7 +1673,7 @@ static int __devinit au1200fb_drv_probe(struct platform_device *dev)
/* Now hook interrupt too */
irq = platform_get_irq(dev, 0);
ret = request_irq(irq, au1200fb_handle_irq,
- IRQF_DISABLED | IRQF_SHARED, "lcd", (void *)dev);
+ IRQF_SHARED, "lcd", (void *)dev);
if (ret) {
print_err("fail to request interrupt line %d (err: %d)",
irq, ret);
diff --git a/drivers/video/bf54x-lq043fb.c b/drivers/video/bf54x-lq043fb.c
index 2464b91..56720fb 100644
--- a/drivers/video/bf54x-lq043fb.c
+++ b/drivers/video/bf54x-lq043fb.c
@@ -633,7 +633,7 @@ static int __devinit bfin_bf54x_probe(struct platform_device *pdev)
goto out7;
}
- if (request_irq(info->irq, bfin_bf54x_irq_error, IRQF_DISABLED,
+ if (request_irq(info->irq, bfin_bf54x_irq_error, 0,
"PPI ERROR", info) < 0) {
printk(KERN_ERR DRIVER_NAME
": unable to request PPI ERROR IRQ\n");
diff --git a/drivers/video/bfin-lq035q1-fb.c b/drivers/video/bfin-lq035q1-fb.c
index 23b6c4b..c633068 100644
--- a/drivers/video/bfin-lq035q1-fb.c
+++ b/drivers/video/bfin-lq035q1-fb.c
@@ -695,7 +695,7 @@ static int __devinit bfin_lq035q1_probe(struct platform_device *pdev)
goto out7;
}
- ret = request_irq(info->irq, bfin_lq035q1_irq_error, IRQF_DISABLED,
+ ret = request_irq(info->irq, bfin_lq035q1_irq_error, 0,
DRIVER_NAME" PPI ERROR", info);
if (ret < 0) {
dev_err(&pdev->dev, "unable to request PPI ERROR IRQ\n");
diff --git a/drivers/video/bfin-t350mcqb-fb.c b/drivers/video/bfin-t350mcqb-fb.c
index d8de29f..d5e1267 100644
--- a/drivers/video/bfin-t350mcqb-fb.c
+++ b/drivers/video/bfin-t350mcqb-fb.c
@@ -529,7 +529,7 @@ static int __devinit bfin_t350mcqb_probe(struct platform_device *pdev)
goto out7;
}
- ret = request_irq(info->irq, bfin_t350mcqb_irq_error, IRQF_DISABLED,
+ ret = request_irq(info->irq, bfin_t350mcqb_irq_error, 0,
"PPI ERROR", info);
if (ret < 0) {
printk(KERN_ERR DRIVER_NAME
diff --git a/drivers/video/bfin_adv7393fb.c b/drivers/video/bfin_adv7393fb.c
index 8486f54..811dd7f 100644
--- a/drivers/video/bfin_adv7393fb.c
+++ b/drivers/video/bfin_adv7393fb.c
@@ -481,7 +481,7 @@ static int __devinit bfin_adv7393_fb_probe(struct i2c_client *client,
goto out_4;
}
- if (request_irq(IRQ_PPI_ERROR, ppi_irq_error, IRQF_DISABLED,
+ if (request_irq(IRQ_PPI_ERROR, ppi_irq_error, 0,
"PPI ERROR", fbdev) < 0) {
dev_err(&client->dev, "unable to request PPI ERROR IRQ\n");
ret = -EFAULT;
diff --git a/drivers/video/mb862xx/mb862xxfbdrv.c b/drivers/video/mb862xx/mb862xxfbdrv.c
index 12a634a..11a7a33 100644
--- a/drivers/video/mb862xx/mb862xxfbdrv.c
+++ b/drivers/video/mb862xx/mb862xxfbdrv.c
@@ -738,7 +738,7 @@ static int __devinit of_platform_mb862xx_probe(struct platform_device *ofdev)
if (mb862xx_gdc_init(par))
goto io_unmap;
- if (request_irq(par->irq, mb862xx_intr, IRQF_DISABLED,
+ if (request_irq(par->irq, mb862xx_intr, 0,
DRV_NAME, (void *)par)) {
dev_err(dev, "Cannot request irq\n");
goto io_unmap;
@@ -1074,7 +1074,7 @@ static int __devinit mb862xx_pci_probe(struct pci_dev *pdev,
if (mb862xx_pci_gdc_init(par))
goto io_unmap;
- if (request_irq(par->irq, mb862xx_intr, IRQF_DISABLED | IRQF_SHARED,
+ if (request_irq(par->irq, mb862xx_intr, IRQF_SHARED,
DRV_NAME, (void *)par)) {
dev_err(dev, "Cannot request irq\n");
goto io_unmap;
diff --git a/drivers/video/msm/mddi.c b/drivers/video/msm/mddi.c
index 178b072..4527cbf 100644
--- a/drivers/video/msm/mddi.c
+++ b/drivers/video/msm/mddi.c
@@ -715,7 +715,7 @@ static int __devinit mddi_probe(struct platform_device *pdev)
mddi->int_enable = 0;
mddi_writel(mddi->int_enable, INTEN);
- ret = request_irq(mddi->irq, mddi_isr, IRQF_DISABLED, "mddi",
+ ret = request_irq(mddi->irq, mddi_isr, 0, "mddi",
&mddi->client_data);
if (ret) {
printk(KERN_ERR "mddi: failed to request enable irq!\n");
diff --git a/drivers/video/msm/mdp.c b/drivers/video/msm/mdp.c
index 2750ed2..cb2ddf1 100644
--- a/drivers/video/msm/mdp.c
+++ b/drivers/video/msm/mdp.c
@@ -426,7 +426,7 @@ int mdp_probe(struct platform_device *pdev)
goto error_get_clk;
}
- ret = request_irq(mdp->irq, mdp_isr, IRQF_DISABLED, "msm_mdp", mdp);
+ ret = request_irq(mdp->irq, mdp_isr, 0, "msm_mdp", mdp);
if (ret)
goto error_request_irq;
disable_irq(mdp->irq);
diff --git a/drivers/video/nuc900fb.c b/drivers/video/nuc900fb.c
index 37dd850..d1fbbd8 100644
--- a/drivers/video/nuc900fb.c
+++ b/drivers/video/nuc900fb.c
@@ -587,7 +587,7 @@ static int __devinit nuc900fb_probe(struct platform_device *pdev)
fbinfo->flags = FBINFO_FLAG_DEFAULT;
fbinfo->pseudo_palette = &fbi->pseudo_pal;
- ret = request_irq(irq, nuc900fb_irqhandler, IRQF_DISABLED,
+ ret = request_irq(irq, nuc900fb_irqhandler, 0,
pdev->name, fbinfo);
if (ret) {
dev_err(&pdev->dev, "cannot register irq handler %d -err %d\n",
diff --git a/drivers/video/omap2/displays/panel-taal.c b/drivers/video/omap2/displays/panel-taal.c
index ddc696d..20d9968 100644
--- a/drivers/video/omap2/displays/panel-taal.c
+++ b/drivers/video/omap2/displays/panel-taal.c
@@ -1053,7 +1053,7 @@ static int taal_probe(struct omap_dss_device *dssdev)
gpio_direction_input(gpio);
r = request_irq(gpio_to_irq(gpio), taal_te_isr,
- IRQF_DISABLED | IRQF_TRIGGER_RISING,
+ IRQF_TRIGGER_RISING,
"taal vsync", dssdev);
if (r) {
diff --git a/drivers/video/ps3fb.c b/drivers/video/ps3fb.c
index 65560a1..213fbbc 100644
--- a/drivers/video/ps3fb.c
+++ b/drivers/video/ps3fb.c
@@ -1082,7 +1082,7 @@ static int __devinit ps3fb_probe(struct ps3_system_bus_device *dev)
}
retval = request_irq(ps3fb.irq_no, ps3fb_vsync_interrupt,
- IRQF_DISABLED, DEVICE_NAME, &dev->core);
+ 0, DEVICE_NAME, &dev->core);
if (retval) {
dev_err(&dev->core, "%s: request_irq failed %d\n", __func__,
retval);
diff --git a/drivers/video/pxa3xx-gcu.c b/drivers/video/pxa3xx-gcu.c
index d8de557..1ed8b36 100644
--- a/drivers/video/pxa3xx-gcu.c
+++ b/drivers/video/pxa3xx-gcu.c
@@ -676,7 +676,7 @@ pxa3xx_gcu_probe(struct platform_device *dev)
}
ret = request_irq(irq, pxa3xx_gcu_handle_irq,
- IRQF_DISABLED, DRV_NAME, priv);
+ 0, DRV_NAME, priv);
if (ret) {
dev_err(&dev->dev, "request_irq failed\n");
ret = -EBUSY;
diff --git a/drivers/video/pxafb.c b/drivers/video/pxafb.c
index 0f4e8c9..e89778f 100644
--- a/drivers/video/pxafb.c
+++ b/drivers/video/pxafb.c
@@ -2191,7 +2191,7 @@ static int __devinit pxafb_probe(struct platform_device *dev)
goto failed_free_mem;
}
- ret = request_irq(irq, pxafb_handle_irq, IRQF_DISABLED, "LCD", fbi);
+ ret = request_irq(irq, pxafb_handle_irq, 0, "LCD", fbi);
if (ret) {
dev_err(&dev->dev, "request_irq failed: %d\n", ret);
ret = -EBUSY;
diff --git a/drivers/video/s3c2410fb.c b/drivers/video/s3c2410fb.c
index 798144a..ee4c0df 100644
--- a/drivers/video/s3c2410fb.c
+++ b/drivers/video/s3c2410fb.c
@@ -910,7 +910,7 @@ static int __devinit s3c24xxfb_probe(struct platform_device *pdev,
for (i = 0; i < 256; i++)
info->palette_buffer[i] = PALETTE_BUFF_CLEAR;
- ret = request_irq(irq, s3c2410fb_irq, IRQF_DISABLED, pdev->name, info);
+ ret = request_irq(irq, s3c2410fb_irq, 0, pdev->name, info);
if (ret) {
dev_err(&pdev->dev, "cannot get irq %d - err %d\n", irq, ret);
ret = -EBUSY;
diff --git a/drivers/video/sa1100fb.c b/drivers/video/sa1100fb.c
index e8b76d6..98d55d0 100644
--- a/drivers/video/sa1100fb.c
+++ b/drivers/video/sa1100fb.c
@@ -1457,8 +1457,7 @@ static int __devinit sa1100fb_probe(struct platform_device *pdev)
if (ret)
goto failed;
- ret = request_irq(irq, sa1100fb_handle_irq, IRQF_DISABLED,
- "LCD", fbi);
+ ret = request_irq(irq, sa1100fb_handle_irq, 0, "LCD", fbi);
if (ret) {
printk(KERN_ERR "sa1100fb: request_irq failed: %d\n", ret);
goto failed;
diff --git a/drivers/video/sh_mobile_lcdcfb.c b/drivers/video/sh_mobile_lcdcfb.c
index 4636f9d..facffc2 100644
--- a/drivers/video/sh_mobile_lcdcfb.c
+++ b/drivers/video/sh_mobile_lcdcfb.c
@@ -1577,7 +1577,7 @@ static int __devinit sh_mobile_lcdc_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, priv);
- error = request_irq(i, sh_mobile_lcdc_irq, IRQF_DISABLED,
+ error = request_irq(i, sh_mobile_lcdc_irq, 0,
dev_name(&pdev->dev), priv);
if (error) {
dev_err(&pdev->dev, "unable to request irq\n");
diff --git a/drivers/video/tmiofb.c b/drivers/video/tmiofb.c
index cd1c4dc..8e4a446 100644
--- a/drivers/video/tmiofb.c
+++ b/drivers/video/tmiofb.c
@@ -744,7 +744,7 @@ static int __devinit tmiofb_probe(struct platform_device *dev)
goto err_ioremap_vram;
}
- retval = request_irq(irq, &tmiofb_irq, IRQF_DISABLED,
+ retval = request_irq(irq, &tmiofb_irq, 0,
dev_name(&dev->dev), info);
if (retval)
diff --git a/drivers/video/vt8500lcdfb.c b/drivers/video/vt8500lcdfb.c
index c13c246..777c21d 100644
--- a/drivers/video/vt8500lcdfb.c
+++ b/drivers/video/vt8500lcdfb.c
@@ -355,7 +355,7 @@ static int __devinit vt8500lcd_probe(struct platform_device *pdev)
goto failed_free_palette;
}
- ret = request_irq(irq, vt8500lcd_handle_irq, IRQF_DISABLED, "LCD", fbi);
+ ret = request_irq(irq, vt8500lcd_handle_irq, 0, "LCD", fbi);
if (ret) {
dev_err(&pdev->dev, "request_irq failed: %d\n", ret);
ret = -EBUSY;
--
1.7.4.1
^ permalink raw reply related
* Re: [PATCH] fsl-diu-fb: remove the ioctl interface
From: Anatolij Gustschin @ 2011-09-21 8:29 UTC (permalink / raw)
To: Tabi Timur-B04825; +Cc: linuxppc-dev@ozlabs.org, linux-fbdev@vger.kernel.org
In-Reply-To: <CAOZdJXUZsByLorWZNtDRMzsezF_qVX62+8zfiMKJqMMhCD2i3g@mail.gmail.com>
On Wed, 21 Sep 2011 02:10:42 +0000
Tabi Timur-B04825 <B04825@freescale.com> wrote:
...
> The definitions of MFB_SET_PIXFMT and MFB_GET_PIXFMT are wrong:
>
> #define MFB_SET_PIXFMT 0x80014d08
> #define MFB_GET_PIXFMT 0x40014d08
>
> The "01" is the size. However, these ioctls take a __u32 as a parameter.
>
> This means that I have to fix the definitions to this:
>
> #define MFB_SET_PIXFMT _IOW('M', 8, __u32)
> #define MFB_GET_PIXFMT _IOR('M', 8, __u32)
>
> This will change the values and break binary compatibility with your
> applications. Are you okay with that?
yes. the app will be fixed for updated kernel.
> > Other ioctls can be removed. I'm not sure if someone
> > uses FBIOGET_GWINFO. If there are no objections from
> > other people, it can also be dropped.
>
> I'm going to remove FBIOGET_GWINFO because no one is using it, and the
> ioctl value is malformed (it doesn't define a direction or size).
okay.
Thanks,
Anatolij
^ permalink raw reply
* Re: [PATCH 1/3] include: fb: Add definiton for window positioning structure
From: Ajay kumar @ 2011-09-21 7:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1316586336.1949.14.camel@deskari>
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
^ permalink raw reply
* Re: [PATCH 1/3] include: fb: Add definiton for window positioning structure
From: Ajay kumar @ 2011-09-21 7:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <4E78C579.9060305@gmx.de>
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
>
^ permalink raw reply
* [patch] smscufx: change edid data to u8 instead of char
From: Dan Carpenter @ 2011-09-21 7:16 UTC (permalink / raw)
To: linux-fbdev
Having "edid" as char caused a problem in ufx_read_edid() where we
compared "edid[i] != 0xFF". Because of the type difference, the
condition was never true and the error checking failed.
Also I added a __user notation to silence a sparse complaint.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Compile tested only. Please test carefully.
diff --git a/drivers/video/smscufx.c b/drivers/video/smscufx.c
index 44c8cab..aaccffa 100644
--- a/drivers/video/smscufx.c
+++ b/drivers/video/smscufx.c
@@ -103,7 +103,7 @@ struct ufx_data {
struct delayed_work free_framebuffer_work;
atomic_t usb_active; /* 0 = update virtual buffer, but no usb traffic */
atomic_t lost_pixels; /* 1 = a render op failed. Need screen refresh */
- char *edid; /* null until we read edid from hw or get from sysfs */
+ u8 *edid; /* null until we read edid from hw or get from sysfs */
size_t edid_size;
u32 pseudo_palette[256];
};
@@ -993,7 +993,7 @@ static int ufx_ops_ioctl(struct fb_info *info, unsigned int cmd,
/* TODO: Update X server to get this from sysfs instead */
if (cmd = UFX_IOCTL_RETURN_EDID) {
- char *edid = (char *)arg;
+ u8 __user *edid = (u8 __user *)arg;
if (copy_to_user(edid, dev->edid, dev->edid_size))
return -EFAULT;
return 0;
@@ -1428,7 +1428,7 @@ static int ufx_i2c_wait_busy(struct ufx_data *dev)
}
/* reads a 128-byte EDID block from the currently selected port and TAR */
-static int ufx_read_edid(struct ufx_data *dev, char *edid, int edid_len)
+static int ufx_read_edid(struct ufx_data *dev, u8 *edid, int edid_len)
{
int i, j, status;
u32 *edid_u32 = (u32 *)edid;
@@ -1491,7 +1491,7 @@ static int ufx_setup_modes(struct ufx_data *dev, struct fb_info *info,
char *default_edid, size_t default_edid_size)
{
const struct fb_videomode *default_vmode = NULL;
- char *edid;
+ u8 *edid;
int i, result = 0, tries = 3;
if (info->dev) /* only use mutex if info has been registered */
^ permalink raw reply related
* Re: [PATCH 1/3] include: fb: Add definiton for window positioning
From: Tomi Valkeinen @ 2011-09-21 6:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20110920170858.GA3827@tarshish>
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
^ permalink raw reply
* Re: Proposal for a low-level Linux display framework
From: Tomi Valkeinen @ 2011-09-21 6:01 UTC (permalink / raw)
To: Patrik Jakobsson
Cc: Keith Packard, Alan Cox, linux-fbdev, linux-kernel, dri-devel,
linaro-dev, Clark, Rob, Archit Taneja
In-Reply-To: <CAMeQTsazfb6xoA6mm8OMVhkx_RDaxogqEa8yDstqmKUP5Ub+yg@mail.gmail.com>
On Tue, 2011-09-20 at 23:20 +0200, Patrik Jakobsson wrote:
> Ok, not sure I understand the complexity of DSI. Can overlay composition
> occur after/at the DSI stage (through MCS perhaps)? Or is it a matter of
> panels requiring special scanout buffer formats that for instance V4L needs
> to know about in order to overlay stuff properly? Or am I getting it all wrong?
I don't know what MCS is. But DSI is just a bi-directional transfer
protocol between the SoC and the peripheral, you can send arbitrary data
over it.
Normally the SoC composes a pixel stream using overlays and whatnot,
which goes to the DSI hardware, which then serializes the data and sends
it to the peripheral.
But you can as well send any data, like commands, and the peripheral can
respond with any relevant data.
Tomi
^ permalink raw reply
* Re: [PATCH] fsl-diu-fb: remove the ioctl interface
From: Tabi Timur-B04825 @ 2011-09-21 2:10 UTC (permalink / raw)
To: Anatolij Gustschin; +Cc: linuxppc-dev@ozlabs.org, linux-fbdev@vger.kernel.org
In-Reply-To: <20110622221143.503add74@wker>
Anatolij,
I'm sorry to resurrect an old thread, but ...
On Wed, Jun 22, 2011 at 3:11 PM, Anatolij Gustschin <agust@denx.de> wrote:
> On Tue, 21 Jun 2011 23:13:11 +0000
> Tabi Timur-B04825 <B04825@freescale.com> wrote:
>
>> Anatolij Gustschin wrote:
>> > No! We are using ioctl interface of this driver in many video
>> > rendering applications on overlay planes on huge number of boards.
>> > So, please don't remove it.
>>
>> Ok, I had no idea anyone was using it.
>>
>> Can you email me details about how you use the ioctl interface? If I can't
>> remove it, maybe I can clean it up.
>
> Following DIU specific ioctls are used:
>
> MFB_SET_CHROMA_KEY
> MFB_SET_PIXFMT
> MFB_GET_PIXFMT
> MFB_SET_AOID
> MFB_GET_AOID
> MFB_GET_ALPHA
> MFB_SET_ALPHA
The definitions of MFB_SET_PIXFMT and MFB_GET_PIXFMT are wrong:
#define MFB_SET_PIXFMT 0x80014d08
#define MFB_GET_PIXFMT 0x40014d08
The "01" is the size. However, these ioctls take a __u32 as a parameter.
This means that I have to fix the definitions to this:
#define MFB_SET_PIXFMT _IOW('M', 8, __u32)
#define MFB_GET_PIXFMT _IOR('M', 8, __u32)
This will change the values and break binary compatibility with your
applications. Are you okay with that?
> Other ioctls can be removed. I'm not sure if someone
> uses FBIOGET_GWINFO. If there are no objections from
> other people, it can also be dropped.
I'm going to remove FBIOGET_GWINFO because no one is using it, and the
ioctl value is malformed (it doesn't define a direction or size).
--
Timur Tabi
Linux kernel developer at Freescale
^ permalink raw reply
* Re: Proposal for a low-level Linux display framework
From: Laurent Pinchart @ 2011-09-20 23:32 UTC (permalink / raw)
To: Rob Clark
Cc: Alan Cox, Keith Packard, linaro-dev, Florian Tobias Schandinat,
linux-kernel, dri-devel, Archit Taneja, linux-fbdev, Alex Deucher,
linux-media
In-Reply-To: <CAN_cFWNkdYEFaeCmJcsPnrt+hoiOZuZEQ6qbrcXWQT_3NSNoLw@mail.gmail.com>
Hi Alan and Rob,
On Monday 19 September 2011 02:09:36 Rob Clark wrote:
> On Sun, Sep 18, 2011 at 5:23 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> >> This would leave us with the issue of controlling formats and other
> >> parameters on the pipelines. We could keep separate DRM, KMS, FB and
> >> V4L APIs for that,
> >
> > There are some other differences that matter. The exact state and
> > behaviour of memory, sequencing of accesses, cache control and management
> > are a critical part of DRM for most GPUs, as is the ability to have them
> > in swap backed objects and to do memory management of them. Fences and
> > the like are a big part of the logic of many renderers and the same
> > fencing has to be applied between capture and GPU, and also in some cases
> > between playback accelerators (eg MP4 playback) and GPU.
That's why I believe the DRM API is our best solution to address all those
issues.
I'm not advocating merging the DRM, FB and V4L APIs for memory management.
What I would like to investigate is whether we can use a common API for the
common needs, which are (in my opinion):
- reporting the entities that make up the graphics pipeline (such as planes,
overlays, compositors, transmitters, connectors, ...), especially when
pipelines get more complex than the plane->crtc->encoder->connector DRM model
- configuring data routing in those complex pipelines
- and possibly configuring formats (pixel format, frame size, crop rectangle,
composition rectangle, ...) on those entities
> > To glue them together I think you'd need to support the use of GEM
> > objects (maybe extended) in V4L. That may actually make life cleaner and
> > simpler in some respects because GEM objects are refcounted nicely and
> > have handles.
>
> fwiw, I think the dmabuf proposal that linaro GWG is working on should
> be sufficient for V4L to capture directly into a GEM buffer that can
> be scanned out (overlay) or composited by GPU, etc, in cases where the
> different dma initiators can all access some common memory:
>
> http://lists.linaro.org/pipermail/linaro-mm-sig/2011-September/000616.html
>
> The idea is that you could allocate a GEM buffer, export a dmabuf
> handle for that buffer that could be passed to v4l2 camera device (ie.
> V4L2_MEMORY_DMABUF), video encoder, etc.. the importing device should
> bracket DMA to/from the buffer w/ get/put_scatterlist() so an unused
> buffer could be unpinned if needed.
I second Rob here, I think that API should be enough to solve our memory
sharing problems between different devices. This is a bit out of scope though,
as neither the low-level Linux display framework proposal nor my comments
target that, but it's an important topic worth mentioning.
> > DRM and KMS abstract out stuff into what is akin to V4L subdevices for
> > the various objects the video card has that matter for display - from
> > scanout buffers to the various video outputs, timings and the like.
> >
> > I don't know what it's like with OMAP but for some of the x86 stuff
> > particularly low speed/power stuff the capture devices, GPU and overlays
> > tend to be fairly incestuous in order to do things like 1080i/p preview
> > while recording from the camera.
>
> We don't like extra memcpy's, but something like dmabuf fits us
> nicely.. and I expect it would work well in any sort of UMA system
> where camera, encoder, GPU, overlay, etc all can share the same memory
> and formats. I suspect the situation is similar in the x86 SoC
> world.. but would be good to get some feedback on the proposal. (I
> guess next version of the RFC would go out to more mailing lists for
> broader review.)
>
> > GPU is also a bit weird in some ways because while its normally
> > nonsensical to do things like use the capture facility one card to drive
> > part of another, it's actually rather useful (although not supported
> > really by DRM) to do exactly that with GPUs. A simple example is a dual
> > headed box with a dumb frame buffer and an accelerated output both of
> > which are using memory that can be hit by the accelerated card. Classic
> > example being a USB plug in monitor.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH] fbdev: sh_mobile_lcdc: Turn dot clock on before resuming from runtime PM
From: Laurent Pinchart @ 2011-09-20 23:03 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1310387754-23033-1-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Florian,
On Wednesday 21 September 2011 00:44:10 Laurent Pinchart wrote:
> On Tuesday 20 September 2011 22:19:25 Florian Tobias Schandinat wrote:
> > On 09/20/2011 07:56 PM, Laurent Pinchart wrote:
> > > On Tuesday 20 September 2011 21:52:36 Florian Tobias Schandinat wrote:
> > >> On 09/20/2011 07:30 PM, Laurent Pinchart wrote:
> > >>> On Monday 11 July 2011 14:35:54 Laurent Pinchart wrote:
> > >>>> Resuming from runtime PM restores all LCDC registers. If the dot
> > >>>> clock is off at that time display panning information will be
> > >>>> corrupted.
> > >>>>
> > >>>> Turn the dot clock on before resuming from runtime PM. Similarly,
> > >>>> turn the clock off after suspending the LCDC.
> > >>>
> > >>> Could you please pick this patch for v3.2 ?
> > >>
> > >> I assume you meant the currently developed 3.1? (as 3.2 is the next
> > >> one for which the patch is lying in my fbdev-next anyway)
> > >
> > > No, I meant v3.2. As I haven't received any pull notification in
> > > response to the patch, and as I had no way to check your fbdev-next
> > > tree on kernel.org, I just wanted to make sure the patch would be
> > > queued for v3.2.
> >
> > Just for your info, my tree [1] never was on kernel.org, you can see the
> > patch in it here [2] (webfrontend). Looks like the patch I got come from
> > your pull request "SH mobile LCDC cleanups and fixes", hope that's the
> > right version.
>
> Oops, my bad.
>
> I'm rebasing my pending patches on top of your fbdev/fbdev-next tree. There
> are a couple of conflicts, I'll resent the rebased patches to the list.
Conflicts are only in the WIP/RFC patches. Those are not meant for upstream
inclusion now, I'll resend them later after getting feedback.
The only pending patches are about the new YUV API. They don't conflict with
your fbdev-next branch.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH v2 0/4] sh_mobile_meram cleanups and fixes
From: Laurent Pinchart @ 2011-09-20 22:59 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1316546490-4639-1-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Florian,
On Tuesday 20 September 2011 21:21:26 Laurent Pinchart wrote:
> Hi,
>
> Here's the second version of the sh_mobile_meram and cleanup fixes.
> Compared to v1, I've incorporated Damian's comments in the register
> definitions (first patch).
>
> Laurent Pinchart (4):
> fbdev: sh_mobile_meram: Replace hardcoded register values with macros
> fbdev: sh_mobile_meram: Validate ICB configuration outside mutex
> fbdev: sh_mobile_meram: Fix MExxCTL register save on runtime PM
> suspend
> fbdev: sh_mobile_meram: Remove unneeded sh_mobile_meram.h
This version conflicts with your fbdev-next branch in which you've already
merged v1. Changes between v1 and v2 are cosmetic only, I'll send a patch
later to be applied on top of your fbdev-next branch. You can thus ignore
these patches.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH] fbdev: sh_mobile_lcdc: Turn dot clock on before resuming from runtime PM
From: Laurent Pinchart @ 2011-09-20 22:44 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1310387754-23033-1-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Florian,
On Tuesday 20 September 2011 22:19:25 Florian Tobias Schandinat wrote:
> On 09/20/2011 07:56 PM, Laurent Pinchart wrote:
> > On Tuesday 20 September 2011 21:52:36 Florian Tobias Schandinat wrote:
> >> On 09/20/2011 07:30 PM, Laurent Pinchart wrote:
> >>> On Monday 11 July 2011 14:35:54 Laurent Pinchart wrote:
> >>>> Resuming from runtime PM restores all LCDC registers. If the dot clock
> >>>> is off at that time display panning information will be corrupted.
> >>>>
> >>>> Turn the dot clock on before resuming from runtime PM. Similarly,
> >>>> turn the clock off after suspending the LCDC.
> >>>
> >>> Could you please pick this patch for v3.2 ?
> >>
> >> I assume you meant the currently developed 3.1? (as 3.2 is the next one
> >> for which the patch is lying in my fbdev-next anyway)
> >
> > No, I meant v3.2. As I haven't received any pull notification in response
> > to the patch, and as I had no way to check your fbdev-next tree on
> > kernel.org, I just wanted to make sure the patch would be queued for
> > v3.2.
>
> Just for your info, my tree [1] never was on kernel.org, you can see the
> patch in it here [2] (webfrontend). Looks like the patch I got come from
> your pull request "SH mobile LCDC cleanups and fixes", hope that's the
> right version.
Oops, my bad.
I'm rebasing my pending patches on top of your fbdev/fbdev-next tree. There
are a couple of conflicts, I'll resent the rebased patches to the list.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [patch 1/1] fb: fix potential deadlock between lock_fb_info and
From: Andrea Righi @ 2011-09-20 22:20 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <201109202111.p8KLBAJD019239@wpaz13.hot.corp.google.com>
On Tue, Sep 20, 2011 at 09:46:08PM +0000, Florian Tobias Schandinat wrote:
> Hi Andrew,
>
> On 09/20/2011 09:11 PM, akpm@google.com wrote:
> > From: Andrea Righi <arighi@develer.com>
> > Subject: fb: fix potential deadlock between lock_fb_info and console_lock
> >
> > fb_set_suspend() must be called with the console semaphore held, which
> > means the code path coming in here will first take the console_lock() and
> > then call lock_fb_info().
> >
> > However several framebuffer ioctl commands acquire these locks in reverse
> > order (lock_fb_info() and then console_lock()). This gives rise to
> > potential AB-BA deadlock.
> >
> > Fix this by changing the order of acquisition in the ioctl commands that
> > make use of console_lock().
>
> I already have another patch [1] that fixes the same issue in a different way.
> It looks less risky than yours and got more feedback.
>
>
> Best regards,
>
> Florian Tobias Schandinat
>
>
> [1] http://marc.info/?l=linux-kernel&m\x130833638508657&w=2
Looks better than my version.
For what it's worth it:
Reviewed-by: Andrea Righi <andrea@betterlinux.com>
Thanks,
-Andrea
^ permalink raw reply
* Re: [patch 1/1] fb: fix potential deadlock between lock_fb_info and
From: Florian Tobias Schandinat @ 2011-09-20 21:46 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <201109202111.p8KLBAJD019239@wpaz13.hot.corp.google.com>
Hi Andrew,
On 09/20/2011 09:11 PM, akpm@google.com wrote:
> From: Andrea Righi <arighi@develer.com>
> Subject: fb: fix potential deadlock between lock_fb_info and console_lock
>
> fb_set_suspend() must be called with the console semaphore held, which
> means the code path coming in here will first take the console_lock() and
> then call lock_fb_info().
>
> However several framebuffer ioctl commands acquire these locks in reverse
> order (lock_fb_info() and then console_lock()). This gives rise to
> potential AB-BA deadlock.
>
> Fix this by changing the order of acquisition in the ioctl commands that
> make use of console_lock().
I already have another patch [1] that fixes the same issue in a different way.
It looks less risky than yours and got more feedback.
Best regards,
Florian Tobias Schandinat
[1] http://marc.info/?l=linux-kernel&m\x130833638508657&w=2
>
> Signed-off-by: Andrea Righi <arighi@develer.com>
> Reported-by: Peter Nordström (Palm GBU) <peter.nordstrom@palm.com>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: <stable@kernel.org>
>
> Signed-off-by: Andrew Morton <akpm@google.com>
> ---
>
> drivers/video/fbmem.c | 24 +++++++++++++++---------
> 1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff -puN drivers/video/fbmem.c~fb-fix-potential-deadlock-between-lock_fb_info-and-console_lock drivers/video/fbmem.c
> --- a/drivers/video/fbmem.c~fb-fix-potential-deadlock-between-lock_fb_info-and-console_lock
> +++ a/drivers/video/fbmem.c
> @@ -1076,14 +1076,16 @@ static long do_fb_ioctl(struct fb_info *
> case FBIOPUT_VSCREENINFO:
> if (copy_from_user(&var, argp, sizeof(var)))
> return -EFAULT;
> - if (!lock_fb_info(info))
> - return -ENODEV;
> console_lock();
> + if (!lock_fb_info(info)) {
> + console_unlock();
> + return -ENODEV;
> + }
> info->flags |= FBINFO_MISC_USEREVENT;
> ret = fb_set_var(info, &var);
> info->flags &= ~FBINFO_MISC_USEREVENT;
> - console_unlock();
> unlock_fb_info(info);
> + console_unlock();
> if (!ret && copy_to_user(argp, &var, sizeof(var)))
> ret = -EFAULT;
> break;
> @@ -1112,12 +1114,14 @@ static long do_fb_ioctl(struct fb_info *
> case FBIOPAN_DISPLAY:
> if (copy_from_user(&var, argp, sizeof(var)))
> return -EFAULT;
> - if (!lock_fb_info(info))
> - return -ENODEV;
> console_lock();
> + if (!lock_fb_info(info)) {
> + console_unlock();
> + return -ENODEV;
> + }
> ret = fb_pan_display(info, &var);
> - console_unlock();
> unlock_fb_info(info);
> + console_unlock();
> if (ret = 0 && copy_to_user(argp, &var, sizeof(var)))
> return -EFAULT;
> break;
> @@ -1159,14 +1163,16 @@ static long do_fb_ioctl(struct fb_info *
> unlock_fb_info(info);
> break;
> case FBIOBLANK:
> - if (!lock_fb_info(info))
> - return -ENODEV;
> console_lock();
> + if (!lock_fb_info(info)) {
> + console_unlock();
> + return -ENODEV;
> + }
> info->flags |= FBINFO_MISC_USEREVENT;
> ret = fb_blank(info, arg);
> info->flags &= ~FBINFO_MISC_USEREVENT;
> - console_unlock();
> unlock_fb_info(info);
> + console_unlock();
> break;
> default:
> if (!lock_fb_info(info))
> _
>
^ permalink raw reply
* Re: [PATCH 1/4] fbdev: sh_mobile_meram: Replace hardcoded register values with macros
From: Laurent Pinchart @ 2011-09-20 21:25 UTC (permalink / raw)
To: linux-fbdev
Hi Damien,
On Tuesday 02 August 2011 10:43:51 Damian Hobson-Garcia wrote:
Oops, late reply :-/
> > -#define MExxCTL 0x0
> > -#define MExxBSIZE 0x4
> > -#define MExxMNCF 0x8
> > -#define MExxSARA 0x10
> > -#define MExxSARB 0x14
> > -#define MExxSBSIZE 0x18
>
> [snip]
>
> > +#define MExxCTL 0x400
> > +#define MExxBSIZE 0x404
> > +#define MExxMNCF 0x408
> > +#define MExxSARA 0x410
> > +#define MExxSARB 0x414
> > +#define MExxSBSIZE 0x418
>
> One small comment on the definition of these register offsets and
> explanation of what I was originally thinking. Since each of these
> actually reprents a series of registers, one per ICB, (i.e. ME00CTL,
> ME01CTL, etc.) it makes sense to me to represent them as an offset from
> the base address (0x400) + the start of each ICB address (0x20 * index).
>
> Other than that, looks great.
Thanks. I've sent a v2.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: Proposal for a low-level Linux display framework
From: Patrik Jakobsson @ 2011-09-20 21:20 UTC (permalink / raw)
To: Keith Packard
Cc: Tomi Valkeinen, Alan Cox, linux-fbdev, linux-kernel, dri-devel,
linaro-dev, Clark, Rob, Archit Taneja
In-Reply-To: <yunpqivfbtf.fsf@aiko.keithp.com>
On Tue, Sep 20, 2011 at 5:55 PM, Keith Packard wrote:
> I'm not sure we need a new abstraction that subsumes both DSI and SDVO,
Ok. SDVO fits within the current abstraction, but I guess what I'm fishing
for is more code sharing of encoders. For instance, the SDVO code in GMA500
could be shared with i915. I'm currently working on copying the i915 SDVO code
over to GMA500, but sharing it would be even better.
> but we may need a DSI library that can be used by both DRM and other
> parts of the kernel.
Ok, not sure I understand the complexity of DSI. Can overlay composition
occur after/at the DSI stage (through MCS perhaps)? Or is it a matter of
panels requiring special scanout buffer formats that for instance V4L needs
to know about in order to overlay stuff properly? Or am I getting it all wrong?
Thanks
-Patrik
^ permalink raw reply
* [patch 1/1] fb: fix potential deadlock between lock_fb_info and console_lock
From: akpm @ 2011-09-20 21:11 UTC (permalink / raw)
To: linux-fbdev
From: Andrea Righi <arighi@develer.com>
Subject: fb: fix potential deadlock between lock_fb_info and console_lock
fb_set_suspend() must be called with the console semaphore held, which
means the code path coming in here will first take the console_lock() and
then call lock_fb_info().
However several framebuffer ioctl commands acquire these locks in reverse
order (lock_fb_info() and then console_lock()). This gives rise to
potential AB-BA deadlock.
Fix this by changing the order of acquisition in the ioctl commands that
make use of console_lock().
Signed-off-by: Andrea Righi <arighi@develer.com>
Reported-by: Peter Nordström (Palm GBU) <peter.nordstrom@palm.com>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: <stable@kernel.org>
Signed-off-by: Andrew Morton <akpm@google.com>
---
drivers/video/fbmem.c | 24 +++++++++++++++---------
1 file changed, 15 insertions(+), 9 deletions(-)
diff -puN drivers/video/fbmem.c~fb-fix-potential-deadlock-between-lock_fb_info-and-console_lock drivers/video/fbmem.c
--- a/drivers/video/fbmem.c~fb-fix-potential-deadlock-between-lock_fb_info-and-console_lock
+++ a/drivers/video/fbmem.c
@@ -1076,14 +1076,16 @@ static long do_fb_ioctl(struct fb_info *
case FBIOPUT_VSCREENINFO:
if (copy_from_user(&var, argp, sizeof(var)))
return -EFAULT;
- if (!lock_fb_info(info))
- return -ENODEV;
console_lock();
+ if (!lock_fb_info(info)) {
+ console_unlock();
+ return -ENODEV;
+ }
info->flags |= FBINFO_MISC_USEREVENT;
ret = fb_set_var(info, &var);
info->flags &= ~FBINFO_MISC_USEREVENT;
- console_unlock();
unlock_fb_info(info);
+ console_unlock();
if (!ret && copy_to_user(argp, &var, sizeof(var)))
ret = -EFAULT;
break;
@@ -1112,12 +1114,14 @@ static long do_fb_ioctl(struct fb_info *
case FBIOPAN_DISPLAY:
if (copy_from_user(&var, argp, sizeof(var)))
return -EFAULT;
- if (!lock_fb_info(info))
- return -ENODEV;
console_lock();
+ if (!lock_fb_info(info)) {
+ console_unlock();
+ return -ENODEV;
+ }
ret = fb_pan_display(info, &var);
- console_unlock();
unlock_fb_info(info);
+ console_unlock();
if (ret = 0 && copy_to_user(argp, &var, sizeof(var)))
return -EFAULT;
break;
@@ -1159,14 +1163,16 @@ static long do_fb_ioctl(struct fb_info *
unlock_fb_info(info);
break;
case FBIOBLANK:
- if (!lock_fb_info(info))
- return -ENODEV;
console_lock();
+ if (!lock_fb_info(info)) {
+ console_unlock();
+ return -ENODEV;
+ }
info->flags |= FBINFO_MISC_USEREVENT;
ret = fb_blank(info, arg);
info->flags &= ~FBINFO_MISC_USEREVENT;
- console_unlock();
unlock_fb_info(info);
+ console_unlock();
break;
default:
if (!lock_fb_info(info))
_
^ permalink raw reply
* Re: [PATCH] fbdev: sh_mobile_lcdc: Turn dot clock on before resuming
From: Florian Tobias Schandinat @ 2011-09-20 20:19 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1310387754-23033-1-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Laurent,
On 09/20/2011 07:56 PM, Laurent Pinchart wrote:
> Hi Florian,
>
> On Tuesday 20 September 2011 21:52:36 Florian Tobias Schandinat wrote:
>> On 09/20/2011 07:30 PM, Laurent Pinchart wrote:
>>> On Monday 11 July 2011 14:35:54 Laurent Pinchart wrote:
>>>> Resuming from runtime PM restores all LCDC registers. If the dot clock
>>>> is off at that time display panning information will be corrupted.
>>>>
>>>> Turn the dot clock on before resuming from runtime PM. Similarly,
>>>> turn the clock off after suspending the LCDC.
>>>
>>> Could you please pick this patch for v3.2 ?
>>
>> I assume you meant the currently developed 3.1? (as 3.2 is the next one for
>> which the patch is lying in my fbdev-next anyway)
>
> No, I meant v3.2. As I haven't received any pull notification in response to
> the patch, and as I had no way to check your fbdev-next tree on kernel.org, I
> just wanted to make sure the patch would be queued for v3.2.
Just for your info, my tree [1] never was on kernel.org, you can see the patch
in it here [2] (webfrontend). Looks like the patch I got come from your pull
request "SH mobile LCDC cleanups and fixes", hope that's the right version.
Best regards,
Florian Tobias Schandinat
[1] git://github.com/schandinat/linux-2.6.git
[2]
https://github.com/schandinat/linux-2.6/commit/f1ad90da5c0fcb8841cc5e6d66c56f4005d8c960
^ permalink raw reply
* Re: [PATCH] fbdev: sh_mobile_lcdc: Turn dot clock on before resuming from runtime PM
From: Laurent Pinchart @ 2011-09-20 19:56 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1310387754-23033-1-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Florian,
On Tuesday 20 September 2011 21:52:36 Florian Tobias Schandinat wrote:
> On 09/20/2011 07:30 PM, Laurent Pinchart wrote:
> > On Monday 11 July 2011 14:35:54 Laurent Pinchart wrote:
> >> Resuming from runtime PM restores all LCDC registers. If the dot clock
> >> is off at that time display panning information will be corrupted.
> >>
> >> Turn the dot clock on before resuming from runtime PM. Similarly,
> >> turn the clock off after suspending the LCDC.
> >
> > Could you please pick this patch for v3.2 ?
>
> I assume you meant the currently developed 3.1? (as 3.2 is the next one for
> which the patch is lying in my fbdev-next anyway)
No, I meant v3.2. As I haven't received any pull notification in response to
the patch, and as I had no way to check your fbdev-next tree on kernel.org, I
just wanted to make sure the patch would be queued for v3.2.
Please ignore this e-mail (and the other similar ones I've just sent) if the
patches are already in your queue. And thank you for picking them.
> Uhm, I originally didn't plan to ask Linus pulling anything before getting
> rid of the old stuff, but okay, will go through my patches in the next
> days and look what else is appropriate unless Linus beats me and releases
> 3.1 (although I do not consider this likely as long as kernel.org is still
> down)
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH] fbdev: sh_mobile_lcdc: Turn dot clock on before resuming
From: Florian Tobias Schandinat @ 2011-09-20 19:52 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1310387754-23033-1-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Laurent,
On 09/20/2011 07:30 PM, Laurent Pinchart wrote:
> Hi Florian,
>
> On Monday 11 July 2011 14:35:54 Laurent Pinchart wrote:
>> Resuming from runtime PM restores all LCDC registers. If the dot clock
>> is off at that time display panning information will be corrupted.
>>
>> Turn the dot clock on before resuming from runtime PM. Similarly,
>> turn the clock off after suspending the LCDC.
>
> Could you please pick this patch for v3.2 ?
I assume you meant the currently developed 3.1? (as 3.2 is the next one for
which the patch is lying in my fbdev-next anyway)
Uhm, I originally didn't plan to ask Linus pulling anything before getting rid
of the old stuff, but okay, will go through my patches in the next days and look
what else is appropriate unless Linus beats me and releases 3.1 (although I do
not consider this likely as long as kernel.org is still down)
Best regards,
Florian Tobias Schandinat
^ permalink raw reply
* Re: [PATCH 0/3] sh_mobile_lcdc cleanup and fixes
From: Laurent Pinchart @ 2011-09-20 19:33 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1310766753-30314-1-git-send-email-laurent.pinchart@ideasonboard.com>
On Tuesday 20 September 2011 21:30:15 Laurent Pinchart wrote:
> Hi Florian,
>
> On Friday 15 July 2011 23:52:30 Laurent Pinchart wrote:
> > Hi everybody,
> >
> > When trying to understand the sh_mobile_lcdc driver, I found it hard to
> > read statements that include hardcoded register values. I thus wrote a
> > patch that replace them with macros, making the code more readable.
> >
> > While doing so, I found two potential issues in the driver. See patches 2
> > and 3 for detailed explanations and fixes.
> >
> > Laurent Pinchart (3):
> > fbdev: sh_mobile_lcdc: Replace hardcoded register values with macros
> > fbdev: sh_mobile_lcdc: Don't acknowlege interrupts unintentionally
> > fbdev: sh_mobile_lcdc: Compute clock pattern using divider
> >
> > denominator
>
> Could you please pick these patches for v3.2 ?
Sorry, I meant to answer the next version of this patch set. Please pick v2
instead.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH] fbdev: sh_mobile_lcdc: Turn dot clock on before resuming from runtime PM
From: Laurent Pinchart @ 2011-09-20 19:30 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1310387754-23033-1-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Florian,
On Monday 11 July 2011 14:35:54 Laurent Pinchart wrote:
> Resuming from runtime PM restores all LCDC registers. If the dot clock
> is off at that time display panning information will be corrupted.
>
> Turn the dot clock on before resuming from runtime PM. Similarly,
> turn the clock off after suspending the LCDC.
Could you please pick this patch for v3.2 ?
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH 0/3] sh_mobile_lcdc cleanup and fixes
From: Laurent Pinchart @ 2011-09-20 19:30 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1310766753-30314-1-git-send-email-laurent.pinchart@ideasonboard.com>
Hi Florian,
On Friday 15 July 2011 23:52:30 Laurent Pinchart wrote:
> Hi everybody,
>
> When trying to understand the sh_mobile_lcdc driver, I found it hard to
> read statements that include hardcoded register values. I thus wrote a
> patch that replace them with macros, making the code more readable.
>
> While doing so, I found two potential issues in the driver. See patches 2
> and 3 for detailed explanations and fixes.
>
> Laurent Pinchart (3):
> fbdev: sh_mobile_lcdc: Replace hardcoded register values with macros
> fbdev: sh_mobile_lcdc: Don't acknowlege interrupts unintentionally
> fbdev: sh_mobile_lcdc: Compute clock pattern using divider
> denominator
Could you please pick these patches for v3.2 ?
--
Regards,
Laurent Pinchart
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox