drm/exynos: some small forgotten patch

drm/exynos: some small forgotten patch

[tjakobi]

Hello,

while looking through my local kernel tree, I noticed that this patch
for the mixer component of drm/exynos, posted some time ago, was
never applied. Since this is an obvious (and small) fix, could this
still go into 3.19?

With best wishes,
Tobias

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

[tjakobi]

From: Tomasz Stanislawski <t.stanislaws@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@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");
 }
-- 
2.0.4

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1322 bytes --]

On Sun, Nov 30, 2014 at 01:35:25AM +0100, tjakobi@math.uni-bielefeld.de wrote:
> From: Tomasz Stanislawski <t.stanislaws@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@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

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Tobias Jakobi]

On 2014-12-01 16:54, Thierry Reding wrote:
> On Sun, Nov 30, 2014 at 01:35:25AM +0100, tjakobi@math.uni-bielefeld.de 
> wrote:
>> From: Tomasz Stanislawski <t.stanislaws@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@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?

With best wishes,
Tobias

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3017 bytes --]

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@math.uni-bielefeld.de
> >wrote:
> >>From: Tomasz Stanislawski <t.stanislaws@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@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.

Then again, nobody seems to have cared so far...

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: 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@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@math.uni-bielefeld.de
>> >wrote:
>> >>From: Tomasz Stanislawski <t.stanislaws@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@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Tobias Jakobi]

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

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

[Seung-Woo Kim]

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
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Seung-Woo Kim
Samsung Software R&D Center
--

Re: drm/exynos: some small forgotten patch

[Inki Dae]

Hi,

On 2014년 11월 30일 09:35, tjakobi@math.uni-bielefeld.de wrote:
> Hello,
> 
> while looking through my local kernel tree, I noticed that this patch
> for the mixer component of drm/exynos, posted some time ago, was

That might be a patch I missed. Can you tell me the Subject?

Thanks,
Inki Dae


> never applied. Since this is an obvious (and small) fix, could this
> still go into 3.19?
> 
> With best wishes,
> Tobias
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: drm/exynos: some small forgotten patch

[Tobias Jakobi]

On 2014-12-09 13:09, Inki Dae wrote:
> That might be a patch I missed. Can you tell me the Subject?
Uhm, I think that is obvious from the mail?
http://www.spinics.net/lists/linux-samsung-soc/msg39811.html

Anyway, I'm still waiting for Thierry's reply on that matter.

With best wishes,
Tobias