Skip to contentSkip to conversation

drm/tilcdc: Fix cpufreq transition related race + cleanup

PATCH Superseded Jyri Sarha 2 participants 10 messages Sep 6, 2016

Subsystems (3)

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Jyri Sarha

author
#0 lore raw

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

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 drm_mode_config mutex for the duration of the clock update.

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

The third patch should not really be needed, for now. However, tilcdc_crtc_enable() and -disable() are called from all over the place and relying on drm to only do one thing at the time may not work forever. Adding one mutex does not cost too much after all.

BR,
Jyri

Jyri Sarha (3):
drm/tilcdc: Take mode config lock while updating the crtc clock rate drm/tilcdc: Add tilcdc_crtc_set_clk() and cleanup cpufreq_transition() drm/tilcdc: Add mutex to protect crtc enable and disable routines

 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 93 +++++++++++++++++++++++-------------
 drivers/gpu/drm/tilcdc/tilcdc_drv.c  | 11 ++---
 drivers/gpu/drm/tilcdc/tilcdc_drv.h  |  3 +-
 3 files changed, 65 insertions(+), 42 deletions(-)
Signature · 6 lines
1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#1 lore raw

PATCH 1/3 drm/tilcdc: Take mode config lock while updating the crtc clock rate

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

---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 5 +++++
 drivers/gpu/drm/tilcdc/tilcdc_drv.h | 2 ++
 2 files changed, 7 insertions(+)
drivers/gpu/drm/tilcdc/tilcdc_drv.c +5 -0
index f8892e9..882d9b5 100644
@@ -184,10 +184,14 @@ static int cpufreq_transition(struct notifier_block *nb,
 {
 	struct tilcdc_drm_private *priv = container_of(nb,
 			struct tilcdc_drm_private, freq_transition);
+	struct drm_mode_config *config = &priv->dev->mode_config;
+
 	if (val == CPUFREQ_POSTCHANGE) {
 		if (priv->lcd_fck_rate != clk_get_rate(priv->clk)) {
+			mutex_lock(&config->mutex);
 			priv->lcd_fck_rate = clk_get_rate(priv->clk);
 			tilcdc_crtc_update_clk(priv->crtc);
+			mutex_unlock(&config->mutex);
 		}
 	}
 
@@ -251,6 +255,7 @@ static int tilcdc_load(struct drm_device *dev, unsigned long flags)
 	}
 
 	dev->dev_private = priv;
+	priv->dev = dev;
 
 	priv->is_componentized =
 		tilcdc_get_external_components(dev->dev, NULL) > 0;
drivers/gpu/drm/tilcdc/tilcdc_drv.h +2 -0
index a6e5e6d..6caecfc 100644
@@ -49,6 +49,8 @@
 struct tilcdc_drm_private {
 	void __iomem *mmio;
 
+	struct drm_device *dev;
+
 	struct clk *clk;         /* functional clock */
 	int rev;                 /* IP revision */
 
Signature · 6 lines
1.9.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#4 lore raw

PATCH 2/3 drm/tilcdc: Add tilcdc_crtc_set_clk() and cleanup cpufreq_transition()

In reply to Jyri Sarha

Add tilcdc_crtc_set_clk() and cleanup cpufreq_transition(). The new tilcdc_crtc_set_clk() is used in tilcdc_crtc_mode_set_nofb() instead tilcdc_crtc_update_clk(). New tilcdc_crtc_update_clk() is implemented using tilcdc_crtc_set_clk() for cpufreq_transition() alone.

---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 78 +++++++++++++++++++++---------------
 drivers/gpu/drm/tilcdc/tilcdc_drv.c  | 14 ++-----
 drivers/gpu/drm/tilcdc/tilcdc_drv.h  |  1 -
 3 files changed, 49 insertions(+), 44 deletions(-)
drivers/gpu/drm/tilcdc/tilcdc_crtc.c +46 -32
index 84b36fd..f9e3da9 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,22 @@ 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);
+	struct drm_mode_config *config = &priv->dev->mode_config;
 
-	pm_runtime_get_sync(dev->dev);
+	if (tilcdc_crtc->lcd_fck_rate != clk_get_rate(priv->clk)) {
+		mutex_lock(&config->mutex);
+		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);
+		}
+		mutex_unlock(&config->mutex);
 	}
-
-	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);
 }
 
 #define SYNC_LOST_COUNT_LIMIT 50
drivers/gpu/drm/tilcdc/tilcdc_drv.c +3 -11
index 882d9b5..4af58b7 100644
@@ -184,16 +184,9 @@ static int cpufreq_transition(struct notifier_block *nb,
 {
 	struct tilcdc_drm_private *priv = container_of(nb,
 			struct tilcdc_drm_private, freq_transition);
-	struct drm_mode_config *config = &priv->dev->mode_config;
-
-	if (val == CPUFREQ_POSTCHANGE) {
-		if (priv->lcd_fck_rate != clk_get_rate(priv->clk)) {
-			mutex_lock(&config->mutex);
-			priv->lcd_fck_rate = clk_get_rate(priv->clk);
-			tilcdc_crtc_update_clk(priv->crtc);
-			mutex_unlock(&config->mutex);
-		}
-	}
+
+	if (val == CPUFREQ_POSTCHANGE)
+		tilcdc_crtc_update_clk(priv->crtc);
 
 	return 0;
 }
@@ -288,7 +281,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 6caecfc..fac0733 100644
@@ -76,7 +76,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
author
#7 lore raw

PATCH 3/3 drm/tilcdc: Add mutex to protect crtc enable and disable routines

In reply to Jyri Sarha

Add mutex to protect crtc enable and disable routines. The tilcdc_crtc_disable() function waits for frame done interrupt, the internal data will get out of sync, should another enable arrive while waiting for the interrupt.

---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)
drivers/gpu/drm/tilcdc/tilcdc_crtc.c +13 -2
index f9e3da9..ac0a63e 100644
@@ -31,6 +31,7 @@ struct tilcdc_crtc {
 	const struct tilcdc_panel_info *info;
 	struct drm_pending_vblank_event *event;
 	bool enabled;
+	struct mutex enable_lock;
 	wait_queue_head_t frame_done_wq;
 	bool frame_done;
 	spinlock_t irq_lock;
@@ -153,8 +154,10 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc)
 	struct drm_device *dev = crtc->dev;
 	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
 
+	mutex_lock(&tilcdc_crtc->enable_lock);
+
 	if (tilcdc_crtc->enabled)
-		return;
+		goto out;
 
 	pm_runtime_get_sync(dev->dev);
 
@@ -169,6 +172,8 @@ static void tilcdc_crtc_enable(struct drm_crtc *crtc)
 	drm_crtc_vblank_on(crtc);
 
 	tilcdc_crtc->enabled = true;
+out:
+	mutex_unlock(&tilcdc_crtc->enable_lock);
 }
 
 void tilcdc_crtc_disable(struct drm_crtc *crtc)
@@ -177,8 +182,10 @@ void tilcdc_crtc_disable(struct drm_crtc *crtc)
 	struct drm_device *dev = crtc->dev;
 	struct tilcdc_drm_private *priv = dev->dev_private;
 
+	mutex_lock(&tilcdc_crtc->enable_lock);
+
 	if (!tilcdc_crtc->enabled)
-		return;
+		goto out;
 
 	tilcdc_crtc->frame_done = false;
 	tilcdc_clear(dev, LCDC_RASTER_CTRL_REG, LCDC_RASTER_ENABLE);
@@ -218,6 +225,8 @@ void tilcdc_crtc_disable(struct drm_crtc *crtc)
 	tilcdc_crtc->last_vblank = ktime_set(0, 0);
 
 	tilcdc_crtc->enabled = false;
+out:
+	mutex_unlock(&tilcdc_crtc->enable_lock);
 }
 
 static bool tilcdc_crtc_is_on(struct drm_crtc *crtc)
@@ -819,6 +828,8 @@ struct drm_crtc *tilcdc_crtc_create(struct drm_device *dev)
 	drm_flip_work_init(&tilcdc_crtc->unref_work,
 			"unref", unref_worker);
 
+	mutex_init(&tilcdc_crtc->enable_lock);
+
 	spin_lock_init(&tilcdc_crtc->irq_lock);
 	INIT_WORK(&tilcdc_crtc->recover_work, tilcdc_crtc_recover_work);
 
Signature · 6 lines
1.9.1

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

Re: [PATCH 1/3] drm/tilcdc: Take mode config lock while updating the crtc clock rate

In reply to Jyri Sarha
Quoted 15 lines · Jyri Sarha

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

---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 5 +++++
 drivers/gpu/drm/tilcdc/tilcdc_drv.h | 2 ++
 2 files changed, 7 insertions(+)
drivers/gpu/drm/tilcdc/tilcdc_drv.c +0 -0
index f8892e9..882d9b5 100644

drivers/gpu/drm/tilcdc/tilcdc_drv.c static int cpufreq_transition(struct notifier_block *nb, · line 191

Quoted 8 lines
drivers/gpu/drm/tilcdc/tilcdc_drv.c +3 -0
@@ -184,10 +184,14 @@ static int cpufreq_transition(struct notifier_block *nb,
 {
 	struct tilcdc_drm_private *priv = container_of(nb,
 			struct tilcdc_drm_private, freq_transition);
+	struct drm_mode_config *config = &priv->dev->mode_config;
+
 	if (val == CPUFREQ_POSTCHANGE) {
 		if (priv->lcd_fck_rate != clk_get_rate(priv->clk)) {
+			mutex_lock(&config->mutex);

drm_modeset_lock_crtc()? Or drm_modeset_lock_all() if per-crtc is not suitable.

Tomi

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

#8 lore raw

Re: [PATCH 3/3] drm/tilcdc: Add mutex to protect crtc enable and disable routines

In reply to Jyri Sarha
Quoted 4 lines · Jyri Sarha

Add mutex to protect crtc enable and disable routines. The tilcdc_crtc_disable() function waits for frame done interrupt, the internal data will get out of sync, should another enable arrive while waiting for the interrupt.

Why doesn't the per-crtc modeset lock work for this?

Tomi

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

#5 lore raw

Re: [PATCH 2/3] drm/tilcdc: Add tilcdc_crtc_set_clk() and cleanup cpufreq_transition()

In reply to Jyri Sarha
Quoted 4 lines · Jyri Sarha

Add tilcdc_crtc_set_clk() and cleanup cpufreq_transition(). The new tilcdc_crtc_set_clk() is used in tilcdc_crtc_mode_set_nofb() instead tilcdc_crtc_update_clk(). New tilcdc_crtc_update_clk() is implemented using tilcdc_crtc_set_clk() for cpufreq_transition() alone.

I think the patch looks ok, but the description doesn't help too much in understanding what this does. It gives no hint on what tilcdc_crtc_set_clk() is, what's its relation to tilcdc_crtc_update_clk(), and what's the overall purpose here.

If I have to read the code to understand the reason for the patch, the description has failed =).

Tomi

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

#6 lore raw

Re: [PATCH 2/3] drm/tilcdc: Add tilcdc_crtc_set_clk() and cleanup cpufreq_transition()

In reply to Jyri Sarha

drivers/gpu/drm/tilcdc/tilcdc_crtc.c static bool tilcdc_crtc_mode_fixup(struct drm_crtc *crtc, · line 334

Quoted 6 lines · Jyri Sarha
drivers/gpu/drm/tilcdc/tilcdc_crtc.c +6 -0
+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 */

Btw, not related to this patch, but you may want to revisit this hardcoded divider at some point. I don't remember the history, but it was a quick solution to some issues. Supporting any divider the HW allows would probably give us better clock rates.

Well, except if the source clock comes right from a dedicated PLL, which supports the whole pixel clock range. If that's the case, then the LCDC divider doesn't help much.

Tomi

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

author
#3 lore raw

Re: [PATCH 1/3] drm/tilcdc: Take mode config lock while updating the crtc clock rate

In reply to Tomi Valkeinen
Quoted 13 lines · Tomi Valkeinen
Quoted 10 lines · Jyri Sarha

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

---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 5 +++++
 drivers/gpu/drm/tilcdc/tilcdc_drv.h | 2 ++
 2 files changed, 7 insertions(+)
Quoted 17 lines

drivers/gpu/drm/tilcdc/tilcdc_drv.c static int cpufreq_transition(struct notifier_block *nb, · line 191

Quoted 13 lines
drivers/gpu/drm/tilcdc/tilcdc_drv.c +3 -0
index f8892e9..882d9b5 100644
@@ -184,10 +184,14 @@ static int cpufreq_transition(struct notifier_block *nb,
 {
 	struct tilcdc_drm_private *priv = container_of(nb,
 			struct tilcdc_drm_private, freq_transition);
+	struct drm_mode_config *config = &priv->dev->mode_config;
+
 	if (val == CPUFREQ_POSTCHANGE) {
 		if (priv->lcd_fck_rate != clk_get_rate(priv->clk)) {
+			mutex_lock(&config->mutex);

drm_modeset_lock_crtc()? Or drm_modeset_lock_all() if per-crtc is not suitable.

I guess that should work too, all I need is just to make sure no one calls mode_set_nofb() while the clock is updated.

I just thought that since I am not actually touching drm state at all the back off mechanisms etc add just unnecessary complexity. But now after reading a little bit drm locking code, for sure the mode config mutex is not the light weight lock I was looking for.

One solution would be adding tilcdc internal mutex to synchronize mode_set_nofb() and cpufreq_transition(), but after all it is probably better to use the mechanisms that are already there and just use drm_modeset_lock_crtc() despite its apparent complexity.

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

author
#9 lore raw

Re: [PATCH 3/3] drm/tilcdc: Add mutex to protect crtc enable and disable routines

In reply to Tomi Valkeinen
Quoted 8 lines · Tomi Valkeinen
Quoted 4 lines · Jyri Sarha

Add mutex to protect crtc enable and disable routines. The tilcdc_crtc_disable() function waits for frame done interrupt, the internal data will get out of sync, should another enable arrive while waiting for the interrupt.

Why doesn't the per-crtc modeset lock work for this?

I am not worried about DRM enabling and disabling the crtc, it should take the locks it needs on its own, and AFAIU the driver should not try take the same locks again.

The purpose of these locks is to protect underneath workings the LCDC driver reacting to something happening outside the DRM, like CPU frequency change or module unloading.

I considered dropping this patch already last night (it was my first attempt to fix the race, and it worked too), but then decided to put it under review just for the sake of argument ;).

All in all it should be enough to take all the necessary DRM locks when fiddling with lcdc hw and not to implement any extra layers of locking.

I'll drop this patch.

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