Re: [PATCH RFC PKS/PMEM 57/58] nvdimm/pmem: Stray access protection for pmem->virt_addr
On Fri, Oct 09, 2020 at 07:53:07PM -0700, John Hubbard wrote:
> On 10/9/20 12:50 PM, ira.we...@intel.com wrote:
> > From: Ira Weiny
> >
> > The pmem driver uses a cached virtual address to access its memory
> > directly. Because the nvdimm driver is well aware of the special
> > protections it has mapped memory with, we call dev_access_[en|dis]able()
> > around the direct pmem->virt_addr (pmem_addr) usage instead of the
> > unnecessary overhead of trying to get a page to kmap.
> >
> > Signed-off-by: Ira Weiny
> > ---
> > drivers/nvdimm/pmem.c | 4
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> > index fab29b514372..e4dc1ae990fc 100644
> > --- a/drivers/nvdimm/pmem.c
> > +++ b/drivers/nvdimm/pmem.c
> > @@ -148,7 +148,9 @@ static blk_status_t pmem_do_read(struct pmem_device
> > *pmem,
> > if (unlikely(is_bad_pmem(>bb, sector, len)))
> > return BLK_STS_IOERR;
> > + dev_access_enable(false);
> > rc = read_pmem(page, page_off, pmem_addr, len);
> > + dev_access_disable(false);
>
> Hi Ira!
>
> The APIs should be tweaked to use a symbol (GLOBAL, PER_THREAD), instead of
> true/false. Try reading the above and you'll see that it sounds like it's
> doing the opposite of what it is ("enable_this(false)" sounds like a clumsy
> API design to *disable*, right?). And there is no hint about the scope.
Sounds reasonable.
>
> And it *could* be so much more readable like this:
>
> dev_access_enable(DEV_ACCESS_THIS_THREAD);
I'll think about the flag name. I'm not liking 'this thread'.
Maybe DEV_ACCESS_[GLOBAL|THREAD]
Ira
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC PKS/PMEM 48/58] drivers/md: Utilize new kmap_thread()
On Sat, Oct 10, 2020 at 10:20:34AM +0800, Coly Li wrote: > On 2020/10/10 03:50, ira.we...@intel.com wrote: > > From: Ira Weiny > > > > These kmap() calls are localized to a single thread. To avoid the over > > head of global PKRS updates use the new kmap_thread() call. > > > > Hi Ira, > > There were a number of options considered. > > 1) Attempt to change all the thread local kmap() calls to kmap_atomic() > 2) Introduce a flags parameter to kmap() to indicate if the mapping > should be global or not > 3) Change ~20-30 call sites to 'kmap_global()' to indicate that they > require a global mapping of the pages > 4) Change ~209 call sites to 'kmap_thread()' to indicate that the > mapping is to be used within that thread of execution only > > > I copied the above information from patch 00/58 to this message. The > idea behind kmap_thread() is fine to me, but as you said the new api is > very easy to be missed in new code (even for me). I would like to be > supportive to option 2) introduce a flag to kmap(), then we won't forget > the new thread-localized kmap method, and people won't ask why a > _thread() function is called but no kthread created. Thanks for the feedback. I'm going to hold off making any changes until others weigh in. FWIW, I kind of like option 2 as well. But there is already kmap_atomic() so it seemed like kmap_() was more in line with the current API. Thanks, Ira > > Thanks. > > > Coly Li > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RFC PKS/PMEM 10/58] drivers/rdma: Utilize new kmap_thread()
On Sat, Oct 10, 2020 at 11:36:49AM +, Bernard Metzler wrote:
> -ira.we...@intel.com wrote: -
>
[snip]
> >@@ -505,7 +505,7 @@ static int siw_tx_hdt(struct siw_iwarp_tx *c_tx,
> >struct socket *s)
> > page_array[seg] = p;
> >
> > if (!c_tx->use_sendpage) {
> >-iov[seg].iov_base = kmap(p) + fp_off;
> >+iov[seg].iov_base = kmap_thread(p) +
> >fp_off;
>
> This misses a corresponding kunmap_thread() in siw_unmap_pages()
> (pls change line 403 in siw_qp_tx.c as well)
Thanks I missed that.
Done.
Ira
>
> Thanks,
> Bernard.
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH RESEND] drm/aspeed: fix Kconfig warning & subsequent build errors
On Sun, 11 Oct 2020 at 23:01, Randy Dunlap wrote:
>
> kernel test robot reported build errors (undefined references)
> that didn't make much sense. After reproducing them, there is also
> a Kconfig warning that is the root cause of the build errors, so
> fix that Kconfig problem.
>
> Fixes this Kconfig warning:
> WARNING: unmet direct dependencies detected for CMA
> Depends on [n]: MMU [=n]
> Selected by [m]:
> - DRM_ASPEED_GFX [=m] && HAS_IOMEM [=y] && DRM [=m] && OF [=y] &&
> (COMPILE_TEST [=y] || ARCH_ASPEED) && HAVE_DMA_CONTIGUOUS [=y]
>
> and these dependent build errors:
> (.text+0x10c8c): undefined reference to `start_isolate_page_range'
> microblaze-linux-ld: (.text+0x10f14): undefined reference to
> `test_pages_isolated'
> microblaze-linux-ld: (.text+0x10fd0): undefined reference to
> `undo_isolate_page_range'
>
> Fixes: 76356a966e33 ("drm: aspeed: Clean up Kconfig options")
> Reported-by: kernel test robot
> Signed-off-by: Randy Dunlap
> Cc: Joel Stanley
> Cc: Andrew Jeffery
> Cc: Daniel Vetter
> Cc: Michal Simek
> Cc: Andrew Morton
> Cc: Mike Rapoport
> Cc: linux...@kvack.org
> Cc: linux-asp...@lists.ozlabs.org
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: David Airlie
> Cc: dri-devel@lists.freedesktop.org
> ---
> First sent on 2020-09-07.
> Feel free to fix the Kconfig warning some other way...
Reviewed-by: Joel Stanley
Thanks Randy. Sorry for missing it the first time around. I'll merge
this into drm-misc-next.
Cheers,
Joel
>
> drivers/gpu/drm/aspeed/Kconfig |1 +
> 1 file changed, 1 insertion(+)
>
> --- linux-next-20201009.orig/drivers/gpu/drm/aspeed/Kconfig
> +++ linux-next-20201009/drivers/gpu/drm/aspeed/Kconfig
> @@ -3,6 +3,7 @@ config DRM_ASPEED_GFX
> tristate "ASPEED BMC Display Controller"
> depends on DRM && OF
> depends on (COMPILE_TEST || ARCH_ASPEED)
> + depends on MMU
> select DRM_KMS_HELPER
> select DRM_KMS_CMA_HELPER
> select DMA_CMA if HAVE_DMA_CONTIGUOUS
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: linux-next: build failure after merge of the drm-misc tree
Hi all,
[Just adding Dave to cc's]
On Mon, 12 Oct 2020 15:24:52 +1100 Stephen Rothwell
wrote:
>
> Hi all,
>
> On Thu, 8 Oct 2020 15:42:02 +1100 Stephen Rothwell
> wrote:
> >
> > On Thu, 8 Oct 2020 14:09:03 +1100 Stephen Rothwell
> > wrote:
> > >
> > > After merging the drm-misc tree, today's linux-next build (x86_64
> > > allmodconfig) failed like this:
> >
> > In file included from include/linux/clk.h:13,
> > from drivers/gpu/drm/ingenic/ingenic-drm-drv.c:10:
> > drivers/gpu/drm/ingenic/ingenic-drm-drv.c: In function
> > 'ingenic_drm_update_palette':
> > drivers/gpu/drm/ingenic/ingenic-drm-drv.c:448:35: error: 'struct
> > ingenic_drm' has no member named 'dma_hwdescs'; did you mean
> > 'dma_hwdesc_f0'?
> > 448 | for (i = 0; i < ARRAY_SIZE(priv->dma_hwdescs->palette); i++) {
> > | ^~~
> > include/linux/kernel.h:47:33: note: in definition of macro 'ARRAY_SIZE'
> >47 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
> > __must_be_array(arr))
> > | ^~~
> > drivers/gpu/drm/ingenic/ingenic-drm-drv.c:448:35: error: 'struct
> > ingenic_drm' has no member named 'dma_hwdescs'; did you mean
> > 'dma_hwdesc_f0'?
> > 448 | for (i = 0; i < ARRAY_SIZE(priv->dma_hwdescs->palette); i++) {
> > | ^~~
> > include/linux/kernel.h:47:48: note: in definition of macro 'ARRAY_SIZE'
> >47 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
> > __must_be_array(arr))
> > |^~~
> > In file included from include/linux/bits.h:22,
> > from include/linux/bitops.h:5,
> > from drivers/gpu/drm/ingenic/ingenic-drm.h:10,
> > from drivers/gpu/drm/ingenic/ingenic-drm-drv.c:7:
> > drivers/gpu/drm/ingenic/ingenic-drm-drv.c:448:35: error: 'struct
> > ingenic_drm' has no member named 'dma_hwdescs'; did you mean
> > 'dma_hwdesc_f0'?
> > 448 | for (i = 0; i < ARRAY_SIZE(priv->dma_hwdescs->palette); i++) {
> > | ^~~
> > include/linux/build_bug.h:16:62: note: in definition of macro
> > 'BUILD_BUG_ON_ZERO'
> >16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e));
> > })))
> > | ^
> > include/linux/compiler.h:224:46: note: in expansion of macro '__same_type'
> > 224 | #define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a),
> > &(a)[0]))
> > | ^~~
> > include/linux/kernel.h:47:59: note: in expansion of macro '__must_be_array'
> >47 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
> > __must_be_array(arr))
> > |
> > ^~~
> > drivers/gpu/drm/ingenic/ingenic-drm-drv.c:448:18: note: in expansion of
> > macro 'ARRAY_SIZE'
> > 448 | for (i = 0; i < ARRAY_SIZE(priv->dma_hwdescs->palette); i++) {
> > | ^~
> > drivers/gpu/drm/ingenic/ingenic-drm-drv.c:448:35: error: 'struct
> > ingenic_drm' has no member named 'dma_hwdescs'; did you mean
> > 'dma_hwdesc_f0'?
> > 448 | for (i = 0; i < ARRAY_SIZE(priv->dma_hwdescs->palette); i++) {
> > | ^~~
> > include/linux/build_bug.h:16:62: note: in definition of macro
> > 'BUILD_BUG_ON_ZERO'
> >16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e));
> > })))
> > | ^
> > include/linux/compiler.h:224:46: note: in expansion of macro '__same_type'
> > 224 | #define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a),
> > &(a)[0]))
> > | ^~~
> > include/linux/kernel.h:47:59: note: in expansion of macro '__must_be_array'
> >47 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
> > __must_be_array(arr))
> > |
> > ^~~
> > drivers/gpu/drm/ingenic/ingenic-drm-drv.c:448:18: note: in expansion of
> > macro 'ARRAY_SIZE'
> > 448 | for (i = 0; i < ARRAY_SIZE(priv->dma_hwdescs->palette); i++) {
> > | ^~
> > include/linux/build_bug.h:16:51: error: bit-field '' width not
> > an integer constant
> >16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e));
> > })))
> > | ^
> > include/linux/compiler.h:224:28: note: in expansion of macro
> > 'BUILD_BUG_ON_ZERO'
> > 224 | #define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a),
> > &(a)[0]))
> > |^
> > include/linux/kernel.h:47:59: note: in expansion of macro '__must_be_array'
> >47 | #define
Re: linux-next: build failure after merge of the drm-misc tree
Hi all,
On Thu, 8 Oct 2020 15:42:02 +1100 Stephen Rothwell
wrote:
>
> On Thu, 8 Oct 2020 14:09:03 +1100 Stephen Rothwell
> wrote:
> >
> > After merging the drm-misc tree, today's linux-next build (x86_64
> > allmodconfig) failed like this:
>
> In file included from include/linux/clk.h:13,
> from drivers/gpu/drm/ingenic/ingenic-drm-drv.c:10:
> drivers/gpu/drm/ingenic/ingenic-drm-drv.c: In function
> 'ingenic_drm_update_palette':
> drivers/gpu/drm/ingenic/ingenic-drm-drv.c:448:35: error: 'struct ingenic_drm'
> has no member named 'dma_hwdescs'; did you mean 'dma_hwdesc_f0'?
> 448 | for (i = 0; i < ARRAY_SIZE(priv->dma_hwdescs->palette); i++) {
> | ^~~
> include/linux/kernel.h:47:33: note: in definition of macro 'ARRAY_SIZE'
>47 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
> __must_be_array(arr))
> | ^~~
> drivers/gpu/drm/ingenic/ingenic-drm-drv.c:448:35: error: 'struct ingenic_drm'
> has no member named 'dma_hwdescs'; did you mean 'dma_hwdesc_f0'?
> 448 | for (i = 0; i < ARRAY_SIZE(priv->dma_hwdescs->palette); i++) {
> | ^~~
> include/linux/kernel.h:47:48: note: in definition of macro 'ARRAY_SIZE'
>47 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
> __must_be_array(arr))
> |^~~
> In file included from include/linux/bits.h:22,
> from include/linux/bitops.h:5,
> from drivers/gpu/drm/ingenic/ingenic-drm.h:10,
> from drivers/gpu/drm/ingenic/ingenic-drm-drv.c:7:
> drivers/gpu/drm/ingenic/ingenic-drm-drv.c:448:35: error: 'struct ingenic_drm'
> has no member named 'dma_hwdescs'; did you mean 'dma_hwdesc_f0'?
> 448 | for (i = 0; i < ARRAY_SIZE(priv->dma_hwdescs->palette); i++) {
> | ^~~
> include/linux/build_bug.h:16:62: note: in definition of macro
> 'BUILD_BUG_ON_ZERO'
>16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> | ^
> include/linux/compiler.h:224:46: note: in expansion of macro '__same_type'
> 224 | #define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a),
> &(a)[0]))
> | ^~~
> include/linux/kernel.h:47:59: note: in expansion of macro '__must_be_array'
>47 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
> __must_be_array(arr))
> |
> ^~~
> drivers/gpu/drm/ingenic/ingenic-drm-drv.c:448:18: note: in expansion of macro
> 'ARRAY_SIZE'
> 448 | for (i = 0; i < ARRAY_SIZE(priv->dma_hwdescs->palette); i++) {
> | ^~
> drivers/gpu/drm/ingenic/ingenic-drm-drv.c:448:35: error: 'struct ingenic_drm'
> has no member named 'dma_hwdescs'; did you mean 'dma_hwdesc_f0'?
> 448 | for (i = 0; i < ARRAY_SIZE(priv->dma_hwdescs->palette); i++) {
> | ^~~
> include/linux/build_bug.h:16:62: note: in definition of macro
> 'BUILD_BUG_ON_ZERO'
>16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> | ^
> include/linux/compiler.h:224:46: note: in expansion of macro '__same_type'
> 224 | #define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a),
> &(a)[0]))
> | ^~~
> include/linux/kernel.h:47:59: note: in expansion of macro '__must_be_array'
>47 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
> __must_be_array(arr))
> |
> ^~~
> drivers/gpu/drm/ingenic/ingenic-drm-drv.c:448:18: note: in expansion of macro
> 'ARRAY_SIZE'
> 448 | for (i = 0; i < ARRAY_SIZE(priv->dma_hwdescs->palette); i++) {
> | ^~
> include/linux/build_bug.h:16:51: error: bit-field '' width not an
> integer constant
>16 | #define BUILD_BUG_ON_ZERO(e) ((int)(sizeof(struct { int:(-!!(e)); })))
> | ^
> include/linux/compiler.h:224:28: note: in expansion of macro
> 'BUILD_BUG_ON_ZERO'
> 224 | #define __must_be_array(a) BUILD_BUG_ON_ZERO(__same_type((a),
> &(a)[0]))
> |^
> include/linux/kernel.h:47:59: note: in expansion of macro '__must_be_array'
>47 | #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) +
> __must_be_array(arr))
> |
> ^~~
> drivers/gpu/drm/ingenic/ingenic-drm-drv.c:448:18: note: in expansion of macro
> 'ARRAY_SIZE'
> 448 | for (i = 0; i < ARRAY_SIZE(priv->dma_hwdescs->palette);
Re: linux-next: build failure after merge of the hmm tree
Hi all,
On Tue, 6 Oct 2020 13:41:20 -0300 Jason Gunthorpe wrote:
>
> On Tue, Oct 06, 2020 at 08:35:08PM +1100, Stephen Rothwell wrote:
> > Hi all,
> >
> > After merging the hmm tree, today's linux-next build (arm
> > multi_v7_defconfig) failed like this:
> >
> >
> > Caused by commit
> >
> > 07da1223ec93 ("lib/scatterlist: Add support in dynamic allocation of SG
> > table from pages")
> >
> > interacting with commit
> >
> > 707d561f77b5 ("drm: allow limiting the scatter list size.")
> >
> > from the drm tree.
> >
> > I have added the following merge fix patch
> >
> > From: Stephen Rothwell
> > Date: Tue, 6 Oct 2020 20:22:51 +1100
> > Subject: [PATCH] lib/scatterlist: merge fix for "drm: allow limiting the
> > scatter list size."
> >
> > Signed-off-by: Stephen Rothwell
> > drivers/gpu/drm/drm_prime.c | 9 ++---
> > 1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> > index 11fe9ff76fd5..83ac901b65a2 100644
> > +++ b/drivers/gpu/drm/drm_prime.c
> > @@ -807,6 +807,7 @@ struct sg_table *drm_prime_pages_to_sg(struct
> > drm_device *dev,
> >struct page **pages, unsigned int
> > nr_pages)
> > {
> > struct sg_table *sg = NULL;
> > + struct scatterlist *sl;
> > size_t max_segment = 0;
> > int ret;
> >
> > @@ -820,11 +821,13 @@ struct sg_table *drm_prime_pages_to_sg(struct
> > drm_device *dev,
> > max_segment = dma_max_mapping_size(dev->dev);
> > if (max_segment == 0 || max_segment > SCATTERLIST_MAX_SEGMENT)
> > max_segment = SCATTERLIST_MAX_SEGMENT;
> > - ret = __sg_alloc_table_from_pages(sg, pages, nr_pages, 0,
> > + sl = __sg_alloc_table_from_pages(sg, pages, nr_pages, 0,
> > nr_pages << PAGE_SHIFT,
> > - max_segment, GFP_KERNEL);
> > - if (ret)
> > + max_segment, NULL, 0, GFP_KERNEL);
> > + if (IS_ERR(sl)) {
> > + ret = PTR_ERR(sl);
> > goto out;
> > + }
> >
> > return sg;
> > out:
>
> This looks OK to me, thanks
This merge fix patch is now being applied to the merge of the drm tree
since the rdma tree (that is merged before the drm tree) has merged the
hmm tree.
--
Cheers,
Stephen Rothwell
pgpWQgXGdYInB.pgp
Description: OpenPGP digital signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: linux-next: manual merge of the tip tree with the amdgpu tree
Hi all,
On Wed, 23 Sep 2020 15:13:36 +1000 Stephen Rothwell
wrote:
>
> Today's linux-next merge of the tip tree got a conflict in:
>
> drivers/gpu/drm/amd/amdkfd/kfd_priv.h
>
> between commit:
>
> 59d7115dae02 ("drm/amdkfd: Move process doorbell allocation into kfd
> device")
>
> from the amdgpu tree and commit:
>
> c7b6bac9c72c ("drm, iommu: Change type of pasid to u32")
>
> from the tip tree.
>
> I fixed it up (see below) and can carry the fix as necessary. This
> is now fixed as far as linux-next is concerned, but any non trivial
> conflicts should be mentioned to your upstream maintainer when your tree
> is submitted for merging. You may also want to consider cooperating
> with the maintainer of the conflicting tree to minimise any particularly
> complex conflicts.
>
>
> diff --cc drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> index 739db04080d0,922ae138ab85..
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_priv.h
> @@@ -739,7 -723,8 +739,7 @@@ struct kfd_process
> /* We want to receive a notification when the mm_struct is destroyed */
> struct mmu_notifier mmu_notifier;
>
> - uint16_t pasid;
> + u32 pasid;
> -unsigned int doorbell_index;
>
> /*
>* List of kfd_process_device structures,
This is now a conflict between the tip tree and the drm tree.
--
Cheers,
Stephen Rothwell
pgpf1Jq1vIyrQ.pgp
Description: OpenPGP digital signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 17/22] drm/msm: Drop struct_mutex from the retire path
From: Rob Clark
Now that we are not relying on dev->struct_mutex to protect the
ring->submits lists, drop the struct_mutex lock.
Signed-off-by: Rob Clark
Reviewed-by: Jordan Crouse
---
drivers/gpu/drm/msm/msm_gpu.c | 8 +---
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 8278a4df331a..a754e84b8b5d 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -707,7 +707,7 @@ static void retire_submit(struct msm_gpu *gpu, struct
msm_ringbuffer *ring,
msm_gem_active_put(_obj->base);
msm_gem_unpin_iova(_obj->base, submit->aspace);
- drm_gem_object_put_locked(_obj->base);
+ drm_gem_object_put(_obj->base);
}
pm_runtime_mark_last_busy(>pdev->dev);
@@ -722,11 +722,8 @@ static void retire_submit(struct msm_gpu *gpu, struct
msm_ringbuffer *ring,
static void retire_submits(struct msm_gpu *gpu)
{
- struct drm_device *dev = gpu->dev;
int i;
- WARN_ON(!mutex_is_locked(>struct_mutex));
-
/* Retire the commits starting with highest priority */
for (i = 0; i < gpu->nr_rings; i++) {
struct msm_ringbuffer *ring = gpu->rb[i];
@@ -756,15 +753,12 @@ static void retire_submits(struct msm_gpu *gpu)
static void retire_worker(struct work_struct *work)
{
struct msm_gpu *gpu = container_of(work, struct msm_gpu, retire_work);
- struct drm_device *dev = gpu->dev;
int i;
for (i = 0; i < gpu->nr_rings; i++)
update_fences(gpu, gpu->rb[i], gpu->rb[i]->memptrs->fence);
- mutex_lock(>struct_mutex);
retire_submits(gpu);
- mutex_unlock(>struct_mutex);
}
/* call from irq handler to schedule work to retire bo's */
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 20/22] drm/msm: drop struct_mutex in madvise path
From: Rob Clark
The obj->lock is sufficient for what we need.
This *does* have the implication that userspace can try to shoot
themselves in the foot by racing madvise(DONTNEED) with submit. But
the result will be about the same if they did madvise(DONTNEED) before
the submit ioctl, ie. they might not get want they want if they race
with shrinker. But iova fault handling is robust enough, and userspace
is only shooting it's own foot.
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/msm_drv.c | 11 ++-
drivers/gpu/drm/msm/msm_gem.c | 4 +---
drivers/gpu/drm/msm/msm_gem.h | 2 --
3 files changed, 3 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 49e6daf30b42..f2d58fe25497 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -912,14 +912,9 @@ static int msm_ioctl_gem_madvise(struct drm_device *dev,
void *data,
return -EINVAL;
}
- ret = mutex_lock_interruptible(>struct_mutex);
- if (ret)
- return ret;
-
obj = drm_gem_object_lookup(file, args->handle);
if (!obj) {
- ret = -ENOENT;
- goto unlock;
+ return -ENOENT;
}
ret = msm_gem_madvise(obj, args->madv);
@@ -928,10 +923,8 @@ static int msm_ioctl_gem_madvise(struct drm_device *dev,
void *data,
ret = 0;
}
- drm_gem_object_put_locked(obj);
+ drm_gem_object_put(obj);
-unlock:
- mutex_unlock(>struct_mutex);
return ret;
}
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index b60eaf6266e2..8852c05775dc 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -658,8 +658,6 @@ int msm_gem_madvise(struct drm_gem_object *obj, unsigned
madv)
msm_gem_lock(obj);
- WARN_ON(!mutex_is_locked(>dev->struct_mutex));
-
if (msm_obj->madv != __MSM_MADV_PURGED)
msm_obj->madv = madv;
@@ -676,7 +674,6 @@ void msm_gem_purge(struct drm_gem_object *obj)
struct msm_gem_object *msm_obj = to_msm_bo(obj);
WARN_ON(!mutex_is_locked(>struct_mutex));
- WARN_ON(!msm_gem_is_locked(obj));
WARN_ON(!is_purgeable(msm_obj));
WARN_ON(obj->import_attach);
@@ -756,6 +753,7 @@ void msm_gem_active_get(struct drm_gem_object *obj, struct
msm_gpu *gpu)
struct msm_drm_private *priv = obj->dev->dev_private;
might_sleep();
+ WARN_ON(!msm_gem_is_locked(obj));
WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED);
if (!atomic_fetch_inc(_obj->active_count)) {
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index c12fedf88e85..1f8f5f3d08c0 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -188,8 +188,6 @@ static inline bool is_active(struct msm_gem_object *msm_obj)
static inline bool is_purgeable(struct msm_gem_object *msm_obj)
{
- WARN_ON(!msm_gem_is_locked(_obj->base));
- WARN_ON(!mutex_is_locked(_obj->base.dev->struct_mutex));
return (msm_obj->madv == MSM_MADV_DONTNEED) && msm_obj->sgt &&
!msm_obj->base.dma_buf && !msm_obj->base.import_attach;
}
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 08/22] drm/msm/gem: Switch over to obj->resv for locking
From: Rob Clark
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/msm_gem.c| 4 +---
drivers/gpu/drm/msm/msm_gem.h| 16 +---
drivers/gpu/drm/msm/msm_gem_submit.c | 4 ++--
drivers/gpu/drm/msm/msm_gpu.c| 2 +-
4 files changed, 9 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index ff8ca257bdc6..210bf5c9c2dd 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -955,9 +955,9 @@ static void free_object(struct msm_gem_object *msm_obj)
put_pages(obj);
}
+ msm_gem_unlock(obj);
drm_gem_object_release(obj);
- msm_gem_unlock(obj);
kfree(msm_obj);
}
@@ -1029,8 +1029,6 @@ static int msm_gem_new_impl(struct drm_device *dev,
if (!msm_obj)
return -ENOMEM;
- mutex_init(_obj->lock);
-
msm_obj->flags = flags;
msm_obj->madv = MSM_MADV_WILLNEED;
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index 744889436a98..ec01f35ce57b 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -85,7 +85,6 @@ struct msm_gem_object {
* an IOMMU. Also used for stolen/splashscreen buffer.
*/
struct drm_mm_node *vram_node;
- struct mutex lock; /* Protects resources associated with bo */
char name[32]; /* Identifier to print for the debugfs files */
@@ -156,36 +155,31 @@ void msm_gem_describe_objects(struct list_head *list,
struct seq_file *m);
static inline void
msm_gem_lock(struct drm_gem_object *obj)
{
- struct msm_gem_object *msm_obj = to_msm_bo(obj);
- mutex_lock(_obj->lock);
+ dma_resv_lock(obj->resv, NULL);
}
static inline bool __must_check
msm_gem_trylock(struct drm_gem_object *obj)
{
- struct msm_gem_object *msm_obj = to_msm_bo(obj);
- return mutex_trylock_recursive(_obj->lock) == MUTEX_TRYLOCK_SUCCESS;
+ return dma_resv_trylock(obj->resv);
}
static inline int
msm_gem_lock_interruptible(struct drm_gem_object *obj)
{
- struct msm_gem_object *msm_obj = to_msm_bo(obj);
- return mutex_lock_interruptible(_obj->lock);
+ return dma_resv_lock_interruptible(obj->resv, NULL);
}
static inline void
msm_gem_unlock(struct drm_gem_object *obj)
{
- struct msm_gem_object *msm_obj = to_msm_bo(obj);
- mutex_unlock(_obj->lock);
+ dma_resv_unlock(obj->resv);
}
static inline bool
msm_gem_is_locked(struct drm_gem_object *obj)
{
- struct msm_gem_object *msm_obj = to_msm_bo(obj);
- return mutex_is_locked(_obj->lock);
+ return dma_resv_is_locked(obj->resv);
}
static inline bool is_active(struct msm_gem_object *msm_obj)
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
b/drivers/gpu/drm/msm/msm_gem_submit.c
index a9422d043bfe..35b7d9d06850 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -215,7 +215,7 @@ static void submit_unlock_unpin_bo(struct msm_gem_submit
*submit,
struct msm_gem_object *msm_obj = submit->bos[i].obj;
if (submit->bos[i].flags & BO_PINNED)
- msm_gem_unpin_iova(_obj->base, submit->aspace);
+ msm_gem_unpin_iova_locked(_obj->base, submit->aspace);
if (submit->bos[i].flags & BO_LOCKED)
dma_resv_unlock(msm_obj->base.resv);
@@ -318,7 +318,7 @@ static int submit_pin_objects(struct msm_gem_submit *submit)
uint64_t iova;
/* if locking succeeded, pin bo: */
- ret = msm_gem_get_and_pin_iova(_obj->base,
+ ret = msm_gem_get_and_pin_iova_locked(_obj->base,
submit->aspace, );
if (ret)
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 55d16489d0f3..dbd9020713e5 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -784,7 +784,7 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct
msm_gem_submit *submit)
/* submit takes a reference to the bo and iova until retired: */
drm_gem_object_get(_obj->base);
- msm_gem_get_and_pin_iova(_obj->base, submit->aspace, );
+ msm_gem_get_and_pin_iova_locked(_obj->base, submit->aspace,
);
if (submit->bos[i].flags & MSM_SUBMIT_BO_WRITE)
dma_resv_add_excl_fence(drm_obj->resv, submit->fence);
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 15/22] drm/msm: Refcount submits
From: Rob Clark
Before we remove dev->struct_mutex from the retire path, we have to deal
with the situation of a submit retiring before the submit ioctl returns.
To deal with this, ring->submits will hold a reference to the submit,
which is dropped when the submit is retired. And the submit ioctl path
holds it's own ref, which it drops when it is done with the submit.
Also, add to submit list *after* getting/pinning bo's, to prevent badness
in case the completed fence is corrupted, and retire_worker mistakenly
believes the submit is done too early.
Signed-off-by: Rob Clark
Reviewed-by: Jordan Crouse
---
drivers/gpu/drm/msm/msm_drv.h| 1 -
drivers/gpu/drm/msm/msm_gem.h| 13 +
drivers/gpu/drm/msm/msm_gem_submit.c | 11 +--
drivers/gpu/drm/msm/msm_gpu.c| 21 -
4 files changed, 34 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index a17dadd38685..2ef5cff19883 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -277,7 +277,6 @@ void msm_unregister_mmu(struct drm_device *dev, struct
msm_mmu *mmu);
bool msm_use_mmu(struct drm_device *dev);
-void msm_gem_submit_free(struct msm_gem_submit *submit);
int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
struct drm_file *file);
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index ec01f35ce57b..93ee73c620ed 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -211,6 +211,7 @@ void msm_gem_free_work(struct work_struct *work);
* lasts for the duration of the submit-ioctl.
*/
struct msm_gem_submit {
+ struct kref ref;
struct drm_device *dev;
struct msm_gpu *gpu;
struct msm_gem_address_space *aspace;
@@ -247,6 +248,18 @@ struct msm_gem_submit {
} bos[];
};
+void __msm_gem_submit_destroy(struct kref *kref);
+
+static inline void msm_gem_submit_get(struct msm_gem_submit *submit)
+{
+ kref_get(>ref);
+}
+
+static inline void msm_gem_submit_put(struct msm_gem_submit *submit)
+{
+ kref_put(>ref, __msm_gem_submit_destroy);
+}
+
/* helper to determine of a buffer in submit should be dumped, used for both
* devcoredump and debugfs cmdstream dumping:
*/
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
b/drivers/gpu/drm/msm/msm_gem_submit.c
index a91c1b99db97..3151a0ca8904 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -42,6 +42,7 @@ static struct msm_gem_submit *submit_create(struct drm_device
*dev,
if (!submit)
return NULL;
+ kref_init(>ref);
submit->dev = dev;
submit->aspace = queue->ctx->aspace;
submit->gpu = gpu;
@@ -60,14 +61,13 @@ static struct msm_gem_submit *submit_create(struct
drm_device *dev,
return submit;
}
-void msm_gem_submit_free(struct msm_gem_submit *submit)
+void __msm_gem_submit_destroy(struct kref *kref)
{
+ struct msm_gem_submit *submit =
+ container_of(kref, struct msm_gem_submit, ref);
unsigned i;
dma_fence_put(submit->fence);
- spin_lock(>ring->submit_lock);
- list_del(>node);
- spin_unlock(>ring->submit_lock);
put_pid(submit->pid);
msm_submitqueue_put(submit->queue);
@@ -841,8 +841,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
submit_cleanup(submit);
if (has_ww_ticket)
ww_acquire_fini(>ticket);
- if (ret)
- msm_gem_submit_free(submit);
+ msm_gem_submit_put(submit);
out_unlock:
if (ret && (out_fence_fd >= 0))
put_unused_fd(out_fence_fd);
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index bb904e467b24..18a7948ac437 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -712,7 +712,12 @@ static void retire_submit(struct msm_gpu *gpu, struct
msm_ringbuffer *ring,
pm_runtime_mark_last_busy(>pdev->dev);
pm_runtime_put_autosuspend(>pdev->dev);
- msm_gem_submit_free(submit);
+
+ spin_lock(>submit_lock);
+ list_del(>node);
+ spin_unlock(>submit_lock);
+
+ msm_gem_submit_put(submit);
}
static void retire_submits(struct msm_gpu *gpu)
@@ -786,10 +791,6 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct
msm_gem_submit *submit)
submit->seqno = ++ring->seqno;
- spin_lock(>submit_lock);
- list_add_tail(>node, >submits);
- spin_unlock(>submit_lock);
-
msm_rd_dump_submit(priv->rd, submit, NULL);
update_sw_cntrs(gpu);
@@ -816,6 +817,16 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct
msm_gem_submit *submit)
msm_gem_active_get(drm_obj, gpu);
}
+ /*
+* ring->submits holds a ref to the submit, to deal with the case
+* that a submit completes before
[PATCH v2 14/22] drm/msm: Protect ring->submits with it's own lock
From: Rob Clark
One less place to rely on dev->struct_mutex.
Signed-off-by: Rob Clark
Reviewed-by: Jordan Crouse
---
drivers/gpu/drm/msm/msm_gem_submit.c | 2 ++
drivers/gpu/drm/msm/msm_gpu.c| 37 ++--
drivers/gpu/drm/msm/msm_ringbuffer.c | 1 +
drivers/gpu/drm/msm/msm_ringbuffer.h | 6 +
4 files changed, 39 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
b/drivers/gpu/drm/msm/msm_gem_submit.c
index 35b7d9d06850..a91c1b99db97 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -65,7 +65,9 @@ void msm_gem_submit_free(struct msm_gem_submit *submit)
unsigned i;
dma_fence_put(submit->fence);
+ spin_lock(>ring->submit_lock);
list_del(>node);
+ spin_unlock(>ring->submit_lock);
put_pid(submit->pid);
msm_submitqueue_put(submit->queue);
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index e5b7c8a77c99..bb904e467b24 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -270,6 +270,7 @@ static void update_fences(struct msm_gpu *gpu, struct
msm_ringbuffer *ring,
{
struct msm_gem_submit *submit;
+ spin_lock(>submit_lock);
list_for_each_entry(submit, >submits, node) {
if (submit->seqno > fence)
break;
@@ -277,6 +278,7 @@ static void update_fences(struct msm_gpu *gpu, struct
msm_ringbuffer *ring,
msm_update_fence(submit->ring->fctx,
submit->fence->seqno);
}
+ spin_unlock(>submit_lock);
}
#ifdef CONFIG_DEV_COREDUMP
@@ -430,11 +432,14 @@ find_submit(struct msm_ringbuffer *ring, uint32_t fence)
{
struct msm_gem_submit *submit;
- WARN_ON(!mutex_is_locked(>gpu->dev->struct_mutex));
-
- list_for_each_entry(submit, >submits, node)
- if (submit->seqno == fence)
+ spin_lock(>submit_lock);
+ list_for_each_entry(submit, >submits, node) {
+ if (submit->seqno == fence) {
+ spin_unlock(>submit_lock);
return submit;
+ }
+ }
+ spin_unlock(>submit_lock);
return NULL;
}
@@ -523,8 +528,10 @@ static void recover_worker(struct work_struct *work)
for (i = 0; i < gpu->nr_rings; i++) {
struct msm_ringbuffer *ring = gpu->rb[i];
+ spin_lock(>submit_lock);
list_for_each_entry(submit, >submits, node)
gpu->funcs->submit(gpu, submit);
+ spin_unlock(>submit_lock);
}
}
@@ -711,7 +718,6 @@ static void retire_submit(struct msm_gpu *gpu, struct
msm_ringbuffer *ring,
static void retire_submits(struct msm_gpu *gpu)
{
struct drm_device *dev = gpu->dev;
- struct msm_gem_submit *submit, *tmp;
int i;
WARN_ON(!mutex_is_locked(>struct_mutex));
@@ -720,9 +726,24 @@ static void retire_submits(struct msm_gpu *gpu)
for (i = 0; i < gpu->nr_rings; i++) {
struct msm_ringbuffer *ring = gpu->rb[i];
- list_for_each_entry_safe(submit, tmp, >submits, node) {
- if (dma_fence_is_signaled(submit->fence))
+ while (true) {
+ struct msm_gem_submit *submit = NULL;
+
+ spin_lock(>submit_lock);
+ submit = list_first_entry_or_null(>submits,
+ struct msm_gem_submit, node);
+ spin_unlock(>submit_lock);
+
+ /*
+* If no submit, we are done. If submit->fence hasn't
+* been signalled, then later submits are not signalled
+* either, so we are also done.
+*/
+ if (submit && dma_fence_is_signaled(submit->fence)) {
retire_submit(gpu, ring, submit);
+ } else {
+ break;
+ }
}
}
}
@@ -765,7 +786,9 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct
msm_gem_submit *submit)
submit->seqno = ++ring->seqno;
+ spin_lock(>submit_lock);
list_add_tail(>node, >submits);
+ spin_unlock(>submit_lock);
msm_rd_dump_submit(priv->rd, submit, NULL);
diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.c
b/drivers/gpu/drm/msm/msm_ringbuffer.c
index 1b6958e908dc..4d2a2a4abef8 100644
--- a/drivers/gpu/drm/msm/msm_ringbuffer.c
+++ b/drivers/gpu/drm/msm/msm_ringbuffer.c
@@ -46,6 +46,7 @@ struct msm_ringbuffer *msm_ringbuffer_new(struct msm_gpu
*gpu, int id,
ring->memptrs_iova = memptrs_iova;
INIT_LIST_HEAD(>submits);
+ spin_lock_init(>submit_lock);
spin_lock_init(>preempt_lock);
[PATCH v2 02/22] drm/msm/gem: Rename internal get_iova_locked helper
From: Rob Clark
We'll need to introduce a _locked() version of msm_gem_get_iova(), so we
need to make that name available.
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/msm_gem.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index afef9c6b1a1c..dec89fe79025 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -376,7 +376,7 @@ put_iova(struct drm_gem_object *obj)
}
}
-static int msm_gem_get_iova_locked(struct drm_gem_object *obj,
+static int get_iova_locked(struct drm_gem_object *obj,
struct msm_gem_address_space *aspace, uint64_t *iova,
u64 range_start, u64 range_end)
{
@@ -448,7 +448,7 @@ int msm_gem_get_and_pin_iova_range(struct drm_gem_object
*obj,
msm_gem_lock(obj);
- ret = msm_gem_get_iova_locked(obj, aspace, ,
+ ret = get_iova_locked(obj, aspace, ,
range_start, range_end);
if (!ret)
@@ -478,7 +478,7 @@ int msm_gem_get_iova(struct drm_gem_object *obj,
int ret;
msm_gem_lock(obj);
- ret = msm_gem_get_iova_locked(obj, aspace, iova, 0, U64_MAX);
+ ret = get_iova_locked(obj, aspace, iova, 0, U64_MAX);
msm_gem_unlock(obj);
return ret;
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 18/22] drm/msm: Drop struct_mutex in free_object() path
From: Rob Clark
Now that active_list/inactive_list is protected by mm_lock, we no longer
need dev->struct_mutex in the free_object() path.
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/msm_gem.c | 8
1 file changed, 8 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index cdbbdd848fe3..9ead1bf223e9 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -934,8 +934,6 @@ static void free_object(struct msm_gem_object *msm_obj)
struct drm_device *dev = obj->dev;
struct msm_drm_private *priv = dev->dev_private;
- WARN_ON(!mutex_is_locked(>struct_mutex));
-
/* object should not be on active list: */
WARN_ON(is_active(msm_obj));
@@ -972,20 +970,14 @@ void msm_gem_free_work(struct work_struct *work)
{
struct msm_drm_private *priv =
container_of(work, struct msm_drm_private, free_work);
- struct drm_device *dev = priv->dev;
struct llist_node *freed;
struct msm_gem_object *msm_obj, *next;
while ((freed = llist_del_all(>free_list))) {
-
- mutex_lock(>struct_mutex);
-
llist_for_each_entry_safe(msm_obj, next,
freed, freed)
free_object(msm_obj);
- mutex_unlock(>struct_mutex);
-
if (need_resched())
break;
}
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 19/22] drm/msm: remove msm_gem_free_work
From: Rob Clark
Now that we don't need struct_mutex in the free path, we can get rid of
the asynchronous free altogether.
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/msm_drv.c | 3 ---
drivers/gpu/drm/msm/msm_drv.h | 5 -
drivers/gpu/drm/msm/msm_gem.c | 27 ---
drivers/gpu/drm/msm/msm_gem.h | 1 -
4 files changed, 36 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 81cb2cecc829..49e6daf30b42 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -438,9 +438,6 @@ static int msm_drm_init(struct device *dev, struct
drm_driver *drv)
priv->wq = alloc_ordered_workqueue("msm", 0);
- INIT_WORK(>free_work, msm_gem_free_work);
- init_llist_head(>free_list);
-
INIT_LIST_HEAD(>inactive_list);
mutex_init(>mm_lock);
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 2ef5cff19883..af296712eae8 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -188,10 +188,6 @@ struct msm_drm_private {
struct list_head inactive_list;
struct mutex mm_lock;
- /* worker for delayed free of objects: */
- struct work_struct free_work;
- struct llist_head free_list;
-
struct workqueue_struct *wq;
unsigned int num_planes;
@@ -291,7 +287,6 @@ struct drm_gem_object *msm_gem_prime_import_sg_table(struct
drm_device *dev,
struct dma_buf_attachment *attach, struct sg_table *sg);
int msm_gem_prime_pin(struct drm_gem_object *obj);
void msm_gem_prime_unpin(struct drm_gem_object *obj);
-void msm_gem_free_work(struct work_struct *work);
int msm_framebuffer_prepare(struct drm_framebuffer *fb,
struct msm_gem_address_space *aspace);
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 9ead1bf223e9..b60eaf6266e2 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -924,16 +924,6 @@ void msm_gem_free_object(struct drm_gem_object *obj)
struct drm_device *dev = obj->dev;
struct msm_drm_private *priv = dev->dev_private;
- if (llist_add(_obj->freed, >free_list))
- queue_work(priv->wq, >free_work);
-}
-
-static void free_object(struct msm_gem_object *msm_obj)
-{
- struct drm_gem_object *obj = _obj->base;
- struct drm_device *dev = obj->dev;
- struct msm_drm_private *priv = dev->dev_private;
-
/* object should not be on active list: */
WARN_ON(is_active(msm_obj));
@@ -966,23 +956,6 @@ static void free_object(struct msm_gem_object *msm_obj)
kfree(msm_obj);
}
-void msm_gem_free_work(struct work_struct *work)
-{
- struct msm_drm_private *priv =
- container_of(work, struct msm_drm_private, free_work);
- struct llist_node *freed;
- struct msm_gem_object *msm_obj, *next;
-
- while ((freed = llist_del_all(>free_list))) {
- llist_for_each_entry_safe(msm_obj, next,
- freed, freed)
- free_object(msm_obj);
-
- if (need_resched())
- break;
- }
-}
-
/* convenience method to construct a GEM buffer object, and userspace handle */
int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
uint32_t size, uint32_t flags, uint32_t *handle,
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index bf5f9e94d0d3..c12fedf88e85 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -202,7 +202,6 @@ static inline bool is_vunmapable(struct msm_gem_object
*msm_obj)
void msm_gem_purge(struct drm_gem_object *obj);
void msm_gem_vunmap(struct drm_gem_object *obj);
-void msm_gem_free_work(struct work_struct *work);
/* Created per submit-ioctl, to track bo's and cmdstream bufs, etc,
* associated with the cmdstream submission for synchronization (and
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 13/22] drm/msm: Document and rename preempt_lock
From: Rob Clark
Before adding another lock, give ring->lock a more descriptive name.
Signed-off-by: Rob Clark
Reviewed-by: Jordan Crouse
---
drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 4 ++--
drivers/gpu/drm/msm/adreno/a5xx_preempt.c | 12 ++--
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 4 ++--
drivers/gpu/drm/msm/msm_ringbuffer.c | 2 +-
drivers/gpu/drm/msm/msm_ringbuffer.h | 7 ++-
5 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index c941c8138f25..543437a2186e 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -36,7 +36,7 @@ void a5xx_flush(struct msm_gpu *gpu, struct msm_ringbuffer
*ring,
OUT_RING(ring, upper_32_bits(shadowptr(a5xx_gpu, ring)));
}
- spin_lock_irqsave(>lock, flags);
+ spin_lock_irqsave(>preempt_lock, flags);
/* Copy the shadow to the actual register */
ring->cur = ring->next;
@@ -44,7 +44,7 @@ void a5xx_flush(struct msm_gpu *gpu, struct msm_ringbuffer
*ring,
/* Make sure to wrap wptr if we need to */
wptr = get_wptr(ring);
- spin_unlock_irqrestore(>lock, flags);
+ spin_unlock_irqrestore(>preempt_lock, flags);
/* Make sure everything is posted before making a decision */
mb();
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_preempt.c
b/drivers/gpu/drm/msm/adreno/a5xx_preempt.c
index 7e04509c4e1f..183de1139eeb 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_preempt.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_preempt.c
@@ -45,9 +45,9 @@ static inline void update_wptr(struct msm_gpu *gpu, struct
msm_ringbuffer *ring)
if (!ring)
return;
- spin_lock_irqsave(>lock, flags);
+ spin_lock_irqsave(>preempt_lock, flags);
wptr = get_wptr(ring);
- spin_unlock_irqrestore(>lock, flags);
+ spin_unlock_irqrestore(>preempt_lock, flags);
gpu_write(gpu, REG_A5XX_CP_RB_WPTR, wptr);
}
@@ -62,9 +62,9 @@ static struct msm_ringbuffer *get_next_ring(struct msm_gpu
*gpu)
bool empty;
struct msm_ringbuffer *ring = gpu->rb[i];
- spin_lock_irqsave(>lock, flags);
+ spin_lock_irqsave(>preempt_lock, flags);
empty = (get_wptr(ring) == ring->memptrs->rptr);
- spin_unlock_irqrestore(>lock, flags);
+ spin_unlock_irqrestore(>preempt_lock, flags);
if (!empty)
return ring;
@@ -132,9 +132,9 @@ void a5xx_preempt_trigger(struct msm_gpu *gpu)
}
/* Make sure the wptr doesn't update while we're in motion */
- spin_lock_irqsave(>lock, flags);
+ spin_lock_irqsave(>preempt_lock, flags);
a5xx_gpu->preempt[ring->id]->wptr = get_wptr(ring);
- spin_unlock_irqrestore(>lock, flags);
+ spin_unlock_irqrestore(>preempt_lock, flags);
/* Set the address of the incoming preemption record */
gpu_write64(gpu, REG_A5XX_CP_CONTEXT_SWITCH_RESTORE_ADDR_LO,
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 8915882e..fc85f008d69d 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -65,7 +65,7 @@ static void a6xx_flush(struct msm_gpu *gpu, struct
msm_ringbuffer *ring)
OUT_RING(ring, upper_32_bits(shadowptr(a6xx_gpu, ring)));
}
- spin_lock_irqsave(>lock, flags);
+ spin_lock_irqsave(>preempt_lock, flags);
/* Copy the shadow to the actual register */
ring->cur = ring->next;
@@ -73,7 +73,7 @@ static void a6xx_flush(struct msm_gpu *gpu, struct
msm_ringbuffer *ring)
/* Make sure to wrap wptr if we need to */
wptr = get_wptr(ring);
- spin_unlock_irqrestore(>lock, flags);
+ spin_unlock_irqrestore(>preempt_lock, flags);
/* Make sure everything is posted before making a decision */
mb();
diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.c
b/drivers/gpu/drm/msm/msm_ringbuffer.c
index 935bf9b1d941..1b6958e908dc 100644
--- a/drivers/gpu/drm/msm/msm_ringbuffer.c
+++ b/drivers/gpu/drm/msm/msm_ringbuffer.c
@@ -46,7 +46,7 @@ struct msm_ringbuffer *msm_ringbuffer_new(struct msm_gpu
*gpu, int id,
ring->memptrs_iova = memptrs_iova;
INIT_LIST_HEAD(>submits);
- spin_lock_init(>lock);
+ spin_lock_init(>preempt_lock);
snprintf(name, sizeof(name), "gpu-ring-%d", ring->id);
diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.h
b/drivers/gpu/drm/msm/msm_ringbuffer.h
index 0987d6bf848c..4956d1bc5d0e 100644
--- a/drivers/gpu/drm/msm/msm_ringbuffer.h
+++ b/drivers/gpu/drm/msm/msm_ringbuffer.h
@@ -46,7 +46,12 @@ struct msm_ringbuffer {
struct msm_rbmemptrs *memptrs;
uint64_t memptrs_iova;
struct msm_fence_context *fctx;
- spinlock_t lock;
+
+ /*
+*
[PATCH v2 11/22] drm/msm: Move update_fences()
From: Rob Clark
Small cleanup, update_fences() is used in the hangcheck path, but also
in the normal retire path.
Signed-off-by: Rob Clark
Reviewed-by: Jordan Crouse
---
drivers/gpu/drm/msm/msm_gpu.c | 28 ++--
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 677b11c5a151..e5b7c8a77c99 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -265,6 +265,20 @@ int msm_gpu_hw_init(struct msm_gpu *gpu)
return ret;
}
+static void update_fences(struct msm_gpu *gpu, struct msm_ringbuffer *ring,
+ uint32_t fence)
+{
+ struct msm_gem_submit *submit;
+
+ list_for_each_entry(submit, >submits, node) {
+ if (submit->seqno > fence)
+ break;
+
+ msm_update_fence(submit->ring->fctx,
+ submit->fence->seqno);
+ }
+}
+
#ifdef CONFIG_DEV_COREDUMP
static ssize_t msm_gpu_devcoredump_read(char *buffer, loff_t offset,
size_t count, void *data, size_t datalen)
@@ -411,20 +425,6 @@ static void msm_gpu_crashstate_capture(struct msm_gpu *gpu,
* Hangcheck detection for locked gpu:
*/
-static void update_fences(struct msm_gpu *gpu, struct msm_ringbuffer *ring,
- uint32_t fence)
-{
- struct msm_gem_submit *submit;
-
- list_for_each_entry(submit, >submits, node) {
- if (submit->seqno > fence)
- break;
-
- msm_update_fence(submit->ring->fctx,
- submit->fence->seqno);
- }
-}
-
static struct msm_gem_submit *
find_submit(struct msm_ringbuffer *ring, uint32_t fence)
{
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 05/22] drm/msm/gem: Move locking in shrinker path
From: Rob Clark
Move grabbing the bo lock into shrinker, with a msm_gem_trylock() to
skip over bo's that are already locked. This gets rid of the nested
lock classes.
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/msm_gem.c | 24 +
drivers/gpu/drm/msm/msm_gem.h | 29 ++
drivers/gpu/drm/msm/msm_gem_shrinker.c | 27 +---
3 files changed, 35 insertions(+), 45 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 7bca2e815933..ff8ca257bdc6 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -17,8 +17,6 @@
#include "msm_gpu.h"
#include "msm_mmu.h"
-static void msm_gem_vunmap_locked(struct drm_gem_object *obj);
-
static dma_addr_t physaddr(struct drm_gem_object *obj)
{
@@ -672,20 +670,19 @@ int msm_gem_madvise(struct drm_gem_object *obj, unsigned
madv)
return (madv != __MSM_MADV_PURGED);
}
-void msm_gem_purge(struct drm_gem_object *obj, enum msm_gem_lock subclass)
+void msm_gem_purge(struct drm_gem_object *obj)
{
struct drm_device *dev = obj->dev;
struct msm_gem_object *msm_obj = to_msm_bo(obj);
WARN_ON(!mutex_is_locked(>struct_mutex));
+ WARN_ON(!msm_gem_is_locked(obj));
WARN_ON(!is_purgeable(msm_obj));
WARN_ON(obj->import_attach);
- mutex_lock_nested(_obj->lock, subclass);
-
put_iova(obj);
- msm_gem_vunmap_locked(obj);
+ msm_gem_vunmap(obj);
put_pages(obj);
@@ -703,11 +700,9 @@ void msm_gem_purge(struct drm_gem_object *obj, enum
msm_gem_lock subclass)
invalidate_mapping_pages(file_inode(obj->filp)->i_mapping,
0, (loff_t)-1);
-
- msm_gem_unlock(obj);
}
-static void msm_gem_vunmap_locked(struct drm_gem_object *obj)
+void msm_gem_vunmap(struct drm_gem_object *obj)
{
struct msm_gem_object *msm_obj = to_msm_bo(obj);
@@ -720,15 +715,6 @@ static void msm_gem_vunmap_locked(struct drm_gem_object
*obj)
msm_obj->vaddr = NULL;
}
-void msm_gem_vunmap(struct drm_gem_object *obj, enum msm_gem_lock subclass)
-{
- struct msm_gem_object *msm_obj = to_msm_bo(obj);
-
- mutex_lock_nested(_obj->lock, subclass);
- msm_gem_vunmap_locked(obj);
- msm_gem_unlock(obj);
-}
-
/* must be called before _move_to_active().. */
int msm_gem_sync_object(struct drm_gem_object *obj,
struct msm_fence_context *fctx, bool exclusive)
@@ -965,7 +951,7 @@ static void free_object(struct msm_gem_object *msm_obj)
drm_prime_gem_destroy(obj, msm_obj->sgt);
} else {
- msm_gem_vunmap_locked(obj);
+ msm_gem_vunmap(obj);
put_pages(obj);
}
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index 016f616dd118..947eeaca661d 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -160,6 +160,13 @@ msm_gem_lock(struct drm_gem_object *obj)
mutex_lock(_obj->lock);
}
+static inline bool __must_check
+msm_gem_trylock(struct drm_gem_object *obj)
+{
+ struct msm_gem_object *msm_obj = to_msm_bo(obj);
+ return mutex_trylock_recursive(_obj->lock) == MUTEX_TRYLOCK_SUCCESS;
+}
+
static inline int
msm_gem_lock_interruptible(struct drm_gem_object *obj)
{
@@ -188,6 +195,7 @@ static inline bool is_active(struct msm_gem_object *msm_obj)
static inline bool is_purgeable(struct msm_gem_object *msm_obj)
{
+ WARN_ON(!msm_gem_is_locked(_obj->base));
WARN_ON(!mutex_is_locked(_obj->base.dev->struct_mutex));
return (msm_obj->madv == MSM_MADV_DONTNEED) && msm_obj->sgt &&
!msm_obj->base.dma_buf && !msm_obj->base.import_attach;
@@ -195,27 +203,12 @@ static inline bool is_purgeable(struct msm_gem_object
*msm_obj)
static inline bool is_vunmapable(struct msm_gem_object *msm_obj)
{
+ WARN_ON(!msm_gem_is_locked(_obj->base));
return (msm_obj->vmap_count == 0) && msm_obj->vaddr;
}
-/* The shrinker can be triggered while we hold objA->lock, and need
- * to grab objB->lock to purge it. Lockdep just sees these as a single
- * class of lock, so we use subclasses to teach it the difference.
- *
- * OBJ_LOCK_NORMAL is implicit (ie. normal mutex_lock() call), and
- * OBJ_LOCK_SHRINKER is used by shrinker.
- *
- * It is *essential* that we never go down paths that could trigger the
- * shrinker for a purgable object. This is ensured by checking that
- * msm_obj->madv == MSM_MADV_WILLNEED.
- */
-enum msm_gem_lock {
- OBJ_LOCK_NORMAL,
- OBJ_LOCK_SHRINKER,
-};
-
-void msm_gem_purge(struct drm_gem_object *obj, enum msm_gem_lock subclass);
-void msm_gem_vunmap(struct drm_gem_object *obj, enum msm_gem_lock subclass);
+void msm_gem_purge(struct drm_gem_object *obj);
+void msm_gem_vunmap(struct drm_gem_object *obj);
void msm_gem_free_work(struct work_struct *work);
/* Created per submit-ioctl, to track
[PATCH v2 09/22] drm/msm: Use correct drm_gem_object_put() in fail case
From: Rob Clark
We only want to use the _unlocked() variant in the unlocked case.
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/msm_gem.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 210bf5c9c2dd..833e3d3c6e8c 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -1120,7 +1120,11 @@ static struct drm_gem_object *_msm_gem_new(struct
drm_device *dev,
return obj;
fail:
- drm_gem_object_put(obj);
+ if (struct_mutex_locked) {
+ drm_gem_object_put_locked(obj);
+ } else {
+ drm_gem_object_put(obj);
+ }
return ERR_PTR(ret);
}
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 03/22] drm/msm/gem: Move prototypes to msm_gem.h
From: Rob Clark
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 1 +
drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 1 +
drivers/gpu/drm/msm/dsi/dsi_host.c| 1 +
drivers/gpu/drm/msm/msm_drv.h | 54 --
drivers/gpu/drm/msm/msm_fbdev.c | 1 +
drivers/gpu/drm/msm/msm_gem.h | 56 +++
6 files changed, 60 insertions(+), 54 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c
b/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c
index a0253297bc76..b65b2329cc8d 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c
@@ -11,6 +11,7 @@
#include
#include "mdp4_kms.h"
+#include "msm_gem.h"
struct mdp4_crtc {
struct drm_crtc base;
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
index c39dad151bb6..81fbd52ad7e7 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
@@ -15,6 +15,7 @@
#include
#include "mdp5_kms.h"
+#include "msm_gem.h"
#define CURSOR_WIDTH 64
#define CURSOR_HEIGHT 64
diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c
b/drivers/gpu/drm/msm/dsi/dsi_host.c
index b17ac6c27554..5e7cdc11c764 100644
--- a/drivers/gpu/drm/msm/dsi/dsi_host.c
+++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
@@ -26,6 +26,7 @@
#include "sfpb.xml.h"
#include "dsi_cfg.h"
#include "msm_kms.h"
+#include "msm_gem.h"
#define DSI_RESET_TOGGLE_DELAY_MS 20
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index b9dd8f8f4887..79ee7d05b363 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -273,28 +273,6 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void
*data,
void msm_gem_shrinker_init(struct drm_device *dev);
void msm_gem_shrinker_cleanup(struct drm_device *dev);
-int msm_gem_mmap_obj(struct drm_gem_object *obj,
- struct vm_area_struct *vma);
-int msm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
-vm_fault_t msm_gem_fault(struct vm_fault *vmf);
-uint64_t msm_gem_mmap_offset(struct drm_gem_object *obj);
-int msm_gem_get_iova(struct drm_gem_object *obj,
- struct msm_gem_address_space *aspace, uint64_t *iova);
-int msm_gem_get_and_pin_iova_range(struct drm_gem_object *obj,
- struct msm_gem_address_space *aspace, uint64_t *iova,
- u64 range_start, u64 range_end);
-int msm_gem_get_and_pin_iova(struct drm_gem_object *obj,
- struct msm_gem_address_space *aspace, uint64_t *iova);
-uint64_t msm_gem_iova(struct drm_gem_object *obj,
- struct msm_gem_address_space *aspace);
-void msm_gem_unpin_iova(struct drm_gem_object *obj,
- struct msm_gem_address_space *aspace);
-struct page **msm_gem_get_pages(struct drm_gem_object *obj);
-void msm_gem_put_pages(struct drm_gem_object *obj);
-int msm_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
- struct drm_mode_create_dumb *args);
-int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev,
- uint32_t handle, uint64_t *offset);
struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj);
void *msm_gem_prime_vmap(struct drm_gem_object *obj);
void msm_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr);
@@ -303,38 +281,8 @@ struct drm_gem_object
*msm_gem_prime_import_sg_table(struct drm_device *dev,
struct dma_buf_attachment *attach, struct sg_table *sg);
int msm_gem_prime_pin(struct drm_gem_object *obj);
void msm_gem_prime_unpin(struct drm_gem_object *obj);
-void *msm_gem_get_vaddr(struct drm_gem_object *obj);
-void *msm_gem_get_vaddr_active(struct drm_gem_object *obj);
-void msm_gem_put_vaddr(struct drm_gem_object *obj);
-int msm_gem_madvise(struct drm_gem_object *obj, unsigned madv);
-int msm_gem_sync_object(struct drm_gem_object *obj,
- struct msm_fence_context *fctx, bool exclusive);
-void msm_gem_active_get(struct drm_gem_object *obj, struct msm_gpu *gpu);
-void msm_gem_active_put(struct drm_gem_object *obj);
-int msm_gem_cpu_prep(struct drm_gem_object *obj, uint32_t op, ktime_t
*timeout);
-int msm_gem_cpu_fini(struct drm_gem_object *obj);
-void msm_gem_free_object(struct drm_gem_object *obj);
-int msm_gem_new_handle(struct drm_device *dev, struct drm_file *file,
- uint32_t size, uint32_t flags, uint32_t *handle, char *name);
-struct drm_gem_object *msm_gem_new(struct drm_device *dev,
- uint32_t size, uint32_t flags);
-struct drm_gem_object *msm_gem_new_locked(struct drm_device *dev,
- uint32_t size, uint32_t flags);
-void *msm_gem_kernel_new(struct drm_device *dev, uint32_t size,
- uint32_t flags, struct msm_gem_address_space *aspace,
- struct drm_gem_object **bo, uint64_t *iova);
-void *msm_gem_kernel_new_locked(struct drm_device *dev, uint32_t size,
-
[PATCH v2 16/22] drm/msm: Remove obj->gpu
From: Rob Clark
It cannot be atomically updated with obj->active_count, and the only
purpose is a useless WARN_ON() (which becomes a buggy WARN_ON() once
retire_submits() is not serialized with incoming submits via
struct_mutex)
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/msm_gem.c | 2 --
drivers/gpu/drm/msm/msm_gem.h | 1 -
drivers/gpu/drm/msm/msm_gpu.c | 5 -
3 files changed, 8 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 15f81ed2e154..cdbbdd848fe3 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -760,7 +760,6 @@ void msm_gem_active_get(struct drm_gem_object *obj, struct
msm_gpu *gpu)
if (!atomic_fetch_inc(_obj->active_count)) {
mutex_lock(>mm_lock);
- msm_obj->gpu = gpu;
list_del_init(_obj->mm_list);
list_add_tail(_obj->mm_list, >active_list);
mutex_unlock(>mm_lock);
@@ -776,7 +775,6 @@ void msm_gem_active_put(struct drm_gem_object *obj)
if (!atomic_dec_return(_obj->active_count)) {
mutex_lock(>mm_lock);
- msm_obj->gpu = NULL;
list_del_init(_obj->mm_list);
list_add_tail(_obj->mm_list, >inactive_list);
mutex_unlock(>mm_lock);
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index 93ee73c620ed..bf5f9e94d0d3 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -64,7 +64,6 @@ struct msm_gem_object {
*
*/
struct list_head mm_list;
- struct msm_gpu *gpu; /* non-null if active */
/* Transiently in the process of submit ioctl, objects associated
* with the submit are on submit->bo_list.. this only lasts for
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 18a7948ac437..8278a4df331a 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -800,11 +800,6 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct
msm_gem_submit *submit)
struct drm_gem_object *drm_obj = _obj->base;
uint64_t iova;
- /* can't happen yet.. but when we add 2d support we'll have
-* to deal w/ cross-ring synchronization:
-*/
- WARN_ON(is_active(msm_obj) && (msm_obj->gpu != gpu));
-
/* submit takes a reference to the bo and iova until retired: */
drm_gem_object_get(_obj->base);
msm_gem_get_and_pin_iova_locked(_obj->base, submit->aspace,
);
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 07/22] drm/msm: Do rpm get sooner in the submit path
From: Rob Clark Unfortunately, due to an dev_pm_opp locking interaction with mm->mmap_sem, we need to do pm get before aquiring obj locks, otherwise we can have anger lockdep with the chain: opp_table_lock --> >mmap_sem --> reservation_ww_class_mutex For an explicit fencing userspace, the impact should be minimal as we do all the fence waits before this point. It could result in some needless resumes in error cases, etc. Signed-off-by: Rob Clark --- drivers/gpu/drm/msm/msm_gem_submit.c | 15 +-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c index 002130d826aa..a9422d043bfe 100644 --- a/drivers/gpu/drm/msm/msm_gem_submit.c +++ b/drivers/gpu/drm/msm/msm_gem_submit.c @@ -744,11 +744,20 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, ret = submit_lookup_objects(submit, args, file); if (ret) - goto out; + goto out_pre_pm; ret = submit_lookup_cmds(submit, args, file); if (ret) - goto out; + goto out_pre_pm; + + /* +* Thanks to dev_pm_opp opp_table_lock interactions with mm->mmap_sem +* in the resume path, we need to to rpm get before we lock objs. +* Which unfortunately might involve powering up the GPU sooner than +* is necessary. But at least in the explicit fencing case, we will +* have already done all the fence waiting. +*/ + pm_runtime_get_sync(>pdev->dev); /* copy_*_user while holding a ww ticket upsets lockdep */ ww_acquire_init(>ticket, _ww_class); @@ -825,6 +834,8 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, out: + pm_runtime_put(>pdev->dev); +out_pre_pm: submit_cleanup(submit); if (has_ww_ticket) ww_acquire_fini(>ticket); -- 2.26.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 12/22] drm/msm: Add priv->mm_lock to protect active/inactive lists
From: Rob Clark
Rather than relying on the big dev->struct_mutex hammer, introduce a
more specific lock for protecting the bo lists.
Signed-off-by: Rob Clark
Reviewed-by: Jordan Crouse
---
drivers/gpu/drm/msm/msm_debugfs.c | 7 +++
drivers/gpu/drm/msm/msm_drv.c | 7 +++
drivers/gpu/drm/msm/msm_drv.h | 13 +++-
drivers/gpu/drm/msm/msm_gem.c | 28 +++---
drivers/gpu/drm/msm/msm_gem_shrinker.c | 12 +++
drivers/gpu/drm/msm/msm_gpu.h | 5 -
6 files changed, 58 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_debugfs.c
b/drivers/gpu/drm/msm/msm_debugfs.c
index ee2e270f464c..64afbed89821 100644
--- a/drivers/gpu/drm/msm/msm_debugfs.c
+++ b/drivers/gpu/drm/msm/msm_debugfs.c
@@ -112,6 +112,11 @@ static int msm_gem_show(struct drm_device *dev, struct
seq_file *m)
{
struct msm_drm_private *priv = dev->dev_private;
struct msm_gpu *gpu = priv->gpu;
+ int ret;
+
+ ret = mutex_lock_interruptible(>mm_lock);
+ if (ret)
+ return ret;
if (gpu) {
seq_printf(m, "Active Objects (%s):\n", gpu->name);
@@ -121,6 +126,8 @@ static int msm_gem_show(struct drm_device *dev, struct
seq_file *m)
seq_printf(m, "Inactive Objects:\n");
msm_gem_describe_objects(>inactive_list, m);
+ mutex_unlock(>mm_lock);
+
return 0;
}
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 49685571dc0e..81cb2cecc829 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -7,6 +7,7 @@
#include
#include
+#include
#include
#include
@@ -441,6 +442,12 @@ static int msm_drm_init(struct device *dev, struct
drm_driver *drv)
init_llist_head(>free_list);
INIT_LIST_HEAD(>inactive_list);
+ mutex_init(>mm_lock);
+
+ /* Teach lockdep about lock ordering wrt. shrinker: */
+ fs_reclaim_acquire(GFP_KERNEL);
+ might_lock(>mm_lock);
+ fs_reclaim_release(GFP_KERNEL);
drm_mode_config_init(ddev);
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 79ee7d05b363..a17dadd38685 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -174,8 +174,19 @@ struct msm_drm_private {
struct msm_rd_state *hangrd; /* debugfs to dump hanging submits */
struct msm_perf_state *perf;
- /* list of GEM objects: */
+ /*
+* List of inactive GEM objects. Every bo is either in the
inactive_list
+* or gpu->active_list (for the gpu it is active on[1])
+*
+* These lists are protected by mm_lock. If struct_mutex is involved,
it
+* should be aquired prior to mm_lock. One should *not* hold mm_lock in
+* get_pages()/vmap()/etc paths, as they can trigger the shrinker.
+*
+* [1] if someone ever added support for the old 2d cores, there could
be
+* more than one gpu object
+*/
struct list_head inactive_list;
+ struct mutex mm_lock;
/* worker for delayed free of objects: */
struct work_struct free_work;
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 833e3d3c6e8c..15f81ed2e154 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -753,13 +753,17 @@ int msm_gem_sync_object(struct drm_gem_object *obj,
void msm_gem_active_get(struct drm_gem_object *obj, struct msm_gpu *gpu)
{
struct msm_gem_object *msm_obj = to_msm_bo(obj);
- WARN_ON(!mutex_is_locked(>dev->struct_mutex));
+ struct msm_drm_private *priv = obj->dev->dev_private;
+
+ might_sleep();
WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED);
if (!atomic_fetch_inc(_obj->active_count)) {
+ mutex_lock(>mm_lock);
msm_obj->gpu = gpu;
list_del_init(_obj->mm_list);
list_add_tail(_obj->mm_list, >active_list);
+ mutex_unlock(>mm_lock);
}
}
@@ -768,12 +772,14 @@ void msm_gem_active_put(struct drm_gem_object *obj)
struct msm_gem_object *msm_obj = to_msm_bo(obj);
struct msm_drm_private *priv = obj->dev->dev_private;
- WARN_ON(!mutex_is_locked(>dev->struct_mutex));
+ might_sleep();
if (!atomic_dec_return(_obj->active_count)) {
+ mutex_lock(>mm_lock);
msm_obj->gpu = NULL;
list_del_init(_obj->mm_list);
list_add_tail(_obj->mm_list, >inactive_list);
+ mutex_unlock(>mm_lock);
}
}
@@ -928,13 +934,16 @@ static void free_object(struct msm_gem_object *msm_obj)
{
struct drm_gem_object *obj = _obj->base;
struct drm_device *dev = obj->dev;
+ struct msm_drm_private *priv = dev->dev_private;
WARN_ON(!mutex_is_locked(>struct_mutex));
/* object should not be on active list: */
[PATCH v2 04/22] drm/msm/gem: Add some _locked() helpers
From: Rob Clark
When we cut-over to using dma_resv_lock/etc instead of msm_obj->lock,
we'll need these for the submit path (where resv->lock is already held).
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/msm_gem.c | 50 +++
drivers/gpu/drm/msm/msm_gem.h | 4 +++
2 files changed, 43 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index dec89fe79025..7bca2e815933 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -435,18 +435,14 @@ static int msm_gem_pin_iova(struct drm_gem_object *obj,
msm_obj->sgt, obj->size >> PAGE_SHIFT);
}
-/*
- * get iova and pin it. Should have a matching put
- * limits iova to specified range (in pages)
- */
-int msm_gem_get_and_pin_iova_range(struct drm_gem_object *obj,
+static int get_and_pin_iova_range_locked(struct drm_gem_object *obj,
struct msm_gem_address_space *aspace, uint64_t *iova,
u64 range_start, u64 range_end)
{
u64 local;
int ret;
- msm_gem_lock(obj);
+ WARN_ON(!msm_gem_is_locked(obj));
ret = get_iova_locked(obj, aspace, ,
range_start, range_end);
@@ -457,10 +453,32 @@ int msm_gem_get_and_pin_iova_range(struct drm_gem_object
*obj,
if (!ret)
*iova = local;
+ return ret;
+}
+
+/*
+ * get iova and pin it. Should have a matching put
+ * limits iova to specified range (in pages)
+ */
+int msm_gem_get_and_pin_iova_range(struct drm_gem_object *obj,
+ struct msm_gem_address_space *aspace, uint64_t *iova,
+ u64 range_start, u64 range_end)
+{
+ int ret;
+
+ msm_gem_lock(obj);
+ ret = get_and_pin_iova_range_locked(obj, aspace, iova, range_start,
range_end);
msm_gem_unlock(obj);
+
return ret;
}
+int msm_gem_get_and_pin_iova_locked(struct drm_gem_object *obj,
+ struct msm_gem_address_space *aspace, uint64_t *iova)
+{
+ return get_and_pin_iova_range_locked(obj, aspace, iova, 0, U64_MAX);
+}
+
/* get iova and pin it. Should have a matching put */
int msm_gem_get_and_pin_iova(struct drm_gem_object *obj,
struct msm_gem_address_space *aspace, uint64_t *iova)
@@ -501,21 +519,31 @@ uint64_t msm_gem_iova(struct drm_gem_object *obj,
}
/*
- * Unpin a iova by updating the reference counts. The memory isn't actually
- * purged until something else (shrinker, mm_notifier, destroy, etc) decides
- * to get rid of it
+ * Locked variant of msm_gem_unpin_iova()
*/
-void msm_gem_unpin_iova(struct drm_gem_object *obj,
+void msm_gem_unpin_iova_locked(struct drm_gem_object *obj,
struct msm_gem_address_space *aspace)
{
struct msm_gem_vma *vma;
- msm_gem_lock(obj);
+ WARN_ON(!msm_gem_is_locked(obj));
+
vma = lookup_vma(obj, aspace);
if (!WARN_ON(!vma))
msm_gem_unmap_vma(aspace, vma);
+}
+/*
+ * Unpin a iova by updating the reference counts. The memory isn't actually
+ * purged until something else (shrinker, mm_notifier, destroy, etc) decides
+ * to get rid of it
+ */
+void msm_gem_unpin_iova(struct drm_gem_object *obj,
+ struct msm_gem_address_space *aspace)
+{
+ msm_gem_lock(obj);
+ msm_gem_unpin_iova_locked(obj, aspace);
msm_gem_unlock(obj);
}
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index fbad08badf43..016f616dd118 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -103,10 +103,14 @@ int msm_gem_get_iova(struct drm_gem_object *obj,
int msm_gem_get_and_pin_iova_range(struct drm_gem_object *obj,
struct msm_gem_address_space *aspace, uint64_t *iova,
u64 range_start, u64 range_end);
+int msm_gem_get_and_pin_iova_locked(struct drm_gem_object *obj,
+ struct msm_gem_address_space *aspace, uint64_t *iova);
int msm_gem_get_and_pin_iova(struct drm_gem_object *obj,
struct msm_gem_address_space *aspace, uint64_t *iova);
uint64_t msm_gem_iova(struct drm_gem_object *obj,
struct msm_gem_address_space *aspace);
+void msm_gem_unpin_iova_locked(struct drm_gem_object *obj,
+ struct msm_gem_address_space *aspace);
void msm_gem_unpin_iova(struct drm_gem_object *obj,
struct msm_gem_address_space *aspace);
struct page **msm_gem_get_pages(struct drm_gem_object *obj);
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 01/22] drm/msm/gem: Add obj->lock wrappers
From: Rob Clark
This will make it easier to transition over to obj->resv locking for
everything that is per-bo locking.
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/msm_gem.c | 99 ---
drivers/gpu/drm/msm/msm_gem.h | 28 ++
2 files changed, 74 insertions(+), 53 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 14e14caf90f9..afef9c6b1a1c 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -178,15 +178,15 @@ struct page **msm_gem_get_pages(struct drm_gem_object
*obj)
struct msm_gem_object *msm_obj = to_msm_bo(obj);
struct page **p;
- mutex_lock(_obj->lock);
+ msm_gem_lock(obj);
if (WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED)) {
- mutex_unlock(_obj->lock);
+ msm_gem_unlock(obj);
return ERR_PTR(-EBUSY);
}
p = get_pages(obj);
- mutex_unlock(_obj->lock);
+ msm_gem_unlock(obj);
return p;
}
@@ -252,14 +252,14 @@ vm_fault_t msm_gem_fault(struct vm_fault *vmf)
* vm_ops.open/drm_gem_mmap_obj and close get and put
* a reference on obj. So, we dont need to hold one here.
*/
- err = mutex_lock_interruptible(_obj->lock);
+ err = msm_gem_lock_interruptible(obj);
if (err) {
ret = VM_FAULT_NOPAGE;
goto out;
}
if (WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED)) {
- mutex_unlock(_obj->lock);
+ msm_gem_unlock(obj);
return VM_FAULT_SIGBUS;
}
@@ -280,7 +280,7 @@ vm_fault_t msm_gem_fault(struct vm_fault *vmf)
ret = vmf_insert_mixed(vma, vmf->address, __pfn_to_pfn_t(pfn, PFN_DEV));
out_unlock:
- mutex_unlock(_obj->lock);
+ msm_gem_unlock(obj);
out:
return ret;
}
@@ -289,10 +289,9 @@ vm_fault_t msm_gem_fault(struct vm_fault *vmf)
static uint64_t mmap_offset(struct drm_gem_object *obj)
{
struct drm_device *dev = obj->dev;
- struct msm_gem_object *msm_obj = to_msm_bo(obj);
int ret;
- WARN_ON(!mutex_is_locked(_obj->lock));
+ WARN_ON(!msm_gem_is_locked(obj));
/* Make it mmapable */
ret = drm_gem_create_mmap_offset(obj);
@@ -308,11 +307,10 @@ static uint64_t mmap_offset(struct drm_gem_object *obj)
uint64_t msm_gem_mmap_offset(struct drm_gem_object *obj)
{
uint64_t offset;
- struct msm_gem_object *msm_obj = to_msm_bo(obj);
- mutex_lock(_obj->lock);
+ msm_gem_lock(obj);
offset = mmap_offset(obj);
- mutex_unlock(_obj->lock);
+ msm_gem_unlock(obj);
return offset;
}
@@ -322,7 +320,7 @@ static struct msm_gem_vma *add_vma(struct drm_gem_object
*obj,
struct msm_gem_object *msm_obj = to_msm_bo(obj);
struct msm_gem_vma *vma;
- WARN_ON(!mutex_is_locked(_obj->lock));
+ WARN_ON(!msm_gem_is_locked(obj));
vma = kzalloc(sizeof(*vma), GFP_KERNEL);
if (!vma)
@@ -341,7 +339,7 @@ static struct msm_gem_vma *lookup_vma(struct drm_gem_object
*obj,
struct msm_gem_object *msm_obj = to_msm_bo(obj);
struct msm_gem_vma *vma;
- WARN_ON(!mutex_is_locked(_obj->lock));
+ WARN_ON(!msm_gem_is_locked(obj));
list_for_each_entry(vma, _obj->vmas, list) {
if (vma->aspace == aspace)
@@ -360,14 +358,14 @@ static void del_vma(struct msm_gem_vma *vma)
kfree(vma);
}
-/* Called with msm_obj->lock locked */
+/* Called with msm_obj locked */
static void
put_iova(struct drm_gem_object *obj)
{
struct msm_gem_object *msm_obj = to_msm_bo(obj);
struct msm_gem_vma *vma, *tmp;
- WARN_ON(!mutex_is_locked(_obj->lock));
+ WARN_ON(!msm_gem_is_locked(obj));
list_for_each_entry_safe(vma, tmp, _obj->vmas, list) {
if (vma->aspace) {
@@ -382,11 +380,10 @@ static int msm_gem_get_iova_locked(struct drm_gem_object
*obj,
struct msm_gem_address_space *aspace, uint64_t *iova,
u64 range_start, u64 range_end)
{
- struct msm_gem_object *msm_obj = to_msm_bo(obj);
struct msm_gem_vma *vma;
int ret = 0;
- WARN_ON(!mutex_is_locked(_obj->lock));
+ WARN_ON(!msm_gem_is_locked(obj));
vma = lookup_vma(obj, aspace);
@@ -421,7 +418,7 @@ static int msm_gem_pin_iova(struct drm_gem_object *obj,
if (msm_obj->flags & MSM_BO_MAP_PRIV)
prot |= IOMMU_PRIV;
- WARN_ON(!mutex_is_locked(_obj->lock));
+ WARN_ON(!msm_gem_is_locked(obj));
if (WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED))
return -EBUSY;
@@ -446,11 +443,10 @@ int msm_gem_get_and_pin_iova_range(struct drm_gem_object
*obj,
struct msm_gem_address_space *aspace, uint64_t *iova,
u64 range_start, u64 range_end)
{
- struct msm_gem_object *msm_obj = to_msm_bo(obj);
u64 local;
[PATCH v2 22/22] drm/msm: Don't implicit-sync if only a single ring
From: Rob Clark
Any cross-device sync use-cases *must* use explicit sync. And if there
is only a single ring (no-preemption), everything is FIFO order and
there is no need to implicit-sync.
Mesa should probably just always use MSM_SUBMIT_NO_IMPLICIT, as behavior
is undefined when fences are not used to synchronize buffer usage across
contexts (which is the only case where multiple different priority rings
could come into play).
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/msm_gem_submit.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
b/drivers/gpu/drm/msm/msm_gem_submit.c
index 3151a0ca8904..c69803ea53c8 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -277,7 +277,7 @@ static int submit_lock_objects(struct msm_gem_submit
*submit)
return ret;
}
-static int submit_fence_sync(struct msm_gem_submit *submit, bool no_implicit)
+static int submit_fence_sync(struct msm_gem_submit *submit, bool implicit_sync)
{
int i, ret = 0;
@@ -297,7 +297,7 @@ static int submit_fence_sync(struct msm_gem_submit *submit,
bool no_implicit)
return ret;
}
- if (no_implicit)
+ if (!implicit_sync)
continue;
ret = msm_gem_sync_object(_obj->base, submit->ring->fctx,
@@ -768,7 +768,8 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
if (ret)
goto out;
- ret = submit_fence_sync(submit, !!(args->flags &
MSM_SUBMIT_NO_IMPLICIT));
+ ret = submit_fence_sync(submit, (gpu->nr_rings > 1) &&
+ !(args->flags & MSM_SUBMIT_NO_IMPLICIT));
if (ret)
goto out;
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 06/22] drm/msm/submit: Move copy_from_user ahead of locking bos
From: Rob Clark
We cannot switch to using obj->resv for locking without first moving all
the copy_from_user() ahead of submit_lock_objects(). Otherwise in the
mm fault path we aquire mm->mmap_sem before obj lock, but in the submit
path the order is reversed.
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/msm_gem.h| 3 +
drivers/gpu/drm/msm/msm_gem_submit.c | 121 ---
2 files changed, 76 insertions(+), 48 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index 947eeaca661d..744889436a98 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -238,7 +238,10 @@ struct msm_gem_submit {
uint32_t type;
uint32_t size; /* in dwords */
uint64_t iova;
+ uint32_t offset;/* in dwords */
uint32_t idx; /* cmdstream buffer idx in bos[] */
+ uint32_t nr_relocs;
+ struct drm_msm_gem_submit_reloc *relocs;
} *cmd; /* array of size nr_cmds */
struct {
uint32_t flags;
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c
b/drivers/gpu/drm/msm/msm_gem_submit.c
index aa5c60a7132d..002130d826aa 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -62,11 +62,16 @@ static struct msm_gem_submit *submit_create(struct
drm_device *dev,
void msm_gem_submit_free(struct msm_gem_submit *submit)
{
+ unsigned i;
+
dma_fence_put(submit->fence);
list_del(>node);
put_pid(submit->pid);
msm_submitqueue_put(submit->queue);
+ for (i = 0; i < submit->nr_cmds; i++)
+ kfree(submit->cmd[i].relocs);
+
kfree(submit);
}
@@ -150,6 +155,60 @@ static int submit_lookup_objects(struct msm_gem_submit
*submit,
return ret;
}
+static int submit_lookup_cmds(struct msm_gem_submit *submit,
+ struct drm_msm_gem_submit *args, struct drm_file *file)
+{
+ unsigned i, sz;
+ int ret = 0;
+
+ for (i = 0; i < args->nr_cmds; i++) {
+ struct drm_msm_gem_submit_cmd submit_cmd;
+ void __user *userptr =
+ u64_to_user_ptr(args->cmds + (i * sizeof(submit_cmd)));
+
+ ret = copy_from_user(_cmd, userptr, sizeof(submit_cmd));
+ if (ret) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ /* validate input from userspace: */
+ switch (submit_cmd.type) {
+ case MSM_SUBMIT_CMD_BUF:
+ case MSM_SUBMIT_CMD_IB_TARGET_BUF:
+ case MSM_SUBMIT_CMD_CTX_RESTORE_BUF:
+ break;
+ default:
+ DRM_ERROR("invalid type: %08x\n", submit_cmd.type);
+ return -EINVAL;
+ }
+
+ if (submit_cmd.size % 4) {
+ DRM_ERROR("non-aligned cmdstream buffer size: %u\n",
+ submit_cmd.size);
+ ret = -EINVAL;
+ goto out;
+ }
+
+ submit->cmd[i].type = submit_cmd.type;
+ submit->cmd[i].size = submit_cmd.size / 4;
+ submit->cmd[i].offset = submit_cmd.submit_offset / 4;
+ submit->cmd[i].idx = submit_cmd.submit_idx;
+ submit->cmd[i].nr_relocs = submit_cmd.nr_relocs;
+
+ sz = sizeof(struct drm_msm_gem_submit_reloc) *
submit_cmd.nr_relocs;
+ submit->cmd[i].relocs = kmalloc(sz, GFP_KERNEL);
+ ret = copy_from_user(submit->cmd[i].relocs, userptr, sz);
+ if (ret) {
+ ret = -EFAULT;
+ goto out;
+ }
+ }
+
+out:
+ return ret;
+}
+
static void submit_unlock_unpin_bo(struct msm_gem_submit *submit,
int i, bool backoff)
{
@@ -301,7 +360,7 @@ static int submit_bo(struct msm_gem_submit *submit,
uint32_t idx,
/* process the reloc's and patch up the cmdstream as needed: */
static int submit_reloc(struct msm_gem_submit *submit, struct msm_gem_object
*obj,
- uint32_t offset, uint32_t nr_relocs, uint64_t relocs)
+ uint32_t offset, uint32_t nr_relocs, struct
drm_msm_gem_submit_reloc *relocs)
{
uint32_t i, last_offset = 0;
uint32_t *ptr;
@@ -327,18 +386,11 @@ static int submit_reloc(struct msm_gem_submit *submit,
struct msm_gem_object *ob
}
for (i = 0; i < nr_relocs; i++) {
- struct drm_msm_gem_submit_reloc submit_reloc;
- void __user *userptr =
- u64_to_user_ptr(relocs + (i * sizeof(submit_reloc)));
+ struct drm_msm_gem_submit_reloc submit_reloc = relocs[i];
uint32_t off;
uint64_t iova;
bool valid;
- if (copy_from_user(_reloc,
[PATCH v2 21/22] drm/msm: Drop struct_mutex in shrinker path
From: Rob Clark
Now that the inactive_list is protected by mm_lock, and everything
else on per-obj basis is protected by obj->lock, we no longer depend
on struct_mutex.
Signed-off-by: Rob Clark
---
drivers/gpu/drm/msm/msm_gem.c | 1 -
drivers/gpu/drm/msm/msm_gem_shrinker.c | 54 --
2 files changed, 55 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 8852c05775dc..ca00c3ccd413 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -673,7 +673,6 @@ void msm_gem_purge(struct drm_gem_object *obj)
struct drm_device *dev = obj->dev;
struct msm_gem_object *msm_obj = to_msm_bo(obj);
- WARN_ON(!mutex_is_locked(>struct_mutex));
WARN_ON(!is_purgeable(msm_obj));
WARN_ON(obj->import_attach);
diff --git a/drivers/gpu/drm/msm/msm_gem_shrinker.c
b/drivers/gpu/drm/msm/msm_gem_shrinker.c
index 6be073b8ca08..6f4b1355725f 100644
--- a/drivers/gpu/drm/msm/msm_gem_shrinker.c
+++ b/drivers/gpu/drm/msm/msm_gem_shrinker.c
@@ -8,48 +8,13 @@
#include "msm_gem.h"
#include "msm_gpu_trace.h"
-static bool msm_gem_shrinker_lock(struct drm_device *dev, bool *unlock)
-{
- /* NOTE: we are *closer* to being able to get rid of
-* mutex_trylock_recursive().. the msm_gem code itself does
-* not need struct_mutex, although codepaths that can trigger
-* shrinker are still called in code-paths that hold the
-* struct_mutex.
-*
-* Also, msm_obj->madv is protected by struct_mutex.
-*
-* The next step is probably split out a seperate lock for
-* protecting inactive_list, so that shrinker does not need
-* struct_mutex.
-*/
- switch (mutex_trylock_recursive(>struct_mutex)) {
- case MUTEX_TRYLOCK_FAILED:
- return false;
-
- case MUTEX_TRYLOCK_SUCCESS:
- *unlock = true;
- return true;
-
- case MUTEX_TRYLOCK_RECURSIVE:
- *unlock = false;
- return true;
- }
-
- BUG();
-}
-
static unsigned long
msm_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc)
{
struct msm_drm_private *priv =
container_of(shrinker, struct msm_drm_private, shrinker);
- struct drm_device *dev = priv->dev;
struct msm_gem_object *msm_obj;
unsigned long count = 0;
- bool unlock;
-
- if (!msm_gem_shrinker_lock(dev, ))
- return 0;
mutex_lock(>mm_lock);
@@ -63,9 +28,6 @@ msm_gem_shrinker_count(struct shrinker *shrinker, struct
shrink_control *sc)
mutex_unlock(>mm_lock);
- if (unlock)
- mutex_unlock(>struct_mutex);
-
return count;
}
@@ -74,13 +36,8 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct
shrink_control *sc)
{
struct msm_drm_private *priv =
container_of(shrinker, struct msm_drm_private, shrinker);
- struct drm_device *dev = priv->dev;
struct msm_gem_object *msm_obj;
unsigned long freed = 0;
- bool unlock;
-
- if (!msm_gem_shrinker_lock(dev, ))
- return SHRINK_STOP;
mutex_lock(>mm_lock);
@@ -98,9 +55,6 @@ msm_gem_shrinker_scan(struct shrinker *shrinker, struct
shrink_control *sc)
mutex_unlock(>mm_lock);
- if (unlock)
- mutex_unlock(>struct_mutex);
-
if (freed > 0)
trace_msm_gem_purge(freed << PAGE_SHIFT);
@@ -112,13 +66,8 @@ msm_gem_shrinker_vmap(struct notifier_block *nb, unsigned
long event, void *ptr)
{
struct msm_drm_private *priv =
container_of(nb, struct msm_drm_private, vmap_notifier);
- struct drm_device *dev = priv->dev;
struct msm_gem_object *msm_obj;
unsigned unmapped = 0;
- bool unlock;
-
- if (!msm_gem_shrinker_lock(dev, ))
- return NOTIFY_DONE;
mutex_lock(>mm_lock);
@@ -141,9 +90,6 @@ msm_gem_shrinker_vmap(struct notifier_block *nb, unsigned
long event, void *ptr)
mutex_unlock(>mm_lock);
- if (unlock)
- mutex_unlock(>struct_mutex);
-
*(unsigned long *)ptr += unmapped;
if (unmapped > 0)
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 00/14] drm/msm: de-struct_mutex-ification
From: Rob Clark This doesn't remove *all* the struct_mutex, but it covers the worst of it, ie. shrinker/madvise/free/retire. The submit path still uses struct_mutex, but it still needs *something* serialize a portion of the submit path, and lock_stat mostly just shows the lock contention there being with other submits. And there are a few other bits of struct_mutex usage in less critical paths (debugfs, etc). But this seems like a reasonable step in the right direction. v2: teach lockdep about shrinker locking patters (danvet) and convert to obj->resv locking (danvet) Rob Clark (22): drm/msm/gem: Add obj->lock wrappers drm/msm/gem: Rename internal get_iova_locked helper drm/msm/gem: Move prototypes to msm_gem.h drm/msm/gem: Add some _locked() helpers drm/msm/gem: Move locking in shrinker path drm/msm/submit: Move copy_from_user ahead of locking bos drm/msm: Do rpm get sooner in the submit path drm/msm/gem: Switch over to obj->resv for locking drm/msm: Use correct drm_gem_object_put() in fail case drm/msm: Drop chatty trace drm/msm: Move update_fences() drm/msm: Add priv->mm_lock to protect active/inactive lists drm/msm: Document and rename preempt_lock drm/msm: Protect ring->submits with it's own lock drm/msm: Refcount submits drm/msm: Remove obj->gpu drm/msm: Drop struct_mutex from the retire path drm/msm: Drop struct_mutex in free_object() path drm/msm: remove msm_gem_free_work drm/msm: drop struct_mutex in madvise path drm/msm: Drop struct_mutex in shrinker path drm/msm: Don't implicit-sync if only a single ring drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 4 +- drivers/gpu/drm/msm/adreno/a5xx_preempt.c | 12 +- drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 4 +- drivers/gpu/drm/msm/disp/mdp4/mdp4_crtc.c | 1 + drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 1 + drivers/gpu/drm/msm/dsi/dsi_host.c| 1 + drivers/gpu/drm/msm/msm_debugfs.c | 7 + drivers/gpu/drm/msm/msm_drv.c | 21 +- drivers/gpu/drm/msm/msm_drv.h | 73 ++- drivers/gpu/drm/msm/msm_fbdev.c | 1 + drivers/gpu/drm/msm/msm_gem.c | 245 ++ drivers/gpu/drm/msm/msm_gem.h | 131 ++-- drivers/gpu/drm/msm/msm_gem_shrinker.c| 81 +++ drivers/gpu/drm/msm/msm_gem_submit.c | 154 +- drivers/gpu/drm/msm/msm_gpu.c | 98 + drivers/gpu/drm/msm/msm_gpu.h | 5 +- drivers/gpu/drm/msm/msm_ringbuffer.c | 3 +- drivers/gpu/drm/msm/msm_ringbuffer.h | 13 +- 18 files changed, 459 insertions(+), 396 deletions(-) -- 2.26.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 10/22] drm/msm: Drop chatty trace
From: Rob Clark
It is somewhat redundant with the gpu tracepoints, and anyways not too
useful to justify spamming the log when debug traces are enabled.
Signed-off-by: Rob Clark
Reviewed-by: Jordan Crouse
---
drivers/gpu/drm/msm/msm_gpu.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index dbd9020713e5..677b11c5a151 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -535,7 +535,6 @@ static void recover_worker(struct work_struct *work)
static void hangcheck_timer_reset(struct msm_gpu *gpu)
{
- DBG("%s", gpu->name);
mod_timer(>hangcheck_timer,
round_jiffies_up(jiffies + DRM_MSM_HANGCHECK_JIFFIES));
}
--
2.26.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 2/2] drm/tilcdc: Remove tilcdc_crtc_max_width(), use private data
Hi Jyri,
Thank you for the patch.
On Sat, Oct 10, 2020 at 08:00:59PM +0300, Jyri Sarha wrote:
> We already have a private data member for maximum display width so
> let's use it and get rid of the redundant tilcdc_crtc_max_width().
>
> The LCDC version probing is moved to before reading the device tree
> properties so that the version information is available when private
> data maximum width is initialized, if "max-width" property is not
> found.
>
> Signed-off-by: Jyri Sarha
> Reviewed-by: Tomi Valkeinen
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 16 +---
> drivers/gpu/drm/tilcdc/tilcdc_drv.c | 38 +++-
> drivers/gpu/drm/tilcdc/tilcdc_drv.h | 7 ++---
> 3 files changed, 26 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 0fd3dafe6404..da2ab2aa3577 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -754,20 +754,6 @@ static const struct drm_crtc_funcs tilcdc_crtc_funcs = {
> .disable_vblank = tilcdc_crtc_disable_vblank,
> };
>
> -int tilcdc_crtc_max_width(struct drm_crtc *crtc)
> -{
> - struct drm_device *dev = crtc->dev;
> - struct tilcdc_drm_private *priv = dev->dev_private;
> - int max_width = 0;
> -
> - if (priv->rev == 1)
> - max_width = 1024;
> - else if (priv->rev == 2)
> - max_width = 2048;
> -
> - return max_width;
> -}
> -
> static enum drm_mode_status
> tilcdc_crtc_mode_valid(struct drm_crtc *crtc,
> const struct drm_display_mode *mode)
> @@ -780,7 +766,7 @@ tilcdc_crtc_mode_valid(struct drm_crtc *crtc,
>* check to see if the width is within the range that
>* the LCD Controller physically supports
>*/
> - if (mode->hdisplay > tilcdc_crtc_max_width(crtc))
> + if (mode->hdisplay > priv->max_width)
> return MODE_VIRTUAL_X;
>
> /* width must be multiple of 16 */
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> index 4f5fc3e87383..c5f82e693f1a 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -105,7 +105,7 @@ static void modeset_init(struct drm_device *dev)
>
> dev->mode_config.min_width = 0;
> dev->mode_config.min_height = 0;
> - dev->mode_config.max_width = tilcdc_crtc_max_width(priv->crtc);
> + dev->mode_config.max_width = priv->max_width;
> dev->mode_config.max_height = 2048;
> dev->mode_config.funcs = _config_funcs;
> }
> @@ -218,22 +218,6 @@ static int tilcdc_init(struct drm_driver *ddrv, struct
> device *dev)
> goto init_failed;
> }
>
> - if (of_property_read_u32(node, "max-bandwidth", >max_bandwidth))
> - priv->max_bandwidth = TILCDC_DEFAULT_MAX_BANDWIDTH;
> -
> - DBG("Maximum Bandwidth Value %d", priv->max_bandwidth);
> -
> - if (of_property_read_u32(node, "max-width", >max_width))
> - priv->max_width = TILCDC_DEFAULT_MAX_WIDTH;
> -
> - DBG("Maximum Horizontal Pixel Width Value %dpixels", priv->max_width);
> -
> - if (of_property_read_u32(node, "max-pixelclock",
> - >max_pixelclock))
> - priv->max_pixelclock = TILCDC_DEFAULT_MAX_PIXELCLOCK;
> -
> - DBG("Maximum Pixel Clock Value %dKHz", priv->max_pixelclock);
> -
> pm_runtime_enable(dev);
>
> /* Determine LCD IP Version */
> @@ -287,6 +271,26 @@ static int tilcdc_init(struct drm_driver *ddrv, struct
> device *dev)
> }
> }
>
> + if (of_property_read_u32(node, "max-bandwidth", >max_bandwidth))
> + priv->max_bandwidth = TILCDC_DEFAULT_MAX_BANDWIDTH;
> +
> + DBG("Maximum Bandwidth Value %d", priv->max_bandwidth);
> +
> + if (of_property_read_u32(node, "max-width", >max_width)) {
> + if (priv->rev == 1)
> + priv->max_width = TILCDC_DEFAULT_MAX_WIDTH_V1;
> + else
> + priv->max_width = TILCDC_DEFAULT_MAX_WIDTH_V2;
> + }
> +
> + DBG("Maximum Horizontal Pixel Width Value %dpixels", priv->max_width);
> +
> + if (of_property_read_u32(node, "max-pixelclock",
> + >max_pixelclock))
> + priv->max_pixelclock = TILCDC_DEFAULT_MAX_PIXELCLOCK;
> +
> + DBG("Maximum Pixel Clock Value %dKHz", priv->max_pixelclock);
> +
> ret = tilcdc_crtc_create(ddev);
> if (ret < 0) {
> dev_err(dev, "failed to create crtc\n");
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> index 18815e75ca4f..d29806ca8817 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.h
> @@ -28,8 +28,10 @@ struct drm_plane;
>
> /* Defaulting to pixel clock defined on AM335x */
> #define TILCDC_DEFAULT_MAX_PIXELCLOCK
Re: [PATCH RESEND v3 1/6] drm/of: Change the prototype of drm_of_lvds_get_dual_link_pixel_order
Hi Maxime,
Thank you for the patch.
On Mon, Oct 05, 2020 at 05:15:39PM +0200, Maxime Ripard wrote:
> The drm_of_lvds_get_dual_link_pixel_order() function took so far the
> device_node of the two ports used together to make up a dual-link LVDS
> output.
>
> This assumes that a binding would use an entire port for the LVDS output.
> However, some bindings have used endpoints instead and thus we need to
> operate at the endpoint level. Change slightly the arguments to allow that.
>
> Signed-off-by: Maxime Ripard
> ---
> drivers/gpu/drm/drm_of.c| 98 +++---
> drivers/gpu/drm/rcar-du/rcar_lvds.c | 8 +--
> include/drm/drm_of.h| 16 +++--
> 3 files changed, 63 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index b50b44e76279..2dcb49b0401b 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -291,50 +291,34 @@ static int drm_of_lvds_get_port_pixels_type(struct
> device_node *port_node)
> (odd_pixels ? DRM_OF_LVDS_ODD : 0);
> }
>
> -static int drm_of_lvds_get_remote_pixels_type(
> - const struct device_node *port_node)
> +static int drm_of_lvds_get_remote_pixels_type(const struct device_node
> *endpoint)
> {
> - struct device_node *endpoint = NULL;
> - int pixels_type = -EPIPE;
> + struct device_node *remote_port;
> + int pixels_type;
>
> - for_each_child_of_node(port_node, endpoint) {
> - struct device_node *remote_port;
> - int current_pt;
> -
> - if (!of_node_name_eq(endpoint, "endpoint"))
> - continue;
> -
> - remote_port = of_graph_get_remote_port(endpoint);
> - if (!remote_port) {
> - of_node_put(remote_port);
> - return -EPIPE;
> - }
> -
> - current_pt = drm_of_lvds_get_port_pixels_type(remote_port);
> + remote_port = of_graph_get_remote_port(endpoint);
> + if (!remote_port) {
> of_node_put(remote_port);
You can drop this line.
> - if (pixels_type < 0)
> - pixels_type = current_pt;
> -
> - /*
> - * Sanity check, ensure that all remote endpoints have the same
> - * pixel type. We may lift this restriction later if we need to
> - * support multiple sinks with different dual-link
> - * configurations by passing the endpoints explicitly to
> - * drm_of_lvds_get_dual_link_pixel_order().
> - */
Shouldn't we keep this check when endpoint_id is -1 in
drm_of_lvds_get_dual_link_pixel_order() ?
> - if (!current_pt || pixels_type != current_pt) {
> - of_node_put(remote_port);
> - return -EINVAL;
> - }
> + return -EPIPE;
> }
>
> + pixels_type = drm_of_lvds_get_port_pixels_type(remote_port);
> + of_node_put(remote_port);
> +
> + if (pixels_type < 0)
> + pixels_type = -EPIPE;
> +
> return pixels_type;
> }
>
> /**
> * drm_of_lvds_get_dual_link_pixel_order - Get LVDS dual-link pixel order
> - * @port1: First DT port node of the Dual-link LVDS source
> - * @port2: Second DT port node of the Dual-link LVDS source
> + * @dev1: First DT device node of the Dual-Link LVDS source
> + * @port1_id: ID of the first DT port node of the Dual-Link LVDS source
> + * @endpoint1_id: ID of the first DT port node of the Dual-Link LVDS source
The port1_id and endpoint1_id parameters have the exact same
documentation. Same for port2.
I would shorten port1_id to port1 and endpoint1_id to endpoint1, but
that's up to you.
> + * @dev2: First DT device node of the Dual-Link LVDS source
> + * @port2_id: ID of the first DT port node of the Dual-Link LVDS source
> + * @endpoint2_id: ID of the first DT port node of the Dual-Link LVDS source
> *
> * An LVDS dual-link connection is made of two links, with even pixels
> * transitting on one link, and odd pixels on the other link. This function
> @@ -348,32 +332,48 @@ static int drm_of_lvds_get_remote_pixels_type(
> *
> * If either port is not connected, this function returns -EPIPE.
> *
> - * @port1 and @port2 are typically DT sibling nodes, but may have different
> - * parents when, for instance, two separate LVDS encoders carry the even and
> odd
> - * pixels.
> + * @port1_id and @port2_id are typically DT sibling nodes, but may have
> + * different parents when, for instance, two separate LVDS encoders carry the
> + * even and odd pixels.
> + *
> + * If @port1_id, @port2_id, @endpoint1_id or @endpoint2_id are set to -1,
> their
> + * value is going to be ignored.
And what happens when they're ignored ? :-) You should document that
the first endpoint / port is then used.
> *
> * Return:
> - * * DRM_LVDS_DUAL_LINK_EVEN_ODD_PIXELS - @port1 carries even pixels and
> @port2
> - *
Re: [PATCH v2 09/17] mm: Add unsafe_follow_pfn
Em Sun, 11 Oct 2020 08:27:41 +0200 Mauro Carvalho Chehab escreveu: > Em Sat, 10 Oct 2020 23:50:27 +0200 > Daniel Vetter escreveu: > > > On Sat, Oct 10, 2020 at 11:36 PM Laurent Pinchart > > wrote: > > > > > > > > We probably still have a few legacy drivers using videobuf (non-2), > > > > but IMHO those should be safe to put behind some disabled-by-default > > > > Kconfig symbol or even completely drop, as the legacy framework has > > > > been deprecated for many years already. > > > > > > There's 8 drivers left, and they support a very large number of devices. > > > I expect unhappy users distros stop shipping them. On the other hand, > > > videobuf has been deprecated for a lng time, so there has been > > > plenty of time to convert the remaining drivers to videobuf2. If nobody > > > can do it, then we'll have to drop support for these devices given the > > > security issues. > > > > Again, the issue here is _only_ with follow_pfn. For videobuf1 this > > means videbuf-dma-contig.c userptr support is broken. Unlike videobuf2 > > it means it's broken for all usage (not just zero-copy userptr), > > because videbuf-dma-contig.c lacks the pin_user_pages path. > > Well, follow_pfn() is used only by videbuf-dma-contig.c. If this is > the only part of VB1 that will have userptr broken, then there's > just one driver that might be affected: davinci. > > Yet, taking a deeper look: > > $ git grep include drivers/media/platform/davinci/|grep -i videobuf > drivers/media/platform/davinci/vpif_capture.h:#include > > drivers/media/platform/davinci/vpif_display.h:#include > > > It sounds to me that it was already converted to VB2, but some VB1 > symbols were not converted at its Kconfig. > > It sounds to me that there are other drivers with some VB1 left overs > at Kconfig, as those are the only ones using VB1 those days: > > $ for i in $(git grep media/videobuf drivers |grep -v videobuf2 |grep > -v v4l2-core|cut -d: -f1); do dirname $i; done|sort|uniq > drivers/media/pci/bt8xx > drivers/media/pci/cx18 > drivers/media/platform > drivers/media/usb/tm6000 > drivers/media/usb/zr364xx > drivers/staging/media/atomisp/pci This is incomplete. There are two drivers that include videobuf indirectly: include/media/davinci/vpfe_capture.h include/media/drv-intf/saa7146_vv.h I double-checked that DaVinci still uses VB1. There are actually two clients for videbuf-dma-contig.c: davinci and fsl-viu. Those two will be affected, if we don't add pin_user_pages_fast() support to VB1 or convert them to VB2. > > > But that > > would be easy to add if this poses a problem I think - we just need > > to carry over the pin_user_pages_fast logic from videbuf2, no driver > > changes required. But of course I don't think we should do that before > > someone reports the regression, since videobuf1 userptr is doubly > > deprecated :-) > > I think otherwise. Keeping a broken component at the Kernel is > a bad idea. > > Yet, from my quick search above, it sounds to me that it is time for > us to retire the VB1 DMA contig support as a hole, as there's no client > for it anymore. > > I'll work on some patches cleaning up the VB1 left overs at > Kconfig and removing videbuf-dma-contig.c for good, if there's > no hidden dependency on it. > > > Thanks, > Mauro Thanks, Mauro ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 09/17] mm: Add unsafe_follow_pfn
Em Sat, 10 Oct 2020 23:50:27 +0200 Daniel Vetter escreveu: > On Sat, Oct 10, 2020 at 11:36 PM Laurent Pinchart > wrote: > > > > > We probably still have a few legacy drivers using videobuf (non-2), > > > but IMHO those should be safe to put behind some disabled-by-default > > > Kconfig symbol or even completely drop, as the legacy framework has > > > been deprecated for many years already. > > > > There's 8 drivers left, and they support a very large number of devices. > > I expect unhappy users distros stop shipping them. On the other hand, > > videobuf has been deprecated for a lng time, so there has been > > plenty of time to convert the remaining drivers to videobuf2. If nobody > > can do it, then we'll have to drop support for these devices given the > > security issues. > > Again, the issue here is _only_ with follow_pfn. For videobuf1 this > means videbuf-dma-contig.c userptr support is broken. Unlike videobuf2 > it means it's broken for all usage (not just zero-copy userptr), > because videbuf-dma-contig.c lacks the pin_user_pages path. Well, follow_pfn() is used only by videbuf-dma-contig.c. If this is the only part of VB1 that will have userptr broken, then there's just one driver that might be affected: davinci. Yet, taking a deeper look: $ git grep include drivers/media/platform/davinci/|grep -i videobuf drivers/media/platform/davinci/vpif_capture.h:#include drivers/media/platform/davinci/vpif_display.h:#include It sounds to me that it was already converted to VB2, but some VB1 symbols were not converted at its Kconfig. It sounds to me that there are other drivers with some VB1 left overs at Kconfig, as those are the only ones using VB1 those days: $ for i in $(git grep media/videobuf drivers |grep -v videobuf2 |grep -v v4l2-core|cut -d: -f1); do dirname $i; done|sort|uniq drivers/media/pci/bt8xx drivers/media/pci/cx18 drivers/media/platform drivers/media/usb/tm6000 drivers/media/usb/zr364xx drivers/staging/media/atomisp/pci > But that > would be easy to add if this poses a problem I think - we just need > to carry over the pin_user_pages_fast logic from videbuf2, no driver > changes required. But of course I don't think we should do that before > someone reports the regression, since videobuf1 userptr is doubly > deprecated :-) I think otherwise. Keeping a broken component at the Kernel is a bad idea. Yet, from my quick search above, it sounds to me that it is time for us to retire the VB1 DMA contig support as a hole, as there's no client for it anymore. I'll work on some patches cleaning up the VB1 left overs at Kconfig and removing videbuf-dma-contig.c for good, if there's no hidden dependency on it. Thanks, Mauro ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
