[PATCH] drm/tilcdc: Recover from sync lost error flood by resetting the LCDC

[Tomi Valkeinen]

Hi,

On 06/04/16 22:04, Jyri Sarha wrote:
> Recover from sync lost error flood by resetting the LCDC in stead of

"instead"

> turning off the SYNC_LOST error IRQ. When LCDC starves on limited
> memory bandwidth it may sometimes result an error situation when the
> picture may have shifted couple of pixels to right and SYNC_LOST
> interrupt is generated on every frame. LCDC main reset recovers from
> this situation and causes a brief blanking on the screen.
> 
> Signed-off-by: Jyri Sarha <jsarha at ti.com>
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 34 +++++++++++++++++++++++++++++++---
>  1 file changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 051e5e1..5aa86e9 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -46,6 +46,8 @@ struct tilcdc_crtc {
>  
>  	int sync_lost_count;
>  	bool frame_intact;
> +	struct workqueue_struct *wq;
> +	struct work_struct recover_work;

I think the system workqueue should work fine, shouldn't it?

>  };
>  #define to_tilcdc_crtc(x) container_of(x, struct tilcdc_crtc, base)
>  
> @@ -60,6 +62,25 @@ static void unref_worker(struct drm_flip_work *work, void *val)
>  	mutex_unlock(&dev->mode_config.mutex);
>  }
>  
> +static void stop(struct drm_crtc *crtc);
> +static void start(struct drm_crtc *crtc);
> +static void tilcdc_crtc_recover_work(struct work_struct *work)
> +{
> +	struct tilcdc_crtc *tilcdc_crtc =
> +		container_of(work, struct tilcdc_crtc, recover_work);
> +	struct drm_crtc *crtc = &tilcdc_crtc->base;
> +
> +	dev_info(crtc->dev->dev, "%s: Reset CRTC", __func__);
> +
> +	if (tilcdc_crtc->dpms == DRM_MODE_DPMS_OFF)
> +		return;

The 'if' above should probably be inside the lock too. Otherwise dpms
might indicate ON, but is turned OFF when inside the lock.

Did you try with a big msleep here? The irqs are still enabled, so they
keep getting triggered. I wonder if something bad might happen because
of that. For example, set_scanout might be called from the irq handler
while the code below is being ran.

> +
> +	drm_modeset_lock_crtc(crtc, NULL);
> +	stop(crtc);
> +	start(crtc);
> +	drm_modeset_unlock_crtc(crtc);
> +}

I wonder if this works right. stop() just clears the enable bit, without
waiting anything, and start() resets the LCDC. dpms() uses stop(), but
after the call dpms() waits until framedone, i.e. the HW doesn't stop at
stop(), but continues until the end of the frame.

Now, start() does reset, so perhaps it's not a problem. But then, my
experience with OMAP DSS tells me that not waiting for framedone is
usually a bad thing.

In fact, the TRM says "In Raster Modes, the module must be disabled
before applying a software reset. When cfg_lcden is set to ‘0’ to
disable the module, the output continues to the end of the current
frame. The Done interrupt will trigger once the frame is complete. The
software reset can then be applied to the module". See 13.4.6 Disable
and Software Reset Sequence.

> +
>  static void set_scanout(struct drm_crtc *crtc, struct drm_framebuffer *fb)
>  {
>  	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
> @@ -125,6 +146,7 @@ static void tilcdc_crtc_destroy(struct drm_crtc *crtc)
>  	tilcdc_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
>  
>  	of_node_put(crtc->port);
> +	destroy_workqueue(tilcdc_crtc->wq);

Does this ensure that all the queued work has been finished?

>  	drm_crtc_cleanup(crtc);
>  	drm_flip_work_cleanup(&tilcdc_crtc->unref_work);
>  }
> @@ -732,10 +754,10 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc)
>  		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",
> +				"%s(0x%08x): Sync lost flood detected, recovering",
>  				__func__, stat);
> -			tilcdc_write(dev, LCDC_INT_ENABLE_CLR_REG,
> -				     LCDC_SYNC_LOST);
> +			queue_work(tilcdc_crtc->wq, &tilcdc_crtc->recover_work);
> +			tilcdc_crtc->sync_lost_count = 0;
>  		}
>  	}
>  
> @@ -768,6 +790,12 @@ struct drm_crtc *tilcdc_crtc_create(struct drm_device *dev)
>  			"unref", unref_worker);
>  
>  	spin_lock_init(&tilcdc_crtc->irq_lock);
> +	tilcdc_crtc->wq = create_singlethread_workqueue("tilcdc/crtc");
> +	if (!tilcdc_crtc->wq) {
> +		kfree(tilcdc_crtc);
> +		return NULL;
> +	}
> +	INIT_WORK(&tilcdc_crtc->recover_work, tilcdc_crtc_recover_work);
>  
>  	ret = drm_crtc_init(dev, crtc, &tilcdc_crtc_funcs);
>  	if (ret < 0)
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20160407/0f4a06b8/attachment.sig>