Re: [igt-dev] [i-g-t, v2, 02/11] tools/intel_guc_logger: Refactor intel_guc_logger globals into structs

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

On Tue, 2023-01-31 at 19:04 -0500, Dong, Zhanjun wrote:
> See my comments below.
> 
> 
> Regards,
> 
> Zhanjun Dong
> 
> On 2022-12-06 3:58 a.m., Alan Previn wrote:
> > Refactor all of the global variables used in intel_guc_logger
> > into abstractions of structures at thread level, GuC-GT level
> > and global level.
> > 
> > While at it, remove asserts from the non primary thread to ensure
> > process cleanup doesn't get stuck.
> > 
alan:snip

> > -	if (ctl_fd) {
> > -		ret = asprintf(&str, "0x%" PRIx64, val);
> > +	if (tdata->guc->control_fd) {
> > +		ret = asprintf(&str, "0x%" PRIx64, (unsigned long)cmd);
> Why don't use typical int or unsigned int for max compatibility?
alan:sure will change to "int" like patch #1 direction and change the enum names to #defines.
side note: above code will look different, not using asprintf, not needing typecase after i fix
it as per your review comment of patch 1 anyway. I will change to int anyway.
alan:snip

> >   	case 'd':
> > -		discard_oldlogs = true;
> > +		data->discard_oldlogs = true;
> >   		igt_debug("old/boot-time logs will be discarded\n");
> >   		break;
> How about unsupported option(s)?
alan: i believe the igt helper takes care of that.
alan:snip

> >   -	get_guc_subbuf_info();
> > -	process_command_line(argc, argv);
> > +	memset(&gucdata[0], 0, sizeof(struct guc_t));
> > +	gucdata[0].gt_id = 0;
> > +	igt_assert_neq(asprintf(&gucdata[0].fspath, "gt/uc"), -1);
> Depends on platform, name could be gt/uc or gt0/uc, gt1/uc
alan: yeah thanks - this series last rev was published sometime back - but i will fix this
on the next patch that properly handles multi-gt platforms - for this patch #2, I'll
keept defaulting to single-gt product only as above. the next patch needs to be fixed for that.