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

[Jyri Sarha]

On 11/18/16 15:34, Bartosz Golaszewski wrote:
> 2016-11-16 13:41 GMT+01:00 Jyri Sarha <jsarha at 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 at 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
>