The git branch bellow is updated.
Changes since v2: - Add: "drm/tilcdc: Fix load mode bit-field setting in tilcdc_crtc_enable()" - Drop: "drm/tilcdc: Free palette dma memory in tilcdc_crtc_destroy()" - Add: "drm/tilcdc: Add timeout wait for palette loading to complete" - Add: "drm/tilcdc: Call reset() before loading the palette" - Add: "drm/tilcdc: Use complete_all() to indicate completed palette loading" - Add "drm/tilcdc: Enable frame done irq and functionality for LCDC rev 1" - Bartosz: Please test if this works! The symptom for not working is "timeout waiting for framedone" message when screen is blanked.
Changes since first version of the series:
- Move tilcdc_regs.h changes from "drm/tilcdc: Enable palette loading for revision 2 LCDC too" to "drm/tilcdc: Add tilcdc_write_mask() to tilcdc_regs.h"
These patches are inspired by this series form Bartosz Golaszewski: https://www.spinics.net/lists/arm-kernel/msg539629.html
The patches are based on drm-next plus the earlier patches that I plan to send in a pull request for 4.10. The base + these patches are pushed here:
https://github.com/jsarha/linux drm-next-tilcdc-for-4.10-wip
Bartosz, please test if this branch works for rev1 LCDC, with your dts file!
Bartosz Golaszewski (1): drm/tilcdc: implement palette loading for rev1
Jyri Sarha (10): drm/tilcdc: Enable sync lost error and recovery handling for rev 1 LCDC drm/tilcdc: Fix tilcdc_crtc_create() return value handling drm/tilcdc: Add tilcdc_write_mask() to tilcdc_regs.h drm/tilcdc: Fix load mode bit-field setting in tilcdc_crtc_enable() drm/tilcdc: Enable palette loading for revision 2 LCDC too drm/tilcdc: Add timeout wait for palette loading to complete drm/tilcdc: Call reset() before loading the palette drm/tilcdc: Use complete_all() to indicate completed palette loading drm/tilcdc: Load palette at the end of mode_set_nofb() drm/tilcdc: Enable frame done irq and functionality for LCDC rev 1
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 163 +++++++++++++++++++++++++++-------- drivers/gpu/drm/tilcdc/tilcdc_drv.c | 23 +++-- drivers/gpu/drm/tilcdc/tilcdc_drv.h | 3 +- drivers/gpu/drm/tilcdc/tilcdc_regs.h | 15 ++++ 4 files changed, 159 insertions(+), 45 deletions(-)
Revision 1 LCDC support also sync lost errors and can benefit from sync lost recovery routine.
Signed-off-by: Jyri Sarha jsarha@ti.com --- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 33 +++++++++++++++++---------------- drivers/gpu/drm/tilcdc/tilcdc_regs.h | 1 + 2 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index c787349..5260eb2 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -113,6 +113,7 @@ static void tilcdc_crtc_enable_irqs(struct drm_device *dev)
if (priv->rev == 1) { tilcdc_set(dev, LCDC_RASTER_CTRL_REG, + LCDC_V1_SYNC_LOST_ENA | LCDC_V1_UNDERFLOW_INT_ENA); tilcdc_set(dev, LCDC_DMA_CTRL_REG, LCDC_V1_END_OF_FRAME_INT_ENA); @@ -130,7 +131,7 @@ static void tilcdc_crtc_disable_irqs(struct drm_device *dev)
/* disable irqs that we might have enabled: */ if (priv->rev == 1) { - tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, + tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_V1_SYNC_LOST_ENA | LCDC_V1_UNDERFLOW_INT_ENA | LCDC_V1_PL_INT_ENA); tilcdc_clear(dev, LCDC_DMA_CTRL_REG, LCDC_V1_END_OF_FRAME_INT_ENA); @@ -845,6 +846,21 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc) dev_err_ratelimited(dev->dev, "%s(0x%08x): FIFO underflow", __func__, stat);
+ if (stat & LCDC_SYNC_LOST) { + dev_err_ratelimited(dev->dev, "%s(0x%08x): Sync lost", + __func__, stat); + tilcdc_crtc->frame_intact = false; + if (tilcdc_crtc->sync_lost_count++ > + SYNC_LOST_COUNT_LIMIT) { + dev_err(dev->dev, "%s(0x%08x): Sync lost flood detected, recovering", __func__, stat); + queue_work(system_wq, + &tilcdc_crtc->recover_work); + tilcdc_write(dev, LCDC_INT_ENABLE_CLR_REG, + LCDC_SYNC_LOST); + tilcdc_crtc->sync_lost_count = 0; + } + } + /* For revision 2 only */ if (priv->rev == 2) { if (stat & LCDC_FRAME_DONE) { @@ -852,21 +868,6 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc) wake_up(&tilcdc_crtc->frame_done_wq); }
- if (stat & LCDC_SYNC_LOST) { - dev_err_ratelimited(dev->dev, "%s(0x%08x): Sync lost", - __func__, stat); - tilcdc_crtc->frame_intact = false; - if (tilcdc_crtc->sync_lost_count++ > - SYNC_LOST_COUNT_LIMIT) { - dev_err(dev->dev, "%s(0x%08x): Sync lost flood detected, recovering", __func__, stat); - queue_work(system_wq, - &tilcdc_crtc->recover_work); - tilcdc_write(dev, LCDC_INT_ENABLE_CLR_REG, - LCDC_SYNC_LOST); - tilcdc_crtc->sync_lost_count = 0; - } - } - /* Indicate to LCDC that the interrupt service routine has * completed, see 13.3.6.1.6 in AM335x TRM. */ diff --git a/drivers/gpu/drm/tilcdc/tilcdc_regs.h b/drivers/gpu/drm/tilcdc/tilcdc_regs.h index f57c0d6..beb8c21 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_regs.h +++ b/drivers/gpu/drm/tilcdc/tilcdc_regs.h @@ -61,6 +61,7 @@ #define LCDC_V2_UNDERFLOW_INT_ENA BIT(5) #define LCDC_V1_PL_INT_ENA BIT(4) #define LCDC_V2_PL_INT_ENA BIT(6) +#define LCDC_V1_SYNC_LOST_ENA BIT(5) #define LCDC_MONOCHROME_MODE BIT(1) #define LCDC_RASTER_ENABLE BIT(0) #define LCDC_TFT_ALT_ENABLE BIT(23)
2016-11-22 17:54 GMT+01:00 Jyri Sarha jsarha@ti.com:
Revision 1 LCDC support also sync lost errors and can benefit from sync lost recovery routine.
Signed-off-by: Jyri Sarha jsarha@ti.com
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 33 +++++++++++++++++---------------- drivers/gpu/drm/tilcdc/tilcdc_regs.h | 1 + 2 files changed, 18 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index c787349..5260eb2 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -113,6 +113,7 @@ static void tilcdc_crtc_enable_irqs(struct drm_device *dev)
if (priv->rev == 1) { tilcdc_set(dev, LCDC_RASTER_CTRL_REG,
LCDC_V1_SYNC_LOST_ENA | LCDC_V1_UNDERFLOW_INT_ENA); tilcdc_set(dev, LCDC_DMA_CTRL_REG, LCDC_V1_END_OF_FRAME_INT_ENA);@@ -130,7 +131,7 @@ static void tilcdc_crtc_disable_irqs(struct drm_device *dev)
/* disable irqs that we might have enabled: */ if (priv->rev == 1) {
tilcdc_clear(dev, LCDC_RASTER_CTRL_REG,
tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_V1_SYNC_LOST_ENA | LCDC_V1_UNDERFLOW_INT_ENA | LCDC_V1_PL_INT_ENA); tilcdc_clear(dev, LCDC_DMA_CTRL_REG, LCDC_V1_END_OF_FRAME_INT_ENA);@@ -845,6 +846,21 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc) dev_err_ratelimited(dev->dev, "%s(0x%08x): FIFO underflow", __func__, stat);
if (stat & LCDC_SYNC_LOST) {dev_err_ratelimited(dev->dev, "%s(0x%08x): Sync lost",__func__, stat);tilcdc_crtc->frame_intact = false;if (tilcdc_crtc->sync_lost_count++ >SYNC_LOST_COUNT_LIMIT) {dev_err(dev->dev, "%s(0x%08x): Sync lost flood detected, recovering", __func__, stat);queue_work(system_wq,&tilcdc_crtc->recover_work);tilcdc_write(dev, LCDC_INT_ENABLE_CLR_REG,LCDC_SYNC_LOST);tilcdc_crtc->sync_lost_count = 0;}}
We need to at least have tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE) here - otherwise the recovery job doesn't even start - as soon as we re-enable interrupts, we get a sync_lost irq again.
/* For revision 2 only */ if (priv->rev == 2) { if (stat & LCDC_FRAME_DONE) {@@ -852,21 +868,6 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc) wake_up(&tilcdc_crtc->frame_done_wq); }
if (stat & LCDC_SYNC_LOST) {dev_err_ratelimited(dev->dev, "%s(0x%08x): Sync lost",__func__, stat);tilcdc_crtc->frame_intact = false;if (tilcdc_crtc->sync_lost_count++ >SYNC_LOST_COUNT_LIMIT) {dev_err(dev->dev, "%s(0x%08x): Sync lost flood detected, recovering", __func__, stat);queue_work(system_wq,&tilcdc_crtc->recover_work);tilcdc_write(dev, LCDC_INT_ENABLE_CLR_REG,LCDC_SYNC_LOST);tilcdc_crtc->sync_lost_count = 0;}}/* Indicate to LCDC that the interrupt service routine has * completed, see 13.3.6.1.6 in AM335x TRM. */diff --git a/drivers/gpu/drm/tilcdc/tilcdc_regs.h b/drivers/gpu/drm/tilcdc/tilcdc_regs.h index f57c0d6..beb8c21 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_regs.h +++ b/drivers/gpu/drm/tilcdc/tilcdc_regs.h @@ -61,6 +61,7 @@ #define LCDC_V2_UNDERFLOW_INT_ENA BIT(5) #define LCDC_V1_PL_INT_ENA BIT(4) #define LCDC_V2_PL_INT_ENA BIT(6) +#define LCDC_V1_SYNC_LOST_ENA BIT(5)
I'd say we call it LCDC_V1_SYNC_LOST_INT_ENA for consistency.
Thanks, Bartosz Golaszewski
#define LCDC_MONOCHROME_MODE BIT(1) #define LCDC_RASTER_ENABLE BIT(0)
#define LCDC_TFT_ALT_ENABLE BIT(23)
1.9.1
From: Bartosz Golaszewski bgolaszewski@baylibre.com
Revision 1 of the IP doesn't work if we don't load the palette (even if it's not used, which is the case for the RGB565 format).
Add a function called from tilcdc_crtc_enable() which performs all required actions if we're dealing with a rev1 chip.
Signed-off-by: Bartosz Golaszewski bgolaszewski@baylibre.com Signed-off-by: Jyri Sarha jsarha@ti.com --- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 88 +++++++++++++++++++++++++++++++++++- 1 file changed, 87 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index 5260eb2..0bfd7dd 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -21,11 +21,15 @@ #include <drm/drm_flip_work.h> #include <drm/drm_plane_helper.h> #include <linux/workqueue.h> +#include <linux/completion.h> +#include <linux/dma-mapping.h>
#include "tilcdc_drv.h" #include "tilcdc_regs.h"
-#define TILCDC_VBLANK_SAFETY_THRESHOLD_US 1000 +#define TILCDC_VBLANK_SAFETY_THRESHOLD_US 1000 +#define TILCDC_REV1_PALETTE_SIZE 32 +#define TILCDC_REV1_PALETTE_FIRST_ENTRY 0x4000
struct tilcdc_crtc { struct drm_crtc base; @@ -56,6 +60,10 @@ struct tilcdc_crtc { int sync_lost_count; bool frame_intact; struct work_struct recover_work; + + dma_addr_t palette_dma_handle; + void *palette_base; + struct completion palette_loaded; }; #define to_tilcdc_crtc(x) container_of(x, struct tilcdc_crtc, base)
@@ -105,6 +113,55 @@ static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb) tilcdc_crtc->curr_fb = fb; }
+/* + * The driver currently only supports the RGB565 format for revision 1. For + * 16 bits-per-pixel the palette block is bypassed, but the first 32 bytes of + * the framebuffer are still considered palette. The first 16-bit entry must + * be 0x4000 while all other entries must be zeroed. + */ +static void tilcdc_crtc_load_palette(struct drm_crtc *crtc) +{ + u32 dma_fb_base, dma_fb_ceiling, raster_ctl; + struct tilcdc_crtc *tilcdc_crtc; + struct drm_device *dev; + u16 *first_entry; + + dev = crtc->dev; + tilcdc_crtc = to_tilcdc_crtc(crtc); + first_entry = tilcdc_crtc->palette_base; + + *first_entry = TILCDC_REV1_PALETTE_FIRST_ENTRY; + + dma_fb_base = tilcdc_read(dev, LCDC_DMA_FB_BASE_ADDR_0_REG); + dma_fb_ceiling = tilcdc_read(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG); + raster_ctl = tilcdc_read(dev, LCDC_RASTER_CTRL_REG); + + /* Tell the LCDC where the palette is located. */ + tilcdc_write(dev, LCDC_DMA_FB_BASE_ADDR_0_REG, + tilcdc_crtc->palette_dma_handle); + tilcdc_write(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG, + (u32)tilcdc_crtc->palette_dma_handle + + TILCDC_REV1_PALETTE_SIZE - 1); + + /* Load it. */ + tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, + LCDC_PALETTE_LOAD_MODE(DATA_ONLY)); + tilcdc_set(dev, LCDC_RASTER_CTRL_REG, + LCDC_PALETTE_LOAD_MODE(PALETTE_ONLY)); + + /* Enable the LCDC and wait for palette to be loaded. */ + tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_V1_PL_INT_ENA); + tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE); + + wait_for_completion(&tilcdc_crtc->palette_loaded); + + /* Restore the registers. */ + tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE); + tilcdc_write(dev, LCDC_DMA_FB_BASE_ADDR_0_REG, dma_fb_base); + tilcdc_write(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG, dma_fb_ceiling); + tilcdc_write(dev, LCDC_RASTER_CTRL_REG, raster_ctl); +} + static void tilcdc_crtc_enable_irqs(struct drm_device *dev) { struct tilcdc_drm_private *priv = dev->dev_private; @@ -160,6 +217,7 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev; struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc); + struct tilcdc_drm_private *priv = dev->dev_private;
WARN_ON(!drm_modeset_is_locked(&crtc->mutex)); mutex_lock(&tilcdc_crtc->enable_lock); @@ -172,6 +230,9 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc)
reset(crtc);
+ if (priv->rev == 1 && !completion_done(&tilcdc_crtc->palette_loaded)) + tilcdc_crtc_load_palette(crtc); + tilcdc_crtc_enable_irqs(dev);
tilcdc_clear(dev, LCDC_DMA_CTRL_REG, LCDC_DUAL_FRAME_BUFFER_ENABLE); @@ -213,6 +274,13 @@ static void tilcdc_crtc_off(struct drm_crtc *crtc, bool shutdown) __func__); }
+ /* + * LCDC will not retain the palette when reset. Make sure it gets + * reloaded on tilcdc_crtc_enable(). + */ + if (priv->rev == 1) + reinit_completion(&tilcdc_crtc->palette_loaded); + drm_crtc_vblank_off(crtc);
tilcdc_crtc_disable_irqs(dev); @@ -846,6 +914,14 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc) dev_err_ratelimited(dev->dev, "%s(0x%08x): FIFO underflow", __func__, stat);
+ if (priv->rev == 1) { + if (stat & LCDC_PL_LOAD_DONE) { + complete(&tilcdc_crtc->palette_loaded); + tilcdc_clear(dev, + LCDC_RASTER_CTRL_REG, LCDC_V1_PL_INT_ENA); + } + } + if (stat & LCDC_SYNC_LOST) { dev_err_ratelimited(dev->dev, "%s(0x%08x): Sync lost", __func__, stat); @@ -890,6 +966,16 @@ struct drm_crtc *tilcdc_crtc_create(struct drm_device *dev) return NULL; }
+ if (priv->rev == 1) { + init_completion(&tilcdc_crtc->palette_loaded); + tilcdc_crtc->palette_base = dmam_alloc_coherent(dev->dev, + TILCDC_REV1_PALETTE_SIZE, + &tilcdc_crtc->palette_dma_handle, + GFP_KERNEL | __GFP_ZERO); + if (!tilcdc_crtc->palette_base) + return ERR_PTR(-ENOMEM); + } + crtc = &tilcdc_crtc->base;
ret = tilcdc_plane_init(dev, &tilcdc_crtc->primary);
On 22/11/16 18:54, Jyri Sarha wrote:
From: Bartosz Golaszewski bgolaszewski@baylibre.com
Revision 1 of the IP doesn't work if we don't load the palette (even if it's not used, which is the case for the RGB565 format).
Add a function called from tilcdc_crtc_enable() which performs all required actions if we're dealing with a rev1 chip.
Signed-off-by: Bartosz Golaszewski bgolaszewski@baylibre.com Signed-off-by: Jyri Sarha jsarha@ti.com
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 88 +++++++++++++++++++++++++++++++++++- 1 file changed, 87 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index 5260eb2..0bfd7dd 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -21,11 +21,15 @@ #include <drm/drm_flip_work.h> #include <drm/drm_plane_helper.h> #include <linux/workqueue.h> +#include <linux/completion.h> +#include <linux/dma-mapping.h>
#include "tilcdc_drv.h" #include "tilcdc_regs.h"
-#define TILCDC_VBLANK_SAFETY_THRESHOLD_US 1000 +#define TILCDC_VBLANK_SAFETY_THRESHOLD_US 1000 +#define TILCDC_REV1_PALETTE_SIZE 32 +#define TILCDC_REV1_PALETTE_FIRST_ENTRY 0x4000
struct tilcdc_crtc { struct drm_crtc base; @@ -56,6 +60,10 @@ struct tilcdc_crtc { int sync_lost_count; bool frame_intact; struct work_struct recover_work;
- dma_addr_t palette_dma_handle;
- void *palette_base;
- struct completion palette_loaded;
}; #define to_tilcdc_crtc(x) container_of(x, struct tilcdc_crtc, base)
@@ -105,6 +113,55 @@ static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb) tilcdc_crtc->curr_fb = fb; }
+/*
- The driver currently only supports the RGB565 format for revision 1. For
- 16 bits-per-pixel the palette block is bypassed, but the first 32 bytes of
- the framebuffer are still considered palette. The first 16-bit entry must
- be 0x4000 while all other entries must be zeroed.
- */
+static void tilcdc_crtc_load_palette(struct drm_crtc *crtc) +{
- u32 dma_fb_base, dma_fb_ceiling, raster_ctl;
- struct tilcdc_crtc *tilcdc_crtc;
- struct drm_device *dev;
- u16 *first_entry;
- dev = crtc->dev;
- tilcdc_crtc = to_tilcdc_crtc(crtc);
- first_entry = tilcdc_crtc->palette_base;
- *first_entry = TILCDC_REV1_PALETTE_FIRST_ENTRY;
- dma_fb_base = tilcdc_read(dev, LCDC_DMA_FB_BASE_ADDR_0_REG);
- dma_fb_ceiling = tilcdc_read(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG);
- raster_ctl = tilcdc_read(dev, LCDC_RASTER_CTRL_REG);
- /* Tell the LCDC where the palette is located. */
- tilcdc_write(dev, LCDC_DMA_FB_BASE_ADDR_0_REG,
tilcdc_crtc->palette_dma_handle);- tilcdc_write(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG,
(u32)tilcdc_crtc->palette_dma_handle+ TILCDC_REV1_PALETTE_SIZE - 1);- /* Load it. */
- tilcdc_clear(dev, LCDC_RASTER_CTRL_REG,
LCDC_PALETTE_LOAD_MODE(DATA_ONLY));- tilcdc_set(dev, LCDC_RASTER_CTRL_REG,
LCDC_PALETTE_LOAD_MODE(PALETTE_ONLY));- /* Enable the LCDC and wait for palette to be loaded. */
- tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_V1_PL_INT_ENA);
- tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
- wait_for_completion(&tilcdc_crtc->palette_loaded);
- /* Restore the registers. */
- tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
- tilcdc_write(dev, LCDC_DMA_FB_BASE_ADDR_0_REG, dma_fb_base);
- tilcdc_write(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG, dma_fb_ceiling);
- tilcdc_write(dev, LCDC_RASTER_CTRL_REG, raster_ctl);
+}
static void tilcdc_crtc_enable_irqs(struct drm_device *dev) { struct tilcdc_drm_private *priv = dev->dev_private; @@ -160,6 +217,7 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev; struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
struct tilcdc_drm_private *priv = dev->dev_private;
WARN_ON(!drm_modeset_is_locked(&crtc->mutex)); mutex_lock(&tilcdc_crtc->enable_lock);
@@ -172,6 +230,9 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc)
reset(crtc);
if (priv->rev == 1 && !completion_done(&tilcdc_crtc->palette_loaded))
tilcdc_crtc_load_palette(crtc);tilcdc_crtc_enable_irqs(dev);
tilcdc_clear(dev, LCDC_DMA_CTRL_REG, LCDC_DUAL_FRAME_BUFFER_ENABLE);
@@ -213,6 +274,13 @@ static void tilcdc_crtc_off(struct drm_crtc *crtc, bool shutdown) __func__); }
- /*
* LCDC will not retain the palette when reset. Make sure it gets* reloaded on tilcdc_crtc_enable().*/- if (priv->rev == 1)
reinit_completion(&tilcdc_crtc->palette_loaded);
So when the crtc is disabled, you do this, so that on crtc enable the driver would load palette? When is the crtc enabled if it hasn't been disabled first? In other words, when will the palette loading in tilcdc_crtc_enable() _not_ trigger for v1?
This looks a bit messy to me.
Why not just load the palette every time on crtc enable? And reinit the completion in tilcdc_crtc_load_palette().
Tomi
On 11/24/16 11:29, Tomi Valkeinen wrote:
@@ -213,6 +274,13 @@ static void tilcdc_crtc_off(struct drm_crtc *crtc, bool shutdown)
__func__);}
- /*
* LCDC will not retain the palette when reset. Make sure it gets* reloaded on tilcdc_crtc_enable().*/- if (priv->rev == 1)
reinit_completion(&tilcdc_crtc->palette_loaded);So when the crtc is disabled, you do this, so that on crtc enable the driver would load palette? When is the crtc enabled if it hasn't been disabled first? In other words, when will the palette loading in tilcdc_crtc_enable() _not_ trigger for v1?
This looks a bit messy to me.
Why not just load the palette every time on crtc enable? And reinit the completion in tilcdc_crtc_load_palette().
My final version loads it at the end of modeset_nofb(), to avoid this:
1. Load the fb address to dma registers with "ingenious" 64 bit write
2. Load the dma registers to a temporary storrage
3. Put palette address in dma registers and load the palette
4. Restore the dma registers (= fb address)
But, sure. I can load the palette every time the mode changes, but not every time the display is enabled.
Thanks, Jyri
On 24/11/16 11:39, Jyri Sarha wrote:
On 11/24/16 11:29, Tomi Valkeinen wrote:
@@ -213,6 +274,13 @@ static void tilcdc_crtc_off(struct drm_crtc *crtc, bool shutdown)
__func__);}
- /*
* LCDC will not retain the palette when reset. Make sure it gets* reloaded on tilcdc_crtc_enable().*/- if (priv->rev == 1)
reinit_completion(&tilcdc_crtc->palette_loaded);So when the crtc is disabled, you do this, so that on crtc enable the driver would load palette? When is the crtc enabled if it hasn't been disabled first? In other words, when will the palette loading in tilcdc_crtc_enable() _not_ trigger for v1?
This looks a bit messy to me.
Why not just load the palette every time on crtc enable? And reinit the completion in tilcdc_crtc_load_palette().
My final version loads it at the end of modeset_nofb(), to avoid this:
Load the fb address to dma registers with "ingenious" 64 bit write
Load the dma registers to a temporary storrage
Put palette address in dma registers and load the palette
Restore the dma registers (= fb address)
But, sure. I can load the palette every time the mode changes, but not every time the display is enabled.
What is the difference? If mode changes, you need to disable and enable the crtc, right? What other cases there are to enable the display? After blank? Then the display has been off, and I presume palette has to be loaded.
Tomi
On 11/24/16 11:43, Tomi Valkeinen wrote:
What is the difference? If mode changes, you need to disable and enable the crtc, right? What other cases there are to enable the display? After blank? Then the display has been off, and I presume palette has to be loaded.
At the moment the palette or register values do not appear to vanish ever. But that is probably due to PM not doing much to optimize the LCDC power consumption.
Anyway, if simple enable is enough to turn on the display - all video timings, frame buffer dma addresses etc. are already in the registers - then I think it is safe to assume that the palette is still in there too.
Then it is a different issue, that I should probably put the same functionality into PM runtime_suspend() and runtime_resume() callbacks, that is currently in suspend() and resume() callbacks, to be ready if PM ever does anything more for LCDC that it does today. I could of course add a test if the registers are still intact before doing a restore.
BR, Jyri
On 24/11/16 12:03, Jyri Sarha wrote:
On 11/24/16 11:43, Tomi Valkeinen wrote:
What is the difference? If mode changes, you need to disable and enable the crtc, right? What other cases there are to enable the display? After blank? Then the display has been off, and I presume palette has to be loaded.
At the moment the palette or register values do not appear to vanish ever. But that is probably due to PM not doing much to optimize the LCDC power consumption.
If runtime PM for the device goes to suspend, you have to presume the IP has lost all context. That may not always happen, but that's what the driver has to presume, unless there's a way for the driver to verify whether the context has been lost or not.
Anyway, if simple enable is enough to turn on the display - all video timings, frame buffer dma addresses etc. are already in the registers - then I think it is safe to assume that the palette is still in there too.
As long as the driver makes sure the device doesn't go to suspend (by having called pm_runtime_get).
Then it is a different issue, that I should probably put the same functionality into PM runtime_suspend() and runtime_resume() callbacks, that is currently in suspend() and resume() callbacks, to be ready if PM ever does anything more for LCDC that it does today. I could of course add a test if the registers are still intact before doing a restore.
You can do things in resume callback, but I think quite often it's simplest to just do those things when enabling the display. The device never goes to suspend if the display is enabled. And if you disable the display, the device should go to suspend, so usually enable is called after the device has been in suspend.
So, I haven't looked at the tilcdc code in detail in this regard, but my guess is that runtime PM resume could be used to set hardcoded things to registers, stuff that you always know how they are set. Everything else can be just programmed at enable.
Looking at the registers to find out if they're intact is fine, but it's just an optimization.
Tomi
On 11/24/16 12:25, Tomi Valkeinen wrote:
On 24/11/16 12:03, Jyri Sarha wrote:
On 11/24/16 11:43, Tomi Valkeinen wrote:
What is the difference? If mode changes, you need to disable and enable the crtc, right? What other cases there are to enable the display? After blank? Then the display has been off, and I presume palette has to be loaded.
At the moment the palette or register values do not appear to vanish ever. But that is probably due to PM not doing much to optimize the LCDC power consumption.
If runtime PM for the device goes to suspend, you have to presume the IP has lost all context. That may not always happen, but that's what the driver has to presume, unless there's a way for the driver to verify whether the context has been lost or not.
There is couple of registers I can verify that from (reset value != value always used by the driver).
Anyway, if simple enable is enough to turn on the display - all video timings, frame buffer dma addresses etc. are already in the registers - then I think it is safe to assume that the palette is still in there too.
As long as the driver makes sure the device doesn't go to suspend (by having called pm_runtime_get).
Runtime get has always been called when modeset_nofb() is called.
Then it is a different issue, that I should probably put the same functionality into PM runtime_suspend() and runtime_resume() callbacks, that is currently in suspend() and resume() callbacks, to be ready if PM ever does anything more for LCDC that it does today. I could of course add a test if the registers are still intact before doing a restore.
You can do things in resume callback, but I think quite often it's simplest to just do those things when enabling the display. The device never goes to suspend if the display is enabled. And if you disable the display, the device should go to suspend, so usually enable is called after the device has been in suspend.
Well, the two places are pretty much the same thing in tilcdc, because enable calls pm_runtime_get(). Also with atomic the suspend/resume implementation is really straight forward. Just get the current atomic state with drm_atomic_helper_suspend() and commit it back in with drm_atomic_helper_resume(), if needed. I don't think I should implement myself something that is so well provided by pm and drm infrastructure.
I will implement that as soon as I get this current mess with LCDC rev 1 and bridge support sorted out.
So, I haven't looked at the tilcdc code in detail in this regard, but my guess is that runtime PM resume could be used to set hardcoded things to registers, stuff that you always know how they are set. Everything else can be just programmed at enable.
Looking at the registers to find out if they're intact is fine, but it's just an optimization.
Yes, of course.
BR, Jyri
On 24/11/16 12:38, Jyri Sarha wrote:
As long as the driver makes sure the device doesn't go to suspend (by having called pm_runtime_get).
Runtime get has always been called when modeset_nofb() is called.
Yes, runtime get is needed to access the HW, but the question here was "has runtime_get been active ever since setting the registers previously".
If you do
runtime_get(); setup_things(); runtime_put();
runtime_get(); setup_more_things(); runtime_put();
the code is broken as the context is possibly lost between those two blocks. Unless runtime suspend/resume callbacks do a full context save and restore.
Then it is a different issue, that I should probably put the same functionality into PM runtime_suspend() and runtime_resume() callbacks, that is currently in suspend() and resume() callbacks, to be ready if PM ever does anything more for LCDC that it does today. I could of course add a test if the registers are still intact before doing a restore.
You can do things in resume callback, but I think quite often it's simplest to just do those things when enabling the display. The device never goes to suspend if the display is enabled. And if you disable the display, the device should go to suspend, so usually enable is called after the device has been in suspend.
Well, the two places are pretty much the same thing in tilcdc, because enable calls pm_runtime_get(). Also with atomic the suspend/resume implementation is really straight forward. Just get the current atomic state with drm_atomic_helper_suspend() and commit it back in with drm_atomic_helper_resume(), if needed. I don't think I should implement myself something that is so well provided by pm and drm infrastructure.
The suspend/resume you're talking there is the system suspend/resume. That's quite different than the runtime suspend/resume, and they should do very different things.
The current system suspend/resume in tilcdc looks fine.
Tomi
On 11/24/16 13:10, Tomi Valkeinen wrote:
On 24/11/16 12:38, Jyri Sarha wrote:
As long as the driver makes sure the device doesn't go to suspend (by having called pm_runtime_get).
Runtime get has always been called when modeset_nofb() is called.
Yes, runtime get is needed to access the HW, but the question here was "has runtime_get been active ever since setting the registers previously".
If you do
runtime_get(); setup_things(); runtime_put();
runtime_get(); setup_more_things(); runtime_put();
the code is broken as the context is possibly lost between those two blocks. Unless runtime suspend/resume callbacks do a full context save and restore.
Yes, I know that. In the atomic driver the setup is always done in the drm_mode_config_funcs atomic_commit. The tilcdc commit takes does runtime_get() there, before the setup starts, and does a runtime_put() after drm_atomic_helper_commit_modeset_enables() has run. The drm_atomic_helper_commit_modeset_enables() has turned the display on, if necessary, an that will keep the HW powered until the display is turned off by another commit.
Now, I am not sure if drm atomic core assumes optimizes the mode setting in commit, if there previously has been
Then it is a different issue, that I should probably put the same functionality into PM runtime_suspend() and runtime_resume() callbacks, that is currently in suspend() and resume() callbacks, to be ready if PM ever does anything more for LCDC that it does today. I could of course add a test if the registers are still intact before doing a restore.
You can do things in resume callback, but I think quite often it's simplest to just do those things when enabling the display. The device never goes to suspend if the display is enabled. And if you disable the display, the device should go to suspend, so usually enable is called after the device has been in suspend.
Well, the two places are pretty much the same thing in tilcdc, because enable calls pm_runtime_get(). Also with atomic the suspend/resume implementation is really straight forward. Just get the current atomic state with drm_atomic_helper_suspend() and commit it back in with drm_atomic_helper_resume(), if needed. I don't think I should implement myself something that is so well provided by pm and drm infrastructure.
The suspend/resume you're talking there is the system suspend/resume. That's quite different than the runtime suspend/resume, and they should do very different things.
The current system suspend/resume in tilcdc looks fine.
I don't undestand the same mechanism could not be used for runtime resume. Why should it matter if we configure the previous drm atomic state after system suspend or simple LCDC power down suspend?
There may be some need for differentiation with more complex display hardware, but I see no such need for tilcdc.
Tomi
On 24/11/16 14:03, Jyri Sarha wrote:
The suspend/resume you're talking there is the system suspend/resume. That's quite different than the runtime suspend/resume, and they should do very different things.
The current system suspend/resume in tilcdc looks fine.
I don't undestand the same mechanism could not be used for runtime resume. Why should it matter if we configure the previous drm atomic state after system suspend or simple LCDC power down suspend?
They are quite different things.
For example, you have display up and running. The user requests for system suspend. System suspend callback is called in tilcdc. That callback should turn off the displays etc.
Runtime PM suspend should not do anything like that, because it's called when the driver says the IP is not used, which means the driver has already turned off the displays etc.
So, for example, when the system suspend happens, tilcdc's system suspend callback disables the displays by calling some functions. These functions stop the HW, maybe do other cleanups, and then do pm_runtime_put(). This then causes runtime PM suspend callback to be called.
And, of course, runtime PM suspend is called anytime the driver is not using the HW, not only when system suspend happens.
So, system suspend is a high level thing, comes at any point of time from the userspace. Runtime PM is a driver internal thing, comes from the driver.
Tomi
On 11/24/16 14:56, Tomi Valkeinen wrote:
On 24/11/16 14:03, Jyri Sarha wrote:
The suspend/resume you're talking there is the system suspend/resume. That's quite different than the runtime suspend/resume, and they should do very different things.
The current system suspend/resume in tilcdc looks fine.
I don't undestand the same mechanism could not be used for runtime resume. Why should it matter if we configure the previous drm atomic state after system suspend or simple LCDC power down suspend?
They are quite different things.
For example, you have display up and running. The user requests for system suspend. System suspend callback is called in tilcdc. That callback should turn off the displays etc.
Runtime PM suspend should not do anything like that, because it's called when the driver says the IP is not used, which means the driver has already turned off the displays etc.
So, for example, when the system suspend happens, tilcdc's system suspend callback disables the displays by calling some functions. These functions stop the HW, maybe do other cleanups, and then do pm_runtime_put(). This then causes runtime PM suspend callback to be called.
And, of course, runtime PM suspend is called anytime the driver is not using the HW, not only when system suspend happens.
So, system suspend is a high level thing, comes at any point of time from the userspace. Runtime PM is a driver internal thing, comes from the driver.
I am aware of the difference, but in theory I thought it should work, because the situation is pretty much the same. We need restore the state that was in LCDC when it was shut down in disable and restore the state right before we are turning it back on, all the runtime_get() and puts would serialize nicely.
But now after giving it a bit more thought, the drm locking prevents this from working. Who ever is enabling the display, is already holding some modeset locks and prevents drm_atomic_helper_resume() from taking the drm_modeset_lock_all().
Actually following your suggestion appears to be really straight forward. Simply get rid of mode_set_nofb() callback and call the same function in enable just before turning the raster on.
I think I'll make a patch right away.
Cheers, Jyri
On Thu, Nov 24, 2016 at 10:32:59PM +0200, Jyri Sarha wrote:
On 11/24/16 14:56, Tomi Valkeinen wrote:
On 24/11/16 14:03, Jyri Sarha wrote:
The suspend/resume you're talking there is the system suspend/resume. That's quite different than the runtime suspend/resume, and they should do very different things.
The current system suspend/resume in tilcdc looks fine.
I don't undestand the same mechanism could not be used for runtime resume. Why should it matter if we configure the previous drm atomic state after system suspend or simple LCDC power down suspend?
They are quite different things.
For example, you have display up and running. The user requests for system suspend. System suspend callback is called in tilcdc. That callback should turn off the displays etc.
Runtime PM suspend should not do anything like that, because it's called when the driver says the IP is not used, which means the driver has already turned off the displays etc.
So, for example, when the system suspend happens, tilcdc's system suspend callback disables the displays by calling some functions. These functions stop the HW, maybe do other cleanups, and then do pm_runtime_put(). This then causes runtime PM suspend callback to be called.
And, of course, runtime PM suspend is called anytime the driver is not using the HW, not only when system suspend happens.
So, system suspend is a high level thing, comes at any point of time from the userspace. Runtime PM is a driver internal thing, comes from the driver.
I am aware of the difference, but in theory I thought it should work, because the situation is pretty much the same. We need restore the state that was in LCDC when it was shut down in disable and restore the state right before we are turning it back on, all the runtime_get() and puts would serialize nicely.
But now after giving it a bit more thought, the drm locking prevents this from working. Who ever is enabling the display, is already holding some modeset locks and prevents drm_atomic_helper_resume() from taking the drm_modeset_lock_all().
Actually following your suggestion appears to be really straight forward. Simply get rid of mode_set_nofb() callback and call the same function in enable just before turning the raster on.
Yup, that's the right way to do this, and the kernel-doc for mode_set_nofb even explains it ;-) -Daniel
Failed tilcdc_crtc_create() error handling was broken, this patch should fix it.
Signed-off-by: Jyri Sarha jsarha@ti.com --- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 12 +++++++----- drivers/gpu/drm/tilcdc/tilcdc_drv.c | 11 ++++------- drivers/gpu/drm/tilcdc/tilcdc_drv.h | 2 +- 3 files changed, 12 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index 0bfd7dd..b3f35dc 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -953,7 +953,7 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc) return IRQ_HANDLED; }
-struct drm_crtc *tilcdc_crtc_create(struct drm_device *dev) +int tilcdc_crtc_create(struct drm_device *dev) { struct tilcdc_drm_private *priv = dev->dev_private; struct tilcdc_crtc *tilcdc_crtc; @@ -963,7 +963,7 @@ struct drm_crtc *tilcdc_crtc_create(struct drm_device *dev) tilcdc_crtc = devm_kzalloc(dev->dev, sizeof(*tilcdc_crtc), GFP_KERNEL); if (!tilcdc_crtc) { dev_err(dev->dev, "allocation failed\n"); - return NULL; + return -ENOMEM; }
if (priv->rev == 1) { @@ -973,7 +973,7 @@ struct drm_crtc *tilcdc_crtc_create(struct drm_device *dev) &tilcdc_crtc->palette_dma_handle, GFP_KERNEL | __GFP_ZERO); if (!tilcdc_crtc->palette_base) - return ERR_PTR(-ENOMEM); + return -ENOMEM; }
crtc = &tilcdc_crtc->base; @@ -1016,13 +1016,15 @@ struct drm_crtc *tilcdc_crtc_create(struct drm_device *dev) if (!crtc->port) { /* This should never happen */ dev_err(dev->dev, "Port node not found in %s\n", dev->dev->of_node->full_name); + ret = -EINVAL; goto fail; } }
- return crtc; + priv->crtc = crtc; + return 0;
fail: tilcdc_crtc_destroy(crtc); - return NULL; + return -ENOMEM; } diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index af959df..28e97d5 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c @@ -153,13 +153,11 @@ static int tilcdc_commit(struct drm_device *dev, .atomic_commit = tilcdc_commit, };
-static int modeset_init(struct drm_device *dev) +static void modeset_init(struct drm_device *dev) { struct tilcdc_drm_private *priv = dev->dev_private; struct tilcdc_module *mod;
- priv->crtc = tilcdc_crtc_create(dev); - list_for_each_entry(mod, &module_list, list) { DBG("loading module: %s", mod->name); mod->funcs->modeset_init(mod, dev); @@ -170,8 +168,6 @@ static int modeset_init(struct drm_device *dev) dev->mode_config.max_width = tilcdc_crtc_max_width(priv->crtc); dev->mode_config.max_height = 2048; dev->mode_config.funcs = &mode_config_funcs; - - return 0; }
#ifdef CONFIG_CPU_FREQ @@ -370,11 +366,12 @@ static int tilcdc_init(struct drm_driver *ddrv, struct device *dev) } }
- ret = modeset_init(ddev); + ret = tilcdc_crtc_create(ddev); if (ret < 0) { - dev_err(dev, "failed to initialize mode setting\n"); + dev_err(dev, "failed to create crtc\n"); goto init_failed; } + modeset_init(ddev);
if (priv->is_componentized) { ret = component_bind_all(dev, ddev); diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h index 283ff28..7913b0e 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h @@ -167,7 +167,7 @@ struct tilcdc_panel_info {
#define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
-struct drm_crtc *tilcdc_crtc_create(struct drm_device *dev); +int tilcdc_crtc_create(struct drm_device *dev); irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc); void tilcdc_crtc_update_clk(struct drm_crtc *crtc); void tilcdc_crtc_set_panel_info(struct drm_crtc *crtc,
On 22/11/16 18:54, Jyri Sarha wrote:
Failed tilcdc_crtc_create() error handling was broken, this patch should fix it.
Signed-off-by: Jyri Sarha jsarha@ti.com
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 12 +++++++----- drivers/gpu/drm/tilcdc/tilcdc_drv.c | 11 ++++------- drivers/gpu/drm/tilcdc/tilcdc_drv.h | 2 +- 3 files changed, 12 insertions(+), 13 deletions(-)
Instead of just checking the tilcdc_crtc_create() return value, you made all these changes? Maybe it's better this way, but I don't see why that is from the diff.
Tomi
Add tilcdc_write_mask() for handling register field wider than one bit and mask values for those fields.
Signed-off-by: Jyri Sarha jsarha@ti.com --- drivers/gpu/drm/tilcdc/tilcdc_regs.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_regs.h b/drivers/gpu/drm/tilcdc/tilcdc_regs.h index beb8c21..56dbfbd 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_regs.h +++ b/drivers/gpu/drm/tilcdc/tilcdc_regs.h @@ -34,11 +34,14 @@
/* LCDC DMA Control Register */ #define LCDC_DMA_BURST_SIZE(x) ((x) << 4) +#define LCDC_DMA_BURST_SIZE_MASK ((0x7) << 4) #define LCDC_DMA_BURST_1 0x0 #define LCDC_DMA_BURST_2 0x1 #define LCDC_DMA_BURST_4 0x2 #define LCDC_DMA_BURST_8 0x3 #define LCDC_DMA_BURST_16 0x4 +#define LCDC_DMA_FIFO_THRESHOLD(x) ((x) << 8) +#define LCDC_DMA_FIFO_THRESHOLD_MASK ((0x3) << 8) #define LCDC_V1_END_OF_FRAME_INT_ENA BIT(2) #define LCDC_V2_END_OF_FRAME0_INT_ENA BIT(8) #define LCDC_V2_END_OF_FRAME1_INT_ENA BIT(9) @@ -46,10 +49,12 @@
/* LCDC Control Register */ #define LCDC_CLK_DIVISOR(x) ((x) << 8) +#define LCDC_CLK_DIVISOR_MASK ((0xFF) << 8) #define LCDC_RASTER_MODE 0x01
/* LCDC Raster Control Register */ #define LCDC_PALETTE_LOAD_MODE(x) ((x) << 20) +#define LCDC_PALETTE_LOAD_MODE_MASK ((0x3) << 20) #define PALETTE_AND_DATA 0x00 #define PALETTE_ONLY 0x01 #define DATA_ONLY 0x02 @@ -75,7 +80,9 @@
/* LCDC Raster Timing 2 Register */ #define LCDC_AC_BIAS_TRANSITIONS_PER_INT(x) ((x) << 16) +#define LCDC_AC_BIAS_TRANSITIONS_PER_INT_MASK ((0xF) << 16) #define LCDC_AC_BIAS_FREQUENCY(x) ((x) << 8) +#define LCDC_AC_BIAS_FREQUENCY_MASK ((0xFF) << 8) #define LCDC_SYNC_CTRL BIT(25) #define LCDC_SYNC_EDGE BIT(24) #define LCDC_INVERT_PIXEL_CLOCK BIT(22) @@ -140,6 +147,12 @@ static inline u32 tilcdc_read(struct drm_device *dev, u32 reg) return ioread32(priv->mmio + reg); }
+static inline void tilcdc_write_mask(struct drm_device *dev, u32 reg, + u32 val, u32 mask) +{ + tilcdc_write(dev, reg, (tilcdc_read(dev, reg) & ~mask) | (val & mask)); +} + static inline void tilcdc_set(struct drm_device *dev, u32 reg, u32 mask) { tilcdc_write(dev, reg, tilcdc_read(dev, reg) | mask);
Set LCDC_PALETTE_LOAD_MODE bit-field with new tilcdc_write_mask() instead of tilcdc_set(). Setting a bit-fields with tilcdc_set() is fundamentally broken.
Signed-off-by: Jyri Sarha jsarha@ti.com --- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index b3f35dc..0f89422 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -236,7 +236,9 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc) tilcdc_crtc_enable_irqs(dev);
tilcdc_clear(dev, LCDC_DMA_CTRL_REG, LCDC_DUAL_FRAME_BUFFER_ENABLE); - tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_PALETTE_LOAD_MODE(DATA_ONLY)); + tilcdc_write_mask(dev, LCDC_RASTER_CTRL_REG, + LCDC_PALETTE_LOAD_MODE(DATA_ONLY), + LCDC_PALETTE_LOAD_MODE_MASK); tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
drm_crtc_vblank_on(crtc);
The LCDC revision 2 documentation also mentions the mandatory palette for true color modes. Even if the rev 2 LCDC appears to work just fine without the palette being loaded loading it helps in testing the feature.
Signed-off-by: Jyri Sarha jsarha@ti.com --- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 85 +++++++++++++++++------------------- 1 file changed, 41 insertions(+), 44 deletions(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index 0f89422..3bdab77 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -28,8 +28,8 @@ #include "tilcdc_regs.h"
#define TILCDC_VBLANK_SAFETY_THRESHOLD_US 1000 -#define TILCDC_REV1_PALETTE_SIZE 32 -#define TILCDC_REV1_PALETTE_FIRST_ENTRY 0x4000 +#define TILCDC_PALETTE_SIZE 32 +#define TILCDC_PALETTE_FIRST_ENTRY 0x4000
struct tilcdc_crtc { struct drm_crtc base; @@ -62,7 +62,7 @@ struct tilcdc_crtc { struct work_struct recover_work;
dma_addr_t palette_dma_handle; - void *palette_base; + u16 *palette_base; struct completion palette_loaded; }; #define to_tilcdc_crtc(x) container_of(x, struct tilcdc_crtc, base) @@ -114,23 +114,17 @@ static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb) }
/* - * The driver currently only supports the RGB565 format for revision 1. For - * 16 bits-per-pixel the palette block is bypassed, but the first 32 bytes of - * the framebuffer are still considered palette. The first 16-bit entry must - * be 0x4000 while all other entries must be zeroed. + * The driver currently only supports only true color formats. For + * true color the palette block is bypassed, but a 32 byte palette + * should still be loaded. The first 16-bit entry must be 0x4000 while + * all other entries must be zeroed. */ static void tilcdc_crtc_load_palette(struct drm_crtc *crtc) { u32 dma_fb_base, dma_fb_ceiling, raster_ctl; - struct tilcdc_crtc *tilcdc_crtc; - struct drm_device *dev; - u16 *first_entry; - - dev = crtc->dev; - tilcdc_crtc = to_tilcdc_crtc(crtc); - first_entry = tilcdc_crtc->palette_base; - - *first_entry = TILCDC_REV1_PALETTE_FIRST_ENTRY; + struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc); + struct drm_device *dev = crtc->dev; + struct tilcdc_drm_private *priv = dev->dev_private;
dma_fb_base = tilcdc_read(dev, LCDC_DMA_FB_BASE_ADDR_0_REG); dma_fb_ceiling = tilcdc_read(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG); @@ -140,23 +134,34 @@ static void tilcdc_crtc_load_palette(struct drm_crtc *crtc) tilcdc_write(dev, LCDC_DMA_FB_BASE_ADDR_0_REG, tilcdc_crtc->palette_dma_handle); tilcdc_write(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG, - (u32)tilcdc_crtc->palette_dma_handle - + TILCDC_REV1_PALETTE_SIZE - 1); + (u32) tilcdc_crtc->palette_dma_handle + + TILCDC_PALETTE_SIZE - 1);
- /* Load it. */ - tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, - LCDC_PALETTE_LOAD_MODE(DATA_ONLY)); - tilcdc_set(dev, LCDC_RASTER_CTRL_REG, - LCDC_PALETTE_LOAD_MODE(PALETTE_ONLY)); + /* Set dma load mode for palette loading only. */ + tilcdc_write_mask(dev, LCDC_RASTER_CTRL_REG, + LCDC_PALETTE_LOAD_MODE(PALETTE_ONLY), + LCDC_PALETTE_LOAD_MODE_MASK); + + /* Enable DMA Palette Loaded Interrupt */ + if (priv->rev == 1) + tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_V1_PL_INT_ENA); + else + tilcdc_write(dev, LCDC_INT_ENABLE_SET_REG, LCDC_V2_PL_INT_ENA);
- /* Enable the LCDC and wait for palette to be loaded. */ - tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_V1_PL_INT_ENA); + /* Enable LCDC DMA and wait for palette to be loaded. */ + tilcdc_clear_irqstatus(dev, 0xffffffff); tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
wait_for_completion(&tilcdc_crtc->palette_loaded);
- /* Restore the registers. */ + /* Disable LCDC DMA and DMA Palette Loaded Interrupt. */ tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE); + if (priv->rev == 1) + tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_V1_PL_INT_ENA); + else + tilcdc_write(dev, LCDC_INT_ENABLE_CLR_REG, LCDC_V2_PL_INT_ENA); + + /* Restore the registers. */ tilcdc_write(dev, LCDC_DMA_FB_BASE_ADDR_0_REG, dma_fb_base); tilcdc_write(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG, dma_fb_ceiling); tilcdc_write(dev, LCDC_RASTER_CTRL_REG, raster_ctl); @@ -217,7 +222,6 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev; struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc); - struct tilcdc_drm_private *priv = dev->dev_private;
WARN_ON(!drm_modeset_is_locked(&crtc->mutex)); mutex_lock(&tilcdc_crtc->enable_lock); @@ -230,7 +234,7 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc)
reset(crtc);
- if (priv->rev == 1 && !completion_done(&tilcdc_crtc->palette_loaded)) + if (!completion_done(&tilcdc_crtc->palette_loaded)) tilcdc_crtc_load_palette(crtc);
tilcdc_crtc_enable_irqs(dev); @@ -280,8 +284,7 @@ static void tilcdc_crtc_off(struct drm_crtc *crtc, bool shutdown) * LCDC will not retain the palette when reset. Make sure it gets * reloaded on tilcdc_crtc_enable(). */ - if (priv->rev == 1) - reinit_completion(&tilcdc_crtc->palette_loaded); + reinit_completion(&tilcdc_crtc->palette_loaded);
drm_crtc_vblank_off(crtc);
@@ -916,13 +919,8 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc) dev_err_ratelimited(dev->dev, "%s(0x%08x): FIFO underflow", __func__, stat);
- if (priv->rev == 1) { - if (stat & LCDC_PL_LOAD_DONE) { - complete(&tilcdc_crtc->palette_loaded); - tilcdc_clear(dev, - LCDC_RASTER_CTRL_REG, LCDC_V1_PL_INT_ENA); - } - } + if (stat & LCDC_PL_LOAD_DONE) + complete(&tilcdc_crtc->palette_loaded);
if (stat & LCDC_SYNC_LOST) { dev_err_ratelimited(dev->dev, "%s(0x%08x): Sync lost", @@ -968,15 +966,14 @@ int tilcdc_crtc_create(struct drm_device *dev) return -ENOMEM; }
- if (priv->rev == 1) { - init_completion(&tilcdc_crtc->palette_loaded); - tilcdc_crtc->palette_base = dmam_alloc_coherent(dev->dev, - TILCDC_REV1_PALETTE_SIZE, + init_completion(&tilcdc_crtc->palette_loaded); + tilcdc_crtc->palette_base = dmam_alloc_coherent(dev->dev, + TILCDC_PALETTE_SIZE, &tilcdc_crtc->palette_dma_handle, GFP_KERNEL | __GFP_ZERO); - if (!tilcdc_crtc->palette_base) - return -ENOMEM; - } + if (!tilcdc_crtc->palette_base) + return -ENOMEM; + *tilcdc_crtc->palette_base = TILCDC_PALETTE_FIRST_ENTRY;
crtc = &tilcdc_crtc->base;
2016-11-22 17:54 GMT+01:00 Jyri Sarha jsarha@ti.com:
The LCDC revision 2 documentation also mentions the mandatory palette for true color modes. Even if the rev 2 LCDC appears to work just fine without the palette being loaded loading it helps in testing the feature.
Signed-off-by: Jyri Sarha jsarha@ti.com
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 85 +++++++++++++++++------------------- 1 file changed, 41 insertions(+), 44 deletions(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index 0f89422..3bdab77 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -28,8 +28,8 @@ #include "tilcdc_regs.h"
#define TILCDC_VBLANK_SAFETY_THRESHOLD_US 1000 -#define TILCDC_REV1_PALETTE_SIZE 32 -#define TILCDC_REV1_PALETTE_FIRST_ENTRY 0x4000 +#define TILCDC_PALETTE_SIZE 32 +#define TILCDC_PALETTE_FIRST_ENTRY 0x4000
struct tilcdc_crtc { struct drm_crtc base; @@ -62,7 +62,7 @@ struct tilcdc_crtc { struct work_struct recover_work;
dma_addr_t palette_dma_handle;
void *palette_base;
u16 *palette_base; struct completion palette_loaded;}; #define to_tilcdc_crtc(x) container_of(x, struct tilcdc_crtc, base) @@ -114,23 +114,17 @@ static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb) }
/*
- The driver currently only supports the RGB565 format for revision 1. For
- 16 bits-per-pixel the palette block is bypassed, but the first 32 bytes of
- the framebuffer are still considered palette. The first 16-bit entry must
- be 0x4000 while all other entries must be zeroed.
- The driver currently only supports only true color formats. For
- true color the palette block is bypassed, but a 32 byte palette
- should still be loaded. The first 16-bit entry must be 0x4000 while
*/
- all other entries must be zeroed.
static void tilcdc_crtc_load_palette(struct drm_crtc *crtc) { u32 dma_fb_base, dma_fb_ceiling, raster_ctl;
struct tilcdc_crtc *tilcdc_crtc;struct drm_device *dev;u16 *first_entry;dev = crtc->dev;tilcdc_crtc = to_tilcdc_crtc(crtc);first_entry = tilcdc_crtc->palette_base;*first_entry = TILCDC_REV1_PALETTE_FIRST_ENTRY;
struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);struct drm_device *dev = crtc->dev;struct tilcdc_drm_private *priv = dev->dev_private; dma_fb_base = tilcdc_read(dev, LCDC_DMA_FB_BASE_ADDR_0_REG); dma_fb_ceiling = tilcdc_read(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG);@@ -140,23 +134,34 @@ static void tilcdc_crtc_load_palette(struct drm_crtc *crtc) tilcdc_write(dev, LCDC_DMA_FB_BASE_ADDR_0_REG, tilcdc_crtc->palette_dma_handle); tilcdc_write(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG,
(u32)tilcdc_crtc->palette_dma_handle+ TILCDC_REV1_PALETTE_SIZE - 1);
(u32) tilcdc_crtc->palette_dma_handle +TILCDC_PALETTE_SIZE - 1);
/* Load it. */tilcdc_clear(dev, LCDC_RASTER_CTRL_REG,LCDC_PALETTE_LOAD_MODE(DATA_ONLY));tilcdc_set(dev, LCDC_RASTER_CTRL_REG,LCDC_PALETTE_LOAD_MODE(PALETTE_ONLY));
/* Set dma load mode for palette loading only. */tilcdc_write_mask(dev, LCDC_RASTER_CTRL_REG,LCDC_PALETTE_LOAD_MODE(PALETTE_ONLY),LCDC_PALETTE_LOAD_MODE_MASK);/* Enable DMA Palette Loaded Interrupt */if (priv->rev == 1)tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_V1_PL_INT_ENA);elsetilcdc_write(dev, LCDC_INT_ENABLE_SET_REG, LCDC_V2_PL_INT_ENA);
/* Enable the LCDC and wait for palette to be loaded. */tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_V1_PL_INT_ENA);
/* Enable LCDC DMA and wait for palette to be loaded. */tilcdc_clear_irqstatus(dev, 0xffffffff); tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE); wait_for_completion(&tilcdc_crtc->palette_loaded);
/* Restore the registers. */
/* Disable LCDC DMA and DMA Palette Loaded Interrupt. */ tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);if (priv->rev == 1)tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_V1_PL_INT_ENA);elsetilcdc_write(dev, LCDC_INT_ENABLE_CLR_REG, LCDC_V2_PL_INT_ENA);/* Restore the registers. */ tilcdc_write(dev, LCDC_DMA_FB_BASE_ADDR_0_REG, dma_fb_base); tilcdc_write(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG, dma_fb_ceiling); tilcdc_write(dev, LCDC_RASTER_CTRL_REG, raster_ctl);@@ -217,7 +222,6 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev; struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
struct tilcdc_drm_private *priv = dev->dev_private; WARN_ON(!drm_modeset_is_locked(&crtc->mutex)); mutex_lock(&tilcdc_crtc->enable_lock);@@ -230,7 +234,7 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc)
reset(crtc);
if (priv->rev == 1 && !completion_done(&tilcdc_crtc->palette_loaded))
if (!completion_done(&tilcdc_crtc->palette_loaded)) tilcdc_crtc_load_palette(crtc); tilcdc_crtc_enable_irqs(dev);@@ -280,8 +284,7 @@ static void tilcdc_crtc_off(struct drm_crtc *crtc, bool shutdown) * LCDC will not retain the palette when reset. Make sure it gets * reloaded on tilcdc_crtc_enable(). */
if (priv->rev == 1)reinit_completion(&tilcdc_crtc->palette_loaded);
reinit_completion(&tilcdc_crtc->palette_loaded); drm_crtc_vblank_off(crtc);@@ -916,13 +919,8 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc) dev_err_ratelimited(dev->dev, "%s(0x%08x): FIFO underflow", __func__, stat);
if (priv->rev == 1) {if (stat & LCDC_PL_LOAD_DONE) {complete(&tilcdc_crtc->palette_loaded);tilcdc_clear(dev,LCDC_RASTER_CTRL_REG, LCDC_V1_PL_INT_ENA);}}
if (stat & LCDC_PL_LOAD_DONE)complete(&tilcdc_crtc->palette_loaded);
The LCDC_V1_PL_INT_ENA bit needs to be cleared here. Otherwise the system freezes - surprisingly later at register_framebuffer() - in revision 1.
Thanks, Bartosz Golaszewski
if (stat & LCDC_SYNC_LOST) { dev_err_ratelimited(dev->dev, "%s(0x%08x): Sync lost",@@ -968,15 +966,14 @@ int tilcdc_crtc_create(struct drm_device *dev) return -ENOMEM; }
if (priv->rev == 1) {init_completion(&tilcdc_crtc->palette_loaded);tilcdc_crtc->palette_base = dmam_alloc_coherent(dev->dev,TILCDC_REV1_PALETTE_SIZE,
init_completion(&tilcdc_crtc->palette_loaded);tilcdc_crtc->palette_base = dmam_alloc_coherent(dev->dev,TILCDC_PALETTE_SIZE, &tilcdc_crtc->palette_dma_handle, GFP_KERNEL | __GFP_ZERO);
if (!tilcdc_crtc->palette_base)return -ENOMEM;}
if (!tilcdc_crtc->palette_base)return -ENOMEM;*tilcdc_crtc->palette_base = TILCDC_PALETTE_FIRST_ENTRY; crtc = &tilcdc_crtc->base;-- 1.9.1
On 22/11/16 18:54, Jyri Sarha wrote:
The LCDC revision 2 documentation also mentions the mandatory palette for true color modes. Even if the rev 2 LCDC appears to work just fine without the palette being loaded loading it helps in testing the feature.
Signed-off-by: Jyri Sarha jsarha@ti.com
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 85 +++++++++++++++++------------------- 1 file changed, 41 insertions(+), 44 deletions(-)
Squash with patch 2?
Tomi
On 11/24/16 11:37, Tomi Valkeinen wrote:
On 22/11/16 18:54, Jyri Sarha wrote:
The LCDC revision 2 documentation also mentions the mandatory palette for true color modes. Even if the rev 2 LCDC appears to work just fine without the palette being loaded loading it helps in testing the feature.
Signed-off-by: Jyri Sarha jsarha@ti.com
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 85 +++++++++++++++++------------------- 1 file changed, 41 insertions(+), 44 deletions(-)
Squash with patch 2?
Tomi
Probably about time to do it, because otherwise the reviewing is pretty hard.
BR, Jyri
Add timeout wait for palette load-ind to complete. We do not want to hang forever if palette loaded interrupt does not arrive for some reason.
Signed-off-by: Jyri Sarha jsarha@ti.com --- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index 3bdab77..fbb41b1 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -125,6 +125,7 @@ static void tilcdc_crtc_load_palette(struct drm_crtc *crtc) struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc); struct drm_device *dev = crtc->dev; struct tilcdc_drm_private *priv = dev->dev_private; + int ret;
dma_fb_base = tilcdc_read(dev, LCDC_DMA_FB_BASE_ADDR_0_REG); dma_fb_ceiling = tilcdc_read(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG); @@ -152,7 +153,10 @@ static void tilcdc_crtc_load_palette(struct drm_crtc *crtc) tilcdc_clear_irqstatus(dev, 0xffffffff); tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
- wait_for_completion(&tilcdc_crtc->palette_loaded); + ret = wait_for_completion_timeout(&tilcdc_crtc->palette_loaded, + msecs_to_jiffies(50)); + if (ret == 0) + dev_err(dev->dev, "%s: Palette loading timeout", __func__);
/* Disable LCDC DMA and DMA Palette Loaded Interrupt. */ tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
On 22/11/16 18:54, Jyri Sarha wrote:
Add timeout wait for palette load-ind to complete. We do not want to hang forever if palette loaded interrupt does not arrive for some reason.
Signed-off-by: Jyri Sarha jsarha@ti.com
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
Squash with the two earlier palette patches?
Tomi
The palette loading does not work reliably without LCDC SW reset before it. The AM335x TRM suggests this [1] after L3 clock domain has been shut down. We have no knowledge of such events so we do it always. The software reset will clear all the frame information in the FIFO. Upon a restart, the L3 DMA will fetch from the fb0_base address [2].
[1] Section 13.3.8 in AM335x TRM (http://www.ti.com/lit/pdf/sprz360) [2] Section 13.4.6 in AM335x TRM
Signed-off-by: Jyri Sarha jsarha@ti.com --- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index fbb41b1..963e0a0 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -113,6 +113,7 @@ static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb) tilcdc_crtc->curr_fb = fb; }
+static void reset(struct drm_crtc *crtc); /* * The driver currently only supports only true color formats. For * true color the palette block is bypassed, but a 32 byte palette @@ -131,6 +132,9 @@ static void tilcdc_crtc_load_palette(struct drm_crtc *crtc) dma_fb_ceiling = tilcdc_read(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG); raster_ctl = tilcdc_read(dev, LCDC_RASTER_CTRL_REG);
+ /* SW reset before turning DMA on (see section 13.3.8 in AM335x TRM)*/ + reset(crtc); + /* Tell the LCDC where the palette is located. */ tilcdc_write(dev, LCDC_DMA_FB_BASE_ADDR_0_REG, tilcdc_crtc->palette_dma_handle);
We need to use complete_all() to indicate completed palette loading in stead of plain complete() if we want to test if the palette has already been loaded with completion_done(). indicated with.
Signed-off-by: Jyri Sarha jsarha@ti.com --- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index 963e0a0..fd3654d 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -928,7 +928,7 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc) __func__, stat);
if (stat & LCDC_PL_LOAD_DONE) - complete(&tilcdc_crtc->palette_loaded); + complete_all(&tilcdc_crtc->palette_loaded);
if (stat & LCDC_SYNC_LOST) { dev_err_ratelimited(dev->dev, "%s(0x%08x): Sync lost",
Load palette at the end of mode_set_nofb() and only if the palette has not been loaded since last runtime resume. Moving the palette loading to mode_set_nofb() saves us from storing and restoring of LCDC dma addresses that were just recently updated.
Signed-off-by: Jyri Sarha jsarha@ti.com --- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 33 +++++++++++++-------------------- drivers/gpu/drm/tilcdc/tilcdc_drv.c | 12 ++++++++++++ drivers/gpu/drm/tilcdc/tilcdc_drv.h | 1 + 3 files changed, 26 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index fd3654d..1a1ff8d 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -113,6 +113,13 @@ static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb) tilcdc_crtc->curr_fb = fb; }
+void tilcdc_crtc_reload_palette(struct drm_crtc *crtc) +{ + struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc); + + reinit_completion(&tilcdc_crtc->palette_loaded); +} + static void reset(struct drm_crtc *crtc); /* * The driver currently only supports only true color formats. For @@ -122,15 +129,13 @@ static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb) */ static void tilcdc_crtc_load_palette(struct drm_crtc *crtc) { - u32 dma_fb_base, dma_fb_ceiling, raster_ctl; struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc); struct drm_device *dev = crtc->dev; struct tilcdc_drm_private *priv = dev->dev_private; int ret;
- dma_fb_base = tilcdc_read(dev, LCDC_DMA_FB_BASE_ADDR_0_REG); - dma_fb_ceiling = tilcdc_read(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG); - raster_ctl = tilcdc_read(dev, LCDC_RASTER_CTRL_REG); + if (completion_done(&tilcdc_crtc->palette_loaded)) + return;
/* SW reset before turning DMA on (see section 13.3.8 in AM335x TRM)*/ reset(crtc); @@ -168,11 +173,6 @@ static void tilcdc_crtc_load_palette(struct drm_crtc *crtc) tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_V1_PL_INT_ENA); else tilcdc_write(dev, LCDC_INT_ENABLE_CLR_REG, LCDC_V2_PL_INT_ENA); - - /* Restore the registers. */ - tilcdc_write(dev, LCDC_DMA_FB_BASE_ADDR_0_REG, dma_fb_base); - tilcdc_write(dev, LCDC_DMA_FB_CEILING_ADDR_0_REG, dma_fb_ceiling); - tilcdc_write(dev, LCDC_RASTER_CTRL_REG, raster_ctl); }
static void tilcdc_crtc_enable_irqs(struct drm_device *dev) @@ -242,9 +242,6 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc)
reset(crtc);
- if (!completion_done(&tilcdc_crtc->palette_loaded)) - tilcdc_crtc_load_palette(crtc); - tilcdc_crtc_enable_irqs(dev);
tilcdc_clear(dev, LCDC_DMA_CTRL_REG, LCDC_DUAL_FRAME_BUFFER_ENABLE); @@ -288,12 +285,6 @@ static void tilcdc_crtc_off(struct drm_crtc *crtc, bool shutdown) __func__); }
- /* - * LCDC will not retain the palette when reset. Make sure it gets - * reloaded on tilcdc_crtc_enable(). - */ - reinit_completion(&tilcdc_crtc->palette_loaded); - drm_crtc_vblank_off(crtc);
tilcdc_crtc_disable_irqs(dev); @@ -681,10 +672,12 @@ static void tilcdc_crtc_mode_set_nofb(struct drm_crtc *crtc)
drm_framebuffer_reference(fb);
- set_scanout(crtc, fb); - tilcdc_crtc_set_clk(crtc);
+ tilcdc_crtc_load_palette(crtc); + + set_scanout(crtc, fb); + crtc->hwmode = crtc->state->adjusted_mode; }
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index 28e97d5..a7c91f7 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c @@ -637,8 +637,20 @@ static int tilcdc_pm_resume(struct device *dev) } #endif
+static int tilcdc_pm_runtime_resume(struct device *dev) +{ + struct drm_device *ddev = dev_get_drvdata(dev); + struct tilcdc_drm_private *priv = ddev->dev_private; + + if (priv->crtc) + tilcdc_crtc_reload_palette(priv->crtc); + + return 0; +} + static const struct dev_pm_ops tilcdc_pm_ops = { SET_SYSTEM_SLEEP_PM_OPS(tilcdc_pm_suspend, tilcdc_pm_resume) + .runtime_resume = tilcdc_pm_runtime_resume, };
/* diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h index 7913b0e..5803848 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h @@ -180,6 +180,7 @@ void tilcdc_crtc_set_simulate_vesa_sync(struct drm_crtc *crtc, int tilcdc_crtc_update_fb(struct drm_crtc *crtc, struct drm_framebuffer *fb, struct drm_pending_vblank_event *event); +void tilcdc_crtc_reload_palette(struct drm_crtc *crtc);
int tilcdc_plane_init(struct drm_device *dev, struct drm_plane *plane);
We should wait for the last frame to complete before shutting things down also on LCDC rev 1.
Signed-off-by: Jyri Sarha jsarha@ti.com --- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 34 +++++++++++++++++----------------- drivers/gpu/drm/tilcdc/tilcdc_regs.h | 1 + 2 files changed, 18 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index 1a1ff8d..f251546 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -183,7 +183,7 @@ static void tilcdc_crtc_enable_irqs(struct drm_device *dev)
if (priv->rev == 1) { tilcdc_set(dev, LCDC_RASTER_CTRL_REG, - LCDC_V1_SYNC_LOST_ENA | + LCDC_V1_SYNC_LOST_ENA | LCDC_V1_FRAME_DONE_ENA | LCDC_V1_UNDERFLOW_INT_ENA); tilcdc_set(dev, LCDC_DMA_CTRL_REG, LCDC_V1_END_OF_FRAME_INT_ENA); @@ -201,7 +201,8 @@ static void tilcdc_crtc_disable_irqs(struct drm_device *dev)
/* disable irqs that we might have enabled: */ if (priv->rev == 1) { - tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_V1_SYNC_LOST_ENA | + tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, + LCDC_V1_SYNC_LOST_ENA | LCDC_V1_FRAME_DONE_ENA | LCDC_V1_UNDERFLOW_INT_ENA | LCDC_V1_PL_INT_ENA); tilcdc_clear(dev, LCDC_DMA_CTRL_REG, LCDC_V1_END_OF_FRAME_INT_ENA); @@ -261,6 +262,7 @@ static void tilcdc_crtc_off(struct drm_crtc *crtc, bool shutdown) struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc); struct drm_device *dev = crtc->dev; struct tilcdc_drm_private *priv = dev->dev_private; + int ret;
mutex_lock(&tilcdc_crtc->enable_lock); if (shutdown) @@ -273,17 +275,15 @@ static void tilcdc_crtc_off(struct drm_crtc *crtc, bool shutdown) tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
/* - * if necessary wait for framedone irq which will still come - * before putting things to sleep.. + * Wait for framedone irq which will still come before putting + * things to sleep.. */ - if (priv->rev == 2) { - int ret = wait_event_timeout(tilcdc_crtc->frame_done_wq, - tilcdc_crtc->frame_done, - msecs_to_jiffies(500)); - if (ret == 0) - dev_err(dev->dev, "%s: timeout waiting for framedone\n", - __func__); - } + ret = wait_event_timeout(tilcdc_crtc->frame_done_wq, + tilcdc_crtc->frame_done, + msecs_to_jiffies(500)); + if (ret == 0) + dev_err(dev->dev, "%s: timeout waiting for framedone\n", + __func__);
drm_crtc_vblank_off(crtc);
@@ -938,13 +938,13 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc) } }
+ if (stat & LCDC_FRAME_DONE) { + tilcdc_crtc->frame_done = true; + wake_up(&tilcdc_crtc->frame_done_wq); + } + /* For revision 2 only */ if (priv->rev == 2) { - if (stat & LCDC_FRAME_DONE) { - tilcdc_crtc->frame_done = true; - wake_up(&tilcdc_crtc->frame_done_wq); - } - /* Indicate to LCDC that the interrupt service routine has * completed, see 13.3.6.1.6 in AM335x TRM. */ diff --git a/drivers/gpu/drm/tilcdc/tilcdc_regs.h b/drivers/gpu/drm/tilcdc/tilcdc_regs.h index 56dbfbd..4e6975a 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_regs.h +++ b/drivers/gpu/drm/tilcdc/tilcdc_regs.h @@ -67,6 +67,7 @@ #define LCDC_V1_PL_INT_ENA BIT(4) #define LCDC_V2_PL_INT_ENA BIT(6) #define LCDC_V1_SYNC_LOST_ENA BIT(5) +#define LCDC_V1_FRAME_DONE_ENA BIT(3) #define LCDC_MONOCHROME_MODE BIT(1) #define LCDC_RASTER_ENABLE BIT(0) #define LCDC_TFT_ALT_ENABLE BIT(23)
2016-11-22 17:54 GMT+01:00 Jyri Sarha jsarha@ti.com:
We should wait for the last frame to complete before shutting things down also on LCDC rev 1.
Signed-off-by: Jyri Sarha jsarha@ti.com
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 34 +++++++++++++++++----------------- drivers/gpu/drm/tilcdc/tilcdc_regs.h | 1 + 2 files changed, 18 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index 1a1ff8d..f251546 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -183,7 +183,7 @@ static void tilcdc_crtc_enable_irqs(struct drm_device *dev)
if (priv->rev == 1) { tilcdc_set(dev, LCDC_RASTER_CTRL_REG,
LCDC_V1_SYNC_LOST_ENA |
LCDC_V1_SYNC_LOST_ENA | LCDC_V1_FRAME_DONE_ENA | LCDC_V1_UNDERFLOW_INT_ENA); tilcdc_set(dev, LCDC_DMA_CTRL_REG, LCDC_V1_END_OF_FRAME_INT_ENA);@@ -201,7 +201,8 @@ static void tilcdc_crtc_disable_irqs(struct drm_device *dev)
/* disable irqs that we might have enabled: */ if (priv->rev == 1) {
tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_V1_SYNC_LOST_ENA |
tilcdc_clear(dev, LCDC_RASTER_CTRL_REG,LCDC_V1_SYNC_LOST_ENA | LCDC_V1_FRAME_DONE_ENA | LCDC_V1_UNDERFLOW_INT_ENA | LCDC_V1_PL_INT_ENA); tilcdc_clear(dev, LCDC_DMA_CTRL_REG, LCDC_V1_END_OF_FRAME_INT_ENA);@@ -261,6 +262,7 @@ static void tilcdc_crtc_off(struct drm_crtc *crtc, bool shutdown) struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc); struct drm_device *dev = crtc->dev; struct tilcdc_drm_private *priv = dev->dev_private;
int ret; mutex_lock(&tilcdc_crtc->enable_lock); if (shutdown)@@ -273,17 +275,15 @@ static void tilcdc_crtc_off(struct drm_crtc *crtc, bool shutdown) tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
/*
* if necessary wait for framedone irq which will still come* before putting things to sleep..
* Wait for framedone irq which will still come before putting* things to sleep.. */
if (priv->rev == 2) {int ret = wait_event_timeout(tilcdc_crtc->frame_done_wq,tilcdc_crtc->frame_done,msecs_to_jiffies(500));if (ret == 0)dev_err(dev->dev, "%s: timeout waiting for framedone\n",__func__);}
ret = wait_event_timeout(tilcdc_crtc->frame_done_wq,tilcdc_crtc->frame_done,msecs_to_jiffies(500));if (ret == 0)dev_err(dev->dev, "%s: timeout waiting for framedone\n",__func__); drm_crtc_vblank_off(crtc);@@ -938,13 +938,13 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc) } }
if (stat & LCDC_FRAME_DONE) {tilcdc_crtc->frame_done = true;wake_up(&tilcdc_crtc->frame_done_wq);}
This seems to be a general quirk with interrupts on rev1, but if we don't disable the FRAME_DONE interrupt here, then - similarly to SYNC_LOST - we get stuck with an interrupt flood.
/* For revision 2 only */ if (priv->rev == 2) {
if (stat & LCDC_FRAME_DONE) {tilcdc_crtc->frame_done = true;wake_up(&tilcdc_crtc->frame_done_wq);}/* Indicate to LCDC that the interrupt service routine has * completed, see 13.3.6.1.6 in AM335x TRM. */diff --git a/drivers/gpu/drm/tilcdc/tilcdc_regs.h b/drivers/gpu/drm/tilcdc/tilcdc_regs.h index 56dbfbd..4e6975a 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_regs.h +++ b/drivers/gpu/drm/tilcdc/tilcdc_regs.h @@ -67,6 +67,7 @@ #define LCDC_V1_PL_INT_ENA BIT(4) #define LCDC_V2_PL_INT_ENA BIT(6) #define LCDC_V1_SYNC_LOST_ENA BIT(5) +#define LCDC_V1_FRAME_DONE_ENA BIT(3)
I'd call it LCDC_V1_FRAME_DONE_INT_ENA for consistency.
Thanks, Bartosz Golaszewski
#define LCDC_MONOCHROME_MODE BIT(1) #define LCDC_RASTER_ENABLE BIT(0)
#define LCDC_TFT_ALT_ENABLE BIT(23)
1.9.1
dri-devel@lists.freedesktop.org
-
Bartosz Golaszewski -
Daniel Vetter -
Jyri Sarha -
Tomi Valkeinen