[v16,3/7] drm/i915: Introduce parameterized DBUF_CTL
diff mbox series

Message ID 20200124084456.2961-4-stanislav.lisovskiy@intel.com
State New
Headers show
Series
  • Enable second DBuf slice for ICL and TGL
Related show

Commit Message

Stanislav Lisovskiy Jan. 24, 2020, 8:44 a.m. UTC
Now start using parameterized DBUF_CTL instead
of hardcoded, this would allow shorter access
functions when reading or storing entire state.

Tried to implement it in a MMIO_PIPE manner, however
DBUF_CTL1 address is higher than DBUF_CTL2, which
implies that we have to now subtract from base
rather than add.

v2: - Removed unneeded DBUF_CTL_DIST and DBUF_CTL_ADDR
      macros. Started to use _PICK construct as suggested
      by Matt Roper.

v3: - DBUF_CTL_S* to _DBUF_CTL_S*, changed X to "slice"
      in macro(Ville Syrjälä)
    - Introduced enum for enumerating DBUF slices(Ville Syrjälä)

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 .../drm/i915/display/intel_display_power.c    | 30 +++++++++++--------
 .../drm/i915/display/intel_display_power.h    |  5 ++++
 drivers/gpu/drm/i915/i915_reg.h               |  7 +++--
 drivers/gpu/drm/i915/intel_pm.c               |  2 +-
 4 files changed, 28 insertions(+), 16 deletions(-)

Comments

Ville Syrjälä Jan. 28, 2020, 5:35 p.m. UTC | #1
On Fri, Jan 24, 2020 at 10:44:52AM +0200, Stanislav Lisovskiy wrote:
> Now start using parameterized DBUF_CTL instead
> of hardcoded, this would allow shorter access
> functions when reading or storing entire state.
> 
> Tried to implement it in a MMIO_PIPE manner, however
> DBUF_CTL1 address is higher than DBUF_CTL2, which
> implies that we have to now subtract from base
> rather than add.
> 
> v2: - Removed unneeded DBUF_CTL_DIST and DBUF_CTL_ADDR
>       macros. Started to use _PICK construct as suggested
>       by Matt Roper.
> 
> v3: - DBUF_CTL_S* to _DBUF_CTL_S*, changed X to "slice"
>       in macro(Ville Syrjälä)
>     - Introduced enum for enumerating DBUF slices(Ville Syrjälä)
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  .../drm/i915/display/intel_display_power.c    | 30 +++++++++++--------
>  .../drm/i915/display/intel_display_power.h    |  5 ++++
>  drivers/gpu/drm/i915/i915_reg.h               |  7 +++--
>  drivers/gpu/drm/i915/intel_pm.c               |  2 +-
>  4 files changed, 28 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 5e1c601f0f99..a59efb24be92 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -4418,9 +4418,11 @@ void icl_dbuf_slices_update(struct drm_i915_private *dev_priv,
>  		return;
>  
>  	if (req_slices > hw_enabled_slices)
> -		ret = intel_dbuf_slice_set(dev_priv, DBUF_CTL_S2, true);
> +		ret = intel_dbuf_slice_set(dev_priv,
> +					   _DBUF_CTL_S(DBUF_S2), true);
>  	else
> -		ret = intel_dbuf_slice_set(dev_priv, DBUF_CTL_S2, false);
> +		ret = intel_dbuf_slice_set(dev_priv,
> +					   _DBUF_CTL_S(DBUF_S2), false);
>  
>  	if (ret)
>  		dev_priv->enabled_dbuf_slices_num = req_slices;
> @@ -4428,14 +4430,16 @@ void icl_dbuf_slices_update(struct drm_i915_private *dev_priv,
>  
>  static void icl_dbuf_enable(struct drm_i915_private *dev_priv)
>  {
> -	I915_WRITE(DBUF_CTL_S1, I915_READ(DBUF_CTL_S1) | DBUF_POWER_REQUEST);
> -	I915_WRITE(DBUF_CTL_S2, I915_READ(DBUF_CTL_S2) | DBUF_POWER_REQUEST);
> -	POSTING_READ(DBUF_CTL_S2);
> +	I915_WRITE(_DBUF_CTL_S(DBUF_S1),
> +		   I915_READ(_DBUF_CTL_S(DBUF_S1)) | DBUF_POWER_REQUEST);
> +	I915_WRITE(_DBUF_CTL_S(DBUF_S2),
> +		   I915_READ(_DBUF_CTL_S(DBUF_S2)) | DBUF_POWER_REQUEST);
> +	POSTING_READ(_DBUF_CTL_S(DBUF_S2));
>  
>  	udelay(10);
>  
> -	if (!(I915_READ(DBUF_CTL_S1) & DBUF_POWER_STATE) ||
> -	    !(I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE))
> +	if (!(I915_READ(_DBUF_CTL_S(DBUF_S1)) & DBUF_POWER_STATE) ||
> +	    !(I915_READ(_DBUF_CTL_S(DBUF_S2)) & DBUF_POWER_STATE))
>  		DRM_ERROR("DBuf power enable timeout\n");
>  	else
>  		/*
> @@ -4447,14 +4451,16 @@ static void icl_dbuf_enable(struct drm_i915_private *dev_priv)
>  
>  static void icl_dbuf_disable(struct drm_i915_private *dev_priv)
>  {
> -	I915_WRITE(DBUF_CTL_S1, I915_READ(DBUF_CTL_S1) & ~DBUF_POWER_REQUEST);
> -	I915_WRITE(DBUF_CTL_S2, I915_READ(DBUF_CTL_S2) & ~DBUF_POWER_REQUEST);
> -	POSTING_READ(DBUF_CTL_S2);
> +	I915_WRITE(_DBUF_CTL_S(DBUF_S1),
> +		   I915_READ(_DBUF_CTL_S(DBUF_S1)) & ~DBUF_POWER_REQUEST);
> +	I915_WRITE(_DBUF_CTL_S(DBUF_S2),
> +		   I915_READ(_DBUF_CTL_S(DBUF_S2)) & ~DBUF_POWER_REQUEST);
> +	POSTING_READ(_DBUF_CTL_S(DBUF_S2));
>  
>  	udelay(10);
>  
> -	if ((I915_READ(DBUF_CTL_S1) & DBUF_POWER_STATE) ||
> -	    (I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE))
> +	if ((I915_READ(_DBUF_CTL_S(DBUF_S1)) & DBUF_POWER_STATE) ||
> +	    (I915_READ(_DBUF_CTL_S(DBUF_S2)) & DBUF_POWER_STATE))
>  		DRM_ERROR("DBuf power disable timeout!\n");
>  	else
>  		/*
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
> index 2608a65af7fa..601e000ffd0d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
> @@ -307,6 +307,11 @@ intel_display_power_put_async(struct drm_i915_private *i915,
>  }
>  #endif
>  
> +enum dbuf_slice {
> +	DBUF_S1,
> +	DBUF_S2,
> +};
> +
>  #define with_intel_display_power(i915, domain, wf) \
>  	for ((wf) = intel_display_power_get((i915), (domain)); (wf); \
>  	     intel_display_power_put_async((i915), (domain), (wf)), (wf) = 0)
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b93c4c18f05c..625be54d3eae 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7748,9 +7748,10 @@ enum {
>  #define DISP_ARB_CTL2	_MMIO(0x45004)
>  #define  DISP_DATA_PARTITION_5_6	(1 << 6)
>  #define  DISP_IPC_ENABLE		(1 << 3)
> -#define DBUF_CTL	_MMIO(0x45008)
> -#define DBUF_CTL_S1	_MMIO(0x45008)
> -#define DBUF_CTL_S2	_MMIO(0x44FE8)
> +#define DBUF_CTL_ADDR1			0x45008
> +#define DBUF_CTL_ADDR2			0x44FE8
> +#define _DBUF_CTL_S(X)			_MMIO(_PICK_EVEN(X, DBUF_CTL_ADDR1, DBUF_CTL_ADDR2))

That's not at all what I meant. Also the 'X' is still there despite what
the changelog says.

#define _DBUF_CTL_S1	0x45008
#define _DBUF_CTL_S2	0x44FE8
#define DBUF_CTL_S(slice)	_MMIO(_PICK_EVEN(slice, _DBUF_CTL_S1, _DBUF_CTL_S2))


> +#define DBUF_CTL			_DBUF_CTL_S(0)
>  #define  DBUF_POWER_REQUEST		(1 << 31)
>  #define  DBUF_POWER_STATE		(1 << 30)
>  #define GEN7_MSG_CTL	_MMIO(0x45010)
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 04f94057d6b3..b8d78e26515c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3660,7 +3660,7 @@ u8 intel_enabled_dbuf_slices_num(struct drm_i915_private *dev_priv)
>  	 * only that 1 slice enabled until we have a proper way for on-demand
>  	 * toggling of the second slice.
>  	 */
> -	if (0 && I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE)
> +	if (0 && I915_READ(_DBUF_CTL_S(DBUF_S2)) & DBUF_POWER_STATE)
>  		enabled_dbuf_slices_num++;
>  
>  	return enabled_dbuf_slices_num;
> -- 
> 2.24.1.485.gad05a3d8e5
Stanislav Lisovskiy Jan. 29, 2020, 8:41 a.m. UTC | #2
On Tue, 2020-01-28 at 19:35 +0200, Ville Syrjälä wrote:
> On Fri, Jan 24, 2020 at 10:44:52AM +0200, Stanislav Lisovskiy wrote:
> > Now start using parameterized DBUF_CTL instead
> > of hardcoded, this would allow shorter access
> > functions when reading or storing entire state.
> > 
> > Tried to implement it in a MMIO_PIPE manner, however
> > DBUF_CTL1 address is higher than DBUF_CTL2, which
> > implies that we have to now subtract from base
> > rather than add.
> > 
> > v2: - Removed unneeded DBUF_CTL_DIST and DBUF_CTL_ADDR
> >       macros. Started to use _PICK construct as suggested
> >       by Matt Roper.
> > 
> > v3: - DBUF_CTL_S* to _DBUF_CTL_S*, changed X to "slice"
> >       in macro(Ville Syrjälä)
> >     - Introduced enum for enumerating DBUF slices(Ville Syrjälä)
> > 
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  .../drm/i915/display/intel_display_power.c    | 30 +++++++++++--
> > ------
> >  .../drm/i915/display/intel_display_power.h    |  5 ++++
> >  drivers/gpu/drm/i915/i915_reg.h               |  7 +++--
> >  drivers/gpu/drm/i915/intel_pm.c               |  2 +-
> >  4 files changed, 28 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> > b/drivers/gpu/drm/i915/display/intel_display_power.c
> > index 5e1c601f0f99..a59efb24be92 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > @@ -4418,9 +4418,11 @@ void icl_dbuf_slices_update(struct
> > drm_i915_private *dev_priv,
> >  		return;
> >  
> >  	if (req_slices > hw_enabled_slices)
> > -		ret = intel_dbuf_slice_set(dev_priv, DBUF_CTL_S2,
> > true);
> > +		ret = intel_dbuf_slice_set(dev_priv,
> > +					   _DBUF_CTL_S(DBUF_S2), true);
> >  	else
> > -		ret = intel_dbuf_slice_set(dev_priv, DBUF_CTL_S2,
> > false);
> > +		ret = intel_dbuf_slice_set(dev_priv,
> > +					   _DBUF_CTL_S(DBUF_S2),
> > false);
> >  
> >  	if (ret)
> >  		dev_priv->enabled_dbuf_slices_num = req_slices;
> > @@ -4428,14 +4430,16 @@ void icl_dbuf_slices_update(struct
> > drm_i915_private *dev_priv,
> >  
> >  static void icl_dbuf_enable(struct drm_i915_private *dev_priv)
> >  {
> > -	I915_WRITE(DBUF_CTL_S1, I915_READ(DBUF_CTL_S1) |
> > DBUF_POWER_REQUEST);
> > -	I915_WRITE(DBUF_CTL_S2, I915_READ(DBUF_CTL_S2) |
> > DBUF_POWER_REQUEST);
> > -	POSTING_READ(DBUF_CTL_S2);
> > +	I915_WRITE(_DBUF_CTL_S(DBUF_S1),
> > +		   I915_READ(_DBUF_CTL_S(DBUF_S1)) |
> > DBUF_POWER_REQUEST);
> > +	I915_WRITE(_DBUF_CTL_S(DBUF_S2),
> > +		   I915_READ(_DBUF_CTL_S(DBUF_S2)) |
> > DBUF_POWER_REQUEST);
> > +	POSTING_READ(_DBUF_CTL_S(DBUF_S2));
> >  
> >  	udelay(10);
> >  
> > -	if (!(I915_READ(DBUF_CTL_S1) & DBUF_POWER_STATE) ||
> > -	    !(I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE))
> > +	if (!(I915_READ(_DBUF_CTL_S(DBUF_S1)) & DBUF_POWER_STATE) ||
> > +	    !(I915_READ(_DBUF_CTL_S(DBUF_S2)) & DBUF_POWER_STATE))
> >  		DRM_ERROR("DBuf power enable timeout\n");
> >  	else
> >  		/*
> > @@ -4447,14 +4451,16 @@ static void icl_dbuf_enable(struct
> > drm_i915_private *dev_priv)
> >  
> >  static void icl_dbuf_disable(struct drm_i915_private *dev_priv)
> >  {
> > -	I915_WRITE(DBUF_CTL_S1, I915_READ(DBUF_CTL_S1) &
> > ~DBUF_POWER_REQUEST);
> > -	I915_WRITE(DBUF_CTL_S2, I915_READ(DBUF_CTL_S2) &
> > ~DBUF_POWER_REQUEST);
> > -	POSTING_READ(DBUF_CTL_S2);
> > +	I915_WRITE(_DBUF_CTL_S(DBUF_S1),
> > +		   I915_READ(_DBUF_CTL_S(DBUF_S1)) &
> > ~DBUF_POWER_REQUEST);
> > +	I915_WRITE(_DBUF_CTL_S(DBUF_S2),
> > +		   I915_READ(_DBUF_CTL_S(DBUF_S2)) &
> > ~DBUF_POWER_REQUEST);
> > +	POSTING_READ(_DBUF_CTL_S(DBUF_S2));
> >  
> >  	udelay(10);
> >  
> > -	if ((I915_READ(DBUF_CTL_S1) & DBUF_POWER_STATE) ||
> > -	    (I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE))
> > +	if ((I915_READ(_DBUF_CTL_S(DBUF_S1)) & DBUF_POWER_STATE) ||
> > +	    (I915_READ(_DBUF_CTL_S(DBUF_S2)) & DBUF_POWER_STATE))
> >  		DRM_ERROR("DBuf power disable timeout!\n");
> >  	else
> >  		/*
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h
> > b/drivers/gpu/drm/i915/display/intel_display_power.h
> > index 2608a65af7fa..601e000ffd0d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_power.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
> > @@ -307,6 +307,11 @@ intel_display_power_put_async(struct
> > drm_i915_private *i915,
> >  }
> >  #endif
> >  
> > +enum dbuf_slice {
> > +	DBUF_S1,
> > +	DBUF_S2,
> > +};
> > +
> >  #define with_intel_display_power(i915, domain, wf) \
> >  	for ((wf) = intel_display_power_get((i915), (domain)); (wf); \
> >  	     intel_display_power_put_async((i915), (domain), (wf)),
> > (wf) = 0)
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index b93c4c18f05c..625be54d3eae 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -7748,9 +7748,10 @@ enum {
> >  #define DISP_ARB_CTL2	_MMIO(0x45004)
> >  #define  DISP_DATA_PARTITION_5_6	(1 << 6)
> >  #define  DISP_IPC_ENABLE		(1 << 3)
> > -#define DBUF_CTL	_MMIO(0x45008)
> > -#define DBUF_CTL_S1	_MMIO(0x45008)
> > -#define DBUF_CTL_S2	_MMIO(0x44FE8)
> > +#define DBUF_CTL_ADDR1			0x45008
> > +#define DBUF_CTL_ADDR2			0x44FE8
> > +#define _DBUF_CTL_S(X)			_MMIO(_PICK_EVEN(X,
> > DBUF_CTL_ADDR1, DBUF_CTL_ADDR2))
> 
> That's not at all what I meant. Also the 'X' is still there despite
> what
> the changelog says.
> 
> #define _DBUF_CTL_S1	0x45008
> #define _DBUF_CTL_S2	0x44FE8
> #define DBUF_CTL_S(slice)	_MMIO(_PICK_EVEN(slice, _DBUF_CTL_S1,
> _DBUF_CTL_S2))

My idea was to still be able to use DBUF_CTL_S1 and DBUF_CTL_S2,  of
course this is a bit redundant, but though similar naming DBUF_CTL_S1
and DBUF_CTL_S(0) might confuse somebody into using it. 
For example in your case we can now only use DBUF_CTL_S() macro because
_DBUF_CTL_S1/2 is not any longer using _MMIO.

But _now_ I get your point, I guess by "_" in the beginning, it is
meant not to be used from outside. Some kind of like private class
members in Python :)

I will change this once the rest of patches are reviewed, because that
change anyway does not affect the actual functionality, but purely
cosmetic.

Stan

> 
> 
> > +#define DBUF_CTL			_DBUF_CTL_S(0)
> >  #define  DBUF_POWER_REQUEST		(1 << 31)
> >  #define  DBUF_POWER_STATE		(1 << 30)
> >  #define GEN7_MSG_CTL	_MMIO(0x45010)
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 04f94057d6b3..b8d78e26515c 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3660,7 +3660,7 @@ u8 intel_enabled_dbuf_slices_num(struct
> > drm_i915_private *dev_priv)
> >  	 * only that 1 slice enabled until we have a proper way for on-
> > demand
> >  	 * toggling of the second slice.
> >  	 */
> > -	if (0 && I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE)
> > +	if (0 && I915_READ(_DBUF_CTL_S(DBUF_S2)) & DBUF_POWER_STATE)
> >  		enabled_dbuf_slices_num++;
> >  
> >  	return enabled_dbuf_slices_num;
> > -- 
> > 2.24.1.485.gad05a3d8e5
> 
>
Ville Syrjälä Jan. 29, 2020, 11:47 a.m. UTC | #3
On Wed, Jan 29, 2020 at 08:41:34AM +0000, Lisovskiy, Stanislav wrote:
> On Tue, 2020-01-28 at 19:35 +0200, Ville Syrjälä wrote:
> > On Fri, Jan 24, 2020 at 10:44:52AM +0200, Stanislav Lisovskiy wrote:
> > > Now start using parameterized DBUF_CTL instead
> > > of hardcoded, this would allow shorter access
> > > functions when reading or storing entire state.
> > > 
> > > Tried to implement it in a MMIO_PIPE manner, however
> > > DBUF_CTL1 address is higher than DBUF_CTL2, which
> > > implies that we have to now subtract from base
> > > rather than add.
> > > 
> > > v2: - Removed unneeded DBUF_CTL_DIST and DBUF_CTL_ADDR
> > >       macros. Started to use _PICK construct as suggested
> > >       by Matt Roper.
> > > 
> > > v3: - DBUF_CTL_S* to _DBUF_CTL_S*, changed X to "slice"
> > >       in macro(Ville Syrjälä)
> > >     - Introduced enum for enumerating DBUF slices(Ville Syrjälä)
> > > 
> > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > ---
> > >  .../drm/i915/display/intel_display_power.c    | 30 +++++++++++--
> > > ------
> > >  .../drm/i915/display/intel_display_power.h    |  5 ++++
> > >  drivers/gpu/drm/i915/i915_reg.h               |  7 +++--
> > >  drivers/gpu/drm/i915/intel_pm.c               |  2 +-
> > >  4 files changed, 28 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> > > b/drivers/gpu/drm/i915/display/intel_display_power.c
> > > index 5e1c601f0f99..a59efb24be92 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > > @@ -4418,9 +4418,11 @@ void icl_dbuf_slices_update(struct
> > > drm_i915_private *dev_priv,
> > >  		return;
> > >  
> > >  	if (req_slices > hw_enabled_slices)
> > > -		ret = intel_dbuf_slice_set(dev_priv, DBUF_CTL_S2,
> > > true);
> > > +		ret = intel_dbuf_slice_set(dev_priv,
> > > +					   _DBUF_CTL_S(DBUF_S2), true);
> > >  	else
> > > -		ret = intel_dbuf_slice_set(dev_priv, DBUF_CTL_S2,
> > > false);
> > > +		ret = intel_dbuf_slice_set(dev_priv,
> > > +					   _DBUF_CTL_S(DBUF_S2),
> > > false);
> > >  
> > >  	if (ret)
> > >  		dev_priv->enabled_dbuf_slices_num = req_slices;
> > > @@ -4428,14 +4430,16 @@ void icl_dbuf_slices_update(struct
> > > drm_i915_private *dev_priv,
> > >  
> > >  static void icl_dbuf_enable(struct drm_i915_private *dev_priv)
> > >  {
> > > -	I915_WRITE(DBUF_CTL_S1, I915_READ(DBUF_CTL_S1) |
> > > DBUF_POWER_REQUEST);
> > > -	I915_WRITE(DBUF_CTL_S2, I915_READ(DBUF_CTL_S2) |
> > > DBUF_POWER_REQUEST);
> > > -	POSTING_READ(DBUF_CTL_S2);
> > > +	I915_WRITE(_DBUF_CTL_S(DBUF_S1),
> > > +		   I915_READ(_DBUF_CTL_S(DBUF_S1)) |
> > > DBUF_POWER_REQUEST);
> > > +	I915_WRITE(_DBUF_CTL_S(DBUF_S2),
> > > +		   I915_READ(_DBUF_CTL_S(DBUF_S2)) |
> > > DBUF_POWER_REQUEST);
> > > +	POSTING_READ(_DBUF_CTL_S(DBUF_S2));
> > >  
> > >  	udelay(10);
> > >  
> > > -	if (!(I915_READ(DBUF_CTL_S1) & DBUF_POWER_STATE) ||
> > > -	    !(I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE))
> > > +	if (!(I915_READ(_DBUF_CTL_S(DBUF_S1)) & DBUF_POWER_STATE) ||
> > > +	    !(I915_READ(_DBUF_CTL_S(DBUF_S2)) & DBUF_POWER_STATE))
> > >  		DRM_ERROR("DBuf power enable timeout\n");
> > >  	else
> > >  		/*
> > > @@ -4447,14 +4451,16 @@ static void icl_dbuf_enable(struct
> > > drm_i915_private *dev_priv)
> > >  
> > >  static void icl_dbuf_disable(struct drm_i915_private *dev_priv)
> > >  {
> > > -	I915_WRITE(DBUF_CTL_S1, I915_READ(DBUF_CTL_S1) &
> > > ~DBUF_POWER_REQUEST);
> > > -	I915_WRITE(DBUF_CTL_S2, I915_READ(DBUF_CTL_S2) &
> > > ~DBUF_POWER_REQUEST);
> > > -	POSTING_READ(DBUF_CTL_S2);
> > > +	I915_WRITE(_DBUF_CTL_S(DBUF_S1),
> > > +		   I915_READ(_DBUF_CTL_S(DBUF_S1)) &
> > > ~DBUF_POWER_REQUEST);
> > > +	I915_WRITE(_DBUF_CTL_S(DBUF_S2),
> > > +		   I915_READ(_DBUF_CTL_S(DBUF_S2)) &
> > > ~DBUF_POWER_REQUEST);
> > > +	POSTING_READ(_DBUF_CTL_S(DBUF_S2));
> > >  
> > >  	udelay(10);
> > >  
> > > -	if ((I915_READ(DBUF_CTL_S1) & DBUF_POWER_STATE) ||
> > > -	    (I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE))
> > > +	if ((I915_READ(_DBUF_CTL_S(DBUF_S1)) & DBUF_POWER_STATE) ||
> > > +	    (I915_READ(_DBUF_CTL_S(DBUF_S2)) & DBUF_POWER_STATE))
> > >  		DRM_ERROR("DBuf power disable timeout!\n");
> > >  	else
> > >  		/*
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h
> > > b/drivers/gpu/drm/i915/display/intel_display_power.h
> > > index 2608a65af7fa..601e000ffd0d 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_power.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
> > > @@ -307,6 +307,11 @@ intel_display_power_put_async(struct
> > > drm_i915_private *i915,
> > >  }
> > >  #endif
> > >  
> > > +enum dbuf_slice {
> > > +	DBUF_S1,
> > > +	DBUF_S2,
> > > +};
> > > +
> > >  #define with_intel_display_power(i915, domain, wf) \
> > >  	for ((wf) = intel_display_power_get((i915), (domain)); (wf); \
> > >  	     intel_display_power_put_async((i915), (domain), (wf)),
> > > (wf) = 0)
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index b93c4c18f05c..625be54d3eae 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -7748,9 +7748,10 @@ enum {
> > >  #define DISP_ARB_CTL2	_MMIO(0x45004)
> > >  #define  DISP_DATA_PARTITION_5_6	(1 << 6)
> > >  #define  DISP_IPC_ENABLE		(1 << 3)
> > > -#define DBUF_CTL	_MMIO(0x45008)
> > > -#define DBUF_CTL_S1	_MMIO(0x45008)
> > > -#define DBUF_CTL_S2	_MMIO(0x44FE8)
> > > +#define DBUF_CTL_ADDR1			0x45008
> > > +#define DBUF_CTL_ADDR2			0x44FE8
> > > +#define _DBUF_CTL_S(X)			_MMIO(_PICK_EVEN(X,
> > > DBUF_CTL_ADDR1, DBUF_CTL_ADDR2))
> > 
> > That's not at all what I meant. Also the 'X' is still there despite
> > what
> > the changelog says.
> > 
> > #define _DBUF_CTL_S1	0x45008
> > #define _DBUF_CTL_S2	0x44FE8
> > #define DBUF_CTL_S(slice)	_MMIO(_PICK_EVEN(slice, _DBUF_CTL_S1,
> > _DBUF_CTL_S2))
> 
> My idea was to still be able to use DBUF_CTL_S1 and DBUF_CTL_S2,  of

Just don't. Aliases are confusing.

> course this is a bit redundant, but though similar naming DBUF_CTL_S1
> and DBUF_CTL_S(0) might confuse somebody into using it. 
> For example in your case we can now only use DBUF_CTL_S() macro because
> _DBUF_CTL_S1/2 is not any longer using _MMIO.
> 
> But _now_ I get your point, I guess by "_" in the beginning, it is
> meant not to be used from outside. Some kind of like private class
> members in Python :)
> 
> I will change this once the rest of patches are reviewed, because that
> change anyway does not affect the actual functionality, but purely
> cosmetic.
> 
> Stan
> 
> > 
> > 
> > > +#define DBUF_CTL			_DBUF_CTL_S(0)
> > >  #define  DBUF_POWER_REQUEST		(1 << 31)
> > >  #define  DBUF_POWER_STATE		(1 << 30)
> > >  #define GEN7_MSG_CTL	_MMIO(0x45010)
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index 04f94057d6b3..b8d78e26515c 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3660,7 +3660,7 @@ u8 intel_enabled_dbuf_slices_num(struct
> > > drm_i915_private *dev_priv)
> > >  	 * only that 1 slice enabled until we have a proper way for on-
> > > demand
> > >  	 * toggling of the second slice.
> > >  	 */
> > > -	if (0 && I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE)
> > > +	if (0 && I915_READ(_DBUF_CTL_S(DBUF_S2)) & DBUF_POWER_STATE)
> > >  		enabled_dbuf_slices_num++;
> > >  
> > >  	return enabled_dbuf_slices_num;
> > > -- 
> > > 2.24.1.485.gad05a3d8e5
> > 
> >

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
index 5e1c601f0f99..a59efb24be92 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.c
+++ b/drivers/gpu/drm/i915/display/intel_display_power.c
@@ -4418,9 +4418,11 @@  void icl_dbuf_slices_update(struct drm_i915_private *dev_priv,
 		return;
 
 	if (req_slices > hw_enabled_slices)
-		ret = intel_dbuf_slice_set(dev_priv, DBUF_CTL_S2, true);
+		ret = intel_dbuf_slice_set(dev_priv,
+					   _DBUF_CTL_S(DBUF_S2), true);
 	else
-		ret = intel_dbuf_slice_set(dev_priv, DBUF_CTL_S2, false);
+		ret = intel_dbuf_slice_set(dev_priv,
+					   _DBUF_CTL_S(DBUF_S2), false);
 
 	if (ret)
 		dev_priv->enabled_dbuf_slices_num = req_slices;
@@ -4428,14 +4430,16 @@  void icl_dbuf_slices_update(struct drm_i915_private *dev_priv,
 
 static void icl_dbuf_enable(struct drm_i915_private *dev_priv)
 {
-	I915_WRITE(DBUF_CTL_S1, I915_READ(DBUF_CTL_S1) | DBUF_POWER_REQUEST);
-	I915_WRITE(DBUF_CTL_S2, I915_READ(DBUF_CTL_S2) | DBUF_POWER_REQUEST);
-	POSTING_READ(DBUF_CTL_S2);
+	I915_WRITE(_DBUF_CTL_S(DBUF_S1),
+		   I915_READ(_DBUF_CTL_S(DBUF_S1)) | DBUF_POWER_REQUEST);
+	I915_WRITE(_DBUF_CTL_S(DBUF_S2),
+		   I915_READ(_DBUF_CTL_S(DBUF_S2)) | DBUF_POWER_REQUEST);
+	POSTING_READ(_DBUF_CTL_S(DBUF_S2));
 
 	udelay(10);
 
-	if (!(I915_READ(DBUF_CTL_S1) & DBUF_POWER_STATE) ||
-	    !(I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE))
+	if (!(I915_READ(_DBUF_CTL_S(DBUF_S1)) & DBUF_POWER_STATE) ||
+	    !(I915_READ(_DBUF_CTL_S(DBUF_S2)) & DBUF_POWER_STATE))
 		DRM_ERROR("DBuf power enable timeout\n");
 	else
 		/*
@@ -4447,14 +4451,16 @@  static void icl_dbuf_enable(struct drm_i915_private *dev_priv)
 
 static void icl_dbuf_disable(struct drm_i915_private *dev_priv)
 {
-	I915_WRITE(DBUF_CTL_S1, I915_READ(DBUF_CTL_S1) & ~DBUF_POWER_REQUEST);
-	I915_WRITE(DBUF_CTL_S2, I915_READ(DBUF_CTL_S2) & ~DBUF_POWER_REQUEST);
-	POSTING_READ(DBUF_CTL_S2);
+	I915_WRITE(_DBUF_CTL_S(DBUF_S1),
+		   I915_READ(_DBUF_CTL_S(DBUF_S1)) & ~DBUF_POWER_REQUEST);
+	I915_WRITE(_DBUF_CTL_S(DBUF_S2),
+		   I915_READ(_DBUF_CTL_S(DBUF_S2)) & ~DBUF_POWER_REQUEST);
+	POSTING_READ(_DBUF_CTL_S(DBUF_S2));
 
 	udelay(10);
 
-	if ((I915_READ(DBUF_CTL_S1) & DBUF_POWER_STATE) ||
-	    (I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE))
+	if ((I915_READ(_DBUF_CTL_S(DBUF_S1)) & DBUF_POWER_STATE) ||
+	    (I915_READ(_DBUF_CTL_S(DBUF_S2)) & DBUF_POWER_STATE))
 		DRM_ERROR("DBuf power disable timeout!\n");
 	else
 		/*
diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
index 2608a65af7fa..601e000ffd0d 100644
--- a/drivers/gpu/drm/i915/display/intel_display_power.h
+++ b/drivers/gpu/drm/i915/display/intel_display_power.h
@@ -307,6 +307,11 @@  intel_display_power_put_async(struct drm_i915_private *i915,
 }
 #endif
 
+enum dbuf_slice {
+	DBUF_S1,
+	DBUF_S2,
+};
+
 #define with_intel_display_power(i915, domain, wf) \
 	for ((wf) = intel_display_power_get((i915), (domain)); (wf); \
 	     intel_display_power_put_async((i915), (domain), (wf)), (wf) = 0)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index b93c4c18f05c..625be54d3eae 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7748,9 +7748,10 @@  enum {
 #define DISP_ARB_CTL2	_MMIO(0x45004)
 #define  DISP_DATA_PARTITION_5_6	(1 << 6)
 #define  DISP_IPC_ENABLE		(1 << 3)
-#define DBUF_CTL	_MMIO(0x45008)
-#define DBUF_CTL_S1	_MMIO(0x45008)
-#define DBUF_CTL_S2	_MMIO(0x44FE8)
+#define DBUF_CTL_ADDR1			0x45008
+#define DBUF_CTL_ADDR2			0x44FE8
+#define _DBUF_CTL_S(X)			_MMIO(_PICK_EVEN(X, DBUF_CTL_ADDR1, DBUF_CTL_ADDR2))
+#define DBUF_CTL			_DBUF_CTL_S(0)
 #define  DBUF_POWER_REQUEST		(1 << 31)
 #define  DBUF_POWER_STATE		(1 << 30)
 #define GEN7_MSG_CTL	_MMIO(0x45010)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 04f94057d6b3..b8d78e26515c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3660,7 +3660,7 @@  u8 intel_enabled_dbuf_slices_num(struct drm_i915_private *dev_priv)
 	 * only that 1 slice enabled until we have a proper way for on-demand
 	 * toggling of the second slice.
 	 */
-	if (0 && I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE)
+	if (0 && I915_READ(_DBUF_CTL_S(DBUF_S2)) & DBUF_POWER_STATE)
 		enabled_dbuf_slices_num++;
 
 	return enabled_dbuf_slices_num;