938809 - Run HAL FM radio commands on a separate thread

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Michael Wu [:mwu]]

For many standard V4L2 FM radio drivers, ioctl calls can stall for some time. We should try to move as much as possible to a separate thread. Not sure if this should be done on the HAL side or the dom/fmradio side yet, but I'll file in DOM for now.

Comment 1

[Michael Wu [:mwu]]

Attachment: Put seek and tuning on a separate thread

Should work, though I need to test on dolphin/tarako to be sure.

Comment 2

[Michael Wu [:mwu]]

Attachment: Support calling seeking/tuning functions off main thread in HAL

This allows seeking/tuning to be called from a separate thread. I have another patch which actually does that, but it's on the dom side.

Comment 3

[Michael Wu [:mwu]]

Attachment: Run seek/tune calls off the main thread

This patch runs the HAL seek/tune calls on a separate thread.

Comment 4

[Dave Hylands [:dhylands]]

Comment on attachment 8455638 [details] [diff] [review]
Support calling seeking/tuning functions off main thread in HAL

Review of attachment 8455638 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

::: hal/Hal.h
@@ +525,5 @@
>  /**
>   * Seek to an available FM radio station.
>   *
> + * This can be called off main thread, but all calls must be completed
> + * before calling DisableFMRadio.

Shouldn't NotifyFMRadioStatus check to see if the radio has been disabled or not?

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8455719 [details] [diff] [review]
Run seek/tune calls off the main thread

I would like yxl to look at this before I do.

Please use NS_NewNamedThread and name this the FM Radio thread.

Comment 6

[Michael Wu [:mwu]]

Comment on attachment 8455638 [details] [diff] [review]
Support calling seeking/tuning functions off main thread in HAL

Review of attachment 8455638 [details] [diff] [review]:
-----------------------------------------------------------------

::: hal/Hal.h
@@ +525,5 @@
>  /**
>   * Seek to an available FM radio station.
>   *
> + * This can be called off main thread, but all calls must be completed
> + * before calling DisableFMRadio.

So frequency updates don't show up after disabling?

Comment 7

[Michael Wu [:mwu]]

Attachment: Run seek/tune calls off the main thread, v2

Now with NS_NewNamedThread.

Comment 8

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 8455847 [details] [diff] [review]
Run seek/tune calls off the main thread, v2

Review of attachment 8455847 [details] [diff] [review]:
-----------------------------------------------------------------

Forward review request to Pin as he is owner of FM radio module and much familar with the related code. If you need additional info from me, please feel free to ping me:)

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Ugh, sorry.  That's the second time I've done that.

Comment 10

[Dave Hylands [:dhylands]]

(In reply to Michael Wu [:mwu] from comment #6)
> Comment on attachment 8455638 [details] [diff] [review]
> Support calling seeking/tuning functions off main thread in HAL
> 
> Review of attachment 8455638 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: hal/Hal.h
> @@ +525,5 @@
> >  /**
> >   * Seek to an available FM radio station.
> >   *
> > + * This can be called off main thread, but all calls must be completed
> > + * before calling DisableFMRadio.
> 
> So frequency updates don't show up after disabling?

You said that all calls must be completed before calling DisableFMRadio.

In the event that the calls weren't completed couldn't NotifyFMRadioStatus just not do the notification if the radio was disabled? Alternatively the RadioUpdate runnable could do the check as well.

I was mostly just wondering what the implications of the comment were and how the caller makes sure that the call is completed before disabling the radio.

Comment 11

[Pin Zhang [:pzhang] (inactive)]

Comment on attachment 8455847 [details] [diff] [review]
Run seek/tune calls off the main thread, v2

Review of attachment 8455847 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

@mwu, may i ask why not dispatch EnableRunnable/DisableRunnable in a separate thread too?

::: dom/fmradio/FMRadioService.cpp
@@ +137,5 @@
>  
> +    FMRadioService* fmRadioService = FMRadioService::Singleton();
> +    if (!fmRadioService->mTuneThread) {
> +      NS_NewNamedThread("FM Tuning", getter_AddRefs(fmRadioService->mTuneThread));
> +    }

Could you please add comments about why we need a thread? Thanks.

Comment 12

[Michael Wu [:mwu]]

(In reply to Dave Hylands [:dhylands] from comment #10)
> You said that all calls must be completed before calling DisableFMRadio.
> 
> In the event that the calls weren't completed couldn't NotifyFMRadioStatus
> just not do the notification if the radio was disabled? Alternatively the
> RadioUpdate runnable could do the check as well.
> 
> I was mostly just wondering what the implications of the comment were and
> how the caller makes sure that the call is completed before disabling the
> radio.

By call is completed, I mean FMRadioSeek and SetFMRadioFrequency need to have returned before calling DisableFMRadio. If these are called on a separate thread, the caller can stop the event loop and join the thread to ensure that these calls are completed before calling DisableFMRadio. We don't want FMRadioSeek and SetFMRadioFrequency running while we disable the radio since we change sRadioFD while disabling.

Comment 13

[Michael Wu [:mwu]]

(In reply to Pin Zhang [:pzhang] from comment #11)
> Comment on attachment 8455847 [details] [diff] [review]
> Run seek/tune calls off the main thread, v2
> 
> Review of attachment 8455847 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me
> 
> @mwu, may i ask why not dispatch EnableRunnable/DisableRunnable in a
> separate thread too?
> 

We might be able to, but I think it might be more complicated to ensure safety. We can call enable/disable/tune/seek on a separate thread, but there's also a number of calls that are synchronous on the main thread. For example, GetFMRadioFrequency(). We would have to make sure that the radio isn't enabled or disabled while GetFMRadioFrequency is called, or make it safe to call that function while the radio is being enabled or disabled.

> ::: dom/fmradio/FMRadioService.cpp
> @@ +137,5 @@
> >  
> > +    FMRadioService* fmRadioService = FMRadioService::Singleton();
> > +    if (!fmRadioService->mTuneThread) {
> > +      NS_NewNamedThread("FM Tuning", getter_AddRefs(fmRadioService->mTuneThread));
> > +    }
> 
> Could you please add comments about why we need a thread? Thanks.

Will do.

Comment 14

[Michael Wu [:mwu]]

Attachment: Run seek/tune calls off the main thread, v3

Comment about the fm tuning thread added.

Comment 15

[Michael Wu [:mwu]]

Attachment: Run seek/tune calls off the main thread, v4

This adds a null check for mTuneThread in the DisableRunnable. Guess we can end up dispatching DisableRunnable twice when quitting the fm radio while playing.

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8459887 [details] [diff] [review]
Run seek/tune calls off the main thread, v4

Review of attachment 8459887 [details] [diff] [review]:
-----------------------------------------------------------------

I'm slightly concerned about the shutdown sequence here ... but I guess we can investigate that separately.

Comment 17

[Michael Wu [:mwu]]

https://hg.mozilla.org/integration/b2g-inbound/rev/d77f989d6e7d
https://hg.mozilla.org/integration/b2g-inbound/rev/b60084fa9d5b

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I filed bug 1045943.

Comment 19

[Carsten Book [:Tomcat]]

sorry had to backout this change for causing bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=44851832&tree=B2g-Inbound

Comment 20

[Michael Wu [:mwu]]

Thanks - looks like it was an issue in non-unified builds, so I didn't catch it locally.

Landed again -

https://hg.mozilla.org/integration/b2g-inbound/rev/ef03bec1272a
https://hg.mozilla.org/integration/b2g-inbound/rev/4cc9e0c5dd67

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ef03bec1272a
https://hg.mozilla.org/mozilla-central/rev/4cc9e0c5dd67