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

[Souza, Jose]

On Mon, 2019-07-01 at 22:26 +0000, Souza, Jose wrote:
> On Fri, 2019-06-28 at 19:25 -0700, Dhinakaran Pandiyan wrote:
> > 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
> 
> The problem with this suggestion is that it would require more
> changes
> to support multiple PSR instances in future, unless we make it
> required
> to have struct intel_psr *psr defined like struct drm_i915_private
> *dev_priv is required by I915_WRITE/READ().
> 
> 
> #define _MMIO_PSR(tran, reg) _MMIO_TRANS2(tran, reg - psr-
> >hsw_adjust)
> 
> If chosen this approach we could already complete remove the tran
> parameter:
> 
> #define _MMIO_PSR(reg) _MMIO_TRANS2(psr->transcoder, reg - psr-
> > hsw_adjust)

Other option here:

#define _MMIO_PSR(tran, reg) _MMIO_TRANS2(tran, reg -
dev_priv.psr_hsw_adjust)

As HSW can only have one PSR instance.

> 
> #
> 
> So we have 4 options:
> 
> 1 - The one implemented in this patch
> 
> 2 - Offset as parameter
> 	#define _SRD_CTL 0x800
> 	#define EDP_PSR_CTL(trans_offset) _MMIO(trans_offset +
> _SRD_CTL)
> 
> 3 - DKs suggestion with one of the suggestions above to support
> multiple PSR instances




> 
> 4 - Have HSW PSR specific macros and have several if (IS_HASWELL())
> spread to PSR code
> 
> 
> My vote is option 1.
> 
> Please let me know your thoughts?
> 	
> 
> > 
> > -DK
> > 
> > BR,
> > Jani.
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx