[Piglit] [PATCH 06/18] shader_runner: Refactor handling of fb commands.
Francisco Jerez
currojerez at riseup.net
Wed Nov 9 22:41:34 UTC 2016
Marek Olšák <maraeo at gmail.com> writes:
> On Wed, Nov 9, 2016 at 8:10 PM, Francisco Jerez <currojerez at riseup.net> wrote:
>> Marek Olšák <maraeo at gmail.com> writes:
>>
>>> On Wed, Oct 19, 2016 at 1:36 AM, Francisco Jerez <currojerez at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-shader_runner-Release-FBOs-at-the-end-of-every-subte.patch
Type: text/x-diff
Size: 1110 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/piglit/attachments/20161109/33b0da9b/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/piglit/attachments/20161109/33b0da9b/attachment.sig>
More information about the Piglit
mailing list