Sorry for late. 2014-12-17 16:54 GMT+09:00 Thierry Reding <thierry.reding@xxxxxxxxx>: > 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. The use of spin lock, reg_slock, has been used for a long time and we hadn't some cleanups to spin lock codes so far. The spin lock is also used in here and there of mixer driver. And at least, it seems that the use of spin lock isn't required in mixer_win_reset. I don't see any atomic contexts in mixer module except interrupt handler. To Seung-Woo, I know that you referred to mixer codes of v4l2 based mixer driver. So was the spin lock used in origin v4l2 driver? or Is there any reason that you used the spin lock? Anyway, we will have some testing to check hdmi and mixer drivers without spin lock. So we will remove or replace it with mutex if needed. Thanks, Inki Dae > > Then again, nobody seems to have cared so far... > > Thierry > > _______________________________________________ > dri-devel mailing list > dri-devel@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/dri-devel > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |