Re: [Intel-gfx] [PATCH v5 05/22] drm/i915/dg1: Wait for pcode/uncore handshake at startup

["Souza, Jose" <jose.souza@intel.com>]

On Mon, 2020-08-24 at 12:24 -0700, Lucas De Marchi wrote:
> On Mon, Aug 03, 2020 at 04:24:17PM -0700, Jose Souza wrote:
> > On Fri, 2020-07-24 at 14:39 -0700, Lucas De Marchi wrote:
> > > From: Matt Roper <
> > > matthew.d.roper@intel.com
> > > 
> > > 
> > > DG1 does some additional pcode/uncore handshaking at
> > > boot time; this handshaking must complete before various other pcode
> > > commands are effective and before general work is submitted to the GPU.
> > > We need to poll a new pcode mailbox during startup until it reports that
> > > this handshaking is complete.
> > > 
> > > The bspec doesn't give guidance on how long we may need to wait for this
> > > handshaking to complete.  For now, let's just set a really long timeout;
> > > if we still don't get a completion status by the end of that timeout,
> > > we'll just continue on and hope for the best.
> > > 
> > > Bspec: 52065
> > > Cc: Clinton Taylor <
> > > Clinton.A.Taylor@intel.com
> > > 
> > > 
> > > Cc: Ville Syrjälä <
> > > ville.syrjala@linux.intel.com
> > > 
> > > 
> > > Cc: Radhakrishna Sripada <
> > > radhakrishna.sripada@intel.com
> > > 
> > > 
> > > Signed-off-by: Matt Roper <
> > > matthew.d.roper@intel.com
> > > 
> > > 
> > > Signed-off-by: Lucas De Marchi <
> > > lucas.demarchi@intel.com
> > > 
> > > 
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.c       |  3 +++
> > >  drivers/gpu/drm/i915/i915_reg.h       |  3 +++
> > >  drivers/gpu/drm/i915/intel_sideband.c | 15 +++++++++++++++
> > >  drivers/gpu/drm/i915/intel_sideband.h |  2 ++
> > >  4 files changed, 23 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index 5fd5af4bc855..5473bfe9126c 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -85,6 +85,7 @@
> > >  #include "intel_gvt.h"
> > >  #include "intel_memory_region.h"
> > >  #include "intel_pm.h"
> > > +#include "intel_sideband.h"
> > >  #include "vlv_suspend.h"
> > > 
> > >  static struct drm_driver driver;
> > > @@ -737,6 +738,8 @@ static int i915_driver_hw_probe(struct drm_i915_private *dev_priv)
> > >  	 */
> > >  	intel_dram_detect(dev_priv);
> > > 
> > > +	intel_pcode_init(dev_priv);
> > > +
> > >  	intel_bw_init_hw(dev_priv);
> > > 
> > >  	return 0;
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index a0d31f3bf634..3767b32127da 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -9245,6 +9245,9 @@ enum {
> > >  #define     GEN9_SAGV_DISABLE			0x0
> > >  #define     GEN9_SAGV_IS_DISABLED		0x1
> > >  #define     GEN9_SAGV_ENABLE			0x3
> > > +#define   DG1_PCODE_STATUS			0x7E
> > > +#define     DG1_CHECK_UNCORE_INIT_STATUS	0x0
> > > +#define     DG1_UNCORE_INIT_COMPLETE		0x1
> > 
> > With s/DG1_CHECK_UNCORE_INIT_STATUS/DG1_CHECK_UNCORE_INIT_STATUS_COMPLETE or something similar that makes easy to understand that 0x1 is the response
> > of the DG1_CHECK_UNCORE_INIT_STATUS sub-command.
> 
> checking all the other users of skl_pcode_request() I don't see a
> pattern there. Examples:
> 
> ret = skl_pcode_request(dev_priv, SKL_PCODE_CDCLK_CONTROL,
>                          SKL_CDCLK_PREPARE_FOR_CHANGE,     
>                          SKL_CDCLK_READY_FOR_CHANGE,       
>                          SKL_CDCLK_READY_FOR_CHANGE, 3);   
> 
> ret = skl_pcode_request(dev_priv, GEN9_PCODE_SAGV_CONTROL,            
>                          GEN9_SAGV_DISABLE,                            
>                          GEN9_SAGV_IS_DISABLED, GEN9_SAGV_IS_DISABLED, 
>                          1);                                           
> 
> Giveng the current uses, I'd rather rename like:
> 
> +#define   DG1_PCODE_STATUS			0x7E
> +#define     DG1_UNCORE_GET_INIT_STATUS		0x0
> +#define     DG1_UNCORE_INIT_STATUS_COMPLETE	0x1
> 
> 
> > Reviewed-by: José Roberto de Souza <
> > jose.souza@intel.com
> > >
> 
> does that still stands with the rename above?

LGTM, keep it please.

> 
> thanks
> Lucas De Marchi
> 
> > 
> > >  #define GEN12_PCODE_READ_SAGV_BLOCK_TIME_US	0x23
> > >  #define GEN6_PCODE_DATA				_MMIO(0x138128)
> > >  #define   GEN6_PCODE_FREQ_IA_RATIO_SHIFT	8
> > > diff --git a/drivers/gpu/drm/i915/intel_sideband.c b/drivers/gpu/drm/i915/intel_sideband.c
> > > index 916ccd1c0e96..8b093525240d 100644
> > > --- a/drivers/gpu/drm/i915/intel_sideband.c
> > > +++ b/drivers/gpu/drm/i915/intel_sideband.c
> > > @@ -543,3 +543,18 @@ int skl_pcode_request(struct drm_i915_private *i915, u32 mbox, u32 request,
> > >  	return ret ? ret : status;
> > >  #undef COND
> > >  }
> > > +
> > > +void intel_pcode_init(struct drm_i915_private *i915)
> > > +{
> > > +	int ret;
> > > +
> > > +	if (!IS_DGFX(i915))
> > > +		return;
> > > +
> > > +	ret = skl_pcode_request(i915, DG1_PCODE_STATUS,
> > > +				DG1_CHECK_UNCORE_INIT_STATUS,
> > > +				DG1_UNCORE_INIT_COMPLETE,
> > > +				DG1_UNCORE_INIT_COMPLETE, 50);
> > > +	if (ret)
> > > +		drm_err(&i915->drm, "Pcode did not report uncore initialization completion!\n");
> > > +}
> > > diff --git a/drivers/gpu/drm/i915/intel_sideband.h b/drivers/gpu/drm/i915/intel_sideband.h
> > > index 7fb95745a444..094c7b19c5d4 100644
> > > --- a/drivers/gpu/drm/i915/intel_sideband.h
> > > +++ b/drivers/gpu/drm/i915/intel_sideband.h
> > > @@ -138,4 +138,6 @@ int sandybridge_pcode_write_timeout(struct drm_i915_private *i915, u32 mbox,
> > >  int skl_pcode_request(struct drm_i915_private *i915, u32 mbox, u32 request,
> > >  		      u32 reply_mask, u32 reply, int timeout_base_ms);
> > > 
> > > +void intel_pcode_init(struct drm_i915_private *i915);
> > > +
> > >  #endif /* _INTEL_SIDEBAND_H */
> > > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx