| Message ID | e8921dc7096d0b9b336e9a1956d3d634801e56ac.1473148345.git.jsarha@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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);
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(-)