Bug 240808 - Review Request: xorg-x11-drv-vermilion - Intel Vermilion driver
Review Request: xorg-x11-drv-vermilion - Intel Vermilion driver
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-05-21 17:06 EDT by Adam Jackson
Modified: 2007-11-30 17:12 EST (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-05-26 13:54:53 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑review+
tcallawa: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Adam Jackson 2007-05-21 17:06:09 EDT
Spec URL: http://people.redhat.com/ajackson/xorg-x11-drv-vermilion.spec
SRPM URL: http://people.redhat.com/ajackson/xorg-x11-drv-vermilion-1.0.0-1.src.rpm
Description: Xorg driver for Intel Vermilion chips.

Apparently this is used in some new mobile platforms.  Don't have the hardware to test yet, but someone surely does.
Comment 2 Adam Jackson 2007-05-22 11:10:23 EDT
In case anyone's wondering, the 'no-wfb' patch disables the code path that goes
through the wfb rendering core, which doesn't exist in xserver 1.3.  It'll pop
into existence in 1.4 though.

As a side effect, the driver only works at 24bpp until wfb exists.
Comment 3 Kevin Fenzi 2007-05-22 14:47:00 EDT
Here's a review: 


OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License (MIT)
OK - License field in spec matches
See below - License file included in package
OK - Spec in American English
OK - Spec is legible.
? - Sources match upstream md5sum:

OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Package has rm -rf RPM_BUILD_ROOT at top of %install

OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
See below - No rpmlint output.
See below - final provides and requires are sane:

SHOULD Items:

OK - Should build in mock.
See below - Should build on all supported archs
OK - Should have dist tag
OK - Should package latest version

Issues:

1. Non blocker. Might ask upstream to include that this is released under
the MIT license in the COPYING file instead of that saying it's just a stub file.

2. Non blocker. rpmlint says:
W: xorg-x11-drv-vermilion mixed-use-of-spaces-and-tabs (spaces: line 3, tab:
line 12)
Fix if you get a chance.

3. Shouldn't this package "Require: hwdata" ? It puts a file under
/usr/share/hwdata/videoaliases/ which hwdata owns. Or does this get pulled in
from some dependency of xorg-x11-server-Xorg?

4. Doesn't build on ppc32. I get:
 gcc -DHAVE_CONFIG_H -I. -I.. -I/usr/include/xorg -O2 -g -pipe -Wall
-Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4
-m32 -MT vermilion.lo -MD -MP -MF .deps/vermilion.Tpo -c vermilion.c  -fPIC
-DPIC -o .libs/vermilion.o
vermilion.c: In function 'VERMILIONKernelOpen':
vermilion.c:380: warning: ignoring return value of 'fgets', declared with
attribute warn_unused_result
vermilion.c:916:2: error: #error VERMILIONReadMemory and VERMILIONWriteMemory
only work on little endian
make[2]: *** [vermilion.lo] Error 1
make[2]: Leaving directory `/builddir/build/BUILD/xf86-video-vermilion-1.0.0/src'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/builddir/build/BUILD/xf86-video-vermilion-1.0.0'
make: *** [all] Error 2
error: Bad exit status from /var/tmp/rpm-tmp.8790 (%build)

Does this hardware exist on ppc? Perhaps ppc/ppc64 should also be excluded?

5. I wasn't yet able to verify the md5 on the sources.
Will do that later today once freedesktop is back up.
Comment 4 Adam Jackson 2007-05-22 16:18:30 EDT
(In reply to comment #3)

> 2. Non blocker. rpmlint says:
> W: xorg-x11-drv-vermilion mixed-use-of-spaces-and-tabs (spaces: line 3, tab:
> line 12)
> Fix if you get a chance.

Fixed.

> 3. Shouldn't this package "Require: hwdata" ? It puts a file under
> /usr/share/hwdata/videoaliases/ which hwdata owns. Or does this get pulled in
> from some dependency of xorg-x11-server-Xorg?

% rpm -q --whatrequires hwdata       
pciutils-2.2.3-4
usbutils-0.71-2.1
kudzu-1.2.64-2
isdn4k-utils-3.2-54.fc7
rhpxl-0.43-1.fc7
system-config-display-1.0.51-1.fc7

None of those are in the dep chain for Xorg, afaik.  None of the other drivers
are requiring on hwdata yet, but they're all languishing in mergereview so
that's not really an excuse.

Fixed.

> 4. Doesn't build on ppc32. I get:
>  gcc -DHAVE_CONFIG_H -I. -I.. -I/usr/include/xorg -O2 -g -pipe -Wall
> -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4
> -m32 -MT vermilion.lo -MD -MP -MF .deps/vermilion.Tpo -c vermilion.c  -fPIC
> -DPIC -o .libs/vermilion.o
> vermilion.c: In function 'VERMILIONKernelOpen':
> vermilion.c:380: warning: ignoring return value of 'fgets', declared with
> attribute warn_unused_result
> vermilion.c:916:2: error: #error VERMILIONReadMemory and VERMILIONWriteMemory
> only work on little endian
> make[2]: *** [vermilion.lo] Error 1
> make[2]: Leaving directory `/builddir/build/BUILD/xf86-video-vermilion-1.0.0/src'
> make[1]: *** [all-recursive] Error 1
> make[1]: Leaving directory `/builddir/build/BUILD/xf86-video-vermilion-1.0.0'
> make: *** [all] Error 2
> error: Bad exit status from /var/tmp/rpm-tmp.8790 (%build)
> 
> Does this hardware exist on ppc? Perhaps ppc/ppc64 should also be excluded?

I'm not even close to sure.  I think it's x86 only for now.

Changed to ExclusiveArch: %{ix86}, might add x86_64 if the need presents itself.
 I suspect this is in-chipset hardware so would never need to be added to
anything but intel chipsets.

Updated:
http://people.redhat.com/ajackson/vermilion/xorg-x11-drv-vermilion.spec
http://people.redhat.com/ajackson/vermilion/xorg-x11-drv-vermilion-1.0.0-2.src.rpm
Comment 5 Kevin Fenzi 2007-05-22 23:17:26 EDT
2. ok. looks good. 
3. ok. looks good. 
4. ok. Note that it did build just fine on x86_64 here, but I don't know if the
hardware appears there or not. 
5. I was able to get thru to freedesktop.org now, and verify the md5sum (which
checks out fine). 
3a45adbfbcae487cf04dbcd089533c3d  xf86-video-vermilion-1.0.0.tar.bz2
3a45adbfbcae487cf04dbcd089533c3d  xf86-video-vermilion-1.0.0.tar.bz2.1

I see no further blockers, so this package is APPROVED. 

Don't forget to close this once it's been imported and built. 
Comment 6 Adam Jackson 2007-05-24 13:44:58 EDT
New Package CVS Request
=======================
Package Name: xorg-x11-drv-vermilion
Short Description: Xorg driver for Intel Vermilion chipset
Owners: ajackson@redhat.com
Branches: F-7
InitialCC:
Comment 7 Tom "spot" Callaway 2007-05-24 14:19:43 EDT
cvs done
Comment 8 Adam Jackson 2007-05-26 13:54:53 EDT
Imported, closing.  Thanks all!

Note You need to log in before you can comment on or make changes to this bug.