From patchwork Wed Nov 9 22:41:34 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Francisco Jerez X-Patchwork-Id: 81578 Delivered-To: patch@linaro.org Received: by 10.140.97.165 with SMTP id m34csp440736qge; Wed, 9 Nov 2016 14:45:35 -0800 (PST) X-Received: by 10.99.120.75 with SMTP id t72mr29754957pgc.17.1478731535086; Wed, 09 Nov 2016 14:45:35 -0800 (PST) Return-Path: Received: from gabe.freedesktop.org (gabe.freedesktop.org. [131.252.210.177]) by mx.google.com with ESMTPS id kb6si1273189pab.157.2016.11.09.14.45.34 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 09 Nov 2016 14:45:35 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of piglit-bounces@lists.freedesktop.org designates 131.252.210.177 as permitted sender) client-ip=131.252.210.177; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@riseup.net; spf=pass (google.com: best guess record for domain of piglit-bounces@lists.freedesktop.org designates 131.252.210.177 as permitted sender) smtp.mailfrom=piglit-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id A21F86E070; Wed, 9 Nov 2016 22:45:34 +0000 (UTC) X-Original-To: piglit@lists.freedesktop.org Delivered-To: piglit@lists.freedesktop.org Received: from mx1.riseup.net (mx1.riseup.net [198.252.153.129]) by gabe.freedesktop.org (Postfix) with ESMTPS id 2A14C6E070 for ; Wed, 9 Nov 2016 22:45:33 +0000 (UTC) Received: from cotinga.riseup.net (unknown [10.0.1.164]) (using TLSv1 with cipher ECDHE-RSA-AES256-SHA (256/256 bits)) (Client CN "*.riseup.net", Issuer "COMODO RSA Domain Validation Secure Server CA" (verified OK)) by mx1.riseup.net (Postfix) with ESMTPS id 991851A1C38; Wed, 9 Nov 2016 22:45:32 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=riseup.net; s=squak; t=1478731532; bh=bCkoAEh1SuXte5jgec9+3IWHXYjOPZmUoD65l2tonNg=; h=From:To:Cc:Subject:In-Reply-To:References:Date:From; b=KmAudpy0qbB1/BoECg4EA8J53jXx0wop8a1MJYIJUlYIiVvfX0t3I06z9GLgbSgES NQa3deWoRlQ2d8vcAhF9l0RYwFu5TE/DJn29OQY/9rK2us0uNOVAGQU9meyp3fYSM2 doZhwLU9Z6Uw1hG54inVAiQ3kfsMY7iaCPirp4bM= Received: from [127.0.0.1] (localhost [127.0.0.1]) (Authenticated sender: currojerez) with ESMTPSA id 688F0400EF From: Francisco Jerez To: Marek =?utf-8?B?T2zFocOhaw==?= In-Reply-To: References: <20161018233644.15946-1-currojerez@riseup.net> <20161018233644.15946-7-currojerez@riseup.net> <87pom4mpv1.fsf@riseup.net> Date: Wed, 09 Nov 2016 14:41:34 -0800 Message-ID: <87d1i4mg2p.fsf@riseup.net> MIME-Version: 1.0 Cc: piglit Subject: Re: [Piglit] [PATCH 06/18] shader_runner: Refactor handling of fb commands. X-BeenThere: piglit@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: piglit-bounces@lists.freedesktop.org Sender: "Piglit" Marek Olšák writes: > On Wed, Nov 9, 2016 at 8:10 PM, Francisco Jerez wrote: >> Marek Olšák writes: >> >>> On Wed, Oct 19, 2016 at 1:36 AM, Francisco Jerez 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 Reviewed-by: Marek Olšák >From c210a937315a2f2eb63926af1cb15f27a4fa3e4c Mon Sep 17 00:00:00 2001 From: Francisco Jerez 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