Re: [Intel-gfx] [PATCH 1/1] drm/i915/mtl: Enable Idle Messaging for GSC CS

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Nov 18, 2022 at 09:32:37AM -0500, Rodrigo Vivi wrote:
> On Fri, Nov 18, 2022 at 09:35:41AM +0530, Nilawar, Badal wrote:
> > 
> > 
> > On 18-11-2022 03:44, Rodrigo Vivi wrote:
> > > On Tue, Nov 15, 2022 at 07:14:40PM +0530, Badal Nilawar wrote:
> > > > From: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
> > > > 
> > > > By defaut idle mesaging is disabled for GSC CS so to unblock RC6
> > > > entry on media tile idle messaging need to be enabled.
> > > > 
> > > > v2:
> > > >   - Fix review comments (Vinay)
> > > >   - Set GSC idle hysterisis to 5 us (Badal)
> > > > 
> > > > Bspec: 71496
> > > > 
> > > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > > > Signed-off-by: Vinay Belgaumkar <vinay.belgaumkar@intel.com>
> > > > Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/i915/gt/intel_engine_pm.c | 18 ++++++++++++++++++
> > > >   drivers/gpu/drm/i915/gt/intel_gt_regs.h   |  4 ++++
> > > >   2 files changed, 22 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> > > > index b0a4a2dbe3ee..5522885b2db0 100644
> > > > --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> > > > +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
> > > > @@ -15,6 +15,22 @@
> > > >   #include "intel_rc6.h"
> > > >   #include "intel_ring.h"
> > > >   #include "shmem_utils.h"
> > > > +#include "intel_gt_regs.h"
> > > > +
> > > > +static void intel_gsc_idle_msg_enable(struct intel_engine_cs *engine)
> > > > +{
> > > > +	struct drm_i915_private *i915 = engine->i915;
> > > > +
> > > > +	if (IS_METEORLAKE(i915) && engine->id == GSC0) {
> > > > +		intel_uncore_write(engine->gt->uncore,
> > > > +				   RC_PSMI_CTRL_GSCCS,
> > > > +				   _MASKED_BIT_DISABLE(IDLE_MSG_DISABLE));
> > > 
> > > disable the disable? shouldn't be enable the disable?
> > > 1 = disable, no?
> 
> doh! the function here is enable so the disable of disable is the
> right thing for this bit... Sorry for that.
> 
> 
> > > 
> > > > +		/* 5 us hysterisis */
> > > 
> > > could you please mention here in the comment that 0xA = 5 us per spec?
> > > I got confused again even though you had explained already...
> > Sure I will add the comment "0xA=5 us as per spec"
> 
> Thank you
> 
> > > 
> > > BTW, how reliable that spec is? Because according to that same line
> > > we should be setting the bit 16, not the bit 0 in the previous reg!
> > Bit 16 is mask bit.
> 
> Okay, and we need to clear the bit 0. It makes sense. However the spec
> seems to ask us to set the mask, but we are not. Should we?
> 
> Also from the register page:
> "Must be set to modify corresponding bit in Bits 15:0. (All implemented bits)"
> 
> So it looks to me that we do need to set the bit16 to ensure that the
> clear of the bit 0 is valid, otherwise this is a bogus call.

oh, I hate these macros hiding things up... bit 16 is there on that
_MASKED_BIT_DISABLED as you showed me offline...

So, with the comment to the 5usec added, feel free to use:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


> 
> > Bit 0 need to be cleared to enable Idle messaging.
> > Bit[0] = 1 Disable Idle Messaging / 0 Enable Idle Messaging.
> > 
> > Regards,
> > Badal
> > > 
> > > > +		intel_uncore_write(engine->gt->uncore,
> > > > +				   PWRCTX_MAXCNT_GSCCS,
> > > > +				   0xA);
> > > > +	}
> > > > +}
> > > >   static void dbg_poison_ce(struct intel_context *ce)
> > > >   {
> > > > @@ -275,6 +291,8 @@ void intel_engine_init__pm(struct intel_engine_cs *engine)
> > > >   	intel_wakeref_init(&engine->wakeref, rpm, &wf_ops);
> > > >   	intel_engine_init_heartbeat(engine);
> > > > +
> > > > +	intel_gsc_idle_msg_enable(engine);
> > > >   }
> > > >   /**
> > > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > > index 07031e03f80c..20472eb15364 100644
> > > > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > > > @@ -913,6 +913,10 @@
> > > >   #define  MSG_IDLE_FW_MASK	REG_GENMASK(13, 9)
> > > >   #define  MSG_IDLE_FW_SHIFT	9
> > > > +#define	RC_PSMI_CTRL_GSCCS	_MMIO(0x11a050)
> > > > +#define	  IDLE_MSG_DISABLE	BIT(0)
> > > > +#define PWRCTX_MAXCNT_GSCCS	_MMIO(0x11a054)
> > > > +
> > > >   #define FORCEWAKE_MEDIA_GEN9			_MMIO(0xa270)
> > > >   #define FORCEWAKE_RENDER_GEN9			_MMIO(0xa278)
> > > > -- 
> > > > 2.25.1
> > > >