| Message ID | 20240619143127.110045-4-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
-----Original Message----- From: Auld, Matthew <matthew.auld@intel.com> Sent: Wednesday, June 19, 2024 7:31 AM To: intel-xe@lists.freedesktop.org Cc: Cavitt, Jonathan <jonathan.cavitt@intel.com>; Roper, Matthew D <matthew.d.roper@intel.com>; De Marchi, Lucas <lucas.demarchi@intel.com>; Govindapillai, Vinod <vinod.govindapillai@intel.com>; intel-gfx@lists.freedesktop.org Subject: [PATCH 2/2] drm/i915: disable fbc due to Wa_16023588340 > > On BMG-G21 we need to disable fbc due to complications around the WA. > > 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: intel-gfx@lists.freedesktop.org > --- > drivers/gpu/drm/i915/display/intel_display_wa.h | 8 ++++++++ > drivers/gpu/drm/i915/display/intel_fbc.c | 6 ++++++ > drivers/gpu/drm/xe/Makefile | 4 +++- > drivers/gpu/drm/xe/display/xe_display_wa.c | 16 ++++++++++++++++ > 4 files changed, 33 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/xe/display/xe_display_wa.c > > diff --git a/drivers/gpu/drm/i915/display/intel_display_wa.h b/drivers/gpu/drm/i915/display/intel_display_wa.h > index 63201d09852c..be644ab6ae00 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_wa.h > +++ b/drivers/gpu/drm/i915/display/intel_display_wa.h > @@ -6,8 +6,16 @@ > #ifndef __INTEL_DISPLAY_WA_H__ > #define __INTEL_DISPLAY_WA_H__ > > +#include <linux/types.h> > + > struct drm_i915_private; > > void intel_display_wa_apply(struct drm_i915_private *i915); > > +#ifdef I915 > +static inline bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915) { return false; } > +#else > +bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915); > +#endif > + > #endif > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > index 67116c9f1464..8488f82143a4 100644 > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > @@ -56,6 +56,7 @@ > #include "intel_display_device.h" > #include "intel_display_trace.h" > #include "intel_display_types.h" > +#include "intel_display_wa.h" > #include "intel_fbc.h" > #include "intel_fbc_regs.h" > #include "intel_frontbuffer.h" > @@ -1237,6 +1238,11 @@ static int intel_fbc_check_plane(struct intel_atomic_state *state, > return 0; > } > > + if (intel_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/xe/Makefile b/drivers/gpu/drm/xe/Makefile > index 0e16e5029081..f7521fd5db4c 100644 > --- a/drivers/gpu/drm/xe/Makefile > +++ b/drivers/gpu/drm/xe/Makefile > @@ -34,7 +34,8 @@ uses_generated_oob := \ > $(obj)/xe_ring_ops.o \ > $(obj)/xe_vm.o \ > $(obj)/xe_wa.o \ > - $(obj)/xe_ttm_stolen_mgr.o > + $(obj)/xe_ttm_stolen_mgr.o \ > + $(obj)/display/xe_display_wa.o \ > > $(uses_generated_oob): $(generated_oob) > > @@ -192,6 +193,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \ > display/xe_display.o \ > display/xe_display_misc.o \ > display/xe_display_rps.o \ > + display/xe_display_wa.o \ > display/xe_dsb_buffer.o \ > display/xe_fb_pin.o \ > display/xe_hdcp_gsc.o \ > diff --git a/drivers/gpu/drm/xe/display/xe_display_wa.c b/drivers/gpu/drm/xe/display/xe_display_wa.c > new file mode 100644 > index 000000000000..68e3d1959ad6 > --- /dev/null > +++ b/drivers/gpu/drm/xe/display/xe_display_wa.c > @@ -0,0 +1,16 @@ > +// SPDX-License-Identifier: MIT > +/* > + * Copyright © 2024 Intel Corporation > + */ > + > +#include "intel_display_wa.h" > + > +#include "xe_device.h" > +#include "xe_wa.h" > + > +#include <generated/xe_wa_oob.h> > + > +bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915) > +{ > + return XE_WA(xe_root_mmio_gt(i915), 16023588340); You'll probably want to get verification this is the correct WA number before pushing this change, but other than that: Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com> -Jonathan Cavitt > +} > -- > 2.45.1 > >
On 19/06/2024 15:31, Matthew Auld wrote: > On BMG-G21 we need to disable fbc due to complications around the WA. > > 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: intel-gfx@lists.freedesktop.org Can this be merged via drm-xe-next? The first patch is the xe centric part of the WA, but here this is touching i915 display and xe. > --- > drivers/gpu/drm/i915/display/intel_display_wa.h | 8 ++++++++ > drivers/gpu/drm/i915/display/intel_fbc.c | 6 ++++++ > drivers/gpu/drm/xe/Makefile | 4 +++- > drivers/gpu/drm/xe/display/xe_display_wa.c | 16 ++++++++++++++++ > 4 files changed, 33 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/xe/display/xe_display_wa.c > > diff --git a/drivers/gpu/drm/i915/display/intel_display_wa.h b/drivers/gpu/drm/i915/display/intel_display_wa.h > index 63201d09852c..be644ab6ae00 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_wa.h > +++ b/drivers/gpu/drm/i915/display/intel_display_wa.h > @@ -6,8 +6,16 @@ > #ifndef __INTEL_DISPLAY_WA_H__ > #define __INTEL_DISPLAY_WA_H__ > > +#include <linux/types.h> > + > struct drm_i915_private; > > void intel_display_wa_apply(struct drm_i915_private *i915); > > +#ifdef I915 > +static inline bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915) { return false; } > +#else > +bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915); > +#endif > + > #endif > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > index 67116c9f1464..8488f82143a4 100644 > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > @@ -56,6 +56,7 @@ > #include "intel_display_device.h" > #include "intel_display_trace.h" > #include "intel_display_types.h" > +#include "intel_display_wa.h" > #include "intel_fbc.h" > #include "intel_fbc_regs.h" > #include "intel_frontbuffer.h" > @@ -1237,6 +1238,11 @@ static int intel_fbc_check_plane(struct intel_atomic_state *state, > return 0; > } > > + if (intel_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/xe/Makefile b/drivers/gpu/drm/xe/Makefile > index 0e16e5029081..f7521fd5db4c 100644 > --- a/drivers/gpu/drm/xe/Makefile > +++ b/drivers/gpu/drm/xe/Makefile > @@ -34,7 +34,8 @@ uses_generated_oob := \ > $(obj)/xe_ring_ops.o \ > $(obj)/xe_vm.o \ > $(obj)/xe_wa.o \ > - $(obj)/xe_ttm_stolen_mgr.o > + $(obj)/xe_ttm_stolen_mgr.o \ > + $(obj)/display/xe_display_wa.o \ > > $(uses_generated_oob): $(generated_oob) > > @@ -192,6 +193,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \ > display/xe_display.o \ > display/xe_display_misc.o \ > display/xe_display_rps.o \ > + display/xe_display_wa.o \ > display/xe_dsb_buffer.o \ > display/xe_fb_pin.o \ > display/xe_hdcp_gsc.o \ > diff --git a/drivers/gpu/drm/xe/display/xe_display_wa.c b/drivers/gpu/drm/xe/display/xe_display_wa.c > new file mode 100644 > index 000000000000..68e3d1959ad6 > --- /dev/null > +++ b/drivers/gpu/drm/xe/display/xe_display_wa.c > @@ -0,0 +1,16 @@ > +// SPDX-License-Identifier: MIT > +/* > + * Copyright © 2024 Intel Corporation > + */ > + > +#include "intel_display_wa.h" > + > +#include "xe_device.h" > +#include "xe_wa.h" > + > +#include <generated/xe_wa_oob.h> > + > +bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915) > +{ > + return XE_WA(xe_root_mmio_gt(i915), 16023588340); > +}
On 25/06/2024 09:09, Matthew Auld wrote: > On 19/06/2024 15:31, Matthew Auld wrote: >> On BMG-G21 we need to disable fbc due to complications around the WA. >> >> 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: intel-gfx@lists.freedesktop.org > Can this be merged via drm-xe-next? The first patch is the xe centric > part of the WA, but here this is touching i915 display and xe. Ping? Can this be landed via drm-xe-next? Lucas, Jani? > >> --- >> drivers/gpu/drm/i915/display/intel_display_wa.h | 8 ++++++++ >> drivers/gpu/drm/i915/display/intel_fbc.c | 6 ++++++ >> drivers/gpu/drm/xe/Makefile | 4 +++- >> drivers/gpu/drm/xe/display/xe_display_wa.c | 16 ++++++++++++++++ >> 4 files changed, 33 insertions(+), 1 deletion(-) >> create mode 100644 drivers/gpu/drm/xe/display/xe_display_wa.c >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_wa.h >> b/drivers/gpu/drm/i915/display/intel_display_wa.h >> index 63201d09852c..be644ab6ae00 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display_wa.h >> +++ b/drivers/gpu/drm/i915/display/intel_display_wa.h >> @@ -6,8 +6,16 @@ >> #ifndef __INTEL_DISPLAY_WA_H__ >> #define __INTEL_DISPLAY_WA_H__ >> +#include <linux/types.h> >> + >> struct drm_i915_private; >> void intel_display_wa_apply(struct drm_i915_private *i915); >> +#ifdef I915 >> +static inline bool intel_display_needs_wa_16023588340(struct >> drm_i915_private *i915) { return false; } >> +#else >> +bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915); >> +#endif >> + >> #endif >> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c >> b/drivers/gpu/drm/i915/display/intel_fbc.c >> index 67116c9f1464..8488f82143a4 100644 >> --- a/drivers/gpu/drm/i915/display/intel_fbc.c >> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c >> @@ -56,6 +56,7 @@ >> #include "intel_display_device.h" >> #include "intel_display_trace.h" >> #include "intel_display_types.h" >> +#include "intel_display_wa.h" >> #include "intel_fbc.h" >> #include "intel_fbc_regs.h" >> #include "intel_frontbuffer.h" >> @@ -1237,6 +1238,11 @@ static int intel_fbc_check_plane(struct >> intel_atomic_state *state, >> return 0; >> } >> + if (intel_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/xe/Makefile b/drivers/gpu/drm/xe/Makefile >> index 0e16e5029081..f7521fd5db4c 100644 >> --- a/drivers/gpu/drm/xe/Makefile >> +++ b/drivers/gpu/drm/xe/Makefile >> @@ -34,7 +34,8 @@ uses_generated_oob := \ >> $(obj)/xe_ring_ops.o \ >> $(obj)/xe_vm.o \ >> $(obj)/xe_wa.o \ >> - $(obj)/xe_ttm_stolen_mgr.o >> + $(obj)/xe_ttm_stolen_mgr.o \ >> + $(obj)/display/xe_display_wa.o \ >> $(uses_generated_oob): $(generated_oob) >> @@ -192,6 +193,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \ >> display/xe_display.o \ >> display/xe_display_misc.o \ >> display/xe_display_rps.o \ >> + display/xe_display_wa.o \ >> display/xe_dsb_buffer.o \ >> display/xe_fb_pin.o \ >> display/xe_hdcp_gsc.o \ >> diff --git a/drivers/gpu/drm/xe/display/xe_display_wa.c >> b/drivers/gpu/drm/xe/display/xe_display_wa.c >> new file mode 100644 >> index 000000000000..68e3d1959ad6 >> --- /dev/null >> +++ b/drivers/gpu/drm/xe/display/xe_display_wa.c >> @@ -0,0 +1,16 @@ >> +// SPDX-License-Identifier: MIT >> +/* >> + * Copyright © 2024 Intel Corporation >> + */ >> + >> +#include "intel_display_wa.h" >> + >> +#include "xe_device.h" >> +#include "xe_wa.h" >> + >> +#include <generated/xe_wa_oob.h> >> + >> +bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915) >> +{ >> + return XE_WA(xe_root_mmio_gt(i915), 16023588340); >> +}
On Wed, Jun 19, 2024 at 03:31:27PM +0100, Matthew Auld wrote: > On BMG-G21 we need to disable fbc due to complications around the WA. > > 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: intel-gfx@lists.freedesktop.org > --- > drivers/gpu/drm/i915/display/intel_display_wa.h | 8 ++++++++ > drivers/gpu/drm/i915/display/intel_fbc.c | 6 ++++++ > drivers/gpu/drm/xe/Makefile | 4 +++- > drivers/gpu/drm/xe/display/xe_display_wa.c | 16 ++++++++++++++++ > 4 files changed, 33 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/xe/display/xe_display_wa.c > > diff --git a/drivers/gpu/drm/i915/display/intel_display_wa.h b/drivers/gpu/drm/i915/display/intel_display_wa.h > index 63201d09852c..be644ab6ae00 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_wa.h > +++ b/drivers/gpu/drm/i915/display/intel_display_wa.h > @@ -6,8 +6,16 @@ > #ifndef __INTEL_DISPLAY_WA_H__ > #define __INTEL_DISPLAY_WA_H__ > > +#include <linux/types.h> > + > struct drm_i915_private; > > void intel_display_wa_apply(struct drm_i915_private *i915); > > +#ifdef I915 > +static inline bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915) { return false; } > +#else > +bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915); > +#endif please avoid the ifdef I915 in new patches as we are trying to get away from that in favor of a clean separation. > + > #endif > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > index 67116c9f1464..8488f82143a4 100644 > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > @@ -56,6 +56,7 @@ > #include "intel_display_device.h" > #include "intel_display_trace.h" > #include "intel_display_types.h" > +#include "intel_display_wa.h" > #include "intel_fbc.h" > #include "intel_fbc_regs.h" > #include "intel_frontbuffer.h" > @@ -1237,6 +1238,11 @@ static int intel_fbc_check_plane(struct intel_atomic_state *state, > return 0; > } > > + if (intel_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/xe/Makefile b/drivers/gpu/drm/xe/Makefile > index 0e16e5029081..f7521fd5db4c 100644 > --- a/drivers/gpu/drm/xe/Makefile > +++ b/drivers/gpu/drm/xe/Makefile > @@ -34,7 +34,8 @@ uses_generated_oob := \ > $(obj)/xe_ring_ops.o \ > $(obj)/xe_vm.o \ > $(obj)/xe_wa.o \ > - $(obj)/xe_ttm_stolen_mgr.o > + $(obj)/xe_ttm_stolen_mgr.o \ > + $(obj)/display/xe_display_wa.o \ > > $(uses_generated_oob): $(generated_oob) > > @@ -192,6 +193,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \ > display/xe_display.o \ > display/xe_display_misc.o \ > display/xe_display_rps.o \ > + display/xe_display_wa.o \ > display/xe_dsb_buffer.o \ > display/xe_fb_pin.o \ > display/xe_hdcp_gsc.o \ > diff --git a/drivers/gpu/drm/xe/display/xe_display_wa.c b/drivers/gpu/drm/xe/display/xe_display_wa.c > new file mode 100644 > index 000000000000..68e3d1959ad6 > --- /dev/null > +++ b/drivers/gpu/drm/xe/display/xe_display_wa.c > @@ -0,0 +1,16 @@ > +// SPDX-License-Identifier: MIT > +/* > + * Copyright © 2024 Intel Corporation > + */ > + > +#include "intel_display_wa.h" > + > +#include "xe_device.h" > +#include "xe_wa.h" > + > +#include <generated/xe_wa_oob.h> > + > +bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915) > +{ > + return XE_WA(xe_root_mmio_gt(i915), 16023588340); > +} > -- > 2.45.1 >
On Wed, Jun 26, 2024 at 04:14:02PM +0100, Matthew Auld wrote: > On 25/06/2024 09:09, Matthew Auld wrote: > > On 19/06/2024 15:31, Matthew Auld wrote: > > > On BMG-G21 we need to disable fbc due to complications around the WA. > > > > > > 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: intel-gfx@lists.freedesktop.org > > Can this be merged via drm-xe-next? The first patch is the xe centric > > part of the WA, but here this is touching i915 display and xe. > > Ping? Can this be landed via drm-xe-next? Lucas, Jani? I'm afraid this patch-2 only submission to intel-gfx didn't trigger the i915 CI. probably good to submit the entire series to both mailing lists so we get both CIs? > > > > > > --- > > > drivers/gpu/drm/i915/display/intel_display_wa.h | 8 ++++++++ > > > drivers/gpu/drm/i915/display/intel_fbc.c | 6 ++++++ > > > drivers/gpu/drm/xe/Makefile | 4 +++- > > > drivers/gpu/drm/xe/display/xe_display_wa.c | 16 ++++++++++++++++ > > > 4 files changed, 33 insertions(+), 1 deletion(-) > > > create mode 100644 drivers/gpu/drm/xe/display/xe_display_wa.c > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_wa.h > > > b/drivers/gpu/drm/i915/display/intel_display_wa.h > > > index 63201d09852c..be644ab6ae00 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_wa.h > > > +++ b/drivers/gpu/drm/i915/display/intel_display_wa.h > > > @@ -6,8 +6,16 @@ > > > #ifndef __INTEL_DISPLAY_WA_H__ > > > #define __INTEL_DISPLAY_WA_H__ > > > +#include <linux/types.h> > > > + > > > struct drm_i915_private; > > > void intel_display_wa_apply(struct drm_i915_private *i915); > > > +#ifdef I915 > > > +static inline bool intel_display_needs_wa_16023588340(struct > > > drm_i915_private *i915) { return false; } > > > +#else > > > +bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915); > > > +#endif > > > + > > > #endif > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c > > > b/drivers/gpu/drm/i915/display/intel_fbc.c > > > index 67116c9f1464..8488f82143a4 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > > @@ -56,6 +56,7 @@ > > > #include "intel_display_device.h" > > > #include "intel_display_trace.h" > > > #include "intel_display_types.h" > > > +#include "intel_display_wa.h" > > > #include "intel_fbc.h" > > > #include "intel_fbc_regs.h" > > > #include "intel_frontbuffer.h" > > > @@ -1237,6 +1238,11 @@ static int intel_fbc_check_plane(struct > > > intel_atomic_state *state, > > > return 0; > > > } > > > + if (intel_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/xe/Makefile b/drivers/gpu/drm/xe/Makefile > > > index 0e16e5029081..f7521fd5db4c 100644 > > > --- a/drivers/gpu/drm/xe/Makefile > > > +++ b/drivers/gpu/drm/xe/Makefile > > > @@ -34,7 +34,8 @@ uses_generated_oob := \ > > > $(obj)/xe_ring_ops.o \ > > > $(obj)/xe_vm.o \ > > > $(obj)/xe_wa.o \ > > > - $(obj)/xe_ttm_stolen_mgr.o > > > + $(obj)/xe_ttm_stolen_mgr.o \ > > > + $(obj)/display/xe_display_wa.o \ > > > $(uses_generated_oob): $(generated_oob) > > > @@ -192,6 +193,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \ > > > display/xe_display.o \ > > > display/xe_display_misc.o \ > > > display/xe_display_rps.o \ > > > + display/xe_display_wa.o \ > > > display/xe_dsb_buffer.o \ > > > display/xe_fb_pin.o \ > > > display/xe_hdcp_gsc.o \ > > > diff --git a/drivers/gpu/drm/xe/display/xe_display_wa.c > > > b/drivers/gpu/drm/xe/display/xe_display_wa.c > > > new file mode 100644 > > > index 000000000000..68e3d1959ad6 > > > --- /dev/null > > > +++ b/drivers/gpu/drm/xe/display/xe_display_wa.c > > > @@ -0,0 +1,16 @@ > > > +// SPDX-License-Identifier: MIT > > > +/* > > > + * Copyright © 2024 Intel Corporation > > > + */ > > > + > > > +#include "intel_display_wa.h" > > > + > > > +#include "xe_device.h" > > > +#include "xe_wa.h" > > > + > > > +#include <generated/xe_wa_oob.h> > > > + > > > +bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915) > > > +{ > > > + return XE_WA(xe_root_mmio_gt(i915), 16023588340); > > > +}
On 26/06/2024 16:53, Rodrigo Vivi wrote: > On Wed, Jun 19, 2024 at 03:31:27PM +0100, Matthew Auld wrote: >> On BMG-G21 we need to disable fbc due to complications around the WA. >> >> 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: intel-gfx@lists.freedesktop.org >> --- >> drivers/gpu/drm/i915/display/intel_display_wa.h | 8 ++++++++ >> drivers/gpu/drm/i915/display/intel_fbc.c | 6 ++++++ >> drivers/gpu/drm/xe/Makefile | 4 +++- >> drivers/gpu/drm/xe/display/xe_display_wa.c | 16 ++++++++++++++++ >> 4 files changed, 33 insertions(+), 1 deletion(-) >> create mode 100644 drivers/gpu/drm/xe/display/xe_display_wa.c >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_wa.h b/drivers/gpu/drm/i915/display/intel_display_wa.h >> index 63201d09852c..be644ab6ae00 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display_wa.h >> +++ b/drivers/gpu/drm/i915/display/intel_display_wa.h >> @@ -6,8 +6,16 @@ >> #ifndef __INTEL_DISPLAY_WA_H__ >> #define __INTEL_DISPLAY_WA_H__ >> >> +#include <linux/types.h> >> + >> struct drm_i915_private; >> >> void intel_display_wa_apply(struct drm_i915_private *i915); >> >> +#ifdef I915 >> +static inline bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915) { return false; } >> +#else >> +bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915); >> +#endif > > please avoid the ifdef I915 in new patches as we are trying to get away from that > in favor of a clean separation. Can you please share an example for the best way to do that here, with clean separation? I can add a new .c just for intel_display_needs_wa_16023588340 and move it there, which then avoids the ifdef I think, but that then adds an entirely new file just for this tiny stub. Unless I can dump it somewhere else? > >> + >> #endif >> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c >> index 67116c9f1464..8488f82143a4 100644 >> --- a/drivers/gpu/drm/i915/display/intel_fbc.c >> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c >> @@ -56,6 +56,7 @@ >> #include "intel_display_device.h" >> #include "intel_display_trace.h" >> #include "intel_display_types.h" >> +#include "intel_display_wa.h" >> #include "intel_fbc.h" >> #include "intel_fbc_regs.h" >> #include "intel_frontbuffer.h" >> @@ -1237,6 +1238,11 @@ static int intel_fbc_check_plane(struct intel_atomic_state *state, >> return 0; >> } >> >> + if (intel_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/xe/Makefile b/drivers/gpu/drm/xe/Makefile >> index 0e16e5029081..f7521fd5db4c 100644 >> --- a/drivers/gpu/drm/xe/Makefile >> +++ b/drivers/gpu/drm/xe/Makefile >> @@ -34,7 +34,8 @@ uses_generated_oob := \ >> $(obj)/xe_ring_ops.o \ >> $(obj)/xe_vm.o \ >> $(obj)/xe_wa.o \ >> - $(obj)/xe_ttm_stolen_mgr.o >> + $(obj)/xe_ttm_stolen_mgr.o \ >> + $(obj)/display/xe_display_wa.o \ >> >> $(uses_generated_oob): $(generated_oob) >> >> @@ -192,6 +193,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \ >> display/xe_display.o \ >> display/xe_display_misc.o \ >> display/xe_display_rps.o \ >> + display/xe_display_wa.o \ >> display/xe_dsb_buffer.o \ >> display/xe_fb_pin.o \ >> display/xe_hdcp_gsc.o \ >> diff --git a/drivers/gpu/drm/xe/display/xe_display_wa.c b/drivers/gpu/drm/xe/display/xe_display_wa.c >> new file mode 100644 >> index 000000000000..68e3d1959ad6 >> --- /dev/null >> +++ b/drivers/gpu/drm/xe/display/xe_display_wa.c >> @@ -0,0 +1,16 @@ >> +// SPDX-License-Identifier: MIT >> +/* >> + * Copyright © 2024 Intel Corporation >> + */ >> + >> +#include "intel_display_wa.h" >> + >> +#include "xe_device.h" >> +#include "xe_wa.h" >> + >> +#include <generated/xe_wa_oob.h> >> + >> +bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915) >> +{ >> + return XE_WA(xe_root_mmio_gt(i915), 16023588340); >> +} >> -- >> 2.45.1 >>
On 26/06/2024 16:55, Rodrigo Vivi wrote: > On Wed, Jun 26, 2024 at 04:14:02PM +0100, Matthew Auld wrote: >> On 25/06/2024 09:09, Matthew Auld wrote: >>> On 19/06/2024 15:31, Matthew Auld wrote: >>>> On BMG-G21 we need to disable fbc due to complications around the WA. >>>> >>>> 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: intel-gfx@lists.freedesktop.org >>> Can this be merged via drm-xe-next? The first patch is the xe centric >>> part of the WA, but here this is touching i915 display and xe. >> >> Ping? Can this be landed via drm-xe-next? Lucas, Jani? > > I'm afraid this patch-2 only submission to intel-gfx didn't trigger > the i915 CI. probably good to submit the entire series to both mailing > lists so we get both CIs? Yes, I missed that. > >> >>> >>>> --- >>>> drivers/gpu/drm/i915/display/intel_display_wa.h | 8 ++++++++ >>>> drivers/gpu/drm/i915/display/intel_fbc.c | 6 ++++++ >>>> drivers/gpu/drm/xe/Makefile | 4 +++- >>>> drivers/gpu/drm/xe/display/xe_display_wa.c | 16 ++++++++++++++++ >>>> 4 files changed, 33 insertions(+), 1 deletion(-) >>>> create mode 100644 drivers/gpu/drm/xe/display/xe_display_wa.c >>>> >>>> diff --git a/drivers/gpu/drm/i915/display/intel_display_wa.h >>>> b/drivers/gpu/drm/i915/display/intel_display_wa.h >>>> index 63201d09852c..be644ab6ae00 100644 >>>> --- a/drivers/gpu/drm/i915/display/intel_display_wa.h >>>> +++ b/drivers/gpu/drm/i915/display/intel_display_wa.h >>>> @@ -6,8 +6,16 @@ >>>> #ifndef __INTEL_DISPLAY_WA_H__ >>>> #define __INTEL_DISPLAY_WA_H__ >>>> +#include <linux/types.h> >>>> + >>>> struct drm_i915_private; >>>> void intel_display_wa_apply(struct drm_i915_private *i915); >>>> +#ifdef I915 >>>> +static inline bool intel_display_needs_wa_16023588340(struct >>>> drm_i915_private *i915) { return false; } >>>> +#else >>>> +bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915); >>>> +#endif >>>> + >>>> #endif >>>> diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c >>>> b/drivers/gpu/drm/i915/display/intel_fbc.c >>>> index 67116c9f1464..8488f82143a4 100644 >>>> --- a/drivers/gpu/drm/i915/display/intel_fbc.c >>>> +++ b/drivers/gpu/drm/i915/display/intel_fbc.c >>>> @@ -56,6 +56,7 @@ >>>> #include "intel_display_device.h" >>>> #include "intel_display_trace.h" >>>> #include "intel_display_types.h" >>>> +#include "intel_display_wa.h" >>>> #include "intel_fbc.h" >>>> #include "intel_fbc_regs.h" >>>> #include "intel_frontbuffer.h" >>>> @@ -1237,6 +1238,11 @@ static int intel_fbc_check_plane(struct >>>> intel_atomic_state *state, >>>> return 0; >>>> } >>>> + if (intel_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/xe/Makefile b/drivers/gpu/drm/xe/Makefile >>>> index 0e16e5029081..f7521fd5db4c 100644 >>>> --- a/drivers/gpu/drm/xe/Makefile >>>> +++ b/drivers/gpu/drm/xe/Makefile >>>> @@ -34,7 +34,8 @@ uses_generated_oob := \ >>>> $(obj)/xe_ring_ops.o \ >>>> $(obj)/xe_vm.o \ >>>> $(obj)/xe_wa.o \ >>>> - $(obj)/xe_ttm_stolen_mgr.o >>>> + $(obj)/xe_ttm_stolen_mgr.o \ >>>> + $(obj)/display/xe_display_wa.o \ >>>> $(uses_generated_oob): $(generated_oob) >>>> @@ -192,6 +193,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \ >>>> display/xe_display.o \ >>>> display/xe_display_misc.o \ >>>> display/xe_display_rps.o \ >>>> + display/xe_display_wa.o \ >>>> display/xe_dsb_buffer.o \ >>>> display/xe_fb_pin.o \ >>>> display/xe_hdcp_gsc.o \ >>>> diff --git a/drivers/gpu/drm/xe/display/xe_display_wa.c >>>> b/drivers/gpu/drm/xe/display/xe_display_wa.c >>>> new file mode 100644 >>>> index 000000000000..68e3d1959ad6 >>>> --- /dev/null >>>> +++ b/drivers/gpu/drm/xe/display/xe_display_wa.c >>>> @@ -0,0 +1,16 @@ >>>> +// SPDX-License-Identifier: MIT >>>> +/* >>>> + * Copyright © 2024 Intel Corporation >>>> + */ >>>> + >>>> +#include "intel_display_wa.h" >>>> + >>>> +#include "xe_device.h" >>>> +#include "xe_wa.h" >>>> + >>>> +#include <generated/xe_wa_oob.h> >>>> + >>>> +bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915) >>>> +{ >>>> + return XE_WA(xe_root_mmio_gt(i915), 16023588340); >>>> +}
On Wed, Jun 26, 2024 at 05:17:41PM +0100, Matthew Auld wrote: > On 26/06/2024 16:53, Rodrigo Vivi wrote: > > On Wed, Jun 19, 2024 at 03:31:27PM +0100, Matthew Auld wrote: > > > On BMG-G21 we need to disable fbc due to complications around the WA. > > > > > > 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: intel-gfx@lists.freedesktop.org > > > --- > > > drivers/gpu/drm/i915/display/intel_display_wa.h | 8 ++++++++ > > > drivers/gpu/drm/i915/display/intel_fbc.c | 6 ++++++ > > > drivers/gpu/drm/xe/Makefile | 4 +++- > > > drivers/gpu/drm/xe/display/xe_display_wa.c | 16 ++++++++++++++++ > > > 4 files changed, 33 insertions(+), 1 deletion(-) > > > create mode 100644 drivers/gpu/drm/xe/display/xe_display_wa.c > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_wa.h b/drivers/gpu/drm/i915/display/intel_display_wa.h > > > index 63201d09852c..be644ab6ae00 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_wa.h > > > +++ b/drivers/gpu/drm/i915/display/intel_display_wa.h > > > @@ -6,8 +6,16 @@ > > > #ifndef __INTEL_DISPLAY_WA_H__ > > > #define __INTEL_DISPLAY_WA_H__ > > > +#include <linux/types.h> > > > + > > > struct drm_i915_private; > > > void intel_display_wa_apply(struct drm_i915_private *i915); > > > +#ifdef I915 > > > +static inline bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915) { return false; } > > > +#else > > > +bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915); > > > +#endif > > > > please avoid the ifdef I915 in new patches as we are trying to get away from that > > in favor of a clean separation. > > Can you please share an example for the best way to do that here, with clean > separation? hmmm... looking more to the patch now... I don't believe that the WA/RTP rule from Xe should leak into i915 to be honest. It looks like we are trending to a separate intel-display.ko that shouldn't depend on driver's declarations like this. Ideally I would also say that wa in the display code should relly on the 'D' (display-id) of the GMD-ID. But I see that this 16023588340 is for the 'G' ip. So, perhaps the display code should inspect the 'G' id from the device inside display code? Jani, thoughts on this? > > I can add a new .c just for intel_display_needs_wa_16023588340 and move it > there, which then avoids the ifdef I think, but that then adds an entirely > new file just for this tiny stub. Unless I can dump it somewhere else? One temporary workaround that I see without the ifdef I915 would be to declare this function in i915_drv.h so in xe you add to the compat-i915-headers instead of creating a new file there. > > > > > > + > > > #endif > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > > > index 67116c9f1464..8488f82143a4 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > > @@ -56,6 +56,7 @@ > > > #include "intel_display_device.h" > > > #include "intel_display_trace.h" > > > #include "intel_display_types.h" > > > +#include "intel_display_wa.h" > > > #include "intel_fbc.h" > > > #include "intel_fbc_regs.h" > > > #include "intel_frontbuffer.h" > > > @@ -1237,6 +1238,11 @@ static int intel_fbc_check_plane(struct intel_atomic_state *state, > > > return 0; > > > } > > > + if (intel_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/xe/Makefile b/drivers/gpu/drm/xe/Makefile > > > index 0e16e5029081..f7521fd5db4c 100644 > > > --- a/drivers/gpu/drm/xe/Makefile > > > +++ b/drivers/gpu/drm/xe/Makefile > > > @@ -34,7 +34,8 @@ uses_generated_oob := \ > > > $(obj)/xe_ring_ops.o \ > > > $(obj)/xe_vm.o \ > > > $(obj)/xe_wa.o \ > > > - $(obj)/xe_ttm_stolen_mgr.o > > > + $(obj)/xe_ttm_stolen_mgr.o \ > > > + $(obj)/display/xe_display_wa.o \ > > > $(uses_generated_oob): $(generated_oob) > > > @@ -192,6 +193,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \ > > > display/xe_display.o \ > > > display/xe_display_misc.o \ > > > display/xe_display_rps.o \ > > > + display/xe_display_wa.o \ > > > display/xe_dsb_buffer.o \ > > > display/xe_fb_pin.o \ > > > display/xe_hdcp_gsc.o \ > > > diff --git a/drivers/gpu/drm/xe/display/xe_display_wa.c b/drivers/gpu/drm/xe/display/xe_display_wa.c > > > new file mode 100644 > > > index 000000000000..68e3d1959ad6 > > > --- /dev/null > > > +++ b/drivers/gpu/drm/xe/display/xe_display_wa.c > > > @@ -0,0 +1,16 @@ > > > +// SPDX-License-Identifier: MIT > > > +/* > > > + * Copyright © 2024 Intel Corporation > > > + */ > > > + > > > +#include "intel_display_wa.h" > > > + > > > +#include "xe_device.h" > > > +#include "xe_wa.h" > > > + > > > +#include <generated/xe_wa_oob.h> > > > + > > > +bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915) > > > +{ > > > + return XE_WA(xe_root_mmio_gt(i915), 16023588340); > > > +} > > > -- > > > 2.45.1 > > >
On Wed, Jun 26, 2024 at 01:26:00PM -0400, Rodrigo Vivi wrote: > On Wed, Jun 26, 2024 at 05:17:41PM +0100, Matthew Auld wrote: > > On 26/06/2024 16:53, Rodrigo Vivi wrote: > > > On Wed, Jun 19, 2024 at 03:31:27PM +0100, Matthew Auld wrote: > > > > On BMG-G21 we need to disable fbc due to complications around the WA. > > > > > > > > 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: intel-gfx@lists.freedesktop.org > > > > --- > > > > drivers/gpu/drm/i915/display/intel_display_wa.h | 8 ++++++++ > > > > drivers/gpu/drm/i915/display/intel_fbc.c | 6 ++++++ > > > > drivers/gpu/drm/xe/Makefile | 4 +++- > > > > drivers/gpu/drm/xe/display/xe_display_wa.c | 16 ++++++++++++++++ > > > > 4 files changed, 33 insertions(+), 1 deletion(-) > > > > create mode 100644 drivers/gpu/drm/xe/display/xe_display_wa.c > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_wa.h b/drivers/gpu/drm/i915/display/intel_display_wa.h > > > > index 63201d09852c..be644ab6ae00 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_display_wa.h > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_wa.h > > > > @@ -6,8 +6,16 @@ > > > > #ifndef __INTEL_DISPLAY_WA_H__ > > > > #define __INTEL_DISPLAY_WA_H__ > > > > +#include <linux/types.h> > > > > + > > > > struct drm_i915_private; > > > > void intel_display_wa_apply(struct drm_i915_private *i915); > > > > +#ifdef I915 > > > > +static inline bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915) { return false; } > > > > +#else > > > > +bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915); > > > > +#endif > > > > > > please avoid the ifdef I915 in new patches as we are trying to get away from that > > > in favor of a clean separation. > > > > Can you please share an example for the best way to do that here, with clean > > separation? > > hmmm... looking more to the patch now... > I don't believe that the WA/RTP rule from Xe should leak into i915 to be honest. > > It looks like we are trending to a separate intel-display.ko that shouldn't depend > on driver's declarations like this. > > Ideally I would also say that wa in the display code should relly on the 'D' > (display-id) of the GMD-ID. But I see that this 16023588340 is for the 'G' ip. > So, perhaps the display code should inspect the 'G' id from the device inside > display code? This is one of those rare cases where a GT-based workaround also has a side effect of "oh, and you also need to disable FBC in the display driver." So as you said, the need to disable FBC is entirely tied to details on the graphics side of the IP, not the display version. :-( So there are two options --- either you duplicate the logic to decide whether the workaround applies in both the display driver and the core (i915/Xe) driver, or you make the core driver the authoritative decision maker for GT-based workarounds and give the display driver some way to query the core driver. The patch here is following the latter approach, and for the short term future while display code just gets re-compiled into each core driver, this seems to be an accurate implementation (always false on i915 builds, and querying RTP for Xe builds). As we proceed with moving intel_display into its own standalone driver, we'll need a more formal display<->core driver interface and it will probably make sense to have a formal "query a GT workaround" entrypoint as part of that interface so that we don't need to keep adding more one-off "needs_XXXXX" for future workarounds that wind up in the same boat. Matt > > Jani, thoughts on this? > > > > > I can add a new .c just for intel_display_needs_wa_16023588340 and move it > > there, which then avoids the ifdef I think, but that then adds an entirely > > new file just for this tiny stub. Unless I can dump it somewhere else? > > One temporary workaround that I see without the ifdef I915 would be to > declare this function in i915_drv.h so in xe you add to the compat-i915-headers > instead of creating a new file there. > > > > > > > > > > + > > > > #endif > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > > > > index 67116c9f1464..8488f82143a4 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > > > @@ -56,6 +56,7 @@ > > > > #include "intel_display_device.h" > > > > #include "intel_display_trace.h" > > > > #include "intel_display_types.h" > > > > +#include "intel_display_wa.h" > > > > #include "intel_fbc.h" > > > > #include "intel_fbc_regs.h" > > > > #include "intel_frontbuffer.h" > > > > @@ -1237,6 +1238,11 @@ static int intel_fbc_check_plane(struct intel_atomic_state *state, > > > > return 0; > > > > } > > > > + if (intel_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/xe/Makefile b/drivers/gpu/drm/xe/Makefile > > > > index 0e16e5029081..f7521fd5db4c 100644 > > > > --- a/drivers/gpu/drm/xe/Makefile > > > > +++ b/drivers/gpu/drm/xe/Makefile > > > > @@ -34,7 +34,8 @@ uses_generated_oob := \ > > > > $(obj)/xe_ring_ops.o \ > > > > $(obj)/xe_vm.o \ > > > > $(obj)/xe_wa.o \ > > > > - $(obj)/xe_ttm_stolen_mgr.o > > > > + $(obj)/xe_ttm_stolen_mgr.o \ > > > > + $(obj)/display/xe_display_wa.o \ > > > > $(uses_generated_oob): $(generated_oob) > > > > @@ -192,6 +193,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \ > > > > display/xe_display.o \ > > > > display/xe_display_misc.o \ > > > > display/xe_display_rps.o \ > > > > + display/xe_display_wa.o \ > > > > display/xe_dsb_buffer.o \ > > > > display/xe_fb_pin.o \ > > > > display/xe_hdcp_gsc.o \ > > > > diff --git a/drivers/gpu/drm/xe/display/xe_display_wa.c b/drivers/gpu/drm/xe/display/xe_display_wa.c > > > > new file mode 100644 > > > > index 000000000000..68e3d1959ad6 > > > > --- /dev/null > > > > +++ b/drivers/gpu/drm/xe/display/xe_display_wa.c > > > > @@ -0,0 +1,16 @@ > > > > +// SPDX-License-Identifier: MIT > > > > +/* > > > > + * Copyright © 2024 Intel Corporation > > > > + */ > > > > + > > > > +#include "intel_display_wa.h" > > > > + > > > > +#include "xe_device.h" > > > > +#include "xe_wa.h" > > > > + > > > > +#include <generated/xe_wa_oob.h> > > > > + > > > > +bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915) > > > > +{ > > > > + return XE_WA(xe_root_mmio_gt(i915), 16023588340); > > > > +} > > > > -- > > > > 2.45.1 > > > >
On Wed, Jun 26, 2024 at 10:42:24AM GMT, Matt Roper wrote: >On Wed, Jun 26, 2024 at 01:26:00PM -0400, Rodrigo Vivi wrote: >> On Wed, Jun 26, 2024 at 05:17:41PM +0100, Matthew Auld wrote: >> > On 26/06/2024 16:53, Rodrigo Vivi wrote: >> > > On Wed, Jun 19, 2024 at 03:31:27PM +0100, Matthew Auld wrote: >> > > > On BMG-G21 we need to disable fbc due to complications around the WA. >> > > > >> > > > 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: intel-gfx@lists.freedesktop.org >> > > > --- >> > > > drivers/gpu/drm/i915/display/intel_display_wa.h | 8 ++++++++ >> > > > drivers/gpu/drm/i915/display/intel_fbc.c | 6 ++++++ >> > > > drivers/gpu/drm/xe/Makefile | 4 +++- >> > > > drivers/gpu/drm/xe/display/xe_display_wa.c | 16 ++++++++++++++++ >> > > > 4 files changed, 33 insertions(+), 1 deletion(-) >> > > > create mode 100644 drivers/gpu/drm/xe/display/xe_display_wa.c >> > > > >> > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_wa.h b/drivers/gpu/drm/i915/display/intel_display_wa.h >> > > > index 63201d09852c..be644ab6ae00 100644 >> > > > --- a/drivers/gpu/drm/i915/display/intel_display_wa.h >> > > > +++ b/drivers/gpu/drm/i915/display/intel_display_wa.h >> > > > @@ -6,8 +6,16 @@ >> > > > #ifndef __INTEL_DISPLAY_WA_H__ >> > > > #define __INTEL_DISPLAY_WA_H__ >> > > > +#include <linux/types.h> >> > > > + >> > > > struct drm_i915_private; >> > > > void intel_display_wa_apply(struct drm_i915_private *i915); >> > > > +#ifdef I915 >> > > > +static inline bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915) { return false; } >> > > > +#else >> > > > +bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915); >> > > > +#endif >> > > >> > > please avoid the ifdef I915 in new patches as we are trying to get away from that >> > > in favor of a clean separation. >> > >> > Can you please share an example for the best way to do that here, with clean >> > separation? >> >> hmmm... looking more to the patch now... >> I don't believe that the WA/RTP rule from Xe should leak into i915 to be honest. >> >> It looks like we are trending to a separate intel-display.ko that shouldn't depend >> on driver's declarations like this. >> >> Ideally I would also say that wa in the display code should relly on the 'D' >> (display-id) of the GMD-ID. But I see that this 16023588340 is for the 'G' ip. >> So, perhaps the display code should inspect the 'G' id from the device inside >> display code? > >This is one of those rare cases where a GT-based workaround also has a >side effect of "oh, and you also need to disable FBC in the display >driver." So as you said, the need to disable FBC is entirely tied to >details on the graphics side of the IP, not the display version. :-( > >So there are two options --- either you duplicate the logic to decide >whether the workaround applies in both the display driver and the core >(i915/Xe) driver, or you make the core driver the authoritative decision >maker for GT-based workarounds and give the display driver some way to >query the core driver. The patch here is following the latter approach, >and for the short term future while display code just gets re-compiled >into each core driver, this seems to be an accurate implementation >(always false on i915 builds, and querying RTP for Xe builds). As we >proceed with moving intel_display into its own standalone driver, we'll >need a more formal display<->core driver interface and it will probably >make sense to have a formal "query a GT workaround" entrypoint as part >of that interface so that we don't need to keep adding more one-off >"needs_XXXXX" for future workarounds that wind up in the same boat. agreed. Let's not leak the decision on other places: it belongs in the core driver. When there's the proper separation, then I believe we can just copy the root_gt->wa_active.oob over to the display struct. And then implement a macro on the display side to do the same check. Or we may have a set of function pointers and one of them would be to query if a WA should be enabled. Lucas De Marchi > > >Matt > >> >> Jani, thoughts on this? >> >> > >> > I can add a new .c just for intel_display_needs_wa_16023588340 and move it >> > there, which then avoids the ifdef I think, but that then adds an entirely >> > new file just for this tiny stub. Unless I can dump it somewhere else? >> >> One temporary workaround that I see without the ifdef I915 would be to >> declare this function in i915_drv.h so in xe you add to the compat-i915-headers >> instead of creating a new file there. >> >> > >> > > >> > > > + >> > > > #endif >> > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c >> > > > index 67116c9f1464..8488f82143a4 100644 >> > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c >> > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c >> > > > @@ -56,6 +56,7 @@ >> > > > #include "intel_display_device.h" >> > > > #include "intel_display_trace.h" >> > > > #include "intel_display_types.h" >> > > > +#include "intel_display_wa.h" >> > > > #include "intel_fbc.h" >> > > > #include "intel_fbc_regs.h" >> > > > #include "intel_frontbuffer.h" >> > > > @@ -1237,6 +1238,11 @@ static int intel_fbc_check_plane(struct intel_atomic_state *state, >> > > > return 0; >> > > > } >> > > > + if (intel_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/xe/Makefile b/drivers/gpu/drm/xe/Makefile >> > > > index 0e16e5029081..f7521fd5db4c 100644 >> > > > --- a/drivers/gpu/drm/xe/Makefile >> > > > +++ b/drivers/gpu/drm/xe/Makefile >> > > > @@ -34,7 +34,8 @@ uses_generated_oob := \ >> > > > $(obj)/xe_ring_ops.o \ >> > > > $(obj)/xe_vm.o \ >> > > > $(obj)/xe_wa.o \ >> > > > - $(obj)/xe_ttm_stolen_mgr.o >> > > > + $(obj)/xe_ttm_stolen_mgr.o \ >> > > > + $(obj)/display/xe_display_wa.o \ >> > > > $(uses_generated_oob): $(generated_oob) >> > > > @@ -192,6 +193,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \ >> > > > display/xe_display.o \ >> > > > display/xe_display_misc.o \ >> > > > display/xe_display_rps.o \ >> > > > + display/xe_display_wa.o \ >> > > > display/xe_dsb_buffer.o \ >> > > > display/xe_fb_pin.o \ >> > > > display/xe_hdcp_gsc.o \ >> > > > diff --git a/drivers/gpu/drm/xe/display/xe_display_wa.c b/drivers/gpu/drm/xe/display/xe_display_wa.c >> > > > new file mode 100644 >> > > > index 000000000000..68e3d1959ad6 >> > > > --- /dev/null >> > > > +++ b/drivers/gpu/drm/xe/display/xe_display_wa.c >> > > > @@ -0,0 +1,16 @@ >> > > > +// SPDX-License-Identifier: MIT >> > > > +/* >> > > > + * Copyright © 2024 Intel Corporation >> > > > + */ >> > > > + >> > > > +#include "intel_display_wa.h" >> > > > + >> > > > +#include "xe_device.h" >> > > > +#include "xe_wa.h" >> > > > + >> > > > +#include <generated/xe_wa_oob.h> >> > > > + >> > > > +bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915) >> > > > +{ >> > > > + return XE_WA(xe_root_mmio_gt(i915), 16023588340); >> > > > +} >> > > > -- >> > > > 2.45.1 >> > > > > >-- >Matt Roper >Graphics Software Engineer >Linux GPU Platform Enablement >Intel Corporation
On Fri, Jun 28, 2024 at 12:30:57AM -0500, Lucas De Marchi wrote: > On Wed, Jun 26, 2024 at 10:42:24AM GMT, Matt Roper wrote: > > On Wed, Jun 26, 2024 at 01:26:00PM -0400, Rodrigo Vivi wrote: > > > On Wed, Jun 26, 2024 at 05:17:41PM +0100, Matthew Auld wrote: > > > > On 26/06/2024 16:53, Rodrigo Vivi wrote: > > > > > On Wed, Jun 19, 2024 at 03:31:27PM +0100, Matthew Auld wrote: > > > > > > On BMG-G21 we need to disable fbc due to complications around the WA. > > > > > > > > > > > > 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: intel-gfx@lists.freedesktop.org > > > > > > --- > > > > > > drivers/gpu/drm/i915/display/intel_display_wa.h | 8 ++++++++ > > > > > > drivers/gpu/drm/i915/display/intel_fbc.c | 6 ++++++ > > > > > > drivers/gpu/drm/xe/Makefile | 4 +++- > > > > > > drivers/gpu/drm/xe/display/xe_display_wa.c | 16 ++++++++++++++++ > > > > > > 4 files changed, 33 insertions(+), 1 deletion(-) > > > > > > create mode 100644 drivers/gpu/drm/xe/display/xe_display_wa.c > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_wa.h b/drivers/gpu/drm/i915/display/intel_display_wa.h > > > > > > index 63201d09852c..be644ab6ae00 100644 > > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_wa.h > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_wa.h > > > > > > @@ -6,8 +6,16 @@ > > > > > > #ifndef __INTEL_DISPLAY_WA_H__ > > > > > > #define __INTEL_DISPLAY_WA_H__ > > > > > > +#include <linux/types.h> > > > > > > + > > > > > > struct drm_i915_private; > > > > > > void intel_display_wa_apply(struct drm_i915_private *i915); > > > > > > +#ifdef I915 > > > > > > +static inline bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915) { return false; } > > > > > > +#else > > > > > > +bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915); > > > > > > +#endif > > > > > > > > > > please avoid the ifdef I915 in new patches as we are trying to get away from that > > > > > in favor of a clean separation. > > > > > > > > Can you please share an example for the best way to do that here, with clean > > > > separation? > > > > > > hmmm... looking more to the patch now... > > > I don't believe that the WA/RTP rule from Xe should leak into i915 to be honest. > > > > > > It looks like we are trending to a separate intel-display.ko that shouldn't depend > > > on driver's declarations like this. > > > > > > Ideally I would also say that wa in the display code should relly on the 'D' > > > (display-id) of the GMD-ID. But I see that this 16023588340 is for the 'G' ip. > > > So, perhaps the display code should inspect the 'G' id from the device inside > > > display code? > > > > This is one of those rare cases where a GT-based workaround also has a > > side effect of "oh, and you also need to disable FBC in the display > > driver." So as you said, the need to disable FBC is entirely tied to > > details on the graphics side of the IP, not the display version. :-( > > > > So there are two options --- either you duplicate the logic to decide > > whether the workaround applies in both the display driver and the core > > (i915/Xe) driver, or you make the core driver the authoritative decision > > maker for GT-based workarounds and give the display driver some way to > > query the core driver. The patch here is following the latter approach, > > and for the short term future while display code just gets re-compiled > > into each core driver, this seems to be an accurate implementation > > (always false on i915 builds, and querying RTP for Xe builds). As we > > proceed with moving intel_display into its own standalone driver, we'll > > need a more formal display<->core driver interface and it will probably > > make sense to have a formal "query a GT workaround" entrypoint as part > > of that interface so that we don't need to keep adding more one-off > > "needs_XXXXX" for future workarounds that wind up in the same boat. > > agreed. Let's not leak the decision on other places: it belongs in the > core driver. > > When there's the proper separation, then I believe we can just copy the > root_gt->wa_active.oob over to the display struct. And then implement a > macro on the display side to do the same check. Or we may have a set of > function pointers and one of them would be to query if a WA should be > enabled. Fair enough. Perhaps we could at least define this in i915_drv.h so we implement in the compat headers and avoid the ifdef I915? But anyway, if this is something that will remaing for later for the separation perhaps one extra ifdef doesn't hurt. Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Lucas De Marchi > > > > > > > Matt > > > > > > > > Jani, thoughts on this? > > > > > > > > > > > I can add a new .c just for intel_display_needs_wa_16023588340 and move it > > > > there, which then avoids the ifdef I think, but that then adds an entirely > > > > new file just for this tiny stub. Unless I can dump it somewhere else? > > > > > > One temporary workaround that I see without the ifdef I915 would be to > > > declare this function in i915_drv.h so in xe you add to the compat-i915-headers > > > instead of creating a new file there. > > > > > > > > > > > > > > > > > > + > > > > > > #endif > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > > > > > > index 67116c9f1464..8488f82143a4 100644 > > > > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > > > > > @@ -56,6 +56,7 @@ > > > > > > #include "intel_display_device.h" > > > > > > #include "intel_display_trace.h" > > > > > > #include "intel_display_types.h" > > > > > > +#include "intel_display_wa.h" > > > > > > #include "intel_fbc.h" > > > > > > #include "intel_fbc_regs.h" > > > > > > #include "intel_frontbuffer.h" > > > > > > @@ -1237,6 +1238,11 @@ static int intel_fbc_check_plane(struct intel_atomic_state *state, > > > > > > return 0; > > > > > > } > > > > > > + if (intel_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/xe/Makefile b/drivers/gpu/drm/xe/Makefile > > > > > > index 0e16e5029081..f7521fd5db4c 100644 > > > > > > --- a/drivers/gpu/drm/xe/Makefile > > > > > > +++ b/drivers/gpu/drm/xe/Makefile > > > > > > @@ -34,7 +34,8 @@ uses_generated_oob := \ > > > > > > $(obj)/xe_ring_ops.o \ > > > > > > $(obj)/xe_vm.o \ > > > > > > $(obj)/xe_wa.o \ > > > > > > - $(obj)/xe_ttm_stolen_mgr.o > > > > > > + $(obj)/xe_ttm_stolen_mgr.o \ > > > > > > + $(obj)/display/xe_display_wa.o \ > > > > > > $(uses_generated_oob): $(generated_oob) > > > > > > @@ -192,6 +193,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \ > > > > > > display/xe_display.o \ > > > > > > display/xe_display_misc.o \ > > > > > > display/xe_display_rps.o \ > > > > > > + display/xe_display_wa.o \ > > > > > > display/xe_dsb_buffer.o \ > > > > > > display/xe_fb_pin.o \ > > > > > > display/xe_hdcp_gsc.o \ > > > > > > diff --git a/drivers/gpu/drm/xe/display/xe_display_wa.c b/drivers/gpu/drm/xe/display/xe_display_wa.c > > > > > > new file mode 100644 > > > > > > index 000000000000..68e3d1959ad6 > > > > > > --- /dev/null > > > > > > +++ b/drivers/gpu/drm/xe/display/xe_display_wa.c > > > > > > @@ -0,0 +1,16 @@ > > > > > > +// SPDX-License-Identifier: MIT > > > > > > +/* > > > > > > + * Copyright © 2024 Intel Corporation > > > > > > + */ > > > > > > + > > > > > > +#include "intel_display_wa.h" > > > > > > + > > > > > > +#include "xe_device.h" > > > > > > +#include "xe_wa.h" > > > > > > + > > > > > > +#include <generated/xe_wa_oob.h> > > > > > > + > > > > > > +bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915) > > > > > > +{ > > > > > > + return XE_WA(xe_root_mmio_gt(i915), 16023588340); > > > > > > +} > > > > > > -- > > > > > > 2.45.1 > > > > > > > > > > -- > > Matt Roper > > Graphics Software Engineer > > Linux GPU Platform Enablement > > Intel Corporation
On Fri, Jun 28, 2024 at 02:36:16PM GMT, Rodrigo Vivi wrote: >On Fri, Jun 28, 2024 at 12:30:57AM -0500, Lucas De Marchi wrote: >> On Wed, Jun 26, 2024 at 10:42:24AM GMT, Matt Roper wrote: >> > On Wed, Jun 26, 2024 at 01:26:00PM -0400, Rodrigo Vivi wrote: >> > > On Wed, Jun 26, 2024 at 05:17:41PM +0100, Matthew Auld wrote: >> > > > On 26/06/2024 16:53, Rodrigo Vivi wrote: >> > > > > On Wed, Jun 19, 2024 at 03:31:27PM +0100, Matthew Auld wrote: >> > > > > > On BMG-G21 we need to disable fbc due to complications around the WA. >> > > > > > >> > > > > > 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: intel-gfx@lists.freedesktop.org >> > > > > > --- >> > > > > > drivers/gpu/drm/i915/display/intel_display_wa.h | 8 ++++++++ >> > > > > > drivers/gpu/drm/i915/display/intel_fbc.c | 6 ++++++ >> > > > > > drivers/gpu/drm/xe/Makefile | 4 +++- >> > > > > > drivers/gpu/drm/xe/display/xe_display_wa.c | 16 ++++++++++++++++ >> > > > > > 4 files changed, 33 insertions(+), 1 deletion(-) >> > > > > > create mode 100644 drivers/gpu/drm/xe/display/xe_display_wa.c >> > > > > > >> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_wa.h b/drivers/gpu/drm/i915/display/intel_display_wa.h >> > > > > > index 63201d09852c..be644ab6ae00 100644 >> > > > > > --- a/drivers/gpu/drm/i915/display/intel_display_wa.h >> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_display_wa.h >> > > > > > @@ -6,8 +6,16 @@ >> > > > > > #ifndef __INTEL_DISPLAY_WA_H__ >> > > > > > #define __INTEL_DISPLAY_WA_H__ >> > > > > > +#include <linux/types.h> >> > > > > > + >> > > > > > struct drm_i915_private; >> > > > > > void intel_display_wa_apply(struct drm_i915_private *i915); >> > > > > > +#ifdef I915 >> > > > > > +static inline bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915) { return false; } >> > > > > > +#else >> > > > > > +bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915); >> > > > > > +#endif >> > > > > >> > > > > please avoid the ifdef I915 in new patches as we are trying to get away from that >> > > > > in favor of a clean separation. >> > > > >> > > > Can you please share an example for the best way to do that here, with clean >> > > > separation? >> > > >> > > hmmm... looking more to the patch now... >> > > I don't believe that the WA/RTP rule from Xe should leak into i915 to be honest. >> > > >> > > It looks like we are trending to a separate intel-display.ko that shouldn't depend >> > > on driver's declarations like this. >> > > >> > > Ideally I would also say that wa in the display code should relly on the 'D' >> > > (display-id) of the GMD-ID. But I see that this 16023588340 is for the 'G' ip. >> > > So, perhaps the display code should inspect the 'G' id from the device inside >> > > display code? >> > >> > This is one of those rare cases where a GT-based workaround also has a >> > side effect of "oh, and you also need to disable FBC in the display >> > driver." So as you said, the need to disable FBC is entirely tied to >> > details on the graphics side of the IP, not the display version. :-( >> > >> > So there are two options --- either you duplicate the logic to decide >> > whether the workaround applies in both the display driver and the core >> > (i915/Xe) driver, or you make the core driver the authoritative decision >> > maker for GT-based workarounds and give the display driver some way to >> > query the core driver. The patch here is following the latter approach, >> > and for the short term future while display code just gets re-compiled >> > into each core driver, this seems to be an accurate implementation >> > (always false on i915 builds, and querying RTP for Xe builds). As we >> > proceed with moving intel_display into its own standalone driver, we'll >> > need a more formal display<->core driver interface and it will probably >> > make sense to have a formal "query a GT workaround" entrypoint as part >> > of that interface so that we don't need to keep adding more one-off >> > "needs_XXXXX" for future workarounds that wind up in the same boat. >> >> agreed. Let's not leak the decision on other places: it belongs in the >> core driver. >> >> When there's the proper separation, then I believe we can just copy the >> root_gt->wa_active.oob over to the display struct. And then implement a >> macro on the display side to do the same check. Or we may have a set of >> function pointers and one of them would be to query if a WA should be >> enabled. > >Fair enough. Perhaps we could at least define this in i915_drv.h so we implement >in the compat headers and avoid the ifdef I915? yeah, I'm fine with that approach. Lucas De Marchi > >But anyway, if this is something that will remaing for later for the >separation perhaps one extra ifdef doesn't hurt. > >Acked-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > >> >> Lucas De Marchi >> >> > >> > >> > Matt >> > >> > > >> > > Jani, thoughts on this? >> > > >> > > > >> > > > I can add a new .c just for intel_display_needs_wa_16023588340 and move it >> > > > there, which then avoids the ifdef I think, but that then adds an entirely >> > > > new file just for this tiny stub. Unless I can dump it somewhere else? >> > > >> > > One temporary workaround that I see without the ifdef I915 would be to >> > > declare this function in i915_drv.h so in xe you add to the compat-i915-headers >> > > instead of creating a new file there. >> > > >> > > > >> > > > > >> > > > > > + >> > > > > > #endif >> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c >> > > > > > index 67116c9f1464..8488f82143a4 100644 >> > > > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c >> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c >> > > > > > @@ -56,6 +56,7 @@ >> > > > > > #include "intel_display_device.h" >> > > > > > #include "intel_display_trace.h" >> > > > > > #include "intel_display_types.h" >> > > > > > +#include "intel_display_wa.h" >> > > > > > #include "intel_fbc.h" >> > > > > > #include "intel_fbc_regs.h" >> > > > > > #include "intel_frontbuffer.h" >> > > > > > @@ -1237,6 +1238,11 @@ static int intel_fbc_check_plane(struct intel_atomic_state *state, >> > > > > > return 0; >> > > > > > } >> > > > > > + if (intel_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/xe/Makefile b/drivers/gpu/drm/xe/Makefile >> > > > > > index 0e16e5029081..f7521fd5db4c 100644 >> > > > > > --- a/drivers/gpu/drm/xe/Makefile >> > > > > > +++ b/drivers/gpu/drm/xe/Makefile >> > > > > > @@ -34,7 +34,8 @@ uses_generated_oob := \ >> > > > > > $(obj)/xe_ring_ops.o \ >> > > > > > $(obj)/xe_vm.o \ >> > > > > > $(obj)/xe_wa.o \ >> > > > > > - $(obj)/xe_ttm_stolen_mgr.o >> > > > > > + $(obj)/xe_ttm_stolen_mgr.o \ >> > > > > > + $(obj)/display/xe_display_wa.o \ >> > > > > > $(uses_generated_oob): $(generated_oob) >> > > > > > @@ -192,6 +193,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \ >> > > > > > display/xe_display.o \ >> > > > > > display/xe_display_misc.o \ >> > > > > > display/xe_display_rps.o \ >> > > > > > + display/xe_display_wa.o \ >> > > > > > display/xe_dsb_buffer.o \ >> > > > > > display/xe_fb_pin.o \ >> > > > > > display/xe_hdcp_gsc.o \ >> > > > > > diff --git a/drivers/gpu/drm/xe/display/xe_display_wa.c b/drivers/gpu/drm/xe/display/xe_display_wa.c >> > > > > > new file mode 100644 >> > > > > > index 000000000000..68e3d1959ad6 >> > > > > > --- /dev/null >> > > > > > +++ b/drivers/gpu/drm/xe/display/xe_display_wa.c >> > > > > > @@ -0,0 +1,16 @@ >> > > > > > +// SPDX-License-Identifier: MIT >> > > > > > +/* >> > > > > > + * Copyright © 2024 Intel Corporation >> > > > > > + */ >> > > > > > + >> > > > > > +#include "intel_display_wa.h" >> > > > > > + >> > > > > > +#include "xe_device.h" >> > > > > > +#include "xe_wa.h" >> > > > > > + >> > > > > > +#include <generated/xe_wa_oob.h> >> > > > > > + >> > > > > > +bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915) >> > > > > > +{ >> > > > > > + return XE_WA(xe_root_mmio_gt(i915), 16023588340); >> > > > > > +} >> > > > > > -- >> > > > > > 2.45.1 >> > > > > > >> > >> > -- >> > Matt Roper >> > Graphics Software Engineer >> > Linux GPU Platform Enablement >> > Intel Corporation
On Wed, 19 Jun 2024, Matthew Auld <matthew.auld@intel.com> wrote: > diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile > index 0e16e5029081..f7521fd5db4c 100644 > --- a/drivers/gpu/drm/xe/Makefile > +++ b/drivers/gpu/drm/xe/Makefile > @@ -34,7 +34,8 @@ uses_generated_oob := \ > $(obj)/xe_ring_ops.o \ > $(obj)/xe_vm.o \ > $(obj)/xe_wa.o \ > - $(obj)/xe_ttm_stolen_mgr.o > + $(obj)/xe_ttm_stolen_mgr.o \ > + $(obj)/display/xe_display_wa.o \ There's an extra \ there. BR, Jani.
On Fri, 28 Jun 2024, Lucas De Marchi <lucas.demarchi@intel.com> wrote: > On Fri, Jun 28, 2024 at 02:36:16PM GMT, Rodrigo Vivi wrote: >>Fair enough. Perhaps we could at least define this in i915_drv.h so we implement >>in the compat headers and avoid the ifdef I915? > > yeah, I'm fine with that approach. Based on Lucas' feedback [1] on v2, looks like we'll need the .c file anyway. Whether that's then wrapped in intel_display_wa.h with #ifdef I915, or i915_drv.h, up to you. Also, for the record, Acked-by: Jani Nikula <jani.nikula@intel.com> on merging this via drm-xe-next. BR, Jani. [1] https://lore.kernel.org/r/6hq2htqmbjjhrdad3jbgsesvteqe3g65hpznzsyk6bxj42iowq@my4rit2pa4sm
diff --git a/drivers/gpu/drm/i915/display/intel_display_wa.h b/drivers/gpu/drm/i915/display/intel_display_wa.h index 63201d09852c..be644ab6ae00 100644 --- a/drivers/gpu/drm/i915/display/intel_display_wa.h +++ b/drivers/gpu/drm/i915/display/intel_display_wa.h @@ -6,8 +6,16 @@ #ifndef __INTEL_DISPLAY_WA_H__ #define __INTEL_DISPLAY_WA_H__ +#include <linux/types.h> + struct drm_i915_private; void intel_display_wa_apply(struct drm_i915_private *i915); +#ifdef I915 +static inline bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915) { return false; } +#else +bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915); +#endif + #endif diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c index 67116c9f1464..8488f82143a4 100644 --- a/drivers/gpu/drm/i915/display/intel_fbc.c +++ b/drivers/gpu/drm/i915/display/intel_fbc.c @@ -56,6 +56,7 @@ #include "intel_display_device.h" #include "intel_display_trace.h" #include "intel_display_types.h" +#include "intel_display_wa.h" #include "intel_fbc.h" #include "intel_fbc_regs.h" #include "intel_frontbuffer.h" @@ -1237,6 +1238,11 @@ static int intel_fbc_check_plane(struct intel_atomic_state *state, return 0; } + if (intel_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/xe/Makefile b/drivers/gpu/drm/xe/Makefile index 0e16e5029081..f7521fd5db4c 100644 --- a/drivers/gpu/drm/xe/Makefile +++ b/drivers/gpu/drm/xe/Makefile @@ -34,7 +34,8 @@ uses_generated_oob := \ $(obj)/xe_ring_ops.o \ $(obj)/xe_vm.o \ $(obj)/xe_wa.o \ - $(obj)/xe_ttm_stolen_mgr.o + $(obj)/xe_ttm_stolen_mgr.o \ + $(obj)/display/xe_display_wa.o \ $(uses_generated_oob): $(generated_oob) @@ -192,6 +193,7 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \ display/xe_display.o \ display/xe_display_misc.o \ display/xe_display_rps.o \ + display/xe_display_wa.o \ display/xe_dsb_buffer.o \ display/xe_fb_pin.o \ display/xe_hdcp_gsc.o \ diff --git a/drivers/gpu/drm/xe/display/xe_display_wa.c b/drivers/gpu/drm/xe/display/xe_display_wa.c new file mode 100644 index 000000000000..68e3d1959ad6 --- /dev/null +++ b/drivers/gpu/drm/xe/display/xe_display_wa.c @@ -0,0 +1,16 @@ +// SPDX-License-Identifier: MIT +/* + * Copyright © 2024 Intel Corporation + */ + +#include "intel_display_wa.h" + +#include "xe_device.h" +#include "xe_wa.h" + +#include <generated/xe_wa_oob.h> + +bool intel_display_needs_wa_16023588340(struct drm_i915_private *i915) +{ + return XE_WA(xe_root_mmio_gt(i915), 16023588340); +}
On BMG-G21 we need to disable fbc due to complications around the WA. 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: intel-gfx@lists.freedesktop.org --- drivers/gpu/drm/i915/display/intel_display_wa.h | 8 ++++++++ drivers/gpu/drm/i915/display/intel_fbc.c | 6 ++++++ drivers/gpu/drm/xe/Makefile | 4 +++- drivers/gpu/drm/xe/display/xe_display_wa.c | 16 ++++++++++++++++ 4 files changed, 33 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/xe/display/xe_display_wa.c