Re: [Intel-gfx] [PATCH v12 4/5] drm/i915: Introduce parameterized DBUF_CTL

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Jan 16, 2020 at 11:22:13AM +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.
> 
> 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    | 19 +++----------------
>  drivers/gpu/drm/i915/i915_reg.h               |  7 ++++---
>  drivers/gpu/drm/i915/intel_pm.c               | 18 ++----------------
>  3 files changed, 9 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> index 474d6d4f3eb5..4729bcafb937 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> @@ -4410,22 +4410,9 @@ void icl_dbuf_slices_update(struct drm_i915_private *dev_priv,
>  	DRM_DEBUG_KMS("Updating dbuf slices to 0x%x\n", req_slices);
>  
>  	for (i = 0; i < max_slices; i++) {
> -		u8 slice_set = req_slices & BIT(i);
> -
> -		switch (i) {
> -		case DBUF_S1:
> -			intel_dbuf_slice_set(dev_priv,
> -					     DBUF_CTL_S1,
> -					     slice_set);
> -			break;
> -		case DBUF_S2:
> -			intel_dbuf_slice_set(dev_priv,
> -					     DBUF_CTL_S2,
> -					     slice_set);
> -			break;
> -		default:
> -			MISSING_CASE(i);
> -		}

I think you added this code in one of the previous patches. So would be
better to make this patch the first in the series so you don't end up
adding code only to rewrite it one patch later.

> +		intel_dbuf_slice_set(dev_priv,
> +				     DBUF_CTL_S(i),
> +				     (req_slices & BIT(i)) != 0);
>  	}
>  
>  	dev_priv->enabled_dbuf_slices_mask = req_slices;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index e5071af4a3b3..b3de69a0ea50 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7745,9 +7745,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

_DBUF_CTL_S1 etc. is the usual naming pattern for the raw reg offsets.

> +#define DBUF_CTL_S(X)			_MMIO(_PICK(X, DBUF_CTL_ADDR1, DBUF_CTL_ADDR2))

s/X/slice/ perhaps

_PICK_EVEN() will work just fine here.

With those
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Was more or less also expecting to see the enum here, but I guess
that ended up somewhere else?

> +#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 525578933658..b4b291d4244b 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3651,22 +3651,8 @@ u8 intel_enabled_dbuf_slices_mask(struct drm_i915_private *dev_priv)
>  	u8 enabled_slices_mask = 0;
>  
>  	for (i = 0; i < max_slices; i++) {
> -		u8 slice_bit = BIT(i);
> -		bool res;
> -
> -		switch (i) {
> -		case DBUF_S1:
> -			res = I915_READ(DBUF_CTL_S1) & DBUF_POWER_STATE;
> -			break;
> -		case DBUF_S2:
> -			res = I915_READ(DBUF_CTL_S2) & DBUF_POWER_STATE;
> -			break;
> -		default:
> -			MISSING_CASE(slice_bit);
> -		}
> -
> -		if (res)
> -			enabled_slices_mask |= slice_bit;
> +		if (I915_READ(DBUF_CTL_S(i)) & DBUF_POWER_STATE)
> +			enabled_slices_mask |= BIT(i);
>  	}
>  
>  	return enabled_slices_mask;
> -- 
> 2.24.1.485.gad05a3d8e5

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx