diff mbox

[06/18] shader_runner: Refactor handling of fb commands.

Message ID 87d1i4mg2p.fsf@riseup.net
State New
Headers show

Commit Message

Francisco Jerez Nov. 9, 2016, 10:41 p.m. UTC 
Marek Olšák <maraeo@gmail.com> writes:

> On Wed, Nov 9, 2016 at 8:10 PM, Francisco Jerez <currojerez@riseup.net> wrote:

>> Marek Olšák <maraeo@gmail.com> writes:

>>

>>> On Wed, Oct 19, 2016 at 1:36 AM, Francisco Jerez <currojerez@riseup.net> wrote:

>>>> This refactors the implementation of the various "fb" commands to be

>>>> part of a single 'if (parse_str(line, "fb ", ...)) {}' block in order

>>>> to make code-sharing easier among fb subcommands.  Will be more useful

>>>> when we start introducing additional fb subcommands.

>>>> ---

>>>>  tests/shaders/shader_runner.c | 67 ++++++++++++++++++++++++++-----------------

>>>>  1 file changed, 40 insertions(+), 27 deletions(-)

>>>>

>>>> diff --git a/tests/shaders/shader_runner.c b/tests/shaders/shader_runner.c

>>>> index ab2b907..4a2c807 100644

>>>> --- a/tests/shaders/shader_runner.c

>>>> +++ b/tests/shaders/shader_runner.c

>>>> @@ -140,7 +140,7 @@ static bool prog_in_use = false;

>>>>  static bool sso_in_use = false;

>>>>  static GLchar *prog_err_info = NULL;

>>>>  static GLuint vao = 0;

>>>> -static GLuint fbo = 0;

>>>> +static GLuint draw_fbo = 0;

>>>>  static GLint render_width, render_height;

>>>>

>>>>  static bool report_subtests = false;

>>>> @@ -2959,13 +2959,16 @@ piglit_display(void)

>>>>                         do_enable_disable(rest, true);

>>>>                 } else if (sscanf(line, "depthfunc %31s", s) == 1) {

>>>>                         glDepthFunc(piglit_get_gl_enum_from_name(s));

>>>> -               } else if (sscanf(line, "fb tex 2d %d", &tex) == 1) {

>>>> -                       GLenum status;

>>>> +               } else if (parse_str(line, "fb ", &rest)) {

>>>> +               GLuint fbo = 0;

>>>

>>> Wrong indentation. (it can lead to buggy code)

>>>

>>

>> Yeah, the whole block is intentionally indented one tab to the left,

>> it's cleaned up in the next commit.

>>

>>>>

>>>> -                       if (fbo == 0) {

>>>> -                               glGenFramebuffers(1, &fbo);

>>>> -                               glBindFramebuffer(GL_FRAMEBUFFER, fbo);

>>>> -                       }

>>>> +               if (parse_str(rest, "tex 2d ", &rest)) {

>>>> +                       glGenFramebuffers(1, &fbo);

>>>> +                       glBindFramebuffer(GL_FRAMEBUFFER, fbo);

>>>> +

>>>> +                       REQUIRE(parse_int(rest, &tex, &rest),

>>>> +                               "Framebuffer binding command not "

>>>> +                               "understood at: %s\n", rest);

>>>>

>>>>                         glFramebufferTexture2D(GL_FRAMEBUFFER,

>>>>                                                GL_COLOR_ATTACHMENT0,

>>>> @@ -2976,21 +2979,12 @@ piglit_display(void)

>>>>                                 piglit_report_result(PIGLIT_FAIL);

>>>>                         }

>>>>

>>>> -                       status = glCheckFramebufferStatus(GL_FRAMEBUFFER);

>>>> -                       if (status != GL_FRAMEBUFFER_COMPLETE) {

>>>> -                               fprintf(stderr, "incomplete fbo (status 0x%x)\n", status);

>>>> -                               piglit_report_result(PIGLIT_FAIL);

>>>> -                       }

>>>> -

>>>> -                       render_width = get_texture_binding(tex)->width;

>>>> -                       render_height = get_texture_binding(tex)->height;

>>>> -               } else if (sscanf(line, "fb tex layered %d", &tex) == 1) {

>>>> -                       GLenum status;

>>>> +                       w = get_texture_binding(tex)->width;

>>>> +                       h = get_texture_binding(tex)->height;

>>>>

>>>> -                       if (fbo == 0) {

>>>> -                               glGenFramebuffers(1, &fbo);

>>>> -                               glBindFramebuffer(GL_FRAMEBUFFER, fbo);

>>>> -                       }

>>>> +               } else if (sscanf(rest, "tex layered %d", &tex) == 1) {

>>>> +                       glGenFramebuffers(1, &fbo);

>>>> +                       glBindFramebuffer(GL_FRAMEBUFFER, fbo);

>>>>

>>>>                         glFramebufferTexture(GL_FRAMEBUFFER,

>>>>                                              GL_COLOR_ATTACHMENT0,

>>>> @@ -3000,11 +2994,31 @@ piglit_display(void)

>>>>                                 piglit_report_result(PIGLIT_FAIL);

>>>>                         }

>>>>

>>>> -                       status = glCheckFramebufferStatus(GL_FRAMEBUFFER);

>>>> -                       if (status != GL_FRAMEBUFFER_COMPLETE) {

>>>> -                               fprintf(stderr, "incomplete fbo (status 0x%x)\n", status);

>>>> -                               piglit_report_result(PIGLIT_FAIL);

>>>> -                       }

>>>> +                       w = get_texture_binding(tex)->width;

>>>> +                       h = get_texture_binding(tex)->height;

>>>> +

>>>> +               } else {

>>>> +                       fprintf(stderr, "Unknown fb bind subcommand "

>>>> +                               "\"%s\"\n", rest);

>>>> +                       piglit_report_result(PIGLIT_FAIL);

>>>> +               }

>>>> +

>>>> +               const GLenum status = glCheckFramebufferStatus(GL_FRAMEBUFFER);

>>>> +               if (status != GL_FRAMEBUFFER_COMPLETE) {

>>>> +                       fprintf(stderr, "incomplete fbo (status 0x%x)\n", status);

>>>> +                       piglit_report_result(PIGLIT_FAIL);

>>>> +               }

>>>> +

>>>> +               render_width = w;

>>>> +               render_height = h;

>>>> +

>>>> +               /* Delete the previous draw FB in case

>>>> +                * it's no longer reachable.

>>>> +                */

>>>> +               if (draw_fbo != 0)

>>>> +                       glDeleteFramebuffers(1, &draw_fbo);

>>>> +

>>>> +               draw_fbo = fbo;

>>>>

>>>>                 } else if (parse_str(line, "frustum", &rest)) {

>>>>                         parse_floats(rest, c, 6, NULL);

>>>> @@ -3622,7 +3636,6 @@ piglit_init(int argc, char **argv)

>>>>                         sso_in_use = false;

>>>>                         prog_err_info = NULL;

>>>>                         vao = 0;

>>>> -                       fbo = 0;

>>>

>>> This breaks some piglit tests with --process-isolation 0. Adding

>>> "draw_fbo = 0" here fixes that. Is that the right fix?

>>>

>>

>> Interesting, I guess the test breaks while trying to deallocate the FBO

>> From the previous test?  Or does it break in some other way?  In the

>> former case I guess that setting 'draw_fbo = 0' would work around the

>> issue, but it would probably also lead to FBO object leaks.

>

> No idea what's causing the failures, but releasing the FBO should be

> really easy at the end of that block.

>


Does the attached patch fix the problem for you?  I didn't get a full
piglit run to work with --process-isolation 0, so I couldn't confirm
whether it fixes the issue.

> Marek

Comments

Marek Olšák Nov. 9, 2016, 11:01 p.m. UTC | #1 
On Wed, Nov 9, 2016 at 11:41 PM, Francisco Jerez <currojerez@riseup.net> wrote:
>>>>> @@ -3622,7 +3636,6 @@ piglit_init(int argc, char **argv)
>>>>>                         sso_in_use = false;
>>>>>                         prog_err_info = NULL;
>>>>>                         vao = 0;
>>>>> -                       fbo = 0;
>>>>
>>>> This breaks some piglit tests with --process-isolation 0. Adding
>>>> "draw_fbo = 0" here fixes that. Is that the right fix?
>>>>
>>>
>>> Interesting, I guess the test breaks while trying to deallocate the FBO
>>> From the previous test?  Or does it break in some other way?  In the
>>> former case I guess that setting 'draw_fbo = 0' would work around the
>>> issue, but it would probably also lead to FBO object leaks.
>>
>> No idea what's causing the failures, but releasing the FBO should be
>> really easy at the end of that block.
>>
>
> Does the attached patch fix the problem for you?  I didn't get a full
> piglit run to work with --process-isolation 0, so I couldn't confirm
> whether it fixes the issue.

Yes, it fixes the issue. Thanks.

Reviewed-by: Marek Olšák <marek.olsak@amd.com>

Marek
diff mbox

Patch

From c210a937315a2f2eb63926af1cb15f27a4fa3e4c Mon Sep 17 00:00:00 2001
From: Francisco Jerez <currojerez@riseup.net>
Date: Wed, 9 Nov 2016 14:36:47 -0800
Subject: [PATCH] shader_runner: Release FBOs at the end of every subtest run.

---
 tests/shaders/shader_runner.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/tests/shaders/shader_runner.c b/tests/shaders/shader_runner.c
index e2d9849..a835781 100644
--- a/tests/shaders/shader_runner.c
+++ b/tests/shaders/shader_runner.c
@@ -2666,6 +2666,22 @@  setup_ubos(void)
 }
 
 static void
+teardown_fbos(void)
+{
+	if (draw_fbo != 0 &&
+	    draw_fbo != piglit_winsys_fbo)
+		glDeleteFramebuffers(1, &draw_fbo);
+
+	if (read_fbo != 0 &&
+	    read_fbo != piglit_winsys_fbo &&
+	    read_fbo != draw_fbo)
+		glDeleteFramebuffers(1, &read_fbo);
+
+	draw_fbo = 0;
+	read_fbo = 0;
+}
+
+static void
 teardown_ubos(void)
 {
 	if (num_uniform_blocks == 0) {
@@ -3983,6 +3999,7 @@  piglit_init(int argc, char **argv)
 			/* destroy GL objects? */
 			teardown_ubos();
 			teardown_atomics();
+			teardown_fbos();
 		}
 		exit(0);
 	}
-- 
2.10.1