885370 - [FM] Let GonkFM.cpp Call Dynamically library outside the B2G for Adapting Different HW.

Status: RESOLVED INVALID

(Firefox OS Graveyard :: General, defect)

Description

[Marco Chen [:mchen]]

Currently the GonkFM.cpp called into kernel driver and loaded firmware directly for a specific HW platform. In order to support different FM chips we need to move the implementation on GonkFM.cpp now outside the B2G and as a dynamical library. So different FM Chips can implement their own library and need to follow the APIs definition.

Comment 1

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Hi Michael,
Would we want use the dlopen to open the pre-build shared library or use the factory design pattern to create instance by system property?

Comment 2

[Marco Chen [:mchen]]

Maybe we need the consultant from HAL module owner.

Hi Justin,

Please refer to the Description of this bug and help to give any suggestion for Comment 1. Thanks.

From my point of view, I will choose to use dlopen because we don't want to maintain all device specific code in our repository if we used factory design pattern.

Comment 3

[Justin Lebar (not reading bugmail)]

I don't understand why we need to use a shared library here.

Do we need to have a single B2G binary that works with multiple radios?  Or is recompiling so you can work with a different radio good enough?

If it's the latter, we could have GonkFMX.cpp, GonkFMY.cpp, and so on, and select the appropriate one at compile-time.

If it's the former, then we could still compile GonkFMX.cpp and GonkFMY.cpp; we'd just have to be a bit clever about how we pick which one we're going to execute.

What's the advantage of using a shared library?

Comment 4

[Dave Hylands [:dhylands]]

I think that the Gecko side code should use a traditional class hiearchy for the different devices. So create an FM Radio base class and create a derived class for each type of HW that we support.

Its perfectly reasonable for the derived class to use dlopen/dlsym to open the device specific shared library that comes with android.

Comment 5

[Justin Lebar (not reading bugmail)]

We don't use C++ class hierarchies in hal.  Not that we couldn't, but it's something we don't do now.  Instead, we have different, swappable layers (cpp files).

It may make sense to do some sort of C++ class hierarchy, but if the different FM chips are sufficiently different from one another, it will probably make sense to re-implement GonkFM.cpp for each new chip.  Instead of factoring out shared code into a superclass, we can move it into the FM radio code that lives outside hal.  That would be consistent with the existing design.

Comment 6

[Marco Chen [:mchen]]

Hi Justin and Dave,

Thanks for your input here.

The reason to use dlopen is
  1. I don't want to maintain or put FM chip specific codes into Gecko source tree.
     If there are 100 FM chips supported in the future, do you want to put 100 GonkFM0~100.cpp into our source tree?

  2. I would like to define our own FM interfaces/library naming convention then implemented by each soc chip vendor or FM chip vendor as a shared library but not to implement by ourselves in the future.

  3. For license issue (As I knew), if GonkFM.cpp utilized the FM shared library then we can keep it as MPL. If GonkFM.cpp directly calls into dev node then it will be GPL.

Thanks.

Comment 7

[Dave Hylands [:dhylands]]

(In reply to Marco Chen [:mchen] from comment #6)
> Hi Justin and Dave,
> 
> Thanks for your input here.
> 
> The reason to use dlopen is
>   1. I don't want to maintain or put FM chip specific codes into Gecko
> source tree.
>      If there are 100 FM chips supported in the future, do you want to put
> 100 GonkFM0~100.cpp into our source tree?

I'd be really surprised if you could find more than a handful.

>   3. For license issue (As I knew), if GonkFM.cpp utilized the FM shared
> library then we can keep it as MPL. If GonkFM.cpp directly calls into dev
> node then it will be GPL.

If there is a licensing issue, it doesn't go away by using shared libraries (unless I'm misunderstanding something here). It only goes away by using a separate process.

I think to understand things a little better, I'd need to understand exactly what source gets compiled into what shared libraries, and the origins of the source. Are we talking about using vendor supplied shared libraries, and then also creating our own shared libraries? Or creating our own shared libraries which statically compile against some vendor source code? Or what precisely?

Comment 8

[Marco Chen [:mchen]]

(In reply to Dave Hylands [:dhylands] from comment #7) 
> I'd be really surprised if you could find more than a handful.
> 

Platform vendor: QCT, SPRD, MTK, Hisi, Broadcomm, Marvell, Freescale, rockchip ...etc.
FM Chip vendor: ST-Ericsson, NaviLink, NXP, Atmel ...etc.
There are more on mobile FM chip vendors and plug multiple generations on their product lines.

I think the point here is that do we really want to maintain the FM chips specific codes in our Gecko source tree. I didn't see any examples now in Gecko tree.

> I think to understand things a little better, 

My answer is "using vendor supplied shared libraries".
So the current APIs implementation in GonkFM.cpp can be moved to a shared library and maybe called hw.XXX.fm.so.
Then the job in GonkFM.cpp will be
  1. According to naming convention, it tries to dlopen corresponding library.
  2. To check whether this library contained the symbols (variable or function) we needs.
  3. Redirect the request from FMRadio.cpp to shared library.

Each platform or fm chip vendors can provide their own hw.xxx.fm.so, so we don't need to modify the Gecko tree anymore.

Comment 9

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

This problem comes from android doesn't have the fmradio interface, then the partner hook this function into system by themselves. 
I think the job is likely to 
1. Re-write the FMRadio function to be modulize. (Like android HwModule style) and have a git-hub repository to store those code and management by manifest.xml.
2. Have a FMRadio Generic interface to load the module by system property and redirect the function call to this module.

Does it make any sense?

Comment 10

[Justin Lebar (not reading bugmail)]

> If there is a licensing issue, it doesn't go away by using shared libraries (unless I'm 
> misunderstanding something here). It only goes away by using a separate process.

Of course that depends on the particular license.

> I think the point here is that do we really want to maintain the FM chips specific codes 
> in our Gecko source tree.

I'd rather wait until our current scheme is collapsing under its own weight than start hot-plugging pieces of code into Gecko on the fear that it might be necessary someday.  This is the "you ain't gonna need it" principal.

Usually these things are much less contentious once they're demonstrably necessary.

> I didn't see any examples now in Gecko tree.

We have lots of platform-specific code in the tree.  For example, there's code specific to Windows, MacOS, desktop Linux, Android, and Gonk.  I don't see this as fundamentally different.

> My answer is "using vendor supplied shared libraries".

Do these libraries already exist, or are you suggesting that someone (us or vendors) would write them?

If the libraries already exist, would you mind pointing us to some of them?

If the libraries don't exist, do we have commitments from partners to write them?

In either case, I seriously doubt that we're going to be able to create a multitude of FM radio libraries that all respect the same invariants.  That is to say, we're still discovering quirks of the current FM radio interface in edge cases.  I have very little confidence that we'll be able to find or write an interface for a completely different chip that has the exact same quirks.

If the quirks are different between the chips, we're going to have to have separate code in Gecko to handle these quirks, anyway.  So this whole idea of avoiding code in Gecko tailored to the specific chips breaks down.

Anyway if you really think that this is the right way to do this, feel free to attach a patch.  I've found that it's a lot easier for someone who's not involved in writing the code to dismiss an idea in the abstract, before he's seen the code.

Comment 11

[Marco Chen [:mchen]]

(In reply to Justin Lebar [:jlebar] from comment #10)
> We have lots of platform-specific code in the tree.  For example, there's
> code specific to Windows, MacOS, desktop Linux, Android, and Gonk.  I don't
> see this as fundamentally different.

Yes, we had platform-specific code in the tree. Just like dhyland's question - "I'd be really surprised if you could find more than a handful.". I can agree that there are not many platform needed to do porting.
 
But as I listed on comment 8 there are many FM chips in the market (I can list more) so I just recommend to open this bug.
 
> Do these libraries already exist, or are you suggesting that someone (us or
> vendors) would write them?
> If the libraries already exist, would you mind pointing us to some of them?
> If the libraries don't exist, do we have commitments from partners to write
> them?
> 

The current situation and plan will be
  1. We actively re-write the current GonkFM to what planed in comment 8.
  2. Then ask others to follow our interfaces then provide their own shared library.  (ex: Bug 852497 - BRCM FM chip)

> If the quirks are different between the chips, we're going to have to have separate code in 
> Gecko to handle these quirks, anyway.  So this whole idea of avoiding code in Gecko 
> tailored to the specific chips breaks down.

There are already many samples from AOSP to define interfaces on various HW. (ex: Camera, graphic, audio, sensor ...etc). I don't think FM is too complex then these HW components.

Comment 12

[Justin Lebar (not reading bugmail)]

Okay, we have two peers here saying that they don't want to do it the way you're describing.  That signals to me that the plan you've proposed is going to be a tough sell.

I know you disagree, but how horrible would it be if instead of asking partners to write shared libraries, we asked them to write GonkFM_XXX.cpp for Gecko?  Would you find this unacceptable?  I honestly don't see this as a lot different than asking vendors to write shared libraries (except that we can guarantee that everything stays open-source -- that's obviously pretty important).

I would definitely be willing to reconsider this bug at the point that we have tons of GonkFM_XXX.cpp files, we find that it's unwieldy and actively getting in the way of accomplishing work, and we think that what's presented here would be a solution.

Comment 13

[Marco Chen [:mchen]]

(In reply to Justin Lebar [:jlebar] from comment #12)
Hi Justin,

Okay, I think I should also respect the opinion from direct module owner and peers.
So we will go toward to your suggestion then back here if we meet any obstacle.

Thanks for your input here.

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

Looks like we're going to have a conclusion here.  But one thing I have trouble dealing with would be the default implementation that is used for tbpl integration.  Bug 872417 wants to add an emulated device for FM API testing.  However, we have now FM setup code, which works only for WCN2243 -- FM hardware used on Unagi, etched in Gecko.  It follows that newly added emulator hardware has to emulated behaviours of WCN2243, and that does introduces some extra work and unnecessary limits.  So if we don't prefer creating a new hardware module type and use shared library here, I suggest:

1. have a common interface for all FM devices for sure,
2. guard all FM code with MOZ_B2G_FM (maybe rename to MOZ_FM),
3. disable FM by default and select by --with-fm-backend=xxx

Comment 15

[Marco Chen [:mchen]]

Hi Vicamo and all,

I would like to change the bug status to invalid since there is no action here.
And if we want to discuss about how to distinguish the build option for different GonkFMXXX.cpp could we move to Bug 852497? That one is an another real case for BRCM FM Chip then we can also guide our partner to continue their contribution.

Thanks.

Comment 16

[Marco Chen [:mchen]]

(In reply to Justin Lebar [:jlebar] from comment #12)
> a lot different than asking vendors to write shared libraries (except that
> we can guarantee that everything stays open-source -- that's obviously
> pretty important).

By the way, about "guarantee everything stayed open-source is pretty important" the way on GonkFMXXX.cpp can't guarantee this too. Because MPL doesn't force licensee need to public their modified or added codes, licensee still can hide their private codes in their internal repo. So I don't see this advantage on the way of GonkFMXXX.cpp.

Comment 17

[Michael Wu [:mwu]]

Sorry to reopen this bug - I've been forced to think about this a bit while reviewing bug 852497.

When I originally wrote GonkFMRadio.cpp, the intent was to cover several different FM chips with a single implementation by using V4L2. The Linux kernel's V4L2 interface would be the HAL. There are several Tavarua specific things in GonkFMRadio.cpp, but the thing started as chip agnostic. So ideally, all FM chips would implement V4L2 and we would never have to implement another GonkFMRadio. Some chips may require additional help to account for certain quirks, but the core is still a V4L2 interface.

<unimportant technical details>
Unfortunately, Broadcom decided to stick a FM radio behind the HCI interface in their bluetooth chip, so you have to use the bluetooth HCI interface to talk to the FM radio. Talking to the FM radio requires quite a bit of coordination since bluetooth needs to be on first, and is a pain to do within the kernel.

We do know how to talk to the FM radio on the bluetooth chip though, as the FM radio uses largely identical registers to another Broadcom FM radio which has an open driver. It's just a matter of running hcidump to figure out the custom commands the HCI interface uses to write FM radio registers.
</unimportant technical details>

In the end, it seems like we do need support for userspace FM radio drivers.

However, for those devices I'm not sure I agree that we want different FM radio implementations in hal/gonk/ in order to account for the different quirks of different chips. Different devices will certainly have different quirks, but it's the job of the driver writer to simulate the FM radio device that gecko expects.

Comment 18

[Michael Wu [:mwu]]

There's another possibility here - we might be able to assume that Broadcom will be the only chip which requires this special treatment, and dynamically load the necessary library to talk to btld.

Closing this bug again for now. We'll have to do some investigation.

Comment 19

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Michael Wu [:mwu] from comment #18)
> Closing this bug again for now. We'll have to do some investigation.

When can I have a stable interface for Goldfish?

Comment 20

[Michael Wu [:mwu]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #19)
> (In reply to Michael Wu [:mwu] from comment #18)
> > Closing this bug again for now. We'll have to do some investigation.
> 
> When can I have a stable interface for Goldfish?

I recommend creating a dummy V4L2 driver and making sure our current FM radio code has a generic FM radio path that skips all Tavarua specific stuff.

Comment 21

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Michael Wu [:mwu] from comment #20)
> I recommend creating a dummy V4L2 driver and making sure our current FM
> radio code has a generic FM radio path that skips all Tavarua specific stuff.

All I want to know is how will the code looks like, not what to do.  --enable-fm-backend, or android hardware module, or a shared lib to be loaded by Gecko itself.  That architecture has to come first before one can start implement another driver.  Do you agree?