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(-)
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 jsarha@ti.com --- drivers/gpu/drm/tilcdc/tilcdc_drv.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index f8892e9..b1ac61e 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c @@ -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); } }
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 jsarha@ti.com --- 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(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index 84b36fd..f4e6a5b 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -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 diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index b1ac61e..52ff3e1 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c @@ -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); diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h index a6e5e6d..9780c37 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h @@ -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;
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 jsarha@ti.com --- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index f4e6a5b..6974624 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -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);
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 jsarha@ti.com --- drivers/gpu/drm/tilcdc/tilcdc_drv.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c index 52ff3e1..1981ae9 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c @@ -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);
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 jsarha@ti.com --- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index 6974624..94a78e2 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -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;
On 06/09/16 23:59, Jyri Sarha wrote:
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@lists.freedesktop.org
-
Jyri Sarha -
Tomi Valkeinen