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

[Dongwon Kim]

Thanks for the review, Ken
I agree on most of your proposals.
I will upload another version shortly.

On Wed, Mar 14, 2018 at 03:10:25PM -0700, Kenneth Graunke wrote:
> On Monday, February 26, 2018 2:17:05 PM PDT 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.
> > 
> > To enable this feature, a new option '--bin' has to be passed to the
> > program execution.
> > 
> > v2: 1. define MAX_LOG_LEN and use it as the size of gl log
> > 2. define MAX_PROG_SIZE and use it as the max size of extracted
> >shader_program
> > 3. out_file is now pointer allocated by strdup for the file name
> > 
> > v3: 1. automatically using original shader test file's name +  ".bin"
> >as a filename for program binary - better way to cover the case
> >with batch compilation of many shader test files in the same
> >directory
> > 2. remove --out= since it is now unnecessary (due to v3-1.)
> >to provide custom file name. Instead, option, "--bin", which is
> >basically a flag that enables getting program binary as a file.
> > 3. Now it tries to get the length of binary by reading program's
> >GL_PROGRAM_BINARY_LENGTH_OES parameter
> > 
> > Signed-off-by: Dongwon Kim 
> > ---
> >  run.c | 68 
> > +++
> >  1 file changed, 64 insertions(+), 4 deletions(-)
> > 
> > diff --git a/run.c b/run.c
> > index d066567..bbab5d9 100644
> > --- a/run.c
> > +++ b/run.c
> > @@ -52,6 +52,9 @@
> >  
> >  #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
> >  
> > +#define MAX_LOG_LEN 4096
> > +#define MAX_PROG_SIZE (10*1024*1024) /* maximum 10MB for shader program */
> > +
> >  struct context_info {
> >  char *extension_string;
> >  int extension_string_len;
> > @@ -358,18 +361,20 @@ const struct platform platforms[] = {
> >  enum
> >  {
> >  PCI_ID_OVERRIDE_OPTION = CHAR_MAX + 1,
> > +LOADABLE_PROGRAM_BINARY_OPTION,
> >  };
> >  
> >  const struct option const long_options[] =
> >  {
> >  {"pciid", required_argument, NULL, PCI_ID_OVERRIDE_OPTION},
> > +{"bin", no_argument, NULL, LOADABLE_PROGRAM_BINARY_OPTION},
> 
> This sounds like we're loading binaries.  Can we call it
> GENERATE_PROGRAM_BINARY_OPTION instead?

Yeah, I will change this.

> 
> >  {NULL, 0, NULL, 0}
> >  };
> >  
> >  void print_usage(const char *prog_name)
> >  {
> >  fprintf(stderr,
> > -"Usage: %s [-d ] [-j ] [-o ] [-p 
> > ] [--pciid=]  > *.shader_test files>\n",
> > +"Usage: %s [-d ] [-j ] [-o ] [-p 
> > ] [--pciid=]  > *.shader_test files>\n",
> >  prog_name);
> >  }
> >  
> > @@ -450,6 +455,7 @@ main(int argc, char **argv)
> >  int opt;
> >  bool platf_overridden = 0;
> >  bool pci_id_overridden = 0;
> > +bool enable_prog_bin = 0;
> 
> Maybe generate_prog_bin here as well.

sure.

> 
> >  
> >  max_threads = omp_get_max_threads();
> >  
> > @@ -518,6 +524,9 @@ main(int argc, char **argv)
> >  setenv("INTEL_DEVID_OVERRIDE", optarg, 1);
> >  pci_id_overridden = 1;
> >  break;
> > +case LOADABLE_PROGRAM_BINARY_OPTION:
> > +enable_prog_bin = 1;
> > +break;
> >  default:
> >  fprintf(stderr, "Unknown option: %x\n", opt);
> >  print_usage(argv[0]);
> > @@ -858,18 +867,18 @@ main(int argc, char **argv)
> >  }
> >  } else if (type == TYPE_CORE || type == TYPE_COMPAT || type == 
> > TYPE_ES) {
> >  GLuint prog = glCreateProgram();
> > +GLint param;
> 
> So...putting this here means that you're not going to support generating
> program binaries for SSO-based programs.  That seems a bit unfortunate...
> 

I can consider this later.

> >  
> >  for (unsigned i = 0; i < num_shaders; i++) {
> >  GLuint s = glCreateShader(shader[i].type);
> >  glShaderSource(s, 1, [i].text, 
> > [i].length);
> >  glCompileShader(s);
> >  
> > -GLint param;
> >  glGetShaderiv(s, GL_COMPILE_STATUS, );
> >  if (unlikely(!param)) {
> > -GLchar log[4096];
> > +GLchar log[MAX_LOG_LEN];
> >  GLsizei length;
> > -glGetShaderInfoLog(s, 4096, , log);
> > +glGetShaderInfoLog(s, sizeof(log), , log);
> 
> It would be nice to make a helper function for getting the info log and
> printing an error, since you've now got it twice.  Should probably be a
> separate patch (and include the MAX_LOG_LEN change).
> 

I will work on it (another patch.)

> >  
> >  fprintf(stderr, "ERROR: %s failed 

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

[Kenneth Graunke]

On Monday, February 26, 2018 2:17:05 PM PDT 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.
> 
> To enable this feature, a new option '--bin' has to be passed to the
> program execution.
> 
> v2: 1. define MAX_LOG_LEN and use it as the size of gl log
> 2. define MAX_PROG_SIZE and use it as the max size of extracted
>shader_program
> 3. out_file is now pointer allocated by strdup for the file name
> 
> v3: 1. automatically using original shader test file's name +  ".bin"
>as a filename for program binary - better way to cover the case
>with batch compilation of many shader test files in the same
>directory
> 2. remove --out= since it is now unnecessary (due to v3-1.)
>to provide custom file name. Instead, option, "--bin", which is
>basically a flag that enables getting program binary as a file.
> 3. Now it tries to get the length of binary by reading program's
>GL_PROGRAM_BINARY_LENGTH_OES parameter
> 
> Signed-off-by: Dongwon Kim 
> ---
>  run.c | 68 
> +++
>  1 file changed, 64 insertions(+), 4 deletions(-)
> 
> diff --git a/run.c b/run.c
> index d066567..bbab5d9 100644
> --- a/run.c
> +++ b/run.c
> @@ -52,6 +52,9 @@
>  
>  #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>  
> +#define MAX_LOG_LEN 4096
> +#define MAX_PROG_SIZE (10*1024*1024) /* maximum 10MB for shader program */
> +
>  struct context_info {
>  char *extension_string;
>  int extension_string_len;
> @@ -358,18 +361,20 @@ const struct platform platforms[] = {
>  enum
>  {
>  PCI_ID_OVERRIDE_OPTION = CHAR_MAX + 1,
> +LOADABLE_PROGRAM_BINARY_OPTION,
>  };
>  
>  const struct option const long_options[] =
>  {
>  {"pciid", required_argument, NULL, PCI_ID_OVERRIDE_OPTION},
> +{"bin", no_argument, NULL, LOADABLE_PROGRAM_BINARY_OPTION},

This sounds like we're loading binaries.  Can we call it
GENERATE_PROGRAM_BINARY_OPTION instead?

>  {NULL, 0, NULL, 0}
>  };
>  
>  void print_usage(const char *prog_name)
>  {
>  fprintf(stderr,
> -"Usage: %s [-d ] [-j ] [-o ] [-p 
> ] [--pciid=]  *.shader_test files>\n",
> +"Usage: %s [-d ] [-j ] [-o ] [-p 
> ] [--pciid=]  *.shader_test files>\n",
>  prog_name);
>  }
>  
> @@ -450,6 +455,7 @@ main(int argc, char **argv)
>  int opt;
>  bool platf_overridden = 0;
>  bool pci_id_overridden = 0;
> +bool enable_prog_bin = 0;

Maybe generate_prog_bin here as well.

>  
>  max_threads = omp_get_max_threads();
>  
> @@ -518,6 +524,9 @@ main(int argc, char **argv)
>  setenv("INTEL_DEVID_OVERRIDE", optarg, 1);
>  pci_id_overridden = 1;
>  break;
> +case LOADABLE_PROGRAM_BINARY_OPTION:
> +enable_prog_bin = 1;
> +break;
>  default:
>  fprintf(stderr, "Unknown option: %x\n", opt);
>  print_usage(argv[0]);
> @@ -858,18 +867,18 @@ main(int argc, char **argv)
>  }
>  } else if (type == TYPE_CORE || type == TYPE_COMPAT || type == 
> TYPE_ES) {
>  GLuint prog = glCreateProgram();
> +GLint param;

So...putting this here means that you're not going to support generating
program binaries for SSO-based programs.  That seems a bit unfortunate...

>  
>  for (unsigned i = 0; i < num_shaders; i++) {
>  GLuint s = glCreateShader(shader[i].type);
>  glShaderSource(s, 1, [i].text, [i].length);
>  glCompileShader(s);
>  
> -GLint param;
>  glGetShaderiv(s, GL_COMPILE_STATUS, );
>  if (unlikely(!param)) {
> -GLchar log[4096];
> +GLchar log[MAX_LOG_LEN];
>  GLsizei length;
> -glGetShaderInfoLog(s, 4096, , log);
> +glGetShaderInfoLog(s, sizeof(log), , log);

It would be nice to make a helper function for getting the info log and
printing an error, since you've now got it twice.  Should probably be a
separate patch (and include the MAX_LOG_LEN change).

>  
>  fprintf(stderr, "ERROR: %s failed to compile:\n%s\n",
>  current_shader_name, log);
> @@ -879,6 +888,57 @@ main(int argc, char **argv)
>  }
>  
>  glLinkProgram(prog);
> +
> +glGetProgramiv(prog, GL_LINK_STATUS, );
> +if (unlikely(!param)) {
> +   GLchar log[MAX_LOG_LEN];
> +   GLsizei length;
> +   glGetProgramInfoLog(prog, sizeof(log), , log);
> +
> +   fprintf(stderr, "ERROR: failed to link 

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

[Dongwon Kim]

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.

To enable this feature, a new option '--bin' has to be passed to the
program execution.

v2: 1. define MAX_LOG_LEN and use it as the size of gl log
2. define MAX_PROG_SIZE and use it as the max size of extracted
   shader_program
3. out_file is now pointer allocated by strdup for the file name

v3: 1. automatically using original shader test file's name +  ".bin"
   as a filename for program binary - better way to cover the case
   with batch compilation of many shader test files in the same
   directory
2. remove --out= since it is now unnecessary (due to v3-1.)
   to provide custom file name. Instead, option, "--bin", which is
   basically a flag that enables getting program binary as a file.
3. Now it tries to get the length of binary by reading program's
   GL_PROGRAM_BINARY_LENGTH_OES parameter

Signed-off-by: Dongwon Kim 
---
 run.c | 68 +++
 1 file changed, 64 insertions(+), 4 deletions(-)

diff --git a/run.c b/run.c
index d066567..bbab5d9 100644
--- a/run.c
+++ b/run.c
@@ -52,6 +52,9 @@
 
 #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
 
+#define MAX_LOG_LEN 4096
+#define MAX_PROG_SIZE (10*1024*1024) /* maximum 10MB for shader program */
+
 struct context_info {
 char *extension_string;
 int extension_string_len;
@@ -358,18 +361,20 @@ const struct platform platforms[] = {
 enum
 {
 PCI_ID_OVERRIDE_OPTION = CHAR_MAX + 1,
+LOADABLE_PROGRAM_BINARY_OPTION,
 };
 
 const struct option const long_options[] =
 {
 {"pciid", required_argument, NULL, PCI_ID_OVERRIDE_OPTION},
+{"bin", no_argument, NULL, LOADABLE_PROGRAM_BINARY_OPTION},
 {NULL, 0, NULL, 0}
 };
 
 void print_usage(const char *prog_name)
 {
 fprintf(stderr,
-"Usage: %s [-d ] [-j ] [-o ] [-p 
] [--pciid=] \n",
+"Usage: %s [-d ] [-j ] [-o ] [-p 
] [--pciid=] \n",
 prog_name);
 }
 
@@ -450,6 +455,7 @@ main(int argc, char **argv)
 int opt;
 bool platf_overridden = 0;
 bool pci_id_overridden = 0;
+bool enable_prog_bin = 0;
 
 max_threads = omp_get_max_threads();
 
@@ -518,6 +524,9 @@ main(int argc, char **argv)
 setenv("INTEL_DEVID_OVERRIDE", optarg, 1);
 pci_id_overridden = 1;
 break;
+case LOADABLE_PROGRAM_BINARY_OPTION:
+enable_prog_bin = 1;
+break;
 default:
 fprintf(stderr, "Unknown option: %x\n", opt);
 print_usage(argv[0]);
@@ -858,18 +867,18 @@ 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, [i].text, [i].length);
 glCompileShader(s);
 
-GLint param;
 glGetShaderiv(s, GL_COMPILE_STATUS, );
 if (unlikely(!param)) {
-GLchar log[4096];
+GLchar log[MAX_LOG_LEN];
 GLsizei length;
-glGetShaderInfoLog(s, 4096, , log);
+glGetShaderInfoLog(s, sizeof(log), , log);
 
 fprintf(stderr, "ERROR: %s failed to compile:\n%s\n",
 current_shader_name, log);
@@ -879,6 +888,57 @@ main(int argc, char **argv)
 }
 
 glLinkProgram(prog);
+
+glGetProgramiv(prog, GL_LINK_STATUS, );
+if (unlikely(!param)) {
+   GLchar log[MAX_LOG_LEN];
+   GLsizei length;
+   glGetProgramInfoLog(prog, sizeof(log), , log);
+
+   fprintf(stderr, "ERROR: failed to link progam:\n%s\n",
+   log);
+} else if (enable_prog_bin) { /* generate program binary if 
enable_prog_bin == true */
+   char *prog_buf;
+   GLenum format;
+   GLsizei length = 0;
+   FILE *fp;
+
+   glGetProgramiv(prog,
+  (type == TYPE_ES) ? 
GL_PROGRAM_BINARY_LENGTH_OES :
+  GL_PROGRAM_BINARY_LENGTH, );
+
+   if (glGetError() != GL_NO_ERROR)
+   length = MAX_PROG_SIZE;
+
+   prog_buf = (char *)malloc(length);
+   glGetProgramBinary(prog, length, , , 
prog_buf);
+
+   if (glGetError() != GL_NO_ERROR) {
+  fprintf(stderr, "ERROR: failed to