Closed Bug 938809 Opened 11 years ago Closed 10 years ago

Run HAL FM radio commands on a separate thread

Categories

(Core :: DOM: Device Interfaces, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: mwu, Assigned: mwu)

Details

Attachments

(2 files, 4 obsolete files)

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.
Assignee: nobody → mwu
Should work, though I need to test on dolphin/tarako to be sure.
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.
Attachment #8449166 - Attachment is obsolete: true
Attachment #8455638 - Flags: review?(dhylands)
This patch runs the HAL seek/tune calls on a separate thread.
Attachment #8455719 - Flags: review?(khuey)
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?
Attachment #8455638 - Flags: review?(dhylands) → review+
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.
Attachment #8455719 - Flags: review?(xyuan)
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?
Now with NS_NewNamedThread.
Attachment #8455719 - Attachment is obsolete: true
Attachment #8455719 - Flags: review?(xyuan)
Attachment #8455719 - Flags: review?(khuey)
Attachment #8455847 - Flags: review?(xyuan)
Attachment #8455847 - Flags: review?(khuey)
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:)
Attachment #8455847 - Flags: review?(xyuan) → review?(pzhang)
Ugh, sorry. That's the second time I've done that.
(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 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.
Attachment #8455847 - Flags: review?(pzhang) → review+
(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.
(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 about the fm tuning thread added.
Attachment #8455847 - Attachment is obsolete: true
Attachment #8455847 - Flags: review?(khuey)
Attachment #8456467 - Flags: review?(khuey)
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.
Attachment #8456467 - Attachment is obsolete: true
Attachment #8456467 - Flags: review?(khuey)
Attachment #8459887 - Flags: review?(khuey)
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.
Attachment #8459887 - Flags: review?(khuey) → review+
sorry had to backout this change for causing bustage like https://tbpl.mozilla.org/php/getParsedLog.php?id=44851832&tree=B2g-Inbound
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
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: