* [PATCH v2 1/5] drm/tilcdc: Take crtc modeset lock while updating the crtc clock rate
2016-09-06 20:59 [PATCH v2 0/5] drm/tilcdc: Fix cpufreq transition related race + cleanup Jyri Sarha
@ 2016-09-06 20:59 ` Jyri Sarha
2016-09-06 20:59 ` [PATCH v2 2/5] drm/tilcdc: Clean up LCDC functional clock rate setting code Jyri Sarha
` (3 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Jyri Sarha @ 2016-09-06 20:59 UTC (permalink / raw)
To: dri-devel; +Cc: Jyri Sarha, peter.ujfalusi, tomi.valkeinen, laurent.pinchart
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);
}
}
--
1.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v2 2/5] drm/tilcdc: Clean up LCDC functional clock rate setting code
2016-09-06 20:59 [PATCH v2 0/5] drm/tilcdc: Fix cpufreq transition related race + cleanup Jyri Sarha
2016-09-06 20:59 ` [PATCH v2 1/5] drm/tilcdc: Take crtc modeset lock while updating the crtc clock rate Jyri Sarha
@ 2016-09-06 20:59 ` Jyri Sarha
2016-09-06 20:59 ` [PATCH v2 3/5] drm/tilcdc: Flush flip-work workqueue before drm_flip_work_cleanup() Jyri Sarha
` (2 subsequent siblings)
4 siblings, 0 replies; 7+ messages in thread
From: Jyri Sarha @ 2016-09-06 20:59 UTC (permalink / raw)
To: dri-devel; +Cc: Jyri Sarha, peter.ujfalusi, tomi.valkeinen, laurent.pinchart
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;
--
1.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH v2 5/5] drm/tilcdc: WARN if CRTC is touched without CRTC lock
2016-09-06 20:59 [PATCH v2 0/5] drm/tilcdc: Fix cpufreq transition related race + cleanup Jyri Sarha
` (3 preceding siblings ...)
2016-09-06 20:59 ` [PATCH v2 4/5] drm/tilcdc: Remove unnecessary tilcdc_crtc_disable() from tilcdc_unload() Jyri Sarha
@ 2016-09-06 20:59 ` Jyri Sarha
2016-09-07 8:09 ` Tomi Valkeinen
4 siblings, 1 reply; 7+ messages in thread
From: Jyri Sarha @ 2016-09-06 20:59 UTC (permalink / raw)
To: dri-devel; +Cc: Jyri Sarha, peter.ujfalusi, tomi.valkeinen, laurent.pinchart
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;
--
1.9.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread