[PATCH v3 00/11] drm/tilcdc: LCDC Revision 1 related fixes

[PATCH v3 00/11] drm/tilcdc: LCDC Revision 1 related fixes

[Jyri Sarha]

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(-)

-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 01/11] drm/tilcdc: Enable sync lost error and recovery handling for rev 1 LCDC

[Jyri Sarha]

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)
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 02/11] drm/tilcdc: implement palette loading for rev1

[Jyri Sarha]

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);
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 03/11] drm/tilcdc: Fix tilcdc_crtc_create() return value handling

[Jyri Sarha]

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,
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 04/11] drm/tilcdc: Add tilcdc_write_mask() to tilcdc_regs.h

[Jyri Sarha]

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);
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 05/11] drm/tilcdc: Fix load mode bit-field setting in tilcdc_crtc_enable()

[Jyri Sarha]

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);
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 06/11] drm/tilcdc: Enable palette loading for revision 2 LCDC too

[Jyri Sarha]

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;
 
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 07/11] drm/tilcdc: Add timeout wait for palette loading to complete

[Jyri Sarha]

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);
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 08/11] drm/tilcdc: Call reset() before loading the palette

[Jyri Sarha]

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);
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 09/11] drm/tilcdc: Use complete_all() to indicate completed palette loading

[Jyri Sarha]

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",
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 10/11] drm/tilcdc: Load palette at the end of mode_set_nofb()

[Jyri Sarha]

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);
 
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 11/11] drm/tilcdc: Enable frame done irq and functionality for LCDC rev 1

[Jyri Sarha]

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)
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 01/11] drm/tilcdc: Enable sync lost error and recovery handling for rev 1 LCDC

[Bartosz Golaszewski]

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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 06/11] drm/tilcdc: Enable palette loading for revision 2 LCDC too

[Bartosz Golaszewski]

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);
> +       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);
>

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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 11/11] drm/tilcdc: Enable frame done irq and functionality for LCDC rev 1

[Bartosz Golaszewski]

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 mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 02/11] drm/tilcdc: implement palette loading for rev1

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 5354 bytes --]

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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 03/11] drm/tilcdc: Fix tilcdc_crtc_create() return value handling

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 581 bytes --]

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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 06/11] drm/tilcdc: Enable palette loading for revision 2 LCDC too

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 495 bytes --]

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



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 07/11] drm/tilcdc: Add timeout wait for palette loading to complete

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 410 bytes --]

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



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 02/11] drm/tilcdc: implement palette loading for rev1

[Jyri Sarha]

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



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 06/11] drm/tilcdc: Enable palette loading for revision 2 LCDC too

[Jyri Sarha]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 02/11] drm/tilcdc: implement palette loading for rev1

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 1569 bytes --]

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:
> 
> 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.

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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 02/11] drm/tilcdc: implement palette loading for rev1

[Jyri Sarha]

[-- Attachment #1.1.1: Type: text/plain, Size: 1037 bytes --]

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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 02/11] drm/tilcdc: implement palette loading for rev1

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 2148 bytes --]

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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 02/11] drm/tilcdc: implement palette loading for rev1

[Jyri Sarha]

[-- Attachment #1.1.1: Type: text/plain, Size: 2999 bytes --]

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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 02/11] drm/tilcdc: implement palette loading for rev1

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 2058 bytes --]

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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 02/11] drm/tilcdc: implement palette loading for rev1

[Jyri Sarha]

[-- Attachment #1.1.1: Type: text/plain, Size: 3072 bytes --]

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
> 



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 02/11] drm/tilcdc: implement palette loading for rev1

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 1475 bytes --]

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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 02/11] drm/tilcdc: implement palette loading for rev1

[Jyri Sarha]

[-- Attachment #1.1.1: Type: text/plain, Size: 2386 bytes --]

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



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 02/11] drm/tilcdc: implement palette loading for rev1

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel