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

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

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

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev