On Wed, 2020-11-25 at 10:03 -0800, Aditya Swarup wrote:
> On 11/25/20 5:21 AM, Souza, Jose wrote:
> > On Tue, 2020-11-24 at 16:31 -0800, Aditya Swarup wrote:
> > > Fix TGL REVID macros to fetch correct display/gt stepping based
> > > on SOC rev id from INTEL_REVID() macro. Previously, we were just
> > > returning the first element of the revid array instead of using
> > > the correct index based on SOC rev id.
> > >
> > > Also, add array bound checks for TGL REV ID array. Since, there
> > > might be a possibility of using older kernels on latest platform
> > > revision, resulting in out of bounds access for rev ID array.
> > > In this scenario, print message for unsupported rev ID and apply
> > > settings for latest rev ID available.
> > >
> > > Fixes: ("drm/i915/tgl: Fix stepping WA matching")
> > > Cc: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > > Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > > Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Aditya Swarup <aditya.swarup@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/i915_drv.h | 35 +++++++++++++++++++++++++++------
> > > 1 file changed, 29 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index 15be8debae54..29d55b7017be 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1572,16 +1572,37 @@ enum {
> > > TGL_REVID_D0,
> > > };
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > -extern const struct i915_rev_steppings tgl_uy_revids[];
> > > -extern const struct i915_rev_steppings tgl_revids[];
> > > +extern const struct i915_rev_steppings tgl_uy_revids[4];
> > > +extern const struct i915_rev_steppings tgl_revids[2];
> >
> > Not sure if the above will work, saw a comment from Jani please check that.
>
> This works otherwise I can't use ARRAY_SIZE() macro as it is just an extern declaration,
> so the sizeof() doesn't have clue about the size. The only way I can think of working
> around this is by moving tables here but Matt's KBL REVID patch suggests unused variables errors
> but my compiler didn't complain.
>
> >
> > > +
> > > +#define TGL_UY_REVID_RANGE(revid) \
> > > + ((revid) < ARRAY_SIZE(tgl_uy_revids))
> > > +
> > > +#define TGL_REVID_RANGE(revid) \
> > > + ((revid) < ARRAY_SIZE(tgl_revids))
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > static inline const struct i915_rev_steppings *
> > > tgl_revids_get(struct drm_i915_private *dev_priv)
> > > {
> > > - if (IS_TGL_U(dev_priv) || IS_TGL_Y(dev_priv))
> > > - return tgl_uy_revids;
> > > - else
> > > - return tgl_revids;
> > > + const u8 revid = INTEL_REVID(dev_priv);
> > > +
> > > + if (IS_TGL_U(dev_priv) || IS_TGL_Y(dev_priv)) {
> > > + if (TGL_UY_REVID_RANGE(revid)) {
> > > + return tgl_uy_revids + revid;
> >
> > Why not help readers and go simple? tgl_uy_revids[revid]
>
> Hmm I will have to change the return type then, as you were returning a pointer and introduces
> compiler error. I will change the return type.
No need to change the return type. &tgl_uy_revids[revid]
>
> >
> > > + } else {
> > > + drm_dbg_kms(&dev_priv->drm,
> > > + "Unsupported SOC stepping found %u, using %lu instead\n",
> > > + revid, ARRAY_SIZE(tgl_uy_revids) - 1);
> > > + return tgl_uy_revids + (ARRAY_SIZE(tgl_uy_revids) - 1);
> > > + }
> > > + } else if (TGL_REVID_RANGE(revid)) {
> > > + return tgl_revids + revid;
> > > + } else {
> > > + drm_dbg_kms(&dev_priv->drm,
> > > + "Unsupported SOC stepping found %u, using %lu instead\n",
> > > + revid, ARRAY_SIZE(tgl_revids) - 1);
> > > + return tgl_uy_revids + (ARRAY_SIZE(tgl_revids) - 1);
> > > + }
> >
> > I bet you can re arrange it and end up with one drm_dbg_kms() call.
>
> I can but that will involve more macros as we are dealing with two different array tables and each one
> with a different range. I will use just one print to say what SOC rev id we get from pci dev and what
> we will be using.
>
> >
> >
> > > }
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > #define IS_TGL_DISP_REVID(p, since, until) \
> > > @@ -1591,12 +1612,14 @@ tgl_revids_get(struct drm_i915_private *dev_priv)
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > #define IS_TGL_UY_GT_REVID(p, since, until) \
> > > ((IS_TGL_U(p) || IS_TGL_Y(p)) && \
> > > + TGL_UY_REVID_RANGE(INTEL_REVID(p)) && \
> > > tgl_uy_revids->gt_stepping >= (since) && \
> > > tgl_uy_revids->gt_stepping <= (until))
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > > #define IS_TGL_GT_REVID(p, since, until) \
> > > (IS_TIGERLAKE(p) && \
> > > !(IS_TGL_U(p) || IS_TGL_Y(p)) && \
> > > + TGL_REVID_RANGE(INTEL_REVID(p)) && \
> > > tgl_revids->gt_stepping >= (since) && \
> > > tgl_revids->gt_stepping <= (until))
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> >
> > You did not fixed the issue for GT.
>
> Yes.. I didn't notice that.. Will change in the next revision.
>
> Aditya
>
> >
> > >
> > >
> > >
> > >
> > >
> >
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
