diff mbox series

[3/8] drm/i915/psr: Make all PSR register relative to mmio base

Message ID 20190321180143.22519-3-jose.souza@intel.com (mailing list archive)
State New, archived
Headers show
Series [1/8] drm/i915/psr: Remove partial PSR support on multiple transcoders | expand

Commit Message

Souza, Jose March 21, 2019, 6:01 p.m. UTC 
Right now it have a mix of PSR registers that are relative to PSR
mmio base and other register with a hardcoded address, lets keep it
consistented and have it all relative to mmio base.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

Comments

Jani Nikula March 22, 2019, 9:15 a.m. UTC | #1 
On Thu, 21 Mar 2019, José Roberto de Souza <jose.souza@intel.com> wrote:
> Right now it have a mix of PSR registers that are relative to PSR
> mmio base and other register with a hardcoded address, lets keep it
> consistented and have it all relative to mmio base.

This is not strictly limited to this patch, but an overall trend. The
thing that really bugs me with this is losing more of the actual
absolute mmio addresses from the file. When you're seeking to add a new
register, you can't trivially grep for it in the file anymore. Not all
of our register names match the spec (and the spec occasionally also
changes register names) so being able to find the offset is important.

When we added the macros that use ->pipe_offsets and ->trans_offsets, we
took care to have at least one of the offsets in the file. I'm wondering
if we could do something like that here as well.

BR,
Jani.


>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 28728399e607..e1ed2ba1c315 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4326,7 +4326,7 @@ enum {
>  #define   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE    (1 << 16) /* Reserved in ICL+ */
>  #define   EDP_PSR_DEBUG_EXIT_ON_PIXEL_UNDERRUN (1 << 15) /* SKL+ */
>  
> -#define EDP_PSR2_CTL			_MMIO(0x6f900)
> +#define EDP_PSR2_CTL			_MMIO(dev_priv->psr.mmio_base + 0x100)
>  #define   EDP_PSR2_ENABLE		(1 << 31)
>  #define   EDP_SU_TRACK_ENABLE		(1 << 30)
>  #define   EDP_Y_COORDINATE_VALID	(1 << 26) /* GLK and CNL+ */
> @@ -4344,7 +4344,7 @@ enum {
>  #define   EDP_PSR2_IDLE_FRAME_MASK	0xf
>  #define   EDP_PSR2_IDLE_FRAME_SHIFT	0
>  
> -#define PSR_EVENT				_MMIO(0x6F848)
> +#define PSR_EVENT				_MMIO(dev_priv->psr.mmio_base + 0x48)
>  #define  PSR_EVENT_PSR2_WD_TIMER_EXPIRE		(1 << 17)
>  #define  PSR_EVENT_PSR2_DISABLED		(1 << 16)
>  #define  PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN	(1 << 15)
> @@ -4362,14 +4362,11 @@ enum {
>  #define  PSR_EVENT_LPSP_MODE_EXIT		(1 << 1)
>  #define  PSR_EVENT_PSR_DISABLE			(1 << 0)
>  
> -#define EDP_PSR2_STATUS			_MMIO(0x6f940)
> +#define EDP_PSR2_STATUS			_MMIO(dev_priv->psr.mmio_base + 0x140)
>  #define EDP_PSR2_STATUS_STATE_MASK     (0xf << 28)
>  #define EDP_PSR2_STATUS_STATE_SHIFT    28
>  
> -#define _PSR2_SU_STATUS_0		0x6F914
> -#define _PSR2_SU_STATUS_1		0x6F918
> -#define _PSR2_SU_STATUS_2		0x6F91C
> -#define _PSR2_SU_STATUS(index)		_MMIO(_PICK_EVEN((index), _PSR2_SU_STATUS_0, _PSR2_SU_STATUS_1))
> +#define _PSR2_SU_STATUS(index)		_MMIO(dev_priv->psr.mmio_base + 0x114 + (index) * 4)
>  #define PSR2_SU_STATUS(frame)		(_PSR2_SU_STATUS((frame) / 3))
>  #define PSR2_SU_STATUS_SHIFT(frame)	(((frame) % 3) * 10)
>  #define PSR2_SU_STATUS_MASK(frame)	(0x3ff << PSR2_SU_STATUS_SHIFT(frame))
Dhinakaran Pandiyan March 22, 2019, 5:27 p.m. UTC | #2 
On Fri, 2019-03-22 at 11:15 +0200, Jani Nikula wrote:
> On Thu, 21 Mar 2019, José Roberto de Souza <jose.souza@intel.com> wrote:
> > Right now it have a mix of PSR registers that are relative to PSR
> > mmio base and other register with a hardcoded address, lets keep it
> > consistented and have it all relative to mmio base.
> 
> This is not strictly limited to this patch, but an overall trend. The
> thing that really bugs me with this is losing more of the actual
> absolute mmio addresses from the file. When you're seeking to add a new
> register, you can't trivially grep for it in the file anymore. Not all
> of our register names match the spec (and the spec occasionally also
> changes register names) so being able to find the offset is important.

Fully agreed.

I think we can do something along the lines of  

#define _HSW_PSR_OFFSET BDW_EDP_PSR_BASE - HSW_PSR_PSR_BASE
#define _BDW_PSR_CTL 0x6f800

_MMIO_HSW_ADJUST(pipe, reg)  IS_HASWELL(dev_priv) ?
				MMIO_TRANS2((pipe), reg - _HSW_PSR_OFFSET) : 					MMIO_TRANS2((pipe), reg)

#define EDP_PSR_CTL(pipe) _MMIO_HSW_ADJUST((pipe), _BDW_PSR_CTL)


I'd like at least BDW+ addresses to be in the code.

-DK

> 
> When we added the macros that use ->pipe_offsets and ->trans_offsets, we
> took care to have at least one of the offsets in the file. I'm wondering
> if we could do something like that here as well.
> 
> BR,
> Jani.
> 
> 
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h | 11 ++++-------
> >  1 file changed, 4 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 28728399e607..e1ed2ba1c315 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4326,7 +4326,7 @@ enum {
> >  #define   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE    (1 << 16) /* Reserved in
> > ICL+ */
> >  #define   EDP_PSR_DEBUG_EXIT_ON_PIXEL_UNDERRUN (1 << 15) /* SKL+ */
> >  
> > -#define EDP_PSR2_CTL			_MMIO(0x6f900)
> > +#define EDP_PSR2_CTL			_MMIO(dev_priv->psr.mmio_base +
> > 0x100)
> >  #define   EDP_PSR2_ENABLE		(1 << 31)
> >  #define   EDP_SU_TRACK_ENABLE		(1 << 30)
> >  #define   EDP_Y_COORDINATE_VALID	(1 << 26) /* GLK and CNL+ */
> > @@ -4344,7 +4344,7 @@ enum {
> >  #define   EDP_PSR2_IDLE_FRAME_MASK	0xf
> >  #define   EDP_PSR2_IDLE_FRAME_SHIFT	0
> >  
> > -#define PSR_EVENT				_MMIO(0x6F848)
> > +#define PSR_EVENT				_MMIO(dev_priv->psr.mmio_base +
> > 0x48)
> >  #define  PSR_EVENT_PSR2_WD_TIMER_EXPIRE		(1 << 17)
> >  #define  PSR_EVENT_PSR2_DISABLED		(1 << 16)
> >  #define  PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN	(1 << 15)
> > @@ -4362,14 +4362,11 @@ enum {
> >  #define  PSR_EVENT_LPSP_MODE_EXIT		(1 << 1)
> >  #define  PSR_EVENT_PSR_DISABLE			(1 << 0)
> >  
> > -#define EDP_PSR2_STATUS			_MMIO(0x6f940)
> > +#define EDP_PSR2_STATUS			_MMIO(dev_priv->psr.mmio_base +
> > 0x140)
> >  #define EDP_PSR2_STATUS_STATE_MASK     (0xf << 28)
> >  #define EDP_PSR2_STATUS_STATE_SHIFT    28
> >  
> > -#define _PSR2_SU_STATUS_0		0x6F914
> > -#define _PSR2_SU_STATUS_1		0x6F918
> > -#define _PSR2_SU_STATUS_2		0x6F91C
> > -#define _PSR2_SU_STATUS(index)		_MMIO(_PICK_EVEN((index),
> > _PSR2_SU_STATUS_0, _PSR2_SU_STATUS_1))
> > +#define _PSR2_SU_STATUS(index)		_MMIO(dev_priv->psr.mmio_base +
> > 0x114 + (index) * 4)
> >  #define PSR2_SU_STATUS(frame)		(_PSR2_SU_STATUS((frame) / 3))
> >  #define PSR2_SU_STATUS_SHIFT(frame)	(((frame) % 3) * 10)
> >  #define PSR2_SU_STATUS_MASK(frame)	(0x3ff << PSR2_SU_STATUS_SHIFT(frame))
> 
>
Souza, Jose March 26, 2019, 8:44 p.m. UTC | #3 
On Fri, 2019-03-22 at 10:27 -0700, Dhinakaran Pandiyan wrote:
> On Fri, 2019-03-22 at 11:15 +0200, Jani Nikula wrote:
> > On Thu, 21 Mar 2019, José Roberto de Souza <jose.souza@intel.com>
> > wrote:
> > > Right now it have a mix of PSR registers that are relative to PSR
> > > mmio base and other register with a hardcoded address, lets keep
> > > it
> > > consistented and have it all relative to mmio base.
> > 
> > This is not strictly limited to this patch, but an overall trend.
> > The
> > thing that really bugs me with this is losing more of the actual
> > absolute mmio addresses from the file. When you're seeking to add a
> > new
> > register, you can't trivially grep for it in the file anymore. Not
> > all
> > of our register names match the spec (and the spec occasionally
> > also
> > changes register names) so being able to find the offset is
> > important.

I understand but for new gens BSpec is using relative address see BSpec
50583 and 50577 for example.

> 
> Fully agreed.
> 
> I think we can do something along the lines of  
> 
> #define _HSW_PSR_OFFSET BDW_EDP_PSR_BASE - HSW_PSR_PSR_BASE
> #define _BDW_PSR_CTL 0x6f800
> 
> _MMIO_HSW_ADJUST(pipe, reg)  IS_HASWELL(dev_priv) ?
> 				MMIO_TRANS2((pipe), reg -
> _HSW_PSR_OFFSET) : 					MMIO_TRANS2((pi
> pe), reg)

To MMIO_TRANS2() work we need to give the reg based on the first
transcoder, what you think about this?


#define _HSW_EDP_PSR_BASE	0x64000

/* _PSR_CTL_A to follow BSpec naming or we could keep _PSR_CTL_A */
#define _SRD_CTL_A		0x60800
#define _SRD_CTL_EDP		0x6F800
#define EDP_PSR_CTL				(_MMIO_TRANS2(dev_priv-
>psr.transcoder, _SRD_CTL_A) - dev_priv->psr.mmio_base_adjust)

intel_psr_enable_locked()
	if (IS_HASWELL(dev_priv))
		dev_priv->psr.mmio_base_adjust = _SRD_CTL_EDP
- _HSW_EDP_PSR_BASE;

The only concern here it that _SRD_CTL_A(and the other registers
conterparts) could give the understand that PSR could be enabled in
regular transcoders what it not the case in current gens.


> 
> #define EDP_PSR_CTL(pipe) _MMIO_HSW_ADJUST((pipe), _BDW_PSR_CTL)
> 
> 
> I'd like at least BDW+ addresses to be in the code.
> 
> -DK
> 
> > When we added the macros that use ->pipe_offsets and
> > ->trans_offsets, we
> > took care to have at least one of the offsets in the file. I'm
> > wondering
> > if we could do something like that here as well.
> > 
> > BR,
> > Jani.
> > 
> > 
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h | 11 ++++-------
> > >  1 file changed, 4 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index 28728399e607..e1ed2ba1c315 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -4326,7 +4326,7 @@ enum {
> > >  #define   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE    (1 << 16) /*
> > > Reserved in
> > > ICL+ */
> > >  #define   EDP_PSR_DEBUG_EXIT_ON_PIXEL_UNDERRUN (1 << 15) /* SKL+
> > > */
> > >  
> > > -#define EDP_PSR2_CTL			_MMIO(0x6f900)
> > > +#define EDP_PSR2_CTL			_MMIO(dev_priv-
> > > >psr.mmio_base +
> > > 0x100)
> > >  #define   EDP_PSR2_ENABLE		(1 << 31)
> > >  #define   EDP_SU_TRACK_ENABLE		(1 << 30)
> > >  #define   EDP_Y_COORDINATE_VALID	(1 << 26) /* GLK and CNL+ */
> > > @@ -4344,7 +4344,7 @@ enum {
> > >  #define   EDP_PSR2_IDLE_FRAME_MASK	0xf
> > >  #define   EDP_PSR2_IDLE_FRAME_SHIFT	0
> > >  
> > > -#define PSR_EVENT				_MMIO(0x6F848)
> > > +#define PSR_EVENT				_MMIO(dev_priv-
> > > >psr.mmio_base +
> > > 0x48)
> > >  #define  PSR_EVENT_PSR2_WD_TIMER_EXPIRE		(1 << 17)
> > >  #define  PSR_EVENT_PSR2_DISABLED		(1 << 16)
> > >  #define  PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN	(1 << 15)
> > > @@ -4362,14 +4362,11 @@ enum {
> > >  #define  PSR_EVENT_LPSP_MODE_EXIT		(1 << 1)
> > >  #define  PSR_EVENT_PSR_DISABLE			(1 << 0)
> > >  
> > > -#define EDP_PSR2_STATUS			_MMIO(0x6f940)
> > > +#define EDP_PSR2_STATUS			_MMIO(dev_priv-
> > > >psr.mmio_base +
> > > 0x140)
> > >  #define EDP_PSR2_STATUS_STATE_MASK     (0xf << 28)
> > >  #define EDP_PSR2_STATUS_STATE_SHIFT    28
> > >  
> > > -#define _PSR2_SU_STATUS_0		0x6F914
> > > -#define _PSR2_SU_STATUS_1		0x6F918
> > > -#define _PSR2_SU_STATUS_2		0x6F91C
> > > -#define _PSR2_SU_STATUS(index)		_MMIO(_PICK_EVEN((index
> > > ),
> > > _PSR2_SU_STATUS_0, _PSR2_SU_STATUS_1))
> > > +#define _PSR2_SU_STATUS(index)		_MMIO(dev_priv-
> > > >psr.mmio_base +
> > > 0x114 + (index) * 4)
> > >  #define PSR2_SU_STATUS(frame)		(_PSR2_SU_STATUS((frame
> > > ) / 3))
> > >  #define PSR2_SU_STATUS_SHIFT(frame)	(((frame) % 3) * 10)
> > >  #define PSR2_SU_STATUS_MASK(frame)	(0x3ff <<
> > > PSR2_SU_STATUS_SHIFT(frame))
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 28728399e607..e1ed2ba1c315 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4326,7 +4326,7 @@  enum {
 #define   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE    (1 << 16) /* Reserved in ICL+ */
 #define   EDP_PSR_DEBUG_EXIT_ON_PIXEL_UNDERRUN (1 << 15) /* SKL+ */
 
-#define EDP_PSR2_CTL			_MMIO(0x6f900)
+#define EDP_PSR2_CTL			_MMIO(dev_priv->psr.mmio_base + 0x100)
 #define   EDP_PSR2_ENABLE		(1 << 31)
 #define   EDP_SU_TRACK_ENABLE		(1 << 30)
 #define   EDP_Y_COORDINATE_VALID	(1 << 26) /* GLK and CNL+ */
@@ -4344,7 +4344,7 @@  enum {
 #define   EDP_PSR2_IDLE_FRAME_MASK	0xf
 #define   EDP_PSR2_IDLE_FRAME_SHIFT	0
 
-#define PSR_EVENT				_MMIO(0x6F848)
+#define PSR_EVENT				_MMIO(dev_priv->psr.mmio_base + 0x48)
 #define  PSR_EVENT_PSR2_WD_TIMER_EXPIRE		(1 << 17)
 #define  PSR_EVENT_PSR2_DISABLED		(1 << 16)
 #define  PSR_EVENT_SU_DIRTY_FIFO_UNDERRUN	(1 << 15)
@@ -4362,14 +4362,11 @@  enum {
 #define  PSR_EVENT_LPSP_MODE_EXIT		(1 << 1)
 #define  PSR_EVENT_PSR_DISABLE			(1 << 0)
 
-#define EDP_PSR2_STATUS			_MMIO(0x6f940)
+#define EDP_PSR2_STATUS			_MMIO(dev_priv->psr.mmio_base + 0x140)
 #define EDP_PSR2_STATUS_STATE_MASK     (0xf << 28)
 #define EDP_PSR2_STATUS_STATE_SHIFT    28
 
-#define _PSR2_SU_STATUS_0		0x6F914
-#define _PSR2_SU_STATUS_1		0x6F918
-#define _PSR2_SU_STATUS_2		0x6F91C
-#define _PSR2_SU_STATUS(index)		_MMIO(_PICK_EVEN((index), _PSR2_SU_STATUS_0, _PSR2_SU_STATUS_1))
+#define _PSR2_SU_STATUS(index)		_MMIO(dev_priv->psr.mmio_base + 0x114 + (index) * 4)
 #define PSR2_SU_STATUS(frame)		(_PSR2_SU_STATUS((frame) / 3))
 #define PSR2_SU_STATUS_SHIFT(frame)	(((frame) % 3) * 10)
 #define PSR2_SU_STATUS_MASK(frame)	(0x3ff << PSR2_SU_STATUS_SHIFT(frame))