Changes since v2: - Fix typo from "drm/tilcdc: Clean up LCDC functional clock rate setting code" description - Split "drm/tilcdc: Take CRTC lock when calling tilcdc_crtc_disable()" out of "drm/tilcdc: WARN if CRTC is touched without CRTC lock"
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 (6): 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: Take CRTC lock when calling tilcdc_crtc_disable() 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 setting 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);
Take CRTC lock when calling tilcdc_crtc_disable() in tilcdc_crtc_destroy().
In theory there could still be some operation ongoing, which should finish before destroying the CRTC. However, the main reason for adding this is to be able to add WARNing in tilcdc_crtc_disable() if CRTC is not locked.
Signed-off-by: Jyri Sarha jsarha@ti.com --- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index 6974624..2763f9f 100644 --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c @@ -250,7 +250,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);
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.
Signed-off-by: Jyri Sarha jsarha@ti.com --- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c index 2763f9f..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;
@@ -269,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; @@ -373,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 07/09/16 11:59, Jyri Sarha wrote:
Changes since v2:
- Fix typo from "drm/tilcdc: Clean up LCDC functional clock rate setting code" description
- Split "drm/tilcdc: Take CRTC lock when calling tilcdc_crtc_disable()" out of "drm/tilcdc: WARN if CRTC is touched without CRTC lock"
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 (6): 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: Take CRTC lock when calling tilcdc_crtc_disable() 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(-)
Reviewed-by: Tomi Valkeinen tomi.valkeinen@ti.com
Tomi
dri-devel@lists.freedesktop.org
-
Jyri Sarha -
Tomi Valkeinen