From: Alyssa Rosenzweig <alyssa@collabora.com>
To: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Tomeu Vizoso <tomeu.vizoso@collabora.com>,
Will Deacon <will@kernel.org>,
dri-devel@lists.freedesktop.org,
Steven Price <steven.price@arm.com>,
iommu@lists.linux-foundation.org,
Rob Herring <robh+dt@kernel.org>,
Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>,
Robin Murphy <robin.murphy@arm.com>,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 4/5] drm/panfrost: Add a PANFROST_BO_GPUONLY flag
Date: Fri, 1 Oct 2021 13:30:43 -0400 [thread overview]
Message-ID: <YVdFw9FivIyUCgU4@maud> (raw)
In-Reply-To: <20211001182206.78eb87ab@collabora.com>
> > This seems reasonable to me - it matches the kbase
> > BASE_MEM_COHERENT_SYSTEM (only backwards obviously) and it worked
> > reasonably well for the blob.
Oh, is that what that was for? I remember seeing it set on Midgard for
varyings. Good to go full circle now.
> > But I'm wondering if we need to do anything special to deal with the
> > fact we will now have some non-coherent mappings on an otherwise
> > coherent device.
> >
> > There are certainly some oddities around how these buffers will be
> > mapped into user space if requested, e.g. panfrost_gem_create_object()
> > sets 'map_wc' based on pfdev->coherent which is arguably wrong for
> > GPUONLY. So there are two things we could consider:
> >
> > a) Actually prevent user space mapping GPUONLY flagged buffers. Which
> > matches the intention of the name.
>
> I intended to do that, just forgot to add wrappers around
> drm_gem_shmem_{mmap,vmap}() to forbid CPU-mappings on gpuonly buffers.
This feels like the cleaner solution to me.
> > b) Attempt to provide user space with the tools to safely interact with
> > the buffers (this is the kbase approach). This does have the benefit of
> > allowing *mostly* GPU access. An example here is the tiler heap where
> > the CPU could zero out as necessary but mostly the GPU has ownership and
> > the CPU never reads the contents. GPUONLY/DEVONLY might not be the best
> > name in that case.
>
> Uh, right, I forgot we had to zero the tiler heap on Midgard (most of
> the time done with a WRITE_VALUE job, but there's an exception on some
> old Midgard GPUs IIRC).
"Attempt" is the key word here :|
We indeed only touch the tiler heap from the CPU on v4, and life's too
short to care about new optimizations for v4. Unless the patch is
trivial, my vote is for a) preventing the mappings and only setting
GPUONLY on the tiler_heap starting with v5.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
WARNING: multiple messages have this Message-ID (diff)
From: Alyssa Rosenzweig <alyssa@collabora.com>
To: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Steven Price <steven.price@arm.com>,
Joerg Roedel <joro@8bytes.org>,
iommu@lists.linux-foundation.org,
Rob Herring <robh+dt@kernel.org>,
Tomeu Vizoso <tomeu.vizoso@collabora.com>,
Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>,
Robin Murphy <robin.murphy@arm.com>,
Will Deacon <will@kernel.org>,
linux-arm-kernel@lists.infradead.org,
dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2 4/5] drm/panfrost: Add a PANFROST_BO_GPUONLY flag
Date: Fri, 1 Oct 2021 13:30:43 -0400 [thread overview]
Message-ID: <YVdFw9FivIyUCgU4@maud> (raw)
In-Reply-To: <20211001182206.78eb87ab@collabora.com>
> > This seems reasonable to me - it matches the kbase
> > BASE_MEM_COHERENT_SYSTEM (only backwards obviously) and it worked
> > reasonably well for the blob.
Oh, is that what that was for? I remember seeing it set on Midgard for
varyings. Good to go full circle now.
> > But I'm wondering if we need to do anything special to deal with the
> > fact we will now have some non-coherent mappings on an otherwise
> > coherent device.
> >
> > There are certainly some oddities around how these buffers will be
> > mapped into user space if requested, e.g. panfrost_gem_create_object()
> > sets 'map_wc' based on pfdev->coherent which is arguably wrong for
> > GPUONLY. So there are two things we could consider:
> >
> > a) Actually prevent user space mapping GPUONLY flagged buffers. Which
> > matches the intention of the name.
>
> I intended to do that, just forgot to add wrappers around
> drm_gem_shmem_{mmap,vmap}() to forbid CPU-mappings on gpuonly buffers.
This feels like the cleaner solution to me.
> > b) Attempt to provide user space with the tools to safely interact with
> > the buffers (this is the kbase approach). This does have the benefit of
> > allowing *mostly* GPU access. An example here is the tiler heap where
> > the CPU could zero out as necessary but mostly the GPU has ownership and
> > the CPU never reads the contents. GPUONLY/DEVONLY might not be the best
> > name in that case.
>
> Uh, right, I forgot we had to zero the tiler heap on Midgard (most of
> the time done with a WRITE_VALUE job, but there's an exception on some
> old Midgard GPUs IIRC).
"Attempt" is the key word here :|
We indeed only touch the tiler heap from the CPU on v4, and life's too
short to care about new optimizations for v4. Unless the patch is
trivial, my vote is for a) preventing the mappings and only setting
GPUONLY on the tiler_heap starting with v5.
WARNING: multiple messages have this Message-ID (diff)
From: Alyssa Rosenzweig <alyssa@collabora.com>
To: Boris Brezillon <boris.brezillon@collabora.com>
Cc: Steven Price <steven.price@arm.com>,
Joerg Roedel <joro@8bytes.org>,
iommu@lists.linux-foundation.org,
Rob Herring <robh+dt@kernel.org>,
Tomeu Vizoso <tomeu.vizoso@collabora.com>,
Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>,
Robin Murphy <robin.murphy@arm.com>,
Will Deacon <will@kernel.org>,
linux-arm-kernel@lists.infradead.org,
dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2 4/5] drm/panfrost: Add a PANFROST_BO_GPUONLY flag
Date: Fri, 1 Oct 2021 13:30:43 -0400 [thread overview]
Message-ID: <YVdFw9FivIyUCgU4@maud> (raw)
In-Reply-To: <20211001182206.78eb87ab@collabora.com>
> > This seems reasonable to me - it matches the kbase
> > BASE_MEM_COHERENT_SYSTEM (only backwards obviously) and it worked
> > reasonably well for the blob.
Oh, is that what that was for? I remember seeing it set on Midgard for
varyings. Good to go full circle now.
> > But I'm wondering if we need to do anything special to deal with the
> > fact we will now have some non-coherent mappings on an otherwise
> > coherent device.
> >
> > There are certainly some oddities around how these buffers will be
> > mapped into user space if requested, e.g. panfrost_gem_create_object()
> > sets 'map_wc' based on pfdev->coherent which is arguably wrong for
> > GPUONLY. So there are two things we could consider:
> >
> > a) Actually prevent user space mapping GPUONLY flagged buffers. Which
> > matches the intention of the name.
>
> I intended to do that, just forgot to add wrappers around
> drm_gem_shmem_{mmap,vmap}() to forbid CPU-mappings on gpuonly buffers.
This feels like the cleaner solution to me.
> > b) Attempt to provide user space with the tools to safely interact with
> > the buffers (this is the kbase approach). This does have the benefit of
> > allowing *mostly* GPU access. An example here is the tiler heap where
> > the CPU could zero out as necessary but mostly the GPU has ownership and
> > the CPU never reads the contents. GPUONLY/DEVONLY might not be the best
> > name in that case.
>
> Uh, right, I forgot we had to zero the tiler heap on Midgard (most of
> the time done with a WRITE_VALUE job, but there's an exception on some
> old Midgard GPUs IIRC).
"Attempt" is the key word here :|
We indeed only touch the tiler heap from the CPU on v4, and life's too
short to care about new optimizations for v4. Unless the patch is
trivial, my vote is for a) preventing the mappings and only setting
GPUONLY on the tiler_heap starting with v5.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-10-01 17:30 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-01 14:34 [PATCH v2 0/5] drm/panfrost: Add extra GPU-usage flags Boris Brezillon
2021-10-01 14:34 ` Boris Brezillon
2021-10-01 14:34 ` Boris Brezillon
2021-10-01 14:34 ` [PATCH v2 1/5] [RFC]iommu: Add a IOMMU_DEVONLY protection flag Boris Brezillon
2021-10-01 14:34 ` Boris Brezillon
2021-10-01 14:34 ` Boris Brezillon
2021-10-01 17:31 ` Alyssa Rosenzweig
2021-10-01 17:31 ` Alyssa Rosenzweig
2021-10-01 17:31 ` Alyssa Rosenzweig
2021-10-18 10:25 ` Joerg Roedel
2021-10-18 10:25 ` Joerg Roedel
2021-10-18 10:25 ` Joerg Roedel
2021-10-18 12:03 ` Boris Brezillon
2021-10-18 12:03 ` Boris Brezillon
2021-10-18 12:03 ` Boris Brezillon
2021-10-01 14:34 ` [PATCH v2 2/5] [RFC]iommu/io-pgtable-arm: Take the DEVONLY flag into account on ARM_MALI_LPAE Boris Brezillon
2021-10-01 14:34 ` Boris Brezillon
2021-10-01 14:34 ` Boris Brezillon
2021-10-01 14:34 ` [PATCH v2 3/5] drm/panfrost: Add PANFROST_BO_NO{READ,WRITE} flags Boris Brezillon
2021-10-01 14:34 ` Boris Brezillon
2021-10-01 14:34 ` Boris Brezillon
2021-10-01 14:36 ` Boris Brezillon
2021-10-01 14:36 ` Boris Brezillon
2021-10-01 14:36 ` Boris Brezillon
2021-10-01 14:34 ` [PATCH v2 4/5] drm/panfrost: Add a PANFROST_BO_GPUONLY flag Boris Brezillon
2021-10-01 14:34 ` Boris Brezillon
2021-10-01 14:34 ` Boris Brezillon
2021-10-01 15:13 ` Steven Price
2021-10-01 15:13 ` Steven Price
2021-10-01 15:13 ` Steven Price
2021-10-01 16:22 ` Boris Brezillon
2021-10-01 16:22 ` Boris Brezillon
2021-10-01 16:22 ` Boris Brezillon
2021-10-01 17:30 ` Alyssa Rosenzweig [this message]
2021-10-01 17:30 ` Alyssa Rosenzweig
2021-10-01 17:30 ` Alyssa Rosenzweig
2021-10-01 14:34 ` [PATCH v2 5/5] drm/panfrost: Bump the driver version to 1.3 Boris Brezillon
2021-10-01 14:34 ` Boris Brezillon
2021-10-01 14:34 ` Boris Brezillon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YVdFw9FivIyUCgU4@maud \
--to=alyssa@collabora.com \
--cc=alyssa.rosenzweig@collabora.com \
--cc=boris.brezillon@collabora.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=iommu@lists.linux-foundation.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=robh+dt@kernel.org \
--cc=robin.murphy@arm.com \
--cc=steven.price@arm.com \
--cc=tomeu.vizoso@collabora.com \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.