Re: [PATCH i-g-t 4/5] tools/intel_vbt_decode: Also dump the second panel (panel_type2)

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

On Mon, Mar 25, 2024 at 05:04:40PM +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>
> >
> > Modern VBTs can declare two panel types in order to support
> > dual panel systems. Dump the panel information for the second
> > panel as well. Since panel_type2 could also be declared as 255
> > (== match pnpid to EDID) we also add a new command line knob
> > to select panel_type2 by hand.
> >
> > Data for the second panel will be indicated by "(2)", as opposed
> > to "(1)" for the first panel.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  tools/intel_vbt_decode.c | 38 ++++++++++++++++++++++++++++++++------
> >  1 file changed, 32 insertions(+), 6 deletions(-)
> >
> > diff --git a/tools/intel_vbt_decode.c b/tools/intel_vbt_decode.c
> > index 98f64d0822c6..f223ce2bf5a1 100644
> > --- a/tools/intel_vbt_decode.c
> > +++ b/tools/intel_vbt_decode.c
> > @@ -79,7 +79,7 @@ struct context {
> >  	int size;
> >  
> >  	uint32_t devid;
> > -	int panel_type;
> > +	int panel_type, panel_type2;
> >  	bool dump_all_panel_types;
> >  	bool hexdump;
> >  };
> > @@ -87,12 +87,16 @@ struct context {
> >  static bool dump_panel(const struct context *context, int panel_type)
> >  {
> >  	return panel_type == context->panel_type ||
> > +		panel_type == context->panel_type2 ||
> >  		context->dump_all_panel_types;
> >  }
> >  
> >  static const char *panel_str(const struct context *context, int panel_type)
> >  {
> > -	return panel_type == context->panel_type ? " (1)" : "";
> > +	return panel_type == context->panel_type &&
> > +		panel_type == context->panel_type2 ? " (1)(2)" :
> > +		panel_type == context->panel_type ? " (1)" :
> > +		panel_type == context->panel_type2 ? " (2)" : "";
> 
> This might be better broken down to regular if statements for clarity.

Sure.

> 
> Please clue me in, when is it possible for both panel types to match?

Either the VBT is junk, or we can have cases like
panel_type==255 + panel_type2==0 where we can't figure
out what the 255 actually means and just fall back
using panel_type==0.

> Other than that,
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> 
> >  }
> >  
> >  /* Get BDB block size given a pointer to Block ID. */
> > @@ -2503,18 +2507,21 @@ static void dump_compression_parameters(struct context *context,
> >  }
> >  
> >  /* get panel type from lvds options block, or -1 if block not found */
> > -static int get_panel_type(struct context *context)
> > +static int get_panel_type(struct context *context, bool is_panel_type2)
> >  {
> >  	struct bdb_block *block;
> >  	const struct bdb_lvds_options *options;
> > -	int panel_type;
> > +	int panel_type = -1;
> >  
> >  	block = find_section(context, BDB_LVDS_OPTIONS);
> >  	if (!block)
> >  		return -1;
> >  
> >  	options = block_data(block);
> > -	panel_type = options->panel_type;
> > +	if (!is_panel_type2)
> > +		panel_type = options->panel_type;
> > +	else if (context->bdb->version >= 212)
> > +		panel_type = options->panel_type2;
> >  
> >  	free(block);
> >  
> > @@ -2776,6 +2783,7 @@ enum opt {
> >  	OPT_FILE,
> >  	OPT_DEVID,
> >  	OPT_PANEL_TYPE,
> > +	OPT_PANEL_TYPE2,
> >  	OPT_ALL_PANELS,
> >  	OPT_HEXDUMP,
> >  	OPT_BLOCK,
> > @@ -2812,6 +2820,7 @@ int main(int argc, char **argv)
> >  	int size;
> >  	struct context context = {
> >  		.panel_type = -1,
> > +		.panel_type2 = -1,
> >  	};
> >  	char *endp;
> >  	int block_number = -1;
> > @@ -2821,6 +2830,7 @@ 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-type2",	required_argument,	NULL,	OPT_PANEL_TYPE2 },
> >  		{ "all-panels",	no_argument,		NULL,	OPT_ALL_PANELS },
> >  		{ "hexdump",	no_argument,		NULL,	OPT_HEXDUMP },
> >  		{ "block",	required_argument,	NULL,	OPT_BLOCK },
> > @@ -2852,6 +2862,14 @@ int main(int argc, char **argv)
> >  				return EXIT_FAILURE;
> >  			}
> >  			break;
> > +		case OPT_PANEL_TYPE2:
> > +			context.panel_type2 = strtoul(optarg, &endp, 0);
> > +			if (*endp || context.panel_type2 > 15) {
> > +				fprintf(stderr, "invalid panel type2 '%s'\n",
> > +					optarg);
> > +				return EXIT_FAILURE;
> > +			}
> > +			break;
> >  		case OPT_ALL_PANELS:
> >  			context.dump_all_panel_types = true;
> >  			break;
> > @@ -2969,12 +2987,20 @@ int main(int argc, char **argv)
> >  		fprintf(stderr, "Warning: could not find PCI device ID!\n");
> >  
> >  	if (context.panel_type == -1)
> > -		context.panel_type = get_panel_type(&context);
> > +		context.panel_type = get_panel_type(&context, false);
> >  	if (context.panel_type == -1) {
> >  		fprintf(stderr, "Warning: panel type not set, using 0\n");
> >  		context.panel_type = 0;
> >  	}
> >  
> > +	if (context.panel_type2 == -1)
> > +		context.panel_type2 = get_panel_type(&context, true);
> > +	if (context.panel_type2 != -1 && context.bdb->version < 212) {
> > +		fprintf(stderr, "Warning: panel type2 not valid for BDB version %d\n",
> > +			context.bdb->version);
> > +		context.panel_type2 = -1;
> > +	}
> > +
> >  	if (describe) {
> >  		print_description(&context);
> >  	} else if (header_only) {
> 
> -- 
> Jani Nikula, Intel

-- 
Ville Syrjälä
Intel