| Message ID | 20240701132754.101832-4-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] drm/xe/bmg: implement Wa_16023588340 | expand |
On Mon, Jul 01, 2024 at 02:27:56PM GMT, Matthew Auld wrote: >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..a4256144dff7 100644 >--- a/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h >+++ b/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h >@@ -15,6 +15,9 @@ > #include "i915_utils.h" > #include "intel_runtime_pm.h" > #include "xe_device_types.h" >+#include "xe_wa.h" >+ >+#include <generated/xe_wa_oob.h> I think building xe.ko with this include in a header will be racy: any file that has this include needs to declare the dependency in the Makefile. I tried a few alternatives to convince the build system to always execute the generate-rules before anything else, but it failed CI: https://patchwork.freedesktop.org/series/132331/ I think we will need that before allowing XE_WA() to show up in header files. Lucas De Marchi
On Mon, 01 Jul 2024, Matthew Auld <matthew.auld@intel.com> wrote: > On BMG-G21 we need to disable fbc due to complications around the WA. > > v2: > - Try to handle with i915_drv.h and compat layer. (Rodrigo) > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Cc: Jonathan Cavitt <jonathan.cavitt@intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > Cc: Vinod Govindapillai <vinod.govindapillai@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: intel-gfx@lists.freedesktop.org > --- > drivers/gpu/drm/i915/display/intel_fbc.c | 5 +++++ > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h | 5 +++++ > 3 files changed, 12 insertions(+) > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > index 67116c9f1464..60131de77b4c 100644 > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > @@ -1237,6 +1237,11 @@ static int intel_fbc_check_plane(struct intel_atomic_state *state, > return 0; > } > > + if (DISPLAY_NEEDS_WA_16023588340(i915)) { > + plane_state->no_fbc_reason = "Wa_16023588340"; > + return 0; > + } > + > /* WaFbcTurnOffFbcWhenHyperVisorIsUsed:skl,bxt */ > if (i915_vtd_active(i915) && (IS_SKYLAKE(i915) || IS_BROXTON(i915))) { > plane_state->no_fbc_reason = "VT-d enabled"; > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index d7723dd11c80..816a01fda3fe 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -762,4 +762,6 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, > #define HAS_LMEMBAR_SMEM_STOLEN(i915) (!HAS_LMEM(i915) && \ > GRAPHICS_VER_FULL(i915) >= IP_VER(12, 70)) > > +#define DISPLAY_NEEDS_WA_16023588340(i915) false > + Display feature macros don't belong in i915_drv.h. BR, Jani. > #endif > 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..a4256144dff7 100644 > --- a/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h > +++ b/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h > @@ -15,6 +15,9 @@ > #include "i915_utils.h" > #include "intel_runtime_pm.h" > #include "xe_device_types.h" > +#include "xe_wa.h" > + > +#include <generated/xe_wa_oob.h> > > static inline struct drm_i915_private *to_i915(const struct drm_device *dev) > { > @@ -120,6 +123,8 @@ struct i915_sched_attr { > > #define FORCEWAKE_ALL XE_FORCEWAKE_ALL > > +#define DISPLAY_NEEDS_WA_16023588340(xe) XE_WA(xe_root_mmio_gt(xe), 16023588340) > + > #ifdef CONFIG_ARM64 > /* > * arm64 indirectly includes linux/rtc.h,
Hi, On 02/07/2024 09:17, Jani Nikula wrote: > On Mon, 01 Jul 2024, Matthew Auld <matthew.auld@intel.com> wrote: >> On BMG-G21 we need to disable fbc due to complications around the WA. >> >> v2: >> - Try to handle with i915_drv.h and compat layer. (Rodrigo) >> >> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >> Cc: Jonathan Cavitt <jonathan.cavitt@intel.com> >> Cc: Matt Roper <matthew.d.roper@intel.com> >> Cc: Lucas De Marchi <lucas.demarchi@intel.com> >> Cc: Vinod Govindapillai <vinod.govindapillai@intel.com> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Cc: intel-gfx@lists.freedesktop.org >> --- >> drivers/gpu/drm/i915/display/intel_fbc.c | 5 +++++ >> drivers/gpu/drm/i915/i915_drv.h | 2 ++ >> drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h | 5 +++++ >> 3 files changed, 12 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c >> index 67116c9f1464..60131de77b4c 100644 >> --- a/drivers/gpu/drm/i915/display/intel_fbc.c >> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c >> @@ -1237,6 +1237,11 @@ static int intel_fbc_check_plane(struct intel_atomic_state *state, >> return 0; >> } >> >> + if (DISPLAY_NEEDS_WA_16023588340(i915)) { >> + plane_state->no_fbc_reason = "Wa_16023588340"; >> + return 0; >> + } >> + >> /* WaFbcTurnOffFbcWhenHyperVisorIsUsed:skl,bxt */ >> if (i915_vtd_active(i915) && (IS_SKYLAKE(i915) || IS_BROXTON(i915))) { >> plane_state->no_fbc_reason = "VT-d enabled"; >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index d7723dd11c80..816a01fda3fe 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -762,4 +762,6 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, >> #define HAS_LMEMBAR_SMEM_STOLEN(i915) (!HAS_LMEM(i915) && \ >> GRAPHICS_VER_FULL(i915) >= IP_VER(12, 70)) >> >> +#define DISPLAY_NEEDS_WA_16023588340(i915) false >> + > > Display feature macros don't belong in i915_drv.h. There is also the v1 here: https://patchwork.freedesktop.org/patch/599900/?series=135061&rev=1 Where feedback was to handle it in i915_drv.h. Is the v1 method acceptable here? Do you have an alternative suggestion? > > BR, > Jani. > >> #endif >> 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..a4256144dff7 100644 >> --- a/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h >> +++ b/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h >> @@ -15,6 +15,9 @@ >> #include "i915_utils.h" >> #include "intel_runtime_pm.h" >> #include "xe_device_types.h" >> +#include "xe_wa.h" >> + >> +#include <generated/xe_wa_oob.h> >> >> static inline struct drm_i915_private *to_i915(const struct drm_device *dev) >> { >> @@ -120,6 +123,8 @@ struct i915_sched_attr { >> >> #define FORCEWAKE_ALL XE_FORCEWAKE_ALL >> >> +#define DISPLAY_NEEDS_WA_16023588340(xe) XE_WA(xe_root_mmio_gt(xe), 16023588340) >> + >> #ifdef CONFIG_ARM64 >> /* >> * arm64 indirectly includes linux/rtc.h, >
On Tue, 02 Jul 2024, Matthew Auld <matthew.auld@intel.com> wrote: > Hi, > > On 02/07/2024 09:17, Jani Nikula wrote: >> On Mon, 01 Jul 2024, Matthew Auld <matthew.auld@intel.com> wrote: >>> On BMG-G21 we need to disable fbc due to complications around the WA. >>> >>> v2: >>> - Try to handle with i915_drv.h and compat layer. (Rodrigo) >>> >>> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >>> Cc: Jonathan Cavitt <jonathan.cavitt@intel.com> >>> Cc: Matt Roper <matthew.d.roper@intel.com> >>> Cc: Lucas De Marchi <lucas.demarchi@intel.com> >>> Cc: Vinod Govindapillai <vinod.govindapillai@intel.com> >>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >>> Cc: intel-gfx@lists.freedesktop.org >>> --- >>> drivers/gpu/drm/i915/display/intel_fbc.c | 5 +++++ >>> drivers/gpu/drm/i915/i915_drv.h | 2 ++ >>> drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h | 5 +++++ >>> 3 files changed, 12 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c >>> index 67116c9f1464..60131de77b4c 100644 >>> --- a/drivers/gpu/drm/i915/display/intel_fbc.c >>> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c >>> @@ -1237,6 +1237,11 @@ static int intel_fbc_check_plane(struct intel_atomic_state *state, >>> return 0; >>> } >>> >>> + if (DISPLAY_NEEDS_WA_16023588340(i915)) { >>> + plane_state->no_fbc_reason = "Wa_16023588340"; >>> + return 0; >>> + } >>> + >>> /* WaFbcTurnOffFbcWhenHyperVisorIsUsed:skl,bxt */ >>> if (i915_vtd_active(i915) && (IS_SKYLAKE(i915) || IS_BROXTON(i915))) { >>> plane_state->no_fbc_reason = "VT-d enabled"; >>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >>> index d7723dd11c80..816a01fda3fe 100644 >>> --- a/drivers/gpu/drm/i915/i915_drv.h >>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>> @@ -762,4 +762,6 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, >>> #define HAS_LMEMBAR_SMEM_STOLEN(i915) (!HAS_LMEM(i915) && \ >>> GRAPHICS_VER_FULL(i915) >= IP_VER(12, 70)) >>> >>> +#define DISPLAY_NEEDS_WA_16023588340(i915) false >>> + >> >> Display feature macros don't belong in i915_drv.h. > > There is also the v1 here: > https://patchwork.freedesktop.org/patch/599900/?series=135061&rev=1 > > Where feedback was to handle it in i915_drv.h. Is the v1 method > acceptable here? Do you have an alternative suggestion? Based on Lucas' reply, looks like it needs to be a function in a .c file anyway. One of my goals lately has been to avoid including i915_drv.h from display code altogether. We're obviously not even close yet, but anything you put there is just another problem for me to solve. So I'd like you to solve it, not me. ;) BR, Jani. > >> >> BR, >> Jani. >> >>> #endif >>> 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..a4256144dff7 100644 >>> --- a/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h >>> +++ b/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h >>> @@ -15,6 +15,9 @@ >>> #include "i915_utils.h" >>> #include "intel_runtime_pm.h" >>> #include "xe_device_types.h" >>> +#include "xe_wa.h" >>> + >>> +#include <generated/xe_wa_oob.h> >>> >>> static inline struct drm_i915_private *to_i915(const struct drm_device *dev) >>> { >>> @@ -120,6 +123,8 @@ struct i915_sched_attr { >>> >>> #define FORCEWAKE_ALL XE_FORCEWAKE_ALL >>> >>> +#define DISPLAY_NEEDS_WA_16023588340(xe) XE_WA(xe_root_mmio_gt(xe), 16023588340) >>> + >>> #ifdef CONFIG_ARM64 >>> /* >>> * arm64 indirectly includes linux/rtc.h, >>
On 02/07/2024 10:26, Jani Nikula wrote: > On Tue, 02 Jul 2024, Matthew Auld <matthew.auld@intel.com> wrote: >> Hi, >> >> On 02/07/2024 09:17, Jani Nikula wrote: >>> On Mon, 01 Jul 2024, Matthew Auld <matthew.auld@intel.com> wrote: >>>> On BMG-G21 we need to disable fbc due to complications around the WA. >>>> >>>> v2: >>>> - Try to handle with i915_drv.h and compat layer. (Rodrigo) >>>> >>>> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >>>> Cc: Jonathan Cavitt <jonathan.cavitt@intel.com> >>>> Cc: Matt Roper <matthew.d.roper@intel.com> >>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com> >>>> Cc: Vinod Govindapillai <vinod.govindapillai@intel.com> >>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >>>> Cc: intel-gfx@lists.freedesktop.org >>>> --- >>>> drivers/gpu/drm/i915/display/intel_fbc.c | 5 +++++ >>>> drivers/gpu/drm/i915/i915_drv.h | 2 ++ >>>> drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h | 5 +++++ >>>> 3 files changed, 12 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c >>>> index 67116c9f1464..60131de77b4c 100644 >>>> --- a/drivers/gpu/drm/i915/display/intel_fbc.c >>>> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c >>>> @@ -1237,6 +1237,11 @@ static int intel_fbc_check_plane(struct intel_atomic_state *state, >>>> return 0; >>>> } >>>> >>>> + if (DISPLAY_NEEDS_WA_16023588340(i915)) { >>>> + plane_state->no_fbc_reason = "Wa_16023588340"; >>>> + return 0; >>>> + } >>>> + >>>> /* WaFbcTurnOffFbcWhenHyperVisorIsUsed:skl,bxt */ >>>> if (i915_vtd_active(i915) && (IS_SKYLAKE(i915) || IS_BROXTON(i915))) { >>>> plane_state->no_fbc_reason = "VT-d enabled"; >>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >>>> index d7723dd11c80..816a01fda3fe 100644 >>>> --- a/drivers/gpu/drm/i915/i915_drv.h >>>> +++ b/drivers/gpu/drm/i915/i915_drv.h >>>> @@ -762,4 +762,6 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, >>>> #define HAS_LMEMBAR_SMEM_STOLEN(i915) (!HAS_LMEM(i915) && \ >>>> GRAPHICS_VER_FULL(i915) >= IP_VER(12, 70)) >>>> >>>> +#define DISPLAY_NEEDS_WA_16023588340(i915) false >>>> + >>> >>> Display feature macros don't belong in i915_drv.h. >> >> There is also the v1 here: >> https://patchwork.freedesktop.org/patch/599900/?series=135061&rev=1 >> >> Where feedback was to handle it in i915_drv.h. Is the v1 method >> acceptable here? Do you have an alternative suggestion? > > Based on Lucas' reply, looks like it needs to be a function in a .c file > anyway. > > One of my goals lately has been to avoid including i915_drv.h from > display code altogether. We're obviously not even close yet, but > anything you put there is just another problem for me to solve. So I'd > like you to solve it, not me. ;) So no major objections with going back to v1 from your side? In addition, merging both patches via drm-xe-next would be acceptable? > > BR, > Jani. > >> >>> >>> BR, >>> Jani. >>> >>>> #endif >>>> 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..a4256144dff7 100644 >>>> --- a/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h >>>> +++ b/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h >>>> @@ -15,6 +15,9 @@ >>>> #include "i915_utils.h" >>>> #include "intel_runtime_pm.h" >>>> #include "xe_device_types.h" >>>> +#include "xe_wa.h" >>>> + >>>> +#include <generated/xe_wa_oob.h> >>>> >>>> static inline struct drm_i915_private *to_i915(const struct drm_device *dev) >>>> { >>>> @@ -120,6 +123,8 @@ struct i915_sched_attr { >>>> >>>> #define FORCEWAKE_ALL XE_FORCEWAKE_ALL >>>> >>>> +#define DISPLAY_NEEDS_WA_16023588340(xe) XE_WA(xe_root_mmio_gt(xe), 16023588340) >>>> + >>>> #ifdef CONFIG_ARM64 >>>> /* >>>> * arm64 indirectly includes linux/rtc.h, >>> >
On Tue, 02 Jul 2024, Matthew Auld <matthew.auld@intel.com> wrote: > So no major objections with going back to v1 from your side? In > addition, merging both patches via drm-xe-next would be acceptable? As discussed, and FYI to others, no objections on v1 from me, and ack on merging via drm-xe-next. BR, Jani.
diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c index 67116c9f1464..60131de77b4c 100644 --- a/drivers/gpu/drm/i915/display/intel_fbc.c +++ b/drivers/gpu/drm/i915/display/intel_fbc.c @@ -1237,6 +1237,11 @@ static int intel_fbc_check_plane(struct intel_atomic_state *state, return 0; } + if (DISPLAY_NEEDS_WA_16023588340(i915)) { + plane_state->no_fbc_reason = "Wa_16023588340"; + return 0; + } + /* WaFbcTurnOffFbcWhenHyperVisorIsUsed:skl,bxt */ if (i915_vtd_active(i915) && (IS_SKYLAKE(i915) || IS_BROXTON(i915))) { plane_state->no_fbc_reason = "VT-d enabled"; diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index d7723dd11c80..816a01fda3fe 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -762,4 +762,6 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915, #define HAS_LMEMBAR_SMEM_STOLEN(i915) (!HAS_LMEM(i915) && \ GRAPHICS_VER_FULL(i915) >= IP_VER(12, 70)) +#define DISPLAY_NEEDS_WA_16023588340(i915) false + #endif 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..a4256144dff7 100644 --- a/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h +++ b/drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h @@ -15,6 +15,9 @@ #include "i915_utils.h" #include "intel_runtime_pm.h" #include "xe_device_types.h" +#include "xe_wa.h" + +#include <generated/xe_wa_oob.h> static inline struct drm_i915_private *to_i915(const struct drm_device *dev) { @@ -120,6 +123,8 @@ struct i915_sched_attr { #define FORCEWAKE_ALL XE_FORCEWAKE_ALL +#define DISPLAY_NEEDS_WA_16023588340(xe) XE_WA(xe_root_mmio_gt(xe), 16023588340) + #ifdef CONFIG_ARM64 /* * arm64 indirectly includes linux/rtc.h,
On BMG-G21 we need to disable fbc due to complications around the WA. v2: - Try to handle with i915_drv.h and compat layer. (Rodrigo) Signed-off-by: Matthew Auld <matthew.auld@intel.com> Cc: Jonathan Cavitt <jonathan.cavitt@intel.com> Cc: Matt Roper <matthew.d.roper@intel.com> Cc: Lucas De Marchi <lucas.demarchi@intel.com> Cc: Vinod Govindapillai <vinod.govindapillai@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: intel-gfx@lists.freedesktop.org --- drivers/gpu/drm/i915/display/intel_fbc.c | 5 +++++ drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/xe/compat-i915-headers/i915_drv.h | 5 +++++ 3 files changed, 12 insertions(+)