Bug 1226723

Summary: Remove rhel7-specific hack
Product: Red Hat Enterprise Linux 6 Reporter: Marc-Andre Lureau <marcandre.lureau>
Component: xorg-x11-drv-qxlAssignee: Default Assignee for SPICE Bugs <rh-spice-bugs>
Status: CLOSED ERRATA QA Contact: SPICE QE bug list <spice-qe-bugs>
Severity: medium Docs Contact:
Priority: medium    
Version: 6.6CC: cfergeau, dblechte, fidencio, fziglio, marcandre.lureau, rbalakri, tpelka
Target Milestone: rc   
Target Release: 6.8   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2016-05-10 19:49:59 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Attachments:
Description Flags
Package patch none

Description Marc-Andre Lureau 2015-05-31 22:34:21 UTC
RHEL6 qxl driver ships with a rhel7-specific hack, to workaround video detection code when using gnome-shell. Unfortunately, this also makes video detection sometime harder on rhel6 too. It should be removed from rhel6.

Comment 3 Frediano Ziglio 2015-10-14 12:23:20 UTC
Marc, which hack are you referring to?

Comment 4 Marc-Andre Lureau 2015-10-14 12:59:30 UTC
0001-worst-hack-of-all-time-to-qxl-driver.patch
#define HACK_THE_PLANET 1

Comment 5 Frediano Ziglio 2015-10-14 14:54:15 UTC
Original issue (the reason for the patch) is https://bugzilla.redhat.com/show_bug.cgi?id=1020393. Looking at the video seems related to some bug in stream implementation. The patch mainly try to avoid spice-server to detect the streaming (look at the video). I think removing this patch at this stage could cause regressions.
Better things would be to remove the stream bug that cause that strange effects.

Comment 6 Marc-Andre Lureau 2015-10-14 16:59:46 UTC
(In reply to Frediano Ziglio from comment #5)
> Original issue (the reason for the patch) is
> https://bugzilla.redhat.com/show_bug.cgi?id=1020393. Looking at the video
> seems related to some bug in stream implementation. The patch mainly try to
> avoid spice-server to detect the streaming (look at the video). I think
> removing this patch at this stage could cause regressions.
> Better things would be to remove the stream bug that cause that strange
> effects.

Last time I checked, it caused more harm than good on rhel6 guest, basically breaking any chance of video detection :) 

The best way I could think of to solve this for rhel7 guest is to use the mesa shm patch I proposed:
http://lists.freedesktop.org/archives/mesa-dev/2015-June/086459.html.

The other less efficient and unlikely to happen, is to have x11 bigreq support for PutImage.

Comment 7 Frediano Ziglio 2015-10-15 15:29:47 UTC
I really don't understand all the background of this problem.
Looking at the video at https://bugzilla.redhat.com/show_bug.cgi?id=1020393 is not just performance is that streaming is not working correctly and the patch is a workaround to disable streaming detection.
Or perhaps I don't understand the relationship between the glitches and the performances.

Comment 8 Marc-Andre Lureau 2015-10-17 08:05:24 UTC
(In reply to Frediano Ziglio from comment #7)
> I really don't understand all the background of this problem.
> Looking at the video at https://bugzilla.redhat.com/show_bug.cgi?id=1020393
> is not just performance is that streaming is not working correctly and the
> patch is a workaround to disable streaming detection.
> Or perhaps I don't understand the relationship between the glitches and the
> performances.

You have to remember that rhel6 and rhel7 are quite different at rendering. in gnome3, mutter/clutter is responsible for compositing and so llvmpipe for sending drawing commands to the driver, mostly large images.

Video detection is not in sync with the rest of drawing, so you get glitches if done wrong. Dave hack is breaking video detection altogether, to avoid the glitches. But this of course lower performance (more weird drawings) and prevent video streams

Today, clutter is better at updating only the modified regions, and spice server video detection also got improved.

Even if removing Dave hack, the update regions are still quite big (a whole app window for ex), so a large update is done with many PutImage of various size, confusing video detection, typically with various video strips as seen in bug 1020393. Instead the region update should be done in a single request. Then video detection works properly. Having BIGREQ implementation for PutImage is one way. The better way is to use XShm to also avoid extra copy, and this is what I've done http://lists.freedesktop.org/archives/mesa-dev/2015-June/086459.html.

iirc, in short:
1. the hack is terrible breaking many chances of correct video streaming even with rhel6, and should no longer be so bad with rhel7 due to clutter and server improvements
2. video glitches are still possible due to PutImage being splitted in various updates, but the xshm mesa patches allow for a single update of regions & save memcpy (which is really nice for video or animations)

removing 1 & adding 2 is what should be done

Comment 9 Frediano Ziglio 2015-10-20 09:18:39 UTC
Created attachment 1084662 [details]
Package patch

Comment 10 Frediano Ziglio 2015-10-21 15:13:35 UTC
For developers are mostly done (see previous patch)

Comment 15 errata-xmlrpc 2016-05-10 19:49:59 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://rhn.redhat.com/errata/RHBA-2016-0761.html