Bug 99708 - glamor: LineOnOffDash drawn as one whole line
Summary: glamor: LineOnOffDash drawn as one whole line
Status: RESOLVED FIXED
Alias: None
Product: xorg
Classification: Unclassified
Component: Server/Acceleration/glamor (show other bugs)
Version: git
Hardware: All All
: medium normal
Assignee: Xorg Project Team
QA Contact: Xorg Project Team
URL:
Whiteboard:
Keywords:
: 99849 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-02-07 17:50 UTC by Max Staudt
Modified: 2017-03-21 16:49 UTC (History)
6 users (show)

See Also:
i915 platform:
i915 features:


Attachments
X11 test case with LineOnOffDash rendered incorrectly (850 bytes, text/x-c)
2017-02-07 17:50 UTC, Max Staudt
no flags Details
Reference output (440 bytes, image/png)
2017-02-07 17:51 UTC, Max Staudt
no flags Details
Broken output on Xephyr (444 bytes, image/png)
2017-02-07 17:52 UTC, Max Staudt
no flags Details
Broken output on modesetting/Intel on Broadwell (444 bytes, image/png)
2017-02-07 17:53 UTC, Max Staudt
no flags Details
Broken output on modesetting/Intel on Skylake (6.13 KB, image/png)
2017-02-07 18:01 UTC, Max Staudt
no flags Details
Broken output on Xephyr with .w replaced by .r (439 bytes, image/png)
2017-03-01 15:13 UTC, Max Staudt
no flags Details

Description Max Staudt 2017-02-07 17:50:59 UTC
Created attachment 129398 [details]
X11 test case with LineOnOffDash rendered incorrectly

(This was originally reported at: https://bugzilla.opensuse.org/show_bug.cgi?id=1021803 )

When drawing dashed lines with GLAMOR, they are drawn partially or as a regular line, depending on the OpenGL backend.

I have attached an example for X11 that exhibits this problem (the second part of the program in the openSUSE bug report).

This is caused by the GLAMOR code for dashed lines, introduced in xserver commit d18f5801:

  https://cgit.freedesktop.org/xorg/xserver/commit/?id=d18f5801c9a632dd4d9f8b7912491b6623e943d5

Any help would be appreciated. Please let me know if there is something that I can try or look into.
Comment 1 Max Staudt 2017-02-07 17:51:49 UTC
Created attachment 129399 [details]
Reference output

This is what the output should look like when rendered correctly.

In this case, I have used Xephyr *without* GLAMOR.
Comment 2 Max Staudt 2017-02-07 17:52:42 UTC
Created attachment 129400 [details]
Broken output on Xephyr

This is what the output looks like when GLAMOR is used through Xephyr -glamor.
Comment 3 Max Staudt 2017-02-07 17:53:23 UTC
Created attachment 129401 [details]
Broken output on modesetting/Intel on Broadwell

This is what the output looks like when GLAMOR is used through the modesetting DDX on Intel.
Comment 4 Max Staudt 2017-02-07 18:01:34 UTC
Created attachment 129402 [details]
Broken output on modesetting/Intel on Skylake

Another kind of wrong output can be seen in the openSUSE bug report, copied here for documentation purposes. This is on the original reporter's Skylake machine, whereas the previous output was on my Broadwell box. All tests are on Leap 42.2.
Comment 5 Roland Scheidegger 2017-02-28 18:17:51 UTC
*** Bug 99849 has been marked as a duplicate of this bug. ***
Comment 6 Max Staudt 2017-03-01 15:13:47 UTC
Created attachment 130001 [details]
Broken output on Xephyr with .w replaced by .r


As discussed in fdo#99849, this seems to be a problem with the glamor code:

https://bugs.freedesktop.org/show_bug.cgi?id=99849#c6

I've replaced the .w in on_off_fs_exec[] by .r and the line still doesn't look right in Xephyr - see attachment.

Given that in fdo#99705, we also had variations in rendering when compared to the fallback code, maybe it makes sense to disable the glamor code path for now, so as to not confuse users why things look differently on one system or another?
Comment 7 Roland Scheidegger 2017-03-09 18:47:09 UTC
(In reply to Max Staudt from comment #6)
> Created attachment 130001 [details]
> Broken output on Xephyr with .w replaced by .r
> 
> 
> As discussed in fdo#99849, this seems to be a problem with the glamor code:
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=99849#c6
> 
> I've replaced the .w in on_off_fs_exec[] by .r and the line still doesn't
> look right in Xephyr - see attachment.
Oh I missed that. Looks like the calculations are slightly off somewhere (apparently a segment should cover two pixels but only covers one).

> 
> Given that in fdo#99705, we also had variations in rendering when compared
> to the fallback code, maybe it makes sense to disable the glamor code path
> for now, so as to not confuse users why things look differently on one
> system or another?
I don't think it would be really too difficult to fix, but I don't have time to look at glamor.
Comment 8 Roland Scheidegger 2017-03-10 00:51:23 UTC
Actually my analysis was incorrect.
glamor does in fact play around with the texture swizzles, hence .w should be correct.
However, the setup looks busted, as it actually uses the wrong texture in the end.
Here:

/* Set the dash pattern as texture 1 */

    glActiveTexture(GL_TEXTURE1);
    glBindTexture(GL_TEXTURE_2D, dash_priv->base.fbo->tex);
    glUniform1i(prog->dash_uniform, 1);
    glUniform1f(prog->dash_length_uniform, dash_pixmap->drawable.width);

That's not what this does, since dash_priv seems to be the pixmap for the actual drawable, not the dash pattern (I'm not sure, maybe some dash_priv assignment earlier was bogus). Hence this will actually use the main drawable itself as the texture (also known as rendering feedback loop - not good...). The results of that can be anything. (If dash_priv actually were the dash pixmap, I'm not quite convinced the texture swizzles would be set up alright.)

Not sure what a correct fix would be, as it is at this point the dash pattern is luckily already set up as texture 0 (and something set up the correct texture swizzles for it too), so setting that uniform to 0 instead works (in a apitrace, I can't be bothered to recompile this).
Setting up textures and fbos is somewhere buried deeper inside and someone with a bit of knowledge of glamor sure should be able to figure it out...

As a side note, this also relies on GL_REPEAT texture wrap mode without setting it explicitly. I have no idea if that's safe in general to assume it hasn't been set to something else...
Comment 9 Max Staudt 2017-03-13 17:03:38 UTC
Roland, thanks a bunch for investigating this!

Unfortunately, my GL is only very rudimentary, so I hope one of the GLAMOR people can have a look...
Comment 10 Roland Scheidegger 2017-03-21 14:55:51 UTC
(In reply to Max Staudt from comment #9)
> Roland, thanks a bunch for investigating this!
> 
> Unfortunately, my GL is only very rudimentary, so I hope one of the GLAMOR
> people can have a look...

FWIW it looks like Eric Anholt fixed this just recently:
https://cgit.freedesktop.org/xorg/xserver/commit/?id=fe0b297420fc1de8a7fab28457d0864b3182e967
Maybe you can give it a try.
Comment 11 Max Staudt 2017-03-21 16:49:58 UTC
Thanks for the pointer, Roland.

This commit indeed fixes the dashed lines, so let's close the bug.

Thanks also to Eric for fixing this.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.