Re: [igt-dev] [PATCH i-g-t 2/2] tooks/vbt_decode: Print PSR2 training pattern durations.

[Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>]

On Fri, 2019-07-19 at 19:15 +0300, Ville Syrjälä wrote:
> On Fri, Jul 19, 2019 at 08:12:07AM -0700, Dhinakaran Pandiyan wrote:
> > On Thu, 2019-07-18 at 14:35 +0300, Ville Syrjälä wrote:
> > > On Wed, Jul 17, 2019 at 05:43:31PM -0700, Dhinakaran Pandiyan wrote:
> > > > There is a new field for PSR2 training pattern duration in VBT versions
> > > > > = 226, decode that.
> > > > 
> > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > ---
> > > >  tools/intel_vbt_decode.c | 11 +++++++++++
> > > >  1 file changed, 11 insertions(+)
> > > > 
> > > > diff --git a/tools/intel_vbt_decode.c b/tools/intel_vbt_decode.c
> > > > index 38eccc48..4004762c 100644
> > > > --- a/tools/intel_vbt_decode.c
> > > > +++ b/tools/intel_vbt_decode.c
> > > > @@ -944,11 +944,13 @@ static void dump_psr(struct context *context,
> > > >  {
> > > >  	const struct bdb_psr *psr_block = block->data;
> > > >  	int i;
> > > > +	int psr2_tp_time;
> > > 
> > > uint32_t
> > > 
> > > >  
> > > >  	/* The same block ID was used for something else before? */
> > > >  	if (context->bdb->version < 165)
> > > >  		return;
> > > >  
> > > > +	psr2_tp_time = psr_block->psr2_tp2_tp3_wakeup_time;
> > > >  	for (i = 0; i < 16; i++) {
> > > >  		const struct psr_table *psr = &psr_block->psr_table[i];
> > > >  
> > > > @@ -987,6 +989,15 @@ static void dump_psr(struct context *context,
> > > >  		printf("\t\tTP2/TP3 wakeup time: %d usec (0x%x)\n",
> > > >  		       psr->tp2_tp3_wakeup_time * 100,
> > > >  		       psr->tp2_tp3_wakeup_time);
> > > > +
> > > > +		if (context->bdb->version >= 226) {
> > > > +			int index;
> > > > +			static const int psr2_tp_times[] = {500, 100, 2500, 5};
> > > 
> > > Everthing fits into uint16_t.
> > 
> > Do we want a fixed-width type for values that are not read from/written to HW?
> > I changed it in the second version anyway. 
> > > 
> > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Oh, and the psr1 parsing doesn't seem to be following this silly
> > > new style of putting the hw register value directly into the vbt.
> > > Would be nice to fix that too. Actually, since the spec is a mess
> > > and it's not 100% clear which VBTs might follow which style maybe
> > > we should in fact decode both ways?
> > 
> > I thought of fixing it, but left it alone because it is a mess as you said.
> > Are suggesting something like this - "TP2/TP3 wakeup time: 200 usec/2500 usec (0x02)"?
> 
> Yeah something like that, but with maybe a bit more verbosity to explain
> the two values.
> 
> > I feel the ambiguity makes decoding useless as it requires someone to go read
> > the driver code to know what will get programmed. Might as well avoid decoding and
> > just print the raw value. The other option is to replicate the platform checks used in
> > the driver.
> 
> I don't think we have platform infomation beyond what the
> signature + version gives us. And IIRC all SKL derivatives
> use the  "$VBT SKYLAKE" signature so we can't actually tell
> those apart. So I guess we'd just have to decode based on
> the version number for all those.
> 
> But maybe we can tell BXT apart from the rest based on the
> signature?
> 
> So we could do:
> if !BXT && v >= 205
> 	new deocde
> else
> 	old decode

I like this better. Got a BXT to see what the signature actually looks like, "$VBT BROXTON" it is.

-DK
> 

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev