[Mesa-dev] [PATCH shaderdb 3/3] run: shader program file created via GetProgramBinary

[Dongwon Kim]

Thanks for the review. I put my comments below yours.

On Wed, Feb 14, 2018 at 10:56:14AM +0200, Eero Tamminen wrote:
> Hi,
> 
> On 13.02.2018 03:26, Dongwon Kim wrote:
> >extraction of linked binary program to a file using glGetProgramBinary.
> >This file is intended to be loaded by glProgramBinary in the graphic
> >application running on the target system.
> >
> >A new option, '--out=<file name>' is available to be used for specifying
> >the output file name.
> >
> >Signed-off-by: Dongwon Kim <dongwon.kim at intel.com>
> >---
> >  run.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 44 insertions(+), 2 deletions(-)
> >
> >diff --git a/run.c b/run.c
> >index d066567..54575e1 100644
> >--- a/run.c
> >+++ b/run.c
> >@@ -358,18 +358,20 @@ const struct platform platforms[] = {
> >  enum
> >  {
> >      PCI_ID_OVERRIDE_OPTION = CHAR_MAX + 1,
> >+    OUT_PROGRAM_OPTION,
> >  };
> >  const struct option const long_options[] =
> >  {
> >      {"pciid", required_argument, NULL, PCI_ID_OVERRIDE_OPTION},
> >+    {"out", required_argument, NULL, OUT_PROGRAM_OPTION},
> >      {NULL, 0, NULL, 0}
> >  };
> >  void print_usage(const char *prog_name)
> >  {
> >      fprintf(stderr,
> >-            "Usage: %s [-d <device>] [-j <max_threads>] [-o <driver>] [-p <platform>] [--pciid=<chip id of targetted gen arch>] <directories and *.shader_test files>\n",
> >+            "Usage: %s [-d <device>] [-j <max_threads>] [-o <driver>] [-p <platform>] [--pciid=<chip id of targetted gen arch>] [--out=<file name of output shader program>] <directories and *.shader_test files>\n",
> >              prog_name);
> >  }
> >@@ -450,6 +452,7 @@ main(int argc, char **argv)
> >      int opt;
> >      bool platf_overridden = 0;
> >      bool pci_id_overridden = 0;
> >+    char out_file[64] = {0};
> 
> File names can be potentially thousands of chars long.
> 
> Why not just use:
> 	const char *out_file = NULL;
> ?
> 
> >      max_threads = omp_get_max_threads();
> >@@ -518,6 +521,13 @@ main(int argc, char **argv)
> >              setenv("INTEL_DEVID_OVERRIDE", optarg, 1);
> >              pci_id_overridden = 1;
> >              break;
> >+        case OUT_PROGRAM_OPTION:
> >+            if (optarg[0] == 0) {
> >+                  fprintf(stderr, "Output file name is empty.\n");
> >+                  return -1;
> >+            }
> >+            strncpy(out_file, optarg, 64);
> 
> ...and if pointer cannot assigned directly, strdup & assert if that fails.

yeah, your proposal sounds better. I will do so.

> 
> 
> >+            break;
> >          default:
> >              fprintf(stderr, "Unknown option: %x\n", opt);
> >              print_usage(argv[0]);
> >@@ -858,13 +868,13 @@ main(int argc, char **argv)
> >                  }
> >              } else if (type == TYPE_CORE || type == TYPE_COMPAT || type == TYPE_ES) {
> >                  GLuint prog = glCreateProgram();
> >+                GLint param;
> >                  for (unsigned i = 0; i < num_shaders; i++) {
> >                      GLuint s = glCreateShader(shader[i].type);
> >                      glShaderSource(s, 1, &shader[i].text, &shader[i].length);
> >                      glCompileShader(s);
> >-                    GLint param;
> >                      glGetShaderiv(s, GL_COMPILE_STATUS, &param);
> >                      if (unlikely(!param)) {
> >                          GLchar log[4096];
> >@@ -879,6 +889,38 @@ main(int argc, char **argv)
> >                  }
> >                  glLinkProgram(prog);
> >+
> >+                glGetProgramiv(prog, GL_LINK_STATUS, &param);
> >+                if (unlikely(!param)) {
> >+                   GLchar log[4096];
> 
> Maybe add define for log buffer size as it's used in multiple places?

I just followed the existing notation. However, I agree with you on this.
I am going to define a constant and use it instead.

> 
> 
> >+                   GLsizei length;
> >+                   glGetProgramInfoLog(prog, 4096, &length, log);
> 
> 4096 -> sizeof(log)
> 
> 
> >+
> >+                   fprintf(stderr, "ERROR: failed to link progam:\n%s\n",
> >+                           log);
> >+                } else {
> >+                   if (out_file[0] != 0) {
> 
> If changed to pointer, check for NULL.

With assert(outfile != NULL) above, I don't think I actually need this condition
check..

> 
> 
> >+                      char *prog_buf = (char *)malloc(10*1024*1024);
> >+                      GLenum format;
> >+                      GLsizei length;
> >+                      FILE *fp;
> >+
> >+                      glGetProgramBinary(prog, 10*1024*1024, &length, &format, prog_buf);
> 
> Use a define for size instead of magic value.

Yeah, agreed. I will change this.

> 
> >+
> >+                      param = glGetError();
> >+                      if (param != GL_NO_ERROR) {
> >+                         fprintf(stderr, "ERROR: failed to get Program Binary\n");
> >+                      } else {
> >+                         fp = fopen(out_file, "wb");
> >+                         fprintf(stdout, "Binary program is generated (%d Byte).\n", length);
> >+                         fprintf(stdout, "Binary Format is %d\n", format);
> >+                         fprintf(stdout, "Now writing to the file\n");
> >+                         fwrite(prog_buf, sizeof(char), length, fp);
> >+                         fclose(fp);
> >+                      }
> >+                      free(prog_buf);
> >+                   }
> >+                }
> >                  glDeleteProgram(prog);
> >              } else {
> >                  for (unsigned i = 0; i < num_shaders; i++) {
> >
> 
> 
> 	- Eero
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev