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 Tue, 14 Dec 2021, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> 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.

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.

-- 
Jani Nikula, Intel Open Source Graphics Center




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

  Powered by Linux