[PATCH] drm/tilcdc: Set framebuffer DMA address to HW only if CRTC is enabled

[Jyri Sarha]

Touching HW while clocks are of is a serious error and for instance
breaks suspend functionality. After the patch tilcdc_crtc_update_fb()
always updates the primary planes framebuffer pointer, increases fb's
reference count and stores vblank event. The framebuffer's DMA address
is written to HW only if the CRTC is enabled at the moment. The
primary plane's DMA address is written to HW when the CRTC is enabled.

This patch also refactors the tilcdc_crtc_update_fb() a bit. Number of
subsequent small changes to had made it almost unreadable. There
should be no other functional changes but checking the CRTC's enable
state. However, the locking goes a bit differently and some of the
redundant checks have been removed in this new version.

The enable_lock should be enough to protect the access to
tilcdc_crtc->enabled. The irq_lock protects the access to last_vblank
and next_fb. The check for vrefresh and last_vblank being valid is
redundant, as they should now always be valid if the crtc is
enabled. If they are not a division by zero waning is quite
appropriate.

Signed-off-by: Jyri Sarha <jsarha at ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index f80bf93..26cb509 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -464,6 +464,7 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
+	unsigned long flags;
 
 	WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
 	mutex_lock(&tilcdc_crtc->enable_lock);
@@ -484,7 +485,11 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc)
 	tilcdc_write_mask(dev, LCDC_RASTER_CTRL_REG,
 			  LCDC_PALETTE_LOAD_MODE(DATA_ONLY),
 			  LCDC_PALETTE_LOAD_MODE_MASK);
+
+	spin_lock_irqsave(&tilcdc_crtc->irq_lock, flags);
 	tilcdc_set(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
+	tilcdc_crtc->last_vblank = ktime_get();
+	spin_unlock_irqrestore(&tilcdc_crtc->irq_lock, flags);
 
 	drm_crtc_vblank_on(crtc);
 
@@ -539,7 +544,6 @@ static void tilcdc_crtc_off(struct drm_crtc *crtc, bool shutdown)
 	}
 
 	drm_flip_work_commit(&tilcdc_crtc->unref_work, priv->wq);
-	tilcdc_crtc->last_vblank = 0;
 
 	tilcdc_crtc->enabled = false;
 	mutex_unlock(&tilcdc_crtc->enable_lock);
@@ -602,7 +606,6 @@ int tilcdc_crtc_update_fb(struct drm_crtc *crtc,
 {
 	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
 	struct drm_device *dev = crtc->dev;
-	unsigned long flags;
 
 	WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
 
@@ -615,27 +618,29 @@ int tilcdc_crtc_update_fb(struct drm_crtc *crtc,
 
 	crtc->primary->fb = fb;
 
-	spin_lock_irqsave(&tilcdc_crtc->irq_lock, flags);
+	mutex_lock(&tilcdc_crtc->enable_lock);
 
-	if (crtc->hwmode.vrefresh && ktime_to_ns(tilcdc_crtc->last_vblank)) {
+	if (tilcdc_crtc->enabled) {
+		unsigned long flags;
 		ktime_t next_vblank;
 		s64 tdiff;
 
+		spin_lock_irqsave(&tilcdc_crtc->irq_lock, flags);
+
 		next_vblank = ktime_add_us(tilcdc_crtc->last_vblank,
-			1000000 / crtc->hwmode.vrefresh);
-
+					   1000000 / crtc->hwmode.vrefresh);
 		tdiff = ktime_to_us(ktime_sub(next_vblank, ktime_get()));
 
 		if (tdiff < TILCDC_VBLANK_SAFETY_THRESHOLD_US)
 			tilcdc_crtc->next_fb = fb;
+		else
+			set_scanout(crtc, fb);
+
+		spin_unlock_irqrestore(&tilcdc_crtc->irq_lock, flags);
 	}
-
-	if (tilcdc_crtc->next_fb != fb)
-		set_scanout(crtc, fb);
-
 	tilcdc_crtc->event = event;
 
-	spin_unlock_irqrestore(&tilcdc_crtc->irq_lock, flags);
+	mutex_unlock(&tilcdc_crtc->enable_lock);
 
 	return 0;
 }
-- 
1.9.1