Bug 189400

Summary: Review Request: em8300(-kmod) - Hollywood+/DXR3 hardware MPEG decoder drivers and tools
Product: [Fedora] Fedora Reporter: Ville Skyttä <scop>
Component: Package ReviewAssignee: Thorsten Leemhuis <fedora>
Status: CLOSED NEXTRELEASE QA Contact: Fedora Package Reviews List <fedora-package-review>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: axel.thimm, davej, dwmw2, fedora
Target Milestone: ---   
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2006-07-23 09:46:03 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 163779    

Description Ville Skyttä 2006-04-19 18:37:19 UTC
http://dxr3.sourceforge.net/
http://cachalot.mine.nu/5/SRPMS/em8300-0.15.3-3.src.rpm
http://cachalot.mine.nu/5/SRPMS/em8300-kmod-0.15.3-2.2.6.16_1.2080_FC5.src.rpm

These packages provide drivers and utilities for Sigma Designs Hollywood Plus/ Creative DXR3 hardware MPEG decoder cards.  The cards require a microcode (firmware) blob which is not included due to redistribution issues but can be extracted from vendor provided Windows drivers using the tools in the em8300-devel package, or downloaded from the upstream project site at http://dxr3.sourceforge.net/download/

Comment 1 Ville Skyttä 2006-04-27 18:59:03 UTC
http://cachalot.mine.nu/5/SRPMS/em8300-kmod-0.15.3-3.2.6.16_1.2096_FC5.src.rpm

* Thu Apr 27 2006 Ville Skyttä <ville.skytta at iki.fi> - 0.15.3-3
- Provide "kernel-modules" instead of "kernel-module" to match yum's config.


Comment 2 Ville Skyttä 2006-06-23 05:41:29 UTC
http://cachalot.mine.nu/5/SRPMS/em8300-kmod-0.15.3-4.2.6.16_1.2133_FC5.src.rpm

* Fri Jun 23 2006 Ville Skyttä <ville.skytta at iki.fi> - 0.15.3-4
- Invoke kmodtool with bash instead of sh.


Comment 3 Ville Skyttä 2006-06-26 17:39:25 UTC
http://cachalot.mine.nu/5/SRPMS/em8300-kmod-0.15.3-5.2.6.17_1.2139_FC5.src.rpm

* Mon Jun 26 2006 Ville Skyttä <ville.skytta at iki.fi> - 0.15.3-5
- Apply upstream patch to fix build with kernel 2.6.17, rediff WSS patch.


Comment 4 Ville Skyttä 2006-07-06 06:28:43 UTC
Per Packaging/KernelModules in the wiki, here's the additional required
explanation about the mainline merge status (from Nicolas Boullis and yours truly):

    The em8300 modules are not part of mainline kernel yet partly because
    nobody has got around to submit it, and partly because there some
    things are still thought of as unfinished in the sense that they'd
    be better off behaving and being accessible more like other related
    devices in the kernel do, requiring less manual configuration, and
    the like.  At this time, there are no specific deadlines or promises
    set for the submission, but it is expected to happen in not too
    distant future.

Comment 5 Thorsten Leemhuis 2006-07-11 18:20:37 UTC
== General ==

Review without appropriate hardware to test, but I'm sure it'll work.

>These packages provide drivers and utilities for Sigma Designs Hollywood 
> Plus/ Creative DXR3 hardware MPEG decoder cards.  The cards require a 
> microcode (firmware) blob which is not included due to redistribution
> issues

Ehh, I don't get that here. I saw a file 
 modules/em8300.uc
in the package. Is that the firmware? If yes: why it is included? At least there
is a file 
 em8300-nofirmware-0.15.3.tar.gz
on
 http://sourceforge.net/project/showfiles.php?group_id=5165&package_id=5202
and it's missing exactly that file. But from the docs it look like a file
"em8300.bin" needs to contain the firmware.
/me confused

> but can be extracted from vendor provided Windows drivers using the tools in
the em8300-devel package, or >downloaded from the upstream project site at 

Is there a good reason why it needs to be in the devel package? It's not the
obvious place where I would look out for it.

== em8300 ==

rpmlint:

SRPM:
no output

RPM:
W: em8300-devel no-documentation
-> can be ignored

* download of Source0 doesn't work -- seems to be a sourceforge problem afaics
* Open-Source-License (GPL)
* Source matches upstream
* package is named according to the guidelines
* builds in mock on ix86-FC5
* Is there a reason why the devel package doesn't require the base package?
* "Requires:       kmod-em8300 = %{version}" is wrong -- it needs to be 
  "Requires:       em8300-kmod >= %{version}"
 
== em8300-kmod ==

rpmlint:

SRPM:
W: em8300-kmod mixed-use-of-spaces-and-tabs
-> Might be a good idea to fix that in the longer term

RPMS:
W: kmod-em8300 summary-not-capitalized em8300 kernel module(s)
-> This is probably the right thing in this case

W: kmod-em8300 unstripped-binary-or-object
/lib/modules/2.6.17-1.2139_FC5/extra/em8300/adv717x.ko
W: kmod-em8300 unstripped-binary-or-object
/lib/modules/2.6.17-1.2139_FC5/extra/em8300/bt865.ko
W: kmod-em8300 unstripped-binary-or-object
/lib/modules/2.6.17-1.2139_FC5/extra/em8300/em8300.ko
-> they get stripped during build. Hmmm, this looks like a problem in rpm. Could
you look into this?

W: kmod-em8300 no-documentation
-> expected behaviour for kmod-packages

Review:
* package is named according to the guidelines
* Open-Source-License (GPL)
* Source matches upstream
* builds in mock on ix86-FC5
* download of Source0  doesn't work -- seems to be a sourceforge problem afaics

Probably needs fixing: 
- The module isn't build for smp-kernels on ppc. Please fix if there isn't a
good reason not to do so.

Comment 6 Ville Skyttä 2006-07-11 19:45:13 UTC
Thanks for the thorough review.

modules/em8300.uc is indeed the firmware blob (which should be installed as
/lib/firmware/em8300.bin), and the kmod package used accidentally the wrong
source tarball even though the firmware wasn't installed.

The microcode/firmware extractor depends on perl which was a bigger issue a long
time ago when things were packaged differently, but that's no longer the case.

Devel doesn't require the main package or -utils simply because there's nothing
in it that would require those, -devel is self contained.  Adding the dependency
would additionally inflict the need to install a kernel and a suitable em8300
module package for it which seems just useless and may be a problem in eg. some
build systems.

Strip vs not-stripped: the modules are stripped of something indeed, but
something is also left behind which causes "file" to report "not stripped" and
rpmlint uses that for its check.  Note: it's the same as for all kernel modules,
including those shipped with the kernel itself.  I'm not compentent to analyze
this, to me it's just how /usr/lib/rpm/debugedit seems to behave ;)

Other than those:

http://cachalot.mine.nu/5/SRPMS/em8300-0.15.3-4.src.rpm
* Tue Jul 11 2006 Ville Skyttä <ville.skytta at iki.fi> - 0.15.3-4
- Require >= em8300-kmod, not = kmod-em8300 (#189400).
- Move microcode extractor to -utils as em8300-mc_ex (like Debian) (#189400).
- Don't ship microcode_upload.pl, the modules and em8300setup already can
  handle that.

http://cachalot.mine.nu/5/SRPMS/em8300-kmod-0.15.3-6.2.6.17_1.2145_FC5.src.rpm
* Tue Jul 11 2006 Ville Skyttä <ville.skytta at iki.fi> - 0.15.3-6
- Enable PPC SMP builds (#189400).
- Use firmwareless tarball (#189400).
- Untabify specfile (#189400).


Comment 7 Thorsten Leemhuis 2006-07-15 18:26:48 UTC
Review:

== em8300 ==
rpmlint:

SRPM:
no output

RPM:
W: em8300-devel no-documentation
-> can be ignored

* download of Source0 doesn't work -- seems to be a sourceforge problem afaics
* Open-Source-License (GPL)
* Source matches upstream
* package is named according to the guidelines
* builds in mock on ix86-FC5
 
== em8300-kmod ==

rpmlint:

SRPM:
no output

RPMS:
W: kmod-em8300 summary-not-capitalized em8300 kernel module(s)
-> This is probably the right thing in this case

W: kmod-em8300 unstripped-binary-or-object
/lib/modules/2.6.17-1.2139_FC5/extra/em8300/adv717x.ko
W: kmod-em8300 unstripped-binary-or-object
/lib/modules/2.6.17-1.2139_FC5/extra/em8300/bt865.ko
W: kmod-em8300 unstripped-binary-or-object
/lib/modules/2.6.17-1.2139_FC5/extra/em8300/em8300.ko
-> they get stripped during build, we will ignore that

W: kmod-em8300 no-documentation
-> expected behaviour for kmod-packages

Review:
* package is named according to the guidelines
* Open-Source-License (GPL)
* Source matches upstream
* builds in mock on ix86-FC5
* matches fedora kmod scheme

== Summary ==

both nearly approved -- waiting for FESCo approval of kmod (we forgot to discuss
those in the last meeting -- sorry)

Comment 8 Ville Skyttä 2006-07-23 09:46:03 UTC
Was approved in the last FESCO meeting, and has been now imported and built for
FC-5.  Thanks.

Comment 9 David Woodhouse 2006-07-23 09:54:59 UTC
If the kernel module has a suitable licence for FC5, then it presumably has a
suitable licence for being included directly in the kernel. Thus, the correct
approach would be to get the support into the upstream kernel and therefore into
our base kernel package, rather than having a separate kmod package for it.

I strongly believe we should veto _all_ kmod packages in Core and Extras. The
only reason for kernel support not being upstream is because it's not acceptable
there.... which means it probably shouldn't be acceptable to us either.

And if it _is_ acceptable to us but isn't upstream for some reason, then we can
consider adding it to the kernel package properly. In certain exceptional cases.
You should file a separate bug (with patch) for that.

Comment 10 Thorsten Leemhuis 2006-07-23 14:46:41 UTC
(In reply to comment #9)
> If the kernel module has a suitable licence for FC5, then it 
> presumably has a suitable licence for being included directly in the 
> kernel.

Agreed.

> Thus, the correct approach would be to get the support into the 
> upstream kernel and therefore into our base kernel package, rather 
> than having a separate kmod package for it.

Also agreed, and that's why we we plan to try our best to tell upstream
maintainers "get your stuff merged". That why we require a statement
when people plan to get their stuff merged (see comment #4).

> I strongly believe we should veto _all_ kmod packages in Core and 
> Extras.

This was discussed before when we started the development of the
kmod-standard for Extras -- several people from Red Hat were involved in
that discussion and we agreed to work on it nevertheless the political
problems.

Anyway, I can really understand your point. That why I suggested in the
beginning that all kmods are only allowed for X months (X=12 or 18) in
Extras before they get dropped again and that upstream shall work on
getting its suff into the kernel in that timeframe. But that idea was
not accepted by other involved in the discussion.

Anyway, I don't think we should discuss this further here and probably
won't reply again in this bug. fedora-devel-list probably is the better
place for this sort of political discussions.


Site note: It's IMHO more a "kernel" vs. "distributions" problem:

I'm sure all those people currently listed in the CC of this bug agree
that it's important and best to get all drivers into the kernel as fast
as possible (as long as they are "clean" and even if they are far from feature
complete ) and that external drivers have many pitfalls and should be avoided as
much as possible.

But it seems some open-source-driver-developers don't feel that way and
prefer to maintain their drivers outside of the upstream kernel. They
probably fear (for no good reason) getting "Christophed". That's a
pity, but it's life in 2006.

But some distributions include these external drivers. That creates
pressure on those distributions that don't include external drivers
because people run away. E.g.: "I like Fedora. But it doesn't include
the external driver foo -- distribution bar has it already and thus is
far easier to use for me because I need that driver. That's why I
switched over to bar."

So IMHO kernel developers and the kernel-maintainers of the most
important distributions need to work together closely and create
policies like "every distribution may only contain 10 kernel-drivers that are
not yet merged into the upstream kernel". The pressure on driver developers to
get their drivers merged into linus kernel would be a lot bigger then.

Just my 2 cent.

Comment 11 Axel Thimm 2006-07-30 09:35:31 UTC
I saw a referecne to this bug about David's comment #9. I'd like to point out
that one of the oldest kernel modules packages in Fedora are Cluster/GFS bits.
Following this kind of veto, we wouldn't have GFS in Fedora.

And there isn't really a difference between externally built kernel modules and
patched in pieces that haven't been accepted upstream like for example xen.

So at the very end the xen/gfs deviation from upstream really justify most of
the external kernel module projects (keeping binary code out of the discussion,
that's very different and much uglier).

Don't get me wrong, I'm very happy with gfs/xen! And I'm also very happy with
some external kernel modules.


Comment 12 David Woodhouse 2006-07-30 09:56:38 UTC
Xen and GFS2 are in the main kernel package in rawhide/FC6. They're not in
separate packages any more.

Comment 13 Ville Skyttä 2006-07-30 10:09:43 UTC
Please take this discussion to an appropriate mailing list.

Comment 14 Thorsten Leemhuis 2006-07-30 10:20:34 UTC
(In reply to comment #13)
> Please take this discussion to an appropriate mailing list.

+1