On Mon, 11 Jan 2021, Aditya Swarup <aditya.swarup@xxxxxxxxx> wrote: > On 1/11/21 12:57 PM, Matt Roper wrote: >> On Mon, Jan 11, 2021 at 10:18:45PM +0200, Jani Nikula wrote: >>> On Mon, 11 Jan 2021, Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote: >>>> On Fri, 08 Jan 2021, Matt Roper <matthew.d.roper@xxxxxxxxx> wrote: >>>>> On Fri, Jan 08, 2021 at 03:18:52PM -0800, Aditya Swarup wrote: >>>>>> TGL adds another level of indirection for applying WA based on stepping >>>>>> information rather than PCI REVID. So change TGL_REVID enum into >>>>>> stepping enum and use PCI REVID as index into revid to stepping table to >>>>>> fetch correct display and GT stepping for application of WAs as >>>>>> suggested by Matt Roper. >>>>> >>>>> So to clarify the goal is to rename "revid" -> "stepping" because the >>>>> values like "A1," "C0," etc. are't the actual PCI revision ID, but >>>>> rather descriptions of the stepping of a given IP block; the enum values >>>>> we use to represent those are arbitrary and don't matter as long as >>>>> they're monotonically increasing for comparisons. The PCI revision ID >>>>> is just the input we use today to deduce what the IP steppings are, and >>>>> there's talk that we could determine the IP steppings in a different way >>>>> at some point in the future. >>>>> >>>>> Furthermore, since the same scheme will be used at least for ADL-S, we >>>>> should drop the "TGL" prefix since there's no need to name these general >>>>> enum values in a platform-specific manner. >>>>> >>>>> Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx> >>>>> >>>>> We should probably make the same kind of change to KBL (and use the same >>>>> stepping enum) too since it has the same kind of extra indirection as >>>>> TGL/ADL-S, but we can do that as a followup patch. >>>> >>>> FWIW I have a wip series changing the whole thing to abstract steppings >>>> enums that are shared between platforms, but it's in a bit of limbo >>>> because the previous revid changes were applied to drm-intel-gt-next, >>>> and it's fallen pretty far out of sync with drm-intel-next. All of this >>>> really belongs to drm-intel-next, but can't do that until the branches >>>> sync up again. >>> >>> Btw this series doesn't apply to drm-intel-next either, for the same >>> reason, and the ADL-S platform definition and PCI IDs must *not* be >>> applied to drm-intel-gt-next. > > The reason behind this patch not cleanly applying on drm-intel-next is because > drm/i915/tgl: Add bound checks and simplify TGL REVID macros > isn't present on that branch but present on gt-next. > > The patch doesn't apply on gt-next because of a conflict in the following hunk: > /* Wa_1409825376:tgl (pre-prod)*/ > - if (IS_TGL_DISP_REVID(dev_priv, TGL_REVID_A0, TGL_REVID_B1)) > + if (IS_TGL_DISP_STEPPING(dev_priv, STEP_A0, STEP_B1)) > > which can be easily fixed during backmerge process as I was able apply the patch > cleanly on gt-next. > I don't understand the "must *not*" reasoning behind not applying this patch on gt-next. I think I've explained this in several replies in this thread now. > It was common consesus during initial review that separating > stepping/revid parsing in a separate .c file will be pushed in after > ADLS changes and adding this patch won't add any extra churn, just a > minor rebase for your approach. Misunderstanding I guess. I thought the required changes had already been pushed, and we weren't waiting for further changes on this. I certainly wasn't expecting the generic revid -> stepping rename at this point, as I don't think they are required for ADL-S at all. I thought the consensus was that I'll do the refactoring. Anyway, I can deal with the churn and the rebases, no problem. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
