Re: [PATCH xserver] glamor: Paint first and last pixel of lines

2017-03-15 Thread Eric Anholt 
Keith Packard  writes:

> [ Unknown signature status ]
> Adam Jackson  writes:
>
>> Is there some reason you believe GL's rasterisation rules for lines
>> match fb's zero-width lines? Section 14.5.1 of the 4.5 spec looks quite
>> a bit looser than fb to me.
>
> I think they're 'good enough'; there aren't any rules requiring
> particular rasterization for either, the only thing X requests is that
> 'not last' cap mode not draw the last pixel. afaict, GL suggests 'not
> last' as the only option. It sounds like some drivers are doing both
> 'not last' and 'not first' though?

By "some drivers" we mean i965, it seems.  The problem is that we're not
getting 0-width, specified-under-DirectX lines.  We've got glLineWidth
== 1, so we're getting the garbage "let's turn it into a polygon with
major-axis-aligned ends!" behavior.  Thanks to glamor's +0.5 translation
based on the assumption that we were drawing 0-width lines, we now have
a polygon whose start and end lie on pixel centers (other than float
rounding), for bonus unstable behavior.  If you go into i965 and program
line width to 0, everything's great.

And, if you read the GL spec, it talks a bunch about the diamond exit
rule, which would only make sense if you could actually ask for 0-width
lines.  Thanks, Khronos.

Looking at vc4, it doesn't even have 0-width line behavior, and does
even worse at this testcase.

I'm thinking we need to specify an extension allowing glLineWidth(0) for
diamond exit rule, maybe with a hint to choose between the dx9 and dx10
modes.  Then glamor could check for it and use GL, and fall back to mi
otherwise.


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] glamor: Paint first and last pixel of lines

2017-02-14 Thread Max Staudt 
On 02/14/2017 12:10 AM, Adam Jackson wrote:
> fb has been the only extant renderer for nearly nine years. Clearly
> there exists code that expects it. I'm not convinced that pushing back
> on that expectation is a good move, technically correct though it may
> be.

I'll second that.

So shall we use the fallback code for now, until the wrinkles are ironed
out of GLAMOR?


Here's something else to consider:

With hardware specific 2D acceleration on the way out, clients mostly
pushing huge images (GTK, Qt, ...), and thus Wayland around the corner,
I see the Xorg server mvoe away from its original role as a driver layer.

Instead, it's becoming a pure 2D rendering framework for the apps that
still use raw X11 commands. These programs expect it to work precisely
and reproducibly. Thus, how about focusing on doing precise 2D rendering -
it is the reference X server after all - and making sure that everything
behaves like mi?

I'd even leave the existing GL lines code intact, as it's really cool to
have. But disabled by default, so users get the precise experience first,
and can decide to switch to a quick-but-imprecise OpenGL based experience
if they wish to do so.



Max

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] glamor: Paint first and last pixel of lines

2017-02-14 Thread Max Staudt 
On 02/13/2017 11:53 PM, Keith Packard wrote:
> afaict, GL suggests 'not
> last' as the only option. It sounds like some drivers are doing both
> 'not last' and 'not first' though?

In the case that I reproduced, the 'not first' only happens occasionally, but 
reproducibly.


Max

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] glamor: Paint first and last pixel of lines

2017-02-13 Thread Adam Jackson 
On Mon, 2017-02-13 at 14:54 -0800, Keith Packard wrote:

> X doesn't put any hard requirements on 0-width lines though; why do
> we think we can't use whatever GL does for this?

fb has been the only extant renderer for nearly nine years. Clearly
there exists code that expects it. I'm not convinced that pushing back
on that expectation is a good move, technically correct though it may
be.

- ajax
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] glamor: Paint first and last pixel of lines

2017-02-13 Thread Keith Packard 
Eric Anholt  writes:

> Yeah, I basically think that using GL lines ever is a mistake.  They're
> simple, fast, and wrong.  You wish the hardware did the thing you think
> is sensible, but it doesn't.

X doesn't put any hard requirements on 0-width lines though; why do we
think we can't use whatever GL does for this?

> I think one of our rendering requirements is that GXcopy and then
> GXinvert of that line erases it, which means we can't even do this just
> for GXcopy.

Yeah, we do need to use the same code for all rasterops (and dashing).

> Alternative: Could we draw a short line into a little pixmap at startup,
> and decide if the hardware renders things well enough that our current
> line code is usable?

maybe?

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] glamor: Paint first and last pixel of lines

2017-02-13 Thread Keith Packard 
Adam Jackson  writes:

> Is there some reason you believe GL's rasterisation rules for lines
> match fb's zero-width lines? Section 14.5.1 of the 4.5 spec looks quite
> a bit looser than fb to me.

I think they're 'good enough'; there aren't any rules requiring
particular rasterization for either, the only thing X requests is that
'not last' cap mode not draw the last pixel. afaict, GL suggests 'not
last' as the only option. It sounds like some drivers are doing both
'not last' and 'not first' though?

The more serious problem is that dashing isn't working, and that's just
bugs (either driver or glamor) as dashing is done in the fragment
shader.

I guess we could just run some tests at server startup to see what kind
of rasterization the driver was performing and compensate? Do a survey
of current results and try to come up with some bright-line tests to
distinguish between them, maybe bailing to MI if we get inconsistent results?

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] glamor: Paint first and last pixel of lines

2017-02-13 Thread Eric Anholt 
Adam Jackson  writes:

> On Mon, 2017-02-13 at 09:42 -0800, Keith Packard wrote:
>> Max Staudt  writes:
>> 
>> > Okay, so do you think we can take the patch for the GXcopy case,
>> > and fall back to mi otherwise?
>> 
>> We shouldn't mask driver bugs like this; is there some reason you
>> believe we can't actually go get the GL drivers to work right?
>
> Is there some reason you believe GL's rasterisation rules for lines
> match fb's zero-width lines? Section 14.5.1 of the 4.5 spec looks quite
> a bit looser than fb to me.

Yeah, I basically think that using GL lines ever is a mistake.  They're
simple, fast, and wrong.  You wish the hardware did the thing you think
is sensible, but it doesn't.

I think one of our rendering requirements is that GXcopy and then
GXinvert of that line erases it, which means we can't even do this just
for GXcopy.  If so, the only answer I see is to punt to mi (sadly) until
we write the fb rasterization in a fragment shader.

Alternative: Could we draw a short line into a little pixmap at startup,
and decide if the hardware renders things well enough that our current
line code is usable?


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] glamor: Paint first and last pixel of lines

2017-02-13 Thread Adam Jackson 
On Mon, 2017-02-13 at 09:42 -0800, Keith Packard wrote:
> Max Staudt  writes:
> 
> > Okay, so do you think we can take the patch for the GXcopy case,
> > and fall back to mi otherwise?
> 
> We shouldn't mask driver bugs like this; is there some reason you
> believe we can't actually go get the GL drivers to work right?

Is there some reason you believe GL's rasterisation rules for lines
match fb's zero-width lines? Section 14.5.1 of the 4.5 spec looks quite
a bit looser than fb to me.

- ajax
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] glamor: Paint first and last pixel of lines

2017-02-13 Thread Keith Packard 
Max Staudt  writes:

> Okay, so do you think we can take the patch for the GXcopy case, and fall
> back to mi otherwise?

We shouldn't mask driver bugs like this; is there some reason you
believe we can't actually go get the GL drivers to work right?

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] glamor: Paint first and last pixel of lines

2017-02-13 Thread Max Staudt 
On 02/08/2017 06:30 PM, Adam Jackson wrote:
> I think, practically speaking, that glamor needs to match fb even along
> the corner cases where the X spec allows driver-dependent behavior. The
> simplest way to do this for this case would be to fall down to the mi
> line code for the case of non-trivial cap style and rop that reads the
> destination; if done correctly this will still be "accelerated" in that
> we'll still hit glamor's SetSpans path.

Okay, so do you think we can take the patch for the GXcopy case, and fall
back to mi otherwise?


And how about https://bugs.freedesktop.org/show_bug.cgi?id=99708 - should
we fall back to mi until we have a better approximation in GLAMOR?

It's sad to see acceleration go, but currently it seems to create a lot of
confusion. And raw X11 ops are barely used (if at all) in average modern
desktop environments.



Max

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] glamor: Paint first and last pixel of lines

2017-02-08 Thread Adam Jackson 
On Wed, 2017-02-08 at 14:44 +0100, Max Staudt wrote:
> On 02/07/2017 09:06 PM, Keith Packard wrote:
> > > > Max Staudt  writes:
> > 
> > > OpenGL implementations are allowed to be imprecise in drawing line caps.
> > > This patch expands on the original workaround in dc9fa908.
> > 
> > Yeah, finding a way to work around GL differences seems like a good
> > idea. In this case, however, I think you're fixing some drivers and
> > breaking others -- when drawing with non-idempotent raster
> > ops.
> > 
> > Idempotent raster ops are those for which multiple draws generate the
> > same result, like GXcopy; hence non-idempotent operations are those
> > which do not, such as GXxor.
> 
> What does this mean in practice - is the manual pixel painting the
> problem you're seeing here, because it may draw over the previously
> drawn lines?
> 
> How does a GXor line draw operation currently work in GLAMOR? Maybe it
> would be easier to limit acceleration to GXcopy (which I suppose is by
> far the most useful case) and do that case as correct as possible?

On desktop GL, non-GXcopy rops work by setting glLogicOp appropriately.
 So on drivers that _do_ draw lines like the server's zero-width
implementation, the extra points your patch adds for the caps would be
drawn twice, and with GXxor that would mean they're painted and then
immediately reset.

I think, practically speaking, that glamor needs to match fb even along
the corner cases where the X spec allows driver-dependent behavior. The
simplest way to do this for this case would be to fall down to the mi
line code for the case of non-trivial cap style and rop that reads the
destination; if done correctly this will still be "accelerated" in that
we'll still hit glamor's SetSpans path.

A better, but more complicated, solution would be to use the fragment
shader to draw the line, where the GL primitive is just some quad that
happens to extend a bit past the boundaries of the X primitive so we
definitely hit every pixel we need to.

- ajax
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] glamor: Paint first and last pixel of lines

2017-02-08 Thread Max Staudt 
On 02/07/2017 09:06 PM, Keith Packard wrote:
> Max Staudt  writes:
> 
>> OpenGL implementations are allowed to be imprecise in drawing line caps.
>> This patch expands on the original workaround in dc9fa908.
> 
> Yeah, finding a way to work around GL differences seems like a good
> idea. In this case, however, I think you're fixing some drivers and
> breaking others -- when drawing with non-idempotent raster
> ops.
> 
> Idempotent raster ops are those for which multiple draws generate the
> same result, like GXcopy; hence non-idempotent operations are those
> which do not, such as GXxor.

What does this mean in practice - is the manual pixel painting the
problem you're seeing here, because it may draw over the previously
drawn lines?

How does a GXor line draw operation currently work in GLAMOR? Maybe it
would be easier to limit acceleration to GXcopy (which I suppose is by
far the most useful case) and do that case as correct as possible?


> I'm not sure what we should do here; there's no requirement in the
> protocol that we do anything at all as the server is allowed to draw
> pretty much whatever it wants for zero-width lines.

The X11 client developer who filed the openSUSE bug

  https://bugzilla.opensuse.org/show_bug.cgi?id=1021803

is confused because the software fallback and the GLAMOR version produce
different results. IMHO, since the Xorg server's software fallback
tends to be regarded as the reference X11 implementation, we should try
to emulate that as closely as possible.


I have to agree that this is a funny situation. Neither X11 nor GL are
rigidly specified, yet we're connecting them :)


> If you've found specific problems with Mesa drivers, I'd suggest we
> actually go fix those instead of working around them in the X
> server. The GL spec seems pretty clear in what it wants, and I think
> that aligns with what the X server currently expects.

I'm not sure, do you think this is a problem in the OpenGL drivers used?
It seems within the GL spec, so I'd say Mesa is not the right thing to
fix.

Given that there are plenty of binary drivers, and that we may end up
doing GLAMOR in an Xwayland session, I think we should aim for GLAMOR
being as precise as the fallback. Or disable the acceleration (we could
make it a configuration option). Most modern clients just display images
anyway, and the few remaining ones probably expect rather precise output
from Xorg as a rasterizer.

Consistency is important, as anything else can severely confuse users
and debugging developers alike - I think I've once seen that last line
pixel (which you use to draw the cap) being drawn on some backend. Now
that's a weird graphics glitch! And we can avoid it by setting the cap
pixel manually. Though as you say, only in case of GXcopy.


Max

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] glamor: Paint first and last pixel of lines

2017-02-07 Thread Keith Packard 
Max Staudt  writes:

> OpenGL implementations are allowed to be imprecise in drawing line caps.
> This patch expands on the original workaround in dc9fa908.

Yeah, finding a way to work around GL differences seems like a good
idea. In this case, however, I think you're fixing some drivers and
breaking others -- when drawing with non-idempotent raster
ops.

Idempotent raster ops are those for which multiple draws generate the
same result, like GXcopy; hence non-idempotent operations are those
which do not, such as GXxor.

I'm not sure what we should do here; there's no requirement in the
protocol that we do anything at all as the server is allowed to draw
pretty much whatever it wants for zero-width lines.

If you've found specific problems with Mesa drivers, I'd suggest we
actually go fix those instead of working around them in the X
server. The GL spec seems pretty clear in what it wants, and I think
that aligns with what the X server currently expects.

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver] glamor: Paint first and last pixel of lines

2017-02-07 Thread Max Staudt 
OpenGL implementations are allowed to be imprecise in drawing line caps.
This patch expands on the original workaround in dc9fa908.

Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=99705
Signed-off-by: Max Staudt 
---
 glamor/glamor_lines.c | 21 +
 glamor/glamor_segs.c  | 43 ---
 2 files changed, 33 insertions(+), 31 deletions(-)

diff --git a/glamor/glamor_lines.c b/glamor/glamor_lines.c
index ca3cccf..47f8ed2 100644
--- a/glamor/glamor_lines.c
+++ b/glamor/glamor_lines.c
@@ -45,7 +45,6 @@ glamor_poly_lines_gl(DrawablePtr drawable, GCPtr gc,
 DDXPointPtr v;
 char *vbo_offset;
 int box_x, box_y;
-int add_last;
 
 pixmap_priv = glamor_get_pixmap_private(pixmap);
 if (!GLAMOR_PIXMAP_PRIV_HAS_FBO(pixmap_priv))
@@ -57,10 +56,6 @@ glamor_poly_lines_gl(DrawablePtr drawable, GCPtr gc,
 if (gc->lineStyle != LineSolid)
 goto bail;
 
-add_last = 0;
-if (gc->capStyle != CapNotLast)
-add_last = 1;
-
 if (n < 2)
 return TRUE;
 
@@ -76,7 +71,7 @@ glamor_poly_lines_gl(DrawablePtr drawable, GCPtr gc,
 /* Set up the vertex buffers for the points */
 
 v = glamor_get_vbo_space(drawable->pScreen,
- (n + add_last) * sizeof (DDXPointRec),
+ n * sizeof (DDXPointRec),
  _offset);
 
 glEnableVertexAttribArray(GLAMOR_VERTEX_POS);
@@ -96,11 +91,6 @@ glamor_poly_lines_gl(DrawablePtr drawable, GCPtr gc,
 memcpy(v, points, n * sizeof (DDXPointRec));
 }
 
-if (add_last) {
-v[n].x = v[n-1].x + 1;
-v[n].y = v[n-1].y;
-}
-
 glamor_put_vbo_space(screen);
 
 glEnable(GL_SCISSOR_TEST);
@@ -118,7 +108,14 @@ glamor_poly_lines_gl(DrawablePtr drawable, GCPtr gc,
   box->x2 - box->x1,
   box->y2 - box->y1);
 box++;
-glDrawArrays(GL_LINE_STRIP, 0, n + add_last);
+glDrawArrays(GL_LINE_STRIP, 0, n);
+
+/* Draw the first and last pixels, as implementations are
+ * allowed to be inaccurate there. */
+if (gc->capStyle == CapNotLast)
+glDrawArrays(GL_POINTS, 0, n-1);
+else
+glDrawArrays(GL_POINTS, 0, n);
 }
 }
 
diff --git a/glamor/glamor_segs.c b/glamor/glamor_segs.c
index 0168d05..b571b10 100644
--- a/glamor/glamor_segs.c
+++ b/glamor/glamor_segs.c
@@ -45,7 +45,6 @@ glamor_poly_segment_gl(DrawablePtr drawable, GCPtr gc,
 xSegment *v;
 char *vbo_offset;
 int box_x, box_y;
-int add_last;
 
 pixmap_priv = glamor_get_pixmap_private(pixmap);
 if (!GLAMOR_PIXMAP_PRIV_HAS_FBO(pixmap_priv))
@@ -57,10 +56,6 @@ glamor_poly_segment_gl(DrawablePtr drawable, GCPtr gc,
 if (gc->lineStyle != LineSolid)
 goto bail;
 
-add_last = 0;
-if (gc->capStyle != CapNotLast)
-add_last = 1;
-
 glamor_make_current(glamor_priv);
 
 prog = glamor_use_program_fill(pixmap, gc,
@@ -71,27 +66,25 @@ glamor_poly_segment_gl(DrawablePtr drawable, GCPtr gc,
 goto bail_ctx;
 
 /* Set up the vertex buffers for the points */
-
+/* We need 1.5 times the space in case of CapNotLast,
+ * so let's just always allocate 2 times the space. */
 v = glamor_get_vbo_space(drawable->pScreen,
- (nseg << add_last) * sizeof (xSegment),
+ nseg * sizeof (xSegment) * 2,
  _offset);
 
 glEnableVertexAttribArray(GLAMOR_VERTEX_POS);
 glVertexAttribPointer(GLAMOR_VERTEX_POS, 2, GL_SHORT, GL_FALSE,
   sizeof(DDXPointRec), vbo_offset);
 
-if (add_last) {
-int i, j;
-for (i = 0, j=0; i < nseg; i++) {
-v[j++] = segs[i];
-v[j].x1 = segs[i].x2;
-v[j].y1 = segs[i].y2;
-v[j].x2 = segs[i].x2+1;
-v[j].y2 = segs[i].y2;
-j++;
+memcpy(v, segs, nseg * sizeof (xSegment));
+
+/* In case of CapNotLast, copy the line starting points for later */
+if (gc->capStyle != CapNotLast) {
+int i;
+for (i = 0; i < nseg; i++) {
+v[2*nseg + i] = v[2*i];
 }
-} else
-memcpy(v, segs, nseg * sizeof (xSegment));
+}
 
 glamor_put_vbo_space(screen);
 
@@ -110,7 +103,19 @@ glamor_poly_segment_gl(DrawablePtr drawable, GCPtr gc,
   box->x2 - box->x1,
   box->y2 - box->y1);
 box++;
-glDrawArrays(GL_LINES, 0, nseg << (1 + add_last));
+glDrawArrays(GL_LINES, 0, nseg * 2);
+
+/* Draw the first and last pixel of each line, as OpenGL
+ * implementations are allowed to be inaccurate there.
+ * Since we're drawing all line ends, we can just reuse
+ * the array of all line starts and ends. */
+if (gc->capStyle != CapNotLast) {
+