drm/tilcdc: Fix cpufreq transition related race + cleanu
Subsystems (3)
v3 cover drm/tilcdc: Fix cpufreq transition related race + cleanu
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(-)
Signature · 6 lines
1.9.1 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH v3 1/6 drm/tilcdc: Take crtc modeset lock while updating the crtc clock rate
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.
--- drivers/gpu/drm/tilcdc/tilcdc_drv.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/gpu/drm/tilcdc/tilcdc_drv.c +3 -0
@@ -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); } }
Signature · 6 lines
1.9.1 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH v3 2/6 drm/tilcdc: Clean up LCDC functional clock rate setting code
In reply to Jyri SarhaClean 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.
--- 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(-)
drivers/gpu/drm/tilcdc/tilcdc_crtc.c +45 -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,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
drivers/gpu/drm/tilcdc/tilcdc_drv.c +2 -9
@@ -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 );
drivers/gpu/drm/tilcdc/tilcdc_drv.h +0 -1
@@ -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;
Signature · 6 lines
1.9.1 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH v3 3/6 drm/tilcdc: Flush flip-work workqueue before drm_flip_work_cleanup()
In reply to Jyri SarhaFlush 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.
--- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/gpu/drm/tilcdc/tilcdc_crtc.c +4 -0
@@ -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);
Signature · 6 lines
1.9.1 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH v3 4/6 drm/tilcdc: Remove unnecessary tilcdc_crtc_disable() from tilcdc_unload()
In reply to Jyri SarhaRemove 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.
--- drivers/gpu/drm/tilcdc/tilcdc_drv.c | 2 -- 1 file changed, 2 deletions(-)
drivers/gpu/drm/tilcdc/tilcdc_drv.c +0 -2
@@ -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);
Signature · 6 lines
1.9.1 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH v3 5/6 drm/tilcdc: Take CRTC lock when calling tilcdc_crtc_disable()
In reply to Jyri SarhaTake 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.
--- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/gpu/drm/tilcdc/tilcdc_crtc.c +2 -0
@@ -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);
Signature · 6 lines
1.9.1 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH v3 6/6 drm/tilcdc: WARN if CRTC is touched without CRTC lock
In reply to Jyri SarhaWARN 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.
--- drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 8 ++++++++ 1 file changed, 8 insertions(+)
drivers/gpu/drm/tilcdc/tilcdc_crtc.c +8 -0
@@ -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 ;
Signature · 6 lines
1.9.1 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 39 lines · Jyri Sarha
Changes since v2:
- Fix typo from "drm/tilcdc: Clean up LCDC functional clock rate setting code" description
- Split "drm/tilcdc: Take CRTC lock when callingtilcdc_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: Addtilcdc_crtc_set_clk()and cleanupcpufreq_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 beforedrm_flip_work_cleanup()"
- Added "drm/tilcdc: Remove unnecessarytilcdc_crtc_disable()fromtilcdc_unload()"
- Added "drm/tilcdc: WARN if CRTC is touched without CRTC lock"There was a race between
mode_set_nofb()andcpufreq_transition()callingtilcdc_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,
JyriJyri 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 beforedrm_flip_work_cleanup()drm/tilcdc: Remove unnecessarytilcdc_crtc_disable()from
tilcdc_unload()
drm/tilcdc: Take CRTC lock when callingtilcdc_crtc_disable()drm/tilcdc: WARN if CRTC is touched without CRTC lock
Quoted 5 lines
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(-)
Tomi
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel