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@xxxxxxxxx> > > > > > Cc: Jonathan Cavitt <jonathan.cavitt@xxxxxxxxx> > > > > > Cc: Matt Roper <matthew.d.roper@xxxxxxxxx> > > > > > Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx> > > > > > Cc: Vinod Govindapillai <vinod.govindapillai@xxxxxxxxx> > > > > > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > > > > --- > > > > > 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@xxxxxxxxx>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
