Run HAL FM radio commands on a separate thread

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mwu, Assigned: mwu)

Tracking

unspecified
mozilla34
All
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Assignee

Description

6 years ago
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

Updated

5 years ago
Assignee: nobody → mwu
Assignee

Comment 1

5 years ago
Should work, though I need to test on dolphin/tarako to be sure.
Assignee

Comment 2

5 years ago
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)
Assignee

Comment 3

5 years ago
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)
Assignee

Comment 6

5 years ago
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?
Assignee

Comment 7

5 years ago
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+
Assignee

Comment 12

5 years ago
(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.
Assignee

Comment 13

5 years ago
(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.
Assignee

Comment 14

5 years ago
Comment about the fm tuning thread added.
Attachment #8455847 - Attachment is obsolete: true
Attachment #8455847 - Flags: review?(khuey)
Attachment #8456467 - Flags: review?(khuey)
Assignee

Comment 15

5 years ago
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
Assignee

Comment 20

5 years ago
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
https://hg.mozilla.org/mozilla-central/rev/ef03bec1272a
https://hg.mozilla.org/mozilla-central/rev/4cc9e0c5dd67
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.