| Message ID | d16d40e1c9530ecfd5e32669e5e2f8a61a5a29ca.1479297559.git.jsarha@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
On 11/18/16 15:34, Bartosz Golaszewski wrote: > 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. > Argh, stupid me. I just over looked that there there were actually two different registers here. Thanks a lot for finding this out for me! > 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. > That is surprising. So turning off the raster and resetting the LCDC does not get rid off sync lost error flood. But the flood stops if we clear the IRQ here and does not reappear when we turn it back on after the reset, does it? > Thanks, > Bartosz Golaszewski >
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, @@ -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)
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 | 39 ++++++++++++++++++------------------ drivers/gpu/drm/tilcdc/tilcdc_regs.h | 1 + 2 files changed, 21 insertions(+), 19 deletions(-)