On Wed, Jan 10, 2024 at 11:11:10AM +0100, Andrzej Hajda wrote:
> On 15.12.2023 11:59, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > On MTL the GOP (for whatever reason) likes to bind its framebuffer
> > high up in the ggtt address space. This can conflict with whatever
> > ggtt_reserve_guc_top() is trying to do, and the result is that
> > ggtt_reserve_guc_top() fails and then we proceed to explode when
> > trying to tear down the driver. Thus far I haven't analyzed what
> > causes the actual fireworks, but it's not super important as even
> > if it didn't explode we'd still fail the driver load and the user
> > would be left with an unusable GPU.
> >
> > To remedy this (without having to figure out exactly what
> > ggtt_reserve_guc_top() is trying to achieve) we can attempt to
> > relocate the BIOS framebuffer to a lower ggtt address. We can do
> > this at this early point in driver init because nothing else is
> > supposed to be clobbering the ggtt yet. So we simply change where
> > in the ggtt we pin the vma, the original PTEs will be left as is,
> > and the new PTEs will get written with the same dma addresses.
> > The plane will keep on scanning out from the original PTEs until
> > we are done with the whole process, and at that point we rewrite
> > the plane's surface address register to point at the new ggtt
> > address.
> >
> > Since we don't need a specific ggtt address for the plane
> > (apart from needing it to land in the mappable region for
> > normal stolen objects) we'll just try to pin it without a fixed
> > offset first. It should end up at the lowest available address
> > (which really should be 0 at this point in the driver init).
> > If that fails we'll fall back to just pinning it exactly to the
> > origianal address.
> >
> > To make sure we don't accidentlally pin it partially over the
> > original ggtt range (as that would corrupt the original PTEs)
> > we reserve the original range temporarily during this process.
> >
> > Cc: Paz Zcharya <pazz@xxxxxxxxxxxx>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/i9xx_plane.c | 30 ++++++++++
> > drivers/gpu/drm/i915/display/i9xx_plane.h | 7 +++
> > drivers/gpu/drm/i915/display/intel_display.c | 5 ++
> > .../gpu/drm/i915/display/intel_display_core.h | 2 +
> > .../drm/i915/display/intel_plane_initial.c | 57 ++++++++++++++++++-
> > .../drm/i915/display/skl_universal_plane.c | 28 +++++++++
> > .../drm/i915/display/skl_universal_plane.h | 2 +
> > 7 files changed, 128 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/i9xx_plane.c b/drivers/gpu/drm/i915/display/i9xx_plane.c
> > index 91f2bc405cba..0279c8aabdd1 100644
> > --- a/drivers/gpu/drm/i915/display/i9xx_plane.c
> > +++ b/drivers/gpu/drm/i915/display/i9xx_plane.c
> > @@ -1060,3 +1060,33 @@ i9xx_get_initial_plane_config(struct intel_crtc *crtc,
> >
> > plane_config->fb = intel_fb;
> > }
> > +
> > +bool i9xx_fixup_initial_plane_config(struct intel_crtc *crtc,
> > + const struct intel_initial_plane_config *plane_config)
> > +{
> > + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > + struct intel_plane *plane = to_intel_plane(crtc->base.primary);
> > + const struct intel_plane_state *plane_state =
> > + to_intel_plane_state(plane->base.state);
> > + enum i9xx_plane_id i9xx_plane = plane->i9xx_plane;
> > + u32 base;
> > +
> > + if (!plane_state->uapi.visible)
> > + return false;
> > +
> > + base = intel_plane_ggtt_offset(plane_state);
> > +
> > + /*
> > + * We may have moved the surface to a different
> > + * part of ggtt, make the plane aware of that.
> > + */
> > + if (plane_config->base == base)
> > + return false;
> > +
> > + if (DISPLAY_VER(dev_priv) >= 4)
> > + intel_de_write(dev_priv, DSPSURF(i9xx_plane), base);
> > + else
> > + intel_de_write(dev_priv, DSPADDR(i9xx_plane), base);
>
> It seems skl_fixup_initial_plane_config is the same, except
> intel_de_write part. Couldn't we merge both functions into one and just
> add another elseif branch here? Maybe abstracting out somehow surface
> registers writes? Just loose ideas.
> However I wouldn't be surprised if there is good reason to keep it as is.
It's just that we generally want to keep the skl+ vs. pre-skl
low level plane code separate. There are enough differences between
them, so separate code is probably less messy than trying to share
it. And I don't think the common parts between this and its counterpart
are significant enough to really go against that.
>
> Reviewed-by: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>
>
> Regards
> Andrzej
>
> > +
> > + return true;
> > +}
> > diff --git a/drivers/gpu/drm/i915/display/i9xx_plane.h b/drivers/gpu/drm/i915/display/i9xx_plane.h
> > index b3d724a144cb..0ca12d1e6839 100644
> > --- a/drivers/gpu/drm/i915/display/i9xx_plane.h
> > +++ b/drivers/gpu/drm/i915/display/i9xx_plane.h
> > @@ -26,6 +26,8 @@ intel_primary_plane_create(struct drm_i915_private *dev_priv, enum pipe pipe);
> >
> > void i9xx_get_initial_plane_config(struct intel_crtc *crtc,
> > struct intel_initial_plane_config *plane_config);
> > +bool i9xx_fixup_initial_plane_config(struct intel_crtc *crtc,
> > + const struct intel_initial_plane_config *plane_config);
> > #else
> > static inline unsigned int i965_plane_max_stride(struct intel_plane *plane,
> > u32 pixel_format, u64 modifier,
> > @@ -46,6 +48,11 @@ static inline void i9xx_get_initial_plane_config(struct intel_crtc *crtc,
> > struct intel_initial_plane_config *plane_config)
> > {
> > }
> > +static inline bool i9xx_fixup_initial_plane_config(struct intel_crtc *crtc,
> > + const struct intel_initial_plane_config *plane_config)
> > +{
> > + return false;
> > +}
> > #endif
> >
> > #endif
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index d955957b7d18..92b4a894c9b9 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -7820,6 +7820,7 @@ static const struct intel_display_funcs skl_display_funcs = {
> > .crtc_disable = hsw_crtc_disable,
> > .commit_modeset_enables = skl_commit_modeset_enables,
> > .get_initial_plane_config = skl_get_initial_plane_config,
> > + .fixup_initial_plane_config = skl_fixup_initial_plane_config,
> > };
> >
> > static const struct intel_display_funcs ddi_display_funcs = {
> > @@ -7828,6 +7829,7 @@ static const struct intel_display_funcs ddi_display_funcs = {
> > .crtc_disable = hsw_crtc_disable,
> > .commit_modeset_enables = intel_commit_modeset_enables,
> > .get_initial_plane_config = i9xx_get_initial_plane_config,
> > + .fixup_initial_plane_config = i9xx_fixup_initial_plane_config,
> > };
> >
> > static const struct intel_display_funcs pch_split_display_funcs = {
> > @@ -7836,6 +7838,7 @@ static const struct intel_display_funcs pch_split_display_funcs = {
> > .crtc_disable = ilk_crtc_disable,
> > .commit_modeset_enables = intel_commit_modeset_enables,
> > .get_initial_plane_config = i9xx_get_initial_plane_config,
> > + .fixup_initial_plane_config = i9xx_fixup_initial_plane_config,
> > };
> >
> > static const struct intel_display_funcs vlv_display_funcs = {
> > @@ -7844,6 +7847,7 @@ static const struct intel_display_funcs vlv_display_funcs = {
> > .crtc_disable = i9xx_crtc_disable,
> > .commit_modeset_enables = intel_commit_modeset_enables,
> > .get_initial_plane_config = i9xx_get_initial_plane_config,
> > + .fixup_initial_plane_config = i9xx_fixup_initial_plane_config,
> > };
> >
> > static const struct intel_display_funcs i9xx_display_funcs = {
> > @@ -7852,6 +7856,7 @@ static const struct intel_display_funcs i9xx_display_funcs = {
> > .crtc_disable = i9xx_crtc_disable,
> > .commit_modeset_enables = intel_commit_modeset_enables,
> > .get_initial_plane_config = i9xx_get_initial_plane_config,
> > + .fixup_initial_plane_config = i9xx_fixup_initial_plane_config,
> > };
> >
> > /**
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_core.h b/drivers/gpu/drm/i915/display/intel_display_core.h
> > index 7e82b87e9cde..3f17328ff690 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_core.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_core.h
> > @@ -64,6 +64,8 @@ struct intel_display_funcs {
> > struct intel_crtc_state *);
> > void (*get_initial_plane_config)(struct intel_crtc *,
> > struct intel_initial_plane_config *);
> > + bool (*fixup_initial_plane_config)(struct intel_crtc *crtc,
> > + const struct intel_initial_plane_config *plane_config);
> > void (*crtc_enable)(struct intel_atomic_state *state,
> > struct intel_crtc *crtc);
> > void (*crtc_disable)(struct intel_atomic_state *state,
> > diff --git a/drivers/gpu/drm/i915/display/intel_plane_initial.c b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > index 82ab98985a09..72f509f8bc63 100644
> > --- a/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > +++ b/drivers/gpu/drm/i915/display/intel_plane_initial.c
> > @@ -3,9 +3,11 @@
> > * Copyright © 2021 Intel Corporation
> > */
> >
> > +#include "gem/i915_gem_lmem.h"
> > #include "gem/i915_gem_region.h"
> > #include "i915_drv.h"
> > #include "intel_atomic_plane.h"
> > +#include "intel_crtc.h"
> > #include "intel_display.h"
> > #include "intel_display_types.h"
> > #include "intel_fb.h"
> > @@ -138,6 +140,7 @@ initial_plane_vma(struct drm_i915_private *i915,
> > {
> > struct intel_memory_region *mem;
> > struct drm_i915_gem_object *obj;
> > + struct drm_mm_node orig_mm = {};
> > struct i915_vma *vma;
> > resource_size_t phys_base;
> > u32 base, size;
> > @@ -195,23 +198,68 @@ initial_plane_vma(struct drm_i915_private *i915,
> > goto err_obj;
> > }
> >
> > + /*
> > + * MTL GOP likes to place the framebuffer high up in ggtt,
> > + * which can cause problems for ggtt_reserve_guc_top().
> > + * Try to pin it to a low ggtt address instead to avoid that.
> > + */
> > + base = 0;
> > +
> > + if (base != plane_config->base) {
> > + struct i915_ggtt *ggtt = to_gt(i915)->ggtt;
> > + int ret;
> > +
> > + /*
> > + * Make sure the original and new locations
> > + * can't overlap. That would corrupt the original
> > + * PTEs which are still being used for scanout.
> > + */
> > + ret = i915_gem_gtt_reserve(&ggtt->vm, NULL, &orig_mm,
> > + size, plane_config->base,
> > + I915_COLOR_UNEVICTABLE, PIN_NOEVICT);
> > + if (ret)
> > + goto err_obj;
> > + }
> > +
> > vma = i915_vma_instance(obj, &to_gt(i915)->ggtt->vm, NULL);
> > if (IS_ERR(vma))
> > goto err_obj;
> >
> > - pinctl = PIN_GLOBAL | PIN_OFFSET_FIXED | base;
> > - if (HAS_GMCH(i915))
> > +retry:
> > + pinctl = PIN_GLOBAL;
> > + if (base == plane_config->base)
> > + pinctl |= PIN_OFFSET_FIXED | base;
> > + if (!i915_gem_object_is_lmem(obj))
> > pinctl |= PIN_MAPPABLE;
> > - if (i915_vma_pin(vma, 0, 0, pinctl))
> > + if (i915_vma_pin(vma, 0, 0, pinctl)) {
> > + if (drm_mm_node_allocated(&orig_mm)) {
> > + drm_mm_remove_node(&orig_mm);
> > + /*
> > + * Try again, but this time pin
> > + * it to its original location.
> > + */
> > + base = plane_config->base;
> > + goto retry;
> > + }
> > goto err_obj;
> > + }
> >
> > if (i915_gem_object_is_tiled(obj) &&
> > !i915_vma_is_map_and_fenceable(vma))
> > goto err_obj;
> >
> > + if (drm_mm_node_allocated(&orig_mm))
> > + drm_mm_remove_node(&orig_mm);
> > +
> > + drm_dbg_kms(&i915->drm,
> > + "Initial plane fb bound to 0x%x in the ggtt (original 0x%x)\n",
> > + i915_ggtt_offset(vma), plane_config->base);
> > +
> > return vma;
> >
> > err_obj:
> > + if (drm_mm_node_allocated(&orig_mm))
> > + drm_mm_remove_node(&orig_mm);
> > i915_gem_object_put(obj);
> > return NULL;
> > }
> > @@ -386,6 +434,9 @@ void intel_initial_plane_config(struct drm_i915_private *i915)
> > */
> > intel_find_initial_plane_obj(crtc, plane_configs);
> >
> > + if (i915->display.funcs.display->fixup_initial_plane_config(crtc, plane_config))
> > + intel_crtc_wait_for_next_vblank(crtc);
> > +
> > plane_config_fini(plane_config);
> > }
> > }
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > index 511dc1544854..392d93e97bf8 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> > @@ -2624,3 +2624,31 @@ skl_get_initial_plane_config(struct intel_crtc *crtc,
> > error:
> > kfree(intel_fb);
> > }
> > +
> > +bool skl_fixup_initial_plane_config(struct intel_crtc *crtc,
> > + const struct intel_initial_plane_config *plane_config)
> > +{
> > + struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> > + struct intel_plane *plane = to_intel_plane(crtc->base.primary);
> > + const struct intel_plane_state *plane_state =
> > + to_intel_plane_state(plane->base.state);
> > + enum plane_id plane_id = plane->id;
> > + enum pipe pipe = crtc->pipe;
> > + u32 base;
> > +
> > + if (!plane_state->uapi.visible)
> > + return false;
> > +
> > + base = intel_plane_ggtt_offset(plane_state);
> > +
> > + /*
> > + * We may have moved the surface to a different
> > + * part of ggtt, make the plane aware of that.
> > + */
> > + if (plane_config->base == base)
> > + return false;
> > +
> > + intel_de_write(i915, PLANE_SURF(pipe, plane_id), base);
> > +
> > + return true;
> > +}
> > diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.h b/drivers/gpu/drm/i915/display/skl_universal_plane.h
> > index be64c201f9b3..e92e00c01b29 100644
> > --- a/drivers/gpu/drm/i915/display/skl_universal_plane.h
> > +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.h
> > @@ -22,6 +22,8 @@ skl_universal_plane_create(struct drm_i915_private *dev_priv,
> >
> > void skl_get_initial_plane_config(struct intel_crtc *crtc,
> > struct intel_initial_plane_config *plane_config);
> > +bool skl_fixup_initial_plane_config(struct intel_crtc *crtc,
> > + const struct intel_initial_plane_config *plane_config);
> >
> > int skl_format_to_fourcc(int format, bool rgb_order, bool alpha);
> >
--
Ville Syrjälä
Intel
