* Re: [PATCH v2 07/13] OMAPDSS: HDMI: Add a get_timing function for HDMI interface
From: Archit Taneja @ 2012-08-14 13:27 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap
In-Reply-To: <1344949348.4845.46.camel@lappyti>
On Tuesday 14 August 2012 06:32 PM, Tomi Valkeinen wrote:
> Hi,
>
> On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote:
>> Add function omapdss_hdmi_display_get_timing() which returns the timings
>> maintained by the HDMI interface driver in it's hdmi_config field. This
>> prevents the need for the panel driver to configure default timings in it's
>> probe.
>>
>> This function is just intended to be used once during the panel driver's probe.
>> It makes sense for those interfaces which can be configured to a default timing.
>
> I'm not sure about this patch. So I think the basic idea here is that
> HDMI will use VGA video mode if, for whatever reason, no better mode is
> found out.
>
> After this change, the panel driver doesn't seem to do that. Instead it
> uses whatever mode was used previously by the hdmi output driver.
>
> Is the only reason for this patch to clean up the hdmi_panel driver? We
> could have a dss helper function which returns the timings for VGA
> (well, just a public const variable would be enough), and the panel
> driver could use that to initialize the timings struct.
Yes, that's the only reason, basically, to sync the panel with the
timings to which the hdmi output driver configured itself during it's
probe. We don't use hdmi_get_timing anywhere apart from here.
I thought it would be clean to retrieve the timings by taking whatever
is stored in the hdmi output driver, but we could leave it to a
hardcoded VGA as before.
I have done the same for venc, let me know if you think these should be
removed.
Archit
^ permalink raw reply
* [PATCH v5] da8xx-fb: add 24bpp LCD configuration support
From: Manjunathappa, Prakash @ 2012-08-14 13:33 UTC (permalink / raw)
To: linux-fbdev
LCD controller on am335x supports 24bpp raster configuration in addition
to ones on da850. LCDC also supports 24bpp in unpacked format having
ARGB:8888 32bpp format data in DDR, but it doesn't interpret alpha
component of the data.
Signed-off-by: Manjunathappa, Prakash <prakash.pm@ti.com>
Cc: Anatolij Gustschin <agust@denx.de>
---
Applies on top of fbdev-next of Florian Tobias Schandinat's tree.
Since v4:
Re-define CNVT_TOHW macro.
Since v3:
Minor nit, declare pseudo_palette as u32 type.
Since v2:
Fixed additional configurations for 24bpp support.
Since v1:
Simplified calculation of pseudopalette for FB_VISUAL_TRUECOLOR type.
drivers/video/da8xx-fb.c | 132 +++++++++++++++++++++++++++++++++------------
1 files changed, 97 insertions(+), 35 deletions(-)
diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index cb696ff..5c6df7b 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -87,6 +87,8 @@
#define LCD_V2_LIDD_CLK_EN BIT(1)
#define LCD_V2_CORE_CLK_EN BIT(0)
#define LCD_V2_LPP_B10 26
+#define LCD_V2_TFT_24BPP_MODE BIT(25)
+#define LCD_V2_TFT_24BPP_UNPACK BIT(26)
/* LCD Raster Timing 2 Register */
#define LCD_AC_BIAS_TRANSITIONS_PER_INT(x) ((x) << 16)
@@ -157,7 +159,6 @@ struct da8xx_fb_par {
unsigned int dma_end;
struct clk *lcdc_clk;
int irq;
- unsigned short pseudo_palette[16];
unsigned int palette_sz;
unsigned int pxl_clk;
int blank;
@@ -176,6 +177,7 @@ struct da8xx_fb_par {
unsigned int lcd_fck_rate;
#endif
void (*panel_power_ctrl)(int);
+ u32 pseudo_palette[16];
};
/* Variable Screen Information */
@@ -528,6 +530,9 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
{
u32 reg;
+ if (bpp > 16 && lcd_revision = LCD_VERSION_1)
+ return -EINVAL;
+
/* Set the Panel Width */
/* Pixels per line = (PPL + 1)*16 */
if (lcd_revision = LCD_VERSION_1) {
@@ -571,14 +576,19 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
reg = lcdc_read(LCD_RASTER_CTRL_REG) & ~(1 << 8);
if (raster_order)
reg |= LCD_RASTER_ORDER;
- lcdc_write(reg, LCD_RASTER_CTRL_REG);
+
+ par->palette_sz = 16 * 2;
switch (bpp) {
case 1:
case 2:
case 4:
case 16:
- par->palette_sz = 16 * 2;
+ break;
+ case 24:
+ reg |= LCD_V2_TFT_24BPP_MODE;
+ case 32:
+ reg |= LCD_V2_TFT_24BPP_UNPACK;
break;
case 8:
@@ -589,9 +599,12 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
return -EINVAL;
}
+ lcdc_write(reg, LCD_RASTER_CTRL_REG);
+
return 0;
}
+#define CNVT_TOHW(val, width) ((((val) << (width)) + 0x7FFF - (val)) >> 16)
static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
unsigned blue, unsigned transp,
struct fb_info *info)
@@ -607,13 +620,38 @@ static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
if (info->fix.visual = FB_VISUAL_DIRECTCOLOR)
return 1;
- if (info->var.bits_per_pixel = 4) {
- if (regno > 15)
- return 1;
+ if (info->var.bits_per_pixel > 16 && lcd_revision = LCD_VERSION_1)
+ return -EINVAL;
- if (info->var.grayscale) {
- pal = regno;
- } else {
+ switch (info->fix.visual) {
+ case FB_VISUAL_TRUECOLOR:
+ red = CNVT_TOHW(red, info->var.red.length);
+ green = CNVT_TOHW(green, info->var.green.length);
+ blue = CNVT_TOHW(blue, info->var.blue.length);
+ break;
+ case FB_VISUAL_PSEUDOCOLOR:
+ switch (info->var.bits_per_pixel) {
+ case 4:
+ if (regno > 15)
+ return -EINVAL;
+
+ if (info->var.grayscale) {
+ pal = regno;
+ } else {
+ red >>= 4;
+ green >>= 8;
+ blue >>= 12;
+
+ pal = red & 0x0f00;
+ pal |= green & 0x00f0;
+ pal |= blue & 0x000f;
+ }
+ if (regno = 0)
+ pal |= 0x2000;
+ palette[regno] = pal;
+ break;
+
+ case 8:
red >>= 4;
green >>= 8;
blue >>= 12;
@@ -621,36 +659,36 @@ static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
pal = (red & 0x0f00);
pal |= (green & 0x00f0);
pal |= (blue & 0x000f);
- }
- if (regno = 0)
- pal |= 0x2000;
- palette[regno] = pal;
- } else if (info->var.bits_per_pixel = 8) {
- red >>= 4;
- green >>= 8;
- blue >>= 12;
-
- pal = (red & 0x0f00);
- pal |= (green & 0x00f0);
- pal |= (blue & 0x000f);
-
- if (palette[regno] != pal) {
- update_hw = 1;
- palette[regno] = pal;
+ if (palette[regno] != pal) {
+ update_hw = 1;
+ palette[regno] = pal;
+ }
+ break;
}
- } else if ((info->var.bits_per_pixel = 16) && regno < 16) {
- red >>= (16 - info->var.red.length);
- red <<= info->var.red.offset;
+ break;
+ }
- green >>= (16 - info->var.green.length);
- green <<= info->var.green.offset;
+ /* Truecolor has hardware independent palette */
+ if (info->fix.visual = FB_VISUAL_TRUECOLOR) {
+ u32 v;
- blue >>= (16 - info->var.blue.length);
- blue <<= info->var.blue.offset;
+ if (regno > 15)
+ return -EINVAL;
- par->pseudo_palette[regno] = red | green | blue;
+ v = (red << info->var.red.offset) |
+ (green << info->var.green.offset) |
+ (blue << info->var.blue.offset);
+ switch (info->var.bits_per_pixel) {
+ case 16:
+ ((u16 *) (info->pseudo_palette))[regno] = v;
+ break;
+ case 24:
+ case 32:
+ ((u32 *) (info->pseudo_palette))[regno] = v;
+ break;
+ }
if (palette[0] != 0x4000) {
update_hw = 1;
palette[0] = 0x4000;
@@ -663,6 +701,7 @@ static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
return 0;
}
+#undef CNVT_TOHW
static void lcd_reset(struct da8xx_fb_par *par)
{
@@ -871,6 +910,9 @@ static int fb_check_var(struct fb_var_screeninfo *var,
{
int err = 0;
+ if (var->bits_per_pixel > 16 && lcd_revision = LCD_VERSION_1)
+ return -EINVAL;
+
switch (var->bits_per_pixel) {
case 1:
case 8:
@@ -906,6 +948,26 @@ static int fb_check_var(struct fb_var_screeninfo *var,
var->transp.length = 0;
var->nonstd = 0;
break;
+ case 24:
+ var->red.offset = 16;
+ var->red.length = 8;
+ var->green.offset = 8;
+ var->green.length = 8;
+ var->blue.offset = 0;
+ var->blue.length = 8;
+ var->nonstd = 0;
+ break;
+ case 32:
+ var->transp.offset = 24;
+ var->transp.length = 8;
+ var->red.offset = 16;
+ var->red.length = 8;
+ var->green.offset = 8;
+ var->green.length = 8;
+ var->blue.offset = 0;
+ var->blue.length = 8;
+ var->nonstd = 0;
+ break;
default:
err = -EINVAL;
}
--
1.7.1
^ permalink raw reply related
* [PATCH v3] da8xx-fb: allow frame to complete after disabling LCDC
From: Manjunathappa, Prakash @ 2012-08-14 13:35 UTC (permalink / raw)
To: linux-fbdev
Wait for active frame transfer to complete after disabling LCDC.
At the same this wait is not be required when there are sync and
underflow errors.
More information on disable and reset sequence can be found in
section 13.4.6 of AM335x TRM @www.ti.com/am335x.
Signed-off-by: Manjunathappa, Prakash <prakash.pm@ti.com>
---
Applies on top of fbdev-next of Florian Tobias Schandinat's tree.
Since v2:
Optimized the lcd_disable_raster function.
Since v1:
Changed the commit message, also added link to hardware specification.
drivers/video/da8xx-fb.c | 49 ++++++++++++++++++++++++++++++++++++---------
1 files changed, 39 insertions(+), 10 deletions(-)
diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 7ae9d53..cb696ff 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -48,6 +48,7 @@
#define LCD_PL_LOAD_DONE BIT(6)
#define LCD_FIFO_UNDERFLOW BIT(5)
#define LCD_SYNC_LOST BIT(2)
+#define LCD_FRAME_DONE BIT(0)
/* LCD DMA Control Register */
#define LCD_DMA_BURST_SIZE(x) ((x) << 4)
@@ -288,13 +289,41 @@ static inline void lcd_enable_raster(void)
}
/* Disable the Raster Engine of the LCD Controller */
-static inline void lcd_disable_raster(void)
+static inline void lcd_disable_raster(bool wait_for_frame_done)
{
u32 reg;
+ u32 stat_reg = LCD_STAT_REG;
+ u32 loop_cnt = 0;
reg = lcdc_read(LCD_RASTER_CTRL_REG);
if (reg & LCD_RASTER_ENABLE)
lcdc_write(reg & ~LCD_RASTER_ENABLE, LCD_RASTER_CTRL_REG);
+
+ if (lcd_revision = LCD_VERSION_2)
+ stat_reg = LCD_RAW_STAT_REG;
+
+ if (wait_for_frame_done) {
+ /*
+ * 50 milli seconds should be sufficient for a frame to
+ * complete
+ */
+ loop_cnt = 50;
+ while (!(lcdc_read(stat_reg) & LCD_FRAME_DONE)) {
+ /* Handle timeout */
+ if (unlikely(0 = --loop_cnt)) {
+ pr_err("LCD Controller timed out\n");
+ break;
+ }
+ mdelay(1);
+ }
+ }
+
+ /* clear asserted interrupts */
+ reg = lcdc_read(stat_reg);
+ if (lcd_revision = LCD_VERSION_1)
+ lcdc_write(reg, LCD_STAT_REG);
+ else
+ lcdc_write(reg, LCD_MASKED_STAT_REG);
}
static void lcd_blit(int load_mode, struct da8xx_fb_par *par)
@@ -638,7 +667,7 @@ static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
static void lcd_reset(struct da8xx_fb_par *par)
{
/* Disable the Raster if previously Enabled */
- lcd_disable_raster();
+ lcd_disable_raster(false);
/* DMA has to be disabled */
lcdc_write(0, LCD_DMA_CTRL_REG);
@@ -734,7 +763,7 @@ static irqreturn_t lcdc_irq_handler_rev02(int irq, void *arg)
u32 stat = lcdc_read(LCD_MASKED_STAT_REG);
if ((stat & LCD_SYNC_LOST) && (stat & LCD_FIFO_UNDERFLOW)) {
- lcd_disable_raster();
+ lcd_disable_raster(false);
lcdc_write(stat, LCD_MASKED_STAT_REG);
lcd_enable_raster();
} else if (stat & LCD_PL_LOAD_DONE) {
@@ -744,7 +773,7 @@ static irqreturn_t lcdc_irq_handler_rev02(int irq, void *arg)
* interrupt via the following write to the status register. If
* this is done after then one gets multiple PL done interrupts.
*/
- lcd_disable_raster();
+ lcd_disable_raster(false);
lcdc_write(stat, LCD_MASKED_STAT_REG);
@@ -789,7 +818,7 @@ static irqreturn_t lcdc_irq_handler_rev01(int irq, void *arg)
u32 reg_ras;
if ((stat & LCD_SYNC_LOST) && (stat & LCD_FIFO_UNDERFLOW)) {
- lcd_disable_raster();
+ lcd_disable_raster(false);
lcdc_write(stat, LCD_STAT_REG);
lcd_enable_raster();
} else if (stat & LCD_PL_LOAD_DONE) {
@@ -799,7 +828,7 @@ static irqreturn_t lcdc_irq_handler_rev01(int irq, void *arg)
* interrupt via the following write to the status register. If
* this is done after then one gets multiple PL done interrupts.
*/
- lcd_disable_raster();
+ lcd_disable_raster(false);
lcdc_write(stat, LCD_STAT_REG);
@@ -898,7 +927,7 @@ static int lcd_da8xx_cpufreq_transition(struct notifier_block *nb,
if (val = CPUFREQ_POSTCHANGE) {
if (par->lcd_fck_rate != clk_get_rate(par->lcdc_clk)) {
par->lcd_fck_rate = clk_get_rate(par->lcdc_clk);
- lcd_disable_raster();
+ lcd_disable_raster(true);
lcd_calc_clk_divider(par);
lcd_enable_raster();
}
@@ -935,7 +964,7 @@ static int __devexit fb_remove(struct platform_device *dev)
if (par->panel_power_ctrl)
par->panel_power_ctrl(0);
- lcd_disable_raster();
+ lcd_disable_raster(true);
lcdc_write(0, LCD_RASTER_CTRL_REG);
/* disable DMA */
@@ -1051,7 +1080,7 @@ static int cfb_blank(int blank, struct fb_info *info)
if (par->panel_power_ctrl)
par->panel_power_ctrl(0);
- lcd_disable_raster();
+ lcd_disable_raster(true);
break;
default:
ret = -EINVAL;
@@ -1411,7 +1440,7 @@ static int fb_suspend(struct platform_device *dev, pm_message_t state)
par->panel_power_ctrl(0);
fb_set_suspend(info, 1);
- lcd_disable_raster();
+ lcd_disable_raster(true);
clk_disable(par->lcdc_clk);
console_unlock();
--
1.7.1
^ permalink raw reply related
* Re: [PATCH v2 09/13] OMAPDSS: SDI: Create a function to set timings
From: Tomi Valkeinen @ 2012-08-14 13:44 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-fbdev, linux-omap
In-Reply-To: <1344512989-4071-10-git-send-email-archit@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1614 bytes --]
On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote:
> Create function omapdss_sdi_set_timings(). Configuring new timings is done the
> same way as before, SDI is disabled, and re-enabled with the new timings in
> dssdev. This just moves the code from the panel drivers to the SDI driver.
>
> The panel drivers shouldn't be aware of how SDI manages to configure a new set
> of timings. This should be taken care of by the SDI driver itself.
I'm not sure about this one. Although I see that dpi.c does currently
the same thing as you're doing in your patch.
One thing is that we should try to remove dssdev uses from the output
drivers, including use of dssdev->state.
The other thing is that I don't think the output driver should disable &
enable the output during set timings. I think sdi's set_timings should
return EBUSY if the output is enabled. The same way as other
configuration functions should (like dpi_set_data_lines or such).
I'm actually not sure if even the panel driver should disable & enable
the output during set_timings. Perhaps it should be the caller's
(omapdrm or such) responsibility....
My reasoning here is that disabling & enabling the video output is not
invisible to the upper layers, so doing it "in secret" may be bad.
Then again, perhaps timings can be changed freely on some other
platforms, and then it'd be nice if the panel driver wouldn't disable &
enable the output.
So I'm again not quite sure what's the best way to handle this... (of
the dssdev->state I'm sure, its use should be removed from omapdss). Any
thoughts?
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v2 07/13] OMAPDSS: HDMI: Add a get_timing function for HDMI interface
From: Tomi Valkeinen @ 2012-08-14 14:10 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-fbdev, linux-omap
In-Reply-To: <502A4F62.5050400@ti.com>
[-- Attachment #1: Type: text/plain, Size: 3399 bytes --]
On Tue, 2012-08-14 at 18:45 +0530, Archit Taneja wrote:
> On Tuesday 14 August 2012 06:32 PM, Tomi Valkeinen wrote:
> > Hi,
> >
> > On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote:
> >> Add function omapdss_hdmi_display_get_timing() which returns the timings
> >> maintained by the HDMI interface driver in it's hdmi_config field. This
> >> prevents the need for the panel driver to configure default timings in it's
> >> probe.
> >>
> >> This function is just intended to be used once during the panel driver's probe.
> >> It makes sense for those interfaces which can be configured to a default timing.
> >
> > I'm not sure about this patch. So I think the basic idea here is that
> > HDMI will use VGA video mode if, for whatever reason, no better mode is
> > found out.
> >
> > After this change, the panel driver doesn't seem to do that. Instead it
> > uses whatever mode was used previously by the hdmi output driver.
> >
> > Is the only reason for this patch to clean up the hdmi_panel driver? We
> > could have a dss helper function which returns the timings for VGA
> > (well, just a public const variable would be enough), and the panel
> > driver could use that to initialize the timings struct.
>
> Yes, that's the only reason, basically, to sync the panel with the
> timings to which the hdmi output driver configured itself during it's
> probe. We don't use hdmi_get_timing anywhere apart from here.
Does the hdmi output driver even need to configure any defaults?
Wouldn't it be ok to presume that the panel driver configures the
timings?
I don't think it's sensible to think about default values in the output
driver generally (well, at least for things like timings). Although in
HDMI's case VGA is quite sensible default, but that's not the case for
any other output.
So I think generally we should just trust the panel driver to tell the
output driver what configuration should be used before the panel driver
enables the output.
That said, it doesn't hurt that the output drivers initialize their own
datastructures to something relatively sane to avoid any BUG() or WARN()
calls in the omapdss. Although we could also somehow track if the
timings has been set, and return an error when enabling the display if
timings hasn't been set. But that requires an extra flag, so perhaps
it's simpler to have some initial values in the output driver also.
> I thought it would be clean to retrieve the timings by taking whatever
> is stored in the hdmi output driver, but we could leave it to a
> hardcoded VGA as before.
My main worry is that it's not easily clear what is going on if you look
at the panel code. It looks that the panel just uses whatever is in the
output driver. It's not clear that it is always VGA. What if the
previous mode was something else?
I think it's much clearer if the panel driver sets the timings
explicitly before enabling the output. It doesn't have to be in panel's
probe, although that's perhaps the easiest place for it.
> I have done the same for venc, let me know if you think these should be
> removed.
Well, I agree that the initial timings code in hdmi and venc is a bit
ugly. I don't think it's bad as such, just that the timings are standard
ones, and instead of having all the timings numbers there, we should
have a common place for them.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 3/6] OMAPDSS: DSS: Cleanup cpu_is_xxxx checks
From: Tomi Valkeinen @ 2012-08-14 14:34 UTC (permalink / raw)
To: Mahapatra, Chandrabhanu; +Cc: linux-omap, linux-fbdev
In-Reply-To: <CAF0AtAsgTHwnUKaU8AzO5b7sThQTaocDGdb1Ebknuev_DgC0hg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4096 bytes --]
On Tue, 2012-08-14 at 18:00 +0530, Mahapatra, Chandrabhanu wrote:
> On Tue, Aug 14, 2012 at 3:18 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Mon, 2012-08-13 at 17:29 +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.
> >
> > It'd be good to have some version information here. Even just [PATCH v3
> > 3/6] in the subject is good, but of course the best is if there's a
> > short change log
> >
>
> Is it ok to have short change log in the individual patch description
> itself. I normally prefer the cover letter for this.
No, the patch description is added to git repository, and shouldn't
contain that kind of "extra" information. But you can add the version
information to the posted patch.
The diff stats below, after the "---" line, are discarded by git when
applying the patch. So I think you can just insert any text below ---
(but before the actual diffs) and they do not appear in the final git
commit.
I've not actually used that myself, and I can't find notes about it in
the documentation, so I'm not sure if there are any restrictions about
it. I've seen it used, though.
> >> Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
> >> ---
> >> drivers/video/omap2/dss/dss.c | 115 ++++++++++++++++++++++++++---------------
> >> 1 file changed, 74 insertions(+), 41 deletions(-)
> >>
> >> diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
> >> index 7b1c6ac..6ab236e 100644
> >> --- a/drivers/video/omap2/dss/dss.c
> >> +++ b/drivers/video/omap2/dss/dss.c
> >> @@ -31,6 +31,7 @@
> >> #include <linux/clk.h>
> >> #include <linux/platform_device.h>
> >> #include <linux/pm_runtime.h>
> >> +#include <linux/dma-mapping.h>
> >
> > Hmm, what is this for?
> >
>
> I should have included "include/linux/gfp.h" instead. It defines GFP_KERNEL.
>
> >> #include <video/omapdss.h>
> >>
> >> @@ -65,6 +66,13 @@ struct dss_reg {
> >> static int dss_runtime_get(void);
> >> static void dss_runtime_put(void);
> >>
> >> +struct dss_features {
> >> + u16 fck_div_max;
> >> + int factor;
> >> + char *clk_name;
> >> + bool (*check_cinfo_fck) (void);
> >> +};
> >
> > Is the check_cinfo_fck a leftover from previous versions?
> >
>
> Sorry, to have skipped that.
>
> > I think "factor" name is too vague. If I remember right, it's some kind
> > of implicit multiplier for the dss fck. So "dss_fck_multiplier"? You
> > could check the clock trees to verify what the multiplier exactly was,
> > and see if you can come up with a better name =).
> >
>
> dss_fck_multiplier sounds good to me. DSS clocks trees do not seem to
Yes, it's not really DSS thing, but PRCM. If things were perfect, DSS
wouldn't even need to know about the clock.
> mention anything about this multiplier. I found clock "dpll4_mx4_clk"
> in omap3 trm but nothing called "dpll_per_m5x2_clk" in omap4 trm. Any
> references please you know about?
Hmm, I think that's the linux's name for it. TRM seems to call it
CLKOUTX2_M5 (in the power, reset and clock management section).
> >> + if (cpu_is_omap24xx())
> >> + dss.feat = &omap2_dss_features;
> >> + else if (cpu_is_omap34xx())
> >> + dss.feat = &omap34_dss_features;
> >> + else if (cpu_is_omap3630())
> >> + dss.feat = &omap36_dss_features;
> >> + else
> >> + dss.feat = &omap4_dss_features;
> >
> > Check for omap4 also, and if the cpu is none of the above, return error.
> >
>
> I was wondering what error value to return. I was considering EINVAL
> (error in value) and ENODEV (no such device). But nothing seems to fit
> this case.
ENODEV sounds fine to me. I think EINVAL should be used when some given
parameter was wrong.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v2 09/13] OMAPDSS: SDI: Create a function to set timings
From: Archit Taneja @ 2012-08-14 17:08 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap
In-Reply-To: <1344951845.4845.58.camel@lappyti>
On Tuesday 14 August 2012 07:14 PM, Tomi Valkeinen wrote:
> On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote:
>> Create function omapdss_sdi_set_timings(). Configuring new timings is done the
>> same way as before, SDI is disabled, and re-enabled with the new timings in
>> dssdev. This just moves the code from the panel drivers to the SDI driver.
>>
>> The panel drivers shouldn't be aware of how SDI manages to configure a new set
>> of timings. This should be taken care of by the SDI driver itself.
>
> I'm not sure about this one. Although I see that dpi.c does currently
> the same thing as you're doing in your patch.
Even HDMI does the same thing.
>
> One thing is that we should try to remove dssdev uses from the output
> drivers, including use of dssdev->state.
Yes, we could do that by keeping a state of the output(and also checking
state of the manager)
>
> The other thing is that I don't think the output driver should disable &
> enable the output during set timings. I think sdi's set_timings should
> return EBUSY if the output is enabled. The same way as other
> configuration functions should (like dpi_set_data_lines or such).
>
> I'm actually not sure if even the panel driver should disable & enable
> the output during set_timings. Perhaps it should be the caller's
> (omapdrm or such) responsibility....
>
> My reasoning here is that disabling & enabling the video output is not
> invisible to the upper layers, so doing it "in secret" may be bad.
>
> Then again, perhaps timings can be changed freely on some other
> platforms, and then it'd be nice if the panel driver wouldn't disable &
> enable the output.
>
> So I'm again not quite sure what's the best way to handle this... (of
> the dssdev->state I'm sure, its use should be removed from omapdss). Any
> thoughts?
I guess it depends on how drm/fb want to use it. I guess an output
should have a set_timings() kind of op if it can do it seamlessly. I
guess we can do that easily in DPI, for example, we could reduce the fps
from 60 to 30 without causing an artefacts(I think). For outputs which
can't do it, we could remove the set_timings totally.
However, it'll be kind of inconsistent for some outputs to set timings,
and for others to not, and if in the future drm/fb gets exposed to ops
too, we may have dirty checks to see if set_timings is populated or not.
The easiest way would be to make all set_timings just update the copy of
the timings output has, and expect drm/fb to disable and re enable the
panel. We may end up doing unnecessary gpio resets and configuration of
the panels though.
Archit
^ permalink raw reply
* Re: [PATCH v2 07/13] OMAPDSS: HDMI: Add a get_timing function for HDMI interface
From: Archit Taneja @ 2012-08-14 17:28 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap
In-Reply-To: <1344953420.4845.72.camel@lappyti>
On Tuesday 14 August 2012 07:40 PM, Tomi Valkeinen wrote:
> On Tue, 2012-08-14 at 18:45 +0530, Archit Taneja wrote:
>> On Tuesday 14 August 2012 06:32 PM, Tomi Valkeinen wrote:
>>> Hi,
>>>
>>> On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote:
>>>> Add function omapdss_hdmi_display_get_timing() which returns the timings
>>>> maintained by the HDMI interface driver in it's hdmi_config field. This
>>>> prevents the need for the panel driver to configure default timings in it's
>>>> probe.
>>>>
>>>> This function is just intended to be used once during the panel driver's probe.
>>>> It makes sense for those interfaces which can be configured to a default timing.
>>>
>>> I'm not sure about this patch. So I think the basic idea here is that
>>> HDMI will use VGA video mode if, for whatever reason, no better mode is
>>> found out.
>>>
>>> After this change, the panel driver doesn't seem to do that. Instead it
>>> uses whatever mode was used previously by the hdmi output driver.
>>>
>>> Is the only reason for this patch to clean up the hdmi_panel driver? We
>>> could have a dss helper function which returns the timings for VGA
>>> (well, just a public const variable would be enough), and the panel
>>> driver could use that to initialize the timings struct.
>>
>> Yes, that's the only reason, basically, to sync the panel with the
>> timings to which the hdmi output driver configured itself during it's
>> probe. We don't use hdmi_get_timing anywhere apart from here.
>
> Does the hdmi output driver even need to configure any defaults?
> Wouldn't it be ok to presume that the panel driver configures the
> timings?
>
> I don't think it's sensible to think about default values in the output
> driver generally (well, at least for things like timings). Although in
> HDMI's case VGA is quite sensible default, but that's not the case for
> any other output.
>
> So I think generally we should just trust the panel driver to tell the
> output driver what configuration should be used before the panel driver
> enables the output.
>
> That said, it doesn't hurt that the output drivers initialize their own
> datastructures to something relatively sane to avoid any BUG() or WARN()
> calls in the omapdss. Although we could also somehow track if the
> timings has been set, and return an error when enabling the display if
> timings hasn't been set. But that requires an extra flag, so perhaps
> it's simpler to have some initial values in the output driver also.
>
>> I thought it would be clean to retrieve the timings by taking whatever
>> is stored in the hdmi output driver, but we could leave it to a
>> hardcoded VGA as before.
>
> My main worry is that it's not easily clear what is going on if you look
> at the panel code. It looks that the panel just uses whatever is in the
> output driver. It's not clear that it is always VGA. What if the
> previous mode was something else?
>
> I think it's much clearer if the panel driver sets the timings
> explicitly before enabling the output. It doesn't have to be in panel's
> probe, although that's perhaps the easiest place for it.
>
>> I have done the same for venc, let me know if you think these should be
>> removed.
>
> Well, I agree that the initial timings code in hdmi and venc is a bit
> ugly. I don't think it's bad as such, just that the timings are standard
> ones, and instead of having all the timings numbers there, we should
> have a common place for them.
Okay, I'll remove the get_timing ops, keep defaults in the panel
driver's probe, have a const variable for the defaults to make it look
less ugly, and make sure that there is a set_timings for the output in
the hdmi and venc panel drivers before they are enabled, it sort of okay
for hdmi and venc panel drivers as there is going to be only one of them
and be in our control.
Archit
^ permalink raw reply
* Re: [PATCH 0/7] HID: picoLCD updates
From: Tejun Heo @ 2012-08-14 17:31 UTC (permalink / raw)
To: Bruno Prémont; +Cc: linux-input, linux-kernel, Jiri Kosina, linux-fbdev
In-Reply-To: <20120814083044.78b401d2@pluto.restena.lu>
Hello,
On Tue, Aug 14, 2012 at 08:30:44AM +0200, Bruno Prémont wrote:
> > I'm kinda shooting in the dark but who flushes / cancels
> > fb_info->deferred_work?
>
> fb_deferred_io_cleanup() does so and is called by destroy fbops
> (when last reference to fb_info is returned):
I see. Sorry but just from glancing it I can't really tell what's
going wrong.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v2 09/13] OMAPDSS: SDI: Create a function to set timings
From: Tomi Valkeinen @ 2012-08-14 17:33 UTC (permalink / raw)
To: Archit Taneja, Rob Clark; +Cc: linux-fbdev, linux-omap
In-Reply-To: <502A8322.6000309@ti.com>
[-- Attachment #1: Type: text/plain, Size: 2119 bytes --]
On Tue, 2012-08-14 at 22:26 +0530, Archit Taneja wrote:
> On Tuesday 14 August 2012 07:14 PM, Tomi Valkeinen wrote:
> I guess it depends on how drm/fb want to use it. I guess an output
> should have a set_timings() kind of op if it can do it seamlessly. I
> guess we can do that easily in DPI, for example, we could reduce the fps
> from 60 to 30 without causing an artefacts(I think). For outputs which
Yes, that kind of thing is easy to do by just changing the pck divider,
which is in a shadow register. I mean, "easy" in theory, at least. Our
clock calculation doesn't work like that currently, though, so it could
end up changing DSS fck.
> can't do it, we could remove the set_timings totally.
But we do need set_timings for other outputs also (like sdi). We just
can't change them just like that. Only outputs that do not need timings
are DSI command mode and rfbi.
> However, it'll be kind of inconsistent for some outputs to set timings,
> and for others to not, and if in the future drm/fb gets exposed to ops
> too, we may have dirty checks to see if set_timings is populated or not.
>
> The easiest way would be to make all set_timings just update the copy of
> the timings output has, and expect drm/fb to disable and re enable the
> panel. We may end up doing unnecessary gpio resets and configuration of
> the panels though.
I think changing things like timings is quite a rare operation. The only
case it'd be necessary to change timings often, with speed, and without
artifacts would be the fps drop you mentioned, for lower power use with
panels that don't mind the fps drop.
If I understood correctly, Rob said that drm already disables the output
when changing the mode, when I asked if it's ok for the apply's
set_timings to require the output to be off.
In any case this is not a big issue, I mean, it's not causing any
problems. Somebody is going to disable the output anyway when changing
the timings. Perhaps even these patches are good, because they make the
set_timings consistent across the output drivers (don't they?).
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v2 09/13] OMAPDSS: SDI: Create a function to set timings
From: Archit Taneja @ 2012-08-14 19:20 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: Rob Clark, linux-fbdev, linux-omap
In-Reply-To: <1344965600.2345.70.camel@deskari>
On Tuesday 14 August 2012 11:03 PM, Tomi Valkeinen wrote:
> On Tue, 2012-08-14 at 22:26 +0530, Archit Taneja wrote:
>> On Tuesday 14 August 2012 07:14 PM, Tomi Valkeinen wrote:
>
>> I guess it depends on how drm/fb want to use it. I guess an output
>> should have a set_timings() kind of op if it can do it seamlessly. I
>> guess we can do that easily in DPI, for example, we could reduce the fps
>> from 60 to 30 without causing an artefacts(I think). For outputs which
>
> Yes, that kind of thing is easy to do by just changing the pck divider,
> which is in a shadow register. I mean, "easy" in theory, at least. Our
> clock calculation doesn't work like that currently, though, so it could
> end up changing DSS fck.
>
>> can't do it, we could remove the set_timings totally.
>
> But we do need set_timings for other outputs also (like sdi). We just
> can't change them just like that. Only outputs that do not need timings
> are DSI command mode and rfbi.
>
>> However, it'll be kind of inconsistent for some outputs to set timings,
>> and for others to not, and if in the future drm/fb gets exposed to ops
>> too, we may have dirty checks to see if set_timings is populated or not.
>>
>> The easiest way would be to make all set_timings just update the copy of
>> the timings output has, and expect drm/fb to disable and re enable the
>> panel. We may end up doing unnecessary gpio resets and configuration of
>> the panels though.
>
> I think changing things like timings is quite a rare operation. The only
> case it'd be necessary to change timings often, with speed, and without
> artifacts would be the fps drop you mentioned, for lower power use with
> panels that don't mind the fps drop.
>
> If I understood correctly, Rob said that drm already disables the output
> when changing the mode, when I asked if it's ok for the apply's
> set_timings to require the output to be off.
>
> In any case this is not a big issue, I mean, it's not causing any
> problems. Somebody is going to disable the output anyway when changing
> the timings. Perhaps even these patches are good, because they make the
> set_timings consistent across the output drivers (don't they?).
Yes, they do, there isn't a set_timings for RFBI though, only a
set_size, and DSI has set_timings for video mode and a set_size for
command mode, I haven't put checks in the ops for a panel driver to
wrongly call set_timings in commmand mode, and set_size in video mode.
I haven't done that yet because a future patch of mine will have a DSI
specific op called set_operation_mode() to make us independent of
dssdev->panel.dsi_mode, there is no guarantee that the panel driver to
first call set_operation_mode(), and then set_timings(), so I'm not sure
yet how to deal with that. Probably having a mode/state which says that
a field is unintialized might help, but that would overcomplicate things.
Archit
^ permalink raw reply
* Re: [PATCH v2 09/13] OMAPDSS: SDI: Create a function to set timings
From: Rob Clark @ 2012-08-14 19:26 UTC (permalink / raw)
To: Archit Taneja; +Cc: Tomi Valkeinen, linux-fbdev, linux-omap
In-Reply-To: <502A8322.6000309@ti.com>
On Tue, Aug 14, 2012 at 11:56 AM, Archit Taneja <archit@ti.com> wrote:
> On Tuesday 14 August 2012 07:14 PM, Tomi Valkeinen wrote:
>>
>> On Thu, 2012-08-09 at 17:19 +0530, Archit Taneja wrote:
>>>
>>> Create function omapdss_sdi_set_timings(). Configuring new timings is
>>> done the
>>> same way as before, SDI is disabled, and re-enabled with the new timings
>>> in
>>> dssdev. This just moves the code from the panel drivers to the SDI
>>> driver.
>>>
>>> The panel drivers shouldn't be aware of how SDI manages to configure a
>>> new set
>>> of timings. This should be taken care of by the SDI driver itself.
>>
>>
>> I'm not sure about this one. Although I see that dpi.c does currently
>> the same thing as you're doing in your patch.
>
>
> Even HDMI does the same thing.
>
>
>>
>> One thing is that we should try to remove dssdev uses from the output
>> drivers, including use of dssdev->state.
>
>
> Yes, we could do that by keeping a state of the output(and also checking
> state of the manager)
>
>
>>
>> The other thing is that I don't think the output driver should disable &
>> enable the output during set timings. I think sdi's set_timings should
>> return EBUSY if the output is enabled. The same way as other
>> configuration functions should (like dpi_set_data_lines or such).
>>
>> I'm actually not sure if even the panel driver should disable & enable
>> the output during set_timings. Perhaps it should be the caller's
>> (omapdrm or such) responsibility....
>>
>> My reasoning here is that disabling & enabling the video output is not
>> invisible to the upper layers, so doing it "in secret" may be bad.
>>
>> Then again, perhaps timings can be changed freely on some other
>> platforms, and then it'd be nice if the panel driver wouldn't disable &
>> enable the output.
>>
>> So I'm again not quite sure what's the best way to handle this... (of
>> the dssdev->state I'm sure, its use should be removed from omapdss). Any
>> thoughts?
>
>
> I guess it depends on how drm/fb want to use it. I guess an output should
> have a set_timings() kind of op if it can do it seamlessly. I guess we can
> do that easily in DPI, for example, we could reduce the fps from 60 to 30
> without causing an artefacts(I think). For outputs which can't do it, we
> could remove the set_timings totally.
fwiw, drm wouldn't try to change timings on the fly.. or at least it
is bracketed by a call to the driver's crtc->prepare() and
crtc->commit() fxns (which in our case disable/enable output).
I haven't seen much userspace that tries to do things like this,
except maybe some apps like xbmc which seem to have some options to
attempt to change timings to align w/ video playback framerate. I am
a bit curious how many tv's and drivers could support this in a
glitch-free way.
BR,
-R
> However, it'll be kind of inconsistent for some outputs to set timings, and
> for others to not, and if in the future drm/fb gets exposed to ops too, we
> may have dirty checks to see if set_timings is populated or not.
>
> The easiest way would be to make all set_timings just update the copy of the
> timings output has, and expect drm/fb to disable and re enable the panel. We
> may end up doing unnecessary gpio resets and configuration of the panels
> though.
>
> Archit
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] video/mx3fb: remove stray l from debug output
From: Geert Uytterhoeven @ 2012-08-14 19:30 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1344843875-8174-1-git-send-email-u.kleine-koenig@pengutronix.de>
On Mon, Aug 13, 2012 at 7:09 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Mon, Aug 13, 2012 at 9:44 AM, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> ---
>> drivers/video/mx3fb.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/video/mx3fb.c b/drivers/video/mx3fb.c
>> index c89f8a8..1dfdeb1 100644
>> --- a/drivers/video/mx3fb.c
>> +++ b/drivers/video/mx3fb.c
>> @@ -771,7 +771,7 @@ static int __set_par(struct fb_info *fbi, bool lock)
>> if (fbi->var.sync & FB_SYNC_SHARP_MODE)
>> mode = IPU_PANEL_SHARP_TFT;
>>
>> - dev_dbg(fbi->device, "pixclock = %ul Hz\n",
>> + dev_dbg(fbi->device, "pixclock = %u Hz\n",
>> (u32) (PICOS2KHZ(fbi->var.pixclock) * 1000UL));
>
> I think a better fix is to change the dyslectic "%ul" to "%lu", and
> drop the cast to
> u32, which was probably only added to kill the compiler warning caused by the
> dyslectia issue.
BTW, 'git grep "%ul"' shows a few more of these dyslectia issues.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH 05/11] fblog: register one fblog object per framebuffer
From: Ryan Mallon @ 2012-08-15 0:17 UTC (permalink / raw)
To: David Herrmann
Cc: linux-fbdev, Florian Tobias Schandinat, Greg Kroah-Hartman,
linux-serial, Alan Cox, linux-kernel, Geert Uytterhoeven
In-Reply-To: <CANq1E4QP8PwQRFBr++b3cu1dM9NWf6+Y45rq4UAirjaAHyeM4Q@mail.gmail.com>
On 14/08/12 21:01, David Herrmann wrote:
> Hi Ryan
>
> On Mon, Aug 13, 2012 at 1:54 AM, Ryan Mallon <rmallon@gmail.com> wrote:
>> On 13/08/12 00:53, David Herrmann wrote:
>>> drivers/video/console/fblog.c | 195 ++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 195 insertions(+)
>>>
>>> diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
>>> index fb39737..279f4d8 100644
>>> --- a/drivers/video/console/fblog.c
>>> +++ b/drivers/video/console/fblog.c
>>> @@ -23,15 +23,210 @@
>>> * all fblog instances before running other graphics applications.
>>> */
>>>
>>> +#define pr_fmt(_fmt) KBUILD_MODNAME ": " _fmt
>>> +
>>> +#include <linux/device.h>
>>> +#include <linux/fb.h>
>>> #include <linux/module.h>
>>> +#include <linux/mutex.h>
>>> +
>>> +enum fblog_flags {
>>> + FBLOG_KILLED,
>>> +};
>>> +
>>> +struct fblog_fb {
>>> + unsigned long flags;
>>
>> Are more flags added in later patches? If not, why not just have:
>>
>> bool is_killed;
>>
>> ?
>
> Yes, more are added in patch-6 and patch-8 which includes FBLOG_OPEN,
> FBLOG_SUSPENDED, FBLOG_BLANKED.
>
>>> +static void fblog_release(struct device *dev)
>>> +{
>>> + struct fblog_fb *fb = to_fblog_dev(dev);
>>> +
>>> + kfree(fb);
>>> + module_put(THIS_MODULE);
>>> +}
>>> +
>>> +static void fblog_do_unregister(struct fb_info *info)
>>> +{
>>> + struct fblog_fb *fb;
>>> +
>>> + fb = fblog_fbs[info->node];
>>> + if (!fb || fb->info != info)
>>> + return;
>>> +
>>> + fblog_fbs[info->node] = NULL;
>>> +
>>> + device_del(&fb->dev);
>>> + put_device(&fb->dev);
>>
>> device_unregister?
>
> Right, I will replace it.
>
>>> +}
>>> +
>>> +static void fblog_do_register(struct fb_info *info, bool force)
>>> +{
>>> + struct fblog_fb *fb;
>>> + int ret;
>>> +
>>> + fb = fblog_fbs[info->node];
>>> + if (fb && fb->info != info) {
>>> + if (!force)
>>> + return;
>>> +
>>> + fblog_do_unregister(fb->info);
>>> + }
>>> +
>>> + fb = kzalloc(sizeof(*fb), GFP_KERNEL);
>>> + if (!fb)
>>> + return;
>>> +
>>> + fb->info = info;
>>> + __module_get(THIS_MODULE);
>>> + device_initialize(&fb->dev);
>>> + fb->dev.class = fb_class;
>>> + fb->dev.release = fblog_release;
>>> + dev_set_name(&fb->dev, "fblog%d", info->node);
>>> + fblog_fbs[info->node] = fb;
>>> +
>>> + ret = device_add(&fb->dev);
>>> + if (ret) {
>>> + fblog_fbs[info->node] = NULL;
>>> + set_bit(FBLOG_KILLED, &fb->flags);
>>> + put_device(&fb->dev);
>>
>> kfree(fb); ?
>
> No. See device_initialize() in ./drivers/base/core.c. After a call to
> device_initialize() the object is ref-counted so put_device() will
> invoke the fblog_release() callback which will call kfree(fb) itself.
>
>>> + return;
>>> + }
>>> +}
>>> +
>>> +static void fblog_register(struct fb_info *info, bool force)
>>> +{
>>> + mutex_lock(&fblog_registration_lock);
>>> + fblog_do_register(info, force);
>>> + mutex_unlock(&fblog_registration_lock);
>>> +}
>>> +
>>> +static void fblog_unregister(struct fb_info *info)
>>> +{
>>> + mutex_lock(&fblog_registration_lock);
>>> + fblog_do_unregister(info);
>>> + mutex_unlock(&fblog_registration_lock);
>>> +}
>>
>> This locking is needlessly heavy, and could easily pushed down into the
>> fb_do_(un)register functions. It would also help make it clear exactly
>> what the lock is protecting.
>
> I need to call fblog_do_unregister() from within fblog_do_register().
> I cannot release the locks while calling fblog_do_unregister() so I
> need the unlocked fblog_do_unregister() function. So the locking must
> be in a wrapper function.
>
> See below for an explanation of the locks.
I meant something like the below. It doesn't actually make the lock much
more fine-grained, but (IMHO) it does make it a bit more clear how the
lock is being used. I also don't think you need to split
device_initialize and device_add, which can make the code a bit simpler:
static void __fblog_unregister(struct fblog_fb *fb)
{
fblog_fbs[fb->info->node] = NULL;
device_unregister(&fb->dev);
}
static void fblog_unregister(struct fb_info *info)
{
struct fblog_fb *fb;
mutex_lock(&fblog_registration_lock);
fb = fblog_fbs[info->node];
if (!fb || fb->info != info) {
mutex_unlock(&fblog_registration_lock);
return;
}
__fblog_unregister(fb);
mutex_unlock(&fblog_registration_lock);
}
static int fblog_register(struct fb_info *info, bool force)
{
struct fblog_fb *fb;
int ret;
mutex_lock(&fblog_registration_lock);
fb = fblog_fbs[info->node];
if (fb && fb->info != info) {
if (!force) {
mutex_unlock(&fblog_registration_lock);
return -EEXIST;
}
__fblog_unregister(fb);
}
fb = kzalloc(sizeof(*fb), GFP_KERNEL);
if (!fb)
return;
fb->info = info;
__module_get(THIS_MODULE);
fb->dev.class = fb_class;
fb->dev.release = fblog_release;
dev_set_name(&fb->dev, "fblog%d", info->node);
ret = device_register(&fb->dev);
if (ret) {
mutex_unlock(&fblog_registeration_lock);
put_device(&fb->dev);
return ret;
}
fblog_fbs[info->node] = fb;
mutex_unlock(&fblog_registeration_lock);
return 0;
}
Functions which do:
foo() {
lock(some_lock);
do_foo();
unlock(some_lock);
}
can be a valid pattern for locked/unlocked versions (usually the
unlocked version do_foo will be called __foo). But other times it looks
lazy, where the lock is just serialising everthing and doesn't scale
well. Granted, in a case like this it probably doesn't matter, but it
still a good idea to try and make the locking as fine grained as
possible. It also helps when trying to determine what a lock is actually
protecting, since if do_foo is long, the lock may or may not be
protecting any number of things inside it.
>>> +static int fblog_event(struct notifier_block *self, unsigned long action,
>>> + void *data)
>>> +{
>>> + struct fb_event *event = data;
>>> + struct fb_info *info = event->info;
>>> +
>>> + switch(action) {
>>> + case FB_EVENT_FB_REGISTERED:
>>> + /* This is called when a low-level system driver registers a new
>>> + * framebuffer. The registration lock is held but the console
>>> + * lock might not be held when this is called. */
>>
>> Nitpick:
>>
>> /*
>> * The Linux kernel multi-line
>> * comment style looks like
>> * this.
>> */
>
> I was confused by a recent discussion on the LKML:
> http://comments.gmane.org/gmane.linux.kernel/1282421
> However, turns out they didn't add this to CodingStyle so I will adopt
> the old style again. Will be fixed in the next revision, thanks.
>
>>> + fblog_register(info, true);
>>> + break;
>>> + case FB_EVENT_FB_UNREGISTERED:
>>> + /* This is called when a low-level system driver unregisters a
>>> + * framebuffer. The registration lock is held but the console
>>> + * lock might not be held. */
>>> + fblog_unregister(info);
>>> + break;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void fblog_scan(void)
>>> +{
>>> + unsigned int i;
>>> + struct fb_info *info, *tmp;
>>> +
>>> + for (i = 0; i < FB_MAX; ++i) {
>>> + info = get_fb_info(i);
>>> + if (!info || IS_ERR(info))
>>
>> Nitpick:
>>
>> if (IS_ERR_OR_NULL(info))
>
> Didn't know of this macro, thanks.
>
>>> + continue;
>>> +
>>> + fblog_register(info, false);
>>
>> This function should really return a value to indicate if it failed.
>> There is no point continuing if it didn't register anything.
>
> Indeed.
>
>>> + /* There is a very subtle race-condition. Even though we might
>>> + * own a reference to the fb, it may still get unregistered
>>> + * between our call from get_fb_info() and fblog_register().
>>> + * Therefore, we simply check whether the same fb still is
>>> + * registered by calling get_fb_info() again. Only if they
>>> + * differ we know that it got unregistered, therefore, we
>>> + * call fblog_unregister() with the old pointer. */
>>> +
>>> + tmp = get_fb_info(i);
>>> + if (tmp && !IS_ERR(tmp))
>>> + put_fb_info(tmp);
>>> + if (tmp != info)
>>> + fblog_unregister(info);
>>
>> It would be better to fix this issue properly. Calling fblog_unregister
>> here also looks unsafe if the call to fblog_register above failed.
>
> fblog_unregister() does nothing if the passed "info" does not match
> the found "info" so this is safe. And as we are holding a reference to
> "info" here, the pointer is always valid and cannot be used by other
> FBs.
>
> Fixing this properly means changing the locking of fbdev. This can
> either be done by exporting the fbdev-registration-lock (which I want
> to avoid as locking should never be exported in an API) or by changing
> fbdev to provide an scan/enumeration function itself. However, in my
> opinion both would be uglier than using this race-condition-check
> here.
>
> The best way would be redesigning the fbdev in-kernel API. But that
> means checking that fbcon will not break and this is something I
> really don't want to touch.
Fair enough. It might be something to come back to once this gets merged.
>>> + /* Here we either called fblog_unregister() and therefore do not
>>> + * need any reference to the fb, or we can be sure that the FB
>>> + * is registered and FB_EVENT_FB_UNREGISTERED will be called
>>> + * before the last reference is dropped. Hence, we can drop our
>>> + * reference here. */
>>
>> This seems a slightly odd reasoning. Why would you not hold a reference
>> to something you are using?
>
> It's because get_fb_info() takes the fbdev-registration-lock so we
> cannot call it from fblog_event().
That could possibly be fixed by providing an unlocked __get_fb_info
function. Then the ref-counting could be done by calling __get_fb_info
in fblog_register and __put_fb_info in fblog_unregister, so that you
hold the ref-count for the lifetime of the fblog object which I think
makes a bit more sense.
> And fblog_event() calls
> fblog_register() for hotplugged framebuffers so we cannot get a refcnt
> for hotplugged framebuffers.
> Now I can either increase the fbdev-refcount manually without calling
> get_fb_info() in fblog_register() or I can simply drop the framebuffer
> when the fbdev-core notifies me that it is gone. I chose the latter
> one.
>
>>> + put_fb_info(info);
>>> + }
>>> +}
>>> +
>>> +static struct notifier_block fblog_notifier = {
>>> + .notifier_call = fblog_event,
>>> +};
>>>
>>> static int __init fblog_init(void)
>>> {
>>> + int ret;
>>> +
>>> + ret = fb_register_client(&fblog_notifier);
>>> + if (ret) {
>>> + pr_err("cannot register framebuffer notifier\n");
>>> + return ret;
>>> + }
>>> +
>>> + fblog_scan();
>>> +
>>> return 0;
>>> }
>>>
>>> static void __exit fblog_exit(void)
>>> {
>>> + unsigned int i;
>>> + struct fb_info *info;
>>> +
>>> + fb_unregister_client(&fblog_notifier);
>>> +
>>> + /* We scan through the whole registered_fb array here instead of
>>> + * fblog_fbs because we need to get the device lock _before_ the
>>> + * fblog-registration-lock. */
>>> +
>>> + for (i = 0; i < FB_MAX; ++i) {
>>> + info = get_fb_info(i);
>>> + if (!info || IS_ERR(info))
>>> + continue;
>>> +
>>> + fblog_unregister(info);
>>
>> Given the description of the get_fb_info/fblog_register race above, can
>> this unregister the wrong framebuffer?
>
> No. fblog_unregister() will do nothing if the "info" pointers do not
> match. Moreover, this unregisters _all_ framebuffers, so I don't
> understand how this can unregister the "wrong" framebuffer?
>
> But I just noticed a race here. If the fbdev core unregisters a
> framebuffer during my loop, I will not call fblog_unregister() on it,
> as it does not exist anymore. Therefore, I will not free it in my
> fblog_fbs array as the fblog_notifier has already been unregistered.
> So I need to set a global "exiting" flag and fblog_event() shall only
> handle the UNBIND/UNREGISTER events during exit. Then I simply move
> the fb_unregister_client() call below this loop.
>
>>> + put_fb_info(info);
>>> + }
>>> }
>>>
>>> module_init(fblog_init);
>>>
>>
>
> A short explanation for all locks:
> First of all, I spent hours getting this right. The easy way would be
> removing all locks and relying on the fbdev locking (fbcon does this).
> But obviously, that doesn't work as fbdev has no safe way of
> scanning/enumerating all existing devices. And this is needed as fblog
> can be compiled as a module.
> fbdev has a core registration-lock and each framebuffer has its own
> fb-lock. fblog_event() is sometimes called with these locks held,
> sometimes without any locks held (see the comments in this function)
> and I need to work around this.
> So fblog_event() as entry point for hotplugging is called with the
> fbdev-registration-lock held. And it calls fblog_(un)register() which
> uses its own locks. So when calling this from fblog_scan(), I need to
> make sure to guarantee that the locks are acquired in the same order
> as in fblog_event(), otherwise I might have deadlocks. But I have no
> outside access to the fbdev-registration-lock, therefore, the
> fblog-registration-lock is needed and fblog_scan() needs to check for
> those ugly races.
Right, I think providing unlocked versions of get/put_fb_info will help
fix part of this.
> As you mentioned that this locking is needlessly complex, I have to
> disagree.
Sorry, poor choice of words. I meant 'coarse-grained', not complex.
However, some documentation in the code explaining how the locking
works, and what the locking order is never goes amiss.
> I use one lock to protect the registration
> (fblog_registration_lock) and one lock to protect each registered
> framebuffer from concurrent access (struct fblog_fb->lock). This is
> the most common way to protect hotplugged devices and I don't see how
> this can be done with less locks? (these are the only locks that are
> added by fblog).
I was only referring to the 'heavy' usage of the registration lock by
just acquiring it for the whole register/unregister functions. I was
skimming through the code and was assuming that the actual concurrent
part would just be the addition/removal in the fblog_fbs array, and
therefore the lock was being held for much longer than it needed to be.
As shown above, it isn't as bad as I thought it was.
> Normally, this would be all locks I have to access. However, the
> fbdev-core wasn't designed as in-kernel API and thus has very
> inconsistent locking. And all entry points to fblog that are not
> through fbdev-notifier-callbacks (eg, fblog_scan) need to make sure to
> acquire the same locks as the fbdev-core to avoid races with the
> fbdev-core. As this is not possible, because the fbdev-locks are not
> exported, I need to carefully use fbdev-functions that guarantee that
> I have no races. And I think I found the only way to guarantee this.
> If anyone has other ideas, I would be glad to hear them.
Yeah, this makes sense. It would be good, as you say, to not export the
locks for fbmem. I think adding a couple of functions to fbmem.c for
doing unlocked access might help a lot though.
~Ryan
^ permalink raw reply
* Re: [PATCH v2 09/13] OMAPDSS: SDI: Create a function to set timings
From: Tomi Valkeinen @ 2012-08-15 6:43 UTC (permalink / raw)
To: Archit Taneja; +Cc: Rob Clark, linux-fbdev, linux-omap
In-Reply-To: <502AA232.1040305@ti.com>
[-- Attachment #1: Type: text/plain, Size: 3953 bytes --]
On Wed, 2012-08-15 at 00:38 +0530, Archit Taneja wrote:
> On Tuesday 14 August 2012 11:03 PM, Tomi Valkeinen wrote:
> > On Tue, 2012-08-14 at 22:26 +0530, Archit Taneja wrote:
> >> On Tuesday 14 August 2012 07:14 PM, Tomi Valkeinen wrote:
> >
> >> I guess it depends on how drm/fb want to use it. I guess an output
> >> should have a set_timings() kind of op if it can do it seamlessly. I
> >> guess we can do that easily in DPI, for example, we could reduce the fps
> >> from 60 to 30 without causing an artefacts(I think). For outputs which
> >
> > Yes, that kind of thing is easy to do by just changing the pck divider,
> > which is in a shadow register. I mean, "easy" in theory, at least. Our
> > clock calculation doesn't work like that currently, though, so it could
> > end up changing DSS fck.
> >
> >> can't do it, we could remove the set_timings totally.
> >
> > But we do need set_timings for other outputs also (like sdi). We just
> > can't change them just like that. Only outputs that do not need timings
> > are DSI command mode and rfbi.
> >
> >> However, it'll be kind of inconsistent for some outputs to set timings,
> >> and for others to not, and if in the future drm/fb gets exposed to ops
> >> too, we may have dirty checks to see if set_timings is populated or not.
> >>
> >> The easiest way would be to make all set_timings just update the copy of
> >> the timings output has, and expect drm/fb to disable and re enable the
> >> panel. We may end up doing unnecessary gpio resets and configuration of
> >> the panels though.
> >
> > I think changing things like timings is quite a rare operation. The only
> > case it'd be necessary to change timings often, with speed, and without
> > artifacts would be the fps drop you mentioned, for lower power use with
> > panels that don't mind the fps drop.
> >
> > If I understood correctly, Rob said that drm already disables the output
> > when changing the mode, when I asked if it's ok for the apply's
> > set_timings to require the output to be off.
> >
> > In any case this is not a big issue, I mean, it's not causing any
> > problems. Somebody is going to disable the output anyway when changing
> > the timings. Perhaps even these patches are good, because they make the
> > set_timings consistent across the output drivers (don't they?).
>
> Yes, they do, there isn't a set_timings for RFBI though, only a
> set_size, and DSI has set_timings for video mode and a set_size for
> command mode, I haven't put checks in the ops for a panel driver to
> wrongly call set_timings in commmand mode, and set_size in video mode.
Ok. Well, perhaps we should go forward with these patches then. They
make things consistent, and we don't really know which would be the best
way to handle this, so the method in your patches is as good as some
other.
The dssdev->state needs to be removed at some point, but that can be a
separate task.
> I haven't done that yet because a future patch of mine will have a DSI
> specific op called set_operation_mode() to make us independent of
> dssdev->panel.dsi_mode, there is no guarantee that the panel driver to
> first call set_operation_mode(), and then set_timings(), so I'm not sure
> yet how to deal with that. Probably having a mode/state which says that
> a field is unintialized might help, but that would overcomplicate things.
Well, we can define that set_operation_mode needs to be called before
any other dsi functions. They are kernel drivers, we can presume they
act correctly (although we should of course try to handle error cases
anyway). If they don't, we need to fix them.
If you want to be extra safe there, you could add third value to the
dsi_mode enum: UNDEFINED or such (I guess this is what you meant also).
By default dsi's mode would be undefined, and functions could check it.
But I agree it'd add lots of checks all around.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 6/7] HID: picoLCD: disable version check during probe
From: Jiri Kosina @ 2012-08-15 8:15 UTC (permalink / raw)
To: Bruno Prémont; +Cc: linux-input, linux-kernel, linux-fbdev
In-Reply-To: <20120730213859.063173c1@neptune.home>
On Mon, 30 Jul 2012, Bruno Prémont wrote:
> Commit 4ea5454203d991ec85264f64f89ca8855fce69b0
> [HID: Fix race condition between driver core and ll-driver] introduced
> new locking around proce/remove functions that prevent 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.
>
> Disabling the check lets driver sucessfully probe again.
>
> Signed-off-by: Bruno Prémont <bonbons@linux-vserver.org>
> ---
> drivers/hid/hid-picolcd_core.c | 8 ++++----
> 1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
> index 2d7ef68..42d0791 100644
> --- a/drivers/hid/hid-picolcd_core.c
> +++ b/drivers/hid/hid-picolcd_core.c
> @@ -478,13 +478,13 @@ static int picolcd_probe_lcd(struct hid_device *hdev, struct picolcd_data *data)
> {
> int error;
>
> - error = picolcd_check_version(hdev);
> +/* 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));
> + dev_name(&hdev->dev)); */
Please just remove it altogether, I don't see a reason to keep the
commented-out code in the in-tree driver.
Once the locking mess is sorted out, we can re-introduce it again as
necessary.
Thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 0/7] HID: picoLCD updates
From: Jiri Kosina @ 2012-08-15 8:27 UTC (permalink / raw)
To: Bruno Prémont; +Cc: linux-input, linux-kernel, linux-fbdev
In-Reply-To: <20120730213656.0a9f6d30@neptune.home>
On Mon, 30 Jul 2012, Bruno Prémont wrote:
> Hi,
>
> This series updates picoLCD driver:
> - split the driver functions into separate files which get included
> depending on Kconfig selection
> (implementation for CIR using RC_CORE will follow later)
> - drop private framebuffer refcounting in favor of refcounting added
> to fb_info some time ago
> - fix various bugs issues
> - disabled firmware version checking in probe() as it does not work
> anymore since commit 4ea5454203d991ec85264f64f89ca8855fce69b0
> [HID: Fix race condition between driver core and ll-driver]
I have now applied the series to my 'picolcd' branch, except for 6/7,
please see the comment I sent to it separately.
> Note: I still get weird behavior on quick unbind/bind sequences
> issued via sysfs (CONFIG_SMP=n system) that are triggered by framebuffer
> support and apparently more specifically fb_defio part of it.
>
> Unfortunately I'm out of ideas as to how to track down the problem which
> shows either as SLAB corruption (detected with SLUB debugging, e.g.
Would be nice to have this sorted out before the next merge window indeed,
so that it can go in together with the rest of the changes.
>
> [ 6383.521833] ======================================> [ 6383.530020] BUG kmalloc-64 (Not tainted): Object already free
> [ 6383.530020] -----------------------------------------------------------------------------
> [ 6383.530020]
> [ 6383.530020] INFO: Slab 0xdde0ea20 objectsQ used@ fp=0xcef516e0 flags=0x40000080
> [ 6383.530020] INFO: Object 0xcef51190 @offset@0 fp=0xcef51f50
> [ 6383.530020]
> [ 6383.530020] Bytes b4 cef51180: cc cc cc cc d0 12 f5 ce 5a 5a 5a 5a 5a 5a 5a 5a ........ZZZZZZZZ
> [ 6383.530020] Object cef51190: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
> [ 6383.530020] Object cef511a0: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
> [ 6383.530020] Object cef511b0: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
> [ 6383.530020] Object cef511c0: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5 kkkkkkkkkkkkkkk.
> [ 6383.530020] Redzone cef511d0: bb bb bb bb ....
> [ 6383.530020] Padding cef511d8: 5a 5a 5a 5a 5a 5a 5a 5a ZZZZZZZZ
> [ 6383.530020] Pid: 1922, comm: bash Not tainted 3.5.0-jupiter-00003-g8d858b1-dirty #2
> [ 6383.530020] Call Trace:
> [ 6383.530020] [<c10bd3cc>] print_trailer+0x11c/0x130
> [ 6383.530020] [<c10bd415>] object_err+0x35/0x40
> [ 6383.530020] [<c10be809>] free_debug_processing+0x99/0x200
> [ 6383.530020] [<c10bf77e>] __slab_free+0x2e/0x280
> [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> [ 6383.530020] [<c1322870>] ? __usbhid_submit_report+0xc0/0x3c0
> [ 6383.530020] [<c10bfbda>] ? kfree+0xfa/0x110
> [ 6383.530020] [<de932aa4>] ? picolcd_debug_out_report+0x8c4/0x8e0 [hid_picolcd]
> [ 6383.530020] [<c10bfbda>] kfree+0xfa/0x110
> [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> [ 6383.530020] [<c1322284>] hid_submit_out+0xa4/0x120
> [ 6383.530020] [<c1322908>] __usbhid_submit_report+0x158/0x3c0
> [ 6383.530020] [<c1322c2b>] usbhid_submit_report+0x1b/0x30
> [ 6383.530020] [<de930789>] picolcd_fb_reset+0xb9/0x180 [hid_picolcd]
> [ 6383.530020] [<de930f1d>] picolcd_init_framebuffer+0x20d/0x2e0 [hid_picolcd]
> [ 6383.530020] [<de92fb9c>] picolcd_probe+0x3cc/0x580 [hid_picolcd]
> [ 6383.530020] [<c1319147>] hid_device_probe+0x67/0xf0
> [ 6383.530020] [<c1282f97>] ? driver_sysfs_add+0x57/0x80
> [ 6383.530020] [<c128329d>] driver_probe_device+0xbd/0x1c0
> [ 6383.530020] [<c1318a1b>] ? hid_match_device+0x7b/0x90
> [ 6383.530020] [<c12821e5>] driver_bind+0x75/0xd0
> [ 6383.530020] [<c1282170>] ? driver_unbind+0x90/0x90
> [ 6383.530020] [<c12818b7>] drv_attr_store+0x27/0x30
> [ 6383.530020] [<c1114aec>] sysfs_write_file+0xac/0xf0
> [ 6383.530020] [<c10c794c>] vfs_write+0x9c/0x130
> [ 6383.530020] [<c10d4a1f>] ? sys_dup3+0x11f/0x160
> [ 6383.530020] [<c1114a40>] ? sysfs_poll+0x90/0x90
> [ 6383.530020] [<c10c7bbd>] sys_write+0x3d/0x70
> [ 6383.530020] [<c13f2557>] sysenter_do_call+0x12/0x26
So I am wondering whether the path this happens on is
if (!test_bit(HID_OUT_RUNNING, &usbhid->iofl)) {
usbhid_restart_out_queue(usbhid);
in __usbhid_submit_report(). It would then indicate perhaps some race with
iofl handling.
Could you please stick some printk() just before and after this
usbhid_restart_out_queue() call, so that we know that it's this triggering
it?
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 0/5] Pass data lines and pixel format info from panel driver to interface
From: Tomi Valkeinen @ 2012-08-15 8:41 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-fbdev, linux-omap
In-Reply-To: <1344595026-10607-1-git-send-email-archit@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1074 bytes --]
On Fri, 2012-08-10 at 16:07 +0530, Archit Taneja wrote:
> This is a continuation of the work started in the series:
>
> http://marc.info/?l=linux-omap&m=134381744304672&w=2
>
> This makes the panel driver call interface specific functions to configure the
> data lines and pixel format requested by the panel.
>
> Configuration of data lines doesn't happen for DSI or HDMI. For DSI, this
> information is more complex and is taken care by omapdss_dsi_configure_pins(),
> for HDMI, the number of TMDS lines is always fixed.
>
> Configuration of pixel format is done only for DSI in command mode and RFBI.
> Currently, these new functions take different arguments,
> omapdss_dsi_set_pixel_format() takes pixel format of type
> omap_dss_dsi_pixel_format enum, and RFBI omapdss_rfbi_set_pixel_size() takes
> the pixel size directly. It would be good if these could be merged by using
> some sort of MIPI standard for pixel formats. Having the same argument would
> help in having a common interface op in the future.
This series looks good.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH] video:uvesafb: reduce the double check
From: Wang YanQing @ 2012-08-15 9:33 UTC (permalink / raw)
To: FlorianSchandinat; +Cc: linux-fbdev, linux-kernel, spock
uvesafb_open had checked the par->vbe_state_size,
so we don't need to check it again in uvesafb_vbe_state_save,
this patch just can reduce a few lines of code.
Signed-off-by: Wang YanQing <udknight@gmail.com>
---
drivers/video/uvesafb.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/video/uvesafb.c b/drivers/video/uvesafb.c
index 2f8f82d..b064a3e 100644
--- a/drivers/video/uvesafb.c
+++ b/drivers/video/uvesafb.c
@@ -357,9 +357,6 @@ static u8 *uvesafb_vbe_state_save(struct uvesafb_par *par)
u8 *state;
int err;
- if (!par->vbe_state_size)
- return NULL;
-
state = kmalloc(par->vbe_state_size, GFP_KERNEL);
if (!state)
return ERR_PTR(-ENOMEM);
--
1.7.11.1.116.g8228a23
^ permalink raw reply related
* Re: [PATCH 0/7] HID: picoLCD updates
From: Bruno Prémont @ 2012-08-15 9:42 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, linux-kernel, linux-fbdev
In-Reply-To: <alpine.LNX.2.00.1208151016221.7026@pobox.suse.cz>
Hi Jiri,
On Wed, 15 August 2012 Jiri Kosina <jkosina@suse.cz> wrote:
> On Mon, 30 Jul 2012, Bruno Prémont wrote:
> > Hi,
> >
> > This series updates picoLCD driver:
> > - split the driver functions into separate files which get included
> > depending on Kconfig selection
> > (implementation for CIR using RC_CORE will follow later)
> > - drop private framebuffer refcounting in favor of refcounting added
> > to fb_info some time ago
> > - fix various bugs issues
> > - disabled firmware version checking in probe() as it does not work
> > anymore since commit 4ea5454203d991ec85264f64f89ca8855fce69b0
> > [HID: Fix race condition between driver core and ll-driver]
>
> I have now applied the series to my 'picolcd' branch, except for 6/7,
> please see the comment I sent to it separately.
Will respin that one soon
> > Note: I still get weird behavior on quick unbind/bind sequences
> > issued via sysfs (CONFIG_SMP=n system) that are triggered by framebuffer
> > support and apparently more specifically fb_defio part of it.
> >
> > Unfortunately I'm out of ideas as to how to track down the problem which
> > shows either as SLAB corruption (detected with SLUB debugging, e.g.
>
> Would be nice to have this sorted out before the next merge window indeed,
> so that it can go in together with the rest of the changes.
>
> >
> > [ 6383.521833] ======================================> > [ 6383.530020] BUG kmalloc-64 (Not tainted): Object already free
> > [ 6383.530020] -----------------------------------------------------------------------------
> > [ 6383.530020]
> > [ 6383.530020] INFO: Slab 0xdde0ea20 objectsQ used@ fp=0xcef516e0 flags=0x40000080
> > [ 6383.530020] INFO: Object 0xcef51190 @offset@0 fp=0xcef51f50
> > [ 6383.530020]
> > [ 6383.530020] Bytes b4 cef51180: cc cc cc cc d0 12 f5 ce 5a 5a 5a 5a 5a 5a 5a 5a ........ZZZZZZZZ
> > [ 6383.530020] Object cef51190: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
> > [ 6383.530020] Object cef511a0: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
> > [ 6383.530020] Object cef511b0: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
> > [ 6383.530020] Object cef511c0: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5 kkkkkkkkkkkkkkk.
> > [ 6383.530020] Redzone cef511d0: bb bb bb bb ....
> > [ 6383.530020] Padding cef511d8: 5a 5a 5a 5a 5a 5a 5a 5a ZZZZZZZZ
> > [ 6383.530020] Pid: 1922, comm: bash Not tainted 3.5.0-jupiter-00003-g8d858b1-dirty #2
> > [ 6383.530020] Call Trace:
> > [ 6383.530020] [<c10bd3cc>] print_trailer+0x11c/0x130
> > [ 6383.530020] [<c10bd415>] object_err+0x35/0x40
> > [ 6383.530020] [<c10be809>] free_debug_processing+0x99/0x200
> > [ 6383.530020] [<c10bf77e>] __slab_free+0x2e/0x280
> > [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> > [ 6383.530020] [<c1322870>] ? __usbhid_submit_report+0xc0/0x3c0
> > [ 6383.530020] [<c10bfbda>] ? kfree+0xfa/0x110
> > [ 6383.530020] [<de932aa4>] ? picolcd_debug_out_report+0x8c4/0x8e0 [hid_picolcd]
> > [ 6383.530020] [<c10bfbda>] kfree+0xfa/0x110
> > [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> > [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> > [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> > [ 6383.530020] [<c1322284>] hid_submit_out+0xa4/0x120
> > [ 6383.530020] [<c1322908>] __usbhid_submit_report+0x158/0x3c0
> > [ 6383.530020] [<c1322c2b>] usbhid_submit_report+0x1b/0x30
> > [ 6383.530020] [<de930789>] picolcd_fb_reset+0xb9/0x180 [hid_picolcd]
> > [ 6383.530020] [<de930f1d>] picolcd_init_framebuffer+0x20d/0x2e0 [hid_picolcd]
> > [ 6383.530020] [<de92fb9c>] picolcd_probe+0x3cc/0x580 [hid_picolcd]
> > [ 6383.530020] [<c1319147>] hid_device_probe+0x67/0xf0
> > [ 6383.530020] [<c1282f97>] ? driver_sysfs_add+0x57/0x80
> > [ 6383.530020] [<c128329d>] driver_probe_device+0xbd/0x1c0
> > [ 6383.530020] [<c1318a1b>] ? hid_match_device+0x7b/0x90
> > [ 6383.530020] [<c12821e5>] driver_bind+0x75/0xd0
> > [ 6383.530020] [<c1282170>] ? driver_unbind+0x90/0x90
> > [ 6383.530020] [<c12818b7>] drv_attr_store+0x27/0x30
> > [ 6383.530020] [<c1114aec>] sysfs_write_file+0xac/0xf0
> > [ 6383.530020] [<c10c794c>] vfs_write+0x9c/0x130
> > [ 6383.530020] [<c10d4a1f>] ? sys_dup3+0x11f/0x160
> > [ 6383.530020] [<c1114a40>] ? sysfs_poll+0x90/0x90
> > [ 6383.530020] [<c10c7bbd>] sys_write+0x3d/0x70
> > [ 6383.530020] [<c13f2557>] sysenter_do_call+0x12/0x26
>
> So I am wondering whether the path this happens on is
>
> if (!test_bit(HID_OUT_RUNNING, &usbhid->iofl)) {
> usbhid_restart_out_queue(usbhid);
>
> in __usbhid_submit_report(). It would then indicate perhaps some race with
> iofl handling.
Huh, that specific test_bit hunk I can't find in __usbhid_submit_report,
is that 3.6 material?
I'm running my tests against 3.5...
The nearest I have is:
if (!test_bit(HID_OUT_RUNNING, &usbhid->iofl))
if (!irq_out_pump_restart(hid))
set_bit(HID_OUT_RUNNING, &usbhid->iofl);
> Could you please stick some printk() just before and after this
> usbhid_restart_out_queue() call, so that we know that it's this triggering
> it?
Thanks,
Bruno
^ permalink raw reply
* Re: [PATCH 0/7] HID: picoLCD updates
From: Jiri Kosina @ 2012-08-15 12:11 UTC (permalink / raw)
To: Bruno Prémont; +Cc: linux-input, linux-kernel, linux-fbdev
In-Reply-To: <20120815114236.0f7db40e@neptune.home>
On Wed, 15 Aug 2012, Bruno Prémont wrote:
> > > [ 6383.521833] ======================================> > > [ 6383.530020] BUG kmalloc-64 (Not tainted): Object already free
> > > [ 6383.530020] -----------------------------------------------------------------------------
> > > [ 6383.530020]
> > > [ 6383.530020] INFO: Slab 0xdde0ea20 objectsQ used@ fp=0xcef516e0 flags=0x40000080
> > > [ 6383.530020] INFO: Object 0xcef51190 @offset@0 fp=0xcef51f50
> > > [ 6383.530020]
> > > [ 6383.530020] Bytes b4 cef51180: cc cc cc cc d0 12 f5 ce 5a 5a 5a 5a 5a 5a 5a 5a ........ZZZZZZZZ
> > > [ 6383.530020] Object cef51190: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
> > > [ 6383.530020] Object cef511a0: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
> > > [ 6383.530020] Object cef511b0: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b kkkkkkkkkkkkkkkk
> > > [ 6383.530020] Object cef511c0: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5 kkkkkkkkkkkkkkk.
> > > [ 6383.530020] Redzone cef511d0: bb bb bb bb ....
> > > [ 6383.530020] Padding cef511d8: 5a 5a 5a 5a 5a 5a 5a 5a ZZZZZZZZ
> > > [ 6383.530020] Pid: 1922, comm: bash Not tainted 3.5.0-jupiter-00003-g8d858b1-dirty #2
> > > [ 6383.530020] Call Trace:
> > > [ 6383.530020] [<c10bd3cc>] print_trailer+0x11c/0x130
> > > [ 6383.530020] [<c10bd415>] object_err+0x35/0x40
> > > [ 6383.530020] [<c10be809>] free_debug_processing+0x99/0x200
> > > [ 6383.530020] [<c10bf77e>] __slab_free+0x2e/0x280
> > > [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> > > [ 6383.530020] [<c1322870>] ? __usbhid_submit_report+0xc0/0x3c0
> > > [ 6383.530020] [<c10bfbda>] ? kfree+0xfa/0x110
> > > [ 6383.530020] [<de932aa4>] ? picolcd_debug_out_report+0x8c4/0x8e0 [hid_picolcd]
> > > [ 6383.530020] [<c10bfbda>] kfree+0xfa/0x110
> > > [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> > > [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> > > [ 6383.530020] [<c1322284>] ? hid_submit_out+0xa4/0x120
> > > [ 6383.530020] [<c1322284>] hid_submit_out+0xa4/0x120
> > > [ 6383.530020] [<c1322908>] __usbhid_submit_report+0x158/0x3c0
> > > [ 6383.530020] [<c1322c2b>] usbhid_submit_report+0x1b/0x30
> > > [ 6383.530020] [<de930789>] picolcd_fb_reset+0xb9/0x180 [hid_picolcd]
> > > [ 6383.530020] [<de930f1d>] picolcd_init_framebuffer+0x20d/0x2e0 [hid_picolcd]
> > > [ 6383.530020] [<de92fb9c>] picolcd_probe+0x3cc/0x580 [hid_picolcd]
> > > [ 6383.530020] [<c1319147>] hid_device_probe+0x67/0xf0
> > > [ 6383.530020] [<c1282f97>] ? driver_sysfs_add+0x57/0x80
> > > [ 6383.530020] [<c128329d>] driver_probe_device+0xbd/0x1c0
> > > [ 6383.530020] [<c1318a1b>] ? hid_match_device+0x7b/0x90
> > > [ 6383.530020] [<c12821e5>] driver_bind+0x75/0xd0
> > > [ 6383.530020] [<c1282170>] ? driver_unbind+0x90/0x90
> > > [ 6383.530020] [<c12818b7>] drv_attr_store+0x27/0x30
> > > [ 6383.530020] [<c1114aec>] sysfs_write_file+0xac/0xf0
> > > [ 6383.530020] [<c10c794c>] vfs_write+0x9c/0x130
> > > [ 6383.530020] [<c10d4a1f>] ? sys_dup3+0x11f/0x160
> > > [ 6383.530020] [<c1114a40>] ? sysfs_poll+0x90/0x90
> > > [ 6383.530020] [<c10c7bbd>] sys_write+0x3d/0x70
> > > [ 6383.530020] [<c13f2557>] sysenter_do_call+0x12/0x26
> >
> > So I am wondering whether the path this happens on is
> >
> > if (!test_bit(HID_OUT_RUNNING, &usbhid->iofl)) {
> > usbhid_restart_out_queue(usbhid);
> >
> > in __usbhid_submit_report(). It would then indicate perhaps some race with
> > iofl handling.
>
> Huh, that specific test_bit hunk I can't find in __usbhid_submit_report,
> is that 3.6 material?
> I'm running my tests against 3.5...
I see. Alan Stern has fixed a huge pile of things in this area in 3.6-rc1.
I have expected all of those to actually be on theoretical problems not
ever having happened in the wild, but it might be that you are actually
chasing on of those.
Could you please retest with latest Linus' tree (or at least eb055fd0560b)
to see whether this hasn't actually been fixed already by Alan's series?
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 0/7] HID: picoLCD updates
From: Bruno Prémont @ 2012-08-15 15:16 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, linux-kernel, linux-fbdev
In-Reply-To: <alpine.LNX.2.00.1208151409500.7026@pobox.suse.cz>
Hi Jiri,
On Wed, 15 August 2012 Jiri Kosina <jkosina@suse.cz> wrote:
> I see. Alan Stern has fixed a huge pile of things in this area in 3.6-rc1.
> I have expected all of those to actually be on theoretical problems not
> ever having happened in the wild, but it might be that you are actually
> chasing on of those.
>
> Could you please retest with latest Linus' tree (or at least eb055fd0560b)
> to see whether this hasn't actually been fixed already by Alan's series?
I've started trying that out, it seems Alan's work improved things.
For the first few attempts I have not seen SLAB corruptions, though after
a few rounds I hit accumulation of the following messages:
[ 297.174828] hid-picolcd 0003:04D8:C002.0003: output queue full
[ 297.181098] hid-picolcd 0003:04D8:C002.0003: output queue full
[ 297.187820] hid-picolcd 0003:04D8:C002.0003: output queue full
[ 297.194087] hid-picolcd 0003:04D8:C002.0003: output queue full
with sporadically in between:
[ 292.668019] hid-picolcd 0003:04D8:C002.0003: usb_submit_urb(out) failed: -1
At first glance I think the queue filling up and never draining is caused
by hid_hw_stop() stalling the queue and the time between both being just too
short.
Maybe me (un)binding on hid-picolcd driver level is the major cause of this
and (un)binding at lower usb level would kill the queue thus preventing
it just growing until full...
Is there a proper way to do rate-limiting or throttling when submitting
reports (which are the urbs deeper in the stack) and catch -ENODEV early
while remove() is delayed for locking reasons?
At least until 3.5 submitting reports provides no return value for driver
which could convey errors like -ENODEV, -EBUSY.
After a few bind-unbind iterations I have also hit a race in hid-picolcd
with regard to fbdefio.
That looks like chicken-egg release sequencing between hid-picolcd and
fb_info subdev -- though I'm wondering how that happens as the only framebuffer
user I know of - fbcon - should have stopped using it after
unregister_framebuffer() returned and released the last reference!
But that part can be improved rather easily with a new spinlock synchronizing
interoperation between hid side and framebuffer side.
Thanks,
Bruno
^ permalink raw reply
* Re: [PATCHv2 0/8] *** ARM: Update arch-vt8500 to Devicetree ***
From: Stephen Warren @ 2012-08-15 19:23 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1344477300-25251-1-git-send-email-linux@prisktech.co.nz>
On 08/08/2012 07:54 PM, Tony Prisk wrote:
> This patchset updates arch-vt8500 to devicetree and removes all the old-style
> code. Support for WM8650 has also been added.
Sorry for taking such a long time to re-review this.
I scanned the series looking for just the changes to the issues I raised
on v1, and I do see almost everything has been fixed up OK, except for
the one issue I just commented on. So, aside from that, this series is
fine by me. I'll defer to others about whether it's necessary to fix the
display node-name-vs.-phandle issue I pointed out, or defer that until
later.
^ permalink raw reply
* Re: [PATCH 0/7] HID: picoLCD updates
From: Jiri Kosina @ 2012-08-15 21:32 UTC (permalink / raw)
To: Bruno Prémont; +Cc: linux-input, linux-kernel, linux-fbdev
In-Reply-To: <20120815171635.2564a4a8@neptune.home>
On Wed, 15 Aug 2012, Bruno Prémont wrote:
> > I see. Alan Stern has fixed a huge pile of things in this area in 3.6-rc1.
> > I have expected all of those to actually be on theoretical problems not
> > ever having happened in the wild, but it might be that you are actually
> > chasing on of those.
> >
> > Could you please retest with latest Linus' tree (or at least eb055fd0560b)
> > to see whether this hasn't actually been fixed already by Alan's series?
>
> I've started trying that out, it seems Alan's work improved things.
>
> For the first few attempts I have not seen SLAB corruptions, though after
> a few rounds I hit accumulation of the following messages:
> [ 297.174828] hid-picolcd 0003:04D8:C002.0003: output queue full
> [ 297.181098] hid-picolcd 0003:04D8:C002.0003: output queue full
> [ 297.187820] hid-picolcd 0003:04D8:C002.0003: output queue full
> [ 297.194087] hid-picolcd 0003:04D8:C002.0003: output queue full
>
> with sporadically in between:
> [ 292.668019] hid-picolcd 0003:04D8:C002.0003: usb_submit_urb(out) failed: -1
>
> At first glance I think the queue filling up and never draining is caused
> by hid_hw_stop() stalling the queue and the time between both being just too
> short.
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).
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* [PATCH v4 0/3] Runtime Interpreted Power Sequences
From: Alexandre Courbot @ 2012-08-16 6:08 UTC (permalink / raw)
To: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely,
Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse,
Arnd Bergmann
Cc: Leela Krishna Amudala, linux-tegra, linux-kernel, linux-fbdev,
devicetree-discuss, linux-doc, Alexandre Courbot
Overdue revision of this new feature, some changes required additional thought
and rework.
The most important change is in the way power sequences are expressed in the
device tree. In order to avoid having to specify #address-cells, #size-cells and
reg properties, the @ notation in the step names is dropped, and instead a
fixed, sequential naming is adopted. The type of the resource used by a step is
decided by the presence of some recognized properties:
power-on-sequence {
step0 {
regulator = "power";
enable;
};
step1 {
delay = <10000>;
};
step2 {
pwm = "backlight";
enable;
};
...
To me this looks safe, clear and close to the platform data representation, but
needs approval from DT experts.
Resources are still referenced by name instead of having their phandles defined
directly inside the sequences, as previous discussion came to the conclusion
that doing so would require controversial changes to the regulator and PWM
frameworks, and that having the resources declared at the device level was
making sense logically speaking.
Other changes/fixes since last revision:
* Move to drivers/power/ (hope this is ok with the maintainers?)
* Use microseconds for delay
* Use devm for PWM resources and remove cleanup function as all resources are
devm-managed
* Remove "-gpio" suffix for GPIO reference in the driver
* Remove params structure
* Make power_seq structure private
* Number of steps in a sequence is explicitly stated instead of resorting to a
"stop" sequence step
* Delays are a step instead of being a step parameter
* Use flexible member arrays to limit number of memory allocations
* Add documentation to DT bindings
There was a lot of feedback on the previous version (thanks!) so if I forgot
to address some important point, please bring it to my attention again.
Alexandre Courbot (3):
Runtime Interpreted Power Sequences
pwm_backlight: use power sequences
tegra: add pwm backlight device tree nodes
.../devicetree/bindings/power_seq/power_seq.txt | 101 +++++
.../bindings/video/backlight/pwm-backlight.txt | 62 ++-
Documentation/power/power_seq.txt | 129 +++++++
arch/arm/boot/dts/tegra20-ventana.dts | 58 +++
arch/arm/boot/dts/tegra20.dtsi | 2 +-
drivers/power/Kconfig | 3 +
drivers/power/Makefile | 1 +
drivers/power/power_seq.c | 420 +++++++++++++++++++++
drivers/video/backlight/Kconfig | 1 +
drivers/video/backlight/pwm_bl.c | 192 +++++++---
include/linux/power_seq.h | 142 +++++++
include/linux/pwm_backlight.h | 16 +-
12 files changed, 1071 insertions(+), 56 deletions(-)
create mode 100644 Documentation/devicetree/bindings/power_seq/power_seq.txt
create mode 100644 Documentation/power/power_seq.txt
create mode 100644 drivers/power/power_seq.c
create mode 100644 include/linux/power_seq.h
--
1.7.11.4
^ 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