| Message ID | d45b88bfd192a783bbc14cfcc6e75ba85298c4f4.1463726043.git.jsarha@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On 20/05/16 09:35, Jyri Sarha wrote: > Work-a-round for errata i734 in DSS dispc > - LCD1 Gamma Correction Is Not Working When GFX Pipe Is Disabled > > For gamma tables to work on LCD1 the GFX plane has to be used at least > once after DSS HW has come out of reset. The work-a-round sets up a > minimal LCD setup with GFX plane and waits for one vertical sync irq > before disabling the setup and continuing with the context > restore. The physical outputs are gated during the operation. This > work-a-round requires that gamma table's LOADMODE is set to 0x2 in > DISPC_CONTROL1 register. > > For details see: > OMAP543x Multimedia Device Silicon Revision 2.0 Silicon Errata > Literature Number: SWPZ037E > Or some other relevant errata document for the DSS IP version. > > Signed-off-by: Jyri Sarha <jsarha@ti.com> > --- > drivers/gpu/drm/omapdrm/dss/dispc.c | 166 +++++++++++++++++++++++++++++ > drivers/gpu/drm/omapdrm/dss/dss_features.c | 2 + > drivers/gpu/drm/omapdrm/dss/dss_features.h | 1 + > 3 files changed, 169 insertions(+) > > diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c > index 87dde05..3608cf2 100644 > --- a/drivers/gpu/drm/omapdrm/dss/dispc.c > +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c > @@ -4201,6 +4201,164 @@ void dispc_free_irq(void *dev_id) > } > EXPORT_SYMBOL(dispc_free_irq); > > +/* > + * Work-a-round for errata i734 in DSS dispc > + * - LCD1 Gamma Correction Is Not Working When GFX Pipe Is Disabled > + * > + * For gamma tables to work on LCD1 the GFX plane has to be used at > + * least once after DSS HW has come out of reset. The work-a-round > + * sets up a minimal LCD setup with GFX plane and waits for one > + * vertical sync irq before disabling the setup and continuing with > + * the context restore. The physical outputs are gated during the > + * operation. This work-a-round requires that gamma table's LOADMODE > + * is set to 0x2 in DISPC_CONTROL1 register. > + * > + * For details see: > + * OMAP543x Multimedia Device Silicon Revision 2.0 Silicon Errata > + * Literature Number: SWPZ037E > + * Or some other relevant errata document for the DSS IP version. > + */ > + > +static struct dispc_errata_i734_data { > + struct omap_video_timings timings; > + struct omap_overlay_info ovli; > + struct omap_overlay_manager_info mgri; > + struct dss_lcd_mgr_config lcd_conf; > + struct i734_buf { > + dma_addr_t paddr; > + void *vaddr; > + } buf; It might be good to separate the setup data to a const struct, and the buffer data to a mutable one. You need to set the ovl info base address, but you could just create a local copy of the const ovl_info when doing the setup. > +} i734 = { > + .timings = { > + .x_res = 8, .y_res = 1, > + .pixelclock = 16000000, > + .hsw = 8, .hfp = 4, .hbp = 4, > + .vsw = 1, .vfp = 1, .vbp = 1, > + .vsync_level = OMAPDSS_SIG_ACTIVE_LOW, > + .hsync_level = OMAPDSS_SIG_ACTIVE_LOW, > + .interlace = false, > + .data_pclk_edge = OMAPDSS_DRIVE_SIG_RISING_EDGE, > + .de_level = OMAPDSS_SIG_ACTIVE_HIGH, > + .sync_pclk_edge = OMAPDSS_DRIVE_SIG_RISING_EDGE, > + .double_pixel = false, > + }, > + .ovli = { > + .screen_width = 1, > + .width = 1, .height = 1, > + .color_mode = OMAP_DSS_COLOR_RGB24U, > + .rotation = OMAP_DSS_ROT_0, > + .rotation_type = OMAP_DSS_ROT_DMA, > + .mirror = 0, > + .pos_x = 0, .pos_y = 0, > + .out_width = 0, .out_height = 0, > + .global_alpha = 0xff, > + .pre_mult_alpha = 0, > + .zorder = 0, > + }, > + .mgri = { > + .default_color = 0, > + .trans_enabled = false, > + .partial_alpha_enabled = false, > + .cpr_enable = false, > + }, > + .lcd_conf = { > + .io_pad_mode = DSS_IO_PAD_MODE_BYPASS, > + .stallmode = false, > + .fifohandcheck = false, > + .clock_info = { > + .lck_div = 1, > + .pck_div = 2, > + }, > + .video_port_width = 24, > + .lcden_sig_polarity = 0, > + }, > +}; > + > +static int dispc_errata_i734_wa_init(void) > +{ > + size_t buff_size = i734.ovli.width * i734.ovli.height * "buf_size" would match the other names =). Usually I try to do only simple initializations when declaring the locals. This is a bit on the complex side, and also there's a check below that can make all this init calculation not needed. > + color_mode_to_bpp(i734.ovli.color_mode) / 8; > + > + if (!dss_has_feature(FEAT_GAMMA_I734_BUG)) > + return 0; > + > + i734.buf.vaddr = dma_alloc_writecombine(&dispc.pdev->dev, buff_size, > + &i734.buf.paddr, GFP_KERNEL); > + if (WARN_ON(IS_ERR(i734.buf.vaddr))) > + return PTR_ERR(i734.buf.vaddr); I think a plain dev_err() would be enough here, not WARN. > + > + i734.ovli.paddr = i734.buf.paddr; > + > + return 0; > +} > + > +static void dispc_errata_i734_wa_fini(void) > +{ > + size_t buff_size = i734.ovli.width * i734.ovli.height * > + color_mode_to_bpp(i734.ovli.color_mode) / 8; > + > + if (!dss_has_feature(FEAT_GAMMA_I734_BUG)) > + return; > + > + dma_free_writecombine(&dispc.pdev->dev, buff_size, i734.buf.vaddr, > + i734.buf.paddr); > +} > + > +static void dispc_errata_i734_wa(void) > +{ > + u32 vsync_irq = dispc_mgr_get_vsync_irq(OMAP_DSS_CHANNEL_LCD); > + u32 framedone_irq = dispc_mgr_get_framedone_irq(OMAP_DSS_CHANNEL_LCD); > + u32 gatestate = REG_GET(DISPC_CONTROL, 8, 4); Here too, too complex initializations. Especially doing register reads/writes should be something done in the "real" code. The bits you read might not even be valid on some platforms which don't have the i734. > + unsigned int count; > + > + if (!dss_has_feature(FEAT_GAMMA_I734_BUG)) > + return; > + > + /* Gate all LCD1 outputs */ > + REG_FLD_MOD(DISPC_CONTROL, 0, 8, 4); This ungates the outputs. Or would, if it used the right register, but it's using wrong register... > + /* Setup and enable GFX plane */ > + dispc_ovl_set_channel_out(OMAP_DSS_GFX, OMAP_DSS_CHANNEL_LCD); > + dispc_ovl_setup(OMAP_DSS_GFX, &i734.ovli, false, &i734.timings, false); > + dispc_ovl_enable(OMAP_DSS_GFX, true); > + > + /* Set up and enable display manager for LCD1 */ > + dispc_mgr_setup(OMAP_DSS_CHANNEL_LCD, &i734.mgri); > + dispc_calc_clock_rates(dss_get_dispc_clk_rate(), > + &i734.lcd_conf.clock_info); > + dispc_mgr_set_lcd_config(OMAP_DSS_CHANNEL_LCD, &i734.lcd_conf); > + dispc_mgr_set_timings(OMAP_DSS_CHANNEL_LCD, &i734.timings); > + dispc_mgr_enable(OMAP_DSS_CHANNEL_LCD, true); When you enable the LCD channel, it starts sending the stream. So... > + dispc_clear_irqstatus(vsync_irq | framedone_irq); ...clearing the irqstatus should be done before enabling the output. > + /* Busy wait for vsync (we can't fiddle with irq handlers in resume) */ > + count = 0; > + while (!(dispc_read_irqstatus() & vsync_irq)) > + if (count++ > 10000) { > + dev_err(&dispc.pdev->dev, "%s: vsync timeout\n", > + __func__); > + break; > + } > + > + /* Shut down the WA setup */ > + dispc_mgr_enable(OMAP_DSS_CHANNEL_LCD, false); You can disable the LCD right after enabling it. What happens is that when you set the enable bit, DISPC starts the output. When you clear the enable bit, DISPC will stop at the end of the next frame. In other words, you don't need to wait for vsync at all, just framedone. That is, presuming it's enough for the WA to work. I think it is, as it does send a single frame. > + dispc_ovl_enable(OMAP_DSS_GFX, false); > + count = 0; > + while (!(dispc_read_irqstatus() & framedone_irq)) > + if (count++ > 10000) { > + dev_err(&dispc.pdev->dev, "%s: framedone timeout\n", > + __func__); > + break; > + } > + > + /* Clear all irq bits before continuing */ > + dispc_clear_irqstatus(0xffffffff); > + > + /* Restore the original state to LCD1 output gates */ > + REG_FLD_MOD(DISPC_CONTROL, gatestate, 8, 4); Still wrong register =). > +} > + > /* DISPC HW IP initialisation */ > static int dispc_bind(struct device *dev, struct device *master, void *data) > { > @@ -4214,6 +4372,10 @@ static int dispc_bind(struct device *dev, struct device *master, void *data) > > spin_lock_init(&dispc.control_lock); > > + r = dispc_errata_i734_wa_init(); > + if (r) > + return r; > + > r = dispc_init_features(dispc.pdev); > if (r) > return r; > @@ -4282,6 +4444,8 @@ static void dispc_unbind(struct device *dev, struct device *master, > void *data) > { > pm_runtime_disable(dev); > + > + dispc_errata_i734_wa_fini(); > } > > static const struct component_ops dispc_component_ops = { > @@ -4324,6 +4488,8 @@ static int dispc_runtime_resume(struct device *dev) > if (REG_GET(DISPC_CONFIG, 2, 1) != OMAP_DSS_LOAD_FRAME_ONLY) { > _omap_dispc_initial_config(); > > + dispc_errata_i734_wa(); > + > dispc_restore_context(); > > dispc_restore_gamma_tables(); > diff --git a/drivers/gpu/drm/omapdrm/dss/dss_features.c b/drivers/gpu/drm/omapdrm/dss/dss_features.c > index f77b1c5..3127bd6 100644 > --- a/drivers/gpu/drm/omapdrm/dss/dss_features.c > +++ b/drivers/gpu/drm/omapdrm/dss/dss_features.c > @@ -594,6 +594,7 @@ static const enum dss_feat_id omap4_dss_feat_list[] = { > FEAT_FIFO_MERGE, > FEAT_BURST_2D, > FEAT_GAMMA_TABLE, > + FEAT_GAMMA_I734_BUG, This, too, should be in dispc features. Tomi
On 05/20/16 16:21, Tomi Valkeinen wrote: >> > + >> > +static int dispc_errata_i734_wa_init(void) >> > +{ >> > + size_t buff_size = i734.ovli.width * i734.ovli.height * > "buf_size" would match the other names =). > > Usually I try to do only simple initializations when declaring the > locals. This is a bit on the complex side, and also there's a check > below that can make all this init calculation not needed. > Compiler optimizations should delay the initialization to happen only if it is needed. I usually try to keep the functions as compact as possible so that they fit on the screen at once. But I think it is better to move the size also inside the struct i734_buf so it we only need to calculate the size once and functions become even smaller :). ... >> > + >> > +static void dispc_errata_i734_wa(void) >> > +{ >> > + u32 vsync_irq = dispc_mgr_get_vsync_irq(OMAP_DSS_CHANNEL_LCD); >> > + u32 framedone_irq = dispc_mgr_get_framedone_irq(OMAP_DSS_CHANNEL_LCD); >> > + u32 gatestate = REG_GET(DISPC_CONTROL, 8, 4); > Here too, too complex initializations. Especially doing register > reads/writes should be something done in the "real" code. The bits you > read might not even be valid on some platforms which don't have the i734. > You are right about the register read. Compiler should not optimize the register access. But - but with your permission - I'll keep the IRQ mask initializations as they are quite constant in nature anyway. BR, Jyri
On 23/05/16 12:50, Jyri Sarha wrote: > On 05/20/16 16:21, Tomi Valkeinen wrote: >>>> + >>>> +static int dispc_errata_i734_wa_init(void) >>>> +{ >>>> + size_t buff_size = i734.ovli.width * i734.ovli.height * >> "buf_size" would match the other names =). >> >> Usually I try to do only simple initializations when declaring the >> locals. This is a bit on the complex side, and also there's a check >> below that can make all this init calculation not needed. >> > > Compiler optimizations should delay the initialization to happen only if > it is needed. That is true. My worry is more on the functional side: if you do, say, calculations there and, e.g. do a division, it's easy to get division by zero if all the variables are not actually valid. Or if you do get_foo(channel), and get_foo() only works for certain channels, again you could hit a crash. Those kind of things happen easily when you have code like: int foo = get_foo(channel); if (channel != XYZ) return; >>>> + >>>> +static void dispc_errata_i734_wa(void) >>>> +{ >>>> + u32 vsync_irq = dispc_mgr_get_vsync_irq(OMAP_DSS_CHANNEL_LCD); >>>> + u32 framedone_irq = dispc_mgr_get_framedone_irq(OMAP_DSS_CHANNEL_LCD); >>>> + u32 gatestate = REG_GET(DISPC_CONTROL, 8, 4); >> Here too, too complex initializations. Especially doing register >> reads/writes should be something done in the "real" code. The bits you >> read might not even be valid on some platforms which don't have the i734. >> > > You are right about the register read. Compiler should not optimize the > register access. But - but with your permission - I'll keep the IRQ mask > initializations as they are quite constant in nature anyway. Yes, those should be fine. Presuming all DSS versions have CHANNEL_LCD, which happens to be the case =). Tomi
diff --git a/drivers/gpu/drm/omapdrm/dss/dispc.c b/drivers/gpu/drm/omapdrm/dss/dispc.c index 87dde05..3608cf2 100644 --- a/drivers/gpu/drm/omapdrm/dss/dispc.c +++ b/drivers/gpu/drm/omapdrm/dss/dispc.c @@ -4201,6 +4201,164 @@ void dispc_free_irq(void *dev_id) } EXPORT_SYMBOL(dispc_free_irq); +/* + * Work-a-round for errata i734 in DSS dispc + * - LCD1 Gamma Correction Is Not Working When GFX Pipe Is Disabled + * + * For gamma tables to work on LCD1 the GFX plane has to be used at + * least once after DSS HW has come out of reset. The work-a-round + * sets up a minimal LCD setup with GFX plane and waits for one + * vertical sync irq before disabling the setup and continuing with + * the context restore. The physical outputs are gated during the + * operation. This work-a-round requires that gamma table's LOADMODE + * is set to 0x2 in DISPC_CONTROL1 register. + * + * For details see: + * OMAP543x Multimedia Device Silicon Revision 2.0 Silicon Errata + * Literature Number: SWPZ037E + * Or some other relevant errata document for the DSS IP version. + */ + +static struct dispc_errata_i734_data { + struct omap_video_timings timings; + struct omap_overlay_info ovli; + struct omap_overlay_manager_info mgri; + struct dss_lcd_mgr_config lcd_conf; + struct i734_buf { + dma_addr_t paddr; + void *vaddr; + } buf; +} i734 = { + .timings = { + .x_res = 8, .y_res = 1, + .pixelclock = 16000000, + .hsw = 8, .hfp = 4, .hbp = 4, + .vsw = 1, .vfp = 1, .vbp = 1, + .vsync_level = OMAPDSS_SIG_ACTIVE_LOW, + .hsync_level = OMAPDSS_SIG_ACTIVE_LOW, + .interlace = false, + .data_pclk_edge = OMAPDSS_DRIVE_SIG_RISING_EDGE, + .de_level = OMAPDSS_SIG_ACTIVE_HIGH, + .sync_pclk_edge = OMAPDSS_DRIVE_SIG_RISING_EDGE, + .double_pixel = false, + }, + .ovli = { + .screen_width = 1, + .width = 1, .height = 1, + .color_mode = OMAP_DSS_COLOR_RGB24U, + .rotation = OMAP_DSS_ROT_0, + .rotation_type = OMAP_DSS_ROT_DMA, + .mirror = 0, + .pos_x = 0, .pos_y = 0, + .out_width = 0, .out_height = 0, + .global_alpha = 0xff, + .pre_mult_alpha = 0, + .zorder = 0, + }, + .mgri = { + .default_color = 0, + .trans_enabled = false, + .partial_alpha_enabled = false, + .cpr_enable = false, + }, + .lcd_conf = { + .io_pad_mode = DSS_IO_PAD_MODE_BYPASS, + .stallmode = false, + .fifohandcheck = false, + .clock_info = { + .lck_div = 1, + .pck_div = 2, + }, + .video_port_width = 24, + .lcden_sig_polarity = 0, + }, +}; + +static int dispc_errata_i734_wa_init(void) +{ + size_t buff_size = i734.ovli.width * i734.ovli.height * + color_mode_to_bpp(i734.ovli.color_mode) / 8; + + if (!dss_has_feature(FEAT_GAMMA_I734_BUG)) + return 0; + + i734.buf.vaddr = dma_alloc_writecombine(&dispc.pdev->dev, buff_size, + &i734.buf.paddr, GFP_KERNEL); + if (WARN_ON(IS_ERR(i734.buf.vaddr))) + return PTR_ERR(i734.buf.vaddr); + + i734.ovli.paddr = i734.buf.paddr; + + return 0; +} + +static void dispc_errata_i734_wa_fini(void) +{ + size_t buff_size = i734.ovli.width * i734.ovli.height * + color_mode_to_bpp(i734.ovli.color_mode) / 8; + + if (!dss_has_feature(FEAT_GAMMA_I734_BUG)) + return; + + dma_free_writecombine(&dispc.pdev->dev, buff_size, i734.buf.vaddr, + i734.buf.paddr); +} + +static void dispc_errata_i734_wa(void) +{ + u32 vsync_irq = dispc_mgr_get_vsync_irq(OMAP_DSS_CHANNEL_LCD); + u32 framedone_irq = dispc_mgr_get_framedone_irq(OMAP_DSS_CHANNEL_LCD); + u32 gatestate = REG_GET(DISPC_CONTROL, 8, 4); + unsigned int count; + + if (!dss_has_feature(FEAT_GAMMA_I734_BUG)) + return; + + /* Gate all LCD1 outputs */ + REG_FLD_MOD(DISPC_CONTROL, 0, 8, 4); + + /* Setup and enable GFX plane */ + dispc_ovl_set_channel_out(OMAP_DSS_GFX, OMAP_DSS_CHANNEL_LCD); + dispc_ovl_setup(OMAP_DSS_GFX, &i734.ovli, false, &i734.timings, false); + dispc_ovl_enable(OMAP_DSS_GFX, true); + + /* Set up and enable display manager for LCD1 */ + dispc_mgr_setup(OMAP_DSS_CHANNEL_LCD, &i734.mgri); + dispc_calc_clock_rates(dss_get_dispc_clk_rate(), + &i734.lcd_conf.clock_info); + dispc_mgr_set_lcd_config(OMAP_DSS_CHANNEL_LCD, &i734.lcd_conf); + dispc_mgr_set_timings(OMAP_DSS_CHANNEL_LCD, &i734.timings); + dispc_mgr_enable(OMAP_DSS_CHANNEL_LCD, true); + + dispc_clear_irqstatus(vsync_irq | framedone_irq); + + /* Busy wait for vsync (we can't fiddle with irq handlers in resume) */ + count = 0; + while (!(dispc_read_irqstatus() & vsync_irq)) + if (count++ > 10000) { + dev_err(&dispc.pdev->dev, "%s: vsync timeout\n", + __func__); + break; + } + + /* Shut down the WA setup */ + dispc_mgr_enable(OMAP_DSS_CHANNEL_LCD, false); + dispc_ovl_enable(OMAP_DSS_GFX, false); + count = 0; + while (!(dispc_read_irqstatus() & framedone_irq)) + if (count++ > 10000) { + dev_err(&dispc.pdev->dev, "%s: framedone timeout\n", + __func__); + break; + } + + /* Clear all irq bits before continuing */ + dispc_clear_irqstatus(0xffffffff); + + /* Restore the original state to LCD1 output gates */ + REG_FLD_MOD(DISPC_CONTROL, gatestate, 8, 4); +} + /* DISPC HW IP initialisation */ static int dispc_bind(struct device *dev, struct device *master, void *data) { @@ -4214,6 +4372,10 @@ static int dispc_bind(struct device *dev, struct device *master, void *data) spin_lock_init(&dispc.control_lock); + r = dispc_errata_i734_wa_init(); + if (r) + return r; + r = dispc_init_features(dispc.pdev); if (r) return r; @@ -4282,6 +4444,8 @@ static void dispc_unbind(struct device *dev, struct device *master, void *data) { pm_runtime_disable(dev); + + dispc_errata_i734_wa_fini(); } static const struct component_ops dispc_component_ops = { @@ -4324,6 +4488,8 @@ static int dispc_runtime_resume(struct device *dev) if (REG_GET(DISPC_CONFIG, 2, 1) != OMAP_DSS_LOAD_FRAME_ONLY) { _omap_dispc_initial_config(); + dispc_errata_i734_wa(); + dispc_restore_context(); dispc_restore_gamma_tables(); diff --git a/drivers/gpu/drm/omapdrm/dss/dss_features.c b/drivers/gpu/drm/omapdrm/dss/dss_features.c index f77b1c5..3127bd6 100644 --- a/drivers/gpu/drm/omapdrm/dss/dss_features.c +++ b/drivers/gpu/drm/omapdrm/dss/dss_features.c @@ -594,6 +594,7 @@ static const enum dss_feat_id omap4_dss_feat_list[] = { FEAT_FIFO_MERGE, FEAT_BURST_2D, FEAT_GAMMA_TABLE, + FEAT_GAMMA_I734_BUG, }; static const enum dss_feat_id omap5_dss_feat_list[] = { @@ -617,6 +618,7 @@ static const enum dss_feat_id omap5_dss_feat_list[] = { FEAT_DSI_PHY_DCC, FEAT_MFLAG, FEAT_GAMMA_TABLE, + FEAT_GAMMA_I734_BUG, }; /* OMAP2 DSS Features */ diff --git a/drivers/gpu/drm/omapdrm/dss/dss_features.h b/drivers/gpu/drm/omapdrm/dss/dss_features.h index d4d14db..0f74083 100644 --- a/drivers/gpu/drm/omapdrm/dss/dss_features.h +++ b/drivers/gpu/drm/omapdrm/dss/dss_features.h @@ -63,6 +63,7 @@ enum dss_feat_id { FEAT_DSI_PHY_DCC, FEAT_MFLAG, FEAT_GAMMA_TABLE, + FEAT_GAMMA_I734_BUG, }; /* DSS register field id */
Work-a-round for errata i734 in DSS dispc - LCD1 Gamma Correction Is Not Working When GFX Pipe Is Disabled For gamma tables to work on LCD1 the GFX plane has to be used at least once after DSS HW has come out of reset. The work-a-round sets up a minimal LCD setup with GFX plane and waits for one vertical sync irq before disabling the setup and continuing with the context restore. The physical outputs are gated during the operation. This work-a-round requires that gamma table's LOADMODE is set to 0x2 in DISPC_CONTROL1 register. For details see: OMAP543x Multimedia Device Silicon Revision 2.0 Silicon Errata Literature Number: SWPZ037E Or some other relevant errata document for the DSS IP version. Signed-off-by: Jyri Sarha <jsarha@ti.com> --- drivers/gpu/drm/omapdrm/dss/dispc.c | 166 +++++++++++++++++++++++++++++ drivers/gpu/drm/omapdrm/dss/dss_features.c | 2 + drivers/gpu/drm/omapdrm/dss/dss_features.h | 1 + 3 files changed, 169 insertions(+)