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

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@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)