drm/tilcdc: Fix cpufreq transition related race + cleanup
Subsystems (3)
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
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
@@ -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
@@ -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
PATCH 2/3 drm/tilcdc: Add tilcdc_crtc_set_clk() and cleanup cpufreq_transition()
In reply to Jyri SarhaAdd 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
@@ -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
@@ -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
@@ -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
PATCH 3/3 drm/tilcdc: Add mutex to protect crtc enable and disable routines
In reply to Jyri SarhaAdd 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
@@ -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
Re: [PATCH 1/3] drm/tilcdc: Take mode config lock while updating the crtc clock rate
In reply to Jyri SarhaQuoted 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.Signed-off-by Jyri Sarha <[email protected]>--- 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
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
Re: [PATCH 3/3] drm/tilcdc: Add mutex to protect crtc enable and disable routines
In reply to Jyri SarhaQuoted 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
Re: [PATCH 2/3] drm/tilcdc: Add tilcdc_crtc_set_clk() and cleanup cpufreq_transition()
In reply to Jyri SarhaQuoted 4 lines · Jyri Sarha
Add
tilcdc_crtc_set_clk()and cleanupcpufreq_transition(). The newtilcdc_crtc_set_clk()is used intilcdc_crtc_mode_set_nofb()insteadtilcdc_crtc_update_clk(). Newtilcdc_crtc_update_clk()is implemented usingtilcdc_crtc_set_clk()forcpufreq_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
Re: [PATCH 2/3] drm/tilcdc: Add tilcdc_crtc_set_clk() and cleanup cpufreq_transition()
In reply to Jyri Sarhadrivers/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
Re: [PATCH 1/3] drm/tilcdc: Take mode config lock while updating the crtc clock rate
In reply to Tomi ValkeinenQuoted 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.Signed-off-by Jyri Sarha <[email protected]>--- 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@@ -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()? Ordrm_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
Re: [PATCH 3/3] drm/tilcdc: Add mutex to protect crtc enable and disable routines
In reply to Tomi ValkeinenQuoted 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