[v2,1/4] drm/i915/fbc: Parametrize FBC register offsets

Message ID	20211213134450.3082-2-ville.syrjala@linux.intel.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <intel-gfx-bounces@lists.freedesktop.org> From: Ville Syrjala <ville.syrjala@linux.intel.com> To: intel-gfx@lists.freedesktop.org Date: Mon, 13 Dec 2021 15:44:47 +0200 Message-Id: <20211213134450.3082-2-ville.syrjala@linux.intel.com> In-Reply-To: <20211213134450.3082-1-ville.syrjala@linux.intel.com> References: <20211213134450.3082-1-ville.syrjala@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Subject: [Intel-gfx] [PATCH v2 1/4] drm/i915/fbc: Parametrize FBC register offsets Precedence: list Cc: Jani Nikula <jani.nikula@intel.com> Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" <intel-gfx-bounces@lists.freedesktop.org>
Series	drm/i915/fbc: More multi-FBC refactoring \| expand [v2,0/4] drm/i915/fbc: More multi-FBC refactoring [v2,1/4] drm/i915/fbc: Parametrize FBC register offsets [v2,2/4] drm/i915/fbc: Loop through FBC instances in various places [v2,3/4] drm/i915/fbc: Introduce device info fbc_mask [v2,4/4] drm/i915/fbc: Register per-crtc debugfs files

Ville Syrjälä Dec. 13, 2021, 1:44 p.m. UTC

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Parametrize ilk+ FBC register offsets based on the FBC instance.

v2: More intel_ namespace (Jani)

Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_fbc.c | 34 +++++++++++++-----------
 drivers/gpu/drm/i915/display/intel_fbc.h |  6 +++++
 drivers/gpu/drm/i915/i915_reg.h          | 34 ++++++++++++------------
 drivers/gpu/drm/i915/intel_pm.c          | 31 ++++++++++++---------
 4 files changed, 60 insertions(+), 45 deletions(-)

Jani Nikula Dec. 13, 2021, 7:54 p.m. UTC | #1

On Mon, 13 Dec 2021, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Parametrize ilk+ FBC register offsets based on the FBC instance.
>
> v2: More intel_ namespace (Jani)
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Some questions below, apart from that,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_fbc.c | 34 +++++++++++++-----------
>  drivers/gpu/drm/i915/display/intel_fbc.h |  6 +++++
>  drivers/gpu/drm/i915/i915_reg.h          | 34 ++++++++++++------------
>  drivers/gpu/drm/i915/intel_pm.c          | 31 ++++++++++++---------
>  4 files changed, 60 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> index 8be01b93015f..112aafa72253 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> @@ -85,6 +85,8 @@ struct intel_fbc {
>  	struct drm_mm_node compressed_fb;
>  	struct drm_mm_node compressed_llb;
>  
> +	enum intel_fbc_id id;
> +
>  	u8 limit;
>  
>  	bool false_color;
> @@ -454,10 +456,10 @@ static void ilk_fbc_activate(struct intel_fbc *fbc)
>  	struct intel_fbc_state *fbc_state = &fbc->state;
>  	struct drm_i915_private *i915 = fbc->i915;
>  
> -	intel_de_write(i915, ILK_DPFC_FENCE_YOFF,
> +	intel_de_write(i915, ILK_DPFC_FENCE_YOFF(fbc->id),
>  		       fbc_state->fence_y_offset);
>  
> -	intel_de_write(i915, ILK_DPFC_CONTROL,
> +	intel_de_write(i915, ILK_DPFC_CONTROL(fbc->id),
>  		       DPFC_CTL_EN | g4x_dpfc_ctl(fbc));
>  }
>  
> @@ -467,28 +469,28 @@ static void ilk_fbc_deactivate(struct intel_fbc *fbc)
>  	u32 dpfc_ctl;
>  
>  	/* Disable compression */
> -	dpfc_ctl = intel_de_read(i915, ILK_DPFC_CONTROL);
> +	dpfc_ctl = intel_de_read(i915, ILK_DPFC_CONTROL(fbc->id));
>  	if (dpfc_ctl & DPFC_CTL_EN) {
>  		dpfc_ctl &= ~DPFC_CTL_EN;
> -		intel_de_write(i915, ILK_DPFC_CONTROL, dpfc_ctl);
> +		intel_de_write(i915, ILK_DPFC_CONTROL(fbc->id), dpfc_ctl);
>  	}
>  }
>  
>  static bool ilk_fbc_is_active(struct intel_fbc *fbc)
>  {
> -	return intel_de_read(fbc->i915, ILK_DPFC_CONTROL) & DPFC_CTL_EN;
> +	return intel_de_read(fbc->i915, ILK_DPFC_CONTROL(fbc->id)) & DPFC_CTL_EN;
>  }
>  
>  static bool ilk_fbc_is_compressing(struct intel_fbc *fbc)
>  {
> -	return intel_de_read(fbc->i915, ILK_DPFC_STATUS) & DPFC_COMP_SEG_MASK;
> +	return intel_de_read(fbc->i915, ILK_DPFC_STATUS(fbc->id)) & DPFC_COMP_SEG_MASK;
>  }
>  
>  static void ilk_fbc_program_cfb(struct intel_fbc *fbc)
>  {
>  	struct drm_i915_private *i915 = fbc->i915;
>  
> -	intel_de_write(i915, ILK_DPFC_CB_BASE, fbc->compressed_fb.start);
> +	intel_de_write(i915, ILK_DPFC_CB_BASE(fbc->id), fbc->compressed_fb.start);
>  }
>  
>  static const struct intel_fbc_funcs ilk_fbc_funcs = {
> @@ -524,8 +526,8 @@ static void snb_fbc_nuke(struct intel_fbc *fbc)
>  {
>  	struct drm_i915_private *i915 = fbc->i915;
>  
> -	intel_de_write(i915, MSG_FBC_REND_STATE, FBC_REND_NUKE);
> -	intel_de_posting_read(i915, MSG_FBC_REND_STATE);
> +	intel_de_write(i915, MSG_FBC_REND_STATE(fbc->id), FBC_REND_NUKE);
> +	intel_de_posting_read(i915, MSG_FBC_REND_STATE(fbc->id));
>  }
>  
>  static const struct intel_fbc_funcs snb_fbc_funcs = {
> @@ -547,7 +549,7 @@ static void glk_fbc_program_cfb_stride(struct intel_fbc *fbc)
>  		val |= FBC_STRIDE_OVERRIDE |
>  			FBC_STRIDE(fbc_state->override_cfb_stride / fbc->limit);
>  
> -	intel_de_write(i915, GLK_FBC_STRIDE, val);
> +	intel_de_write(i915, GLK_FBC_STRIDE(fbc->id), val);
>  }
>  
>  static void skl_fbc_program_cfb_stride(struct intel_fbc *fbc)
> @@ -598,19 +600,19 @@ static void ivb_fbc_activate(struct intel_fbc *fbc)
>  	if (i915->ggtt.num_fences)
>  		snb_fbc_program_fence(fbc);
>  
> -	intel_de_write(i915, ILK_DPFC_CONTROL,
> +	intel_de_write(i915, ILK_DPFC_CONTROL(fbc->id),
>  		       DPFC_CTL_EN | ivb_dpfc_ctl(fbc));
>  }
>  
>  static bool ivb_fbc_is_compressing(struct intel_fbc *fbc)
>  {
> -	return intel_de_read(fbc->i915, ILK_DPFC_STATUS2) & DPFC_COMP_SEG_MASK_IVB;
> +	return intel_de_read(fbc->i915, ILK_DPFC_STATUS2(fbc->id)) & DPFC_COMP_SEG_MASK_IVB;
>  }
>  
>  static void ivb_fbc_set_false_color(struct intel_fbc *fbc,
>  				    bool enable)
>  {
> -	intel_de_rmw(fbc->i915, ILK_DPFC_CONTROL,
> +	intel_de_rmw(fbc->i915, ILK_DPFC_CONTROL(fbc->id),
>  		     DPFC_CTL_FALSE_COLOR, enable ? DPFC_CTL_FALSE_COLOR : 0);
>  }
>  
> @@ -1620,7 +1622,8 @@ void intel_fbc_add_plane(struct intel_fbc *fbc, struct intel_plane *plane)
>  	fbc->possible_framebuffer_bits |= plane->frontbuffer_bit;
>  }
>  
> -static struct intel_fbc *intel_fbc_create(struct drm_i915_private *i915)
> +static struct intel_fbc *intel_fbc_create(struct drm_i915_private *i915,
> +					  enum intel_fbc_id fbc_id)
>  {
>  	struct intel_fbc *fbc;
>  
> @@ -1628,6 +1631,7 @@ static struct intel_fbc *intel_fbc_create(struct drm_i915_private *i915)
>  	if (!fbc)
>  		return NULL;
>  
> +	fbc->id = fbc_id;
>  	fbc->i915 = i915;
>  	INIT_WORK(&fbc->underrun_work, intel_fbc_underrun_work_fn);
>  	mutex_init(&fbc->lock);
> @@ -1671,7 +1675,7 @@ void intel_fbc_init(struct drm_i915_private *i915)
>  	if (!HAS_FBC(i915))
>  		return;
>  
> -	fbc = intel_fbc_create(i915);
> +	fbc = intel_fbc_create(i915, INTEL_FBC_A);
>  	if (!fbc)
>  		return;
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.h b/drivers/gpu/drm/i915/display/intel_fbc.h
> index 07ad0411fcc3..7b7631aec527 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbc.h
> +++ b/drivers/gpu/drm/i915/display/intel_fbc.h
> @@ -17,6 +17,12 @@ struct intel_fbc;
>  struct intel_plane;
>  struct intel_plane_state;
>  
> +enum intel_fbc_id {
> +	INTEL_FBC_A,
> +
> +	I915_MAX_FBCS,
> +};
> +
>  int intel_fbc_atomic_check(struct intel_atomic_state *state);
>  bool intel_fbc_pre_update(struct intel_atomic_state *state,
>  			  struct intel_crtc *crtc);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index d27ba273cc68..698a023e70f5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3386,10 +3386,10 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>  #define FBC_LL_SIZE		(1536)
>  
>  /* Framebuffer compression for GM45+ */
> -#define DPFC_CB_BASE		_MMIO(0x3200)
> -#define ILK_DPFC_CB_BASE	_MMIO(0x43200)
> -#define DPFC_CONTROL		_MMIO(0x3208)
> -#define ILK_DPFC_CONTROL	_MMIO(0x43208)
> +#define DPFC_CB_BASE			_MMIO(0x3200)
> +#define ILK_DPFC_CB_BASE(fbc_id)	_MMIO_PIPE((fbc_id), 0x43200, 0x43240)
> +#define DPFC_CONTROL			_MMIO(0x3208)
> +#define ILK_DPFC_CONTROL(fbc_id)	_MMIO_PIPE((fbc_id), 0x43208, 0x43248)
>  #define   DPFC_CTL_EN				REG_BIT(31)
>  #define   DPFC_CTL_PLANE_MASK_G4X		REG_BIT(30) /* g4x-snb */
>  #define   DPFC_CTL_PLANE_G4X(i9xx_plane)	REG_FIELD_PREP(DPFC_CTL_PLANE_MASK_G4X, (i9xx_plane))
> @@ -3407,28 +3407,28 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>  #define   DPFC_CTL_LIMIT_4X			REG_FIELD_PREP(DPFC_CTL_LIMIT_MASK, 2)
>  #define   DPFC_CTL_FENCENO_MASK			REG_GENMASK(3, 0)
>  #define   DPFC_CTL_FENCENO(fence)		REG_FIELD_PREP(DPFC_CTL_FENCENO_MASK, (fence))
> -#define DPFC_RECOMP_CTL		_MMIO(0x320c)
> -#define ILK_DPFC_RECOMP_CTL	_MMIO(0x4320c)
> +#define DPFC_RECOMP_CTL			_MMIO(0x320c)
> +#define ILK_DPFC_RECOMP_CTL(fbc_id)	_MMIO_PIPE((fbc_id), 0x4320c, 0x4324c)

This is display 5 and 6 only, right? Will there be a register instance
for fbc_id > INTEL_FBC_A? Or is the parametrization just for
completeness?

This one is only used in gvt, anyway. And that actually makes me wonder
if this should be breaking the build. Does CI not have gvt enabled?

>  #define   DPFC_RECOMP_STALL_EN			REG_BIT(27)
>  #define   DPFC_RECOMP_STALL_WM_MASK		REG_GENMASK(26, 16)
>  #define   DPFC_RECOMP_TIMER_COUNT_MASK		REG_GENMASK(5, 0)
> -#define DPFC_STATUS		_MMIO(0x3210)
> -#define ILK_DPFC_STATUS		_MMIO(0x43210)
> +#define DPFC_STATUS			_MMIO(0x3210)
> +#define ILK_DPFC_STATUS(fbc_id)		_MMIO_PIPE((fbc_id), 0x43210, 0x43250)

Ditto, apart from the gvt part.

>  #define   DPFC_INVAL_SEG_MASK			REG_GENMASK(26, 16)
>  #define   DPFC_COMP_SEG_MASK			REG_GENMASK(10, 0)
> -#define DPFC_STATUS2		_MMIO(0x3214)
> -#define ILK_DPFC_STATUS2		_MMIO(0x43214)
> +#define DPFC_STATUS2			_MMIO(0x3214)
> +#define ILK_DPFC_STATUS2(fbc_id)	_MMIO_PIPE((fbc_id), 0x43214, 0x43254)
>  #define   DPFC_COMP_SEG_MASK_IVB		REG_GENMASK(11, 0)
> -#define DPFC_FENCE_YOFF		_MMIO(0x3218)
> -#define ILK_DPFC_FENCE_YOFF	_MMIO(0x43218)
> -#define DPFC_CHICKEN		_MMIO(0x3224)
> -#define ILK_DPFC_CHICKEN	_MMIO(0x43224)
> +#define DPFC_FENCE_YOFF			_MMIO(0x3218)
> +#define ILK_DPFC_FENCE_YOFF(fbc_id)	_MMIO_PIPE((fbc_id), 0x43218, 0x43258)

Ditto.

BR,
Jani.

> +#define DPFC_CHICKEN			_MMIO(0x3224)
> +#define ILK_DPFC_CHICKEN(fbc_id)	_MMIO_PIPE((fbc_id), 0x43224, 0x43264)
>  #define   DPFC_HT_MODIFY			REG_BIT(31) /* pre-ivb */
>  #define   DPFC_NUKE_ON_ANY_MODIFICATION		REG_BIT(23) /* bdw+ */
>  #define   DPFC_CHICKEN_COMP_DUMMY_PIXEL		REG_BIT(14) /* glk+ */
>  #define   DPFC_DISABLE_DUMMY0			REG_BIT(8) /* ivb+ */
>  
> -#define GLK_FBC_STRIDE		_MMIO(0x43228)
> +#define GLK_FBC_STRIDE(fbc_id)	_MMIO_PIPE((fbc_id), 0x43228, 0x43268)
>  #define   FBC_STRIDE_OVERRIDE	REG_BIT(15)
>  #define   FBC_STRIDE_MASK	REG_GENMASK(14, 0)
>  #define   FBC_STRIDE(x)		REG_FIELD_PREP(FBC_STRIDE_MASK, (x))
> @@ -3471,9 +3471,9 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>  #define IPS_CTL		_MMIO(0x43408)
>  #define   IPS_ENABLE	(1 << 31)
>  
> -#define MSG_FBC_REND_STATE	_MMIO(0x50380)
> +#define MSG_FBC_REND_STATE(fbc_id)	_MMIO_PIPE((fbc_id), 0x50380, 0x50384)
>  #define   FBC_REND_NUKE			REG_BIT(2)
> -#define   FBC_REND_CACHE_CLEAN			REG_BIT(1)
> +#define   FBC_REND_CACHE_CLEAN		REG_BIT(1)
>  
>  /*
>   * GPIO regs
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 434b1f8b7fe3..bdf97a8c9ef3 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -160,8 +160,9 @@ static void bxt_init_clock_gating(struct drm_i915_private *dev_priv)
>  	 * WaFbcHighMemBwCorruptionAvoidance:bxt
>  	 * Display WA #0883: bxt
>  	 */
> -	intel_uncore_write(&dev_priv->uncore, ILK_DPFC_CHICKEN, intel_uncore_read(&dev_priv->uncore, ILK_DPFC_CHICKEN) |
> -		   DPFC_DISABLE_DUMMY0);
> +	intel_uncore_write(&dev_priv->uncore, ILK_DPFC_CHICKEN(INTEL_FBC_A),
> +			   intel_uncore_read(&dev_priv->uncore, ILK_DPFC_CHICKEN(INTEL_FBC_A)) |
> +			   DPFC_DISABLE_DUMMY0);
>  }
>  
>  static void glk_init_clock_gating(struct drm_i915_private *dev_priv)
> @@ -7451,8 +7452,8 @@ static void gen8_set_l3sqc_credits(struct drm_i915_private *dev_priv,
>  static void icl_init_clock_gating(struct drm_i915_private *dev_priv)
>  {
>  	/* Wa_1409120013:icl,ehl */
> -	intel_uncore_write(&dev_priv->uncore, ILK_DPFC_CHICKEN,
> -		   DPFC_CHICKEN_COMP_DUMMY_PIXEL);
> +	intel_uncore_write(&dev_priv->uncore, ILK_DPFC_CHICKEN(INTEL_FBC_A),
> +			   DPFC_CHICKEN_COMP_DUMMY_PIXEL);
>  
>  	/*Wa_14010594013:icl, ehl */
>  	intel_uncore_rmw(&dev_priv->uncore, GEN8_CHICKEN_DCPR_1,
> @@ -7464,7 +7465,7 @@ static void gen12lp_init_clock_gating(struct drm_i915_private *dev_priv)
>  	/* Wa_1409120013:tgl,rkl,adl-s,dg1,dg2 */
>  	if (IS_TIGERLAKE(dev_priv) || IS_ROCKETLAKE(dev_priv) ||
>  	    IS_ALDERLAKE_S(dev_priv) || IS_DG1(dev_priv) || IS_DG2(dev_priv))
> -		intel_uncore_write(&dev_priv->uncore, ILK_DPFC_CHICKEN,
> +		intel_uncore_write(&dev_priv->uncore, ILK_DPFC_CHICKEN(INTEL_FBC_A),
>  				   DPFC_CHICKEN_COMP_DUMMY_PIXEL);
>  
>  	/* Wa_1409825376:tgl (pre-prod)*/
> @@ -7549,8 +7550,9 @@ static void cfl_init_clock_gating(struct drm_i915_private *dev_priv)
>  	 * WaFbcNukeOnHostModify:cfl
>  	 * Display WA #0873: cfl
>  	 */
> -	intel_uncore_write(&dev_priv->uncore, ILK_DPFC_CHICKEN, intel_uncore_read(&dev_priv->uncore, ILK_DPFC_CHICKEN) |
> -		   DPFC_NUKE_ON_ANY_MODIFICATION);
> +	intel_uncore_write(&dev_priv->uncore, ILK_DPFC_CHICKEN(INTEL_FBC_A),
> +			   intel_uncore_read(&dev_priv->uncore, ILK_DPFC_CHICKEN(INTEL_FBC_A)) |
> +			   DPFC_NUKE_ON_ANY_MODIFICATION);
>  }
>  
>  static void kbl_init_clock_gating(struct drm_i915_private *dev_priv)
> @@ -7582,8 +7584,9 @@ static void kbl_init_clock_gating(struct drm_i915_private *dev_priv)
>  	 * WaFbcNukeOnHostModify:kbl
>  	 * Display WA #0873: kbl
>  	 */
> -	intel_uncore_write(&dev_priv->uncore, ILK_DPFC_CHICKEN, intel_uncore_read(&dev_priv->uncore, ILK_DPFC_CHICKEN) |
> -		   DPFC_NUKE_ON_ANY_MODIFICATION);
> +	intel_uncore_write(&dev_priv->uncore, ILK_DPFC_CHICKEN(INTEL_FBC_A),
> +			   intel_uncore_read(&dev_priv->uncore, ILK_DPFC_CHICKEN(INTEL_FBC_A)) |
> +			   DPFC_NUKE_ON_ANY_MODIFICATION);
>  }
>  
>  static void skl_init_clock_gating(struct drm_i915_private *dev_priv)
> @@ -7609,15 +7612,17 @@ static void skl_init_clock_gating(struct drm_i915_private *dev_priv)
>  	 * WaFbcNukeOnHostModify:skl
>  	 * Display WA #0873: skl
>  	 */
> -	intel_uncore_write(&dev_priv->uncore, ILK_DPFC_CHICKEN, intel_uncore_read(&dev_priv->uncore, ILK_DPFC_CHICKEN) |
> -		   DPFC_NUKE_ON_ANY_MODIFICATION);
> +	intel_uncore_write(&dev_priv->uncore, ILK_DPFC_CHICKEN(INTEL_FBC_A),
> +			   intel_uncore_read(&dev_priv->uncore, ILK_DPFC_CHICKEN(INTEL_FBC_A)) |
> +			   DPFC_NUKE_ON_ANY_MODIFICATION);
>  
>  	/*
>  	 * WaFbcHighMemBwCorruptionAvoidance:skl
>  	 * Display WA #0883: skl
>  	 */
> -	intel_uncore_write(&dev_priv->uncore, ILK_DPFC_CHICKEN, intel_uncore_read(&dev_priv->uncore, ILK_DPFC_CHICKEN) |
> -		   DPFC_DISABLE_DUMMY0);
> +	intel_uncore_write(&dev_priv->uncore, ILK_DPFC_CHICKEN(INTEL_FBC_A),
> +			   intel_uncore_read(&dev_priv->uncore, ILK_DPFC_CHICKEN(INTEL_FBC_A)) |
> +			   DPFC_DISABLE_DUMMY0);
>  }
>  
>  static void bdw_init_clock_gating(struct drm_i915_private *dev_priv)

Ville Syrjälä Dec. 14, 2021, 4:25 p.m. UTC | #2

On Mon, Dec 13, 2021 at 09:54:04PM +0200, Jani Nikula wrote:
> On Mon, 13 Dec 2021, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Parametrize ilk+ FBC register offsets based on the FBC instance.
> >
> > v2: More intel_ namespace (Jani)
> >
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Some questions below, apart from that,
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> > ---
> >  drivers/gpu/drm/i915/display/intel_fbc.c | 34 +++++++++++++-----------
> >  drivers/gpu/drm/i915/display/intel_fbc.h |  6 +++++
> >  drivers/gpu/drm/i915/i915_reg.h          | 34 ++++++++++++------------
> >  drivers/gpu/drm/i915/intel_pm.c          | 31 ++++++++++++---------
> >  4 files changed, 60 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> > index 8be01b93015f..112aafa72253 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > @@ -85,6 +85,8 @@ struct intel_fbc {
> >  	struct drm_mm_node compressed_fb;
> >  	struct drm_mm_node compressed_llb;
> >  
> > +	enum intel_fbc_id id;
> > +
> >  	u8 limit;
> >  
> >  	bool false_color;
> > @@ -454,10 +456,10 @@ static void ilk_fbc_activate(struct intel_fbc *fbc)
> >  	struct intel_fbc_state *fbc_state = &fbc->state;
> >  	struct drm_i915_private *i915 = fbc->i915;
> >  
> > -	intel_de_write(i915, ILK_DPFC_FENCE_YOFF,
> > +	intel_de_write(i915, ILK_DPFC_FENCE_YOFF(fbc->id),
> >  		       fbc_state->fence_y_offset);
> >  
> > -	intel_de_write(i915, ILK_DPFC_CONTROL,
> > +	intel_de_write(i915, ILK_DPFC_CONTROL(fbc->id),
> >  		       DPFC_CTL_EN | g4x_dpfc_ctl(fbc));
> >  }
> >  
> > @@ -467,28 +469,28 @@ static void ilk_fbc_deactivate(struct intel_fbc *fbc)
> >  	u32 dpfc_ctl;
> >  
> >  	/* Disable compression */
> > -	dpfc_ctl = intel_de_read(i915, ILK_DPFC_CONTROL);
> > +	dpfc_ctl = intel_de_read(i915, ILK_DPFC_CONTROL(fbc->id));
> >  	if (dpfc_ctl & DPFC_CTL_EN) {
> >  		dpfc_ctl &= ~DPFC_CTL_EN;
> > -		intel_de_write(i915, ILK_DPFC_CONTROL, dpfc_ctl);
> > +		intel_de_write(i915, ILK_DPFC_CONTROL(fbc->id), dpfc_ctl);
> >  	}
> >  }
> >  
> >  static bool ilk_fbc_is_active(struct intel_fbc *fbc)
> >  {
> > -	return intel_de_read(fbc->i915, ILK_DPFC_CONTROL) & DPFC_CTL_EN;
> > +	return intel_de_read(fbc->i915, ILK_DPFC_CONTROL(fbc->id)) & DPFC_CTL_EN;
> >  }
> >  
> >  static bool ilk_fbc_is_compressing(struct intel_fbc *fbc)
> >  {
> > -	return intel_de_read(fbc->i915, ILK_DPFC_STATUS) & DPFC_COMP_SEG_MASK;
> > +	return intel_de_read(fbc->i915, ILK_DPFC_STATUS(fbc->id)) & DPFC_COMP_SEG_MASK;
> >  }
> >  
> >  static void ilk_fbc_program_cfb(struct intel_fbc *fbc)
> >  {
> >  	struct drm_i915_private *i915 = fbc->i915;
> >  
> > -	intel_de_write(i915, ILK_DPFC_CB_BASE, fbc->compressed_fb.start);
> > +	intel_de_write(i915, ILK_DPFC_CB_BASE(fbc->id), fbc->compressed_fb.start);
> >  }
> >  
> >  static const struct intel_fbc_funcs ilk_fbc_funcs = {
> > @@ -524,8 +526,8 @@ static void snb_fbc_nuke(struct intel_fbc *fbc)
> >  {
> >  	struct drm_i915_private *i915 = fbc->i915;
> >  
> > -	intel_de_write(i915, MSG_FBC_REND_STATE, FBC_REND_NUKE);
> > -	intel_de_posting_read(i915, MSG_FBC_REND_STATE);
> > +	intel_de_write(i915, MSG_FBC_REND_STATE(fbc->id), FBC_REND_NUKE);
> > +	intel_de_posting_read(i915, MSG_FBC_REND_STATE(fbc->id));
> >  }
> >  
> >  static const struct intel_fbc_funcs snb_fbc_funcs = {
> > @@ -547,7 +549,7 @@ static void glk_fbc_program_cfb_stride(struct intel_fbc *fbc)
> >  		val |= FBC_STRIDE_OVERRIDE |
> >  			FBC_STRIDE(fbc_state->override_cfb_stride / fbc->limit);
> >  
> > -	intel_de_write(i915, GLK_FBC_STRIDE, val);
> > +	intel_de_write(i915, GLK_FBC_STRIDE(fbc->id), val);
> >  }
> >  
> >  static void skl_fbc_program_cfb_stride(struct intel_fbc *fbc)
> > @@ -598,19 +600,19 @@ static void ivb_fbc_activate(struct intel_fbc *fbc)
> >  	if (i915->ggtt.num_fences)
> >  		snb_fbc_program_fence(fbc);
> >  
> > -	intel_de_write(i915, ILK_DPFC_CONTROL,
> > +	intel_de_write(i915, ILK_DPFC_CONTROL(fbc->id),
> >  		       DPFC_CTL_EN | ivb_dpfc_ctl(fbc));
> >  }
> >  
> >  static bool ivb_fbc_is_compressing(struct intel_fbc *fbc)
> >  {
> > -	return intel_de_read(fbc->i915, ILK_DPFC_STATUS2) & DPFC_COMP_SEG_MASK_IVB;
> > +	return intel_de_read(fbc->i915, ILK_DPFC_STATUS2(fbc->id)) & DPFC_COMP_SEG_MASK_IVB;
> >  }
> >  
> >  static void ivb_fbc_set_false_color(struct intel_fbc *fbc,
> >  				    bool enable)
> >  {
> > -	intel_de_rmw(fbc->i915, ILK_DPFC_CONTROL,
> > +	intel_de_rmw(fbc->i915, ILK_DPFC_CONTROL(fbc->id),
> >  		     DPFC_CTL_FALSE_COLOR, enable ? DPFC_CTL_FALSE_COLOR : 0);
> >  }
> >  
> > @@ -1620,7 +1622,8 @@ void intel_fbc_add_plane(struct intel_fbc *fbc, struct intel_plane *plane)
> >  	fbc->possible_framebuffer_bits |= plane->frontbuffer_bit;
> >  }
> >  
> > -static struct intel_fbc *intel_fbc_create(struct drm_i915_private *i915)
> > +static struct intel_fbc *intel_fbc_create(struct drm_i915_private *i915,
> > +					  enum intel_fbc_id fbc_id)
> >  {
> >  	struct intel_fbc *fbc;
> >  
> > @@ -1628,6 +1631,7 @@ static struct intel_fbc *intel_fbc_create(struct drm_i915_private *i915)
> >  	if (!fbc)
> >  		return NULL;
> >  
> > +	fbc->id = fbc_id;
> >  	fbc->i915 = i915;
> >  	INIT_WORK(&fbc->underrun_work, intel_fbc_underrun_work_fn);
> >  	mutex_init(&fbc->lock);
> > @@ -1671,7 +1675,7 @@ void intel_fbc_init(struct drm_i915_private *i915)
> >  	if (!HAS_FBC(i915))
> >  		return;
> >  
> > -	fbc = intel_fbc_create(i915);
> > +	fbc = intel_fbc_create(i915, INTEL_FBC_A);
> >  	if (!fbc)
> >  		return;
> >  
> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.h b/drivers/gpu/drm/i915/display/intel_fbc.h
> > index 07ad0411fcc3..7b7631aec527 100644
> > --- a/drivers/gpu/drm/i915/display/intel_fbc.h
> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.h
> > @@ -17,6 +17,12 @@ struct intel_fbc;
> >  struct intel_plane;
> >  struct intel_plane_state;
> >  
> > +enum intel_fbc_id {
> > +	INTEL_FBC_A,
> > +
> > +	I915_MAX_FBCS,
> > +};
> > +
> >  int intel_fbc_atomic_check(struct intel_atomic_state *state);
> >  bool intel_fbc_pre_update(struct intel_atomic_state *state,
> >  			  struct intel_crtc *crtc);
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index d27ba273cc68..698a023e70f5 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3386,10 +3386,10 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
> >  #define FBC_LL_SIZE		(1536)
> >  
> >  /* Framebuffer compression for GM45+ */
> > -#define DPFC_CB_BASE		_MMIO(0x3200)
> > -#define ILK_DPFC_CB_BASE	_MMIO(0x43200)
> > -#define DPFC_CONTROL		_MMIO(0x3208)
> > -#define ILK_DPFC_CONTROL	_MMIO(0x43208)
> > +#define DPFC_CB_BASE			_MMIO(0x3200)
> > +#define ILK_DPFC_CB_BASE(fbc_id)	_MMIO_PIPE((fbc_id), 0x43200, 0x43240)
> > +#define DPFC_CONTROL			_MMIO(0x3208)
> > +#define ILK_DPFC_CONTROL(fbc_id)	_MMIO_PIPE((fbc_id), 0x43208, 0x43248)
> >  #define   DPFC_CTL_EN				REG_BIT(31)
> >  #define   DPFC_CTL_PLANE_MASK_G4X		REG_BIT(30) /* g4x-snb */
> >  #define   DPFC_CTL_PLANE_G4X(i9xx_plane)	REG_FIELD_PREP(DPFC_CTL_PLANE_MASK_G4X, (i9xx_plane))
> > @@ -3407,28 +3407,28 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
> >  #define   DPFC_CTL_LIMIT_4X			REG_FIELD_PREP(DPFC_CTL_LIMIT_MASK, 2)
> >  #define   DPFC_CTL_FENCENO_MASK			REG_GENMASK(3, 0)
> >  #define   DPFC_CTL_FENCENO(fence)		REG_FIELD_PREP(DPFC_CTL_FENCENO_MASK, (fence))
> > -#define DPFC_RECOMP_CTL		_MMIO(0x320c)
> > -#define ILK_DPFC_RECOMP_CTL	_MMIO(0x4320c)
> > +#define DPFC_RECOMP_CTL			_MMIO(0x320c)
> > +#define ILK_DPFC_RECOMP_CTL(fbc_id)	_MMIO_PIPE((fbc_id), 0x4320c, 0x4324c)
> 
> This is display 5 and 6 only, right?

Hmm. Looks like it may be removed in adl-p. But definitely still
present in tgl.

> Will there be a register instance
> for fbc_id > INTEL_FBC_A? Or is the parametrization just for
> completeness?

If it's not present in future hw then I guess it won't have a second
instance. But I might prefer parametrizing all of them anyway.

> 
> This one is only used in gvt, anyway. And that actually makes me wonder
> if this should be breaking the build. Does CI not have gvt enabled?

Hmm. I thought it was enabled in CI, but maybe not. I've often broken
gvt with register define changes but I've always caught it before
pushing. I think I have gvt enabled in my "make sure all commits build
before I push" test config, so maybe that's where I caught most of them.

Tomi, can we enable gvt in ci builds to make sure it at least still
builds?

> >  #define   DPFC_RECOMP_STALL_EN			REG_BIT(27)
> >  #define   DPFC_RECOMP_STALL_WM_MASK		REG_GENMASK(26, 16)
> >  #define   DPFC_RECOMP_TIMER_COUNT_MASK		REG_GENMASK(5, 0)
> > -#define DPFC_STATUS		_MMIO(0x3210)
> > -#define ILK_DPFC_STATUS		_MMIO(0x43210)
> > +#define DPFC_STATUS			_MMIO(0x3210)
> > +#define ILK_DPFC_STATUS(fbc_id)		_MMIO_PIPE((fbc_id), 0x43210, 0x43250)
> 
> Ditto, apart from the gvt part.

Looks like this too might be gone in adl-p.

> 
> >  #define   DPFC_INVAL_SEG_MASK			REG_GENMASK(26, 16)
> >  #define   DPFC_COMP_SEG_MASK			REG_GENMASK(10, 0)
> > -#define DPFC_STATUS2		_MMIO(0x3214)
> > -#define ILK_DPFC_STATUS2		_MMIO(0x43214)
> > +#define DPFC_STATUS2			_MMIO(0x3214)
> > +#define ILK_DPFC_STATUS2(fbc_id)	_MMIO_PIPE((fbc_id), 0x43214, 0x43254)
> >  #define   DPFC_COMP_SEG_MASK_IVB		REG_GENMASK(11, 0)
> > -#define DPFC_FENCE_YOFF		_MMIO(0x3218)
> > -#define ILK_DPFC_FENCE_YOFF	_MMIO(0x43218)
> > -#define DPFC_CHICKEN		_MMIO(0x3224)
> > -#define ILK_DPFC_CHICKEN	_MMIO(0x43224)
> > +#define DPFC_FENCE_YOFF			_MMIO(0x3218)
> > +#define ILK_DPFC_FENCE_YOFF(fbc_id)	_MMIO_PIPE((fbc_id), 0x43218, 0x43258)
> 
> Ditto.

I think this got nuked in ivb. It's not really used on snb either 
since snb introduced the system agent version of this register, but
I'm pretty sure the old register was still present in the snb hw.

Ville Syrjälä Dec. 14, 2021, 5:27 p.m. UTC | #3

On Tue, Dec 14, 2021 at 06:25:43PM +0200, Ville Syrjälä wrote:
> On Mon, Dec 13, 2021 at 09:54:04PM +0200, Jani Nikula wrote:
> > On Mon, 13 Dec 2021, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > 
> > This one is only used in gvt, anyway. And that actually makes me wonder
> > if this should be breaking the build. Does CI not have gvt enabled?
> 
> Hmm. I thought it was enabled in CI, but maybe not. I've often broken
> gvt with register define changes but I've always caught it before
> pushing. I think I have gvt enabled in my "make sure all commits build
> before I push" test config, so maybe that's where I caught most of them.
> 
> Tomi, can we enable gvt in ci builds to make sure it at least still
> builds?

Actually cc Tomi..

Jani Nikula Dec. 14, 2021, 6:34 p.m. UTC | #4

On Tue, 14 Dec 2021, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Mon, Dec 13, 2021 at 09:54:04PM +0200, Jani Nikula wrote:
>> On Mon, 13 Dec 2021, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
>> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> >
>> > Parametrize ilk+ FBC register offsets based on the FBC instance.
>> >
>> > v2: More intel_ namespace (Jani)
>> >
>> > Cc: Jani Nikula <jani.nikula@intel.com>
>> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> 
>> Some questions below, apart from that,
>> 
>> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>> 
>> > ---
>> >  drivers/gpu/drm/i915/display/intel_fbc.c | 34 +++++++++++++-----------
>> >  drivers/gpu/drm/i915/display/intel_fbc.h |  6 +++++
>> >  drivers/gpu/drm/i915/i915_reg.h          | 34 ++++++++++++------------
>> >  drivers/gpu/drm/i915/intel_pm.c          | 31 ++++++++++++---------
>> >  4 files changed, 60 insertions(+), 45 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
>> > index 8be01b93015f..112aafa72253 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
>> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
>> > @@ -85,6 +85,8 @@ struct intel_fbc {
>> >  	struct drm_mm_node compressed_fb;
>> >  	struct drm_mm_node compressed_llb;
>> >  
>> > +	enum intel_fbc_id id;
>> > +
>> >  	u8 limit;
>> >  
>> >  	bool false_color;
>> > @@ -454,10 +456,10 @@ static void ilk_fbc_activate(struct intel_fbc *fbc)
>> >  	struct intel_fbc_state *fbc_state = &fbc->state;
>> >  	struct drm_i915_private *i915 = fbc->i915;
>> >  
>> > -	intel_de_write(i915, ILK_DPFC_FENCE_YOFF,
>> > +	intel_de_write(i915, ILK_DPFC_FENCE_YOFF(fbc->id),
>> >  		       fbc_state->fence_y_offset);
>> >  
>> > -	intel_de_write(i915, ILK_DPFC_CONTROL,
>> > +	intel_de_write(i915, ILK_DPFC_CONTROL(fbc->id),
>> >  		       DPFC_CTL_EN | g4x_dpfc_ctl(fbc));
>> >  }
>> >  
>> > @@ -467,28 +469,28 @@ static void ilk_fbc_deactivate(struct intel_fbc *fbc)
>> >  	u32 dpfc_ctl;
>> >  
>> >  	/* Disable compression */
>> > -	dpfc_ctl = intel_de_read(i915, ILK_DPFC_CONTROL);
>> > +	dpfc_ctl = intel_de_read(i915, ILK_DPFC_CONTROL(fbc->id));
>> >  	if (dpfc_ctl & DPFC_CTL_EN) {
>> >  		dpfc_ctl &= ~DPFC_CTL_EN;
>> > -		intel_de_write(i915, ILK_DPFC_CONTROL, dpfc_ctl);
>> > +		intel_de_write(i915, ILK_DPFC_CONTROL(fbc->id), dpfc_ctl);
>> >  	}
>> >  }
>> >  
>> >  static bool ilk_fbc_is_active(struct intel_fbc *fbc)
>> >  {
>> > -	return intel_de_read(fbc->i915, ILK_DPFC_CONTROL) & DPFC_CTL_EN;
>> > +	return intel_de_read(fbc->i915, ILK_DPFC_CONTROL(fbc->id)) & DPFC_CTL_EN;
>> >  }
>> >  
>> >  static bool ilk_fbc_is_compressing(struct intel_fbc *fbc)
>> >  {
>> > -	return intel_de_read(fbc->i915, ILK_DPFC_STATUS) & DPFC_COMP_SEG_MASK;
>> > +	return intel_de_read(fbc->i915, ILK_DPFC_STATUS(fbc->id)) & DPFC_COMP_SEG_MASK;
>> >  }
>> >  
>> >  static void ilk_fbc_program_cfb(struct intel_fbc *fbc)
>> >  {
>> >  	struct drm_i915_private *i915 = fbc->i915;
>> >  
>> > -	intel_de_write(i915, ILK_DPFC_CB_BASE, fbc->compressed_fb.start);
>> > +	intel_de_write(i915, ILK_DPFC_CB_BASE(fbc->id), fbc->compressed_fb.start);
>> >  }
>> >  
>> >  static const struct intel_fbc_funcs ilk_fbc_funcs = {
>> > @@ -524,8 +526,8 @@ static void snb_fbc_nuke(struct intel_fbc *fbc)
>> >  {
>> >  	struct drm_i915_private *i915 = fbc->i915;
>> >  
>> > -	intel_de_write(i915, MSG_FBC_REND_STATE, FBC_REND_NUKE);
>> > -	intel_de_posting_read(i915, MSG_FBC_REND_STATE);
>> > +	intel_de_write(i915, MSG_FBC_REND_STATE(fbc->id), FBC_REND_NUKE);
>> > +	intel_de_posting_read(i915, MSG_FBC_REND_STATE(fbc->id));
>> >  }
>> >  
>> >  static const struct intel_fbc_funcs snb_fbc_funcs = {
>> > @@ -547,7 +549,7 @@ static void glk_fbc_program_cfb_stride(struct intel_fbc *fbc)
>> >  		val |= FBC_STRIDE_OVERRIDE |
>> >  			FBC_STRIDE(fbc_state->override_cfb_stride / fbc->limit);
>> >  
>> > -	intel_de_write(i915, GLK_FBC_STRIDE, val);
>> > +	intel_de_write(i915, GLK_FBC_STRIDE(fbc->id), val);
>> >  }
>> >  
>> >  static void skl_fbc_program_cfb_stride(struct intel_fbc *fbc)
>> > @@ -598,19 +600,19 @@ static void ivb_fbc_activate(struct intel_fbc *fbc)
>> >  	if (i915->ggtt.num_fences)
>> >  		snb_fbc_program_fence(fbc);
>> >  
>> > -	intel_de_write(i915, ILK_DPFC_CONTROL,
>> > +	intel_de_write(i915, ILK_DPFC_CONTROL(fbc->id),
>> >  		       DPFC_CTL_EN | ivb_dpfc_ctl(fbc));
>> >  }
>> >  
>> >  static bool ivb_fbc_is_compressing(struct intel_fbc *fbc)
>> >  {
>> > -	return intel_de_read(fbc->i915, ILK_DPFC_STATUS2) & DPFC_COMP_SEG_MASK_IVB;
>> > +	return intel_de_read(fbc->i915, ILK_DPFC_STATUS2(fbc->id)) & DPFC_COMP_SEG_MASK_IVB;
>> >  }
>> >  
>> >  static void ivb_fbc_set_false_color(struct intel_fbc *fbc,
>> >  				    bool enable)
>> >  {
>> > -	intel_de_rmw(fbc->i915, ILK_DPFC_CONTROL,
>> > +	intel_de_rmw(fbc->i915, ILK_DPFC_CONTROL(fbc->id),
>> >  		     DPFC_CTL_FALSE_COLOR, enable ? DPFC_CTL_FALSE_COLOR : 0);
>> >  }
>> >  
>> > @@ -1620,7 +1622,8 @@ void intel_fbc_add_plane(struct intel_fbc *fbc, struct intel_plane *plane)
>> >  	fbc->possible_framebuffer_bits |= plane->frontbuffer_bit;
>> >  }
>> >  
>> > -static struct intel_fbc *intel_fbc_create(struct drm_i915_private *i915)
>> > +static struct intel_fbc *intel_fbc_create(struct drm_i915_private *i915,
>> > +					  enum intel_fbc_id fbc_id)
>> >  {
>> >  	struct intel_fbc *fbc;
>> >  
>> > @@ -1628,6 +1631,7 @@ static struct intel_fbc *intel_fbc_create(struct drm_i915_private *i915)
>> >  	if (!fbc)
>> >  		return NULL;
>> >  
>> > +	fbc->id = fbc_id;
>> >  	fbc->i915 = i915;
>> >  	INIT_WORK(&fbc->underrun_work, intel_fbc_underrun_work_fn);
>> >  	mutex_init(&fbc->lock);
>> > @@ -1671,7 +1675,7 @@ void intel_fbc_init(struct drm_i915_private *i915)
>> >  	if (!HAS_FBC(i915))
>> >  		return;
>> >  
>> > -	fbc = intel_fbc_create(i915);
>> > +	fbc = intel_fbc_create(i915, INTEL_FBC_A);
>> >  	if (!fbc)
>> >  		return;
>> >  
>> > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.h b/drivers/gpu/drm/i915/display/intel_fbc.h
>> > index 07ad0411fcc3..7b7631aec527 100644
>> > --- a/drivers/gpu/drm/i915/display/intel_fbc.h
>> > +++ b/drivers/gpu/drm/i915/display/intel_fbc.h
>> > @@ -17,6 +17,12 @@ struct intel_fbc;
>> >  struct intel_plane;
>> >  struct intel_plane_state;
>> >  
>> > +enum intel_fbc_id {
>> > +	INTEL_FBC_A,
>> > +
>> > +	I915_MAX_FBCS,
>> > +};
>> > +
>> >  int intel_fbc_atomic_check(struct intel_atomic_state *state);
>> >  bool intel_fbc_pre_update(struct intel_atomic_state *state,
>> >  			  struct intel_crtc *crtc);
>> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> > index d27ba273cc68..698a023e70f5 100644
>> > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > @@ -3386,10 +3386,10 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>> >  #define FBC_LL_SIZE		(1536)
>> >  
>> >  /* Framebuffer compression for GM45+ */
>> > -#define DPFC_CB_BASE		_MMIO(0x3200)
>> > -#define ILK_DPFC_CB_BASE	_MMIO(0x43200)
>> > -#define DPFC_CONTROL		_MMIO(0x3208)
>> > -#define ILK_DPFC_CONTROL	_MMIO(0x43208)
>> > +#define DPFC_CB_BASE			_MMIO(0x3200)
>> > +#define ILK_DPFC_CB_BASE(fbc_id)	_MMIO_PIPE((fbc_id), 0x43200, 0x43240)
>> > +#define DPFC_CONTROL			_MMIO(0x3208)
>> > +#define ILK_DPFC_CONTROL(fbc_id)	_MMIO_PIPE((fbc_id), 0x43208, 0x43248)
>> >  #define   DPFC_CTL_EN				REG_BIT(31)
>> >  #define   DPFC_CTL_PLANE_MASK_G4X		REG_BIT(30) /* g4x-snb */
>> >  #define   DPFC_CTL_PLANE_G4X(i9xx_plane)	REG_FIELD_PREP(DPFC_CTL_PLANE_MASK_G4X, (i9xx_plane))
>> > @@ -3407,28 +3407,28 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
>> >  #define   DPFC_CTL_LIMIT_4X			REG_FIELD_PREP(DPFC_CTL_LIMIT_MASK, 2)
>> >  #define   DPFC_CTL_FENCENO_MASK			REG_GENMASK(3, 0)
>> >  #define   DPFC_CTL_FENCENO(fence)		REG_FIELD_PREP(DPFC_CTL_FENCENO_MASK, (fence))
>> > -#define DPFC_RECOMP_CTL		_MMIO(0x320c)
>> > -#define ILK_DPFC_RECOMP_CTL	_MMIO(0x4320c)
>> > +#define DPFC_RECOMP_CTL			_MMIO(0x320c)
>> > +#define ILK_DPFC_RECOMP_CTL(fbc_id)	_MMIO_PIPE((fbc_id), 0x4320c, 0x4324c)
>> 
>> This is display 5 and 6 only, right?
>
> Hmm. Looks like it may be removed in adl-p. But definitely still
> present in tgl.
>
>> Will there be a register instance
>> for fbc_id > INTEL_FBC_A? Or is the parametrization just for
>> completeness?
>
> If it's not present in future hw then I guess it won't have a second
> instance. But I might prefer parametrizing all of them anyway.

Up to you. R-b stands as long as we don't break gvt build.

BR,
Jani.


>
>> 
>> This one is only used in gvt, anyway. And that actually makes me wonder
>> if this should be breaking the build. Does CI not have gvt enabled?
>
> Hmm. I thought it was enabled in CI, but maybe not. I've often broken
> gvt with register define changes but I've always caught it before
> pushing. I think I have gvt enabled in my "make sure all commits build
> before I push" test config, so maybe that's where I caught most of them.
>
> Tomi, can we enable gvt in ci builds to make sure it at least still
> builds?
>
>> >  #define   DPFC_RECOMP_STALL_EN			REG_BIT(27)
>> >  #define   DPFC_RECOMP_STALL_WM_MASK		REG_GENMASK(26, 16)
>> >  #define   DPFC_RECOMP_TIMER_COUNT_MASK		REG_GENMASK(5, 0)
>> > -#define DPFC_STATUS		_MMIO(0x3210)
>> > -#define ILK_DPFC_STATUS		_MMIO(0x43210)
>> > +#define DPFC_STATUS			_MMIO(0x3210)
>> > +#define ILK_DPFC_STATUS(fbc_id)		_MMIO_PIPE((fbc_id), 0x43210, 0x43250)
>> 
>> Ditto, apart from the gvt part.
>
> Looks like this too might be gone in adl-p.
>
>> 
>> >  #define   DPFC_INVAL_SEG_MASK			REG_GENMASK(26, 16)
>> >  #define   DPFC_COMP_SEG_MASK			REG_GENMASK(10, 0)
>> > -#define DPFC_STATUS2		_MMIO(0x3214)
>> > -#define ILK_DPFC_STATUS2		_MMIO(0x43214)
>> > +#define DPFC_STATUS2			_MMIO(0x3214)
>> > +#define ILK_DPFC_STATUS2(fbc_id)	_MMIO_PIPE((fbc_id), 0x43214, 0x43254)
>> >  #define   DPFC_COMP_SEG_MASK_IVB		REG_GENMASK(11, 0)
>> > -#define DPFC_FENCE_YOFF		_MMIO(0x3218)
>> > -#define ILK_DPFC_FENCE_YOFF	_MMIO(0x43218)
>> > -#define DPFC_CHICKEN		_MMIO(0x3224)
>> > -#define ILK_DPFC_CHICKEN	_MMIO(0x43224)
>> > +#define DPFC_FENCE_YOFF			_MMIO(0x3218)
>> > +#define ILK_DPFC_FENCE_YOFF(fbc_id)	_MMIO_PIPE((fbc_id), 0x43218, 0x43258)
>> 
>> Ditto.
>
> I think this got nuked in ivb. It's not really used on snb either 
> since snb introduced the system agent version of this register, but
> I'm pretty sure the old register was still present in the snb hw.

Sarvela, Tomi P Dec. 15, 2021, 9:05 a.m. UTC | #5

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> On Tue, Dec 14, 2021 at 06:25:43PM +0200, Ville Syrjälä wrote:
> > On Mon, Dec 13, 2021 at 09:54:04PM +0200, Jani Nikula wrote:
> > > On Mon, 13 Dec 2021, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > >
> > > This one is only used in gvt, anyway. And that actually makes me wonder
> > > if this should be breaking the build. Does CI not have gvt enabled?
> >
> > Hmm. I thought it was enabled in CI, but maybe not. I've often broken
> > gvt with register define changes but I've always caught it before
> > pushing. I think I have gvt enabled in my "make sure all commits build
> > before I push" test config, so maybe that's where I caught most of them.
> >
> > Tomi, can we enable gvt in ci builds to make sure it at least still
> > builds?
> 
> Actually cc Tomi..

GVT-d is enabled and tested by fi-bdw-gvtdvm.

Tomi

Ville Syrjälä Dec. 15, 2021, 1:25 p.m. UTC | #6

On Wed, Dec 15, 2021 at 09:05:03AM +0000, Sarvela, Tomi P wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > On Tue, Dec 14, 2021 at 06:25:43PM +0200, Ville Syrjälä wrote:
> > > On Mon, Dec 13, 2021 at 09:54:04PM +0200, Jani Nikula wrote:
> > > > On Mon, 13 Dec 2021, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > > >
> > > > This one is only used in gvt, anyway. And that actually makes me wonder
> > > > if this should be breaking the build. Does CI not have gvt enabled?
> > >
> > > Hmm. I thought it was enabled in CI, but maybe not. I've often broken
> > > gvt with register define changes but I've always caught it before
> > > pushing. I think I have gvt enabled in my "make sure all commits build
> > > before I push" test config, so maybe that's where I caught most of them.
> > >
> > > Tomi, can we enable gvt in ci builds to make sure it at least still
> > > builds?
> > 
> > Actually cc Tomi..
> 
> GVT-d is enabled and tested by fi-bdw-gvtdvm.

We're talking about the other gvt (whatever it was called), ie.
CONFIG_DRM_I915_GVT.

Sarvela, Tomi P Dec. 15, 2021, 1:30 p.m. UTC | #7

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> On Wed, Dec 15, 2021 at 09:05:03AM +0000, Sarvela, Tomi P wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > On Tue, Dec 14, 2021 at 06:25:43PM +0200, Ville Syrjälä wrote:
> > > > On Mon, Dec 13, 2021 at 09:54:04PM +0200, Jani Nikula wrote:
> > > > > On Mon, 13 Dec 2021, Ville Syrjala <ville.syrjala@linux.intel.com>
> wrote:
> > > > >
> > > > > This one is only used in gvt, anyway. And that actually makes me
> wonder
> > > > > if this should be breaking the build. Does CI not have gvt enabled?
> > > >
> > > > Hmm. I thought it was enabled in CI, but maybe not. I've often broken
> > > > gvt with register define changes but I've always caught it before
> > > > pushing. I think I have gvt enabled in my "make sure all commits build
> > > > before I push" test config, so maybe that's where I caught most of
> them.
> > > >
> > > > Tomi, can we enable gvt in ci builds to make sure it at least still
> > > > builds?
> > >
> > > Actually cc Tomi..
> >
> > GVT-d is enabled and tested by fi-bdw-gvtdvm.
> 
> We're talking about the other gvt (whatever it was called), ie.
> CONFIG_DRM_I915_GVT.

This kconfig entry doesn't exist in default CI kconfig, even as 'is not set'
placeholder:
https://gitlab.freedesktop.org/gfx-ci/i915-infra/-/blob/master/kconfig/debug

If the config entry is exact, I'll probably need to upgrade the default config
from 5.13 and add it with requirements. Not today, but maybe soon.

Tomi

Sarvela, Tomi P Dec. 15, 2021, 2:11 p.m. UTC | #8

> From: Sarvela, Tomi P
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > On Wed, Dec 15, 2021 at 09:05:03AM +0000, Sarvela, Tomi P wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > >
> > > > On Tue, Dec 14, 2021 at 06:25:43PM +0200, Ville Syrjälä wrote:
> > > > > On Mon, Dec 13, 2021 at 09:54:04PM +0200, Jani Nikula wrote:
> > > > > > On Mon, 13 Dec 2021, Ville Syrjala <ville.syrjala@linux.intel.com>
> > wrote:
> > > > > >
> > > > > > This one is only used in gvt, anyway. And that actually makes me
> > wonder
> > > > > > if this should be breaking the build. Does CI not have gvt enabled?
> > > > >
> > > > > Hmm. I thought it was enabled in CI, but maybe not. I've often broken
> > > > > gvt with register define changes but I've always caught it before
> > > > > pushing. I think I have gvt enabled in my "make sure all commits build
> > > > > before I push" test config, so maybe that's where I caught most of
> > them.
> > > > >
> > > > > Tomi, can we enable gvt in ci builds to make sure it at least still
> > > > > builds?
> > > >
> > > > Actually cc Tomi..
> > >
> > > GVT-d is enabled and tested by fi-bdw-gvtdvm.
> >
> > We're talking about the other gvt (whatever it was called), ie.
> > CONFIG_DRM_I915_GVT.
> 
> This kconfig entry doesn't exist in default CI kconfig, even as 'is not set'
> placeholder:
> https://gitlab.freedesktop.org/gfx-ci/i915-infra/-
> /blob/master/kconfig/debug
> 
> If the config entry is exact, I'll probably need to upgrade the default config
> from 5.13 and add it with requirements. Not today, but maybe soon.

kconfigs debug, debug-kasan and debug-gcov have been updated to v5.15
with 'make olddefconfig', and CONFIG_DRM_I915_GVT=y has been set.

First CI_DRM to use this kconfig will be CI_DRM_11005.

Tomi

[v2,1/4] drm/i915/fbc: Parametrize FBC register offsets

Commit Message

Comments

Patch