Re: [Intel-gfx] [PATCH 12/13] drm/i915/dsb: Define more DSB registers

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

On Wed, Jan 11, 2023 at 03:59:14PM +0000, Manna, Animesh wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville
> > Syrjala
> > Sent: Friday, December 16, 2022 6:08 AM
> > To: intel-gfx@lists.freedesktop.org
> > Subject: [Intel-gfx] [PATCH 12/13] drm/i915/dsb: Define more DSB registers
> > 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Add definitions for more DSB registers. Less annoying spec trawling when
> > working on the DSB code.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h | 50 +++++++++++++++++++++++++++++++-
> > -
> >  1 file changed, 48 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h index ed989e749635..3b0d07880c30
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -8103,8 +8103,54 @@ enum skl_power_gate {
> >  #define DSB_HEAD(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) +
> > 0x0)
> >  #define DSB_TAIL(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) +
> > 0x4)
> >  #define DSB_CTRL(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) +
> > 0x8)
> > -#define   DSB_ENABLE			(1 << 31)
> > -#define   DSB_STATUS_BUSY		(1 << 0)
> > +#define   DSB_ENABLE			REG_BIT(31)
> > +#define   DSB_BUF_REITERATE		REG_BIT(29)
> > +#define   DSB_WAIT_FOR_VBLANK		REG_BIT(28)
> > +#define   DSB_WAIT_FOR_LINE_IN		REG_BIT(27)
> > +#define   DSB_HALT			REG_BIT(16)
> > +#define   DSB_NON_POSTED		REG_BIT(8)
> > +#define   DSB_STATUS_BUSY		REG_BIT(0)
> > +#define DSB_MMIOCTRL(pipe, id)
> > 	_MMIO(DSBSL_INSTANCE(pipe, id) + 0xc)
> > +#define   DSB_MMIO_DEAD_CLOCKS_ENABLE	REG_BIT(31)
> 
> As per bpsec the programming of the above bit may not be needed for latest platforms as it is taken care in h/w. Do we have any plan to use it for older platform. 
> 
> > +#define   DSB_MMIO_DEAD_CLOCKS_COUNT_MASK	REG_GENMASK(15, 8)
> > +#define   DSB_MMIO_DEAD_CLOCKS_COUNT(x)
> > 	REG_FIELD_PREP(DSB_MMIO_DEAD_CLOCK_COUNT_MASK, (x))
> > +#define   DSB_MMIO_CYCLES_MASK		REG_GENMASK(7, 0)
> > +#define   DSB_MMIO_CYCLES(x)
> > 	REG_FIELD_PREP(DSB_MMIO_CYCLES_MASK, (x))
> > +#define DSB_POLLFUNC(pipe, id)
> > 	_MMIO(DSBSL_INSTANCE(pipe, id) + 0x10)> +#define   DSB_POLL_ENABLE		REG_BIT(31)
> > +#define   DSB_POLL_WAIT_MASK		REG_GENMASK(30, 23)
> > +#define   DSB_POLL_WAIT(x)
> > 	REG_FIELD_PREP(DSB_POLL_WAIT_MASK, (x)) /* usec */
> > +#define   DSB_POLL_COUNT_MASK		REG_GENMASK(22, 15)
> > +#define   DSB_POLL_COUNT(x)
> > 	REG_FIELD_PREP(DSB_POLL_COUNT_MASK, (x))
> > +#define DSB_DEBUG(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) +
> > 0x14)
> 
> I can not see any usage of this register. All bits are mentioned spare in bpsec.
> 
> > +#define DSB_POLLMASK(pipe, id)
> > 	_MMIO(DSBSL_INSTANCE(pipe, id) + 0x1c)
> > +#define DSB_STATUS(pipe, id)		_MMIO(DSBSL_INSTANCE(pipe, id) +
> > 0x24)
> 
> Do we have any plan to check the dsb engine internal details in kernel? Not sure if DSB_STATUS will be needed any time until there is a need to debug the h/w if it is malfunctioning. 
> 
> > +#define DSB_INTERRUPT(pipe, id)
> > 	_MMIO(DSBSL_INSTANCE(pipe, id) + 0x28)
> > +#define   DSB_ATS_FAULT_INT_EN		REG_BIT(20)
> > +#define   DSB_GTT_FAULT_INT_EN		REG_BIT(19)
> > +#define   DSB_RSPTIMEOUT_INT_EN		REG_BIT(18)
> > +#define   DSB_POLL_ERR_INT_EN		REG_BIT(17)
> > +#define   DSB_PROG_INT_EN		REG_BIT(16)
> > +#define   DSB_ATS_FAULT_INT_STATUS	REG_BIT(4)
> > +#define   DSB_GTT_FAULT_INT_STATUS	REG_BIT(3)
> > +#define   DSB_RSPTIMEOUT_INT_STATUS	REG_BIT(2)
> > +#define   DSB_POLL_ERR_INT_STATUS	REG_BIT(1)
> > +#define   DSB_PROG_INT_STATUS		REG_BIT(0)
> > +#define DSB_CURRENT_HEAD(pipe, id)
> > 	_MMIO(DSBSL_INSTANCE(pipe, id) + 0x2c)
> > +#define DSB_RM_TIMEOUT(pipe, id)	_MMIO(DSBSL_INSTANCE(pipe, id) +
> > 0x30)
> > +#define   DSB_RM_CLAIM_TIMEOUT		REG_BIT(31)
> > +#define   DSB_RM_READY_TIMEOUT		REG_BIT(30)
> > +#define   DSB_RM_CLAIM_TIMEOUT_COUNT_MASK	REG_GENMASK(23,
> > 16)
> > +#define   DSB_RM_CLAIM_TIMEOUT_COUNT(x)
> > 	REG_FIELD_PREP(DSB_RM_CLAIM_TIMEOUT_COUNT_MASK, (x)) /*
> > clocks */
> > +#define   DSB_RM_READY_TIMEOUT_VALUE_MASK	REG_GENMASK(15, 0)
> > +#define   DSB_RM_READY_TIMEOUT_VALUE(x)
> > 	REG_FIELD_PREP(DSB_RM_READY_TIMEOUT_VALUE, (x)) /* usec */
> > +#define DSB_RMTIMEOUTREG_CAPTURE(pipe, id)
> > 	_MMIO(DSBSL_INSTANCE(pipe, id) + 0x34)
> > +#define DSB_PMCTRL(pipe, id)
> > 	_MMIO(DSBSL_INSTANCE(pipe, id) + 0x38)
> > +#define DSB_PMCTRL_2(pipe, id)
> > 	_MMIO(DSBSL_INSTANCE(pipe, id) + 0x3c)
> > +#define DSB_PF_LN_LOWER(pipe, id)	_MMIO(DSBSL_INSTANCE(pipe, id) +
> > 0x40)
> > +#define DSB_PF_LN_UPPER(pipe, id)	_MMIO(DSBSL_INSTANCE(pipe, id) +
> > 0x44)
> > +#define DSB_BUFRPT_CNT(pipe, id)	_MMIO(DSBSL_INSTANCE(pipe, id) +
> > 0x48)
> > +#define DSB_CHICKEN(pipe, id)
> > 	_MMIO(DSBSL_INSTANCE(pipe, id) + 0xf0)
> 
> I can understand instead of referring bspec all the above DSB registers are added without its usage and future patch will use it.
> Can we add a FIXME tag to remove unwanted DSB register definition once we achieve our target.

We're not going to remove them. I'm not going to be wasting my
time trawling bspec every time I need to poke at some DSB register.

-- 
Ville Syrjälä
Intel