[PATCH 0/3] drm/tilcdc: Fix cpufreq transition related race + cleanup

[PATCH 0/3] drm/tilcdc: Fix cpufreq transition related race + cleanup

[Jyri Sarha]

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(-)

-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/3] drm/tilcdc: Take mode config lock while updating the crtc clock rate

[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 <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_drv.c | 5 +++++
 drivers/gpu/drm/tilcdc/tilcdc_drv.h | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index f8892e9..882d9b5 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -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;
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
index a6e5e6d..6caecfc 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
@@ -49,6 +49,8 @@
 struct tilcdc_drm_private {
 	void __iomem *mmio;
 
+	struct drm_device *dev;
+
 	struct clk *clk;         /* functional clock */
 	int rev;                 /* IP revision */
 
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/tilcdc: Take mode config lock while updating the crtc clock rate

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 1114 bytes --]



On 06/09/16 11:19, Jyri Sarha wrote:
> 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 <jsarha@ti.com>
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c | 5 +++++
>  drivers/gpu/drm/tilcdc/tilcdc_drv.h | 2 ++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> index f8892e9..882d9b5 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/tilcdc: Take mode config lock while updating the crtc clock rate

[Jyri Sarha]

On 09/06/16 12:07, Tomi Valkeinen wrote:
> 
> 
> On 06/09/16 11:19, Jyri Sarha wrote:
>> 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 <jsarha@ti.com>
>> ---
>>  drivers/gpu/drm/tilcdc/tilcdc_drv.c | 5 +++++
>>  drivers/gpu/drm/tilcdc/tilcdc_drv.h | 2 ++
>>  2 files changed, 7 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
>> index f8892e9..882d9b5 100644
>> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
>> @@ -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.
> 

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
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm/tilcdc: Add tilcdc_crtc_set_clk() and cleanup cpufreq_transition()

[Jyri Sarha]

Add 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.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 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(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 84b36fd..f9e3da9 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,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
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index 882d9b5..4af58b7 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -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);
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
index 6caecfc..fac0733 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
@@ -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;
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/tilcdc: Add tilcdc_crtc_set_clk() and cleanup cpufreq_transition()

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 676 bytes --]

On 06/09/16 11:19, Jyri Sarha wrote:
> Add 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.

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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/tilcdc: Add tilcdc_crtc_set_clk() and cleanup cpufreq_transition()

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 775 bytes --]

On 06/09/16 11:19, Jyri Sarha wrote:

> +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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/tilcdc: Add mutex to protect crtc enable and disable routines

[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.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index f9e3da9..ac0a63e 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -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);
 
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/tilcdc: Add mutex to protect crtc enable and disable routines

[Tomi Valkeinen]

[-- Attachment #1.1.1: Type: text/plain, Size: 341 bytes --]

On 06/09/16 11:19, Jyri Sarha wrote:
> 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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/tilcdc: Add mutex to protect crtc enable and disable routines

[Jyri Sarha]

On 09/06/16 12:30, Tomi Valkeinen wrote:
> On 06/09/16 11:19, Jyri Sarha wrote:
>> 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
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel