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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Dec 13, 2021 at 09:54:04PM +0200, Jani Nikula wrote:
> On Mon, 13 Dec 2021, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > Parametrize ilk+ FBC register offsets based on the FBC instance.
> >
> > v2: More intel_ namespace (Jani)
> >
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> 
> Some questions below, apart from that,
> 
> Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> 
> > ---
> >  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ä
Intel



[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux