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.
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
