| Message ID | 20170126125030.30815-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jan 26, 2017 at 12:50:28PM +0000, Chris Wilson wrote: > As we add new generations, we should keep detecting new system > development platforms that were temporarily enabled (via > "i915.alpha_support") and now superseded by production systems. To make > it easier to add more platforms, split the if into a series of logical > operations. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: "Zanoni, Paulo R" <paulo.r.zanoni@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 008fc1e62a69..c2c210b0f47f 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -782,8 +782,12 @@ static void i915_workqueues_cleanup(struct drm_i915_private *dev_priv) > */ > static void intel_detect_preproduction_hw(struct drm_i915_private *dev_priv) > { > - if (IS_HSW_EARLY_SDV(dev_priv) || > - IS_SKL_REVID(dev_priv, 0, SKL_REVID_F0)) > + bool sdv = false; Presumably not all SDVs use pre-production silicon, so the name of this variable feels a bit off. Also since it doesn't actually check anything about the actual board we can't even say that it's an actual SDV, so the HSW_EARLY_SDV() macro seems somewhat poorly named to me as well. > + > + sdv |= IS_HSW_EARLY_SDV(dev_priv); > + sdv |= IS_SKL_REVID(dev_priv, 0, SKL_REVID_F0); > + > + if (sdv) > DRM_ERROR("This is a pre-production stepping. " > "It may not be fully functional.\n"); > } > -- > 2.11.0
Em Qui, 2017-01-26 às 15:02 +0200, Ville Syrjälä escreveu: > On Thu, Jan 26, 2017 at 12:50:28PM +0000, Chris Wilson wrote: > > > > As we add new generations, we should keep detecting new system > > development platforms that were temporarily enabled (via > > "i915.alpha_support") and now superseded by production systems. To > > make > > it easier to add more platforms, split the if into a series of > > logical > > operations. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: "Zanoni, Paulo R" <paulo.r.zanoni@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Jani Nikula <jani.nikula@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > > b/drivers/gpu/drm/i915/i915_drv.c > > index 008fc1e62a69..c2c210b0f47f 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -782,8 +782,12 @@ static void i915_workqueues_cleanup(struct > > drm_i915_private *dev_priv) > > */ > > static void intel_detect_preproduction_hw(struct drm_i915_private > > *dev_priv) > > { > > - if (IS_HSW_EARLY_SDV(dev_priv) || > > - IS_SKL_REVID(dev_priv, 0, SKL_REVID_F0)) > > + bool sdv = false; > > Presumably not all SDVs use pre-production silicon, so the name of > this variable feels a bit off. Also since it doesn't actually check > anything about the actual board we can't even say that it's an > actual SDV, so the HSW_EARLY_SDV() macro seems somewhat poorly named > to me as well. The HSW case is different since those SDVs even have different PCI IDs too, not only REV IDs, and "_SDV" is how the PCI IDs are named. I wonder if we should just remove them from our list of PCI IDs. Could these IDs be repurposed in the future? > > > > > + > > + sdv |= IS_HSW_EARLY_SDV(dev_priv); > > + sdv |= IS_SKL_REVID(dev_priv, 0, SKL_REVID_F0); > > + > > + if (sdv) > > DRM_ERROR("This is a pre-production stepping. " > > "It may not be fully functional.\n"); > > } > > -- > > 2.11.0 >
On Thu, Jan 26, 2017 at 03:02:14PM +0200, Ville Syrjälä wrote: > On Thu, Jan 26, 2017 at 12:50:28PM +0000, Chris Wilson wrote: > > As we add new generations, we should keep detecting new system > > development platforms that were temporarily enabled (via > > "i915.alpha_support") and now superseded by production systems. To make > > it easier to add more platforms, split the if into a series of logical > > operations. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: "Zanoni, Paulo R" <paulo.r.zanoni@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Jani Nikula <jani.nikula@intel.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > index 008fc1e62a69..c2c210b0f47f 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -782,8 +782,12 @@ static void i915_workqueues_cleanup(struct drm_i915_private *dev_priv) > > */ > > static void intel_detect_preproduction_hw(struct drm_i915_private *dev_priv) > > { > > - if (IS_HSW_EARLY_SDV(dev_priv) || > > - IS_SKL_REVID(dev_priv, 0, SKL_REVID_F0)) > > + bool sdv = false; > > Presumably not all SDVs use pre-production silicon, so the name of > this variable feels a bit off. Also since it doesn't actually check > anything about the actual board we can't even say that it's an > actual SDV, so the HSW_EARLY_SDV() macro seems somewhat poorly named > to me as well. I started with bool preproduction; but that eats too many characters. Ideas for something short and snappy? bool alpha_hw? bool alpha? -Chris >
On Thu, 26 Jan 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: > As we add new generations, we should keep detecting new system > development platforms that were temporarily enabled (via > "i915.alpha_support") and now superseded by production systems. To make > it easier to add more platforms, split the if into a series of logical > operations. Please be careful not to conflate preproduction/early hardware with alpha support in the driver. They are two distinct things. The former is purely about hardware. The latter is purely about software. (Okay, there's the dependency that we're probably unable to say we have a production quality driver until we have production quality hardware to test it on.) BR, Jani. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: "Zanoni, Paulo R" <paulo.r.zanoni@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_drv.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 008fc1e62a69..c2c210b0f47f 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -782,8 +782,12 @@ static void i915_workqueues_cleanup(struct drm_i915_private *dev_priv) > */ > static void intel_detect_preproduction_hw(struct drm_i915_private *dev_priv) > { > - if (IS_HSW_EARLY_SDV(dev_priv) || > - IS_SKL_REVID(dev_priv, 0, SKL_REVID_F0)) > + bool sdv = false; > + > + sdv |= IS_HSW_EARLY_SDV(dev_priv); > + sdv |= IS_SKL_REVID(dev_priv, 0, SKL_REVID_F0); > + > + if (sdv) > DRM_ERROR("This is a pre-production stepping. " > "It may not be fully functional.\n"); > }
On Thu, Jan 26, 2017 at 01:16:03PM +0000, Chris Wilson wrote: > On Thu, Jan 26, 2017 at 03:02:14PM +0200, Ville Syrjälä wrote: > > On Thu, Jan 26, 2017 at 12:50:28PM +0000, Chris Wilson wrote: > > > As we add new generations, we should keep detecting new system > > > development platforms that were temporarily enabled (via > > > "i915.alpha_support") and now superseded by production systems. To make > > > it easier to add more platforms, split the if into a series of logical > > > operations. > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: "Zanoni, Paulo R" <paulo.r.zanoni@intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Cc: Jani Nikula <jani.nikula@intel.com> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_drv.c | 8 ++++++-- > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > > > index 008fc1e62a69..c2c210b0f47f 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.c > > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > > @@ -782,8 +782,12 @@ static void i915_workqueues_cleanup(struct drm_i915_private *dev_priv) > > > */ > > > static void intel_detect_preproduction_hw(struct drm_i915_private *dev_priv) > > > { > > > - if (IS_HSW_EARLY_SDV(dev_priv) || > > > - IS_SKL_REVID(dev_priv, 0, SKL_REVID_F0)) > > > + bool sdv = false; > > > > Presumably not all SDVs use pre-production silicon, so the name of > > this variable feels a bit off. Also since it doesn't actually check > > anything about the actual board we can't even say that it's an > > actual SDV, so the HSW_EARLY_SDV() macro seems somewhat poorly named > > to me as well. > > I started with bool preproduction; but that eats too many characters. > Ideas for something short and snappy? bool alpha_hw? bool alpha? Preferring bool pre; to avoid the connotations of "alpha hardware". All our hardware is perfect. -Chris
On Thu, Jan 26, 2017 at 03:18:28PM +0200, Jani Nikula wrote: > On Thu, 26 Jan 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > As we add new generations, we should keep detecting new system > > development platforms that were temporarily enabled (via > > "i915.alpha_support") and now superseded by production systems. To make > > it easier to add more platforms, split the if into a series of logical > > operations. > > Please be careful not to conflate preproduction/early hardware with > alpha support in the driver. They are two distinct things. The former is > purely about hardware. The latter is purely about software. "temporarily enabled to facilitate development and now superseded..." ? -Chris
On Thu, 26 Jan 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Thu, Jan 26, 2017 at 03:18:28PM +0200, Jani Nikula wrote: >> On Thu, 26 Jan 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> > As we add new generations, we should keep detecting new system >> > development platforms that were temporarily enabled (via >> > "i915.alpha_support") and now superseded by production systems. To make >> > it easier to add more platforms, split the if into a series of logical >> > operations. >> >> Please be careful not to conflate preproduction/early hardware with >> alpha support in the driver. They are two distinct things. The former is >> purely about hardware. The latter is purely about software. > > "temporarily enabled to facilitate development and now superseded..." ? Yes, that's fine. A released kernel with alpha support for a given platform will still only have alpha support for production versions of said platform. BR, Jani.
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 008fc1e62a69..c2c210b0f47f 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -782,8 +782,12 @@ static void i915_workqueues_cleanup(struct drm_i915_private *dev_priv) */ static void intel_detect_preproduction_hw(struct drm_i915_private *dev_priv) { - if (IS_HSW_EARLY_SDV(dev_priv) || - IS_SKL_REVID(dev_priv, 0, SKL_REVID_F0)) + bool sdv = false; + + sdv |= IS_HSW_EARLY_SDV(dev_priv); + sdv |= IS_SKL_REVID(dev_priv, 0, SKL_REVID_F0); + + if (sdv) DRM_ERROR("This is a pre-production stepping. " "It may not be fully functional.\n"); }
As we add new generations, we should keep detecting new system development platforms that were temporarily enabled (via "i915.alpha_support") and now superseded by production systems. To make it easier to add more platforms, split the if into a series of logical operations. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: "Zanoni, Paulo R" <paulo.r.zanoni@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Jani Nikula <jani.nikula@intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> --- drivers/gpu/drm/i915/i915_drv.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)