drm: tilcdc: improved support for rev1
Subsystems (3)
v5 cover drm: tilcdc: improved support for rev1
This series contains two patches needed to support revision 1 of the LCD controller on da850 SoCs.
The first one implements palette loading - this is a resend of v4 of the stand-alone patch.
The second resets the input FIFO in the DMA controller if a sync lost error occurs and disables the interrupt completely if we're being flooded with these errors.
v1 -> v2:
- only allocate dma memory for revision 1
v2 -> v3:
- use devres managed API for dma memory allocation
v3 -> v4:
- reinit the palette completion in tilcdc_crtc_disable()
v4 -> v5:
- add a second patch clearing the sync lost bit in case of a sync lost error due to insufficient bandwidth
Bartosz Golaszewski (2):
drm: tilcdc: implement palette loading for rev1
drm: tilcdc: clear the sync lost bit in crtc isr
drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 138 +++++++++++++++++++++++++++++++---- drivers/gpu/drm/tilcdc/tilcdc_regs.h | 1 + 2 files changed, 124 insertions(+), 15 deletions(-)
Signature · 1 line
2.9.3
PATCH v5 1/2 drm: tilcdc: implement palette loading for rev1
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.
--- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 88 +++++++++++++++++++++++++++++++++++- 1 file changed, 87 insertions(+), 1 deletion(-)
drivers/gpu/drm/tilcdc/tilcdc_crtc.c +87 -1
@@ -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 0x4000struct tilcdc_crtc {struct drm_crtc base;
@@ -53,6 +57,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)
@@ -98,6 +106,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;
@@ -152,6 +209,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));
@@ -162,6 +220,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 );
@@ -200,6 +261,13 @@ void tilcdc_crtc_disable(struct drm_crtc *crtc) __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);
@@ -802,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 ) {
@@ -843,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 =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);
Signature · 1 line
2.9.3
Re: [PATCH v5 1/2] drm: tilcdc: implement palette loading for rev1
Quoted 6 lines · 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.
There is only one thing I do not like about this patch. The palette loading is done so late that the frame buffer address are already placed into DMA base and ceiling registers, and we need to read them from the registers and restore them back after the palette loading.
Could you try if the palette loading function works without trouble when called from tilcdc_pm_resume() before drm_atomic_helper_resume() call? If it does it would be cleaner in the sense that you could get rid off the old dma address restore code. You could reinit the completion always there right before the palette loading.
If you can not get the above suggestion to work, then I can take this patch.
BR,
Jyri
ps. There is one nit pick comment bellow.
Quoted 59 lines
Signed-off-by Bartosz Golaszewski <[email protected]>--- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 88 +++++++++++++++++++++++++++++++++++- 1 file changed, 87 insertions(+), 1 deletion(-)drivers/gpu/drm/tilcdc/tilcdc_crtc.c +27
-1@@ -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 0x4000struct tilcdc_crtc {struct drm_crtc base;@@ -53,6 +57,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)@@ -98,6 +106,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 ;
drivers/gpu/drm/tilcdc/tilcdc_crtc.c static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb) · line 136
Quoted 10 lines
drivers/gpu/drm/tilcdc/tilcdc_crtc.c +10
-0+ + 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
Just a nit pick, but I would put the plus sign to the end of the line above instead of the beginning of the line bellow. However, check_patch.pl does not complain about this so I guess I can accept it too.
Quoted 89 lines
drivers/gpu/drm/tilcdc/tilcdc_crtc.c +50
-0+ + 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;@@ -152,6 +209,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));@@ -162,6 +220,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 );@@ -200,6 +261,13 @@ void tilcdc_crtc_disable(struct drm_crtc *crtc) __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);@@ -802,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 ) {@@ -843,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 =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);
Re: [PATCH v5 1/2] drm: tilcdc: implement palette loading for rev1
2016-10-31 17:05 GMT+01:00 Jyri Sarha <[email protected]>:
Quoted 13 lines · Bartosz Golaszewski
Quoted 6 lines
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.There is only one thing I do not like about this patch. The palette loading is done so late that the frame buffer address are already placed into DMA base and ceiling registers, and we need to read them from the registers and restore them back after the palette loading.
Quoted 9 lines
Could you try if the palette loading function works without trouble when called from
tilcdc_pm_resume()beforedrm_atomic_helper_resume()call? If it does it would be cleaner in the sense that you could get rid off the old dma address restore code. You could reinit the completion always there right before the palette loading.If you can not get the above suggestion to work, then I can take this patch.
Hi Jyri,
the problem is that tilcdc_pm_resume() is not called when tilcdc is initialized. We would have to have two calls in different places for that to work.
Quoted 18 lines
Quoted 18 lines
drivers/gpu/drm/tilcdc/tilcdc_crtc.c +18
-0+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. */
Quoted 11 lines
drivers/gpu/drm/tilcdc/tilcdc_crtc.c static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb) · line 136
Quoted 4 lines
drivers/gpu/drm/tilcdc/tilcdc_crtc.c +4
-0+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_handleJust a nit pick, but I would put the plus sign to the end of the line above instead of the beginning of the line bellow. However, check_patch.pl does not complain about this so I guess I can accept it too.
Quoted 2 lines
(quoted diff) +2
-0+ + TILCDC_REV1_PALETTE_SIZE - 1); +
I'll fix that in v6.
Thanks,
Bartosz Golaszewski
Re: [PATCH v5 1/2] drm: tilcdc: implement palette loading for rev1
Quoted 24 lines · Bartosz Golaszewski
2016-10-31 17:05 GMT+01:00 Jyri Sarha <[email protected]>:
Quoted 13 lines · Bartosz Golaszewski
Quoted 6 lines
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.There is only one thing I do not like about this patch. The palette loading is done so late that the frame buffer address are already placed into DMA base and ceiling registers, and we need to read them from the registers and restore them back after the palette loading.
Quoted 9 lines
Could you try if the palette loading function works without trouble when called from
tilcdc_pm_resume()beforedrm_atomic_helper_resume()call? If it does it would be cleaner in the sense that you could get rid off the old dma address restore code. You could reinit the completion always there right before the palette loading.If you can not get the above suggestion to work, then I can take this patch.
Quoted 6 lines
Hi Jyri,
the problem is that
tilcdc_pm_resume()is not called when tilcdc is initialized. We would have to have two calls in different places for that to work.
Yep, I know now. I worked on the issue while you were on vacation. Please review my latest patch series (including this patch) and more importantly test if the palette loading still works on rev 1 LCDC.
Quoted 35 lines
Quoted 18 lines
Quoted 18 lines
drivers/gpu/drm/tilcdc/tilcdc_crtc.c +18
-0+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. */ Quoted 11 lines
drivers/gpu/drm/tilcdc/tilcdc_crtc.c static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb) · line 136
Quoted 4 lines
drivers/gpu/drm/tilcdc/tilcdc_crtc.c +4
-0+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_handleJust a nit pick, but I would put the plus sign to the end of the line above instead of the beginning of the line bellow. However, check_patch.pl does not complain about this so I guess I can accept it too.
Quoted 2 lines
(quoted diff) +2
-0+ + TILCDC_REV1_PALETTE_SIZE - 1); +I'll fix that in v6.
Thanks,
Bartosz Golaszewski
PATCH v5 2/2 drm: tilcdc: clear the sync lost bit in crtc isr
In reply to Bartosz GolaszewskiThe frame synchronization error happens when the DMA engine attempts to read what it believes to be the first word of the video buffer but it cannot be recognized as such or when the LCDC is starved of data due to insufficient bandwidth of the system interconnect.
On some SoCs (notably: da850) the memory settings do not meet the LCDC throughput requirements even after increasing the memory controller command re-ordering and the LDCD master peripheral priority, sometimes causing the sync lost error (typically when changing the resolution).
When the sync lost error occurs, simply reset the input FIFO in the DMA controller unless a sync lost flood is detected in which case disable the interrupt.
--- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 50 ++++++++++++++++++++++++++---------- drivers/gpu/drm/tilcdc/tilcdc_regs.h | 1 + 2 files changed, 37 insertions(+), 14 deletions(-)
drivers/gpu/drm/tilcdc/tilcdc_crtc.c +36 -14
@@ -163,7 +163,7 @@ static void tilcdc_crtc_enable_irqs(struct drm_device *dev)if (priv->rev ==1 ) {tilcdc_set (dev,LCDC_RASTER_CTRL_REG , -LCDC_V1_UNDERFLOW_INT_ENA ); +LCDC_V1_UNDERFLOW_INT_ENA |LCDC_V1_SYNC_LOST_ENA );tilcdc_set (dev,LCDC_DMA_CTRL_REG ,LCDC_V1_END_OF_FRAME_INT_ENA ); }else {
@@ -181,7 +181,9 @@ 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_UNDERFLOW_INT_ENA |LCDC_V1_PL_INT_ENA ); +LCDC_V1_UNDERFLOW_INT_ENA | +LCDC_V1_PL_INT_ENA | +LCDC_V1_SYNC_LOST_ENA );tilcdc_clear (dev,LCDC_DMA_CTRL_REG ,LCDC_V1_END_OF_FRAME_INT_ENA ); }else {
@@ -885,24 +887,44 @@ 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, disabling the interrupt" , __func__, stat); -tilcdc_write (dev,LCDC_INT_ENABLE_CLR_REG , -LCDC_SYNC_LOST ); - } - } -/* Indicate to LCDC that the interrupt service routine has * completed, see 13.3.6.1.6 in AM335x TRM. */ tilcdc_write (dev,LCDC_END_OF_INT_IND_REG ,0 ); } +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, disabling the interrupt" , + __func__, stat); +if (priv->rev ==2 ) +tilcdc_write (dev,LCDC_INT_ENABLE_CLR_REG , +LCDC_SYNC_LOST ); +else if (priv->rev ==1 ) +tilcdc_clear (dev,LCDC_RASTER_CTRL_REG , +LCDC_V1_SYNC_LOST_ENA ); + } + +if (priv->rev ==2 ) { +/* +* Indicate to LCDC that the interrupt service routine +* has completed, see 13.3.6.1.6 in AM335x TRM. +*/ +tilcdc_write (dev,LCDC_END_OF_INT_IND_REG ,0 ); + }else if (priv->rev ==1 ) { +/* Reset the input FIFO in the DMA controller. */ +tilcdc_clear (dev, +LCDC_RASTER_CTRL_REG ,LCDC_RASTER_ENABLE ); +tilcdc_set (dev, +LCDC_RASTER_CTRL_REG ,LCDC_RASTER_ENABLE ); + } + } +return IRQ_HANDLED ; }
drivers/gpu/drm/tilcdc/tilcdc_regs.h +1 -0
@@ -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)
Signature · 1 line
2.9.3
Re: [PATCH v5 2/2] drm: tilcdc: clear the sync lost bit in crtc isr
Hi,
Quoted 5 lines · Bartosz Golaszewski
The frame synchronization error happens when the DMA engine attempts to read what it believes to be the first word of the video buffer but it cannot be recognized as such or when the LCDC is starved of data due to insufficient bandwidth of the system interconnect.
This also happens when the framebuffer boundaries are "incorrect", eg when flipping while the crtc is enabled. The driver doesn't handle it yet, so working around it is made via toggling LCDC_RASTER_ENABLE as you put in this change, it is worth noting because that's how modetest for example can "seem" to work with v1 when handling SYNC_LOST and is partly why the "vsynced page flipping" of modetest won't work either with v1 ATM. I haven't looked at the different trees since early september but my guess would be that nobody has reworked this yet.
Quoted 81 lines
On some SoCs (notably: da850) the memory settings do not meet the LCDC throughput requirements even after increasing the memory controller command re-ordering and the LDCD master peripheral priority, sometimes causing the sync lost error (typically when changing the resolution).
When the sync lost error occurs, simply reset the input FIFO in the DMA controller unless a sync lost flood is detected in which case disable the interrupt.
Signed-off-by Bartosz Golaszewski <[email protected]>--- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 50 ++++++++++++++++++++++++++---------- drivers/gpu/drm/tilcdc/tilcdc_regs.h | 1 + 2 files changed, 37 insertions(+), 14 deletions(-)drivers/gpu/drm/tilcdc/tilcdc_crtc.c +23
-14@@ -163,7 +163,7 @@ static void tilcdc_crtc_enable_irqs(struct drm_device *dev)if (priv->rev ==1 ) {tilcdc_set (dev,LCDC_RASTER_CTRL_REG , -LCDC_V1_UNDERFLOW_INT_ENA ); +LCDC_V1_UNDERFLOW_INT_ENA |LCDC_V1_SYNC_LOST_ENA );tilcdc_set (dev,LCDC_DMA_CTRL_REG ,LCDC_V1_END_OF_FRAME_INT_ENA ); }else {@@ -181,7 +181,9 @@ 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_UNDERFLOW_INT_ENA |LCDC_V1_PL_INT_ENA ); +LCDC_V1_UNDERFLOW_INT_ENA | +LCDC_V1_PL_INT_ENA | +LCDC_V1_SYNC_LOST_ENA );tilcdc_clear (dev,LCDC_DMA_CTRL_REG ,LCDC_V1_END_OF_FRAME_INT_ENA ); }else {@@ -885,24 +887,44 @@ 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, disabling the interrupt" , __func__, stat); -tilcdc_write (dev,LCDC_INT_ENABLE_CLR_REG , -LCDC_SYNC_LOST ); - } - } -/* Indicate to LCDC that the interrupt service routine has * completed, see 13.3.6.1.6 in AM335x TRM. */ tilcdc_write (dev,LCDC_END_OF_INT_IND_REG ,0 ); } +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, disabling the interrupt" , + __func__, stat); +if (priv->rev ==2 ) +tilcdc_write (dev,LCDC_INT_ENABLE_CLR_REG , +LCDC_SYNC_LOST ); +else if (priv->rev ==1 ) +tilcdc_clear (dev,LCDC_RASTER_CTRL_REG , +LCDC_V1_SYNC_LOST_ENA ); + } + +if (priv->rev ==2 ) { +/*
drivers/gpu/drm/tilcdc/tilcdc_crtc.c irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc) · line 924
Quoted 10 lines
drivers/gpu/drm/tilcdc/tilcdc_crtc.c +10
-0+* Indicate to LCDC that the interrupt service routine +* has completed, see 13.3.6.1.6 in AM335x TRM. +*/ +tilcdc_write (dev,LCDC_END_OF_INT_IND_REG ,0 ); + }else if (priv->rev ==1 ) { +/* Reset the input FIFO in the DMA controller. */ +tilcdc_clear (dev, +LCDC_RASTER_CTRL_REG ,LCDC_RASTER_ENABLE ); +tilcdc_set (dev, +LCDC_RASTER_CTRL_REG ,LCDC_RASTER_ENABLE );
Here you are leaving the RASTER_ENABLE bit set unconditionnally. It is disabled in tilcdc_crtc_disable with the irqs on (for v2 to handle the last vblank), if the controller loses sync in the window between disabling it and disabling the irqs in tilcdc_crtc_disable it is likely to be problematic for the next call to tilcdc_crtc_enable.
Regards,
Karl
Re: [PATCH v5 2/2] drm: tilcdc: clear the sync lost bit in crtc isr
In reply to Bartosz GolaszewskiQuoted 11 lines · Bartosz Golaszewski
The frame synchronization error happens when the DMA engine attempts to read what it believes to be the first word of the video buffer but it cannot be recognized as such or when the LCDC is starved of data due to insufficient bandwidth of the system interconnect.
On some SoCs (notably: da850) the memory settings do not meet the LCDC throughput requirements even after increasing the memory controller command re-ordering and the LDCD master peripheral priority, sometimes causing the sync lost error (typically when changing the resolution).
Quoted 4 lines
When the sync lost error occurs, simply reset the input FIFO in the DMA controller unless a sync lost flood is detected in which case disable the interrupt.
I don't think the current behaviour after detecting a sync lost flood is really good for ver2 either. The flood is an indication of an error situation on LCDC IP and the picture on the screen may be corrupted. Simply turning off the interrupt does not make the problem (specifically the screen corruption) to go away.
I have a patch for an alternative approach that turns off the display and resets the LCDC and turns it back on again. It fixes the sync lost flood and the screen corruption (usually shifting to right and rolling over to left hand side). According to my experiments the toggling of raster to off and back on again does not fix the the flood or the the screen corruption.
I think it is about time for me the send my sync lost flood patch forward. Then you could extend it to work also on rev1 LCDC.
Best regards,
Jyri
Quoted 100 lines
Signed-off-by Bartosz Golaszewski <[email protected]>--- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 50 ++++++++++++++++++++++++++---------- drivers/gpu/drm/tilcdc/tilcdc_regs.h | 1 + 2 files changed, 37 insertions(+), 14 deletions(-)drivers/gpu/drm/tilcdc/tilcdc_crtc.c +36
-14@@ -163,7 +163,7 @@ static void tilcdc_crtc_enable_irqs(struct drm_device *dev)if (priv->rev ==1 ) {tilcdc_set (dev,LCDC_RASTER_CTRL_REG , -LCDC_V1_UNDERFLOW_INT_ENA ); +LCDC_V1_UNDERFLOW_INT_ENA |LCDC_V1_SYNC_LOST_ENA );tilcdc_set (dev,LCDC_DMA_CTRL_REG ,LCDC_V1_END_OF_FRAME_INT_ENA ); }else {@@ -181,7 +181,9 @@ 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_UNDERFLOW_INT_ENA |LCDC_V1_PL_INT_ENA ); +LCDC_V1_UNDERFLOW_INT_ENA | +LCDC_V1_PL_INT_ENA | +LCDC_V1_SYNC_LOST_ENA );tilcdc_clear (dev,LCDC_DMA_CTRL_REG ,LCDC_V1_END_OF_FRAME_INT_ENA ); }else {@@ -885,24 +887,44 @@ 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, disabling the interrupt" , __func__, stat); -tilcdc_write (dev,LCDC_INT_ENABLE_CLR_REG , -LCDC_SYNC_LOST ); - } - } -/* Indicate to LCDC that the interrupt service routine has * completed, see 13.3.6.1.6 in AM335x TRM. */ tilcdc_write (dev,LCDC_END_OF_INT_IND_REG ,0 ); } +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, disabling the interrupt" , + __func__, stat); +if (priv->rev ==2 ) +tilcdc_write (dev,LCDC_INT_ENABLE_CLR_REG , +LCDC_SYNC_LOST ); +else if (priv->rev ==1 ) +tilcdc_clear (dev,LCDC_RASTER_CTRL_REG , +LCDC_V1_SYNC_LOST_ENA ); + } + +if (priv->rev ==2 ) { +/* +* Indicate to LCDC that the interrupt service routine +* has completed, see 13.3.6.1.6 in AM335x TRM. +*/ +tilcdc_write (dev,LCDC_END_OF_INT_IND_REG ,0 ); + }else if (priv->rev ==1 ) { +/* Reset the input FIFO in the DMA controller. */ +tilcdc_clear (dev, +LCDC_RASTER_CTRL_REG ,LCDC_RASTER_ENABLE ); +tilcdc_set (dev, +LCDC_RASTER_CTRL_REG ,LCDC_RASTER_ENABLE ); + } + } +return IRQ_HANDLED ; }drivers/gpu/drm/tilcdc/tilcdc_regs.h +1
-0@@ -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)
Re: [PATCH v5 2/2] drm: tilcdc: clear the sync lost bit in crtc isr
In reply to Bartosz GolaszewskiQuoted 55 lines · Bartosz Golaszewski
The frame synchronization error happens when the DMA engine attempts to read what it believes to be the first word of the video buffer but it cannot be recognized as such or when the LCDC is starved of data due to insufficient bandwidth of the system interconnect.
On some SoCs (notably: da850) the memory settings do not meet the LCDC throughput requirements even after increasing the memory controller command re-ordering and the LDCD master peripheral priority, sometimes causing the sync lost error (typically when changing the resolution).
When the sync lost error occurs, simply reset the input FIFO in the DMA controller unless a sync lost flood is detected in which case disable the interrupt.
Signed-off-by Bartosz Golaszewski <[email protected]>--- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 50 ++++++++++++++++++++++++++---------- drivers/gpu/drm/tilcdc/tilcdc_regs.h | 1 + 2 files changed, 37 insertions(+), 14 deletions(-)drivers/gpu/drm/tilcdc/tilcdc_crtc.c +4
-8@@ -163,7 +163,7 @@ static void tilcdc_crtc_enable_irqs(struct drm_device *dev)if (priv->rev ==1 ) {tilcdc_set (dev,LCDC_RASTER_CTRL_REG , -LCDC_V1_UNDERFLOW_INT_ENA ); +LCDC_V1_UNDERFLOW_INT_ENA |LCDC_V1_SYNC_LOST_ENA );tilcdc_set (dev,LCDC_DMA_CTRL_REG ,LCDC_V1_END_OF_FRAME_INT_ENA ); }else {@@ -181,7 +181,9 @@ 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_UNDERFLOW_INT_ENA |LCDC_V1_PL_INT_ENA ); +LCDC_V1_UNDERFLOW_INT_ENA | +LCDC_V1_PL_INT_ENA | +LCDC_V1_SYNC_LOST_ENA );tilcdc_clear (dev,LCDC_DMA_CTRL_REG ,LCDC_V1_END_OF_FRAME_INT_ENA ); }else {@@ -885,24 +887,44 @@ 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 ) {
drivers/gpu/drm/tilcdc/tilcdc_crtc.c irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc) · line 893
Quoted 10 lines
drivers/gpu/drm/tilcdc/tilcdc_crtc.c +0
-6-dev_err (dev->dev,"%s(0x%08x): Sync lost flood detected, disabling the interrupt" , __func__, stat); -tilcdc_write (dev,LCDC_INT_ENABLE_CLR_REG , -LCDC_SYNC_LOST ); - } - } -/* Indicate to LCDC that the interrupt service routine has * completed, see 13.3.6.1.6 in AM335x TRM. */ tilcdc_write (dev,LCDC_END_OF_INT_IND_REG ,0 );
Oh, I think this should the last thing done in the irq routine, thought I do not really know what it does :).
Quoted 50 lines
drivers/gpu/drm/tilcdc/tilcdc_crtc.c +32
-0} +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, disabling the interrupt" , + __func__, stat); +if (priv->rev ==2 ) +tilcdc_write (dev,LCDC_INT_ENABLE_CLR_REG , +LCDC_SYNC_LOST ); +else if (priv->rev ==1 ) +tilcdc_clear (dev,LCDC_RASTER_CTRL_REG , +LCDC_V1_SYNC_LOST_ENA ); + } + +if (priv->rev ==2 ) { +/* +* Indicate to LCDC that the interrupt service routine +* has completed, see 13.3.6.1.6 in AM335x TRM. +*/ +tilcdc_write (dev,LCDC_END_OF_INT_IND_REG ,0 ); + }else if (priv->rev ==1 ) { +/* Reset the input FIFO in the DMA controller. */ +tilcdc_clear (dev, +LCDC_RASTER_CTRL_REG ,LCDC_RASTER_ENABLE ); +tilcdc_set (dev, +LCDC_RASTER_CTRL_REG ,LCDC_RASTER_ENABLE ); + } + } +return IRQ_HANDLED ; }drivers/gpu/drm/tilcdc/tilcdc_regs.h +1
-0@@ -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)