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....@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, ¶m);
> > if (unlikely(!param)) {
> > GLchar log[4096];
> >@@ -879,6 +889,38 @@ main(int argc, char **argv)
> > }
> > glLinkProgram(prog);
> >+
> >+ glGetProgramiv(prog, GL_LINK_STATUS, ¶m);
> >+ 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
