drm: exynos: mixer: fix using usleep() in atomic context

[Inki Dae]

Sorry for late.


2014-12-17 16:54 GMT+09:00 Thierry Reding <thierry.reding at gmail.com>:
> 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 at math.uni-bielefeld.de
>> >wrote:
>> >>From: Tomasz Stanislawski <t.stanislaws at samsung.com>
>> >>
>> >>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 at samsung.com>
>> >>---
>> >> 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>