On Mon, Dec 01, 2014 at 05:16:17PM +0100, Tobias Jakobi wrote: > On 2014-12-01 16:54, Thierry Reding wrote: > >On Sun, Nov 30, 2014 at 01:35:25AM +0100, tjakobi@xxxxxxxxxxxxxxxxxxxxx > >wrote: > >>From: Tomasz Stanislawski <t.stanislaws@xxxxxxxxxxx> > >> > >>This patch fixes calling usleep_range() after taking reg_slock > >>using spin_lock_irqsave(). The mdelay() is used instead. > >>Waiting in atomic context is not the best idea in general. > >>Hopefully, waiting occurs only when Video Processor fails > >>to reset correctly. > >> > >>Signed-off-by: Tomasz Stanislawski <t.stanislaws@xxxxxxxxxxx> > >>--- > >> drivers/gpu/drm/exynos/exynos_mixer.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >>diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c > >>b/drivers/gpu/drm/exynos/exynos_mixer.c > >>index a41c84e..cc7cccc 100644 > >>--- a/drivers/gpu/drm/exynos/exynos_mixer.c > >>+++ b/drivers/gpu/drm/exynos/exynos_mixer.c > >>@@ -632,7 +632,7 @@ static void vp_win_reset(struct mixer_context *ctx) > >> /* waiting until VP_SRESET_PROCESSING is 0 */ > >> if (~vp_reg_read(res, VP_SRESET) & VP_SRESET_PROCESSING) > >> break; > >>- usleep_range(10000, 12000); > >>+ mdelay(10); > >> } > >> WARN(tries == 0, "failed to reset Video Processor\n"); > >> } > > > >I can't see a reason why you would need to hold the lock around this > >code. Perhaps a better way to fix this would be to drop the lock before > >calling vp_win_reset()? > > > >Thierry > > Hmm, I'm pretty new to spinlocks (only have worked with the usual mutex > stuff in userspace), but wouldn't that mean that it is then possible for > mixer_win_reset to execute while a (previous) vp_win_reset is still running? Indeed it would. I didn't look properly. Looking more closely it seems the call stack for this looks something like: vp_win_reset() mixer_win_reset() mixer_poweron() mixer_dpms() exynos_drm_crtc_dpms() Which can then be called from two places: exynos_drm_crtc_commit() drm_crtc_helper_set_mode() drm_crtc_helper_set_config() drm_helper_connector_dpms() drm_crtc_helper_set_config() drm_crtc_helper_set_config() itself must be called with the all modeset locks held, so I don't see a way how vp_win_reset() could be called concurrently. Anyway, even if you're still concerned about concurrent accesses to the register you'd better lock this section with a mutex to avoid excessive spinning. In fact I think a better option would be to extend the mutex in mixer_poweron() to encompass the whole function. This currently looks broken because one process could go to sleep in pm_runtime_get_sync() or clk_prepare_enable() and another process start running mixer_poweron() concurrently, getting to the second mutex_lock() sooner than the first process. So the lock being dropped between checking for ctx->powered and setting it doesn't actually prevent a race. Then again, nobody seems to have cared so far... Thierry
Attachment:
pgpwD4yG4QZkD.pgp
Description: PGP signature
