Bug 563

Summary: Review request: xorg-x11-drv-catalyst - AMD's proprietary driver for ATI graphic cards
Product: Package Reviews Reporter: Stewart Adam <s.adam>
Component: Review RequestAssignee: Nicolas Chauvet <kwizart>
Status: RESOLVED FIXED    
Severity: normal CC: david, kwizart, rpmfusion-package-review
Priority: P5    
Version: Current   
Hardware: All   
OS: GNU/Linux   
namespace:
Bug Depends on:    
Bug Blocks: 4, 562    

Description Stewart Adam 2009-04-19 22:45:26 CEST
Description:
This package provides the most recent proprietary AMD display driver which
allows for hardware accelerated rendering with ATI Mobility, FireGL and
Desktop GPUs. Some of the Desktop and Mobility GPUs supported are the
Radeon HD 2000 series to the Radeon HD 4800 series.

For the full product support list, please consult the release notes
for release 9.4.

SPEC: http://downloads.diffingo.com/rpmfusion/xorg-x11-drv-catalyst.spec
SRPM: http://downloads.diffingo.com/rpmfusion/xorg-x11-drv-catalyst-9.4-1.fc10.src.rpm

rpmlint output:
xorg-x11-drv-catalyst.x86_64: W: non-conffile-in-etc /etc/udev/makedev.d/40-catalyst-dri.nodes
> This file does not need to be set as %config.

xorg-x11-drv-catalyst.x86_64: W: non-conffile-in-etc /etc/ati/signature
> Immovable file, required by the driver
xorg-x11-drv-catalyst.x86_64: W: obsolete-not-provided catalyst-kmod
> Unimportant, users are asked to install "(a)kmod-foo" and not "foo-(a)kmod"

xorg-x11-drv-catalyst.x86_64: W: conffile-without-noreplace-flag /etc/ati/amdpcsdb.default
xorg-x11-drv-catalyst.x86_64: W: conffile-without-noreplace-flag /etc/ati/atiogl.xml
xorg-x11-drv-catalyst.x86_64: W: conffile-without-noreplace-flag /etc/ati/control
xorg-x11-drv-catalyst.x86_64: W: conffile-without-noreplace-flag /etc/ati/logo.xbm.example
xorg-x11-drv-catalyst.x86_64: W: conffile-without-noreplace-flag /etc/ati/logo_mask.xbm.example
> noreplace has been intentionally left out, these files should be replaceable.

xorg-x11-drv-catalyst.x86_64: W: service-default-enabled /etc/rc.d/init.d/atieventsd
xorg-x11-drv-catalyst.x86_64: W: service-default-enabled /etc/rc.d/init.d/catalyst
> This is OK, the driver should be enabled after installation
xorg-x11-drv-catalyst.x86_64: W: incoherent-subsys /etc/rc.d/init.d/catalyst $name
> This os OK, name="catalyst"
xorg-x11-drv-catalyst-libs.x86_64: W: no-documentation
> All documentation is in the main package
xorg-x11-drv-catalyst-libs.x86_64: W: executable-stack /usr/lib64/dri/fglrx_dri.so
> Binary-only file, nothing we can do
Comment 1 David Hlacik 2009-04-20 11:48:57 CEST
Is this a replacement for fglrx ?

Thanks, David
Comment 2 Nicolas Chauvet 2009-04-20 12:33:51 CEST
No, this is another package, fglrx is aimed to stay at 9.3 until it works.

Comment 3 Nicolas Chauvet 2009-04-20 15:08:54 CEST
1* Does this file remains needed with 9.4? (should be set as %config at least)
%{_sysconfdir}/udev/makedev.d/40-catalyst-dri.nodes
(in other words, Is the module capable to create it own devices on /dev/dri/card? )
the file need to be removed "then" the initrd regenerated in order to test for this feature.

This problem itself is a blocker for later improvement of the packaging scheme of the binary GPU drivers. Now this is not a problem as we will stay with the previous scheme until this is solved.
In general we need to check if every file remains necessary.



2* Having this directory provided as multilibs is a very evil bug. 
No i386 userland application should use this directly on a x86_64 system.
%files libs
..
%{atilibdir}/libAMDXvBA.cap
%{_libdir}/dri/
^^ mesa-dri-drivers.x86_64 owns this directory, but we aren't supposed to requires any other dri modules, so it is fine to have the directory owned by the package itself. (btw, please note that only mesa-dri-drivers.x86_64 is provided in x86_64 repository, not the i386 version)

3* -devel subpackage
%{atilibdir}/*.a
%{_libdir}/xorg/modules/*.a
%{_includedir}/GL/
%{_includedir}/X11/extensions/*.h
Theses files conflict with mesa-ones, the nVidia driver have them installed in /usr/include/nvidia which the package should have them installed probably in /usr/include/catalyst. That needs to be changed with the package converted to  the new scheme.
This could be dropped as a consequence:
Requires:        %{_includedir}/X11/extensions, %{_includedir}/GL


4* ||: or everyline with scriplets.
This tweak is aimed to avoid error that might prevents rpm transaction if ever the scriptlet failed for any reason. That's prevent package to fail to install/uninstall, so it only make sense at the end (un-conditional) of a scriplet.


5* %postun -p /sbin/ldconfig
^^ This is uneeded, (read evil) as the main package doesn't provide any system wide shared objects,that need to be registered with ldconfig. (only xorg libraries that are meant to be dlopened).

6* Missing conflicts :
Conflicts: xorg-x11-drv-nvidia-71xx (legacy new name supposed to be re-introduced at a later time once nVidia support new xorg-server).
Conflicts: xorg-x11-drv-nvidia-custom

7* deprecated Obsoletes/Provides from fglrx package history:
This doesn't have to be introduced in the new package (could be dropped in fglrx also actually.)
Obsoletes:       ati-x11-drv < %{version}-%{release}
Provides:        ati-x11-drv = %{version}-%{release}
...
Obsoletes:       ati-x11-drv-devel < %{version}-%{release}
Provides:        ati-x11-drv-devel = %{version}-%{release}
...
%ifarch %{ix86}
Provides: %{name}-libs-32bit = %{version}-%{release}
Obsoletes: %{name}-libs-32bit <= %{version}-%{release}
%endif
...
Requires:        %{atilibdir}/libGL.so.1.2
^^ this one is not needed along with the use of 
Requires:        %{name}-libs-%{_target_cpu} = %{version}-%{release}
(for the main package)

8* %config need to be appended for earch file using /etc
There is a need to verify if this is still relevant for some files that may even not be relevant to have in /etc but where ati/amd uses some hardcoded path which prevent us to use that.


So to sum-up , I expect this package to be introduced in F-10/F-11 (F-9?).
Most of the above notes at pretty trivial to fix. (since that's just few tweaks from a fglrx->catalyst rename) But I note this is for the old scheme. I really think we need to work on the new scheme for both F-10/F-11.








Comment 4 Stewart Adam 2009-04-21 21:16:26 CEST
BTW - For those who are looking at the bug and want to know what kwizart and I mean by old/new scheme, "old scheme" refers to the current setup with livna-config-display where all configuration files are simply included in the driver packages. "New scheme" refers to a future setup with rpmfusion-config-display where configuration files are generated on-demand, making driver switches much easier.

(In reply to comment #3)
> 1* Does this file remains needed with 9.4? (should be set as %config at least)
> %{_sysconfdir}/udev/makedev.d/40-catalyst-dri.nodes
> (in other words, Is the module capable to create it own devices on
> /dev/dri/card? )
> the file need to be removed "then" the initrd regenerated in order to test for
> this feature.
Will test at home tonight.

> This problem itself is a blocker for later improvement of the packaging scheme
> of the binary GPU drivers. Now this is not a problem as we will stay with the
> previous scheme until this is solved.
I saw in the last git commit you made from rpmfusion-config-display you added a structure so we could create/destroy these files using r-c-d... I will try to continue working on this soon, and if it works then the need for files like the blacklist, udev configuration or power scripts will no longer be a problem.
> In general we need to check if every file remains necessary.
+1

> 3* -devel subpackage
> %{atilibdir}/*.a
> %{_libdir}/xorg/modules/*.a
> %{_includedir}/GL/
> %{_includedir}/X11/extensions/*.h
> Theses files conflict with mesa-ones, the nVidia driver have them installed in
> /usr/include/nvidia which the package should have them installed probably in
> /usr/include/catalyst. That needs to be changed with the package converted to 
> the new scheme.
> This could be dropped as a consequence:
> Requires:        %{_includedir}/X11/extensions, %{_includedir}/GL
Will fix + test compiling software against the relocated headers tonight as well.

> 4* ||: or everyline with scriplets.
> This tweak is aimed to avoid error that might prevents rpm transaction if ever
> the scriptlet failed for any reason. That's prevent package to fail to
> install/uninstall, so it only make sense at the end (un-conditional) of a
> scriplet.
Done

> 5* %postun -p /sbin/ldconfig
> ^^ This is uneeded, (read evil) as the main package doesn't provide any system
> wide shared objects,that need to be registered with ldconfig. (only xorg
> libraries that are meant to be dlopened).
Done

> 6* Missing conflicts :
> Conflicts: xorg-x11-drv-nvidia-71xx (legacy new name supposed to be
> re-introduced at a later time once nVidia support new xorg-server).
> Conflicts: xorg-x11-drv-nvidia-custom
Done

> 7* deprecated Obsoletes/Provides from fglrx package history:
> This doesn't have to be introduced in the new package (could be dropped in
> fglrx also actually.)
> Obsoletes:       ati-x11-drv < %{version}-%{release}
> Provides:        ati-x11-drv = %{version}-%{release}
> ...
> Obsoletes:       ati-x11-drv-devel < %{version}-%{release}
> Provides:        ati-x11-drv-devel = %{version}-%{release}
> ...
If we get rid of these, then FreshRPM users who install RPM Fusion will have no upgrade path... If we no longer wish to support this upgrade path on new branches (ie, F-11), I'm OK with that but I think that we need to keep it for F-9/F-10.

> %ifarch %{ix86}
> Provides: %{name}-libs-32bit = %{version}-%{release}
> Obsoletes: %{name}-libs-32bit <= %{version}-%{release}
> %endif
> ...
> Requires:        %{atilibdir}/libGL.so.1.2
> ^^ this one is not needed along with the use of 
> Requires:        %{name}-libs-%{_target_cpu} = %{version}-%{release}
> (for the main package)
Fixed

> 8* %config need to be appended for earch file using /etc
> There is a need to verify if this is still relevant for some files that may
> even not be relevant to have in /etc but where ati/amd uses some hardcoded path
> which prevent us to use that.
Done

> So to sum-up , I expect this package to be introduced in F-10/F-11 (F-9?).
F-10 and F-11 right away (will migrate from old to new scheme eventually), and F-9 afterwards (but kept at the old scheme).

> Most of the above notes at pretty trivial to fix. (since that's just few tweaks
> from a fglrx->catalyst rename) But I note this is for the old scheme. I really
> think we need to work on the new scheme for both F-10/F-11.
+1 - this is partially my fault, I don't have much time... I'm finishing the semester mid-may, at that point I'll have much more time to finish the last bits of r-c-d.
Comment 5 Nicolas Chauvet 2009-04-25 01:17:16 CEST
(In reply to comment #4)
...
> (In reply to comment #3)
> > 1* Does this file remains needed with 9.4? (should be set as %config at least)
> > %{_sysconfdir}/udev/makedev.d/40-catalyst-dri.nodes
> > (in other words, Is the module capable to create it own devices on
> > /dev/dri/card? )
> > the file need to be removed "then" the initrd regenerated in order to test for
> > this feature.
> Will test at home tonight.
What tell the testing ? 

> structure so we could create/destroy these files using r-c-d... I will try to
> continue working on this soon, and if it works then the need for files like 
udev creation and other power management files shoudn't be the priority for now.

> > 7* deprecated Obsoletes/Provides from fglrx package history:
> > This doesn't have to be introduced in the new package (could be dropped in
> > fglrx also actually.)
...
> If we get rid of these, then FreshRPM users who install RPM Fusion will have no
> upgrade path... If we no longer wish to support this upgrade path on new
> branches (ie, F-11), I'm OK with that but I think that we need to keep it for
> F-9/F-10.
Wrong., only fglrx should have kept them, not catalyst. We could also assume users have upgraded since now. Furthermore, according that F-11 users aren't supposed to upgrade from F-9 (but from a step by F-10), we can even drop it from F-11 fglrx package.


> > So to sum-up , I expect this package to be introduced in F-10/F-11 (F-9?).
> F-10 and F-11 right away (will migrate from old to new scheme eventually), and
> F-9 afterwards (but kept at the old scheme).
> 
> > Most of the above notes at pretty trivial to fix. (since that's just few tweaks
> > from a fglrx->catalyst rename) But I note this is for the old scheme. I really
> > think we need to work on the new scheme for both F-10/F-11.
> +1 - this is partially my fault, I don't have much time... I'm finishing the
> semester mid-may, at that point I'll have much more time to finish the last
> bits of r-c-d.
Okay, so we need to keep it the old way, and to convert eventually convert it later (keeping Conflicts for now)

I'm still testing the new scheme on Rawhide with nvidia.

Comment 6 Stewart Adam 2009-04-25 03:54:42 CEST
(In reply to comment #5)
> (In reply to comment #4)
> ...
> > (In reply to comment #3)
> > > 1* Does this file remains needed with 9.4? (should be set as %config at least)
> > > %{_sysconfdir}/udev/makedev.d/40-catalyst-dri.nodes
> > > (in other words, Is the module capable to create it own devices on
> > > /dev/dri/card? )
> > > the file need to be removed "then" the initrd regenerated in order to test for
> > > this feature.
> > Will test at home tonight.
> What tell the testing ? 
9.4 works well without the udev configuration file, and overall the driver seems to run much more smoothly.

> > If we get rid of these, then FreshRPM users who install RPM Fusion will have no
> > upgrade path... If we no longer wish to support this upgrade path on new
> > branches (ie, F-11), I'm OK with that but I think that we need to keep it for
> > F-9/F-10.
> Wrong., only fglrx should have kept them, not catalyst. We could also assume
> users have upgraded since now. Furthermore, according that F-11 users aren't
> supposed to upgrade from F-9 (but from a step by F-10), we can even drop it
> from F-11 fglrx package.
Okay, that makes sense - provides/obsoletes removed in the package below.

> Okay, so we need to keep it the old way, and to convert eventually convert it
> later (keeping Conflicts for now)
That will be best for now, we can implement the new scheme for fglrx once the power management script integration in r-c-d is finished.

SPEC: http://downloads.diffingo.com/rpmfusion/xorg-x11-drv-catalyst.spec
SRPM: http://downloads.diffingo.com/rpmfusion/xorg-x11-drv-catalyst-9.4-2.fc10.src.rpm
Comment 7 Stewart Adam 2009-04-27 18:07:24 CEST
As per discussion on IRC, this update fixes:
* no longer obsoletes/provides libs-32bit (no need to on a new series)
* uses ||: correctly in scriptlets
* appends .conf to the blacklist filename
* ExclusiveArch: only enabled for F11

SPEC: http://downloads.diffingo.com/rpmfusion/xorg-x11-drv-catalyst.spec
Comment 8 Nicolas Chauvet 2009-05-01 20:22:28 CEST
I cannot test until monday, but i't rather good, so still few minor comments:
- Requires(post):  ldconfig is undeeded since we use %post libs -p /sbin/ldconfig
- Requires(post):  catalyst-kmod >= %{version} - IS this needed ?, Requires is already set, everything which isn't mandatory should be avoided to prevent weird effect or case corner at the rpm transaction check while updating the driver.
( in theory it should only be:  "Requires:  catalyst-kmod = %{version}" ).

- This one is more important and was missing: (for the main package).
Requires:        %{name}-libs-%{_target_cpu} = %{version}-%{release}
the requirement for the libs subpackage must be architecture dependant.

- Are you really sure that all files in /etc should be set as %config ?!

- #ExclusiveArch:   x86_64 i586
We talk about that previously, use the same tweak as the nvidia driver, less problem...




Comment 9 Stewart Adam 2009-05-03 18:59:37 CEST
(In reply to comment #8)
> I cannot test until monday, but i't rather good, so still few minor comments:
> - Requires(post):  ldconfig is undeeded since we use %post libs -p /sbin/ldconfig
Fixed, moved the Requires(post) to the libs package

> - Requires(post):  catalyst-kmod >= %{version} - IS this needed ?, Requires is
> already set, everything which isn't mandatory should be avoided to prevent
> weird effect or case corner at the rpm transaction check while updating the
> driver.
> ( in theory it should only be:  "Requires:  catalyst-kmod = %{version}" ).
I removed this - I don't see why this is here either, the removal order in terms of the main package and the kmod package doesn't matter at all, as long as they are removed together it is OK.

> - This one is more important and was missing: (for the main package).
> Requires:        %{name}-libs-%{_target_cpu} = %{version}-%{release}
> the requirement for the libs subpackage must be architecture dependant.
Fixed

> - Are you really sure that all files in /etc should be set as %config ?!
> (In reply to comment #3)
>> 8* %config need to be appended for earch file using /etc
I interpreted this as %config was needed for every file... I've removed %config from the following files:
%{_sysconfdir}/ati/atiogl.xml
--> Unclear on how this file is used, so have the driver always replace it on upgrades to ensure proper functioning. Apparently contains profiles for various applications...
%{_sysconfdir}/ati/logo.xbm.example
%{_sysconfdir}/ati/logo_mask.xbm.example
--> These files are just logos, no need for %config.
%{_sysconfdir}/ati/amdpcsdb.default
--> This file contains the default values/tweaks for amdcccle and aticonfig. Should be replaced every update.
%{_sysconfdir}/ati/signature
%{_sysconfdir}/ati/control
--> These files control hardware verification: They should be replaced upon every driver update to ensure that the driver can verify hardware correctly.

> - #ExclusiveArch:   x86_64 i586
> We talk about that previously, use the same tweak as the nvidia driver, less
> problem...
Fixed

SPEC: http://downloads.diffingo.com/rpmfusion/xorg-x11-drv-catalyst.spec
SRPM: http://downloads.diffingo.com/rpmfusion/xorg-x11-drv-catalyst-9.4-4.fc10.src.rpm
Comment 10 Nicolas Chauvet 2009-05-03 19:24:02 CEST
(In reply to comment #9)
> (In reply to comment #8)
> > I cannot test until monday, but i't rather good, so still few minor comments:
> > - Requires(post):  ldconfig is undeeded since we use %post libs -p /sbin/ldconfig
> Fixed, moved the Requires(post) to the libs package
You don't need this either, once you are using %post libs -p /sbin/ldconfig
the Requires(post): /sbin/ldconfig is automatically added on the libs subpackage.
This isn't the case anymore when using multiples lines scriptlets.

Comment 11 Stewart Adam 2009-05-03 20:03:36 CEST
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > I cannot test until monday, but i't rather good, so still few minor comments:
> > > - Requires(post):  ldconfig is undeeded since we use %post libs -p /sbin/ldconfig
> > Fixed, moved the Requires(post) to the libs package
> You don't need this either, once you are using %post libs -p /sbin/ldconfig
> the Requires(post): /sbin/ldconfig is automatically added on the libs
> subpackage.
> This isn't the case anymore when using multiples lines scriptlets.
Ah, good I didn't know this. Do you mind if I just remove this at import time? SRPM is large to upload...
Comment 12 Nicolas Chauvet 2009-05-06 16:11:14 CEST
The only problem I see is to deceide either we provide a build for a 2.6.29 kernel (either F-10 or F-11) since the drivers seems very unstable on such kernel (Was reported by someone else for F-11 since I don't have the required hardware).

For the driver itself on a 2.6.27 kernel, and about the catalyst driver name change (and the various configurations changes it implied), the package is good to go.

-------------------

This package (xorg-x11-drv-catalyst) is APPROVED by me
(with the exclusion of a build for 2.6.29 kernel, hence F-11 import excluded)

-------------------
Comment 13 Stewart Adam 2009-05-06 19:41:39 CEST
Package CVS request
======================
Package Name: xorg-x11-drv-catalyst
Short Description: AMD's proprietary driver for ATI graphic cards
Owners: firewing
Branches: F-9 F-10
InitialCC: kwizart
----------------------
License tag: nonfree