Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 10 Jul 2023 03:20:44 +0530
From:   Akhil P Oommen <quic_akhilpo@quicinc.com>
To:     Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Jordan Crouse <jorcrous@amazon.com>
CC:     <freedreno@lists.freedesktop.org>, Sean Paul <sean@poorly.run>,
        <linux-arm-msm@vger.kernel.org>,
        Konrad Dybcio <konrad.dybcio@somainline.org>,
        Abhinav Kumar <quic_abhinavk@quicinc.com>,
        <dri-devel@lists.freedesktop.org>, <linux-kernel@vger.kernel.org>,
        "Nathan Chancellor" <nathan@kernel.org>,
        Rob Clark <robdclark@gmail.com>,
        "Daniel Vetter" <daniel@ffwll.ch>,
        Ricardo Ribalda <ribalda@chromium.org>,
        "Joel Fernandes (Google)" <joel@joelfernandes.org>,
        David Airlie <airlied@gmail.com>
Subject: Re: [Freedreno] [PATCH] drm/msm: Check for the GPU IOMMU during bind
Message-ID: <2xnvyjlwuxft2uk2pirlbvbrg7krcb4alz7yyna72g4t2qrrfm@qtawbelv3n4l>
References: <20230309222049.4180579-1-jorcrous@amazon.com>
 <d73f6733-e605-0cf8-7909-8cced6e3b70d@linaro.org>
 <20230707150307.vb4otu5e6hwoadyf@amazon.com>
 <cc153fa9-b9e0-f714-ce5b-1a4a0cb55cc7@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <cc153fa9-b9e0-f714-ce5b-1a4a0cb55cc7@linaro.org>
Precedence: bulk

On Fri, Jul 07, 2023 at 08:27:18PM +0300, Dmitry Baryshkov wrote:
> 
> On 07/07/2023 18:03, Jordan Crouse wrote:
> > On Thu, Jul 06, 2023 at 09:55:13PM +0300, Dmitry Baryshkov wrote:
> > > 
> > > On 10/03/2023 00:20, Jordan Crouse wrote:
> > > > While booting with amd,imageon on a headless target the GPU probe was
> > > > failing with -ENOSPC in get_pages() from msm_gem.c.
> > > > 
> > > > Investigation showed that the driver was using the default 16MB VRAM
> > > > carveout because msm_use_mmu() was returning false since headless devices
> > > > use a dummy parent device. Avoid this by extending the existing is_a2xx
> > > > priv member to check the GPU IOMMU state on all platforms and use that
> > > > check in msm_use_mmu().
> > > > 
> > > > This works for memory allocations but it doesn't prevent the VRAM carveout
> > > > from being created because that happens before we have a chance to check
> > > > the GPU IOMMU state in adreno_bind.
> > > > 
> > > > There are a number of possible options to resolve this but none of them are
> > > > very clean. The easiest way is to likely specify vram=0 as module parameter
> > > > on headless devices so that the memory doesn't get wasted.
> > > 
> > > This patch was on my plate for quite a while, please excuse me for
> > > taking it so long.
> > 
> > No worries. I'm also chasing a bunch of other stuff too.
> > 
> > > I see the following problem with the current code. We have two different
> > > instances than can access memory: MDP/DPU and GPU. And each of them can
> > > either have or miss the MMU.
> > > 
> > > For some time I toyed with the idea of determining whether the allocated
> > > BO is going to be used by display or by GPU, but then I abandoned it. We
> > > can have display BOs being filled by GPU, so handling it this way would
> > > complicate things a lot.
> > > 
> > > This actually rings a tiny bell in my head with the idea of splitting
> > > the display and GPU parts to two different drivers, but I'm not sure
> > > what would be the overall impact.
> > 
> > As I now exclusively work on headless devices I would be 100% for this,
> > but I'm sure that our laptop friends might not agree :)
> 
> I do not know here. This is probably a question to Rob, as he better
> understands the interaction between GPU and display parts of the userspace.

I fully support this if it is feasible.

In our architecture, display and GPU are completely independent subsystems.
Like Jordan mentioned, there are IOT products without display. And I wouldn't
be surprised if there is a product with just display and uses software rendering.

-Akhil

> 
> > 
> > > More on the msm_use_mmu() below.
> > > 
> > > > 
> > > > Signed-off-by: Jordan Crouse <jorcrous@amazon.com>
> > > > ---
> > > > 
> > > >    drivers/gpu/drm/msm/adreno/adreno_device.c | 6 +++++-
> > > >    drivers/gpu/drm/msm/msm_drv.c              | 7 +++----
> > > >    drivers/gpu/drm/msm/msm_drv.h              | 2 +-
> > > >    3 files changed, 9 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > index 36f062c7582f..4f19da28f80f 100644
> > > > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > > > @@ -539,7 +539,11 @@ static int adreno_bind(struct device *dev, struct device *master, void *data)
> > > >        DBG("Found GPU: %u.%u.%u.%u", config.rev.core, config.rev.major,
> > > >                config.rev.minor, config.rev.patchid);
> > > > 
> > > > -     priv->is_a2xx = config.rev.core == 2;
> > > > +     /*
> > > > +      * A2xx has a built in IOMMU and all other IOMMU enabled targets will
> > > > +      * have an ARM IOMMU attached
> > > > +      */
> > > > +     priv->has_gpu_iommu = config.rev.core == 2 || device_iommu_mapped(dev);
> > > >        priv->has_cached_coherent = config.rev.core >= 6;
> > > > 
> > > >        gpu = info->init(drm);
> > > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > > > index aca48c868c14..a125a351ec90 100644
> > > > --- a/drivers/gpu/drm/msm/msm_drv.c
> > > > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > > > @@ -318,11 +318,10 @@ bool msm_use_mmu(struct drm_device *dev)
> > > >        struct msm_drm_private *priv = dev->dev_private;
> > > > 
> > > >        /*
> > > > -      * a2xx comes with its own MMU
> > > > -      * On other platforms IOMMU can be declared specified either for the
> > > > -      * MDP/DPU device or for its parent, MDSS device.
> > > > +      * Return true if the GPU or the MDP/DPU or parent MDSS device has an
> > > > +      * IOMMU
> > > >         */
> > > > -     return priv->is_a2xx ||
> > > > +     return priv->has_gpu_iommu ||
> > > >                device_iommu_mapped(dev->dev) ||
> > > >                device_iommu_mapped(dev->dev->parent);
> > > 
> > > I have a generic feeling that both old an new code is not fully correct.
> > > Please correct me if I'm wrong:
> > > 
> > > We should be using VRAM, if either of the blocks can not use remapped
> > > memory. So this should have been:
> > > 
> > > bool msm_use_mmu()
> > > {
> > >   if (!gpu_has_iommu)
> > >     return false;
> > > 
> > >   if (have_display_part && !display_has_mmu())
> > >     return false;
> > > 
> > >   return true;
> > > }
> > > 
> > > What do you think.
> > 
> > I would have to see (and try) the real code but that seems like it might
> > be reasonable.
> 
> Sure, let me craft it then.
> 
> > I would like to hear from some of the a2xx users too
> > because this mostly affects them. On 3xx and newer you've always had the
> > option of not having a MMU for GPU or display but I can't think of any
> > use cases where you wouldn't want it.
> 
> msm8974 doesn't have (working) IOMMU driver. I also think there was an issue
> somewhere around sdm630/660. And the wonderful msm8992/4, IIRC, also don't
> have one.
> 
> Also the headless mode was quite useful for bringing up platforms, as it
> allowed us to test GPU separately (and ofc. in some cases even w/o MMU).
> 
> I have both a2xx (only iMX for now) and a3xx for the tests here, on my desk.
> 
> -- 
> With best wishes
> Dmitry
>