| Message ID | fe8743770694e429f6902491cdb306c97bdf701a.1724180287.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/display: identify display steppings in display code | expand |
On Tue, Aug 20, 2024 at 10:00:34PM GMT, Jani Nikula wrote: >It's supposed to be an open range at the end like in i915. Fingers >crossed that nobody relies on this definition. we are checking for step though, so IMO this deserves a Fixes: 44e694958b95 ("drm/xe/display: Implement display support") from a git grep, for the platforms relevants to xe, this mostly affects ADL-P that is used as a test vehicle. > >Signed-off-by: Jani Nikula <jani.nikula@intel.com> >--- > drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > >diff --git a/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h b/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h >index 2feedddf1e40..1f1ad4d3ef51 100644 >--- a/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h >+++ b/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h >@@ -83,7 +83,7 @@ static inline struct drm_i915_private *kdev_to_i915(struct device *kdev) > #define HAS_GMD_ID(xe) GRAPHICS_VERx100(xe) >= 1270 > > /* Workarounds not handled yet */ I guess this can be removed already. >-#define IS_DISPLAY_STEP(xe, first, last) ({u8 __step = (xe)->info.step.display; first <= __step && __step <= last; }) >+#define IS_DISPLAY_STEP(xe, first, last) ({u8 __step = (xe)->info.step.display; first <= __step && __step < last; }) Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> thanks Lucas De Marchi > > #define IS_LP(xe) (0) > #define IS_GEN9_LP(xe) (0) >-- >2.39.2 >
On Tue, Aug 20, 2024 at 02:31:20PM -0500, Lucas De Marchi wrote: > On Tue, Aug 20, 2024 at 10:00:34PM GMT, Jani Nikula wrote: > > It's supposed to be an open range at the end like in i915. Fingers > > crossed that nobody relies on this definition. I think the various IS_DISPLAY_IP_STEP uses in the driver were already assuming the corrected logic from this patch, so we've just been applying workarounds to some steppings where they weren't technically required. The risk now would be if a workaround was somehow recorded with incorrect bounds in the workaround database itself and we were masking that misdocumentation with the wrong logic. That doesn't seem terribly likely though. > > we are checking for step though, so IMO this deserves a > > Fixes: 44e694958b95 ("drm/xe/display: Implement display support") > > from a git grep, for the platforms relevants to xe, this mostly affects > ADL-P that is used as a test vehicle. A simple grep is a bit misleading because IS_DISPLAY_STEP gets used by IS_DISPLAY_IP_STEP, which is what we use instead on the newer GMD_ID-based platforms. If you grep instead for IS_DISPLAY_IP_STEP, it turns up some MTL, BMG, and LNL workarounds that would have been impacted. But as noted above, I still don't think there's much risk here since it would only be a problem if the workaround documentation itself was incorrect. Reviewed-by: Matt Roper <matthew.d.roper@intel.com> > > > > > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > --- > > drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h b/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h > > index 2feedddf1e40..1f1ad4d3ef51 100644 > > --- a/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h > > +++ b/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h > > @@ -83,7 +83,7 @@ static inline struct drm_i915_private *kdev_to_i915(struct device *kdev) > > #define HAS_GMD_ID(xe) GRAPHICS_VERx100(xe) >= 1270 > > > > /* Workarounds not handled yet */ > > I guess this can be removed already. > > > -#define IS_DISPLAY_STEP(xe, first, last) ({u8 __step = (xe)->info.step.display; first <= __step && __step <= last; }) > > +#define IS_DISPLAY_STEP(xe, first, last) ({u8 __step = (xe)->info.step.display; first <= __step && __step < last; }) > > Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> > > thanks > Lucas De Marchi > > > > > > #define IS_LP(xe) (0) > > #define IS_GEN9_LP(xe) (0) > > -- > > 2.39.2 > >
diff --git a/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h b/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h index 2feedddf1e40..1f1ad4d3ef51 100644 --- a/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h +++ b/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h @@ -83,7 +83,7 @@ static inline struct drm_i915_private *kdev_to_i915(struct device *kdev) #define HAS_GMD_ID(xe) GRAPHICS_VERx100(xe) >= 1270 /* Workarounds not handled yet */ -#define IS_DISPLAY_STEP(xe, first, last) ({u8 __step = (xe)->info.step.display; first <= __step && __step <= last; }) +#define IS_DISPLAY_STEP(xe, first, last) ({u8 __step = (xe)->info.step.display; first <= __step && __step < last; }) #define IS_LP(xe) (0) #define IS_GEN9_LP(xe) (0)
It's supposed to be an open range at the end like in i915. Fingers crossed that nobody relies on this definition. Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)