Re: [PATCH v2 04/15] drm/i915: Bypass LMEMBAR/GTTMMADR for MTL stolen memory access

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Jan 10, 2024 at 11:49:47AM +0100, Nirmoy Das wrote:
> Hi Ville,
> 
> Apologies, but I lost track of this series after I returned from sick leave.
> 
> 
> On 12/15/2023 11:59 AM, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > On MTL accessing stolen memory via the BARs is somehow borked,
> > and it can hang the machine. As a workaround let's bypass the
> > BARs and just go straight to DSMBASE/GSMBASE instead.
> >
> > Note that on every other platform this itself would hang the
> > machine, but on MTL the system firmware is expected to relax
> > the access permission guarding stolen memory to enable this
> > workaround, and thus direct CPU accesses should be fine.
> >
> > TODO: add w/a numbers and whatnot
> >
> > Cc: Paz Zcharya <pazz@chromium.org>
> > Cc: Nirmoy Das <nirmoy.das@intel.com>
> > Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 11 ++++++++++-
> >   drivers/gpu/drm/i915/gt/intel_ggtt.c       | 13 ++++++++++++-
> >   2 files changed, 22 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > index ee237043c302..252fe5cd6ede 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> > @@ -941,7 +941,16 @@ i915_gem_stolen_lmem_setup(struct drm_i915_private *i915, u16 type,
> >   		dsm_size = ALIGN_DOWN(lmem_size - dsm_base, SZ_1M);
> >   	}
> >   
> > -	if (pci_resource_len(pdev, GEN12_LMEM_BAR) < lmem_size) {
> > +	if (IS_METEORLAKE(i915)) {
> > +		/*
> > +		 * Workaround: access via BAR can hang MTL, go directly to DSM.
> > +		 *
> > +		 * Normally this would not work but on MTL the system firmware
> > +		 * should have relaxed the access permissions sufficiently.
> > +		 */
> > +		io_start = intel_uncore_read64(uncore, GEN12_DSMBASE) & GEN12_BDSM_MASK;
> > +		io_size = dsm_size;
> 
> This will work well on host driver but I am afraid this will not work on 
> VM when someone tries to do direct device assignment of the igfx.
> 
> GSMBASE/DSMBASE is reserved region so won't show up in VM, last I checked.

Hmm. So BARs get passed over but other regions won't be? I wonder if
there's a way to pass them explicitly...

> 
> This is an obscure usages but are we suppose to support that? If so then 
> we need to detect that and fall back to binder approach.

I suppose some people may attempt it. But I'm not sure how well that
will work in practice even on other platforms. I don't think we've
ever really considered that use case any kind of priority so bug
reports tend to go unanswered.

My main worry with the MI_UPDATE_GTT stuff is:
- only used on this one platform so very limited testing coverage
- async so more opprtunities to screw things up
- what happens if the engine hangs while we're waiting for MI_UPDATE_GTT
  to finish?
- requires working command submission, so even getting a working
  display now depends on a lot more extra components working correctly

hence the patch to disable it. During testing my MTL was very unstable
so I wanted to eliminate all potential sources of new bugs.

Hmm. But we can't even use MI_UPDATE_GTT until command submission is
up and running, so we still need the direct CPU path for early ggtt
setup no? So if we can't pass the stolen directly to the VM the only
option would be to use the BARs for that and risk hanging the machine.

-- 
Ville Syrjälä
Intel