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: Thu, 20 Jun 2019 17:36:55 +0000	[thread overview]
Message-ID: <a0e7424a0c7e856c79fc3a05959f475b9fe67f8b.camel@intel.com> (raw)
In-Reply-To: <878stwu12j.fsf@intel.com>

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.

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