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@xxxxxxxxx>
>
> >
> > Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
>
> thanks
> Lucas De Marchi
>
>
> >
> > #define IS_LP(xe) (0)
> > #define IS_GEN9_LP(xe) (0)
> > --
> > 2.39.2
> >
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
