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

From: "Souza, Jose" <jose.souza@intel.com>
To: "intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"jani.nikula@linux.intel.com" <jani.nikula@linux.intel.com>
Cc: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
Subject: Re: [PATCH v6 3/4] drm/i915/psr: Make PSR registers relative to transcoders
Date: Mon, 24 Jun 2019 21:11:08 +0000	[thread overview]
Message-ID: <5263d1cfbc1fe95596dfa47b2e794e298fd670fa.camel@intel.com> (raw)
In-Reply-To: <a0e7424a0c7e856c79fc3a05959f475b9fe67f8b.camel@intel.com>

On Thu, 2019-06-20 at 17:36 +0000, Souza, Jose wrote:
> On Thu, 2019-06-20 at 11:13 +0300, Jani Nikula wrote:
> > On Wed, 19 Jun 2019, José Roberto de Souza <jose.souza@intel.com>
> > wrote:
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index 4fc8dc083766..31fb4de5081c 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -4225,10 +4225,18 @@ enum {
> > >  #define PIPESRC(trans)		_MMIO_TRANS2(trans, _PIPEASRC)
> > >  #define PIPE_MULT(trans)	_MMIO_TRANS2(trans, _PIPE_MULT_A)
> > >  
> > > -/* HSW+ eDP PSR registers */
> > > -#define HSW_EDP_PSR_BASE	0x64800
> > > -#define BDW_EDP_PSR_BASE	0x6f800
> > > -#define EDP_PSR_CTL				_MMIO(dev_priv-
> > > > psr_mmio_base + 0)
> > > +/*
> > > + * HSW+ eDP PSR registers
> > > + *
> > > + * HSW PSR registers are relative to DDIA(_DDI_BUF_CTL_A +
> > > 0x800)
> > > with just one
> > > + * instance of it
> > > + */
> > > +#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?

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