* Re: [PATCH V4 3/6] OMAPDSS: DSS: Cleanup cpu_is_xxxx checks
From: Tomi Valkeinen @ 2012-08-20 8:42 UTC (permalink / raw)
To: Chandrabhanu Mahapatra; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1345211692.3158.160.camel@deskari>
[-- Attachment #1: Type: text/plain, Size: 1851 bytes --]
On Fri, 2012-08-17 at 16:54 +0300, Tomi Valkeinen wrote:
> On Thu, 2012-08-16 at 16:48 +0530, Chandrabhanu Mahapatra wrote:
> > All the cpu_is checks have been moved to dss_init_features function providing a
> > much more generic and cleaner interface. The OMAP version and revision specific
> > initializations in various functions are cleaned and the necessary data are
> > moved to dss_features structure which is local to dss.c.
> >
> > Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
>
> > +static int __init dss_init_features(struct device *dev)
> > +{
> > + dss.feat = devm_kzalloc(dev, sizeof(*dss.feat), GFP_KERNEL);
> > + if (!dss.feat) {
> > + dev_err(dev, "Failed to allocate local DSS Features\n");
> > + return -ENOMEM;
> > + }
> > +
> > + if (cpu_is_omap24xx())
> > + dss.feat = &omap24xx_dss_features;
> > + else if (cpu_is_omap34xx())
> > + dss.feat = &omap34xx_dss_features;
> > + else if (cpu_is_omap3630())
> > + dss.feat = &omap3630_dss_features;
> > + else if (cpu_is_omap44xx())
> > + dss.feat = &omap44xx_dss_features;
> > + else
> > + return -ENODEV;
> > +
> > + return 0;
> > +}
>
> This is not correct (and same problem in dispc). You allocate the feat
> struct and assign the pointer to dss.feat, but then overwrite dss.feat
> pointer with the pointer to omap24xx_dss_features (which is freed
> later). You need to memcpy it.
>
> I also get a crash on omap3 overo board when loading omapdss:
The crash happens because dss_get_clocks uses the feat stuff, but the
dss_init_features is only called later. Did you test this? I can't see
how that can work on any board.
Also, in the current place you have dss_init_features call, in case
there's an error you can't just return, you need to release the
resources. The same problem is in the dispc.c.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 0/7] HID: picoLCD updates
From: Alan Stern @ 2012-08-19 19:56 UTC (permalink / raw)
To: Bruno Prémont; +Cc: Jiri Kosina, linux-input, linux-kernel, linux-fbdev
In-Reply-To: <20120819182300.63665a0b@neptune.home>
On Sun, 19 Aug 2012, Bruno Prémont wrote:
> > I don't know bout hid_hw_close(). Certainly no more reports should be
> > submitted following usbhid_stop().
>
> Ok, I did just that, prevent new calls to usbhid_submit_report(), after
> calling hid_hw_close(), fixed one bug in my code that triggers the NULL
> pointer dereference (calling hid_set_drvdata(hdev, NULL) too early).
>
> Now I'm still seeing the bad paging request in _mmx_memcpy(), though rather
> sporadically.
> The last ones I saw were during remove() around the time of calling hid_hw_close()
> and hid_hw_stop(). Adding a printk() between the two (at least while picoLCD
> is hosting fbcon) makes it very improbably for the bad page to happen.
>
> It looks like low-level driver did free memory in hid_hw_close() for some
> in-flight URB and thus things break in following USB interrupt.
No, memory is not freed in usbhid_close(). It is freed in
usbhid_stop().
> From mapping trace information to source it seems:
> usbhid/hid-core.c:
> static int hid_submit_out(struct hid_device *hid)
> {
> struct hid_report *report;
> char *raw_report;
> struct usbhid_device *usbhid = hid->driver_data;
> int r;
>
> report = usbhid->out[usbhid->outtail].report;
> raw_report = usbhid->out[usbhid->outtail].raw_report;
>
> usbhid->urbout->transfer_buffer_length = ((report->size - 1) >> 3) +
> 1 + (report->id > 0);
> usbhid->urbout->dev = hid_to_usb_dev(hid);
> if (raw_report) {
> memcpy(usbhid->outbuf, raw_report,
> usbhid->urbout->transfer_buffer_length);
> ^^^^^^^^^^^^^^^_ this is exploding
Right. Like I said, usbhid->outbuf is freed in hid_free_buffers(),
which is called from usbhid_stop().
Forget about usbhid_close(). The race is between hid_submit_out() and
usbhid_stop().
Alan Stern
^ permalink raw reply
* [PATCH 2/6] HID: picoLCD: rework hid-fbdev interaction
From: Bruno Prémont @ 2012-08-19 17:32 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, linux-kernel, linux-fbdev
In-Reply-To: <20120819192859.74136bb0@neptune.home>
Split out all FB related data out of struct picolcd_data into a struct
picolcd_fb_data that is allocated with fb_info. This way fb_info may
cleanly outlive struct picolcd_data for as long as needed for its last
user to drop his reference.
Access to struct picolcd_data is now protected with struct
picolcd_fb_data's lock and tile update reports are only generated
while picolcd_fbdata->picolcd is not NULL and is not marked as failed
(which indicates unplug in progress).
Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
---
drivers/hid/hid-picolcd.h | 19 +++-
drivers/hid/hid-picolcd_fb.c | 216 ++++++++++++++++++++++--------------------
2 files changed, 126 insertions(+), 109 deletions(-)
diff --git a/drivers/hid/hid-picolcd.h b/drivers/hid/hid-picolcd.h
index 9200be1..dc4c795 100644
--- a/drivers/hid/hid-picolcd.h
+++ b/drivers/hid/hid-picolcd.h
@@ -90,11 +90,6 @@ struct picolcd_data {
#ifdef CONFIG_HID_PICOLCD_FB
/* Framebuffer stuff */
- u8 fb_update_rate;
- u8 fb_bpp;
- u8 fb_force;
- u8 *fb_vbitmap; /* local copy of what was sent to PicoLCD */
- u8 *fb_bitmap; /* framebuffer */
struct fb_info *fb_info;
#endif /* CONFIG_HID_PICOLCD_FB */
#ifdef CONFIG_HID_PICOLCD_LCD
@@ -119,9 +114,21 @@ struct picolcd_data {
int status;
#define PICOLCD_BOOTLOADER 1
#define PICOLCD_FAILED 2
-#define PICOLCD_READY_FB 4
};
+#ifdef CONFIG_HID_PICOLCD_FB
+struct picolcd_fb_data {
+ /* Framebuffer stuff */
+ spinlock_t lock;
+ struct picolcd_data *picolcd;
+ u8 update_rate;
+ u8 bpp;
+ u8 force;
+ u8 ready;
+ u8 *vbitmap; /* local copy of what was sent to PicoLCD */
+ u8 *bitmap; /* framebuffer */
+};
+#endif /* CONFIG_HID_PICOLCD_FB */
/* Find a given report */
#define picolcd_in_report(id, dev) picolcd_report(id, dev, HID_INPUT_REPORT)
diff --git a/drivers/hid/hid-picolcd_fb.c b/drivers/hid/hid-picolcd_fb.c
index 4d8e22c..5905bbd 100644
--- a/drivers/hid/hid-picolcd_fb.c
+++ b/drivers/hid/hid-picolcd_fb.c
@@ -98,19 +98,26 @@ static const struct fb_var_screeninfo picolcdfb_var = {
};
/* Send a given tile to PicoLCD */
-static int picolcd_fb_send_tile(struct hid_device *hdev, int chip, int tile)
+static int picolcd_fb_send_tile(struct picolcd_data *data, u8 *vbitmap,
+ int chip, int tile)
{
- struct picolcd_data *data = hid_get_drvdata(hdev);
- struct hid_report *report1 = picolcd_out_report(REPORT_LCD_CMD_DATA, hdev);
- struct hid_report *report2 = picolcd_out_report(REPORT_LCD_DATA, hdev);
+ struct hid_report *report1, *report2;
unsigned long flags;
u8 *tdata;
int i;
- if (!report1 || report1->maxfield != 1 || !report2 || report2->maxfield != 1)
+ report1 = picolcd_out_report(REPORT_LCD_CMD_DATA, data->hdev);
+ if (!report1 || report1->maxfield != 1)
+ return -ENODEV;
+ report2 = picolcd_out_report(REPORT_LCD_DATA, data->hdev);
+ if (!report2 || report2->maxfield != 1)
return -ENODEV;
spin_lock_irqsave(&data->lock, flags);
+ if ((data->status & PICOLCD_FAILED)) {
+ spin_unlock_irqrestore(&data->lock, flags);
+ return -ENODEV;
+ }
hid_set_field(report1->field[0], 0, chip << 2);
hid_set_field(report1->field[0], 1, 0x02);
hid_set_field(report1->field[0], 2, 0x00);
@@ -128,7 +135,7 @@ static int picolcd_fb_send_tile(struct hid_device *hdev, int chip, int tile)
hid_set_field(report2->field[0], 2, 0x00);
hid_set_field(report2->field[0], 3, 32);
- tdata = data->fb_vbitmap + (tile * 4 + chip) * 64;
+ tdata = vbitmap + (tile * 4 + chip) * 64;
for (i = 0; i < 64; i++)
if (i < 32)
hid_set_field(report1->field[0], 11 + i, tdata[i]);
@@ -189,6 +196,7 @@ void picolcd_fb_refresh(struct picolcd_data *data)
int picolcd_fb_reset(struct picolcd_data *data, int clear)
{
struct hid_report *report = picolcd_out_report(REPORT_LCD_CMD, data->hdev);
+ struct picolcd_fb_data *fbdata = data->fb_info->par;
int i, j;
unsigned long flags;
static const u8 mapcmd[8] = { 0x00, 0x02, 0x00, 0x64, 0x3f, 0x00, 0x64, 0xc0 };
@@ -207,20 +215,19 @@ int picolcd_fb_reset(struct picolcd_data *data, int clear)
hid_set_field(report->field[0], j, 0);
usbhid_submit_report(data->hdev, report, USB_DIR_OUT);
}
-
- data->status |= PICOLCD_READY_FB;
spin_unlock_irqrestore(&data->lock, flags);
- if (data->fb_bitmap) {
- if (clear) {
- memset(data->fb_vbitmap, 0, PICOLCDFB_SIZE);
- memset(data->fb_bitmap, 0, PICOLCDFB_SIZE*data->fb_bpp);
- }
- data->fb_force = 1;
+ if (clear) {
+ memset(fbdata->vbitmap, 0, PICOLCDFB_SIZE);
+ memset(fbdata->bitmap, 0, PICOLCDFB_SIZE*fbdata->bpp);
}
+ fbdata->force = 1;
/* schedule first output of framebuffer */
- picolcd_fb_refresh(data);
+ if (fbdata->ready)
+ schedule_delayed_work(&data->fb_info->deferred_work, 0);
+ else
+ fbdata->ready = 1;
return 0;
}
@@ -230,20 +237,15 @@ static void picolcd_fb_update(struct fb_info *info)
{
int chip, tile, n;
unsigned long flags;
+ struct picolcd_fb_data *fbdata = info->par;
struct picolcd_data *data;
mutex_lock(&info->lock);
- data = info->par;
- if (!data)
- goto out;
- spin_lock_irqsave(&data->lock, flags);
- if (!(data->status & PICOLCD_READY_FB)) {
- spin_unlock_irqrestore(&data->lock, flags);
- picolcd_fb_reset(data, 0);
- } else {
- spin_unlock_irqrestore(&data->lock, flags);
- }
+ spin_lock_irqsave(&fbdata->lock, flags);
+ if (!fbdata->ready && fbdata->picolcd)
+ picolcd_fb_reset(fbdata->picolcd, 0);
+ spin_unlock_irqrestore(&fbdata->lock, flags);
/*
* Translate the framebuffer into the format needed by the PicoLCD.
@@ -254,32 +256,38 @@ static void picolcd_fb_update(struct fb_info *info)
*/
n = 0;
for (chip = 0; chip < 4; chip++)
- for (tile = 0; tile < 8; tile++)
- if (picolcd_fb_update_tile(data->fb_vbitmap,
- data->fb_bitmap, data->fb_bpp, chip, tile) ||
- data->fb_force) {
- n += 2;
- if (n >= HID_OUTPUT_FIFO_SIZE / 2) {
- mutex_unlock(&info->lock);
- usbhid_wait_io(data->hdev);
- mutex_lock(&info->lock);
- data = info->par;
- if (!data)
- goto out;
- spin_lock_irqsave(&data->lock, flags);
- if (data->status & PICOLCD_FAILED) {
- spin_unlock_irqrestore(&data->lock, flags);
- goto out;
- }
- spin_unlock_irqrestore(&data->lock, flags);
- n = 0;
- }
- picolcd_fb_send_tile(data->hdev, chip, tile);
+ for (tile = 0; tile < 8; tile++) {
+ if (!fbdata->force && !picolcd_fb_update_tile(
+ fbdata->vbitmap, fbdata->bitmap,
+ fbdata->bpp, chip, tile))
+ continue;
+ n += 2;
+ if (n >= HID_OUTPUT_FIFO_SIZE / 2) {
+ spin_lock_irqsave(&fbdata->lock, flags);
+ data = fbdata->picolcd;
+ spin_unlock_irqrestore(&fbdata->lock, flags);
+ mutex_unlock(&info->lock);
+ if (!data)
+ return;
+ usbhid_wait_io(data->hdev);
+ mutex_lock(&info->lock);
+ n = 0;
}
- data->fb_force = false;
+ spin_lock_irqsave(&fbdata->lock, flags);
+ data = fbdata->picolcd;
+ spin_unlock_irqrestore(&fbdata->lock, flags);
+ if (!data || picolcd_fb_send_tile(data,
+ fbdata->vbitmap, chip, tile))
+ goto out;
+ }
+ fbdata->force = false;
if (n) {
+ spin_lock_irqsave(&fbdata->lock, flags);
+ data = fbdata->picolcd;
+ spin_unlock_irqrestore(&fbdata->lock, flags);
mutex_unlock(&info->lock);
- usbhid_wait_io(data->hdev);
+ if (data)
+ usbhid_wait_io(data->hdev);
return;
}
out:
@@ -336,20 +344,22 @@ static ssize_t picolcd_fb_write(struct fb_info *info, const char __user *buf,
static int picolcd_fb_blank(int blank, struct fb_info *info)
{
- if (!info->par)
- return -ENODEV;
/* We let fb notification do this for us via lcd/backlight device */
return 0;
}
static void picolcd_fb_destroy(struct fb_info *info)
{
+ struct picolcd_fb_data *fbdata = info->par;
+
/* make sure no work is deferred */
fb_deferred_io_cleanup(info);
+ /* No thridparty should ever unregister our framebuffer! */
+ WARN_ON(fbdata->picolcd != NULL);
+
vfree((u8 *)info->fix.smem_start);
framebuffer_release(info);
- printk(KERN_DEBUG "picolcd_fb_destroy(%p)\n", info);
}
static int picolcd_fb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
@@ -376,17 +386,15 @@ static int picolcd_fb_check_var(struct fb_var_screeninfo *var, struct fb_info *i
static int picolcd_set_par(struct fb_info *info)
{
- struct picolcd_data *data = info->par;
+ struct picolcd_fb_data *fbdata = info->par;
u8 *tmp_fb, *o_fb;
- if (!data)
- return -ENODEV;
- if (info->var.bits_per_pixel = data->fb_bpp)
+ if (info->var.bits_per_pixel = fbdata->bpp)
return 0;
/* switch between 1/8 bit depths */
if (info->var.bits_per_pixel != 1 && info->var.bits_per_pixel != 8)
return -EINVAL;
- o_fb = data->fb_bitmap;
+ o_fb = fbdata->bitmap;
tmp_fb = kmalloc(PICOLCDFB_SIZE*info->var.bits_per_pixel, GFP_KERNEL);
if (!tmp_fb)
return -ENOMEM;
@@ -415,7 +423,7 @@ static int picolcd_set_par(struct fb_info *info)
}
kfree(tmp_fb);
- data->fb_bpp = info->var.bits_per_pixel;
+ fbdata->bpp = info->var.bits_per_pixel;
return 0;
}
@@ -453,7 +461,8 @@ static ssize_t picolcd_fb_update_rate_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct picolcd_data *data = dev_get_drvdata(dev);
- unsigned i, fb_update_rate = data->fb_update_rate;
+ struct picolcd_fb_data *fbdata = data->fb_info->par;
+ unsigned i, fb_update_rate = fbdata->update_rate;
size_t ret = 0;
for (i = 1; i <= PICOLCDFB_UPDATE_RATE_LIMIT; i++)
@@ -472,6 +481,7 @@ static ssize_t picolcd_fb_update_rate_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t count)
{
struct picolcd_data *data = dev_get_drvdata(dev);
+ struct picolcd_fb_data *fbdata = data->fb_info->par;
int i;
unsigned u;
@@ -487,8 +497,8 @@ static ssize_t picolcd_fb_update_rate_store(struct device *dev,
else if (u = 0)
u = PICOLCDFB_UPDATE_RATE_DEFAULT;
- data->fb_update_rate = u;
- data->fb_info->fbdefio->delay = HZ / data->fb_update_rate;
+ fbdata->update_rate = u;
+ data->fb_info->fbdefio->delay = HZ / fbdata->update_rate;
return count;
}
@@ -500,30 +510,18 @@ int picolcd_init_framebuffer(struct picolcd_data *data)
{
struct device *dev = &data->hdev->dev;
struct fb_info *info = NULL;
+ struct picolcd_fb_data *fbdata = NULL;
int i, error = -ENOMEM;
- u8 *fb_vbitmap = NULL;
- u8 *fb_bitmap = NULL;
u32 *palette;
- fb_bitmap = vmalloc(PICOLCDFB_SIZE*8);
- if (fb_bitmap = NULL) {
- dev_err(dev, "can't get a free page for framebuffer\n");
- goto err_nomem;
- }
-
- fb_vbitmap = kmalloc(PICOLCDFB_SIZE, GFP_KERNEL);
- if (fb_vbitmap = NULL) {
- dev_err(dev, "can't alloc vbitmap image buffer\n");
- goto err_nomem;
- }
-
- data->fb_update_rate = PICOLCDFB_UPDATE_RATE_DEFAULT;
/* The extra memory is:
* - 256*u32 for pseudo_palette
* - struct fb_deferred_io
*/
info = framebuffer_alloc(256 * sizeof(u32) +
- sizeof(struct fb_deferred_io), dev);
+ sizeof(struct fb_deferred_io) +
+ sizeof(struct picolcd_fb_data) +
+ PICOLCDFB_SIZE, dev);
if (info = NULL) {
dev_err(dev, "failed to allocate a framebuffer\n");
goto err_nomem;
@@ -531,74 +529,86 @@ int picolcd_init_framebuffer(struct picolcd_data *data)
info->fbdefio = info->par;
*info->fbdefio = picolcd_fb_defio;
- palette = info->par + sizeof(struct fb_deferred_io);
+ info->par += sizeof(struct fb_deferred_io);
+ palette = info->par;
+ info->par += 256 * sizeof(u32);
for (i = 0; i < 256; i++)
palette[i] = i > 0 && i < 16 ? 0xff : 0;
info->pseudo_palette = palette;
- info->screen_base = (char __force __iomem *)fb_bitmap;
info->fbops = &picolcdfb_ops;
info->var = picolcdfb_var;
info->fix = picolcdfb_fix;
info->fix.smem_len = PICOLCDFB_SIZE*8;
- info->fix.smem_start = (unsigned long)fb_bitmap;
- info->par = data;
info->flags = FBINFO_FLAG_DEFAULT;
- data->fb_vbitmap = fb_vbitmap;
- data->fb_bitmap = fb_bitmap;
- data->fb_bpp = picolcdfb_var.bits_per_pixel;
+ fbdata = info->par;
+ spin_lock_init(&fbdata->lock);
+ fbdata->picolcd = data;
+ fbdata->update_rate = PICOLCDFB_UPDATE_RATE_DEFAULT;
+ fbdata->bpp = picolcdfb_var.bits_per_pixel;
+ fbdata->force = 1;
+ fbdata->vbitmap = info->par + sizeof(struct picolcd_fb_data);
+ fbdata->bitmap = vmalloc(PICOLCDFB_SIZE*8);
+ if (fbdata->bitmap = NULL) {
+ dev_err(dev, "can't get a free page for framebuffer\n");
+ goto err_nomem;
+ }
+ info->screen_base = (char __force __iomem *)fbdata->bitmap;
+ info->fix.smem_start = (unsigned long)fbdata->bitmap;
+ memset(fbdata->vbitmap, 0xff, PICOLCDFB_SIZE);
+ data->fb_info = info;
+
error = picolcd_fb_reset(data, 1);
if (error) {
dev_err(dev, "failed to configure display\n");
goto err_cleanup;
}
+
error = device_create_file(dev, &dev_attr_fb_update_rate);
if (error) {
dev_err(dev, "failed to create sysfs attributes\n");
goto err_cleanup;
}
+
fb_deferred_io_init(info);
- data->fb_info = info;
error = register_framebuffer(info);
if (error) {
dev_err(dev, "failed to register framebuffer\n");
goto err_sysfs;
}
- /* schedule first output of framebuffer */
- data->fb_force = 1;
- schedule_delayed_work(&info->deferred_work, 0);
return 0;
err_sysfs:
- fb_deferred_io_cleanup(info);
device_remove_file(dev, &dev_attr_fb_update_rate);
+ fb_deferred_io_cleanup(info);
err_cleanup:
- data->fb_vbitmap = NULL;
- data->fb_bitmap = NULL;
- data->fb_bpp = 0;
data->fb_info = NULL;
err_nomem:
+ if (fbdata)
+ vfree(fbdata->bitmap);
framebuffer_release(info);
- vfree(fb_bitmap);
- kfree(fb_vbitmap);
return error;
}
void picolcd_exit_framebuffer(struct picolcd_data *data)
{
struct fb_info *info = data->fb_info;
- u8 *fb_vbitmap = data->fb_vbitmap;
-
- if (!info)
- return;
+ struct picolcd_fb_data *fbdata = info->par;
+ unsigned long flags;
device_remove_file(&data->hdev->dev, &dev_attr_fb_update_rate);
- info->par = NULL;
+
+ /* disconnect framebuffer from HID dev */
+ spin_lock_irqsave(&fbdata->lock, flags);
+ fbdata->picolcd = NULL;
+ spin_unlock_irqrestore(&fbdata->lock, flags);
+
+ /* make sure there is no running update - thus that fbdata->picolcd
+ * once obtained under lock is guaranteed not to get free() under
+ * the feet of the deferred work */
+ flush_delayed_work_sync(&info->deferred_work);
+
+ data->fb_info = NULL;
unregister_framebuffer(info);
- data->fb_vbitmap = NULL;
- data->fb_bitmap = NULL;
- data->fb_bpp = 0;
- data->fb_info = NULL;
- kfree(fb_vbitmap);
}
--
1.7.8.6
^ permalink raw reply related
* [PATCH 0/6] HID: picoLCD additional fixes + CIR support
From: Bruno Prémont @ 2012-08-19 17:28 UTC (permalink / raw)
To: Jiri Kosina
Cc: linux-input, linux-kernel, Mauro Carvalho Chehab, linux-fbdev,
linux-media
In-Reply-To: <20120730213656.0a9f6d30@neptune.home>
Hi,
This series fixes some race conditions in picoLCD driver during remove()
and adds support for IR functionality.
Repeatedly binding/unbinding device at hid-picolcd driver level or at
usbhid level now works properly (except in rare occasions which trigger
a paging error in interrupt context (memcopy) and seem to come from
usbhid internals when usbhid gets interrupt between hid_hw_close() and
hid_hw_stop()).
CIR support is added using RC_CORE for the decoding and handling of
IR-event stream and works for a Sony and a JVC remote.
Bruno
^ permalink raw reply
* Re: [PATCH 6/7 v2] HID: picoLCD: drop version check during probe
From: Bruno Prémont @ 2012-08-19 16:56 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, linux-kernel, linux-fbdev
In-Reply-To: <alpine.LNX.2.00.1208151015220.7026@pobox.suse.cz>
Commit 4ea5454203d991ec85264f64f89ca8855fce69b0
[HID: Fix race condition between driver core and ll-driver] introduced
new locking around probe/remove functions that prevents any report/reply
from hardware to reach driver until it returned from probe.
As such, the ask-reply way to checking picoLCD firmware version during
probe is bound to timeout and let probe fail.
Drop the check to let driver successfully probe again (until locking issues
are resolved allowing to reinstate the check).
Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
---
Changes since v1:
- drop version check during probe instead of commenting it out.
drivers/hid/hid-picolcd_core.c | 18 ------------------
1 files changed, 0 insertions(+), 18 deletions(-)
diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
index 7b566ee..e08ffd2 100644
--- a/drivers/hid/hid-picolcd_core.c
+++ b/drivers/hid/hid-picolcd_core.c
@@ -478,14 +478,6 @@ static int picolcd_probe_lcd(struct hid_device *hdev, struct picolcd_data *data)
{
int error;
- error = picolcd_check_version(hdev);
- if (error)
- return error;
-
- if (data->version[0] != 0 && data->version[1] != 3)
- hid_info(hdev, "Device with untested firmware revision, please submit /sys/kernel/debug/hid/%s/rdesc for this device.\n",
- dev_name(&hdev->dev));
-
/* Setup keypad input device */
error = picolcd_init_keys(data, picolcd_in_report(REPORT_KEY_STATE, hdev));
if (error)
@@ -534,16 +526,6 @@ err:
static int picolcd_probe_bootloader(struct hid_device *hdev, struct picolcd_data *data)
{
- int error;
-
- error = picolcd_check_version(hdev);
- if (error)
- return error;
-
- if (data->version[0] != 1 && data->version[1] != 0)
- hid_info(hdev, "Device with untested bootloader revision, please submit /sys/kernel/debug/hid/%s/rdesc for this device.\n",
- dev_name(&hdev->dev));
-
picolcd_init_devfs(data, NULL, NULL,
picolcd_out_report(REPORT_BL_READ_MEMORY, hdev),
picolcd_out_report(REPORT_BL_WRITE_MEMORY, hdev), NULL);
--
1.7.8.6
^ permalink raw reply related
* Re: [PATCH 0/7] HID: picoLCD updates
From: Bruno Prémont @ 2012-08-19 16:23 UTC (permalink / raw)
To: Alan Stern; +Cc: Jiri Kosina, linux-input, linux-kernel, linux-fbdev
In-Reply-To: <Pine.LNX.4.44L0.1208182001320.25537-100000@netrider.rowland.org>
On Sat, 18 August 2012 Alan Stern wrote:
> On Sat, 18 Aug 2012, Bruno Prémont wrote:
>
> > One thing I just though about, how does usbhid handle the calls to
> > usbhid_submit_report() when hid_hw_stop()/hid_hw_close() have already
> > been called?
>
> Looks like they aren't synchronized at all. That's a bug.
> usbhid_submit_report() should check the HID_DISCONNECTED flag.
>
> > I will attempt to see if it makes a difference to shortcut my
> > usbhid_submit_report() calls from the point on I have called hid_hw_close()...
>
> I don't know bout hid_hw_close(). Certainly no more reports should be
> submitted following usbhid_stop().
Ok, I did just that, prevent new calls to usbhid_submit_report(), after
calling hid_hw_close(), fixed one bug in my code that triggers the NULL
pointer dereference (calling hid_set_drvdata(hdev, NULL) too early).
Now I'm still seeing the bad paging request in _mmx_memcpy(), though rather
sporadically.
The last ones I saw were during remove() around the time of calling hid_hw_close()
and hid_hw_stop(). Adding a printk() between the two (at least while picoLCD
is hosting fbcon) makes it very improbably for the bad page to happen.
It looks like low-level driver did free memory in hid_hw_close() for some
in-flight URB and thus things break in following USB interrupt.
From mapping trace information to source it seems:
usbhid/hid-core.c:
static int hid_submit_out(struct hid_device *hid)
{
struct hid_report *report;
char *raw_report;
struct usbhid_device *usbhid = hid->driver_data;
int r;
report = usbhid->out[usbhid->outtail].report;
raw_report = usbhid->out[usbhid->outtail].raw_report;
usbhid->urbout->transfer_buffer_length = ((report->size - 1) >> 3) +
1 + (report->id > 0);
usbhid->urbout->dev = hid_to_usb_dev(hid);
if (raw_report) {
memcpy(usbhid->outbuf, raw_report,
usbhid->urbout->transfer_buffer_length);
^^^^^^^^^^^^^^^_ this is exploding
kfree(raw_report);
usbhid->out[usbhid->outtail].raw_report = NULL;
}
dbg_hid("submitting out urb\n");
r = usb_submit_urb(usbhid->urbout, GFP_ATOMIC);
if (r < 0) {
hid_err(hid, "usb_submit_urb(out) failed: %d\n", r);
return r;
}
usbhid->last_out = jiffies;
return 0;
}
Bruno
^ permalink raw reply
* [PATCH 8/14] drivers/video/sunxvr2500.c: fix error return code
From: Julia Lawall @ 2012-08-19 8:44 UTC (permalink / raw)
To: Florian Tobias Schandinat; +Cc: kernel-janitors, linux-fbdev, linux-kernel
In-Reply-To: <1345365870-29831-1-git-send-email-Julia.Lawall@lip6.fr>
From: Julia Lawall <Julia.Lawall@lip6.fr>
Initialize return variable before exiting on an error path.
A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)
// <smpl>
(
if@p1 (\(ret < 0\|ret != 0\))
{ ... return ret; }
|
ret@p1 = 0
)
... when != ret = e1
when != &ret
*if(...)
{
... when != ret = e2
when forall
return ret;
}
// </smpl>
Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
---
drivers/video/sunxvr2500.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/video/sunxvr2500.c b/drivers/video/sunxvr2500.c
index 5848436..7fbcba8 100644
--- a/drivers/video/sunxvr2500.c
+++ b/drivers/video/sunxvr2500.c
@@ -181,8 +181,10 @@ static int __devinit s3d_pci_register(struct pci_dev *pdev,
sp->fb_size = info->fix.line_length * sp->height;
sp->fb_base = ioremap(sp->fb_base_phys, sp->fb_size);
- if (!sp->fb_base)
+ if (!sp->fb_base) {
+ err = -ENOMEM;
goto err_release_pci;
+ }
err = s3d_set_fbinfo(sp);
if (err)
^ permalink raw reply related
* Re: [PATCH 0/7] HID: picoLCD updates
From: Alan Stern @ 2012-08-19 0:11 UTC (permalink / raw)
To: Bruno Prémont; +Cc: Jiri Kosina, linux-input, linux-kernel, linux-fbdev
In-Reply-To: <20120818154828.0e992bfe@neptune.home>
On Sat, 18 Aug 2012, Bruno Prémont wrote:
> One thing I just though about, how does usbhid handle the calls to
> usbhid_submit_report() when hid_hw_stop()/hid_hw_close() have already
> been called?
Looks like they aren't synchronized at all. That's a bug.
usbhid_submit_report() should check the HID_DISCONNECTED flag.
> I will attempt to see if it makes a difference to shortcut my
> usbhid_submit_report() calls from the point on I have called hid_hw_close()...
I don't know bout hid_hw_close(). Certainly no more reports should be
submitted following usbhid_stop().
Alan Stern
^ permalink raw reply
* Re: [PATCH 0/7] HID: picoLCD updates
From: Bruno Prémont @ 2012-08-18 18:49 UTC (permalink / raw)
To: Alan Stern, Jiri Kosina; +Cc: linux-input, linux-kernel, linux-fbdev
In-Reply-To: <20120818154828.0e992bfe@neptune.home>
On Sat, 18 August 2012 Bruno Prémont <bonbons@linux-vserver.org> wrote:
> One thing I just though about, how does usbhid handle the calls to
> usbhid_submit_report() when hid_hw_stop()/hid_hw_close() have already
> been called?
> I will attempt to see if it makes a difference to shortcut my
> usbhid_submit_report() calls from the point on I have called hid_hw_close()...
I don't know if I'm progressing or just drawing circles...
I included a check right before calling usbhid_submit_report() to prevent
calling it if hid_hw_close()/hid_hw_stop() has been called and that part
works as expected, just that at some moment it explodes on a NULL-pointer
dereference.
Initially I had usbhid_submit_report macro:
#define usbhid_submit_report(a, b, c) \
do { \
picolcd_debug_out_report(d, a, b); \
usbhid_submit_report(a, b, c); \
} while (0)
Now to make sure no new reports get submitted once hid_hw_stop()
was called I changed it to:
#define usbhid_submit_report(a, b, c) \
do { \
struct picolcd_data *d = hid_get_drvdata(a); \
unsigned long flags; \
picolcd_debug_out_report(d, a, b); \
spin_lock_irqsave(&d->lock, flags); \
if (!(d->status & PICOLCD_FAILED)) \
usbhid_submit_report(a, b, c); \
else \
printk(KERN_INFO "Submitting report after hw_stop!\n"); \
spin_unlock_irqrestore(&d->lock, flags); \
} while (0)
with the printk() to spam dmesg :)
The NULL-pointer dereference I got inside picolcd_fb_deferred_io() was due
to me calling hid_set_drvdata(hdev, NULL) too early in remove(), fixed.
But with that fixed I still encounter the bad page, probably around
the time of hid_hw_stop(), though it always takes the HARD LOCK watchdog
to trigger for the trace to show (probably because or IRQ context):
It seems to happen during the bind/unbind following the unbind that
produces a
hid-picolcd 0003:04D8:C002.0003: usb_submit_urb(out) failed: -1
either a BUG or even as reboot and happend between hid_hw_close() and
hid_hw_stop() though my extra printks seem to reduce probability of
bad things to happen.
[ 1217.643017] BUG: unable to handle kernel paging request at c7cb0000
[ 1217.643017] IP: [<c11e2be7>] _mmx_memcpy+0x27/0x16c
[ 1217.643017] *pde = 19078063 *pte = 07cb0161
[ 1217.643017] Oops: 0003 [#1]
[ 1217.643017] Modules linked in: hid_picolcd fb_sys_fops sysimgblt sysfillrect syscopyarea drm_kms_helper nfs3 nfs_acl nfs lockd sunrpc
[ 1217.643017] Pid: 0, comm: swapper Not tainted 3.6.0-rc2-jupiter-00008-gd3c2b0c-dirty #2 NVIDIA Corporation. nFORCE-MCP/MS-6373
[ 1217.643017] EIP: 0060:[<c11e2be7>] EFLAGS: 00010013 CPU: 0
[ 1217.643017] EIP is at _mmx_memcpy+0x27/0x16c
[ 1217.643017] EAX: dd40a000 EBX: dd6ea010 ECX: 035af77b EDX: d9108c80
[ 1217.643017] ESI: d9121c00 EDI: c7cb0000 EBP: dd40bebc ESP: dd40bea0
[ 1217.643017] DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068
[ 1217.643017] CR0: 8005003b CR2: c7cb0000 CR3: 1cf1c000 CR4: 000007d0
[ 1217.643017] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[ 1217.643017] DR6: ffff0ff0 DR7: 00000400
[ 1217.643017] Process swapper (pid: 0, tiÝ40a000 taskÁ5d24c0 task.tiÁ5c6000)
[ 1217.643017] Stack:
[ 1217.643017] 0d6d6d6f dd40beac c109bba4 c7c97080 dd6ea010 c7c97080 dceb3030 dd40bee4
[ 1217.643017] c133b364 c7d40000 c7d40000 dd40bed4 dceb3030 d9108c80 dd6ea010 00000046
[ 1217.643017] dceb3030 dd40bf04 c133bea0 0000d6d6 dbe901b0 fffffffe dbe901b0 fffffffe
[ 1217.643017] Call Trace:
[ 1217.643017] [<c109bba4>] ? free_compound_page+0x14/0x20
[ 1217.643017] [<c133b364>] hid_submit_out+0xa4/0x130
[ 1217.643017] [<c133bea0>] hid_irq_out+0xa0/0x100
[ 1217.643017] [<c12e425e>] usb_hcd_giveback_urb+0x4e/0x90
[ 1217.643017] [<c12f9bd2>] finish_urb+0xb2/0xf0
[ 1217.643017] [<c12fa8a8>] finish_unlinks+0x168/0x320
[ 1217.643017] [<c12fc43f>] ohci_irq+0x27f/0x300
[ 1217.643017] [<c1077790>] ? unmask_irq+0x20/0x20
[ 1217.643017] [<c12e3ace>] usb_hcd_irq+0x1e/0x40
[ 1217.643017] [<c107549d>] handle_irq_event_percpu+0x6d/0x1c0
[ 1217.643017] [<c1077790>] ? unmask_irq+0x20/0x20
[ 1217.643017] [<c1075618>] handle_irq_event+0x28/0x40
[ 1217.643017] [<c107781a>] handle_fasteoi_irq+0x8a/0xe0
[ 1217.643017] <IRQ>
[ 1217.643017] [<c1003e0a>] ? do_IRQ+0x3a/0xb0
[ 1217.643017] [<c140c870>] ? common_interrupt+0x30/0x38
[ 1217.643017] [<c1009028>] ? default_idle+0x68/0xd0
[ 1217.643017] [<c1009a0f>] ? cpu_idle+0x8f/0xc0
[ 1217.643017] [<c13ffe97>] ? rest_init+0x67/0x70
[ 1217.643017] [<c161b8b8>] ? start_kernel+0x2a3/0x2aa
[ 1217.643017] [<c161b3da>] ? repair_env_string+0x51/0x51
[ 1217.643017] [<c161b28e>] ? i386_start_kernel+0x44/0x46
[ 1217.643017] Code: 90 90 90 90 55 89 e5 57 56 89 d6 53 83 ec 10 89 45 f0 89 4d e4 89 e0 25 00 e0 ff ff f7 40 14 00 ff ff 07 74 17 c1 e9 02 8b 7d f0 <f3> a5 8b 4d e4 83 e1 03 74 02 f3 a4 e9 27 01 00 00 8b 45 1
[ 1217.643017] EIP: [<c11e2be7>] _mmx_memcpy+0x27/0x16c SS:ESP 0068:dd40bea0
[ 1217.643017] CR2: 00000000c7cb0000
[ 1217.643017] ---[ end trace fbaa4c5261e1a846 ]---
Bruno
^ permalink raw reply
* Re: [patch] video: mb862xxfb: prevent divide by zero bug
From: Anatolij Gustschin @ 2012-08-18 18:35 UTC (permalink / raw)
To: Dan Carpenter
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA, H Hartley Sweeten,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
Florian Tobias Schandinat, kernel-janitors-u79uwXL29TY76Z2rM5mHXA,
Rob Herring, Paul Gortmaker, Laurent Pinchart, David Brown
In-Reply-To: <20120818155541.GB22424-mgFCXtclrQlZLf2FXnZxJA@public.gmane.org>
On Sat, 18 Aug 2012 18:55:41 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:
> Do a sanity check on these before using them as divisors.
>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Acked-by: Anatolij Gustschin <agust@denx.de>
> diff --git a/drivers/video/mb862xx/mb862xxfbdrv.c b/drivers/video/mb862xx/mb862xxfbdrv.c
> index 00ce1f3..57d940b 100644
> --- a/drivers/video/mb862xx/mb862xxfbdrv.c
> +++ b/drivers/video/mb862xx/mb862xxfbdrv.c
> @@ -328,6 +328,8 @@ static int mb862xxfb_ioctl(struct fb_info *fbi, unsigned int cmd,
> case MB862XX_L1_SET_CFG:
> if (copy_from_user(l1_cfg, argp, sizeof(*l1_cfg)))
> return -EFAULT;
> + if (l1_cfg->dh = 0 || l1_cfg->dw = 0)
> + return -EINVAL;
> if ((l1_cfg->sw >= l1_cfg->dw) && (l1_cfg->sh >= l1_cfg->dh)) {
> /* downscaling */
> outreg(cap, GC_CAP_CSC,
Thanks,
Anatolij
^ permalink raw reply
* [patch] video: mb862xxfb: prevent divide by zero bug
From: Dan Carpenter @ 2012-08-18 15:55 UTC (permalink / raw)
To: Florian Tobias Schandinat
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA, H Hartley Sweeten,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
kernel-janitors-u79uwXL29TY76Z2rM5mHXA, Rob Herring,
Paul Gortmaker, Laurent Pinchart, David Brown
Do a sanity check on these before using them as divisors.
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/video/mb862xx/mb862xxfbdrv.c b/drivers/video/mb862xx/mb862xxfbdrv.c
index 00ce1f3..57d940b 100644
--- a/drivers/video/mb862xx/mb862xxfbdrv.c
+++ b/drivers/video/mb862xx/mb862xxfbdrv.c
@@ -328,6 +328,8 @@ static int mb862xxfb_ioctl(struct fb_info *fbi, unsigned int cmd,
case MB862XX_L1_SET_CFG:
if (copy_from_user(l1_cfg, argp, sizeof(*l1_cfg)))
return -EFAULT;
+ if (l1_cfg->dh = 0 || l1_cfg->dw = 0)
+ return -EINVAL;
if ((l1_cfg->sw >= l1_cfg->dw) && (l1_cfg->sh >= l1_cfg->dh)) {
/* downscaling */
outreg(cap, GC_CAP_CSC,
^ permalink raw reply related
* viafb PLL/clock tweaking causes XO-1.5 instability
From: Daniel Drake @ 2012-08-18 15:53 UTC (permalink / raw)
To: linux-fbdev
Hi,
I've been investigating a strange regression where in recent kernel
releases, the XO-1.5 cannot reliably suspend/resume when on a virtual
terminal (driven by viafb). Under X there is no instability.
The test case is to suspend and resume a lot (about 10 times a
minute). In less than an hour the system will fail to resume - the
system RAM becomes corrupt on resume and the firmware can't find its
way back into Linux, so it reboots. This is presumably because the GPU
is accessing main memory while the system is suspended, causing the
RAM to come out of self-refresh and lose all its contents before the
CPU wakes up to resume the system.
I have bisected this to find the first bad commit:
commit b692a63af8b63a7a7e84702a713d0072e336b326
Author: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
Date: Thu Mar 24 14:25:51 2011 +0000
viafb: add VIA slapping capability
On the XO-1.5 this code tries to set IGA1=off IGA2=on. However, a
later commit causes them both to be turned on, since we enable
X_COMPATIBILITY (we use X, and we find that setting to be necessary
for X to work).
If I comment the code that modifies IGA1, I find that setting IGA2=on
causes the crashyness, setting IGA2=off results in the display not
working at all. The only non-crashy setting here I can find is to not
touch the IGA2 configuration.
If I comment out the code that modifies IGA2, I find that setting
IGA1=on causes the crashyness, and setting IGA1=off is stable.
Going one step lower to find exactly which calls are responsible for
triggering the crashyness, by only enabling one function call at a
time, I find:
set_secondary_pll_state(on) only = stable
set_secondary_clock_state(on) only = crashy
set_primary_pll_state(on) only = stable
set_primary_clock_state(on) only = crashy
Looking at the set_secondary_clock_state(on) case, I find that the
Secondary Display Engine (Gated Clock <LCK>) bits 7:6 in IO Port
3C5.1B are 11 when untouched, and 10 after
set_secondary_clock_state(on) has run. So the code seems to be doing
what it intends to do, but its not clear why modifying this register
effectively switches between 3 behavioural states (stable, no display
at all, display active but crashy suspend/resume). Also is there
really any need to modify it or would the default (enable clock only
if its needed) suffice?
Any ideas?
Thanks,
Daniel
^ permalink raw reply
* Re: [PATCH 0/7] HID: picoLCD updates
From: Bruno Prémont @ 2012-08-18 13:48 UTC (permalink / raw)
To: Alan Stern; +Cc: Jiri Kosina, linux-input, linux-kernel, linux-fbdev
In-Reply-To: <Pine.LNX.4.44L0.1208180913440.17394-100000@netrider.rowland.org>
On Sat, 18 August 2012 Alan Stern <stern@rowland.harvard.edu> wrote:
> On Sat, 18 Aug 2012, Bruno Prémont wrote:
> > On Thu, 16 August 2012 Jiri Kosina <jkosina@suse.cz> wrote:
> > > On Thu, 16 Aug 2012, Bruno Prémont wrote:
> > >
> > > > > I don't really understand this explanation. Once usb_kill_urb() returns,
> > > > > the URB should be available for future use (and therefore all queues
> > > > > completely drained).
> > > >
> > > > I won't have time today to check, though my guess is that on each
> > > > echo $usb-id > bind; echo $usb-id > unbind
> > > > under /sys/bus/hid/drivers/hid-picolcd/ the USB urb queue fills a bit does
> > > > not get cleared.
> > > >
> > > > Is usb_kill_urb() called when unbinding just the specific hid driver?
> > >
> > > Yup, through hid_hw_stop() -> usbhid_stop().
> > >
> > > > If so my short timing between bind/unbind must be triggering something
> > > > else...
> > > >
> > > > Otherwise I'm missing something as at first time I got no "output queue full"
> > > > messages, but as I repeated the bind/unbind sequences the prints per bind/unbind
> > > > iteration increased in number.
> > > >
> > > > Anyhow, on Friday evening/week-end I will continue digging and report back with my
> > > > findings.
> >
> > Huh, after changing some of the hid-picolcd data in order to have less racy
> > coupling between hid and framebuffer I'm now dying way too often in _mmx_memcpy
> > and most of the time I don't get a (complete) trace...
>
> There was a similar problem reported recently. It turned out to be
> caused by a __devinitconst annotation attached to a usb_device_id
> table.
>
> If there are any __devinit* annotations in the hid-picolcd driver, you
> should see if removing them helps.
There is no such annotation around in hid-picolcd.
One thing I just though about, how does usbhid handle the calls to
usbhid_submit_report() when hid_hw_stop()/hid_hw_close() have already
been called?
I will attempt to see if it makes a difference to shortcut my
usbhid_submit_report() calls from the point on I have called hid_hw_close()...
Bruno
^ permalink raw reply
* [PATCH] fbdev/amifb: Remove write-only variable amifb_inverse
From: Geert Uytterhoeven @ 2012-08-18 13:30 UTC (permalink / raw)
To: Florian Tobias Schandinat; +Cc: linux-fbdev, linux-m68k, Geert Uytterhoeven
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
drivers/video/amifb.c | 2 --
1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/drivers/video/amifb.c b/drivers/video/amifb.c
index 887df9d..7fa1bf8 100644
--- a/drivers/video/amifb.c
+++ b/drivers/video/amifb.c
@@ -949,7 +949,6 @@ static int round_down_bpp = 1; /* for mode probing */
static int amifb_ilbm = 0; /* interleaved or normal bitplanes */
-static int amifb_inverse = 0;
static u32 amifb_hfmin __initdata; /* monitor hfreq lower limit (Hz) */
static u32 amifb_hfmax __initdata; /* monitor hfreq upper limit (Hz) */
@@ -2355,7 +2354,6 @@ static int __init amifb_setup(char *options)
if (!*this_opt)
continue;
if (!strcmp(this_opt, "inverse")) {
- amifb_inverse = 1;
fb_invert_cmaps();
} else if (!strcmp(this_opt, "ilbm"))
amifb_ilbm = 1;
--
1.7.0.4
^ permalink raw reply related
* Re: [PATCH 0/7] HID: picoLCD updates
From: Alan Stern @ 2012-08-18 13:19 UTC (permalink / raw)
To: Bruno Prémont; +Cc: Jiri Kosina, linux-input, linux-kernel, linux-fbdev
In-Reply-To: <20120818144039.0356cc7c@neptune.home>
On Sat, 18 Aug 2012, Bruno Prémont wrote:
> Hi Jiri,
>
> [CCing Alan Stern]
>
> On Thu, 16 August 2012 Jiri Kosina <jkosina@suse.cz> wrote:
> > On Thu, 16 Aug 2012, Bruno Prémont wrote:
> >
> > > > I don't really understand this explanation. Once usb_kill_urb() returns,
> > > > the URB should be available for future use (and therefore all queues
> > > > completely drained).
> > >
> > > I won't have time today to check, though my guess is that on each
> > > echo $usb-id > bind; echo $usb-id > unbind
> > > under /sys/bus/hid/drivers/hid-picolcd/ the USB urb queue fills a bit does
> > > not get cleared.
> > >
> > > Is usb_kill_urb() called when unbinding just the specific hid driver?
> >
> > Yup, through hid_hw_stop() -> usbhid_stop().
> >
> > > If so my short timing between bind/unbind must be triggering something
> > > else...
> > >
> > > Otherwise I'm missing something as at first time I got no "output queue full"
> > > messages, but as I repeated the bind/unbind sequences the prints per bind/unbind
> > > iteration increased in number.
> > >
> > > Anyhow, on Friday evening/week-end I will continue digging and report back with my
> > > findings.
>
> Huh, after changing some of the hid-picolcd data in order to have less racy
> coupling between hid and framebuffer I'm now dying way too often in _mmx_memcpy
> and most of the time I don't get a (complete) trace...
There was a similar problem reported recently. It turned out to be
caused by a __devinitconst annotation attached to a usb_device_id
table.
If there are any __devinit* annotations in the hid-picolcd driver, you
should see if removing them helps.
Alan Stern
^ permalink raw reply
* Re: [PATCH 0/7] HID: picoLCD updates
From: Bruno Prémont @ 2012-08-18 12:40 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, linux-kernel, linux-fbdev, Alan Stern
In-Reply-To: <alpine.LNX.2.00.1208161846240.28475@pobox.suse.cz>
[-- Attachment #1: Type: text/plain, Size: 9570 bytes --]
Hi Jiri,
[CCing Alan Stern]
On Thu, 16 August 2012 Jiri Kosina <jkosina@suse.cz> wrote:
> On Thu, 16 Aug 2012, Bruno Prémont wrote:
>
> > > I don't really understand this explanation. Once usb_kill_urb() returns,
> > > the URB should be available for future use (and therefore all queues
> > > completely drained).
> >
> > I won't have time today to check, though my guess is that on each
> > echo $usb-id > bind; echo $usb-id > unbind
> > under /sys/bus/hid/drivers/hid-picolcd/ the USB urb queue fills a bit does
> > not get cleared.
> >
> > Is usb_kill_urb() called when unbinding just the specific hid driver?
>
> Yup, through hid_hw_stop() -> usbhid_stop().
>
> > If so my short timing between bind/unbind must be triggering something
> > else...
> >
> > Otherwise I'm missing something as at first time I got no "output queue full"
> > messages, but as I repeated the bind/unbind sequences the prints per bind/unbind
> > iteration increased in number.
> >
> > Anyhow, on Friday evening/week-end I will continue digging and report back with my
> > findings.
Huh, after changing some of the hid-picolcd data in order to have less racy
coupling between hid and framebuffer I'm now dying way too often in _mmx_memcpy
and most of the time I don't get a (complete) trace...
The only full trace I got was:
[ 3857.426136] BUG: unable to handle kernel paging request at dbdf9000
[ 3857.432555] IP: [<c11e2be7>] _mmx_memcpy+0x27/0x16c
[ 3857.435906] *pde = 1bebb063 *pte = 1bdf9161
[ 3857.435906] Oops: 0003 [#1]
[ 3857.435906] Modules linked in: hid_picolcd fb_sys_fops sysimgblt sysfillrect syscopyarea drm_kms_helper nfs3 nfs_acl nfs lockd sunrpc
[ 3857.435906] Pid: 1935, comm: bash Not tainted 3.6.0-rc1-jupiter-00363-g0e8ccbc #1 NVIDIA Corporation. nFORCE-MCP/MS-6373
[ 3857.435906] EIP: 0060:[<c11e2be7>] EFLAGS: 00010013 CPU: 0
[ 3857.435906] EIP is at _mmx_memcpy+0x27/0x16c
[ 3857.435906] EAX: dd40a000 EBX: dd6ea010 ECX: 035af77b EDX: dbdf4230
[ 3857.435906] ESI: dbe0d1b0 EDI: dbdf9000 EBP: dd40bea8 ESP: dd40be8c
[ 3857.435906] DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068
[ 3857.435906] CR0: 8005003b CR2: dbdf9000 CR3: 1d7ee000 CR4: 000007d0
[ 3857.435906] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[ 3857.435906] DR6: ffff0ff0 DR7: 00000400
[ 3857.435906] Process bash (pid: 1935, ti=dd40a000 task=dcdb6120 task.ti=dbce2000)
[ 3857.435906] Stack:
[ 3857.435906] 0d6d6d6f dd6eb49c dd40beb8 dbde0080 dd6ea010 dbde0080 dbd31010 dd40bed0
[ 3857.435906] c133b364 00000000 00000000 ddfaa0e0 dbd31010 dbdf4230 dd6ea010 00000046
[ 3857.435906] dbd31010 dd40bef0 c133bea0 00000000 dbd0c3f0 00000000 dbd0c3f0 00000000
[ 3857.435906] Call Trace:
[ 3857.435906] [<c133b364>] hid_submit_out+0xa4/0x130
[ 3857.435906] [<c133bea0>] hid_irq_out+0xa0/0x100
[ 3857.435906] [<c12e425e>] usb_hcd_giveback_urb+0x4e/0x90
[ 3857.435906] [<c12f9bd2>] finish_urb+0xb2/0xf0
[ 3857.435906] [<c12fa67d>] takeback_td+0x3d/0x100
[ 3857.435906] [<c12faccf>] dl_done_list+0x14f/0x180
[ 3857.435906] [<c12fc351>] ohci_irq+0x191/0x300
[ 3857.435906] [<c1077790>] ? unmask_irq+0x20/0x20
[ 3857.435906] [<c12e3ace>] usb_hcd_irq+0x1e/0x40
[ 3857.435906] [<c107549d>] handle_irq_event_percpu+0x6d/0x1c0
[ 3857.435906] [<c1077790>] ? unmask_irq+0x20/0x20
[ 3857.435906] [<c1075618>] handle_irq_event+0x28/0x40
[ 3857.435906] [<c107781a>] handle_fasteoi_irq+0x8a/0xe0
[ 3857.435906] <IRQ>
[ 3857.435906] [<c1003e0a>] ? do_IRQ+0x3a/0xb0
[ 3857.435906] [<c140c870>] ? common_interrupt+0x30/0x38
[ 3857.435906] [<c1035143>] ? __do_softirq+0x53/0x1c0
[ 3857.435906] [<c10350f0>] ? local_bh_enable_ip+0x80/0x80
[ 3857.435906] <IRQ>
[ 3857.435906] [<c103544e>] ? irq_exit+0x3e/0xa0
[ 3857.435906] [<c101dfde>] ? smp_apic_timer_interrupt+0x6e/0x80
[ 3857.435906] [<c140bee1>] ? apic_timer_interrupt+0x31/0x38
[ 3857.435906] [<c102fdba>] ? vprintk_emit+0x34a/0x390
[ 3857.435906] [<c1409f58>] ? printk+0x38/0x3a
[ 3857.435906] [<de94b021>] ? picolcd_init_framebuffer+0x261/0x350 [hid_picolcd]
[ 3857.435906] [<de949bd1>] ? picolcd_probe+0x3d1/0x5a0 [hid_picolcd]
[ 3857.435906] [<c13321f7>] ? hid_device_probe+0x67/0xf0
[ 3857.435906] [<c128ac17>] ? driver_sysfs_add+0x57/0x80
[ 3857.435906] [<c128af1d>] ? driver_probe_device+0xbd/0x1d0
[ 3857.435906] [<c1331aab>] ? hid_match_device+0x7b/0x90
[ 3857.435906] [<c1289e65>] ? driver_bind+0x75/0xd0
[ 3857.435906] [<c1289df0>] ? driver_unbind+0x90/0x90
[ 3857.435906] [<c1289537>] ? drv_attr_store+0x27/0x30
[ 3857.435906] [<c111a59c>] ? sysfs_write_file+0xac/0xf0
[ 3857.435906] [<c10cca0c>] ? vfs_write+0x9c/0x130
[ 3857.435906] [<c10da2ef>] ? sys_dup3+0x11f/0x160
[ 3857.435906] [<c111a4f0>] ? sysfs_poll+0x90/0x90
[ 3857.435906] [<c10ccc7d>] ? sys_write+0x3d/0x70
[ 3857.435906] [<c140c357>] ? sysenter_do_call+0x12/0x26
[ 3857.435906] Code: 90 90 90 90 55 89 e5 57 56 89 d6 53 83 ec 10 89 45 f0 89 4d e4 89 e0 25 00 e0 ff ff f7 40 14 00 ff ff 07 74 17 c1 e9 02 8b 7d f0 <f3> a5 8b 4d e4 83 e1 03 74 02 f3 a4 e9 27 01 00 00 8b 45 1
[ 3857.435906] EIP: [<c11e2be7>] _mmx_memcpy+0x27/0x16c SS:ESP 0068:dd40be8c
[ 3857.435906] CR2: 00000000dbdf9000
[ 3857.435906] ---[ end trace 10ae520e65e3b763 ]---
[ 3857.435906] Kernel panic - not syncing: Fatal exception in interrupt
It happens around the time of calling
device_create_file(dev, &dev_attr_fb_update_rate);
that is, after picolcd_fb_reset(data, 1) has returned but before
calling fb_deferred_io_init(info); according to printk() I put
around the various calls in picolcd_init_framebuffer().
The other case I got part of a trace it was again in _mmx_memcpy()
but the trace output stopped in the middle of register dumping.
The full trace happened after the following sequence:
- system boot
- hotplug of picolcd (triggering modprobe hid-picolcd)
- cd /sys/bus/hid/drivers/hid-picolcd/
- echo 0003:04D8:C002.0003 > unbind
- wait some time
- echo 0003:04D8:C002.0003 > bind
Any idea what it could be? It might be during processing of first
reports send in order to clear the picolcd display.
As (un)binding in hid-picolcd crashes I tried doing so a level above,
at usbhid driver on usb bus which resulted in the following:
- system boot
- hotplug of picolcd (triggering modprobe hid-picolcd)
- cd /sys/bus/usb/drivers/usbhid/
- echo 2-1:1.0 > unbind
- wait some time
- echo 2-1:1.0 > bind
- wait some time
- echo 2-1:1.0 > unbind
produced the following trace:
[ 1418.387466] BUG: unable to handle kernel NULL pointer dereference at 00000030
[ 1418.390013] IP: [<c1339c40>] hid_submit_ctrl+0x80/0x2a0
[ 1418.390013] *pde = 00000000
[ 1418.390013] Oops: 0002 [#1]
[ 1418.390013] Modules linked in: hid_picolcd fb_sys_fops sysimgblt sysfillrect syscopyarea drm_kms_helper nfs3 nfs_acl nfs lockd sunrpc
[ 1418.390013] Pid: 1994, comm: kworker/0:0 Not tainted 3.6.0-rc1-jupiter-00363-g0e8ccbc #1 NVIDIA Corporation. nFORCE-MCP/MS-6373
[ 1418.390013] EIP: 0060:[<c1339c40>] EFLAGS: 00010086 CPU: 0
[ 1418.390013] EIP is at hid_submit_ctrl+0x80/0x2a0
[ 1418.390013] EAX: 80000200 EBX: dc2d0000 ECX: dcfe2020 EDX: 00000000
[ 1418.390013] ESI: 00000040 EDI: dc1f8f00 EBP: dd743e2c ESP: dd743df4
[ 1418.390013] DS: 007b ES: 007b FS: 0000 GS: 00e0 SS: 0068
[ 1418.390013] CR0: 8005003b CR2: 00000030 CR3: 1cd42000 CR4: 000007d0
[ 1418.390013] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
[ 1418.390013] DR6: ffff0ff0 DR7: 00000400
[ 1418.390013] Process kworker/0:0 (pid: 1994, ti=dd742000 task=dcdd0d40 task.ti=dd742000)
[ 1418.390013] Stack:
[ 1418.390013] 0000003e dc2f9020 dcf64c7c 00000005 00000002 dd743e1c 00000002 dc1f8f00
[ 1418.390013] 00000040 dcfe2020 dc2cb4d0 dc2d0000 00000000 dcfe2020 dd743e44 c1339ef6
[ 1418.390013] 00000008 dc2d0000 dc2cb4d0 dcfe2020 dd743e74 c133bbeb 00000023 000000ff
[ 1418.390013] Call Trace:
[ 1418.390013] [<c1339ef6>] usbhid_restart_ctrl_queue+0x96/0xe0
[ 1418.390013] [<c133bbeb>] __usbhid_submit_report+0x29b/0x320
[ 1418.390013] [<c1330200>] ? hid_parser_local+0x260/0x2d0
[ 1418.390013] [<c133bd2b>] usbhid_submit_report+0x1b/0x30
[ 1418.390013] [<de996cb9>] picolcd_fb_deferred_io+0x3b9/0x4c0 [hid_picolcd]
[ 1418.390013] [<c120e118>] fb_deferred_io_work+0x78/0xc0
[ 1418.390013] [<c1045e3f>] process_one_work+0x1ff/0x380
[ 1418.390013] [<c120e0a0>] ? fb_deferred_io_fault+0xa0/0xa0
[ 1418.390013] [<c10467f7>] worker_thread+0x237/0x360
[ 1418.390013] [<c10465c0>] ? manage_workers.clone.25+0x250/0x250
[ 1418.390013] [<c10465c0>] ? manage_workers.clone.25+0x250/0x250
[ 1418.390013] [<c104a30d>] kthread+0x6d/0x80
[ 1418.390013] [<c104a2a0>] ? kthread_freezable_should_stop+0x70/0x70
[ 1418.390013] [<c140c87e>] kernel_thread_helper+0x6/0xd
[ 1418.390013] Code: 01 89 45 e8 0f 85 91 00 00 00 8b 4d ec 8b 53 1c 8b 7d e4 8b 75 e8 8b 81 68 0c 00 00 8b 00 8b 40 9c c1 e0 08 0d 00 00 00 80 85 ff <89> 42 30 8b 43 1c 89 70 54 0f 84 19 01 00 00 81 fe ff 01 0
[ 1418.390013] EIP: [<c1339c40>] hid_submit_ctrl+0x80/0x2a0 SS:ESP 0068:dd743df4
[ 1418.390013] CR2: 0000000000000030
[ 1418.390013] ---[ end trace 089e20ed16261a0a ]---
pretty early during unbind process, e.g. way before picolcd even started to
dispose framebuffer.
Either I'm doing something really wrong or I'm the first one to produce
lots of outbound reports thus exploring corner-cases...
PS: attached is a diff between the hid-picolcd from this patch series to what
I'm testing right now.
Thanks,
Bruno
[-- Attachment #2: pico.diff --]
[-- Type: text/x-patch, Size: 16453 bytes --]
drivers/hid/hid-picolcd.h | 19 +++-
drivers/hid/hid-picolcd_core.c | 6 +
drivers/hid/hid-picolcd_fb.c | 222 +++++++++++++++++++++-------------------
3 files changed, 138 insertions(+), 109 deletions(-)
diff --git a/drivers/hid/hid-picolcd.h b/drivers/hid/hid-picolcd.h
index 9200be1..dc4c795 100644
--- a/drivers/hid/hid-picolcd.h
+++ b/drivers/hid/hid-picolcd.h
@@ -90,11 +90,6 @@ struct picolcd_data {
#ifdef CONFIG_HID_PICOLCD_FB
/* Framebuffer stuff */
- u8 fb_update_rate;
- u8 fb_bpp;
- u8 fb_force;
- u8 *fb_vbitmap; /* local copy of what was sent to PicoLCD */
- u8 *fb_bitmap; /* framebuffer */
struct fb_info *fb_info;
#endif /* CONFIG_HID_PICOLCD_FB */
#ifdef CONFIG_HID_PICOLCD_LCD
@@ -119,9 +114,21 @@ struct picolcd_data {
int status;
#define PICOLCD_BOOTLOADER 1
#define PICOLCD_FAILED 2
-#define PICOLCD_READY_FB 4
};
+#ifdef CONFIG_HID_PICOLCD_FB
+struct picolcd_fb_data {
+ /* Framebuffer stuff */
+ spinlock_t lock;
+ struct picolcd_data *picolcd;
+ u8 update_rate;
+ u8 bpp;
+ u8 force;
+ u8 ready;
+ u8 *vbitmap; /* local copy of what was sent to PicoLCD */
+ u8 *bitmap; /* framebuffer */
+};
+#endif /* CONFIG_HID_PICOLCD_FB */
/* Find a given report */
#define picolcd_in_report(id, dev) picolcd_report(id, dev, HID_INPUT_REPORT)
diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
index e08ffd2..153acc2 100644
--- a/drivers/hid/hid-picolcd_core.c
+++ b/drivers/hid/hid-picolcd_core.c
@@ -539,6 +539,7 @@ static int picolcd_probe(struct hid_device *hdev,
int error = -ENOMEM;
dbg_hid(PICOLCD_NAME " hardware probe...\n");
+ printk(KERN_INFO "hid-picolcd: probe() 1/4\n");
/*
* Let's allocate the picolcd data structure, set some reasonable
@@ -559,6 +560,7 @@ static int picolcd_probe(struct hid_device *hdev,
data->status |= PICOLCD_BOOTLOADER;
hid_set_drvdata(hdev, data);
+ printk(KERN_INFO "hid-picolcd: probe(%p) 2/4\n", data);
/* Parse the device reports and start it up */
error = hid_parse(hdev);
if (error) {
@@ -578,6 +580,7 @@ static int picolcd_probe(struct hid_device *hdev,
goto err_cleanup_hid_hw;
}
+ printk(KERN_INFO "hid-picolcd: probe(%p) 3/4\n", data);
error = device_create_file(&hdev->dev, &dev_attr_operation_mode_delay);
if (error) {
hid_err(hdev, "failed to create sysfs attributes\n");
@@ -597,6 +600,7 @@ static int picolcd_probe(struct hid_device *hdev,
if (error)
goto err_cleanup_sysfs2;
+ printk(KERN_INFO "hid-picolcd: probe(%p, [fb=%p]) 4/4\n", data, data->fb_info);
dbg_hid(PICOLCD_NAME " activated and initialized\n");
return 0;
@@ -621,6 +625,7 @@ static void picolcd_remove(struct hid_device *hdev)
struct picolcd_data *data = hid_get_drvdata(hdev);
unsigned long flags;
+ printk(KERN_INFO "hid-picolcd: remove(%p [fb=%p]) ...\n", data, data->fb_info);
dbg_hid(PICOLCD_NAME " hardware remove...\n");
spin_lock_irqsave(&data->lock, flags);
data->status |= PICOLCD_FAILED;
@@ -649,6 +654,7 @@ static void picolcd_remove(struct hid_device *hdev)
picolcd_exit_cir(data);
picolcd_exit_keys(data);
+ printk(KERN_INFO "hid-picolcd: remove(%p [fb=%p]) done\n", data, data->fb_info);
mutex_destroy(&data->mutex);
/* Finally, clean up the picolcd data itself */
kfree(data);
diff --git a/drivers/hid/hid-picolcd_fb.c b/drivers/hid/hid-picolcd_fb.c
index 4d8e22c..86b7a24 100644
--- a/drivers/hid/hid-picolcd_fb.c
+++ b/drivers/hid/hid-picolcd_fb.c
@@ -98,16 +98,19 @@ static const struct fb_var_screeninfo picolcdfb_var = {
};
/* Send a given tile to PicoLCD */
-static int picolcd_fb_send_tile(struct hid_device *hdev, int chip, int tile)
+static int picolcd_fb_send_tile(struct picolcd_data *data, u8 *vbitmap,
+ int chip, int tile)
{
- struct picolcd_data *data = hid_get_drvdata(hdev);
- struct hid_report *report1 = picolcd_out_report(REPORT_LCD_CMD_DATA, hdev);
- struct hid_report *report2 = picolcd_out_report(REPORT_LCD_DATA, hdev);
+ struct hid_report *report1, *report2;
unsigned long flags;
u8 *tdata;
int i;
- if (!report1 || report1->maxfield != 1 || !report2 || report2->maxfield != 1)
+ report1 = picolcd_out_report(REPORT_LCD_CMD_DATA, data->hdev);
+ if (!report1 || report1->maxfield != 1)
+ return -ENODEV;
+ report2 = picolcd_out_report(REPORT_LCD_DATA, data->hdev);
+ if (!report2 || report2->maxfield != 1)
return -ENODEV;
spin_lock_irqsave(&data->lock, flags);
@@ -128,7 +131,7 @@ static int picolcd_fb_send_tile(struct hid_device *hdev, int chip, int tile)
hid_set_field(report2->field[0], 2, 0x00);
hid_set_field(report2->field[0], 3, 32);
- tdata = data->fb_vbitmap + (tile * 4 + chip) * 64;
+ tdata = vbitmap + (tile * 4 + chip) * 64;
for (i = 0; i < 64; i++)
if (i < 32)
hid_set_field(report1->field[0], 11 + i, tdata[i]);
@@ -189,6 +192,7 @@ void picolcd_fb_refresh(struct picolcd_data *data)
int picolcd_fb_reset(struct picolcd_data *data, int clear)
{
struct hid_report *report = picolcd_out_report(REPORT_LCD_CMD, data->hdev);
+ struct picolcd_fb_data *fbdata = data->fb_info->par;
int i, j;
unsigned long flags;
static const u8 mapcmd[8] = { 0x00, 0x02, 0x00, 0x64, 0x3f, 0x00, 0x64, 0xc0 };
@@ -207,20 +211,19 @@ int picolcd_fb_reset(struct picolcd_data *data, int clear)
hid_set_field(report->field[0], j, 0);
usbhid_submit_report(data->hdev, report, USB_DIR_OUT);
}
-
- data->status |= PICOLCD_READY_FB;
spin_unlock_irqrestore(&data->lock, flags);
- if (data->fb_bitmap) {
- if (clear) {
- memset(data->fb_vbitmap, 0, PICOLCDFB_SIZE);
- memset(data->fb_bitmap, 0, PICOLCDFB_SIZE*data->fb_bpp);
- }
- data->fb_force = 1;
+ if (clear) {
+ memset(fbdata->vbitmap, 0, PICOLCDFB_SIZE);
+ memset(fbdata->bitmap, 0, PICOLCDFB_SIZE*fbdata->bpp);
}
+ fbdata->force = 1;
/* schedule first output of framebuffer */
- picolcd_fb_refresh(data);
+ if (fbdata->ready)
+ schedule_delayed_work(&data->fb_info->deferred_work, 0);
+ else
+ fbdata->ready = 1;
return 0;
}
@@ -230,20 +233,15 @@ static void picolcd_fb_update(struct fb_info *info)
{
int chip, tile, n;
unsigned long flags;
+ struct picolcd_fb_data *fbdata = info->par;
struct picolcd_data *data;
mutex_lock(&info->lock);
- data = info->par;
- if (!data)
- goto out;
- spin_lock_irqsave(&data->lock, flags);
- if (!(data->status & PICOLCD_READY_FB)) {
- spin_unlock_irqrestore(&data->lock, flags);
- picolcd_fb_reset(data, 0);
- } else {
- spin_unlock_irqrestore(&data->lock, flags);
- }
+ spin_lock_irqsave(&fbdata->lock, flags);
+ if (!fbdata->ready && fbdata->picolcd)
+ picolcd_fb_reset(fbdata->picolcd, 0);
+ spin_unlock_irqrestore(&fbdata->lock, flags);
/*
* Translate the framebuffer into the format needed by the PicoLCD.
@@ -254,32 +252,38 @@ static void picolcd_fb_update(struct fb_info *info)
*/
n = 0;
for (chip = 0; chip < 4; chip++)
- for (tile = 0; tile < 8; tile++)
- if (picolcd_fb_update_tile(data->fb_vbitmap,
- data->fb_bitmap, data->fb_bpp, chip, tile) ||
- data->fb_force) {
- n += 2;
- if (n >= HID_OUTPUT_FIFO_SIZE / 2) {
- mutex_unlock(&info->lock);
- usbhid_wait_io(data->hdev);
- mutex_lock(&info->lock);
- data = info->par;
- if (!data)
- goto out;
- spin_lock_irqsave(&data->lock, flags);
- if (data->status & PICOLCD_FAILED) {
- spin_unlock_irqrestore(&data->lock, flags);
- goto out;
- }
- spin_unlock_irqrestore(&data->lock, flags);
- n = 0;
- }
- picolcd_fb_send_tile(data->hdev, chip, tile);
+ for (tile = 0; tile < 8; tile++) {
+ if (!fbdata->force && !picolcd_fb_update_tile(
+ fbdata->vbitmap, fbdata->bitmap,
+ fbdata->bpp, chip, tile))
+ continue;
+ n += 2;
+ if (n >= HID_OUTPUT_FIFO_SIZE / 2) {
+ spin_lock_irqsave(&fbdata->lock, flags);
+ data = fbdata->picolcd;
+ spin_unlock_irqrestore(&fbdata->lock, flags);
+ mutex_unlock(&info->lock);
+ if (!data)
+ return;
+ usbhid_wait_io(data->hdev);
+ mutex_lock(&info->lock);
+ n = 0;
}
- data->fb_force = false;
+ spin_lock_irqsave(&fbdata->lock, flags);
+ data = fbdata->picolcd;
+ spin_unlock_irqrestore(&fbdata->lock, flags);
+ if (!data)
+ goto out;
+ picolcd_fb_send_tile(data, fbdata->vbitmap, chip, tile);
+ }
+ fbdata->force = false;
if (n) {
+ spin_lock_irqsave(&fbdata->lock, flags);
+ data = fbdata->picolcd;
+ spin_unlock_irqrestore(&fbdata->lock, flags);
mutex_unlock(&info->lock);
- usbhid_wait_io(data->hdev);
+ if (data)
+ usbhid_wait_io(data->hdev);
return;
}
out:
@@ -336,20 +340,24 @@ static ssize_t picolcd_fb_write(struct fb_info *info, const char __user *buf,
static int picolcd_fb_blank(int blank, struct fb_info *info)
{
- if (!info->par)
- return -ENODEV;
/* We let fb notification do this for us via lcd/backlight device */
return 0;
}
static void picolcd_fb_destroy(struct fb_info *info)
{
+ struct picolcd_fb_data *fbdata = info->par;
+
+ printk(KERN_INFO "hid-picolcd: fb_destroy(%p [fb=%p]) ...\n", fbdata->picolcd, info);
/* make sure no work is deferred */
fb_deferred_io_cleanup(info);
+ /* No thridparty should ever unregister our framebuffer! */
+ WARN_ON(fbdata->picolcd != NULL);
+
vfree((u8 *)info->fix.smem_start);
framebuffer_release(info);
- printk(KERN_DEBUG "picolcd_fb_destroy(%p)\n", info);
+ printk(KERN_INFO "hid-picolcd: fb_destroy(-- [fb=%p]) done\n", info);
}
static int picolcd_fb_check_var(struct fb_var_screeninfo *var, struct fb_info *info)
@@ -376,17 +384,15 @@ static int picolcd_fb_check_var(struct fb_var_screeninfo *var, struct fb_info *i
static int picolcd_set_par(struct fb_info *info)
{
- struct picolcd_data *data = info->par;
+ struct picolcd_fb_data *fbdata = info->par;
u8 *tmp_fb, *o_fb;
- if (!data)
- return -ENODEV;
- if (info->var.bits_per_pixel == data->fb_bpp)
+ if (info->var.bits_per_pixel == fbdata->bpp)
return 0;
/* switch between 1/8 bit depths */
if (info->var.bits_per_pixel != 1 && info->var.bits_per_pixel != 8)
return -EINVAL;
- o_fb = data->fb_bitmap;
+ o_fb = fbdata->bitmap;
tmp_fb = kmalloc(PICOLCDFB_SIZE*info->var.bits_per_pixel, GFP_KERNEL);
if (!tmp_fb)
return -ENOMEM;
@@ -415,7 +421,7 @@ static int picolcd_set_par(struct fb_info *info)
}
kfree(tmp_fb);
- data->fb_bpp = info->var.bits_per_pixel;
+ fbdata->bpp = info->var.bits_per_pixel;
return 0;
}
@@ -453,7 +459,8 @@ static ssize_t picolcd_fb_update_rate_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct picolcd_data *data = dev_get_drvdata(dev);
- unsigned i, fb_update_rate = data->fb_update_rate;
+ struct picolcd_fb_data *fbdata = data->fb_info->par;
+ unsigned i, fb_update_rate = fbdata->update_rate;
size_t ret = 0;
for (i = 1; i <= PICOLCDFB_UPDATE_RATE_LIMIT; i++)
@@ -472,6 +479,7 @@ static ssize_t picolcd_fb_update_rate_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t count)
{
struct picolcd_data *data = dev_get_drvdata(dev);
+ struct picolcd_fb_data *fbdata = data->fb_info->par;
int i;
unsigned u;
@@ -487,8 +495,8 @@ static ssize_t picolcd_fb_update_rate_store(struct device *dev,
else if (u == 0)
u = PICOLCDFB_UPDATE_RATE_DEFAULT;
- data->fb_update_rate = u;
- data->fb_info->fbdefio->delay = HZ / data->fb_update_rate;
+ fbdata->update_rate = u;
+ data->fb_info->fbdefio->delay = HZ / fbdata->update_rate;
return count;
}
@@ -500,30 +508,18 @@ int picolcd_init_framebuffer(struct picolcd_data *data)
{
struct device *dev = &data->hdev->dev;
struct fb_info *info = NULL;
+ struct picolcd_fb_data *fbdata = NULL;
int i, error = -ENOMEM;
- u8 *fb_vbitmap = NULL;
- u8 *fb_bitmap = NULL;
u32 *palette;
- fb_bitmap = vmalloc(PICOLCDFB_SIZE*8);
- if (fb_bitmap == NULL) {
- dev_err(dev, "can't get a free page for framebuffer\n");
- goto err_nomem;
- }
-
- fb_vbitmap = kmalloc(PICOLCDFB_SIZE, GFP_KERNEL);
- if (fb_vbitmap == NULL) {
- dev_err(dev, "can't alloc vbitmap image buffer\n");
- goto err_nomem;
- }
-
- data->fb_update_rate = PICOLCDFB_UPDATE_RATE_DEFAULT;
/* The extra memory is:
* - 256*u32 for pseudo_palette
* - struct fb_deferred_io
*/
info = framebuffer_alloc(256 * sizeof(u32) +
- sizeof(struct fb_deferred_io), dev);
+ sizeof(struct fb_deferred_io) +
+ sizeof(struct picolcd_fb_data) +
+ PICOLCDFB_SIZE, dev);
if (info == NULL) {
dev_err(dev, "failed to allocate a framebuffer\n");
goto err_nomem;
@@ -531,74 +527,94 @@ int picolcd_init_framebuffer(struct picolcd_data *data)
info->fbdefio = info->par;
*info->fbdefio = picolcd_fb_defio;
- palette = info->par + sizeof(struct fb_deferred_io);
+ info->par += sizeof(struct fb_deferred_io);
+ palette = info->par;
+ info->par += 256 * sizeof(u32);
for (i = 0; i < 256; i++)
palette[i] = i > 0 && i < 16 ? 0xff : 0;
info->pseudo_palette = palette;
- info->screen_base = (char __force __iomem *)fb_bitmap;
info->fbops = &picolcdfb_ops;
info->var = picolcdfb_var;
info->fix = picolcdfb_fix;
info->fix.smem_len = PICOLCDFB_SIZE*8;
- info->fix.smem_start = (unsigned long)fb_bitmap;
- info->par = data;
info->flags = FBINFO_FLAG_DEFAULT;
- data->fb_vbitmap = fb_vbitmap;
- data->fb_bitmap = fb_bitmap;
- data->fb_bpp = picolcdfb_var.bits_per_pixel;
+ fbdata = info->par;
+ spin_lock_init(&fbdata->lock);
+ fbdata->picolcd = data;
+ fbdata->update_rate = PICOLCDFB_UPDATE_RATE_DEFAULT;
+ fbdata->bpp = picolcdfb_var.bits_per_pixel;
+ fbdata->force = 1;
+ fbdata->vbitmap = info->par + sizeof(struct picolcd_fb_data);
+ fbdata->bitmap = vmalloc(PICOLCDFB_SIZE*8);
+ if (fbdata->bitmap == NULL) {
+ dev_err(dev, "can't get a free page for framebuffer\n");
+ goto err_nomem;
+ }
+ info->screen_base = (char __force __iomem *)fbdata->bitmap;
+ info->fix.smem_start = (unsigned long)fbdata->bitmap;
+ memset(fbdata->vbitmap, 0xff, PICOLCDFB_SIZE);
+ data->fb_info = info;
+
+ printk(KERN_INFO "hid-picolcd: init_framebuffer(%p [fb=%p]) 1/5\n", data, info);
error = picolcd_fb_reset(data, 1);
if (error) {
dev_err(dev, "failed to configure display\n");
goto err_cleanup;
}
+
+ printk(KERN_INFO "hid-picolcd: init_framebuffer(%p [fb=%p]) 2/5\n", data, info);
error = device_create_file(dev, &dev_attr_fb_update_rate);
if (error) {
dev_err(dev, "failed to create sysfs attributes\n");
goto err_cleanup;
}
+
+ printk(KERN_INFO "hid-picolcd: init_framebuffer(%p [fb=%p]) 3/5\n", data, info);
fb_deferred_io_init(info);
- data->fb_info = info;
+ printk(KERN_INFO "hid-picolcd: init_framebuffer(%p [fb=%p]) 4/5\n", data, info);
error = register_framebuffer(info);
if (error) {
dev_err(dev, "failed to register framebuffer\n");
goto err_sysfs;
}
- /* schedule first output of framebuffer */
- data->fb_force = 1;
- schedule_delayed_work(&info->deferred_work, 0);
+ printk(KERN_INFO "hid-picolcd: init_framebuffer(%p [fb=%p]) 5/5\n", data, info);
return 0;
err_sysfs:
- fb_deferred_io_cleanup(info);
device_remove_file(dev, &dev_attr_fb_update_rate);
+ fb_deferred_io_cleanup(info);
err_cleanup:
- data->fb_vbitmap = NULL;
- data->fb_bitmap = NULL;
- data->fb_bpp = 0;
data->fb_info = NULL;
err_nomem:
+ if (fbdata)
+ vfree(fbdata->bitmap);
framebuffer_release(info);
- vfree(fb_bitmap);
- kfree(fb_vbitmap);
+ printk(KERN_INFO "hid-picolcd: init_framebuffer(%p [fb=%p]) failed\n", data, info);
return error;
}
void picolcd_exit_framebuffer(struct picolcd_data *data)
{
struct fb_info *info = data->fb_info;
- u8 *fb_vbitmap = data->fb_vbitmap;
-
- if (!info)
- return;
+ struct picolcd_fb_data *fbdata = info->par;
+ unsigned long flags;
+ printk(KERN_INFO "hid-picolcd: exit_framebuffer(%p [fb=%p]) ...\n", data, info);
device_remove_file(&data->hdev->dev, &dev_attr_fb_update_rate);
- info->par = NULL;
+
+ /* disconnect framebuffer from HID dev */
+ spin_lock_irqsave(&fbdata->lock, flags);
+ fbdata->picolcd = NULL;
+ spin_unlock_irqrestore(&fbdata->lock, flags);
+
+ /* make sure there is no running update - thus that fbdata->picolcd
+ * once obtained under lock is guaranteed not to get free() under
+ * the feet of the deferred work */
+ flush_delayed_work_sync(&info->deferred_work);
+
+ data->fb_info = NULL;
unregister_framebuffer(info);
- data->fb_vbitmap = NULL;
- data->fb_bitmap = NULL;
- data->fb_bpp = 0;
- data->fb_info = NULL;
- kfree(fb_vbitmap);
+ printk(KERN_INFO "hid-picolcd: exit_framebuffer(%p [fb=%p]) done\n", data, info);
}
^ permalink raw reply related
* Re: [UDL] general protection fault in fb_deferred_io_mkwrite()
From: Thomas Meyer @ 2012-08-18 11:00 UTC (permalink / raw)
To: Bernie Thompson; +Cc: linux-fbdev, linux-kernel, airlied, dri-devel
In-Reply-To: <CAO1w=s8AdkbSNMe8v0HqkosYNKp05+Udk4d-r1TbRb3GMC-MNw@mail.gmail.com>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1254", Size: 1715 bytes --]
Am Sonntag, den 12.08.2012, 14:22 -0700 schrieb Bernie Thompson:
> On Sun, Aug 12, 2012 at 3:34 AM, Thomas Meyer <thomas@m3y3r.de> wrote:
> guilty driver is probably udl_fb.c
> any ideas?
>
>
> Hi Thomas,
Hi Bernie!
> We were seeing similar issues in udlfb (the original fbdev version of
> this driver), which were fixed earlier this year by getting all
> rendering operations out of probe/disconnect -- those which might
> trigger fb_defio page faults in an inappropriate context, or be
> long-running. Here's some more detail:
> http://plugable.com/2012/06/21/displaylink-usb-devices-on-linux-kernel-3-4-0/comment-page-1/#comment-5896
>
>
> Unfortunately, I haven't had time to get going with udl myself, so
> haven't been able to port and confirm. Thanks for raising and staying
> on this.
Okay, I see. I'll switch to FB_UDL for now and remove DRM_UDL from my
config.
Is somebody working on porting commit
https://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h21547d3c9c3bc653261f26d554cfabc4a083de to the DRM_UDL driver?
In Airlie's tree seems to be no commit related to this:
http://cgit.freedesktop.org/~airlied/linux/
with kind regards
thomas
>
>
> Best wishes,
> Bernie
>
>
> [ 45.633336] RIP [<ffffffff8123becc>]
> fb_deferred_io_mkwrite+0xdc/0xf0
> [ 45.633336] RSP <ffff880126559c98>
> [ 45.711547] ---[ end trace d4732d5a0bf375fb ]---
> [ 45.720961] released /dev/fb1 user=1 count=0
>
>
--
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
^ permalink raw reply
* Re: [RFC 0/5] Generic panel framework
From: Laurent Pinchart @ 2012-08-18 1:16 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: linux-fbdev, Marcus Lorentzon, dri-devel, Kyungmin Park,
Richard Purdie, Sebastien Guiriec, Bryan Wu, linux-leds,
linux-media
In-Reply-To: <1345203751.3158.99.camel@deskari>
Hi Tomi,
On Friday 17 August 2012 14:42:31 Tomi Valkeinen wrote:
> On Fri, 2012-08-17 at 13:10 +0200, Laurent Pinchart wrote:
> > What kind of directory structure do you have in mind ? Panels are already
> > isolated in drivers/video/panel/ so we could already ditch the panel-
> > prefix in drivers.
>
> The same directory also contains files for the framework and buses. But
> perhaps there's no need for additional directories if the amount of
> non-panel files is small. And you can easily see from the name that they
> are not panel drivers (e.g. mipi_dbi_bus.c).
I don't expect the directory to contain many non-panel files, so let's keep it
as-is for now.
mipi-dbi-bus might not belong to include/video/panel/ though, as it can be
used for non-panel devices (at least in theory). The future mipi-dsi-bus
certainly will.
> > Would you also create include/video/panel/ ?
>
> Perhaps that would be good. Well, having all the files prefixed with
> panel- is not bad as such, but just feel extra.
>
> > > ---
> > >
> > > Should we aim for DT only solution from the start? DT is the direction
> > > we are going, and I feel the older platform data stuff would be
> > > deprecated soon.
> >
> > Don't forget about non-ARM architectures :-/ We need panel drivers for SH
> > as well, which doesn't use DT. I don't think that would be a big issue, a
> > DT- compliant solution should be easy to use through board code and
> > platform data as well.
>
> I didn't forget about them as I didn't even think about them ;). I
> somehow had the impression that other architectures would also use DT,
> sooner or later. I could be mistaken, though.
>
> And true, it's not a big issue to support both DT and non-DT versions,
> but I've been porting omap stuff for DT and keeping the old platform
> data stuff also there, and it just feels burdensome. For very simple
> panels it's easy, but when you've passing lots of parameters the code
> starts to get longer.
>
> > > This one would be rather impossible with the upper layer handling the
> > > enabling of the video stream. Thus I see that the panel driver needs to
> > > control the sequences, and the Sharp panel driver's enable would look
> > > something like:
> > >
> > > regulator_enable(...);
> > > sleep();
> > > dpi_enable_video();
> > > sleep();
> > > gpip_set(..);
> >
> > I have to admit I have no better solution to propose at the moment, even
> > if I don't really like making the panel control the video stream. When
> > several devices will be present in the chain all of them might have
> > similar annoying requirements, and my feeling is that the resulting code
> > will be quite messy. At the end of the day the only way to really find
> > out is to write an implementation.
>
> If we have a chain of devices, and each device uses the bus interface
> from the previous device in the chain, there shouldn't be a problem. In
> that model each device can handle the task however is best for it.
>
> I think the problems come from the divided control we'll have. I mean,
> if the panel driver would decide itself what to send to its output, and
> it would provide the data (think of an i2c device), this would be very
> simple. And it actually is for things like configuration data etc, but
> not so for video stream.
Would you be able to send incremental patches on top of v2 to implement the
solution you have in mind ? It would be neat if you could also implement mipi-
dsi-bus for the OMAP DSS and test the code with a real device :-)
> > > It could cause some locking issues, though. First the panel's remove
> > > could take a lock, but the remove sequence would cause the display
> > > driver to call disable on the panel, which could again try to take the
> > > same lock...
> >
> > We have two possible ways of calling panel operations, either directly
> > (panel->bus->ops->enable(...)) or indirectly (panel_enable(...)).
> >
> > The former is what V4L2 currently does with subdevs, and requires display
> > drivers to hold a reference to the panel. The later can do without a
> > direct reference only if we use a global lock, which is something I would
> > like to
>
> Wouldn't panel_enable() just do the same panel->bus->ops->enable()
> anyway, and both require a panel reference? I don't see the difference.
Indeed, you're right. I'm not sure what I was thinking about.
> > avoid. A panel-wide lock wouldn't work, as the access function would need
> > to take the lock on a panel instance that can be removed at any time.
>
> Can't this be handled with some kind of get/put refcounting? If there's
> a ref, it can't be removed.
Trouble will come when the display driver will hold a reference to the panel,
and the panel will hold a reference to the display driver (for instance
because the display driver provides the DBI/DSI bus, or because it provides a
clock used by the panel).
> Generally about locks, if we define that panel ops may only be called
> exclusively, does it simplify things? I think we can make such
> requirements, as there should be only one display framework that handles
> the panel. Then we don't need locking for things like enable/disable.
Pushing locking to callers would indeed simplify panel drivers, but we need to
make sure we won't need to expose a panel to several callers in the future.
> Of course we need to be careful about things where calls come from
> "outside" the display framework. I guess one such thing is rmmod, but if
> that causes a notification to the display framework, which again handles
> locking, it shouldn't be a problem.
>
> Another thing to be careful about is if the panel internally uses irqs,
> workqueues, sysfs files or such. In that case it needs to handle
> locking.
Of course panels will need to manage concurrency for their own infrastructure.
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
From: Mark Brown @ 2012-08-17 23:04 UTC (permalink / raw)
To: Mitch Bradley
Cc: Stephen Warren, Alexandre Courbot, linux-fbdev, Stephen Warren,
linux-doc, linux-kernel, Rob Herring, Anton Vorontsov,
linux-tegra, David Woodhouse, devicetree-discuss
In-Reply-To: <502D61C6.5050101@firmworks.com>
[-- Attachment #1: Type: text/plain, Size: 1206 bytes --]
On Thu, Aug 16, 2012 at 11:10:30AM -1000, Mitch Bradley wrote:
> On 8/16/2012 8:38 AM, Stephen Warren wrote:
> > Device tree bindings shouldn't reference Linux documentation; the
> > bindings are supposed to be OS-agnostic.
> While it is true that bindings should try to be OS-agnostic, there is
> the practical matter of where to put documentation so that it is widely
> accessible. The Linux source tree is one of the most accessible things
> there is, considering how widely it is replicated.
> As the original instigator of the policy that the device tree should
> describe the hardware "OS-neutrally", I personally don't have a problem
> with bindings referring to Linux documentation. I wouldn't like
> references to proprietary and inaccessible documentation.
OS agnosticness isn't the only issue here - the other problem with using
Linux documentation is that except for things that are specifically
userspace interfaces and the DT bindings nothing is intended to be
stable so bindings defined in terms of Linux documentation may randomly
change. We're not doing an awesome job of that with DT right now but we
should try and so we ought to avoid including non-ABI things in ABIs
like this.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH] fbdev: Make the switch from generic to native driver less alarming
From: Adam Jackson @ 2012-08-17 17:10 UTC (permalink / raw)
To: linux-fbdev
Calling this "conflicting" just makes people think there's a problem
when there's not.
Signed-off-by: Adam Jackson <ajax@redhat.com>
---
drivers/video/fbmem.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 0dff12a..42be978 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1582,8 +1582,7 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
(primary && gen_aper && gen_aper->count &&
gen_aper->ranges[0].base = VGA_FB_PHYS)) {
- printk(KERN_INFO "fb: conflicting fb hw usage "
- "%s vs %s - removing generic driver\n",
+ printk(KERN_INFO "fb: switching to %s from %s\n",
name, registered_fb[i]->fix.id);
do_unregister_framebuffer(registered_fb[i]);
}
--
1.7.7.6
^ permalink raw reply related
* Re: [RFC 3/5] video: panel: Add MIPI DBI bus support
From: Laurent Pinchart @ 2012-08-17 14:06 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: linux-fbdev, dri-devel, linux-leds, linux-media, Bryan Wu,
Richard Purdie, Marcus Lorentzon, Sumit Semwal, Archit Taneja,
Sebastien Guiriec, Inki Dae, Kyungmin Park
In-Reply-To: <1345208790.3158.133.camel@deskari>
Hi Tomi,
On Friday 17 August 2012 16:06:30 Tomi Valkeinen wrote:
> On Fri, 2012-08-17 at 14:33 +0200, Laurent Pinchart wrote:
> > > But first, the data type should be byte, not unsigned long. How would
> > > you write 8 bits or 16 bits with your API?
> >
> > u8 and u16 both fit in an unsigned long :-) Please see below.
>
> Ah, I see, so the driver would just discard 24 bits or 16 bits from the
> ulong.
That's right.
> I somehow thought that if you have 8 bit bus, and you call the write with
> ulong, 4 bytes will be written.
>
> > > Then again, I'd hope to have DCS somehow as a separate library, which
> > > would then use DBI/DSI/whatnot to actually send the data.
> > >
> > > I'm not quite sure how easy that is because of the differences between
> > > the busses.
> > >
> > > > Is DBI limited to 8-bit data transfers for commands ? Pixels can be
> > > > transferred 16-bit at a time, commands might as well. While DCS only
> > > > specifies 8-bit command/data, DBI panels that are not DCS compliant
> > > > can use 16-bit command/data (the R61505 panel, albeit a SYS-80 panel,
> > > > does so).
> > >
> > > I have to say I don't remember much about DBI =). Looking at OMAP's
> > > driver, which was made for omap2 and hasn't been much updated since, I
> > > see that there are 4 modes, 8/9/12/16 bits. I think that defines how
> > > many of the parallel data lines are used.
> >
> > SYS-80 also has an 18-bits mode, where bits 0 and 9 are always ignored
> > when transferring instructions and data other than pixels (for pixels the
> > 18-bits bus width can be used to transfer RGB666 in a single clock cycle).
> >
> > See page 87 of
> > http://read.pudn.com/downloads91/sourcecode/others/348230/e61505_103a.pdf.
> >
> > > However, I don't think that matters for the panel driver when it wants
> > > to send data. The panel driver should just call dbi_write(buf, buf_len),
> > > and the dbi driver would send the data in the buffer according to the
> > > bus width.
> >
> > According to the DCS specification, commands and parameters are
> > transferred using 8-bit data. Non-DCS panels can however use wider
> > commands and parameters (all commands and parameters are 16-bits wide for
> > the R61505 for instance).
> >
> > We can add an API to switch the DBI bus width on the fly. For Renesas
> > hardware this would "just" require shifting bits around to output the
> > 8-bit or 16-bit commands on the right data lines (the R61505 uses D17-D9
> > in 8-bit mode, while the DCS specification mentions D7-D0) based on how
> > the panel is connected and on which lines the panel expects data.
> >
> > As commands can be expressed on either 8 or 16 bits I would use a 16 type
> > for them.
>
> I won't put my head on the block, but I don't think DBI has any
> restriction on the size of the command. A "command" just means a data
> transfer while keeping the D/CX line low, and "data" when the line is
> high. Similar transfers for n bytes can be done in both modes.
Right. I'll see if the API could be simplified by having a single write
callback function with a data/command parameter.
> > For parameters, we can either express everything as u8 * in the DBI bus
> > operations, or use a union similar to i2c_smbus_data
> >
> > union i2c_smbus_data {
> >
> > __u8 byte;
> > __u16 word;
> > __u8 block[I2C_SMBUS_BLOCK_MAX + 2]; /* block[0] is used for
> > length */
> >
> > /* and one more for user-space
> > compatibility */
> >
> > };
>
> There's no DBI_BLOCK_MAX, so at least identical union won't work. I
> think it's simplest to have u8 * function as a base, and then a few
> helpers to write the most common datatypes.
OK, that sounds good to me.
> So we could have on the lowest level something like:
>
> dbi_write_command(u8 *buf, size_t size);
> dbi_write_data(u8 *buf, size_t size);
>
> And possible helpers:
>
> dbi_write_data(u8 *cmd_buf, size_t cmd_size, u8 *data_buf, size_t
> data_size);
>
> dbi_write_dcs(u8 cmd, u8 *data, size_t size);
>
> And variations:
>
> dbi_write_dcs_0(u8 cmd);
> dbi_write_dcs_1(u8 cmd, u8 data);
>
> etc. So a simple helper to send 16 bits would be:
>
> dbi_write_data(u16 data)
> {
> // or are the bytes the other way around...
> u8 buf[2] = { data & 0xff, (data >> 8) & 0xff };
> return dbi_write_data(buf, 2);
> }
>
> > Helper functions would be available to perform 8-bit, 16-bit or n*8 bits
> > transfers.
> >
> > Would that work for your use cases ?
> >
> > > Also note that some chips need to change the bus width on the fly. The
> > > chip used on N800 wants configuration to be done with 8-bits, and pixel
> > > transfers with 16-bits. Who knows why...
> >
> > On which data lines is configuration performed ? D7-D0 ?
>
> I guess so, but does it matter? All the bus driver needs to know is how
> to send 8/16/.. bit data. On OMAP we just write the data to a 32 bit
> register, and the HW takes the lowest n bits. Do the bits represent the
> data lines directly on Renesans?
Yes they do. For a SYS-80 panel configured in 18-bits mode, I'll have to write
((data & 0xff00) << 2) | ((data & 0x00ff) << 1)
(d15:8 -> D17:10, d7:0 -> D8:1 where d is the word to be written, and D the
physical lines)
to the hardware data register and trigger the write. In 8 bits mode, there
would be two write operations with
(data & 0xff00) << 2
(data & 0x00ff) << 10
(d15:8 -> D17:10, d7:0 -> D17:10)
However, when writing a 8-bit command to a DBI panel in either 16- or 8-bits
mode, there would be a single write with
d7:0 -> D7:0
How to shift the data thus depends both on the bus width and on which data
lines the panel expects data to be present.
I wrote drivers for two DBI panels based on existing board code, the R61505
and the R61517.
The R61505 datasheet is available online, the panel is a SYS-80 panel that
supports 8-, 9-, 16- and 18-bits bus widths. It aligns data towards the MSB
when using a bus width lower than 18.
The R61517 datasheet doesn't seem to be freely available. The panel seems to
be DBI-compliant as it uses a subset of the DCS commands and a wide range of
panel-specific commands. The panel is connected using a 16-bit bus, all
commands and parameters are 8-bits wide and aligned towards the LSB.
To properly transfer commands and parameters, the DBI host will need to know
on how many bits to perform transfers, and how to align data on the bus. For
the former, your mipi_dbi_set_bus_width() function could be used, although
probably not out of the box. The R61505 panel would call
mipi_dbi_set_bus_width() to set the bus width to 16 (as commands and
parameters are 16-bits wide), but if the panel is connected using only 8 or 9
data lines, the host would need to split the 16-bits writes into two 8-bits
writes. Should that be done transparently ? mipi_dbi_set_bus_width() could
possibly act as a mipi_dbi_set_max_bus_width(), but that might be a bit too
hackish.
I'd like to hide as much of the complexity as possible in mipi-dbi-bus.c but I
don't know whether that's possible.
> The omap driver actually only implements 8 and 16 bit modes, not the 9 and
> 12 bit modes. I'm not sure what kind of shifting is needed for those.
There's no 12-bits mode in DBI-2 as far as I can tell.
We will need to support 8-, 9- and 16-bits modes for DBI-2, and additionally
18-bits mode for SYS-80.
> > > > We might just need to provide fake timings. Video mode timings are at
> > > > the core of display support in all drivers so we can't just get rid of
> > > > them. The h/v front/back porch and sync won't be used by display
> > > > drivers for DBI/DSI panels anyway.
> > >
> > > Right. But we should probably think if we can, at the panel level,
> > > easily separate conventional panels and smart panels. Then this
> > > framework wouldn't need to fake the timings, and it'd be up to the
> > > higher level to decide if and how to fake them. Then again, this is no
> > > biggie. Just thought that at the lowest level it'd be nice to be
> > > "correct" and leave faking to upper layers =).
> >
> > But we would then have two different APIs at the lower level depending on
> > the panel type. I'm not sure that's a good thing.
>
> Different API for what? Why anyway need panel type specific functions.
> In the panel struct we could just have an union of the different types
> of parameters for different types of panels.
>
> But if this complicates things, it's not a biggie. Just something that
> has been in my mind when dealing with smart panels and assigning dummy
> video timings for them =).
Please feel free to make a proposal for this when I'll post v2. A patch would
be nice :-)
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH V4 3/6] OMAPDSS: DSS: Cleanup cpu_is_xxxx checks
From: Tomi Valkeinen @ 2012-08-17 13:54 UTC (permalink / raw)
To: Chandrabhanu Mahapatra; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1345115913-6773-1-git-send-email-cmahapatra@ti.com>
[-- Attachment #1: Type: text/plain, Size: 5554 bytes --]
On Thu, 2012-08-16 at 16:48 +0530, Chandrabhanu Mahapatra wrote:
> All the cpu_is checks have been moved to dss_init_features function providing a
> much more generic and cleaner interface. The OMAP version and revision specific
> initializations in various functions are cleaned and the necessary data are
> moved to dss_features structure which is local to dss.c.
>
> Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
> +static int __init dss_init_features(struct device *dev)
> +{
> + dss.feat = devm_kzalloc(dev, sizeof(*dss.feat), GFP_KERNEL);
> + if (!dss.feat) {
> + dev_err(dev, "Failed to allocate local DSS Features\n");
> + return -ENOMEM;
> + }
> +
> + if (cpu_is_omap24xx())
> + dss.feat = &omap24xx_dss_features;
> + else if (cpu_is_omap34xx())
> + dss.feat = &omap34xx_dss_features;
> + else if (cpu_is_omap3630())
> + dss.feat = &omap3630_dss_features;
> + else if (cpu_is_omap44xx())
> + dss.feat = &omap44xx_dss_features;
> + else
> + return -ENODEV;
> +
> + return 0;
> +}
This is not correct (and same problem in dispc). You allocate the feat
struct and assign the pointer to dss.feat, but then overwrite dss.feat
pointer with the pointer to omap24xx_dss_features (which is freed
later). You need to memcpy it.
I also get a crash on omap3 overo board when loading omapdss:
loading nfs/work/linux/drivers/video/omap2/dss/omapdss.ko debug=y def_disp=lcd43
[ 20.411224] Unable to handle kernel NULL pointer dereference at virtual address 00000008
[ 20.419921] pgd = ce8a8000
[ 20.422790] [00000008] *pgd=8e8c5831, *pte=00000000, *ppte=00000000
[ 20.429473] Internal error: Oops: 17 [#1] SMP ARM
[ 20.434448] Modules linked in: omapdss(+)
[ 20.438690] CPU: 0 Tainted: G W (3.5.0-rc2-00058-g1c1e55c #93)
[ 20.446350] PC is at omap_dsshw_probe+0xa4/0x290 [omapdss]
[ 20.452148] LR is at 0x2e39
[ 20.455108] pc : [<bf043288>] lr : [<00002e39>] psr: 80000013
[ 20.455108] sp : ce89ddd0 ip : c0b797e0 fp : 00006133
[ 20.467224] r10: 00000028 r9 : c0c5c07c r8 : bf02eadc
[ 20.472717] r7 : 00000000 r6 : c06e9644 r5 : cf0cf808 r4 : bf02f430
[ 20.479614] r3 : cf0cf808 r2 : 00000000 r1 : 00000000 r0 : 00000000
[ 20.486511] Flags: Nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
[ 20.494049] Control: 10c5387d Table: 8e8a8019 DAC: 00000015
[ 20.500091] Process insmod (pid: 664, stack limit = 0xce89c2f8)
[ 20.506347] Stack: (0xce89ddd0 to 0xce89e000)
[ 20.510955] ddc0: cf0cf808 c0c96ed8 c0c96ee8 c02c22e4
[ 20.519592] dde0: c02c22cc c02c0f28 22222222 cf0cf808 bf02eadc cf0cf83c 00000000 00000001
[ 20.528198] de00: 00000028 c02c113c bf02eadc c02c10a8 00000000 c02bf6c8 cf0192a8 cf0cec10
[ 20.536834] de20: bf02eadc c072fab8 cf3e7440 c02c05dc bf0249e0 00000000 cf04ce40 bf02eadc
[ 20.545471] de40: c0748880 ce89c000 00000000 00000001 c0c5c07c 00000028 00006133 c02c1670
[ 20.554107] de60: 00000000 bf02eac8 c0748880 ce89c000 00000000 00000001 00000028 c02c26e0
[ 20.562744] de80: 00000003 00000000 c0748880 bf043124 00000000 c0748880 ce89c000 00000000
[ 20.571380] dea0: 00000001 c0008730 bf02f29c 00000001 00000001 bf0430cc c071bcd0 00000000
[ 20.580017] dec0: bf02f29c c006823c 00000000 ce827ec0 cf0001c0 00000000 bf02f29c 00000001
[ 20.588623] dee0: ce851480 00000001 c0c5c07c 00000028 00006133 c0099d20 bf02f2a8 00007fff
[ 20.597259] df00: c0098aa4 c012480c 00000000 c0098890 bf02f3f0 ce89c000 c06d32d8 d08fe09c
[ 20.605895] df20: d0a19624 000a7008 d08ce000 001f2ab7 d0a18bd4 d0a18948 d0aba984 00030ed0
[ 20.614532] df40: 0003b0e0 00000000 00000000 00000042 00000043 00000026 0000002a 00000014
[ 20.623138] df60: 00000000 bf022024 00000043 00000000 00000000 00000000 00000000 c0623b14
[ 20.631774] df80: 001f2ab7 001f2ab7 00000004 beb48e7c 00000080 c0013f28 ce89c000 00000000
[ 20.640411] dfa0: 00000000 c0013d60 001f2ab7 00000004 b6c49008 001f2ab7 000a7008 beb48e7c
[ 20.649047] dfc0: 001f2ab7 00000004 beb48e7c 00000080 000a47f8 00000000 b6f80000 00000000
[ 20.657684] dfe0: beb48bb8 beb48ba8 00019dfc b6f10020 60000010 b6c49008 00000000 00000000
[ 20.666442] [<bf043288>] (omap_dsshw_probe+0xa4/0x290 [omapdss]) from [<c02c22e4>] (platform_drv_
probe+0x18/0x1c)
[ 20.677276] [<c02c22e4>] (platform_drv_probe+0x18/0x1c) from [<c02c0f28>] (driver_probe_device+0x
9c/0x21c)
[ 20.687499] [<c02c0f28>] (driver_probe_device+0x9c/0x21c) from [<c02c113c>] (__driver_attach+0x94
/0x98)
[ 20.697418] [<c02c113c>] (__driver_attach+0x94/0x98) from [<c02bf6c8>] (bus_for_each_dev+0x50/0x7
c)
[ 20.706970] [<c02bf6c8>] (bus_for_each_dev+0x50/0x7c) from [<c02c05dc>] (bus_add_driver+0xa0/0x2a
8)
[ 20.716522] [<c02c05dc>] (bus_add_driver+0xa0/0x2a8) from [<c02c1670>] (driver_register+0x78/0x17
4)
[ 20.726074] [<c02c1670>] (driver_register+0x78/0x174) from [<c02c26e0>] (platform_driver_probe+0x
18/0x9c)
[ 20.736267] [<c02c26e0>] (platform_driver_probe+0x18/0x9c) from [<bf043124>] (omap_dss_init+0x58/
0x118 [omapdss])
[ 20.747192] [<bf043124>] (omap_dss_init+0x58/0x118 [omapdss]) from [<c0008730>] (do_one_initcall+
0x34/0x194)
[ 20.757568] [<c0008730>] (do_one_initcall+0x34/0x194) from [<c0099d20>] (sys_init_module+0xdc/0x1
cc4)
[ 20.767333] [<c0099d20>] (sys_init_module+0xdc/0x1cc4) from [<c0013d60>] (ret_fast_syscall+0x0/0x
3c)
[ 20.776947] Code: ea00000d e5941248 e3a00000 e584600c (e5911008)
[ 20.783599] ---[ end trace bcb6e89e4ea810ae ]---
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFC 3/5] video: panel: Add MIPI DBI bus support
From: Tomi Valkeinen @ 2012-08-17 13:06 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-fbdev, dri-devel, linux-leds, linux-media, Bryan Wu,
Richard Purdie, Marcus Lorentzon, Sumit Semwal, Archit Taneja,
Sebastien Guiriec, Inki Dae, Kyungmin Park
In-Reply-To: <2019849.eCaIrHMssh@avalon>
[-- Attachment #1: Type: text/plain, Size: 6024 bytes --]
On Fri, 2012-08-17 at 14:33 +0200, Laurent Pinchart wrote:
> > But first, the data type should be byte, not unsigned long. How would
> > you write 8 bits or 16 bits with your API?
>
> u8 and u16 both fit in an unsigned long :-) Please see below.
Ah, I see, so the driver would just discard 24 bits or 16 bits from the
ulong. I somehow thought that if you have 8 bit bus, and you call the
write with ulong, 4 bytes will be written.
> > Then again, I'd hope to have DCS somehow as a separate library, which would
> > then use DBI/DSI/whatnot to actually send the data.
> >
> > I'm not quite sure how easy that is because of the differences between
> > the busses.
> >
> > > Is DBI limited to 8-bit data transfers for commands ? Pixels can be
> > > transferred 16-bit at a time, commands might as well. While DCS only
> > > specifies 8-bit command/data, DBI panels that are not DCS compliant can
> > > use 16-bit command/data (the R61505 panel, albeit a SYS-80 panel, does
> > > so).
> >
> > I have to say I don't remember much about DBI =). Looking at OMAP's
> > driver, which was made for omap2 and hasn't been much updated since, I
> > see that there are 4 modes, 8/9/12/16 bits. I think that defines how
> > many of the parallel data lines are used.
>
> SYS-80 also has an 18-bits mode, where bits 0 and 9 are always ignored when
> transferring instructions and data other than pixels (for pixels the 18-bits
> bus width can be used to transfer RGB666 in a single clock cycle).
>
> See page 87 of
> http://read.pudn.com/downloads91/sourcecode/others/348230/e61505_103a.pdf.
>
> > However, I don't think that matters for the panel driver when it wants
> > to send data. The panel driver should just call dbi_write(buf, buf_len),
> > and the dbi driver would send the data in the buffer according to the
> > bus width.
>
> According to the DCS specification, commands and parameters are transferred
> using 8-bit data. Non-DCS panels can however use wider commands and parameters
> (all commands and parameters are 16-bits wide for the R61505 for instance).
>
> We can add an API to switch the DBI bus width on the fly. For Renesas hardware
> this would "just" require shifting bits around to output the 8-bit or 16-bit
> commands on the right data lines (the R61505 uses D17-D9 in 8-bit mode, while
> the DCS specification mentions D7-D0) based on how the panel is connected and
> on which lines the panel expects data.
>
> As commands can be expressed on either 8 or 16 bits I would use a 16 type for
> them.
I won't put my head on the block, but I don't think DBI has any
restriction on the size of the command. A "command" just means a data
transfer while keeping the D/CX line low, and "data" when the line is
high. Similar transfers for n bytes can be done in both modes.
> For parameters, we can either express everything as u8 * in the DBI bus
> operations, or use a union similar to i2c_smbus_data
>
> union i2c_smbus_data {
> __u8 byte;
> __u16 word;
> __u8 block[I2C_SMBUS_BLOCK_MAX + 2]; /* block[0] is used for length */
> /* and one more for user-space compatibility */
> };
There's no DBI_BLOCK_MAX, so at least identical union won't work. I
think it's simplest to have u8 * function as a base, and then a few
helpers to write the most common datatypes.
So we could have on the lowest level something like:
dbi_write_command(u8 *buf, size_t size);
dbi_write_data(u8 *buf, size_t size);
And possible helpers:
dbi_write_data(u8 *cmd_buf, size_t cmd_size, u8 *data_buf, size_t
data_size);
dbi_write_dcs(u8 cmd, u8 *data, size_t size);
And variations:
dbi_write_dcs_0(u8 cmd);
dbi_write_dcs_1(u8 cmd, u8 data);
etc. So a simple helper to send 16 bits would be:
dbi_write_data(u16 data)
{
// or are the bytes the other way around...
u8 buf[2] = { data & 0xff, (data >> 8) & 0xff };
return dbi_write_data(buf, 2);
}
> Helper functions would be available to perform 8-bit, 16-bit or n*8 bits
> transfers.
>
> Would that work for your use cases ?
>
> > Also note that some chips need to change the bus width on the fly. The
> > chip used on N800 wants configuration to be done with 8-bits, and pixel
> > transfers with 16-bits. Who knows why...
>
> On which data lines is configuration performed ? D7-D0 ?
I guess so, but does it matter? All the bus driver needs to know is how
to send 8/16/.. bit data. On OMAP we just write the data to a 32 bit
register, and the HW takes the lowest n bits. Do the bits represent the
data lines directly on Renesans?
The omap driver actually only implements 8 and 16 bit modes, not the 9
and 12 bit modes. I'm not sure what kind of shifting is needed for
those.
> > > We might just need to provide fake timings. Video mode timings are at the
> > > core of display support in all drivers so we can't just get rid of them.
> > > The h/v front/back porch and sync won't be used by display drivers for
> > > DBI/DSI panels anyway.
> >
> > Right. But we should probably think if we can, at the panel level, easily
> > separate conventional panels and smart panels. Then this framework wouldn't
> > need to fake the timings, and it'd be up to the higher level to decide if
> > and how to fake them. Then again, this is no biggie. Just thought that at
> > the lowest level it'd be nice to be "correct" and leave faking to upper
> > layers =).
>
> But we would then have two different APIs at the lower level depending on the
> panel type. I'm not sure that's a good thing.
Different API for what? Why anyway need panel type specific functions.
In the panel struct we could just have an union of the different types
of parameters for different types of panels.
But if this complicates things, it's not a biggie. Just something that
has been in my mind when dealing with smart panels and assigning dummy
video timings for them =).
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 2/3] OMAPDSS: APPLY: Remove omap_dss_device references in wait_for_go functions
From: Tomi Valkeinen @ 2012-08-17 12:35 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1345200551-28712-3-git-send-email-archit@ti.com>
[-- Attachment #1: Type: text/plain, Size: 3338 bytes --]
On Fri, 2012-08-17 at 16:19 +0530, Archit Taneja wrote:
> The functions dss_mgr_wait_for_go() and dss_mgr_wait_for_go_ovl() check if there
> is an enabled display connected to the manager before trying to see the state of
> the GO bit.
>
> The checks related to the display can be replaced by checking the state of the
> manager, i.e, whether the manager is enabled or not. This makes more sense than
> checking with the connected display as the GO bit behaviour is more connected
> with the manager state rather than the display state. A GO bit can only be set
> if the manager is enabled. If a manager isn't enabled, we can safely assume that
> the GO bit is not set.
>
> Signed-off-by: Archit Taneja <archit@ti.com>
> ---
> drivers/video/omap2/dss/apply.c | 32 +++++++++++++++++++++-----------
> 1 file changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
> index 52a5940..74f1a58 100644
> --- a/drivers/video/omap2/dss/apply.c
> +++ b/drivers/video/omap2/dss/apply.c
> @@ -424,17 +424,23 @@ static void wait_pending_extra_info_updates(void)
> int dss_mgr_wait_for_go(struct omap_overlay_manager *mgr)
> {
> unsigned long timeout = msecs_to_jiffies(500);
> - struct mgr_priv_data *mp;
> + struct mgr_priv_data *mp = get_mgr_priv(mgr);
> u32 irq;
> + unsigned long flags;
> int r;
> int i;
> - struct omap_dss_device *dssdev = mgr->device;
>
> - if (!dssdev || dssdev->state != OMAP_DSS_DISPLAY_ACTIVE)
> + if (mgr_manual_update(mgr))
This needs to be inside the spinlock also.
> return 0;
>
> - if (mgr_manual_update(mgr))
> + spin_lock_irqsave(&data_lock, flags);
> +
> + if (!mp->enabled) {
> + spin_unlock_irqrestore(&data_lock, flags);
> return 0;
> + }
> +
> + spin_unlock_irqrestore(&data_lock, flags);
>
> r = dispc_runtime_get();
> if (r)
> @@ -442,10 +448,8 @@ int dss_mgr_wait_for_go(struct omap_overlay_manager *mgr)
>
> irq = dispc_mgr_get_vsync_irq(mgr->id);
>
> - mp = get_mgr_priv(mgr);
> i = 0;
> while (1) {
> - unsigned long flags;
> bool shadow_dirty, dirty;
>
> spin_lock_irqsave(&data_lock, flags);
> @@ -489,21 +493,28 @@ int dss_mgr_wait_for_go_ovl(struct omap_overlay *ovl)
> {
> unsigned long timeout = msecs_to_jiffies(500);
> struct ovl_priv_data *op;
> - struct omap_dss_device *dssdev;
> + struct mgr_priv_data *mp;
> u32 irq;
> + unsigned long flags;
> int r;
> int i;
>
> if (!ovl->manager)
> return 0;
And this should be inside spinlock (yes, you didn't change that, but now
that you're changing these... =)
> - dssdev = ovl->manager->device;
> + mp = get_mgr_priv(ovl->manager);
>
> - if (!dssdev || dssdev->state != OMAP_DSS_DISPLAY_ACTIVE)
> + if (ovl_manual_update(ovl))
Inside spinlock here too.
Actually, shouldn't the whole wait_for functions be locked with the
apply mutex? Otherwise the output can be disabled/changed while waiting.
On the other hand, that could be quite a long lock, and I don't see
anything in the code that could really break if the output is disabled
or similar. Perhaps it's fine to just hit the timeout in case something
has been changed. If we add a mutex, we risk breaking something that
currently works =).
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFC 3/5] video: panel: Add MIPI DBI bus support
From: Laurent Pinchart @ 2012-08-17 12:33 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: linux-fbdev, dri-devel, linux-leds, linux-media, Bryan Wu,
Richard Purdie, Marcus Lorentzon, Sumit Semwal, Archit Taneja,
Sebastien Guiriec, Inki Dae, Kyungmin Park
In-Reply-To: <1345200709.11073.27.camel@lappyti>
Hi Tomi,
On Friday 17 August 2012 13:51:49 Tomi Valkeinen wrote:
> On Fri, 2012-08-17 at 12:02 +0200, Laurent Pinchart wrote:
> > On Friday 17 August 2012 12:03:02 Tomi Valkeinen wrote:
> > > On Fri, 2012-08-17 at 02:49 +0200, Laurent Pinchart wrote:
> > > > +/*
> > > > ----------------------------------------------------------------------
> > > > ---
> > > > ---- + * Bus operations
> > > > + */
> > > > +
> > > > +void panel_dbi_write_command(struct panel_dbi_device *dev, unsigned
> > > > long
> > > > cmd) +{
> > > > + dev->bus->ops->write_command(dev->bus, cmd);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(panel_dbi_write_command);
> > > > +
> > > > +void panel_dbi_write_data(struct panel_dbi_device *dev, unsigned long
> > > > data) +{
> > > > + dev->bus->ops->write_data(dev->bus, data);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(panel_dbi_write_data);
> > > > +
> > > > +unsigned long panel_dbi_read_data(struct panel_dbi_device *dev)
> > > > +{
> > > > + return dev->bus->ops->read_data(dev->bus);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(panel_dbi_read_data);
> > >
> > > I'm not that familiar with how to implement bus drivers, can you
> > > describe in pseudo code how the SoC's DBI driver would register these?
> >
> > Sure.
> >
> > The DBI bus driver first needs to create a panel_dbi_bus_ops instance:
> >
> > static const struct panel_dbi_bus_ops sh_mobile_lcdc_dbi_bus_ops = {
> >
> > .write_command = lcdc_dbi_write_command,
> > .write_data = lcdc_dbi_write_data,
> > .read_data = lcdc_dbi_read_data,
> >
> > };
>
> Thanks for the example, I think it cleared up things a bit.
>
> As I mentioned earlier, I really think "panel" is not right here. While
> the whole framework may be called panel framework, the bus drivers are
> not panels, and we should support external chips also, which are not
> panels either.
I agree. I've renamed panel_dbi_* to mipi_dbi_*.
> > > I think write/read data functions are a bit limited. Shouldn't they be
> > > something like write_data(const u8 *buf, int size) and read_data(u8
> > > *buf, int len)?
> >
> > Good point. My hardware doesn't support multi-byte read/write operations
> > directly so I haven't thought about adding those.
>
> OMAP HW doesn't support it either. Well, not quite true, as OMAP's
> system DMA could be used to write a buffer to the DBI output. But that's
> really the same as doing the write with a a loop with CPU.
>
> But first, the data type should be byte, not unsigned long. How would
> you write 8 bits or 16 bits with your API?
u8 and u16 both fit in an unsigned long :-) Please see below.
> And second, if the function takes just u8, you'd need lots of calls to do
> simple writes.
I agree, an array write function is a good idea.
> > Can your hardware group command + data writes in a single operation ? If
> > so we should expose that at the API level as well.
>
> No it can't. But with DCS that is a common operation, so we could have
> some helpers to send command + data with one call.
Agreed.
> Then again, I'd hope to have DCS somehow as a separate library, which would
> then use DBI/DSI/whatnot to actually send the data.
>
> I'm not quite sure how easy that is because of the differences between
> the busses.
>
> > Is DBI limited to 8-bit data transfers for commands ? Pixels can be
> > transferred 16-bit at a time, commands might as well. While DCS only
> > specifies 8-bit command/data, DBI panels that are not DCS compliant can
> > use 16-bit command/data (the R61505 panel, albeit a SYS-80 panel, does
> > so).
>
> I have to say I don't remember much about DBI =). Looking at OMAP's
> driver, which was made for omap2 and hasn't been much updated since, I
> see that there are 4 modes, 8/9/12/16 bits. I think that defines how
> many of the parallel data lines are used.
SYS-80 also has an 18-bits mode, where bits 0 and 9 are always ignored when
transferring instructions and data other than pixels (for pixels the 18-bits
bus width can be used to transfer RGB666 in a single clock cycle).
See page 87 of
http://read.pudn.com/downloads91/sourcecode/others/348230/e61505_103a.pdf.
> However, I don't think that matters for the panel driver when it wants
> to send data. The panel driver should just call dbi_write(buf, buf_len),
> and the dbi driver would send the data in the buffer according to the
> bus width.
According to the DCS specification, commands and parameters are transferred
using 8-bit data. Non-DCS panels can however use wider commands and parameters
(all commands and parameters are 16-bits wide for the R61505 for instance).
We can add an API to switch the DBI bus width on the fly. For Renesas hardware
this would "just" require shifting bits around to output the 8-bit or 16-bit
commands on the right data lines (the R61505 uses D17-D9 in 8-bit mode, while
the DCS specification mentions D7-D0) based on how the panel is connected and
on which lines the panel expects data.
As commands can be expressed on either 8 or 16 bits I would use a 16 type for
them.
For parameters, we can either express everything as u8 * in the DBI bus
operations, or use a union similar to i2c_smbus_data
union i2c_smbus_data {
__u8 byte;
__u16 word;
__u8 block[I2C_SMBUS_BLOCK_MAX + 2]; /* block[0] is used for length */
/* and one more for user-space compatibility */
};
Helper functions would be available to perform 8-bit, 16-bit or n*8 bits
transfers.
Would that work for your use cases ?
> Also note that some chips need to change the bus width on the fly. The
> chip used on N800 wants configuration to be done with 8-bits, and pixel
> transfers with 16-bits. Who knows why...
On which data lines is configuration performed ? D7-D0 ?
> So I think this, and generally most of the configuration, should be
> somewhat dynamic, so that the panel driver can change them when it
> needs.
>
> > > Something that's totally missing is configuring the DBI bus. There are a
> > > bunch of timing related values that need to be configured. See
> > > include/video/omapdss.h struct rfbi_timings. While the struct is OMAP
> > > specific, if I recall right most of the values match to the MIPI DBI
> > > spec.
> >
> > I've left that out currently, and thought about passing that information
> > as platform data to the DBI bus driver. That's the easiest solution, but I
> > agree that it's a hack. Panel should expose their timing requirements to
> > the DBI host. API wise that wouldn't be difficult (we only need to add
> > timing information to the panel platform data and add a function to the
> > DBI API to retrieve it), one of challenges might be to express it in a
> > way that's both universal enough and easy to use for DBI bus drivers.
>
> As I pointed above, I think the panel driver shouldn't expose it, but
> the panel driver should somehow set it. Or at least allowed to change it
> in some manner. This is actually again, the same problem as with enable
> and transfer: who controls what's going on.
>
> How I think it should work is something like:
>
> mipi_dbi_set_timings(dbi_dev, mytimings);
> mipi_dbi_set_bus_width(dbi_dev, 8);
> mipi_dbi_write(dbi_dev, ...);
> mipi_dbi_set_bus_width(dbi_dev, 16);
> start_frame_transfer(dbi_dev, ...);
I'll first implement bus width setting.
> > > And this makes me wonder, you use DBI bus for SYS-80 panel. The busses
> > > may look similar in operation, but are they really so similar when you
> > > take into account the timings (and perhaps something else, it's been
> > > years since I read the MIPI DBI spec)?
> >
> > I'll have to check all the details. SYS-80 is similar to DBI-B, but
> > supports a wider bus width of 18 bits. I think the interfaces are similar
> > enough to use a single bus implementation, possibly with quirks and/or
> > options (see SCCB support in I2C for instance, with flags to ignore acks,
> > force a stop bit generation, ...). We would duplicate lots of code if we
> > had two different implementations, and would prevent a DBI panel to be
> > attached to a SYS-80 host and vice-versa (the format is known to work).
>
> Ah ok, if a DBI panel can be connected to SYS-80 output and vice versa,
> then I agree they are similar enough.
Not all combination will work (a SYS panel that requires 16-bit transfers
won't work with a DBI host that only supports 8-bit), but some do.
> > We might just need to provide fake timings. Video mode timings are at the
> > core of display support in all drivers so we can't just get rid of them.
> > The h/v front/back porch and sync won't be used by display drivers for
> > DBI/DSI panels anyway.
>
> Right. But we should probably think if we can, at the panel level, easily
> separate conventional panels and smart panels. Then this framework wouldn't
> need to fake the timings, and it'd be up to the higher level to decide if
> and how to fake them. Then again, this is no biggie. Just thought that at
> the lowest level it'd be nice to be "correct" and leave faking to upper
> layers =).
But we would then have two different APIs at the lower level depending on the
panel type. I'm not sure that's a good thing.
--
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