drm/tilcdc: Fix cpufreq transition related race + cleanup

PATCH Superseded Jyri Sarha 2 participants 7 messages Sep 6, 2016 – Sep 7, 2016

Subsystems (3)

DRM drivers DRM misc DRM drivers for TI lcdc

lore

Share & tools

lore

Jyri Sarha

author

2016-09-06 20:59 UTC #0 lore raw

v2 cover drm/tilcdc: Fix cpufreq transition related race + cleanup

Changes since v1:
- Use drm_modeset_lock/unlock_crtc() instead of taking mode config mutex
- Rewrote decsription for old "drm/tilcdc: Add tilcdc_crtc_set_clk() and cleanup cpufreq_transition()" which now called "drm/tilcdc: Clean up LCDC functional clock rate setting code"
- Dropped "drm/tilcdc: Add mutex to protect crtc enable and disable routines"
- Added "drm/tilcdc: Flush flip-work workqueue before drm_flip_work_cleanup()"
- Added "drm/tilcdc: Remove unnecessary tilcdc_crtc_disable() from tilcdc_unload()"
- Added "drm/tilcdc: WARN if CRTC is touched without CRTC lock"

There was a race between mode_set_nofb() and cpufreq_transition() calling tilcdc_crtc_update_clk() without locking.

The first patch fixes the race in with a minimal change by taking the drm CRTC lock for the duration of the clock update.

The second patch goes a step forward and cleans up the clock setting code a bit.

BR,
Jyri

Jyri Sarha (5):
drm/tilcdc: Take crtc modeset lock while updating the crtc clock rate drm/tilcdc: Clean up LCDC functional clock rate setting code drm/tilcdc: Flush flip-work workqueue before drm_flip_work_cleanup() drm/tilcdc: Remove unnecessary tilcdc_crtc_disable() from
tilcdc_unload()
drm/tilcdc: WARN if CRTC is touched without CRTC lock

 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 91 +++++++++++++++++++++++-------------
 drivers/gpu/drm/tilcdc/tilcdc_drv.c  | 12 ++---
 drivers/gpu/drm/tilcdc/tilcdc_drv.h  |  1 -
 3 files changed, 62 insertions(+), 42 deletions(-)

Signature · 6 lines

1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Jyri Sarha

author

2016-09-06 20:59 UTC #1 lore raw

PATCH v2 1/5 drm/tilcdc: Take crtc modeset lock while updating the crtc clock rate

Take crtc modeset lock while updating the crtc clock rate. To avoid a race in tilcdc_crtc_update_clk(), we do not want crtc mode to change while we update crtc clock.

Signed-off-by Jyri Sarha <[email protected]>

---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/gpu/drm/tilcdc/tilcdc_drv.c +3 -0

index f8892e9..b1ac61e 100644
@@ -184,10 +184,13 @@ static int cpufreq_transition(struct notifier_block *nb,
 {
 	struct tilcdc_drm_private *priv = container_of(nb,
 			struct tilcdc_drm_private, freq_transition);
+
 	if (val == CPUFREQ_POSTCHANGE) {
 		if (priv->lcd_fck_rate != clk_get_rate(priv->clk)) {
+			drm_modeset_lock_crtc(priv->crtc, NULL);
 			priv->lcd_fck_rate = clk_get_rate(priv->clk);
 			tilcdc_crtc_update_clk(priv->crtc);
+			drm_modeset_unlock_crtc(priv->crtc);
 		}
 	}

Signature · 6 lines

1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Jyri Sarha

author

2016-09-06 20:59 UTC #2 lore raw

PATCH v2 2/5 drm/tilcdc: Clean up LCDC functional clock rate setting code

In reply to Jyri Sarha

Clean up LCDC functional clock rate seating code.

The LCDC functional clock is set by two functions: mode_set_nofb() and cpufreq_transition().

When tilcdc_crtc_mode_set_nofb() is called in atomic commit phase the drm atomic helpers have taken all the necessary drm locks and turned off the crtc, while tilcdc_commit() is keeping LCDC powered on. For mode_set_nofb() just a simple clock setting function without any locking or power management code is enough. The new tilcdc_crtc_set_clk() is implemented for that purpose.

cpufreq_transition() on the other hand is called from outside DRM and it needs to take the necessary locks and turn off the CRTC while keeping the LCDC powered. The reimplemented tilcdc_crtc_update_clk() is for that purpose and it uses the new tilcdc_crtc_set_clk() to actually set the clock.

Signed-off-by Jyri Sarha <[email protected]>

---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 77 +++++++++++++++++++++---------------
 drivers/gpu/drm/tilcdc/tilcdc_drv.c  | 11 +-----
 drivers/gpu/drm/tilcdc/tilcdc_drv.h  |  1 -
 3 files changed, 47 insertions(+), 42 deletions(-)

drivers/gpu/drm/tilcdc/tilcdc_crtc.c +45 ~~-32~~

index 84b36fd..f4e6a5b 100644
@@ -35,6 +35,8 @@ struct tilcdc_crtc {
 	bool frame_done;
 	spinlock_t irq_lock;
 
+	unsigned int lcd_fck_rate;
+
 	ktime_t last_vblank;
 
 	struct drm_framebuffer *curr_fb;

@@ -324,6 +326,37 @@ static bool tilcdc_crtc_mode_fixup(struct drm_crtc *crtc,
 	return true;
 }
 
+static void tilcdc_crtc_set_clk(struct drm_crtc *crtc)
+{
+	struct drm_device *dev = crtc->dev;
+	struct tilcdc_drm_private *priv = dev->dev_private;
+	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
+	const unsigned clkdiv = 2; /* using a fixed divider of 2 */
+	int ret;
+
+	/* mode.clock is in KHz, set_rate wants parameter in Hz */
+	ret = clk_set_rate(priv->clk, crtc->mode.clock * 1000 * clkdiv);
+	if (ret < 0) {
+		dev_err(dev->dev, "failed to set display clock rate to: %d\n",
+			crtc->mode.clock);
+		return;
+	}
+
+	tilcdc_crtc->lcd_fck_rate = clk_get_rate(priv->clk);
+
+	DBG("lcd_clk=%u, mode clock=%d, div=%u",
+	    tilcdc_crtc->lcd_fck_rate, crtc->mode.clock, clkdiv);
+
+	/* Configure the LCD clock divisor. */
+	tilcdc_write(dev, LCDC_CTRL_REG, LCDC_CLK_DIVISOR(clkdiv) |
+		     LCDC_RASTER_MODE);
+
+	if (priv->rev == 2)
+		tilcdc_set(dev, LCDC_CLK_ENABLE_REG,
+				LCDC_V2_DMA_CLK_EN | LCDC_V2_LIDD_CLK_EN |
+				LCDC_V2_CORE_CLK_EN);
+}
+
 static void tilcdc_crtc_mode_set_nofb(struct drm_crtc *crtc)
 {
 	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);

@@ -486,7 +519,7 @@ static void tilcdc_crtc_mode_set_nofb(struct drm_crtc *crtc)
 
 	set_scanout(crtc, fb);
 
-	tilcdc_crtc_update_clk(crtc);
+	tilcdc_crtc_set_clk(crtc);
 
 	crtc->hwmode = crtc->state->adjusted_mode;
 }

@@ -655,41 +688,21 @@ void tilcdc_crtc_update_clk(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct tilcdc_drm_private *priv = dev->dev_private;
-	unsigned long lcd_clk;
-	const unsigned clkdiv = 2; /* using a fixed divider of 2 */
-	int ret;
+	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
 
-	pm_runtime_get_sync(dev->dev);
+	drm_modeset_lock_crtc(crtc, NULL);
+	if (tilcdc_crtc->lcd_fck_rate != clk_get_rate(priv->clk)) {
+		if (tilcdc_crtc_is_on(crtc)) {
+			pm_runtime_get_sync(dev->dev);
+			tilcdc_crtc_disable(crtc);
 
-	tilcdc_crtc_disable(crtc);
+			tilcdc_crtc_set_clk(crtc);
 
-	/* mode.clock is in KHz, set_rate wants parameter in Hz */
-	ret = clk_set_rate(priv->clk, crtc->mode.clock * 1000 * clkdiv);
-	if (ret < 0) {
-		dev_err(dev->dev, "failed to set display clock rate to: %d\n",
-				crtc->mode.clock);
-		goto out;
+			tilcdc_crtc_enable(crtc);
+			pm_runtime_put_sync(dev->dev);
+		}
 	}
-
-	lcd_clk = clk_get_rate(priv->clk);
-
-	DBG("lcd_clk=%lu, mode clock=%d, div=%u",
-		lcd_clk, crtc->mode.clock, clkdiv);
-
-	/* Configure the LCD clock divisor. */
-	tilcdc_write(dev, LCDC_CTRL_REG, LCDC_CLK_DIVISOR(clkdiv) |
-			LCDC_RASTER_MODE);
-
-	if (priv->rev == 2)
-		tilcdc_set(dev, LCDC_CLK_ENABLE_REG,
-				LCDC_V2_DMA_CLK_EN | LCDC_V2_LIDD_CLK_EN |
-				LCDC_V2_CORE_CLK_EN);
-
-	if (tilcdc_crtc_is_on(crtc))
-		tilcdc_crtc_enable(crtc);
-
-out:
-	pm_runtime_put_sync(dev->dev);
+	drm_modeset_unlock_crtc(crtc);
 }
 
 #define SYNC_LOST_COUNT_LIMIT 50

drivers/gpu/drm/tilcdc/tilcdc_drv.c +2 -9

index b1ac61e..52ff3e1 100644
@@ -185,14 +185,8 @@ static int cpufreq_transition(struct notifier_block *nb,
 	struct tilcdc_drm_private *priv = container_of(nb,
 			struct tilcdc_drm_private, freq_transition);
 
-	if (val == CPUFREQ_POSTCHANGE) {
-		if (priv->lcd_fck_rate != clk_get_rate(priv->clk)) {
-			drm_modeset_lock_crtc(priv->crtc, NULL);
-			priv->lcd_fck_rate = clk_get_rate(priv->clk);
-			tilcdc_crtc_update_clk(priv->crtc);
-			drm_modeset_unlock_crtc(priv->crtc);
-		}
-	}
+	if (val == CPUFREQ_POSTCHANGE)
+		tilcdc_crtc_update_clk(priv->crtc);
 
 	return 0;
 }

@@ -286,7 +280,6 @@ static int tilcdc_load(struct drm_device *dev, unsigned long flags)
 	}
 
 #ifdef CONFIG_CPU_FREQ
-	priv->lcd_fck_rate = clk_get_rate(priv->clk);
 	priv->freq_transition.notifier_call = cpufreq_transition;
 	ret = cpufreq_register_notifier(&priv->freq_transition,
 			CPUFREQ_TRANSITION_NOTIFIER);

drivers/gpu/drm/tilcdc/tilcdc_drv.h +0 -1

index a6e5e6d..9780c37 100644
@@ -74,7 +74,6 @@ struct tilcdc_drm_private {
 
 #ifdef CONFIG_CPU_FREQ
 	struct notifier_block freq_transition;
-	unsigned int lcd_fck_rate;
 #endif
 
 	struct workqueue_struct *wq;

Signature · 6 lines

1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Jyri Sarha

author

2016-09-06 20:59 UTC #3 lore raw

PATCH v2 3/5 drm/tilcdc: Flush flip-work workqueue before drm_flip_work_cleanup()

In reply to Jyri Sarha

Flush flip-work workqueue before drm_flip_work_cleanup(). It causes a nasty warning if there is unfinished flip-work in the queue when drm_flip_work_cleanup() is called. The flush_workqueue() has to be called before drm_crtc_cleanup() for unref_worker() to be able to do its job.

Signed-off-by Jyri Sarha <[email protected]>

---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/gpu/drm/tilcdc/tilcdc_crtc.c +4 -0

index f4e6a5b..6974624 100644
@@ -18,6 +18,7 @@
 #include "drm_flip_work.h"
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_atomic_helper.h>
+#include <linux/workqueue.h>
 
 #include "tilcdc_drv.h"
 #include "tilcdc_regs.h"

@@ -247,9 +248,12 @@ out:
 static void tilcdc_crtc_destroy(struct drm_crtc *crtc)
 {
 	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
+	struct tilcdc_drm_private *priv = crtc->dev->dev_private;
 
 	tilcdc_crtc_disable(crtc);
 
+	flush_workqueue(priv->wq);
+
 	of_node_put(crtc->port);
 	drm_crtc_cleanup(crtc);
 	drm_flip_work_cleanup(&tilcdc_crtc->unref_work);

Signature · 6 lines

1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Jyri Sarha

author

2016-09-06 20:59 UTC #4 lore raw

PATCH v2 4/5 drm/tilcdc: Remove unnecessary tilcdc_crtc_disable() from tilcdc_unload()

In reply to Jyri Sarha

Remove unnecessary tilcdc_crtc_disable() from tilcdc_unload(). The tilcdc_crtc_disable() called via tilcdc_crtc_destroy() by drm_mode_config_cleanup() couple of lines later.

The early call to tilcdc_crtc_disable() was a wrong fix (that worked) for calling drm_flip_work_cleanup() before flushing the flip-work queue.

Signed-off-by Jyri Sarha <[email protected]>

---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 2 --
 1 file changed, 2 deletions(-)

drivers/gpu/drm/tilcdc/tilcdc_drv.c +0 -2

index 52ff3e1..1981ae9 100644
@@ -200,8 +200,6 @@ static int tilcdc_unload(struct drm_device *dev)
 {
 	struct tilcdc_drm_private *priv = dev->dev_private;
 
-	tilcdc_crtc_disable(priv->crtc);
-
 	tilcdc_remove_external_encoders(dev);
 
 	drm_fbdev_cma_fini(priv->fbdev);

Signature · 6 lines

1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Jyri Sarha

author

2016-09-06 20:59 UTC #5 lore raw

PATCH v2 5/5 drm/tilcdc: WARN if CRTC is touched without CRTC lock

In reply to Jyri Sarha

WARN if CRTC is touched without CRTC lock. The crtc functions should not be called simultaneously from multiple threads. Having the DRM CRTC lock should take care of that.

tilcdc_crtc_destroy() has to take the CRTC lock befor calling tilcdc_crtc_disable() because drm_mode_config_cleanup() does not take the lock. If this ever changes the CRTC locking has to be removed from tilcdc_crtc_destroy().

Signed-off-by Jyri Sarha <[email protected]>

---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

drivers/gpu/drm/tilcdc/tilcdc_crtc.c +10 -0

index 6974624..94a78e2 100644
@@ -154,6 +154,8 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc)
 	struct drm_device *dev = crtc->dev;
 	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
 
+	WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
+
 	if (tilcdc_crtc->enabled)
 		return;

@@ -178,6 +180,8 @@ void tilcdc_crtc_disable(struct drm_crtc *crtc)
 	struct drm_device *dev = crtc->dev;
 	struct tilcdc_drm_private *priv = dev->dev_private;
 
+	WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
+
 	if (!tilcdc_crtc->enabled)
 		return;

@@ -250,7 +254,9 @@ static void tilcdc_crtc_destroy(struct drm_crtc *crtc)
 	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
 	struct tilcdc_drm_private *priv = crtc->dev->dev_private;
 
+	drm_modeset_lock_crtc(crtc, NULL);
 	tilcdc_crtc_disable(crtc);
+	drm_modeset_unlock_crtc(crtc);
 
 	flush_workqueue(priv->wq);

@@ -267,6 +273,8 @@ int tilcdc_crtc_update_fb(struct drm_crtc *crtc,
 	struct drm_device *dev = crtc->dev;
 	unsigned long flags;
 
+	WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
+
 	if (tilcdc_crtc->event) {
 		dev_err(dev->dev, "already pending page flip!\n");
 		return -EBUSY;

@@ -371,6 +379,8 @@ static void tilcdc_crtc_mode_set_nofb(struct drm_crtc *crtc)
 	struct drm_display_mode *mode = &crtc->state->adjusted_mode;
 	struct drm_framebuffer *fb = crtc->primary->state->fb;
 
+	WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
+
 	if (WARN_ON(!info))
 		return;

Signature · 6 lines

1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Tomi Valkeinen

2016-09-07 08:10 UTC #6 lore raw

Re: [PATCH v2 5/5] drm/tilcdc: WARN if CRTC is touched without CRTC lock

Quoted 8 lines · Jyri Sarha

WARN if CRTC is touched without CRTC lock. The crtc functions should not be called simultaneously from multiple threads. Having the DRM CRTC lock should take care of that.
tilcdc_crtc_destroy() has to take the CRTC lock befor calling tilcdc_crtc_disable() because drm_mode_config_cleanup() does not take the lock. If this ever changes the CRTC locking has to be removed from tilcdc_crtc_destroy().

Shouldn't these be two separate patches? This one is (according to subject) adding WARNs, but it is actually also adding locking.

Tomi

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel