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

[Bartosz Golaszewski <bgolaszewski@baylibre.com>]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel