Re: [PATCH v2 1/7] drm/tilcdc: Enable sync lost error and recovery handling for rev 1 LCDC

2016-11-16 13:41 GMT+01:00 Jyri Sarha jsarha@ti.com:

Revision 1 LCDC support also sync lost errors and can benefit from sync lost recovery routine.

Hi Jyri,

I think I found the issue with this patch. Please see below.

Signed-off-by: Jyri Sarha jsarha@ti.com

drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 39 ++++++++++++++++++------------------ drivers/gpu/drm/tilcdc/tilcdc_regs.h | 1 + 2 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index c787349..dfe5796 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -113,8 +113,8 @@ 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);
  

•           tilcdc_set(dev, LCDC_DMA_CTRL_REG,
  

•                   LCDC_V1_UNDERFLOW_INT_ENA |
  

•                   LCDC_V1_SYNC_LOST_ENA |
                    LCDC_V1_END_OF_FRAME_INT_ENA);
    } else {
            tilcdc_write(dev, LCDC_INT_ENABLE_SET_REG,
  

@@ -131,8 +131,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);
  

•           tilcdc_clear(dev, LCDC_DMA_CTRL_REG,
  

•                   LCDC_V1_UNDERFLOW_INT_ENA |
  

•                   LCDC_V1_PL_INT_ENA |
  

•                   LCDC_V1_SYNC_LOST_ENA |
                    LCDC_V1_END_OF_FRAME_INT_ENA);
    } else {
            tilcdc_write(dev, LCDC_INT_ENABLE_CLR_REG,
  

This is what breaks the driver. The END_OF_FRAME interrupt is enabled by writing to the LCD DMA control register on rev 1, not LCDC RASTER control. It should be left as it is - you only need to enable the SYNC_LOST interrupt.

Hint: maybe call it LCDC_V1_SYNC_LOST_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;
  

•           }
  

•   }
  

The SYNC_LOST bit still needs clearing here - otherwise the flood never stops even after recovery work completes.

Thanks, Bartosz Golaszewski