Re: [igt-dev] [i-g-t, v2, 01/11] tools/intel_guc_logger: Re-enable basic functionality

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

thanks for reviewing - will rerev with below fixes

On Fri, 2023-01-20 at 15:26 -0500, Dong, Zhanjun wrote:
> See my comments inline below.
> 
> 
> Regards,
> 
> Zhanjun Dong
> 
> 
> On 2022-12-06 3:58 a.m., Alan Previn wrote:
> > Fix these multiple issues to get basic functionality up
> > and running on GuC using:
> > 
> > 
> > 
alan:snip
> > +	subbuf_size = tmp[0];
> > +	subbuf_count = tmp[1];
> How about define tmp[x] the same data type as subbuf_xxx to avoid type 
> rang diff?
> > 
> > 
sure - will fix

alan:snip
> > -	ret = write(control_fd, data, ret);
> > -	igt_assert_f(ret > 0, "couldn't write to the log control file\n");
> > +static void guc_log_control(bool enable, uint32_t log_level)
> How about to have the same log_level type among the file?

right - will fix this.

alan:snip
> > +
> 
> val could only be 0 or 1 in this function now, no uint64 needed, it 
> could be something like:
> 
>      str = val? "0x1" : "0x0";
> 
> No string alloc/free needed.

alan: actually in coming patches more we end up with 3 options "0x1", "0x2" or "0x3".
instead, i could add a local array of 'const char * array_strings = { "0x1", "0x2", "0x3" };'
and use string 'array_strings[val-1]' (after verifying val)?
since its a known short list, we dont need to use the asprintf

alan:snip
> > @@ -404,6 +476,11 @@ int main(int argc, char **argv)
> >   	int nfds;
> >   	int ret;
> >   
> > +	drm_fd = drm_open_driver(DRIVER_INTEL);
> > +	igt_assert(drm_fd != -1);
> > +	igt_assert_neq(asprintf(&gucfspath, "gt0/uc"), -1);
> > +
> > +	get_guc_subbuf_info();
> >   	process_command_line(argc, argv);
> >   
> Can we parse options before open driver? When run with -h, open driver 
> is not expected.

alan: as per our offline joint debug: yes definitely :)