Re: [PATCH i-g-t 5/5] tools/intel_vbt_decode: Optionally determine panel type from EDID

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Mar 25, 2024 at 05:08:57PM +0200, Jani Nikula wrote:
> On Fri, 22 Mar 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > VBT may declare panel type as 255, which means we should match the
> > EDID PnP ID against the panel entries in the VBT to determine the
> > correct panel type. Implemnt support for this by allowing the user
> > to optionally provide the EDID via a command like parameter.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  tools/intel_vbt_decode.c | 96 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 96 insertions(+)
> >
> > diff --git a/tools/intel_vbt_decode.c b/tools/intel_vbt_decode.c
> > index f223ce2bf5a1..9aeb8ea04d06 100644
> > --- a/tools/intel_vbt_decode.c
> > +++ b/tools/intel_vbt_decode.c
> > @@ -84,6 +84,12 @@ struct context {
> >  	bool hexdump;
> >  };
> >  
> > +struct edid {
> > +	uint8_t header[8];
> > +	struct lvds_pnp_id pnpid;
> > +	/* ... */
> > +} __packed;
> > +
> >  static bool dump_panel(const struct context *context, int panel_type)
> >  {
> >  	return panel_type == context->panel_type ||
> > @@ -2506,6 +2512,70 @@ static void dump_compression_parameters(struct context *context,
> >  	}
> >  }
> >  
> > +static int get_panel_type_pnpid(const struct context *context,
> > +				const char *edid_file)
> > +{
> > +	struct bdb_block *ptrs_block, *data_block;
> > +	const struct bdb_lvds_lfp_data *data;
> > +	const struct bdb_lvds_lfp_data_ptrs *ptrs;
> > +	struct lvds_pnp_id edid_id, edid_id_nodate;
> > +	const struct edid *edid;
> > +	int fd, best = -1;
> > +
> > +	fd =  open(edid_file, O_RDONLY);
>             ^
> 
> superfluous space.

ack

> 
> > +	if (fd < 0) {
> > +		fprintf(stderr, "Unable to open EDID file %s\n", edid_file);
> > +		return -1;
> > +	}
> > +
> > +	edid = mmap(0, sizeof(*edid), PROT_READ, MAP_SHARED, fd, 0);
> > +	close(fd);
> > +	if (edid == MAP_FAILED) {
> > +		fprintf(stderr, "Unable to read EDID file %s\n", edid_file);
> > +		return -1;
> > +	}
> > +	edid_id = edid->pnpid;
> > +	munmap((void*)edid, sizeof(*edid));
> > +
> > +	edid_id_nodate = edid_id;
> > +	edid_id_nodate.mfg_week = 0;
> > +	edid_id_nodate.mfg_year = 0;
> > +
> > +	ptrs_block = find_section(context, BDB_LVDS_LFP_DATA_PTRS);
> > +	if (!ptrs_block)
> > +		return -1;
> > +
> > +	data_block = find_section(context, BDB_LVDS_LFP_DATA);
> > +	if (!data_block)
> > +		return -1;
> > +
> > +	ptrs = block_data(ptrs_block);
> > +	data = block_data(data_block);
> > +
> > +	for (int i = 0; i < 16; i++) {
> > +		const struct lvds_pnp_id *vbt_id =
> > +			(const void*)data + ptrs->ptr[i].panel_pnp_id.offset;
> > +
> > +		/* full match? */
> > +		if (!memcmp(vbt_id, &edid_id, sizeof(*vbt_id)))
> > +			return i;
> > +
> > +		/*
> > +		 * Accept a match w/o date if no full match is found,
> > +		 * and the VBT entry does not specify a date.
> > +		 */
> > +		if (best < 0 &&
> > +		    !memcmp(vbt_id, &edid_id_nodate, sizeof(*vbt_id)))
> > +			best = i;
> > +	}
> > +
> > +	if (best >= 0) {
> > +		// ... dump
> > +	}
> 
> Maybe drop this?

Yeah, I had some stuff to dump the pnpids here while
debugging, and looks like this piece ended up in the
wrong patch.

> 
> Other than that,
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>

Thanks.

> 
> 
> > +
> > +	return best;
> > +}
> > +
> >  /* get panel type from lvds options block, or -1 if block not found */
> >  static int get_panel_type(struct context *context, bool is_panel_type2)
> >  {
> > @@ -2784,6 +2854,8 @@ enum opt {
> >  	OPT_DEVID,
> >  	OPT_PANEL_TYPE,
> >  	OPT_PANEL_TYPE2,
> > +	OPT_PANEL_EDID,
> > +	OPT_PANEL_EDID2,
> >  	OPT_ALL_PANELS,
> >  	OPT_HEXDUMP,
> >  	OPT_BLOCK,
> > @@ -2798,6 +2870,9 @@ static void usage(const char *toolname)
> >  	fprintf(stderr, " --file=<rom_file>"
> >  			" [--devid=<device_id>]"
> >  			" [--panel-type=<panel_type>]"
> > +			" [--panel-type2=<panel_type>]"
> > +			" [--panel-edid=<edid file>]"
> > +			" [--panel-edid2=<edid file>]"
> >  			" [--all-panels]"
> >  			" [--hexdump]"
> >  			" [--block=<block_no>]"
> > @@ -2822,6 +2897,7 @@ int main(int argc, char **argv)
> >  		.panel_type = -1,
> >  		.panel_type2 = -1,
> >  	};
> > +	const char *panel_edid = NULL, *panel_edid2 = NULL;
> >  	char *endp;
> >  	int block_number = -1;
> >  	bool header_only = false, describe = false;
> > @@ -2830,7 +2906,9 @@ int main(int argc, char **argv)
> >  		{ "file",	required_argument,	NULL,	OPT_FILE },
> >  		{ "devid",	required_argument,	NULL,	OPT_DEVID },
> >  		{ "panel-type",	required_argument,	NULL,	OPT_PANEL_TYPE },
> > +		{ "panel-edid",	required_argument,	NULL,	OPT_PANEL_EDID },
> >  		{ "panel-type2",	required_argument,	NULL,	OPT_PANEL_TYPE2 },
> > +		{ "panel-edid2",	required_argument,	NULL,	OPT_PANEL_EDID2 },
> >  		{ "all-panels",	no_argument,		NULL,	OPT_ALL_PANELS },
> >  		{ "hexdump",	no_argument,		NULL,	OPT_HEXDUMP },
> >  		{ "block",	required_argument,	NULL,	OPT_BLOCK },
> > @@ -2870,6 +2948,12 @@ int main(int argc, char **argv)
> >  				return EXIT_FAILURE;
> >  			}
> >  			break;
> > +		case OPT_PANEL_EDID:
> > +			panel_edid = optarg;
> > +			break;
> > +		case OPT_PANEL_EDID2:
> > +			panel_edid2 = optarg;
> > +			break;
> >  		case OPT_ALL_PANELS:
> >  			context.dump_all_panel_types = true;
> >  			break;
> > @@ -2988,6 +3072,12 @@ int main(int argc, char **argv)
> >  
> >  	if (context.panel_type == -1)
> >  		context.panel_type = get_panel_type(&context, false);
> > +	if (context.panel_type == 255 && !panel_edid) {
> > +		fprintf(stderr, "Warning: panel type depends on EDID (use --panel-edid), ignoring\n");
> > +		context.panel_type = -1;
> > +	} else if (context.panel_type == 255) {
> > +		context.panel_type = get_panel_type_pnpid(&context, panel_edid);
> > +	}
> >  	if (context.panel_type == -1) {
> >  		fprintf(stderr, "Warning: panel type not set, using 0\n");
> >  		context.panel_type = 0;
> > @@ -2995,6 +3085,12 @@ int main(int argc, char **argv)
> >  
> >  	if (context.panel_type2 == -1)
> >  		context.panel_type2 = get_panel_type(&context, true);
> > +	if (context.panel_type2 == 255 && !panel_edid2) {
> > +		fprintf(stderr, "Warning: panel type2 depends on EDID (use --panel-edid2), ignoring\n");
> > +		context.panel_type2 = -1;
> > +	} else if (context.panel_type2 == 255) {
> > +		context.panel_type2 = get_panel_type_pnpid(&context, panel_edid2);
> > +	}
> >  	if (context.panel_type2 != -1 && context.bdb->version < 212) {
> >  		fprintf(stderr, "Warning: panel type2 not valid for BDB version %d\n",
> >  			context.bdb->version);
> 
> -- 
> Jani Nikula, Intel

-- 
Ville Syrjälä
Intel