Re: [PATCH xserver] glamor: Disable logic ops when doing compositing

[Keith Packard]

Eric Anholt  writes:

> Oh, yeah.  Even better.

And a v3 which catches the other place where GL_BLEND is getting
set. Sent that under a separate cover along with a more complicated
patch for the red/alpha swizzling stuff.

-- 
-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: Disable logic ops when doing compositing

[Eric Anholt]

Keith Packard  writes:

> Eric Anholt  writes:
>
>> Keith Packard  writes:
>>
>>> If the logic op gets left enabled, it overrides the blending
>>> operation, causing incorrect contents on the display.
>>>
>>> Signed-off-by: Keith Packard 
>>
>> Reviewed-by: Eric Anholt 
>
> And it's actually wrong (sigh). Need to do this in all paths through
> this function (even the PictOpSrc case), and we only want to do it on
> non-ES2 flavors. Here's an updated version.
>
> From 1ac2ef66369f36ef132f643969ad176db39babe7 Mon Sep 17 00:00:00 2001
> From: Keith Packard 
> Date: Fri, 13 May 2016 04:25:43 -0700
> Subject: [PATCH xserver] glamor: Disable logic ops when doing compositing [v2]
>
> If the logic op gets left enabled, it overrides the blending
> operation, causing incorrect contents on the display.
>
> v2: Disable only on non-ES2, but disable even for PictOpSrc

Oh, yeah.  Even better.


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: Disable logic ops when doing compositing

[Keith Packard]

Markus Wick  writes:

> Hi,
>
> AFAIK we disable logic ops after each usage, so this seems a bit 
> redundant to me. Did we miss one usages?

Nope, GL_COLOR_LOGIC_OP isn't managed like other glEnable/glDisable
pairs; it's left at the previous value. Someday we should figure out how
we want this to work and make it all consistent.

I'd suggest defining the set of glEnable values that are 'global' and
having every user set them to the desired value. That way, they will get
changed only when actually changing modes, and not just to reset back to
a 'base' state, whatever that means, under an assumption that gl will be
able to easily optimize calls which set the state to the current value,
but that flipping the state between two values may actually cost something.

Right now, in glamor, we use:

GL_BLEND, GL_COLOR_LOGIC_OP, GL_SCISSOR_TEST

> As logic ops aren't very common, this sounds good to me.

It's all inconsistent, which is the worst possible plan.

-- 
-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: Disable logic ops when doing compositing

[Keith Packard]

Eric Anholt  writes:

> Keith Packard  writes:
>
>> If the logic op gets left enabled, it overrides the blending
>> operation, causing incorrect contents on the display.
>>
>> Signed-off-by: Keith Packard 
>
> Reviewed-by: Eric Anholt 

And it's actually wrong (sigh). Need to do this in all paths through
this function (even the PictOpSrc case), and we only want to do it on
non-ES2 flavors. Here's an updated version.

From 1ac2ef66369f36ef132f643969ad176db39babe7 Mon Sep 17 00:00:00 2001
From: Keith Packard 
Date: Fri, 13 May 2016 04:25:43 -0700
Subject: [PATCH xserver] glamor: Disable logic ops when doing compositing [v2]

If the logic op gets left enabled, it overrides the blending
operation, causing incorrect contents on the display.

v2: Disable only on non-ES2, but disable even for PictOpSrc

Signed-off-by: Keith Packard 
---
 glamor/glamor_program.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/glamor/glamor_program.c b/glamor/glamor_program.c
index 0a94de6..322d198 100644
--- a/glamor/glamor_program.c
+++ b/glamor/glamor_program.c
@@ -445,6 +445,7 @@ static struct blendinfo composite_op_info[] = {
 static void
 glamor_set_blend(CARD8 op, glamor_program_alpha alpha, PicturePtr dst)
 {
+glamor_screen_private *glamor_priv = glamor_get_screen_private(dst->pDrawable->pScreen);
 GLenum src_blend, dst_blend;
 struct blendinfo *op_info;
 
@@ -459,6 +460,9 @@ glamor_set_blend(CARD8 op, glamor_program_alpha alpha, PicturePtr dst)
 break;
 }
 
+if (glamor_priv->gl_flavor != GLAMOR_GL_ES2)
+glDisable(GL_COLOR_LOGIC_OP);
+
 if (op == PictOpSrc)
 return;
 
-- 
2.8.0.rc3


-- 
-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: Disable logic ops when doing compositing

[Eric Anholt]

Keith Packard  writes:

> If the logic op gets left enabled, it overrides the blending
> operation, causing incorrect contents on the display.
>
> Signed-off-by: Keith Packard 

Reviewed-by: Eric Anholt 


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: Disable logic ops when doing compositing

[Alex Deucher]

On Fri, May 13, 2016 at 7:29 AM, Keith Packard  wrote:
> If the logic op gets left enabled, it overrides the blending
> operation, causing incorrect contents on the display.
>
> Signed-off-by: Keith Packard 

Makes sense.
Reviewed-by: Alex Deucher 

> ---
>  glamor/glamor_program.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/glamor/glamor_program.c b/glamor/glamor_program.c
> index 0a94de6..a43ee6e 100644
> --- a/glamor/glamor_program.c
> +++ b/glamor/glamor_program.c
> @@ -499,6 +499,7 @@ glamor_set_blend(CARD8 op, glamor_program_alpha alpha, 
> PicturePtr dst)
>  }
>
>  glEnable(GL_BLEND);
> +glDisable(GL_COLOR_LOGIC_OP);
>  glBlendFunc(src_blend, dst_blend);
>  }
>
> --
> 2.8.0.rc3
>
> ___
> 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
___
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: Disable logic ops when doing compositing

[Markus Wick]

Hi,

AFAIK we disable logic ops after each usage, so this seems a bit 
redundant to me. Did we miss one usages? As logic ops aren't very 
common, this sounds good to me.


Am 2016-05-13 13:29, schrieb Keith Packard:

If the logic op gets left enabled, it overrides the blending
operation, causing incorrect contents on the display.

Signed-off-by: Keith Packard 
---
 glamor/glamor_program.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/glamor/glamor_program.c b/glamor/glamor_program.c
index 0a94de6..a43ee6e 100644
--- a/glamor/glamor_program.c
+++ b/glamor/glamor_program.c
@@ -499,6 +499,7 @@ glamor_set_blend(CARD8 op, glamor_program_alpha
alpha, PicturePtr dst)
 }

 glEnable(GL_BLEND);
+glDisable(GL_COLOR_LOGIC_OP);
 glBlendFunc(src_blend, dst_blend);
 }


___
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