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

["Souza, Jose" <jose.souza@intel.com>]

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)

#

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx