| Message ID | 20200724213918.27424-6-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce DG1 | expand |
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. Reviewed-by: José Roberto de Souza <jose.souza@intel.com> > #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 */ >
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? 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 */ >>
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 */ > > >
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 #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 */