Re: [PATCH v6 3/4] drm/i915/psr: Make PSR registers relative to transcoders

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

On Mon, 2019-06-24 at 14:11 -0700, Souza, Jose wrote:
> > > > +#define _HSW_EDP_PSR_BASE                        0x64800
> > > > +#define _SRD_CTL_A                               0x60800
> > > > +#define _SRD_CTL_EDP                             0x6f800
> > > > +#define _HSW_PSR_ADJ(reg)                        ((reg) -
> > > > _SRD_CTL_A +
> > > > _HSW_EDP_PSR_BASE)
> > > > +#define _PSR_ADJ(tran, reg)                      (IS_HASWELL(dev
> > > > _priv) ? _HSW_PSR_ADJ(reg) : _TRANS2(tran, reg))
> > > > +#define EDP_PSR_CTL(tran)                        _MMIO(_PSR_ADJ(
> > > > tran,
> > > > _SRD_CTL_A))
> > > 
> > > There are currently three instances of platform/gen checks in
> > > i915_reg.h. They are the exception, and they're in individual
> > > macros
> > > that aren't even register offset definitions let alone helpers that
> > > get
> > > proliferated to several other macros.
> > > 
> > > This change here is quite a big precedent in that regard, and not
> > > to
> > > be
> > > done lightly. Usually the case is others will follow suit, so this
> > > is
> > > not just about this one instance. It's about deciding whether this
> > > is
> > > the direction we want to take. How far are we prepared to go and
> > > how
> > > do
> > > we stop there?
> > > 
> > > There's really no way to set the psr->transcoder such on HSW that
> > > it
> > > would work with MMIO_TRANS2()?
> > 
> > I'm going to think about but right now the only other option that
> > comes
> > in my mind is to have the transcoder offset as macro parameter:
> > 
> > #define _SRD_CTL 0x800
> > #define EDP_PSR_CTL(trans) _MMIO(trans + _SRD_CTL)
> > 
> > But we would lose the full offset address of PSR registers.
> 
> This is the only other good option that I can think about.
> 
> Any other idea DK?
No good ones unfortunately. This is the simplest one I could think of

intel_psr_init()
{
...
if(IS_HASWELL(dev_priv))
	dev_priv->psr.hsw_adjust = _SRD_CTL_EDP - _HSW_EDP_PSR_BASE; 
...
}


#define _MMIO_PSR(tran, reg) _MMIO_TRANS2(tran, reg - dev_priv.psr.hsw_adjust)
#define EDP_PSR_CTL(tran) _MMIO_PSR(tran, _SRD_CTL_A)


should work because tran == TRANSCODER_EDP for HSW


-DK

BR,
Jani.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx