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

Commit Message

Tobias Jakobi Nov. 30, 2014, 12:35 a.m. UTC

From: Tomasz Stanislawski <[e-mail]>

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 <[e-mail]>
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Thierry Reding Dec. 1, 2014, 3:54 p.m. UTC | #1

On Sun, Nov 30, 2014 at 01:35:25AM +0100, [e-mail] wrote:
> From: Tomasz Stanislawski <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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

Tobias Jakobi Dec. 1, 2014, 4:16 p.m. UTC | #2

On 2014-12-01 16:54, Thierry Reding wrote:
> On Sun, Nov 30, 2014 at 01:35:25AM +0100, [e-mail] 
> wrote:
>> From: Tomasz Stanislawski <[e-mail]>
>> 
>> 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 <[e-mail]>
>> ---
>>  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?

With best wishes,
Tobias

Thierry Reding Dec. 17, 2014, 7:54 a.m. UTC | #3

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, [e-mail]
> >wrote:
> >>From: Tomasz Stanislawski <[e-mail]>
> >>
> >>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 <[e-mail]>
> >>---
> >> 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

Inki Dae Dec. 21, 2014, 3:04 p.m. UTC | #4

Sorry for late.


2014-12-17 16:54 GMT+09:00 Thierry Reding <[e-mail]>:
> 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, [e-mail]
>> >wrote:
>> >>From: Tomasz Stanislawski <[e-mail]>
>> >>
>> >>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 <[e-mail]>
>> >>---
>> >> 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
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Tobias Jakobi Jan. 21, 2015, 10:46 p.m. UTC | #5

Hello!


Inki Dae wrote:
> 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

So it's some weeks later and as far as I can see there has been no
changes to the spinlock usage. Wouldn't it be better to apply this patch
_now_ (since the use of 'usleep_range' is just plain wrong while under
spinlock). When the spinlock setup gets cleaned up later, then we can
always change back to 'usleep_range' again.

Any thoughts?

With best wishes,
Tobias

Seung-Woo Kim Jan. 22, 2015, 7:47 a.m. UTC | #6

Hello,

On 2015? 01? 22? 07:46, Tobias Jakobi wrote:
> Hello!
> 
> 
> Inki Dae wrote:
>> 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?

The spinlock usage was originated from Tomasz Stanislawski's s5p-tv.

>>
>> 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
> 
> So it's some weeks later and as far as I can see there has been no
> changes to the spinlock usage. Wouldn't it be better to apply this patch
> _now_ (since the use of 'usleep_range' is just plain wrong while under
> spinlock). When the spinlock setup gets cleaned up later, then we can
> always change back to 'usleep_range' again.
> 
> Any thoughts?

In s5p-tv, same patch is already applied by Tomasz, so I agree to apply
this patch also.

Best Regards,
- Seung-Woo Kim

> 
> With best wishes,
> Tobias
> 
> _______________________________________________
> dri-devel mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Patch

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");
 }