[PATCH v2] drm: tilcdc: implement palette loading for rev1

[PATCH v2] drm: tilcdc: implement palette loading for rev1

[Bartosz Golaszewski]

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>
---
v1 -> v2:
- only allocate dma memory for revision 1

 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 86 +++++++++++++++++++++++++++++++++++-
 1 file changed, 85 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 87cfde9..92771c6 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -21,11 +21,14 @@
 #include <drm/drm_flip_work.h>
 #include <drm/drm_plane_helper.h>
 #include <linux/workqueue.h>
+#include <linux/completion.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;
@@ -53,6 +56,10 @@ struct tilcdc_crtc {
 
 	int sync_lost_count;
 	bool frame_intact;
+
+	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)
 
@@ -100,6 +107,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;
@@ -154,6 +210,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));
 
@@ -164,6 +221,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);
@@ -245,6 +305,12 @@ static void tilcdc_crtc_destroy(struct drm_crtc *crtc)
 	of_node_put(crtc->port);
 	drm_crtc_cleanup(crtc);
 	drm_flip_work_cleanup(&tilcdc_crtc->unref_work);
+
+	if (priv->rev == 1) {
+		dma_free_coherent(crtc->dev->dev, TILCDC_REV1_PALETTE_SIZE,
+				  tilcdc_crtc->palette_base,
+				  tilcdc_crtc->palette_dma_handle);
+	}
 }
 
 int tilcdc_crtc_update_fb(struct drm_crtc *crtc,
@@ -804,6 +870,14 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc)
 		dev_err_ratelimited(dev->dev, "%s(0x%08x): FIFO underfow",
 				    __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);
+		}
+	}
+
 	/* For revision 2 only */
 	if (priv->rev == 2) {
 		if (stat & LCDC_FRAME_DONE) {
@@ -845,6 +919,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 = dma_zalloc_coherent(dev->dev,
+					TILCDC_REV1_PALETTE_SIZE,
+					&tilcdc_crtc->palette_dma_handle,
+					GFP_KERNEL);
+		if (!tilcdc_crtc->palette_base)
+			return ERR_PTR(-ENOMEM);
+	}
+
 	crtc = &tilcdc_crtc->base;
 
 	ret = tilcdc_plane_init(dev, &tilcdc_crtc->primary);
-- 
2.9.3

Re: [PATCH v2] drm: tilcdc: implement palette loading for rev1

[Bartosz Golaszewski]

2016-10-24 10:43 GMT+02:00 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>
> ---
> v1 -> v2:
> - only allocate dma memory for revision 1
>

Superseded by v3.

Sorry for the noise, but I only noticed that a resource managed
version of dma_alloc_coherent() exists after having sent out v2. This
simplifies the code a bit.

Thanks,
Bartosz

Re: [PATCH v2] drm: tilcdc: implement palette loading for rev1

[Jyri Sarha]

On 10/24/16 11:43, Bartosz Golaszewski wrote:
> 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>
> ---
> v1 -> v2:
> - only allocate dma memory for revision 1
> 
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 86 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 85 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 87cfde9..92771c6 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -21,11 +21,14 @@
>  #include <drm/drm_flip_work.h>
>  #include <drm/drm_plane_helper.h>
>  #include <linux/workqueue.h>
> +#include <linux/completion.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;
> @@ -53,6 +56,10 @@ struct tilcdc_crtc {
>  
>  	int sync_lost_count;
>  	bool frame_intact;
> +
> +	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)
>  
> @@ -100,6 +107,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;
> @@ -154,6 +210,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));
>  
> @@ -164,6 +221,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);
> @@ -245,6 +305,12 @@ static void tilcdc_crtc_destroy(struct drm_crtc *crtc)
>  	of_node_put(crtc->port);
>  	drm_crtc_cleanup(crtc);
>  	drm_flip_work_cleanup(&tilcdc_crtc->unref_work);
> +
> +	if (priv->rev == 1) {
> +		dma_free_coherent(crtc->dev->dev, TILCDC_REV1_PALETTE_SIZE,
> +				  tilcdc_crtc->palette_base,
> +				  tilcdc_crtc->palette_dma_handle);
> +	}
>  }
>  
>  int tilcdc_crtc_update_fb(struct drm_crtc *crtc,
> @@ -804,6 +870,14 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc)
>  		dev_err_ratelimited(dev->dev, "%s(0x%08x): FIFO underfow",
>  				    __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);
> +		}
> +	}
> +
>  	/* For revision 2 only */
>  	if (priv->rev == 2) {
>  		if (stat & LCDC_FRAME_DONE) {
> @@ -845,6 +919,16 @@ struct drm_crtc *tilcdc_crtc_create(struct drm_device *dev)
>  		return NULL;
>  	}
>  
> +	if (priv->rev == 1) {
> +		init_completion(&tilcdc_crtc->palette_loaded);


Is it enough to load the palette only once? What if lcdc is powered down
by power management in the middle?

I think you should reinit the completion struct at least in
tilcdc_pm_resume(), if not in tilcdc_crtc_disable().

Cheers,
Jyri

> +		tilcdc_crtc->palette_base = dma_zalloc_coherent(dev->dev,
> +					TILCDC_REV1_PALETTE_SIZE,
> +					&tilcdc_crtc->palette_dma_handle,
> +					GFP_KERNEL);
> +		if (!tilcdc_crtc->palette_base)
> +			return ERR_PTR(-ENOMEM);
> +	}
> +
>  	crtc = &tilcdc_crtc->base;
>  
>  	ret = tilcdc_plane_init(dev, &tilcdc_crtc->primary);
> 

Re: [PATCH v2] drm: tilcdc: implement palette loading for rev1

[Jyri Sarha]

On 10/24/16 11:43, Bartosz Golaszewski wrote:
> 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>
> ---
> v1 -> v2:
> - only allocate dma memory for revision 1
> 
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 86 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 85 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 87cfde9..92771c6 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -21,11 +21,14 @@
>  #include <drm/drm_flip_work.h>
>  #include <drm/drm_plane_helper.h>
>  #include <linux/workqueue.h>
> +#include <linux/completion.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;
> @@ -53,6 +56,10 @@ struct tilcdc_crtc {
>  
>  	int sync_lost_count;
>  	bool frame_intact;
> +
> +	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)
>  
> @@ -100,6 +107,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;
> @@ -154,6 +210,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));
>  
> @@ -164,6 +221,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);
> @@ -245,6 +305,12 @@ static void tilcdc_crtc_destroy(struct drm_crtc *crtc)
>  	of_node_put(crtc->port);
>  	drm_crtc_cleanup(crtc);
>  	drm_flip_work_cleanup(&tilcdc_crtc->unref_work);
> +
> +	if (priv->rev == 1) {
> +		dma_free_coherent(crtc->dev->dev, TILCDC_REV1_PALETTE_SIZE,
> +				  tilcdc_crtc->palette_base,
> +				  tilcdc_crtc->palette_dma_handle);
> +	}
>  }
>  
>  int tilcdc_crtc_update_fb(struct drm_crtc *crtc,
> @@ -804,6 +870,14 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc)
>  		dev_err_ratelimited(dev->dev, "%s(0x%08x): FIFO underfow",
>  				    __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);
> +		}
> +	}
> +
>  	/* For revision 2 only */
>  	if (priv->rev == 2) {
>  		if (stat & LCDC_FRAME_DONE) {
> @@ -845,6 +919,16 @@ struct drm_crtc *tilcdc_crtc_create(struct drm_device *dev)
>  		return NULL;
>  	}
>  
> +	if (priv->rev == 1) {
> +		init_completion(&tilcdc_crtc->palette_loaded);


Is it enough to load the palette only once? What if lcdc is powered down
by power management in the middle?

I think you should reinit the completion struct at least in
tilcdc_pm_resume(), if not in tilcdc_crtc_disable().

Cheers,
Jyri

> +		tilcdc_crtc->palette_base = dma_zalloc_coherent(dev->dev,
> +					TILCDC_REV1_PALETTE_SIZE,
> +					&tilcdc_crtc->palette_dma_handle,
> +					GFP_KERNEL);
> +		if (!tilcdc_crtc->palette_base)
> +			return ERR_PTR(-ENOMEM);
> +	}
> +
>  	crtc = &tilcdc_crtc->base;
>  
>  	ret = tilcdc_plane_init(dev, &tilcdc_crtc->primary);
> 

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

Re: [PATCH v2] drm: tilcdc: implement palette loading for rev1

[Bartosz Golaszewski]

2016-10-24 11:25 GMT+02:00 Jyri Sarha <jsarha@ti.com>:
> On 10/24/16 11:43, Bartosz Golaszewski wrote:
>> 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>
>> ---
>> v1 -> v2:
>> - only allocate dma memory for revision 1
>>
>
>
> Is it enough to load the palette only once? What if lcdc is powered down
> by power management in the middle?
>
> I think you should reinit the completion struct at least in
> tilcdc_pm_resume(), if not in tilcdc_crtc_disable().
>
> Cheers,
> Jyri
>

Hi Jyri,

I ran the following test:

- tilcdc_crtc_enable() was called on device init
- ran modetest -M tilcdc -s 26:800x600@RG16 and quit
- waited for the screen to go blank
- tilcdc_crtc_disable() was called
- ran modetest again
- tilcdc_crtc_enable() was called again, this time without calling
tilcdc_crtc_load_palette()
- color bar still correctly displayed

Seems like it's only needed once.

Thanks,
Bartosz

Re: [PATCH v2] drm: tilcdc: implement palette loading for rev1

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 1454 bytes --]

On 24/10/16 12:47, Bartosz Golaszewski wrote:
> 2016-10-24 11:25 GMT+02:00 Jyri Sarha <jsarha@ti.com>:
>> On 10/24/16 11:43, Bartosz Golaszewski wrote:
>>> 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>
>>> ---
>>> v1 -> v2:
>>> - only allocate dma memory for revision 1
>>>
>>
>>
>> Is it enough to load the palette only once? What if lcdc is powered down
>> by power management in the middle?
>>
>> I think you should reinit the completion struct at least in
>> tilcdc_pm_resume(), if not in tilcdc_crtc_disable().
>>
>> Cheers,
>> Jyri
>>
> 
> Hi Jyri,
> 
> I ran the following test:
> 
> - tilcdc_crtc_enable() was called on device init
> - ran modetest -M tilcdc -s 26:800x600@RG16 and quit
> - waited for the screen to go blank
> - tilcdc_crtc_disable() was called
> - ran modetest again
> - tilcdc_crtc_enable() was called again, this time without calling
> tilcdc_crtc_load_palette()
> - color bar still correctly displayed
> 
> Seems like it's only needed once.

If the power management does its job properly, the IP will get reset
when the IP is suspended. I'm quite sure it won't retain any palettes.

 Tomi


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