Xv / XVideo accelerated video width is limited by / depending on the screen resolution on S3 Savage, but only if the laptop LCD is the primary screen

[Hans-Juergen Mauser]

Binary package hint: xserver-xorg-video-s3

This bug exists at least since 10.04 lucid, maybe earlier (have not tested it before).

I am using the S3 savage driver for a S3 Savage/MX card on an HP XE3-GC laptop. When playing videos (no matter which player as long as the Xv acceleration is used), I encounter the following problem: whenever the LCD is the primary screen (with its hardware resolution of 1024x768), the video "input" width processed by Xv is limited by the ratio of the actual resolution divided by the LCD hardware width (1024). If I set the resolution to 800x600 for example (to be able to use the TV output in full screen mode), I only can see 800/1024 = 0,78 of the real video width, no matter if the video is displayed in a small player window or enlarged to full screen. On the right of the video display, the remaining width is filled with a black (or sometimes blue) bar. Using s3switch to change the output to TV keeps this limitation, which makes watching video on a TV or projector a pain!

If the CRT (external VGA out) is the main display (e.g. by using the laptop in its docking station), I can change the resolution to whatever I want and still have the full video width. Because of distances, however, I could not yet check if the output transition CRT --> TV keeps this full width also for the video output.

In any case, the xvinfo program does not show any limitation - but this limitation should only concern the OUTPUT resolution anyway, not the "source" width of the video being processed.

[Hans-Juergen Mauser (hjmauser)]

Hello,

now I resolved the problem myself on source code level. The relevant parts are two sections in savage_video.c and savage_streams.c, where scaling factors explicitly for LCD usage are calculated. The problem was mainly that only the drawing starting point was modified by the scaling factor, but not the size-defining end of the drawing rectangle. Additionally I had to remove a 7-pixel offset which seems had been added for a very specific case I could not reproduce.

See the appropriate patch files attached to this comment. They resolve this issue completely and are tested on all available resolutions on my HP XE3-GC machines with Savage/MX 8MB, max. hardware LCD resolution 1024x768.

[Bryce Harrington (bryce)]

Hi Hans,

Nice work finding a patch to solve the issue you found! However, I think it would be best if you would get this patch taken either in Debian or upstream, because the -s3 drivers is not one of our main supported X video drivers - we just sync -s3 directly from Debian, and so prefer not to carry any Ubuntu patches for this driver. So I would recommend that you present this patch at <email address hidden> next. They may have suggestions on how to craft the patches to minimize regressions for other users. Once the patches are accepted in Debian, please file a sync request bug report against xserver-xorg-video-s3 and we can pull it into Ubuntu from there.

Thanks ahead of time, and again good work narrowing down the issue to code.

[Tormod Volden (tormodvolden)]

Hi Hans-Jörgen, thanks for the patch and the work you have done on this! Unfortunately it was filed on the wrong package so it did not get on my radar before now.

Would you be willing to make a git patch (as described in http://www.x.org/wiki/Development/Documentation/SubmittingPatches)? After review on the xorg-devel list I can commit it upstream and in Debian. I will first try it out on my hardware.

The 7-pixel "offset" you removed: Adding 7 in combination with AND'ing 0xfff8 serves to round up to nearest multiple of 8. Without the +7 it will be rounded down instead. Are you sure this is correct? Or do you think the rounding should take place somewhere else? Without understanding this better, I only notice this is done for X and not for Y, so I guess it is intentional.

[Tormod Volden (tormodvolden)]

I have had a closer look, and can confirm the issue on my TwisterK chipset. You should do the same changes in SavageDisplayVideoOld() which is used for the MOBILE_TWISTER chipset series. After doing this, I could confirm that the (extrapolated) patch fixes the issue. I could not see any difference by leaving the +7 out though. Maybe this depends on the location of the video window. Also, I noted that in the original code the calculation is done with floats in SavageDisplayVideoOld but integers in SavageDisplayVideoNew. Could this be related? Can you please check if using floats is better anyway?

[Hans-Juergen Mauser (hjmauser)]

Hello Tormod,

thanks for your comments and tests! Are you more deeply involved into the calculation methods for these savage drivers?

In fact, on my system I had to remove the 7-pixel offset to get the screen full in width.

I'm sorry, at the moment I don't have a lot of time for doing an official commit - but if it may take some time, I can keep an eye on it.

I'd like to improve the driver further, there are several open issues:

- when switching to TV output, similar clipping as I had experienced (and resolved) it on the LCD occurs again, depending on the video source resolution --> maybe an intermediate calculation similar to the expansion code should be applied? Additionally, there is NO resolution that fits TV out perfectly. 800x600 is closest, but the right and bottom ends are not displayed. This looks like even the basic image buffer might need scaling.

- the driver (or the HW) is not able to scale a video DOWN. If there is a display window smaller than the source resolution, it is clipped. Maybe an integer/float issue?

- when using the s3switch tool to use TV mode, this setting is not stored correctly / permanently in any way. Power management interventions or even players like VLC (which I need due to performance issues) disable the TV out and put garbage on the LCD. I have to enter "s3switch lcd" blindly to regain anything visual from the system. The current workaround with VLC is to start and pause a video while still using LCD or CRT, then switch to TV. The simple "play" command from stop state in VLC results in the garbage mentioned...

- I have a little bit of hope to be able to boost the XVideo source window limit from 1024x1024 up to something larger, at least 1280x1280 to allow larger images (which are only data bloat and often the added "resolution" is decreased by compression artifacts, but can sometimes be found) to be displayed.

It would be great if we could improve this driver. The machines using the hardware are great tools, very power-saving and quiet, and the basic acceleration of the savage as well as the TV out rendering are quite nice.

Do you have access to any specs of the S3 chips? And/or can you explain the data formats to me, especially this integer/float issue and "artificial" floats created by dividing an integer data type to a fraction? I searched the internet a lot, but could not find a definite clue. Often such formats are no "standards" but simply a way to resolve a problem - I work a lot with controllers which are only able to use integers, often enough only 16 bit embedded systems... but in these application I can look at the code and see how the numbers are used. X11 is also open source, but a big bunch... so it would be great if you could help me here.

Best regards,

Hans-Juergen

[Tormod Volden (tormodvolden)]

After looking at it, I think the cast to float in the other code is overkill, since the result also is an integer, and there is no intermediate quotient, like in 3*(2/3) = 0. There are parentheses but even without, multiplication and division goes from left to right so it is like (3*2)/3 and nothing is lost. The only worry would be if the intermediate product overflows, in which case a (single) cast to a higher precision integer would make sense. In our case, dstBox->x1 etc are screen coordinates so maximum in the order of 1000, and XExp1 etc (from InitStreamsForExpansion) are up to 0xC. Since XExp1 etc are "int", the multiplication is done with "int" (at least 32 bit) so it will not overflow.

For preparing a proper patch there is not much to do, and I can walk you through it:
 git clone git://anongit.freedesktop.org/xorg/driver/xf86-video-savage # to retrieve the upstream tree
 (apply your patches)
 git commit -a -s # and make a short comment
 git format-patch -o .. HEAD^
Send the resulting file to me for style review if you want. Make sure name and address in the patch header is how you would like to have it in the git history.

To start with I'd suggest you leave out the +7 removal. The rest is a good and straight forward fix I would like to apply as soon as possible, but the +7 removal is a separate issue which is more delicate.

Other improvements to the driver would be very cool! I agree this hardware is perfectly usable for most tasks. Also the driver took many years to get in good shape and it would a pity to let it rot. I have no greater ambitions for its maintenance than to keep it falling into pieces, but if there are still interested users and developers like you I will help with what I can. I am not familiar with all of the code, and I never did much with xvideo or tv-out.

We should maybe take all this somewhere else, but just to comment quickly on your issues:
- TV resolutions are usually different than for monitors. I think PAL is 720x576.
- Scaling down should work fine with xvideo. Seems to work here.
- I believe the hardware is limited to 1024x1024 (source) for xvideo. So a movie of 1280x800 can not be scaled down with xvideo on savage.

Unfortunately I have no documentation. Just the savage guide linked from here: http://dri.freedesktop.org/wiki/S3Savage

[Hans-Juergen Mauser (hjmauser)]

Hello Tormod,

while looking at the code again, I start getting some evidence that the "+7" offset addition really is related to panels with a horizontal hardware resolution of 1400 pixels. I ran doxygen over some old versions of the savage driver to dig into the history and possible changes to the worse.
Namely I took version 1.1.26 and 1.1.27. The interesting version is 1.1.26, which has a rather different structure in the expansion code (but the same main bug not to scale the lower right corner of the drawing area).

Here the addition of +7 to the offset is handled completely conditional. At a first glance, it seems to be dependent on two values, the panel width and the selected scaling factors. At a second look, I start getting the opinion that it is only related to the width as it is likely that certain scaling factors are exclusive to this display width.

Whenever the width of the display is checked within a scaling factor condition, the +7 only gets added when the width equals 1400.

Here is the code - in one case even the Y offset is corrected when a 1400 pixel display is present.

01879 switch( XFactor )
01880 {
01881 case 1:
01882 psav->XExpansion = 0x00010001;
01883 psav->displayXoffset =
01884 (((PanelSizeX - ViewPortWidth) / 2) + 0x7) & 0xFFF8;
01885 break;
01886
01887 case 3:
01888 psav->XExpansion = 0x00090008;
01889 psav->displayXoffset =
01890 (((PanelSizeX - ((9 * ViewPortWidth)/8)) / 2) + 0x7) & 0xFFF8;
01891 break;
01892
01893 case 4:
01894 psav->XExpansion = 0x00050004;
01895
01896 if ((psav->cxScreen == 800) && (PanelSizeX !=1400))
01897 {
01898 psav->displayXoffset =
01899 (((PanelSizeX - ((5 * ViewPortWidth)/4)) / 2) ) & 0xFFF8;
01900 }
01901 else
01902 {
01903 psav->displayXoffset =
01904 (((PanelSizeX - ((5 * ViewPortWidth)/4)) / 2) +0x7) & 0xFFF8;
01905 }
01906 break;
01907
01908 case 6:
01909 psav->XExpansion = 0x00030002;
01910 psav->displayXoffset =
01911 (((PanelSizeX - ((3 * ViewPortWidth)/2)) / 2) + 0x7) & 0xFFF8;
01912 break;
01913
01914 case 7:
01915 psav->XExpansion = 0x00020001;
01916 psav->displayXoffset =
01917 (((PanelSizeX - (2 * ViewPortWidth)) / 2) + 0x7) & 0xFFF8;
01918 break;
01919 }
01920
01921 switch( YFactor )
01922 {
01923 case 0:
01924 psav->YExpansion = 0x00010001;
01925 psav->displayYoffset = (PanelSizeY - ViewPortHeight) / 2;
01926 break;
01927 case 1:
01928 psav->YExpansion = 0x00010001;
01929 psav->displayYoffset = (PanelSizeY - ViewPortHeight) / 2;
01930 break;
01931 case 2:
01932 psav->YExpansion = 0x00040003;
01933 psav->displayYoffset = (PanelSizeY - ((4 * ViewPortHeight)/3)) / 2;
01934 break;
01935 ...

[Tormod Volden (tormodvolden)]

I believe this bug was fixed in savage driver 2.3.3 (Ubuntu 12.04).