Re: [Mesa-dev] [PATCH] intel: Set ctx's drawbuffer according to drawables visual

2011-08-12 Thread Chia-I Wu
On Fri, Aug 12, 2011 at 3:59 PM, Chia-I Wu olva...@gmail.com wrote:
 On Fri, Aug 12, 2011 at 3:30 AM, Brian Paul bri...@vmware.com wrote:
 On 08/11/2011 11:09 AM, Ian Romanick wrote:

 -BEGIN PGP SIGNED MESSAGE-
 Hash: SHA1

 On 08/04/2011 06:29 AM, Brian Paul wrote:

 On 08/04/2011 06:31 AM, Benjamin Franzke wrote:

 Fixes https://bugs.freedesktop.org/show_bug.cgi?id=39588

 egl_dri2 creates contexts with a doubleBufferConfig when PIXMAP and
 WINDOW bit is request, so _mesa_init_color sets DrawBuffer[0] to
 GL_BACK.
 If a pixmap surface is created egl_dri2 will use a single buffer config,
 so MakeCurrent has to adjust DrawBuffer[0] to the current drawable.
 ---
   src/mesa/drivers/dri/intel/intel_context.c |    6 ++
   1 files changed, 6 insertions(+), 0 deletions(-)

 diff --git a/src/mesa/drivers/dri/intel/intel_context.c
 b/src/mesa/drivers/dri/intel/intel_context.c
 index fe8be08..0eeffc0 100644
 --- a/src/mesa/drivers/dri/intel/intel_context.c
 +++ b/src/mesa/drivers/dri/intel/intel_context.c
 @@ -970,6 +970,12 @@ intelMakeCurrent(__DRIcontext * driContextPriv,
        readFb = driReadPriv-driverPrivate;
        driContextPriv-dri2.draw_stamp = driDrawPriv-dri2.stamp - 1;
        driContextPriv-dri2.read_stamp = driReadPriv-dri2.stamp - 1;
 +
 +         if (fb-Visual.doubleBufferMode) {
 +            intel-ctx.Color.DrawBuffer[0] = GL_BACK;
 +         } else {
 +            intel-ctx.Color.DrawBuffer[0] = GL_FRONT;
 +         }
         }

         intel_prepare_render(intel);

 This doesn't seem right to me.  We shouldn't be changing context state
 like that during a make-current() call.

 During context initialization we call _mesa_init_color() where we set
 ctx-Color.DrawBuffer[0] to either GL_BACK or GL_FRONT depending on the
 visual's double-buffer flag.  You might investigate why that's not doing
 the job.

 This entire approach is wrong, and it will break GLX.  Page 28 (page 34
 of the PDF) of the GLX 1.4 spec says (emphasis mine):

     No error will be generated if the value of GL DRAW BUFFER in ctx
     indicates a color buffer that is not supported by draw. In this
     case, *all rendering will behave as if GL DRAW BUFFER was set to
     NONE.* Also, no error will be generated if the value of GL READ
     BUFFER in ctx does not correspond to a valid color buffer. Instead,
     when an operation that reads from the color buffer is executed
     (e.g., glReadPixels or glCopyPixels), the pixel values used will be
     undefined until GL READ BUFFER is set to a color buffer that is
     valid in read. Operations that query the value of GL READ BUFFER or
     GL DRAW BUFFER (i.e., glGet, glPushAttrib) use the value set last
     in the context, independent of whether it is a valid buffer in read
     or draw.

 Page 217 of the GL 3.3 spec says:

     For the default framebuffer, in the initial state the draw buffer
     for fragment color zero is BACK if there is a back buffer; FRONT
     if there is no back buffer; and NONE if no default framebuffer is
     associated with the context.

 This seems a little ambiguous and perhaps in conflict with the GLX text.
  I think the right answer is that the context draw buffer (and read
 buffer) setting is initialized based on the drawable the first time the
 context is bound.  It seems like just changing _mesa_init_color to use
 the double-buffer setting from the drawable (instead of the context)
 should be sufficient.

 Yeah, this is all a bit tricky.

 Suppose the context is initially in a double-buffered state and we're
 drawing to the back buffer.  Then we bind the context to a single-buffered
 window.  We need to make sure we don't try to render to the non-existant
 back buffer.  There may be a few places in Mesa where we're not prepared for
 that.

 Anyway, from what I've gathered we should _not_ change the value of
 GL_DRAW_BUFFER in this situation either.


 I'm putting together a couple piglit GLX tests to exercise this.  I
 don't plan to send them to the piglit list until I can test them on a
 non-Mesa driver.  Since I'm still at SIGGRAPH, that won't be until next
 week.

 I also started on a piglit test for this bug got sidetracked. NVIDIA's
 driver seems to do the following:

 1. create double-buffered ctx and window, and make current.
 2. query GL_DRAW_BUFFER returns GL_BACK
 3. create single-buffered window and bind to the context.
 4. query GL_DRAW_BUFFER still returns GL_BACK
 5. drawing to the single-buffered window has no effect.
 I did a little experiments today.  It seems NVIDIA does not have the
 concept of single-buffered or double-buffered contexts, but
 single-buffered or double-buffered drawables.  In GLX spec,
 GLX_DOUBLEBUFFER seems to only apply for drawables too.  This is what
 I did

  1. create a single-buffered drawable
  2. create a context with a GLX_DOUBLEBUFFER visual
  3. make the context and drawable current
  4. query GL_DRAW_BUFFER returns GL_FRONT

 This could conflict with section 4.2.1 of GL spec

Re: [Mesa-dev] [PATCH] intel: Set ctx's drawbuffer according to drawables visual

2011-08-11 Thread Ian Romanick
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 08/04/2011 06:29 AM, Brian Paul wrote:
 On 08/04/2011 06:31 AM, Benjamin Franzke wrote:
 Fixes https://bugs.freedesktop.org/show_bug.cgi?id=39588

 egl_dri2 creates contexts with a doubleBufferConfig when PIXMAP and
 WINDOW bit is request, so _mesa_init_color sets DrawBuffer[0] to
 GL_BACK.
 If a pixmap surface is created egl_dri2 will use a single buffer config,
 so MakeCurrent has to adjust DrawBuffer[0] to the current drawable.
 ---
   src/mesa/drivers/dri/intel/intel_context.c |6 ++
   1 files changed, 6 insertions(+), 0 deletions(-)

 diff --git a/src/mesa/drivers/dri/intel/intel_context.c
 b/src/mesa/drivers/dri/intel/intel_context.c
 index fe8be08..0eeffc0 100644
 --- a/src/mesa/drivers/dri/intel/intel_context.c
 +++ b/src/mesa/drivers/dri/intel/intel_context.c
 @@ -970,6 +970,12 @@ intelMakeCurrent(__DRIcontext * driContextPriv,
readFb = driReadPriv-driverPrivate;
driContextPriv-dri2.draw_stamp = driDrawPriv-dri2.stamp - 1;
driContextPriv-dri2.read_stamp = driReadPriv-dri2.stamp - 1;
 +
 + if (fb-Visual.doubleBufferMode) {
 +intel-ctx.Color.DrawBuffer[0] = GL_BACK;
 + } else {
 +intel-ctx.Color.DrawBuffer[0] = GL_FRONT;
 + }
 }

 intel_prepare_render(intel);
 
 This doesn't seem right to me.  We shouldn't be changing context state
 like that during a make-current() call.
 
 During context initialization we call _mesa_init_color() where we set
 ctx-Color.DrawBuffer[0] to either GL_BACK or GL_FRONT depending on the
 visual's double-buffer flag.  You might investigate why that's not doing
 the job.

This entire approach is wrong, and it will break GLX.  Page 28 (page 34
of the PDF) of the GLX 1.4 spec says (emphasis mine):

No error will be generated if the value of GL DRAW BUFFER in ctx
indicates a color buffer that is not supported by draw. In this
case, *all rendering will behave as if GL DRAW BUFFER was set to
NONE.* Also, no error will be generated if the value of GL READ
BUFFER in ctx does not correspond to a valid color buffer. Instead,
when an operation that reads from the color buffer is executed
(e.g., glReadPixels or glCopyPixels), the pixel values used will be
undefined until GL READ BUFFER is set to a color buffer that is
valid in read. Operations that query the value of GL READ BUFFER or
GL DRAW BUFFER (i.e., glGet, glPushAttrib) use the value set last
in the context, independent of whether it is a valid buffer in read
or draw.

Page 217 of the GL 3.3 spec says:

For the default framebuffer, in the initial state the draw buffer
for fragment color zero is BACK if there is a back buffer; FRONT
if there is no back buffer; and NONE if no default framebuffer is
associated with the context.

This seems a little ambiguous and perhaps in conflict with the GLX text.
 I think the right answer is that the context draw buffer (and read
buffer) setting is initialized based on the drawable the first time the
context is bound.  It seems like just changing _mesa_init_color to use
the double-buffer setting from the drawable (instead of the context)
should be sufficient.

I'm putting together a couple piglit GLX tests to exercise this.  I
don't plan to send them to the piglit list until I can test them on a
non-Mesa driver.  Since I'm still at SIGGRAPH, that won't be until next
week.
-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk5EDMkACgkQX1gOwKyEAw8vVACeMghE21gtQMYJgAkWXBDmz5zB
qgkAnRLyvNKzJwJuQTx1Yk4v7hLeohfz
=XZUQ
-END PGP SIGNATURE-
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev


Re: [Mesa-dev] [PATCH] intel: Set ctx's drawbuffer according to drawables visual

2011-08-11 Thread Brian Paul

On 08/11/2011 11:09 AM, Ian Romanick wrote:

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 08/04/2011 06:29 AM, Brian Paul wrote:

On 08/04/2011 06:31 AM, Benjamin Franzke wrote:

Fixes https://bugs.freedesktop.org/show_bug.cgi?id=39588

egl_dri2 creates contexts with a doubleBufferConfig when PIXMAP and
WINDOW bit is request, so _mesa_init_color sets DrawBuffer[0] to
GL_BACK.
If a pixmap surface is created egl_dri2 will use a single buffer config,
so MakeCurrent has to adjust DrawBuffer[0] to the current drawable.
---
   src/mesa/drivers/dri/intel/intel_context.c |6 ++
   1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/src/mesa/drivers/dri/intel/intel_context.c
b/src/mesa/drivers/dri/intel/intel_context.c
index fe8be08..0eeffc0 100644
--- a/src/mesa/drivers/dri/intel/intel_context.c
+++ b/src/mesa/drivers/dri/intel/intel_context.c
@@ -970,6 +970,12 @@ intelMakeCurrent(__DRIcontext * driContextPriv,
readFb = driReadPriv-driverPrivate;
driContextPriv-dri2.draw_stamp = driDrawPriv-dri2.stamp - 1;
driContextPriv-dri2.read_stamp = driReadPriv-dri2.stamp - 1;
+
+ if (fb-Visual.doubleBufferMode) {
+intel-ctx.Color.DrawBuffer[0] = GL_BACK;
+ } else {
+intel-ctx.Color.DrawBuffer[0] = GL_FRONT;
+ }
 }

 intel_prepare_render(intel);


This doesn't seem right to me.  We shouldn't be changing context state
like that during a make-current() call.

During context initialization we call _mesa_init_color() where we set
ctx-Color.DrawBuffer[0] to either GL_BACK or GL_FRONT depending on the
visual's double-buffer flag.  You might investigate why that's not doing
the job.


This entire approach is wrong, and it will break GLX.  Page 28 (page 34
of the PDF) of the GLX 1.4 spec says (emphasis mine):

 No error will be generated if the value of GL DRAW BUFFER in ctx
 indicates a color buffer that is not supported by draw. In this
 case, *all rendering will behave as if GL DRAW BUFFER was set to
 NONE.* Also, no error will be generated if the value of GL READ
 BUFFER in ctx does not correspond to a valid color buffer. Instead,
 when an operation that reads from the color buffer is executed
 (e.g., glReadPixels or glCopyPixels), the pixel values used will be
 undefined until GL READ BUFFER is set to a color buffer that is
 valid in read. Operations that query the value of GL READ BUFFER or
 GL DRAW BUFFER (i.e., glGet, glPushAttrib) use the value set last
 in the context, independent of whether it is a valid buffer in read
 or draw.

Page 217 of the GL 3.3 spec says:

 For the default framebuffer, in the initial state the draw buffer
 for fragment color zero is BACK if there is a back buffer; FRONT
 if there is no back buffer; and NONE if no default framebuffer is
 associated with the context.

This seems a little ambiguous and perhaps in conflict with the GLX text.
  I think the right answer is that the context draw buffer (and read
buffer) setting is initialized based on the drawable the first time the
context is bound.  It seems like just changing _mesa_init_color to use
the double-buffer setting from the drawable (instead of the context)
should be sufficient.


Yeah, this is all a bit tricky.

Suppose the context is initially in a double-buffered state and we're 
drawing to the back buffer.  Then we bind the context to a 
single-buffered window.  We need to make sure we don't try to render 
to the non-existant back buffer.  There may be a few places in Mesa 
where we're not prepared for that.


Anyway, from what I've gathered we should _not_ change the value of 
GL_DRAW_BUFFER in this situation either.




I'm putting together a couple piglit GLX tests to exercise this.  I
don't plan to send them to the piglit list until I can test them on a
non-Mesa driver.  Since I'm still at SIGGRAPH, that won't be until next
week.


I also started on a piglit test for this bug got sidetracked. NVIDIA's 
driver seems to do the following:


1. create double-buffered ctx and window, and make current.
2. query GL_DRAW_BUFFER returns GL_BACK
3. create single-buffered window and bind to the context.
4. query GL_DRAW_BUFFER still returns GL_BACK
5. drawing to the single-buffered window has no effect.

-Brian
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev


[Mesa-dev] [PATCH] intel: Set ctx's drawbuffer according to drawables visual

2011-08-04 Thread Benjamin Franzke
Fixes https://bugs.freedesktop.org/show_bug.cgi?id=39588

egl_dri2 creates contexts with a doubleBufferConfig when PIXMAP and
WINDOW bit is request, so _mesa_init_color sets DrawBuffer[0] to
GL_BACK.
If a pixmap surface is created egl_dri2 will use a single buffer config,
so MakeCurrent has to adjust DrawBuffer[0] to the current drawable.
---
 src/mesa/drivers/dri/intel/intel_context.c |6 ++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/src/mesa/drivers/dri/intel/intel_context.c 
b/src/mesa/drivers/dri/intel/intel_context.c
index fe8be08..0eeffc0 100644
--- a/src/mesa/drivers/dri/intel/intel_context.c
+++ b/src/mesa/drivers/dri/intel/intel_context.c
@@ -970,6 +970,12 @@ intelMakeCurrent(__DRIcontext * driContextPriv,
 readFb = driReadPriv-driverPrivate;
 driContextPriv-dri2.draw_stamp = driDrawPriv-dri2.stamp - 1;
 driContextPriv-dri2.read_stamp = driReadPriv-dri2.stamp - 1;
+
+ if (fb-Visual.doubleBufferMode) {
+intel-ctx.Color.DrawBuffer[0] = GL_BACK;
+ } else {
+intel-ctx.Color.DrawBuffer[0] = GL_FRONT;
+ }
   }
 
   intel_prepare_render(intel);
-- 
1.7.3.4

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev


Re: [Mesa-dev] [PATCH] intel: Set ctx's drawbuffer according to drawables visual

2011-08-04 Thread Benjamin Franzke
2011/8/4 Brian Paul bri...@vmware.com:
 On 08/04/2011 06:31 AM, Benjamin Franzke wrote:

 Fixes https://bugs.freedesktop.org/show_bug.cgi?id=39588

 egl_dri2 creates contexts with a doubleBufferConfig when PIXMAP and
 WINDOW bit is request, so _mesa_init_color sets DrawBuffer[0] to
 GL_BACK.
 If a pixmap surface is created egl_dri2 will use a single buffer config,
 so MakeCurrent has to adjust DrawBuffer[0] to the current drawable.
 ---
  src/mesa/drivers/dri/intel/intel_context.c |    6 ++
  1 files changed, 6 insertions(+), 0 deletions(-)

 diff --git a/src/mesa/drivers/dri/intel/intel_context.c
 b/src/mesa/drivers/dri/intel/intel_context.c
 index fe8be08..0eeffc0 100644
 --- a/src/mesa/drivers/dri/intel/intel_context.c
 +++ b/src/mesa/drivers/dri/intel/intel_context.c
 @@ -970,6 +970,12 @@ intelMakeCurrent(__DRIcontext * driContextPriv,
         readFb = driReadPriv-driverPrivate;
         driContextPriv-dri2.draw_stamp = driDrawPriv-dri2.stamp - 1;
         driContextPriv-dri2.read_stamp = driReadPriv-dri2.stamp - 1;
 +
 +         if (fb-Visual.doubleBufferMode) {
 +            intel-ctx.Color.DrawBuffer[0] = GL_BACK;
 +         } else {
 +            intel-ctx.Color.DrawBuffer[0] = GL_FRONT;
 +         }
        }

        intel_prepare_render(intel);

 This doesn't seem right to me.  We shouldn't be changing context state like
 that during a make-current() call.

 During context initialization we call _mesa_init_color() where we set
 ctx-Color.DrawBuffer[0] to either GL_BACK or GL_FRONT depending on the
 visual's double-buffer flag.  You might investigate why that's not doing the
 job.

Yea, I saw that, but the problem is it sets GL_BACK/GL_FRONT depending
on the contexts config,
which may be different from the config used for the drawable (as
described in the commit message).

So mixing configs could be defined as invalid, but its also allowed in
src/mesa/main/context.c:1324

Furthermore st/mesa also modifes the state in make_current, see
st_manager.c:781.

Any ideas where to put it instead?


 -Brian

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev


Re: [Mesa-dev] [PATCH] intel: Set ctx's drawbuffer according to drawables visual

2011-08-04 Thread Benjamin Franzke
2011/8/4 Brian Paul bri...@vmware.com:
 On 08/04/2011 07:39 AM, Benjamin Franzke wrote:

 2011/8/4 Brian Paulbri...@vmware.com:

 On 08/04/2011 06:31 AM, Benjamin Franzke wrote:

 Fixes https://bugs.freedesktop.org/show_bug.cgi?id=39588

 egl_dri2 creates contexts with a doubleBufferConfig when PIXMAP and
 WINDOW bit is request, so _mesa_init_color sets DrawBuffer[0] to
 GL_BACK.
 If a pixmap surface is created egl_dri2 will use a single buffer config,
 so MakeCurrent has to adjust DrawBuffer[0] to the current drawable.
 ---
  src/mesa/drivers/dri/intel/intel_context.c |    6 ++
  1 files changed, 6 insertions(+), 0 deletions(-)

 diff --git a/src/mesa/drivers/dri/intel/intel_context.c
 b/src/mesa/drivers/dri/intel/intel_context.c
 index fe8be08..0eeffc0 100644
 --- a/src/mesa/drivers/dri/intel/intel_context.c
 +++ b/src/mesa/drivers/dri/intel/intel_context.c
 @@ -970,6 +970,12 @@ intelMakeCurrent(__DRIcontext * driContextPriv,
         readFb = driReadPriv-driverPrivate;
         driContextPriv-dri2.draw_stamp = driDrawPriv-dri2.stamp - 1;
         driContextPriv-dri2.read_stamp = driReadPriv-dri2.stamp - 1;
 +
 +         if (fb-Visual.doubleBufferMode) {
 +            intel-ctx.Color.DrawBuffer[0] = GL_BACK;
 +         } else {
 +            intel-ctx.Color.DrawBuffer[0] = GL_FRONT;
 +         }
        }

        intel_prepare_render(intel);

 This doesn't seem right to me.  We shouldn't be changing context state
 like
 that during a make-current() call.

 During context initialization we call _mesa_init_color() where we set
 ctx-Color.DrawBuffer[0] to either GL_BACK or GL_FRONT depending on the
 visual's double-buffer flag.  You might investigate why that's not doing
 the
 job.

 Yea, I saw that, but the problem is it sets GL_BACK/GL_FRONT depending
 on the contexts config,
 which may be different from the config used for the drawable (as
 described in the commit message).

 So mixing configs could be defined as invalid, but its also allowed in
 src/mesa/main/context.c:1324

 Furthermore st/mesa also modifes the state in make_current, see
 st_manager.c:781.

 Any ideas where to put it instead?

 OK, I see.  How about doing something like this instead:

 ctx-Color.DrawBuffer[0] = ctx-DrawBuffer-ColorDrawBuffer[0];

 The gl_framebuffer also carries the current drawbuffer state (since
 GL_EXT_framebuffer_object came along).  The framebuffer's state gets set
 upon creation and when glDrawBuffer() is called.

 I think this is the behaviour that I'd expect if I were alternately
 rendering to single and double-buffered windows with one context.

 What do you think?

That looks like the better solution.

I now think its not really the drivers task, to update the contexts
Draw/ReadBuffer,
so I'd put it directly into _mesa_make_current. New patch attached.

Of course this implies that the framebuffer's
ColorDrawBuffer/ColorReadBuffer is setup
properly by all drivers.
So do you think its safe to always copy those over in _mesa_make_current?

Thanks so far.

 BTW, we'd probably want to copy/update all elements of the ColorDrawBuffer[]
 array plus the ColorReadBuffer state.

 -Brian



0001-mesa-Apply-drawables-draw-readbuffers-to-context-in-.patch
Description: Binary data
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev


Re: [Mesa-dev] [PATCH] intel: Set ctx's drawbuffer according to drawables visual

2011-08-04 Thread Eric Anholt
On Thu,  4 Aug 2011 14:31:01 +0200, Benjamin Franzke 
benjaminfran...@googlemail.com wrote:
 Fixes https://bugs.freedesktop.org/show_bug.cgi?id=39588
 
 egl_dri2 creates contexts with a doubleBufferConfig when PIXMAP and
 WINDOW bit is request, so _mesa_init_color sets DrawBuffer[0] to
 GL_BACK.
 If a pixmap surface is created egl_dri2 will use a single buffer config,
 so MakeCurrent has to adjust DrawBuffer[0] to the current drawable.
 ---
  src/mesa/drivers/dri/intel/intel_context.c |6 ++
  1 files changed, 6 insertions(+), 0 deletions(-)
 
 diff --git a/src/mesa/drivers/dri/intel/intel_context.c 
 b/src/mesa/drivers/dri/intel/intel_context.c
 index fe8be08..0eeffc0 100644
 --- a/src/mesa/drivers/dri/intel/intel_context.c
 +++ b/src/mesa/drivers/dri/intel/intel_context.c
 @@ -970,6 +970,12 @@ intelMakeCurrent(__DRIcontext * driContextPriv,
readFb = driReadPriv-driverPrivate;
driContextPriv-dri2.draw_stamp = driDrawPriv-dri2.stamp - 1;
driContextPriv-dri2.read_stamp = driReadPriv-dri2.stamp - 1;
 +
 + if (fb-Visual.doubleBufferMode) {
 +intel-ctx.Color.DrawBuffer[0] = GL_BACK;
 + } else {
 +intel-ctx.Color.DrawBuffer[0] = GL_FRONT;
 + }
}

Something touching this seems like it should live in _mesa_make_current,
not as a driver workaround.


pgpzYuxltFRgk.pgp
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev