* [PATCH 0/2] Default to cachable mappings for GEM SHMEM @ 2020-05-13 15:03 Thomas Zimmermann 2020-05-13 15:03 ` [PATCH 1/2] drm/shmem: Use cached mappings by default Thomas Zimmermann 2020-05-13 15:03 ` [PATCH 2/2] drm/udl: Use GEM vmap/mmap function from SHMEM helpers Thomas Zimmermann 0 siblings, 2 replies; 19+ messages in thread From: Thomas Zimmermann @ 2020-05-13 15:03 UTC (permalink / raw) To: maarten.lankhorst, mripard, airlied, daniel, sean, kraxel, emil.l.velikov, sam Cc: Thomas Zimmermann, dri-devel This patchset complements Daniel's recent patches for SHMEM. [1] By default SHMEM maps pages using writecombine. Only virtio sets the SHMEM implmentation to use cached mappings. Udl implements its own vmap/mmap functions, which always maps pages with caching. Unify the settings by switching the SHMEM code to use cached mappings (i.e., PAGE_KERNEL actually). The exception is dma-buf imported pages, which are still mapped using writecombine. Tested with udl-driven hardware. [1] https://lists.freedesktop.org/archives/dri-devel/2020-May/265468.html Thomas Zimmermann (2): drm/shmem: Use cached mappings by default drm/udl: Use GEM vmap/mmap function from SHMEM helpers drivers/gpu/drm/drm_gem_shmem_helper.c | 6 +- drivers/gpu/drm/udl/Makefile | 2 +- drivers/gpu/drm/udl/udl_drv.c | 3 - drivers/gpu/drm/udl/udl_drv.h | 3 - drivers/gpu/drm/udl/udl_gem.c | 106 ------------------------ drivers/gpu/drm/virtio/virtgpu_object.c | 1 - include/drm/drm_gem_shmem_helper.h | 4 +- 7 files changed, 7 insertions(+), 118 deletions(-) delete mode 100644 drivers/gpu/drm/udl/udl_gem.c -- 2.26.2 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/2] drm/shmem: Use cached mappings by default 2020-05-13 15:03 [PATCH 0/2] Default to cachable mappings for GEM SHMEM Thomas Zimmermann @ 2020-05-13 15:03 ` Thomas Zimmermann 2020-05-13 15:06 ` Thomas Zimmermann 2020-05-14 12:40 ` Daniel Vetter 2020-05-13 15:03 ` [PATCH 2/2] drm/udl: Use GEM vmap/mmap function from SHMEM helpers Thomas Zimmermann 1 sibling, 2 replies; 19+ messages in thread From: Thomas Zimmermann @ 2020-05-13 15:03 UTC (permalink / raw) To: maarten.lankhorst, mripard, airlied, daniel, sean, kraxel, emil.l.velikov, sam Cc: Thomas Zimmermann, dri-devel SHMEM-buffer backing storage is allocated from system memory; which is typically cachable. Currently, only virtio uses cachable mappings; udl uses its own vmap/mmap implementation for cachable mappings. Other drivers default to writecombine mappings. Use cached mappings by default. The exception is pages imported via dma-buf. DMA memory is usually not cached. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/drm_gem_shmem_helper.c | 6 ++++-- drivers/gpu/drm/virtio/virtgpu_object.c | 1 - include/drm/drm_gem_shmem_helper.h | 4 ++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index df31e5782eed1..1ce90325dfa31 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -259,7 +259,7 @@ static void *drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem) } else { pgprot_t prot = PAGE_KERNEL; - if (!shmem->map_cached) + if (shmem->map_wc) prot = pgprot_writecombine(prot); shmem->vaddr = vmap(shmem->pages, obj->size >> PAGE_SHIFT, VM_MAP, prot); @@ -546,7 +546,7 @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) vma->vm_flags |= VM_MIXEDMAP | VM_DONTEXPAND; vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); - if (!shmem->map_cached) + if (shmem->map_wc) vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); vma->vm_ops = &drm_gem_shmem_vm_ops; @@ -664,6 +664,8 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev, if (IS_ERR(shmem)) return ERR_CAST(shmem); + shmem->map_wc = false; /* dma-buf mappings use writecombine */ + shmem->pages = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL); if (!shmem->pages) { ret = -ENOMEM; diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c index 6ccbd01cd888c..80ba6b2b61668 100644 --- a/drivers/gpu/drm/virtio/virtgpu_object.c +++ b/drivers/gpu/drm/virtio/virtgpu_object.c @@ -132,7 +132,6 @@ struct drm_gem_object *virtio_gpu_create_object(struct drm_device *dev, dshmem = &shmem->base.base; dshmem->base.funcs = &virtio_gpu_shmem_funcs; - dshmem->map_cached = true; return &dshmem->base; } diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h index 294b2931c4cc0..a5bc082a77c48 100644 --- a/include/drm/drm_gem_shmem_helper.h +++ b/include/drm/drm_gem_shmem_helper.h @@ -98,9 +98,9 @@ struct drm_gem_shmem_object { unsigned int vmap_use_count; /** - * @map_cached: map object cached (instead of using writecombine). + * @map_wc: map object using writecombine (instead of cached). */ - bool map_cached; + bool map_wc; }; #define to_drm_gem_shmem_obj(obj) \ -- 2.26.2 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] drm/shmem: Use cached mappings by default 2020-05-13 15:03 ` [PATCH 1/2] drm/shmem: Use cached mappings by default Thomas Zimmermann @ 2020-05-13 15:06 ` Thomas Zimmermann 2020-05-14 12:40 ` Daniel Vetter 1 sibling, 0 replies; 19+ messages in thread From: Thomas Zimmermann @ 2020-05-13 15:06 UTC (permalink / raw) To: maarten.lankhorst, mripard, airlied, daniel, sean, kraxel, emil.l.velikov, sam Cc: dri-devel [-- Attachment #1.1.1: Type: text/plain, Size: 3451 bytes --] Am 13.05.20 um 17:03 schrieb Thomas Zimmermann: > SHMEM-buffer backing storage is allocated from system memory; which is > typically cachable. Currently, only virtio uses cachable mappings; udl > uses its own vmap/mmap implementation for cachable mappings. Other > drivers default to writecombine mappings. > > Use cached mappings by default. The exception is pages imported via > dma-buf. DMA memory is usually not cached. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/drm_gem_shmem_helper.c | 6 ++++-- > drivers/gpu/drm/virtio/virtgpu_object.c | 1 - > include/drm/drm_gem_shmem_helper.h | 4 ++-- > 3 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > index df31e5782eed1..1ce90325dfa31 100644 > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > @@ -259,7 +259,7 @@ static void *drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem) > } else { > pgprot_t prot = PAGE_KERNEL; > > - if (!shmem->map_cached) > + if (shmem->map_wc) > prot = pgprot_writecombine(prot); > shmem->vaddr = vmap(shmem->pages, obj->size >> PAGE_SHIFT, > VM_MAP, prot); > @@ -546,7 +546,7 @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) > > vma->vm_flags |= VM_MIXEDMAP | VM_DONTEXPAND; > vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); > - if (!shmem->map_cached) > + if (shmem->map_wc) > vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); > vma->vm_ops = &drm_gem_shmem_vm_ops; > > @@ -664,6 +664,8 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev, > if (IS_ERR(shmem)) > return ERR_CAST(shmem); > > + shmem->map_wc = false; /* dma-buf mappings use writecombine */ Just when I posted the patch, I saw this bug. map_wc should be set to true to enable writecombine mappings for dma-buf pages. Will be fixed in the next iteration. > + > shmem->pages = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL); > if (!shmem->pages) { > ret = -ENOMEM; > diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c > index 6ccbd01cd888c..80ba6b2b61668 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_object.c > +++ b/drivers/gpu/drm/virtio/virtgpu_object.c > @@ -132,7 +132,6 @@ struct drm_gem_object *virtio_gpu_create_object(struct drm_device *dev, > > dshmem = &shmem->base.base; > dshmem->base.funcs = &virtio_gpu_shmem_funcs; > - dshmem->map_cached = true; > return &dshmem->base; > } > > diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h > index 294b2931c4cc0..a5bc082a77c48 100644 > --- a/include/drm/drm_gem_shmem_helper.h > +++ b/include/drm/drm_gem_shmem_helper.h > @@ -98,9 +98,9 @@ struct drm_gem_shmem_object { > unsigned int vmap_use_count; > > /** > - * @map_cached: map object cached (instead of using writecombine). > + * @map_wc: map object using writecombine (instead of cached). > */ > - bool map_cached; > + bool map_wc; > }; > > #define to_drm_gem_shmem_obj(obj) \ > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer [-- Attachment #1.2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] drm/shmem: Use cached mappings by default 2020-05-13 15:03 ` [PATCH 1/2] drm/shmem: Use cached mappings by default Thomas Zimmermann 2020-05-13 15:06 ` Thomas Zimmermann @ 2020-05-14 12:40 ` Daniel Vetter 2020-05-14 15:27 ` Thomas Zimmermann 2020-05-14 20:36 ` Rob Herring 1 sibling, 2 replies; 19+ messages in thread From: Daniel Vetter @ 2020-05-14 12:40 UTC (permalink / raw) To: Thomas Zimmermann; +Cc: sam, airlied, emil.l.velikov, dri-devel, kraxel, sean On Wed, May 13, 2020 at 05:03:11PM +0200, Thomas Zimmermann wrote: > SHMEM-buffer backing storage is allocated from system memory; which is > typically cachable. Currently, only virtio uses cachable mappings; udl > uses its own vmap/mmap implementation for cachable mappings. Other > drivers default to writecombine mappings. I'm pretty sure this breaks all these drivers. quick grep on a few functions says this is used by lima, panfrost, v3d. And they definitely need uncached/wc stuff afaiui. Or I'm completely missing something? -Daniel > > Use cached mappings by default. The exception is pages imported via > dma-buf. DMA memory is usually not cached. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/drm_gem_shmem_helper.c | 6 ++++-- > drivers/gpu/drm/virtio/virtgpu_object.c | 1 - > include/drm/drm_gem_shmem_helper.h | 4 ++-- > 3 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c > index df31e5782eed1..1ce90325dfa31 100644 > --- a/drivers/gpu/drm/drm_gem_shmem_helper.c > +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c > @@ -259,7 +259,7 @@ static void *drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem) > } else { > pgprot_t prot = PAGE_KERNEL; > > - if (!shmem->map_cached) > + if (shmem->map_wc) > prot = pgprot_writecombine(prot); > shmem->vaddr = vmap(shmem->pages, obj->size >> PAGE_SHIFT, > VM_MAP, prot); > @@ -546,7 +546,7 @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) > > vma->vm_flags |= VM_MIXEDMAP | VM_DONTEXPAND; > vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); > - if (!shmem->map_cached) > + if (shmem->map_wc) > vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); > vma->vm_ops = &drm_gem_shmem_vm_ops; > > @@ -664,6 +664,8 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev, > if (IS_ERR(shmem)) > return ERR_CAST(shmem); > > + shmem->map_wc = false; /* dma-buf mappings use writecombine */ > + > shmem->pages = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL); > if (!shmem->pages) { > ret = -ENOMEM; > diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c > index 6ccbd01cd888c..80ba6b2b61668 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_object.c > +++ b/drivers/gpu/drm/virtio/virtgpu_object.c > @@ -132,7 +132,6 @@ struct drm_gem_object *virtio_gpu_create_object(struct drm_device *dev, > > dshmem = &shmem->base.base; > dshmem->base.funcs = &virtio_gpu_shmem_funcs; > - dshmem->map_cached = true; > return &dshmem->base; > } > > diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h > index 294b2931c4cc0..a5bc082a77c48 100644 > --- a/include/drm/drm_gem_shmem_helper.h > +++ b/include/drm/drm_gem_shmem_helper.h > @@ -98,9 +98,9 @@ struct drm_gem_shmem_object { > unsigned int vmap_use_count; > > /** > - * @map_cached: map object cached (instead of using writecombine). > + * @map_wc: map object using writecombine (instead of cached). > */ > - bool map_cached; > + bool map_wc; > }; > > #define to_drm_gem_shmem_obj(obj) \ > -- > 2.26.2 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] drm/shmem: Use cached mappings by default 2020-05-14 12:40 ` Daniel Vetter @ 2020-05-14 15:27 ` Thomas Zimmermann 2020-05-14 20:36 ` Rob Herring 1 sibling, 0 replies; 19+ messages in thread From: Thomas Zimmermann @ 2020-05-14 15:27 UTC (permalink / raw) To: Daniel Vetter; +Cc: airlied, sam, emil.l.velikov, dri-devel, kraxel, sean [-- Attachment #1.1.1: Type: text/plain, Size: 3731 bytes --] Hi Am 14.05.20 um 14:40 schrieb Daniel Vetter: > On Wed, May 13, 2020 at 05:03:11PM +0200, Thomas Zimmermann wrote: >> SHMEM-buffer backing storage is allocated from system memory; which is >> typically cachable. Currently, only virtio uses cachable mappings; udl >> uses its own vmap/mmap implementation for cachable mappings. Other >> drivers default to writecombine mappings. > > I'm pretty sure this breaks all these drivers. quick grep on a few > functions says this is used by lima, panfrost, v3d. And they definitely > need uncached/wc stuff afaiui. Or I'm completely missing something? OK, I better get some testing for this code. :D > -Daniel > >> >> Use cached mappings by default. The exception is pages imported via >> dma-buf. DMA memory is usually not cached. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/gpu/drm/drm_gem_shmem_helper.c | 6 ++++-- >> drivers/gpu/drm/virtio/virtgpu_object.c | 1 - >> include/drm/drm_gem_shmem_helper.h | 4 ++-- >> 3 files changed, 6 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c >> index df31e5782eed1..1ce90325dfa31 100644 >> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c >> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c >> @@ -259,7 +259,7 @@ static void *drm_gem_shmem_vmap_locked(struct drm_gem_shmem_object *shmem) >> } else { >> pgprot_t prot = PAGE_KERNEL; >> >> - if (!shmem->map_cached) >> + if (shmem->map_wc) >> prot = pgprot_writecombine(prot); >> shmem->vaddr = vmap(shmem->pages, obj->size >> PAGE_SHIFT, >> VM_MAP, prot); >> @@ -546,7 +546,7 @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) >> >> vma->vm_flags |= VM_MIXEDMAP | VM_DONTEXPAND; >> vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); >> - if (!shmem->map_cached) >> + if (shmem->map_wc) >> vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); >> vma->vm_ops = &drm_gem_shmem_vm_ops; >> >> @@ -664,6 +664,8 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev, >> if (IS_ERR(shmem)) >> return ERR_CAST(shmem); >> >> + shmem->map_wc = false; /* dma-buf mappings use writecombine */ >> + >> shmem->pages = kvmalloc_array(npages, sizeof(struct page *), GFP_KERNEL); >> if (!shmem->pages) { >> ret = -ENOMEM; >> diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c >> index 6ccbd01cd888c..80ba6b2b61668 100644 >> --- a/drivers/gpu/drm/virtio/virtgpu_object.c >> +++ b/drivers/gpu/drm/virtio/virtgpu_object.c >> @@ -132,7 +132,6 @@ struct drm_gem_object *virtio_gpu_create_object(struct drm_device *dev, >> >> dshmem = &shmem->base.base; >> dshmem->base.funcs = &virtio_gpu_shmem_funcs; >> - dshmem->map_cached = true; >> return &dshmem->base; >> } >> >> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h >> index 294b2931c4cc0..a5bc082a77c48 100644 >> --- a/include/drm/drm_gem_shmem_helper.h >> +++ b/include/drm/drm_gem_shmem_helper.h >> @@ -98,9 +98,9 @@ struct drm_gem_shmem_object { >> unsigned int vmap_use_count; >> >> /** >> - * @map_cached: map object cached (instead of using writecombine). >> + * @map_wc: map object using writecombine (instead of cached). >> */ >> - bool map_cached; >> + bool map_wc; >> }; >> >> #define to_drm_gem_shmem_obj(obj) \ >> -- >> 2.26.2 >> > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer [-- Attachment #1.2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] drm/shmem: Use cached mappings by default 2020-05-14 12:40 ` Daniel Vetter 2020-05-14 15:27 ` Thomas Zimmermann @ 2020-05-14 20:36 ` Rob Herring 2020-05-15 6:58 ` Thomas Zimmermann 1 sibling, 1 reply; 19+ messages in thread From: Rob Herring @ 2020-05-14 20:36 UTC (permalink / raw) To: Daniel Vetter Cc: David Airlie, Sean Paul, Emil Velikov, dri-devel, Gerd Hoffmann, Thomas Zimmermann, Sam Ravnborg On Thu, May 14, 2020 at 7:40 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Wed, May 13, 2020 at 05:03:11PM +0200, Thomas Zimmermann wrote: > > SHMEM-buffer backing storage is allocated from system memory; which is > > typically cachable. Currently, only virtio uses cachable mappings; udl > > uses its own vmap/mmap implementation for cachable mappings. Other > > drivers default to writecombine mappings. > > I'm pretty sure this breaks all these drivers. quick grep on a few > functions says this is used by lima, panfrost, v3d. And they definitely > need uncached/wc stuff afaiui. Or I'm completely missing something? Yes, that would be my guess. DMA is usually non-coherent on Arm. Rob _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] drm/shmem: Use cached mappings by default 2020-05-14 20:36 ` Rob Herring @ 2020-05-15 6:58 ` Thomas Zimmermann 2020-05-15 14:10 ` Daniel Vetter 0 siblings, 1 reply; 19+ messages in thread From: Thomas Zimmermann @ 2020-05-15 6:58 UTC (permalink / raw) To: Rob Herring, Daniel Vetter Cc: David Airlie, Sam Ravnborg, Emil Velikov, dri-devel, Gerd Hoffmann, Sean Paul [-- Attachment #1.1.1: Type: text/plain, Size: 1454 bytes --] Hi Am 14.05.20 um 22:36 schrieb Rob Herring: > On Thu, May 14, 2020 at 7:40 AM Daniel Vetter <daniel@ffwll.ch> wrote: >> >> On Wed, May 13, 2020 at 05:03:11PM +0200, Thomas Zimmermann wrote: >>> SHMEM-buffer backing storage is allocated from system memory; which is >>> typically cachable. Currently, only virtio uses cachable mappings; udl >>> uses its own vmap/mmap implementation for cachable mappings. Other >>> drivers default to writecombine mappings. >> >> I'm pretty sure this breaks all these drivers. quick grep on a few >> functions says this is used by lima, panfrost, v3d. And they definitely >> need uncached/wc stuff afaiui. Or I'm completely missing something? > OK. I think I'll just make a patch that adds a .gem_create_object helper that sets the map_cached flag. So drivers can opt-in. > Yes, that would be my guess. DMA is usually non-coherent on Arm. Can one of you give me some pointer to what you're looking at? For example, I grepped for use of vmap within lima and only found [1]. That's a for memcpy with no DMA involved. There's got to be something I'm missing. Best regards Thomas [1] https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/lima/lima_sched.c#n391 > > Rob > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer [-- Attachment #1.2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] drm/shmem: Use cached mappings by default 2020-05-15 6:58 ` Thomas Zimmermann @ 2020-05-15 14:10 ` Daniel Vetter 2020-05-18 8:13 ` Thomas Zimmermann 0 siblings, 1 reply; 19+ messages in thread From: Daniel Vetter @ 2020-05-15 14:10 UTC (permalink / raw) To: Thomas Zimmermann Cc: David Airlie, Sean Paul, Emil Velikov, dri-devel, Gerd Hoffmann, Sam Ravnborg On Fri, May 15, 2020 at 08:58:02AM +0200, Thomas Zimmermann wrote: > Hi > > Am 14.05.20 um 22:36 schrieb Rob Herring: > > On Thu, May 14, 2020 at 7:40 AM Daniel Vetter <daniel@ffwll.ch> wrote: > >> > >> On Wed, May 13, 2020 at 05:03:11PM +0200, Thomas Zimmermann wrote: > >>> SHMEM-buffer backing storage is allocated from system memory; which is > >>> typically cachable. Currently, only virtio uses cachable mappings; udl > >>> uses its own vmap/mmap implementation for cachable mappings. Other > >>> drivers default to writecombine mappings. > >> > >> I'm pretty sure this breaks all these drivers. quick grep on a few > >> functions says this is used by lima, panfrost, v3d. And they definitely > >> need uncached/wc stuff afaiui. Or I'm completely missing something? > > > > OK. I think I'll just make a patch that adds a .gem_create_object helper > that sets the map_cached flag. So drivers can opt-in. > > > Yes, that would be my guess. DMA is usually non-coherent on Arm. > > Can one of you give me some pointer to what you're looking at? For > example, I grepped for use of vmap within lima and only found [1]. > That's a for memcpy with no DMA involved. There's got to be something > I'm missing. > > Best regards > Thomas > > [1] > https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/lima/lima_sched.c#n391 $ git grep drm_gem_shmem_mmap We also need correct access from userspace, otherwise the gpu is going to be sad. -Daniel > > > > > Rob > > > > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Maxfeldstr. 5, 90409 Nürnberg, Germany > (HRB 36809, AG Nürnberg) > Geschäftsführer: Felix Imendörffer > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] drm/shmem: Use cached mappings by default 2020-05-15 14:10 ` Daniel Vetter @ 2020-05-18 8:13 ` Thomas Zimmermann 2020-05-18 8:23 ` Gerd Hoffmann 0 siblings, 1 reply; 19+ messages in thread From: Thomas Zimmermann @ 2020-05-18 8:13 UTC (permalink / raw) To: Daniel Vetter Cc: David Airlie, Sean Paul, Emil Velikov, dri-devel, Gerd Hoffmann, Sam Ravnborg [-- Attachment #1.1.1: Type: text/plain, Size: 2407 bytes --] Hi Am 15.05.20 um 16:10 schrieb Daniel Vetter: > On Fri, May 15, 2020 at 08:58:02AM +0200, Thomas Zimmermann wrote: >> Hi >> >> Am 14.05.20 um 22:36 schrieb Rob Herring: >>> On Thu, May 14, 2020 at 7:40 AM Daniel Vetter <daniel@ffwll.ch> wrote: >>>> >>>> On Wed, May 13, 2020 at 05:03:11PM +0200, Thomas Zimmermann wrote: >>>>> SHMEM-buffer backing storage is allocated from system memory; which is >>>>> typically cachable. Currently, only virtio uses cachable mappings; udl >>>>> uses its own vmap/mmap implementation for cachable mappings. Other >>>>> drivers default to writecombine mappings. >>>> >>>> I'm pretty sure this breaks all these drivers. quick grep on a few >>>> functions says this is used by lima, panfrost, v3d. And they definitely >>>> need uncached/wc stuff afaiui. Or I'm completely missing something? >>> >> >> OK. I think I'll just make a patch that adds a .gem_create_object helper >> that sets the map_cached flag. So drivers can opt-in. >> >>> Yes, that would be my guess. DMA is usually non-coherent on Arm. >> >> Can one of you give me some pointer to what you're looking at? For >> example, I grepped for use of vmap within lima and only found [1]. >> That's a for memcpy with no DMA involved. There's got to be something >> I'm missing. >> >> Best regards >> Thomas >> >> [1] >> https://cgit.freedesktop.org/drm/drm-tip/tree/drivers/gpu/drm/lima/lima_sched.c#n391 > > $ git grep drm_gem_shmem_mmap > > We also need correct access from userspace, otherwise the gpu is going to > be sad. I've been thinking about this, and I think it means that we can never have cached mappings anywhere. Even if shmem supports it internally for most drivers, as soon as the page are exported, the importer could expect uncached memory. Given that, I think the correct thing to do is to udl's gem code and use the default implementation. Best regards Thomas > -Daniel >> >>> >>> Rob >>> >> >> -- >> Thomas Zimmermann >> Graphics Driver Developer >> SUSE Software Solutions Germany GmbH >> Maxfeldstr. 5, 90409 Nürnberg, Germany >> (HRB 36809, AG Nürnberg) >> Geschäftsführer: Felix Imendörffer >> > > > > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer [-- Attachment #1.2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] drm/shmem: Use cached mappings by default 2020-05-18 8:13 ` Thomas Zimmermann @ 2020-05-18 8:23 ` Gerd Hoffmann 2020-05-18 8:50 ` Thomas Zimmermann 0 siblings, 1 reply; 19+ messages in thread From: Gerd Hoffmann @ 2020-05-18 8:23 UTC (permalink / raw) To: Thomas Zimmermann Cc: David Airlie, Sean Paul, Emil Velikov, dri-devel, Sam Ravnborg > > $ git grep drm_gem_shmem_mmap > > > > We also need correct access from userspace, otherwise the gpu is going to > > be sad. > > I've been thinking about this, and I think it means that we can never > have cached mappings anywhere. Even if shmem supports it internally for > most drivers, as soon as the page are exported, the importer could > expect uncached memory. The importer should not expect anything but call dma-buf ops so the exporter has a chance to handle this correctly. (Yes, we don't get this right everywhere, some drivers take the dma-bufs list of pages and do their own thing ...). take care, Gerd _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] drm/shmem: Use cached mappings by default 2020-05-18 8:23 ` Gerd Hoffmann @ 2020-05-18 8:50 ` Thomas Zimmermann 2020-05-18 10:11 ` Gerd Hoffmann 0 siblings, 1 reply; 19+ messages in thread From: Thomas Zimmermann @ 2020-05-18 8:50 UTC (permalink / raw) To: Gerd Hoffmann Cc: David Airlie, Emil Velikov, Sean Paul, dri-devel, Sam Ravnborg [-- Attachment #1.1.1: Type: text/plain, Size: 1552 bytes --] Hi Gerd Am 18.05.20 um 10:23 schrieb Gerd Hoffmann: >>> $ git grep drm_gem_shmem_mmap >>> >>> We also need correct access from userspace, otherwise the gpu is going to >>> be sad. >> >> I've been thinking about this, and I think it means that we can never >> have cached mappings anywhere. Even if shmem supports it internally for >> most drivers, as soon as the page are exported, the importer could >> expect uncached memory. > > The importer should not expect anything but call dma-buf ops so the > exporter has a chance to handle this correctly. I have the following case in mind: Suppose the exporter maps cached pages and the importer expects uncached pages for DMA. There is map_dma_buf/unmap_dma_buf, which can implement a cache flush for the cached pages. Is it guaranteed that the importer calls this around each DMA operation? If it doesn't, it could miss later buffer updates from the exporter. And there's apparently no explicit flush op in dma_buf_ops. Best regards Thomas > > (Yes, we don't get this right everywhere, some drivers take the dma-bufs > list of pages and do their own thing ...). > > take care, > Gerd > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer [-- Attachment #1.2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] drm/shmem: Use cached mappings by default 2020-05-18 8:50 ` Thomas Zimmermann @ 2020-05-18 10:11 ` Gerd Hoffmann 2020-05-18 14:40 ` Daniel Vetter 0 siblings, 1 reply; 19+ messages in thread From: Gerd Hoffmann @ 2020-05-18 10:11 UTC (permalink / raw) To: Thomas Zimmermann Cc: David Airlie, Emil Velikov, Sean Paul, dri-devel, Sam Ravnborg On Mon, May 18, 2020 at 10:50:15AM +0200, Thomas Zimmermann wrote: > Hi Gerd > > Am 18.05.20 um 10:23 schrieb Gerd Hoffmann: > >>> $ git grep drm_gem_shmem_mmap > >>> > >>> We also need correct access from userspace, otherwise the gpu is going to > >>> be sad. > >> > >> I've been thinking about this, and I think it means that we can never > >> have cached mappings anywhere. Even if shmem supports it internally for > >> most drivers, as soon as the page are exported, the importer could > >> expect uncached memory. > > > > The importer should not expect anything but call dma-buf ops so the > > exporter has a chance to handle this correctly. > > I have the following case in mind: Suppose the exporter maps cached > pages and the importer expects uncached pages for DMA. There is > map_dma_buf/unmap_dma_buf, which can implement a cache flush for the > cached pages. Is it guaranteed that the importer calls this around each > DMA operation? I think the importer is supposed to do that, but I wouldn't surprised if there are cases in tree where this isn't implemented correctly ... take care, Gerd _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] drm/shmem: Use cached mappings by default 2020-05-18 10:11 ` Gerd Hoffmann @ 2020-05-18 14:40 ` Daniel Vetter 2020-05-19 6:31 ` Thomas Zimmermann 0 siblings, 1 reply; 19+ messages in thread From: Daniel Vetter @ 2020-05-18 14:40 UTC (permalink / raw) To: Gerd Hoffmann Cc: David Airlie, Sam Ravnborg, Emil Velikov, dri-devel, Thomas Zimmermann, Sean Paul On Mon, May 18, 2020 at 12:11:32PM +0200, Gerd Hoffmann wrote: > On Mon, May 18, 2020 at 10:50:15AM +0200, Thomas Zimmermann wrote: > > Hi Gerd > > > > Am 18.05.20 um 10:23 schrieb Gerd Hoffmann: > > >>> $ git grep drm_gem_shmem_mmap > > >>> > > >>> We also need correct access from userspace, otherwise the gpu is going to > > >>> be sad. > > >> > > >> I've been thinking about this, and I think it means that we can never > > >> have cached mappings anywhere. Even if shmem supports it internally for > > >> most drivers, as soon as the page are exported, the importer could > > >> expect uncached memory. > > > > > > The importer should not expect anything but call dma-buf ops so the > > > exporter has a chance to handle this correctly. > > > > I have the following case in mind: Suppose the exporter maps cached > > pages and the importer expects uncached pages for DMA. There is > > map_dma_buf/unmap_dma_buf, which can implement a cache flush for the > > cached pages. Is it guaranteed that the importer calls this around each > > DMA operation? > > I think the importer is supposed to do that, but I wouldn't surprised if > there are cases in tree where this isn't implemented correctly ... Yup, this is very much a case of "supposed to" but "in practice, many actually dont". The reason is that setting up mappings is expensive, so best avoid. We filled that gap a few years after dma-buf landed with the begin/end_cpu_access hooks, which allow the exporter to do cache flushing (using something like dma_sync_sg_for_device/cpu) and for this to all work properly. We even added ioctl so that the mmap on the dma-buf works correctly. But most importers still ignore this, so it's all fail :-/ But in theory the pieces to make cached mappings work over dma-buf, even for importers that need uncached, are all there. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/2] drm/shmem: Use cached mappings by default 2020-05-18 14:40 ` Daniel Vetter @ 2020-05-19 6:31 ` Thomas Zimmermann 0 siblings, 0 replies; 19+ messages in thread From: Thomas Zimmermann @ 2020-05-19 6:31 UTC (permalink / raw) To: Daniel Vetter, Gerd Hoffmann Cc: David Airlie, Sean Paul, Emil Velikov, dri-devel, Sam Ravnborg [-- Attachment #1.1.1: Type: text/plain, Size: 2221 bytes --] Hi Am 18.05.20 um 16:40 schrieb Daniel Vetter: > On Mon, May 18, 2020 at 12:11:32PM +0200, Gerd Hoffmann wrote: >> On Mon, May 18, 2020 at 10:50:15AM +0200, Thomas Zimmermann wrote: >>> Hi Gerd >>> >>> Am 18.05.20 um 10:23 schrieb Gerd Hoffmann: >>>>>> $ git grep drm_gem_shmem_mmap >>>>>> >>>>>> We also need correct access from userspace, otherwise the gpu is going to >>>>>> be sad. >>>>> >>>>> I've been thinking about this, and I think it means that we can never >>>>> have cached mappings anywhere. Even if shmem supports it internally for >>>>> most drivers, as soon as the page are exported, the importer could >>>>> expect uncached memory. >>>> >>>> The importer should not expect anything but call dma-buf ops so the >>>> exporter has a chance to handle this correctly. >>> >>> I have the following case in mind: Suppose the exporter maps cached >>> pages and the importer expects uncached pages for DMA. There is >>> map_dma_buf/unmap_dma_buf, which can implement a cache flush for the >>> cached pages. Is it guaranteed that the importer calls this around each >>> DMA operation? >> >> I think the importer is supposed to do that, but I wouldn't surprised if >> there are cases in tree where this isn't implemented correctly ... > > Yup, this is very much a case of "supposed to" but "in practice, many > actually dont". The reason is that setting up mappings is expensive, so > best avoid. Thanks to both of you for answering the question. > > We filled that gap a few years after dma-buf landed with the > begin/end_cpu_access hooks, which allow the exporter to do cache flushing > (using something like dma_sync_sg_for_device/cpu) and for this to all work > properly. We even added ioctl so that the mmap on the dma-buf works > correctly. > > But most importers still ignore this, so it's all fail :-/ But in theory > the pieces to make cached mappings work over dma-buf, even for importers > that need uncached, are all there. > > Cheers, Daniel > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer [-- Attachment #1.2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/2] drm/udl: Use GEM vmap/mmap function from SHMEM helpers 2020-05-13 15:03 [PATCH 0/2] Default to cachable mappings for GEM SHMEM Thomas Zimmermann 2020-05-13 15:03 ` [PATCH 1/2] drm/shmem: Use cached mappings by default Thomas Zimmermann @ 2020-05-13 15:03 ` Thomas Zimmermann 2020-05-13 15:49 ` Daniel Vetter 2020-05-14 12:44 ` Daniel Vetter 1 sibling, 2 replies; 19+ messages in thread From: Thomas Zimmermann @ 2020-05-13 15:03 UTC (permalink / raw) To: maarten.lankhorst, mripard, airlied, daniel, sean, kraxel, emil.l.velikov, sam Cc: Thomas Zimmermann, dri-devel The udl driver contains an implementation of GEM vmap and mmap operations that is identical to the common SHMEM helper; except that udl's code does not support writecombine mappings. Convert udl to regular SHMEM helper functions. There's no reason to have udl behave differently from all other SHMEM drivers. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/udl/Makefile | 2 +- drivers/gpu/drm/udl/udl_drv.c | 3 - drivers/gpu/drm/udl/udl_drv.h | 3 - drivers/gpu/drm/udl/udl_gem.c | 106 ---------------------------------- 4 files changed, 1 insertion(+), 113 deletions(-) delete mode 100644 drivers/gpu/drm/udl/udl_gem.c diff --git a/drivers/gpu/drm/udl/Makefile b/drivers/gpu/drm/udl/Makefile index b50179bb4de06..24d61f61d7db2 100644 --- a/drivers/gpu/drm/udl/Makefile +++ b/drivers/gpu/drm/udl/Makefile @@ -1,4 +1,4 @@ # SPDX-License-Identifier: GPL-2.0-only -udl-y := udl_drv.o udl_modeset.o udl_connector.o udl_main.o udl_transfer.o udl_gem.o +udl-y := udl_drv.o udl_modeset.o udl_connector.o udl_main.o udl_transfer.o obj-$(CONFIG_DRM_UDL) := udl.o diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c index d1aa50fd6d65a..cf5b679bf58bb 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -37,9 +37,6 @@ DEFINE_DRM_GEM_FOPS(udl_driver_fops); static struct drm_driver driver = { .driver_features = DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET, - /* gem hooks */ - .gem_create_object = udl_driver_gem_create_object, - .fops = &udl_driver_fops, DRM_GEM_SHMEM_DRIVER_OPS, diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h index 2642f94a63fc8..b1461f30780bc 100644 --- a/drivers/gpu/drm/udl/udl_drv.h +++ b/drivers/gpu/drm/udl/udl_drv.h @@ -81,9 +81,6 @@ int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr, const char *front, char **urb_buf_ptr, u32 byte_offset, u32 device_byte_offset, u32 byte_width); -struct drm_gem_object *udl_driver_gem_create_object(struct drm_device *dev, - size_t size); - int udl_drop_usb(struct drm_device *dev); #define CMD_WRITE_RAW8 "\xAF\x60" /**< 8 bit raw write command. */ diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c deleted file mode 100644 index b6e26f98aa0af..0000000000000 --- a/drivers/gpu/drm/udl/udl_gem.c +++ /dev/null @@ -1,106 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-only -/* - * Copyright (C) 2012 Red Hat - */ - -#include <linux/dma-buf.h> -#include <linux/vmalloc.h> - -#include <drm/drm_drv.h> -#include <drm/drm_gem_shmem_helper.h> -#include <drm/drm_mode.h> -#include <drm/drm_prime.h> - -#include "udl_drv.h" - -/* - * GEM object funcs - */ - -static int udl_gem_object_mmap(struct drm_gem_object *obj, - struct vm_area_struct *vma) -{ - int ret; - - ret = drm_gem_shmem_mmap(obj, vma); - if (ret) - return ret; - - vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); - if (obj->import_attach) - vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); - vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot); - - return 0; -} - -static void *udl_gem_object_vmap(struct drm_gem_object *obj) -{ - struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj); - int ret; - - ret = mutex_lock_interruptible(&shmem->vmap_lock); - if (ret) - return ERR_PTR(ret); - - if (shmem->vmap_use_count++ > 0) - goto out; - - ret = drm_gem_shmem_get_pages(shmem); - if (ret) - goto err_zero_use; - - if (obj->import_attach) - shmem->vaddr = dma_buf_vmap(obj->import_attach->dmabuf); - else - shmem->vaddr = vmap(shmem->pages, obj->size >> PAGE_SHIFT, - VM_MAP, PAGE_KERNEL); - - if (!shmem->vaddr) { - DRM_DEBUG_KMS("Failed to vmap pages\n"); - ret = -ENOMEM; - goto err_put_pages; - } - -out: - mutex_unlock(&shmem->vmap_lock); - return shmem->vaddr; - -err_put_pages: - drm_gem_shmem_put_pages(shmem); -err_zero_use: - shmem->vmap_use_count = 0; - mutex_unlock(&shmem->vmap_lock); - return ERR_PTR(ret); -} - -static const struct drm_gem_object_funcs udl_gem_object_funcs = { - .free = drm_gem_shmem_free_object, - .print_info = drm_gem_shmem_print_info, - .pin = drm_gem_shmem_pin, - .unpin = drm_gem_shmem_unpin, - .get_sg_table = drm_gem_shmem_get_sg_table, - .vmap = udl_gem_object_vmap, - .vunmap = drm_gem_shmem_vunmap, - .mmap = udl_gem_object_mmap, -}; - -/* - * Helpers for struct drm_driver - */ - -struct drm_gem_object *udl_driver_gem_create_object(struct drm_device *dev, - size_t size) -{ - struct drm_gem_shmem_object *shmem; - struct drm_gem_object *obj; - - shmem = kzalloc(sizeof(*shmem), GFP_KERNEL); - if (!shmem) - return NULL; - - obj = &shmem->base; - obj->funcs = &udl_gem_object_funcs; - - return obj; -} -- 2.26.2 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] drm/udl: Use GEM vmap/mmap function from SHMEM helpers 2020-05-13 15:03 ` [PATCH 2/2] drm/udl: Use GEM vmap/mmap function from SHMEM helpers Thomas Zimmermann @ 2020-05-13 15:49 ` Daniel Vetter 2020-05-13 17:19 ` Thomas Zimmermann 2020-05-14 12:44 ` Daniel Vetter 1 sibling, 1 reply; 19+ messages in thread From: Daniel Vetter @ 2020-05-13 15:49 UTC (permalink / raw) To: Thomas Zimmermann; +Cc: sam, airlied, emil.l.velikov, dri-devel, kraxel, sean On Wed, May 13, 2020 at 05:03:12PM +0200, Thomas Zimmermann wrote: > The udl driver contains an implementation of GEM vmap and mmap > operations that is identical to the common SHMEM helper; except > that udl's code does not support writecombine mappings. > > Convert udl to regular SHMEM helper functions. There's no reason > to have udl behave differently from all other SHMEM drivers. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> So I remember the problem again I think ... on some x86 gpu drivers (*cough* i915 on specific chipsets) you get an uncached dma-buf. Which means if you set up a cached mapping, you get corruption. But if the shmem helpers to correctly forward _all_ calls to the dma-buf functions of the exporter, this works. I've tried to clean up that a bit with my patch series that I just posted this week, to make sure there's no bugs like that. I think once we have that fully sorted, we could land this and be happy. -Daniel > --- > drivers/gpu/drm/udl/Makefile | 2 +- > drivers/gpu/drm/udl/udl_drv.c | 3 - > drivers/gpu/drm/udl/udl_drv.h | 3 - > drivers/gpu/drm/udl/udl_gem.c | 106 ---------------------------------- > 4 files changed, 1 insertion(+), 113 deletions(-) > delete mode 100644 drivers/gpu/drm/udl/udl_gem.c > > diff --git a/drivers/gpu/drm/udl/Makefile b/drivers/gpu/drm/udl/Makefile > index b50179bb4de06..24d61f61d7db2 100644 > --- a/drivers/gpu/drm/udl/Makefile > +++ b/drivers/gpu/drm/udl/Makefile > @@ -1,4 +1,4 @@ > # SPDX-License-Identifier: GPL-2.0-only > -udl-y := udl_drv.o udl_modeset.o udl_connector.o udl_main.o udl_transfer.o udl_gem.o > +udl-y := udl_drv.o udl_modeset.o udl_connector.o udl_main.o udl_transfer.o > > obj-$(CONFIG_DRM_UDL) := udl.o > diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c > index d1aa50fd6d65a..cf5b679bf58bb 100644 > --- a/drivers/gpu/drm/udl/udl_drv.c > +++ b/drivers/gpu/drm/udl/udl_drv.c > @@ -37,9 +37,6 @@ DEFINE_DRM_GEM_FOPS(udl_driver_fops); > static struct drm_driver driver = { > .driver_features = DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET, > > - /* gem hooks */ > - .gem_create_object = udl_driver_gem_create_object, > - > .fops = &udl_driver_fops, > DRM_GEM_SHMEM_DRIVER_OPS, > > diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h > index 2642f94a63fc8..b1461f30780bc 100644 > --- a/drivers/gpu/drm/udl/udl_drv.h > +++ b/drivers/gpu/drm/udl/udl_drv.h > @@ -81,9 +81,6 @@ int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr, > const char *front, char **urb_buf_ptr, > u32 byte_offset, u32 device_byte_offset, u32 byte_width); > > -struct drm_gem_object *udl_driver_gem_create_object(struct drm_device *dev, > - size_t size); > - > int udl_drop_usb(struct drm_device *dev); > > #define CMD_WRITE_RAW8 "\xAF\x60" /**< 8 bit raw write command. */ > diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c > deleted file mode 100644 > index b6e26f98aa0af..0000000000000 > --- a/drivers/gpu/drm/udl/udl_gem.c > +++ /dev/null > @@ -1,106 +0,0 @@ > -// SPDX-License-Identifier: GPL-2.0-only > -/* > - * Copyright (C) 2012 Red Hat > - */ > - > -#include <linux/dma-buf.h> > -#include <linux/vmalloc.h> > - > -#include <drm/drm_drv.h> > -#include <drm/drm_gem_shmem_helper.h> > -#include <drm/drm_mode.h> > -#include <drm/drm_prime.h> > - > -#include "udl_drv.h" > - > -/* > - * GEM object funcs > - */ > - > -static int udl_gem_object_mmap(struct drm_gem_object *obj, > - struct vm_area_struct *vma) > -{ > - int ret; > - > - ret = drm_gem_shmem_mmap(obj, vma); > - if (ret) > - return ret; > - > - vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); > - if (obj->import_attach) > - vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); > - vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot); > - > - return 0; > -} > - > -static void *udl_gem_object_vmap(struct drm_gem_object *obj) > -{ > - struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj); > - int ret; > - > - ret = mutex_lock_interruptible(&shmem->vmap_lock); > - if (ret) > - return ERR_PTR(ret); > - > - if (shmem->vmap_use_count++ > 0) > - goto out; > - > - ret = drm_gem_shmem_get_pages(shmem); > - if (ret) > - goto err_zero_use; > - > - if (obj->import_attach) > - shmem->vaddr = dma_buf_vmap(obj->import_attach->dmabuf); > - else > - shmem->vaddr = vmap(shmem->pages, obj->size >> PAGE_SHIFT, > - VM_MAP, PAGE_KERNEL); > - > - if (!shmem->vaddr) { > - DRM_DEBUG_KMS("Failed to vmap pages\n"); > - ret = -ENOMEM; > - goto err_put_pages; > - } > - > -out: > - mutex_unlock(&shmem->vmap_lock); > - return shmem->vaddr; > - > -err_put_pages: > - drm_gem_shmem_put_pages(shmem); > -err_zero_use: > - shmem->vmap_use_count = 0; > - mutex_unlock(&shmem->vmap_lock); > - return ERR_PTR(ret); > -} > - > -static const struct drm_gem_object_funcs udl_gem_object_funcs = { > - .free = drm_gem_shmem_free_object, > - .print_info = drm_gem_shmem_print_info, > - .pin = drm_gem_shmem_pin, > - .unpin = drm_gem_shmem_unpin, > - .get_sg_table = drm_gem_shmem_get_sg_table, > - .vmap = udl_gem_object_vmap, > - .vunmap = drm_gem_shmem_vunmap, > - .mmap = udl_gem_object_mmap, > -}; > - > -/* > - * Helpers for struct drm_driver > - */ > - > -struct drm_gem_object *udl_driver_gem_create_object(struct drm_device *dev, > - size_t size) > -{ > - struct drm_gem_shmem_object *shmem; > - struct drm_gem_object *obj; > - > - shmem = kzalloc(sizeof(*shmem), GFP_KERNEL); > - if (!shmem) > - return NULL; > - > - obj = &shmem->base; > - obj->funcs = &udl_gem_object_funcs; > - > - return obj; > -} > -- > 2.26.2 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] drm/udl: Use GEM vmap/mmap function from SHMEM helpers 2020-05-13 15:49 ` Daniel Vetter @ 2020-05-13 17:19 ` Thomas Zimmermann 0 siblings, 0 replies; 19+ messages in thread From: Thomas Zimmermann @ 2020-05-13 17:19 UTC (permalink / raw) To: Daniel Vetter; +Cc: airlied, sam, emil.l.velikov, dri-devel, kraxel, sean [-- Attachment #1.1.1: Type: text/plain, Size: 6568 bytes --] Hi Daniel Am 13.05.20 um 17:49 schrieb Daniel Vetter: > On Wed, May 13, 2020 at 05:03:12PM +0200, Thomas Zimmermann wrote: >> The udl driver contains an implementation of GEM vmap and mmap >> operations that is identical to the common SHMEM helper; except >> that udl's code does not support writecombine mappings. >> >> Convert udl to regular SHMEM helper functions. There's no reason >> to have udl behave differently from all other SHMEM drivers. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > > So I remember the problem again I think ... on some x86 gpu drivers > (*cough* i915 on specific chipsets) you get an uncached dma-buf. > > Which means if you set up a cached mapping, you get corruption. With these patches, dma-buf imports get the writecombine mapping unconditionally. I don't know if that is enough, but at least it's the old behavior. > > But if the shmem helpers to correctly forward _all_ calls to the dma-buf > functions of the exporter, this works. I've tried to clean up that a bit > with my patch series that I just posted this week, to make sure there's no > bugs like that. > > I think once we have that fully sorted, we could land this and be happy. Great. I'll take another look at your patch series tomorrow. Best regards Thomas > -Daniel > >> --- >> drivers/gpu/drm/udl/Makefile | 2 +- >> drivers/gpu/drm/udl/udl_drv.c | 3 - >> drivers/gpu/drm/udl/udl_drv.h | 3 - >> drivers/gpu/drm/udl/udl_gem.c | 106 ---------------------------------- >> 4 files changed, 1 insertion(+), 113 deletions(-) >> delete mode 100644 drivers/gpu/drm/udl/udl_gem.c >> >> diff --git a/drivers/gpu/drm/udl/Makefile b/drivers/gpu/drm/udl/Makefile >> index b50179bb4de06..24d61f61d7db2 100644 >> --- a/drivers/gpu/drm/udl/Makefile >> +++ b/drivers/gpu/drm/udl/Makefile >> @@ -1,4 +1,4 @@ >> # SPDX-License-Identifier: GPL-2.0-only >> -udl-y := udl_drv.o udl_modeset.o udl_connector.o udl_main.o udl_transfer.o udl_gem.o >> +udl-y := udl_drv.o udl_modeset.o udl_connector.o udl_main.o udl_transfer.o >> >> obj-$(CONFIG_DRM_UDL) := udl.o >> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c >> index d1aa50fd6d65a..cf5b679bf58bb 100644 >> --- a/drivers/gpu/drm/udl/udl_drv.c >> +++ b/drivers/gpu/drm/udl/udl_drv.c >> @@ -37,9 +37,6 @@ DEFINE_DRM_GEM_FOPS(udl_driver_fops); >> static struct drm_driver driver = { >> .driver_features = DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET, >> >> - /* gem hooks */ >> - .gem_create_object = udl_driver_gem_create_object, >> - >> .fops = &udl_driver_fops, >> DRM_GEM_SHMEM_DRIVER_OPS, >> >> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h >> index 2642f94a63fc8..b1461f30780bc 100644 >> --- a/drivers/gpu/drm/udl/udl_drv.h >> +++ b/drivers/gpu/drm/udl/udl_drv.h >> @@ -81,9 +81,6 @@ int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr, >> const char *front, char **urb_buf_ptr, >> u32 byte_offset, u32 device_byte_offset, u32 byte_width); >> >> -struct drm_gem_object *udl_driver_gem_create_object(struct drm_device *dev, >> - size_t size); >> - >> int udl_drop_usb(struct drm_device *dev); >> >> #define CMD_WRITE_RAW8 "\xAF\x60" /**< 8 bit raw write command. */ >> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c >> deleted file mode 100644 >> index b6e26f98aa0af..0000000000000 >> --- a/drivers/gpu/drm/udl/udl_gem.c >> +++ /dev/null >> @@ -1,106 +0,0 @@ >> -// SPDX-License-Identifier: GPL-2.0-only >> -/* >> - * Copyright (C) 2012 Red Hat >> - */ >> - >> -#include <linux/dma-buf.h> >> -#include <linux/vmalloc.h> >> - >> -#include <drm/drm_drv.h> >> -#include <drm/drm_gem_shmem_helper.h> >> -#include <drm/drm_mode.h> >> -#include <drm/drm_prime.h> >> - >> -#include "udl_drv.h" >> - >> -/* >> - * GEM object funcs >> - */ >> - >> -static int udl_gem_object_mmap(struct drm_gem_object *obj, >> - struct vm_area_struct *vma) >> -{ >> - int ret; >> - >> - ret = drm_gem_shmem_mmap(obj, vma); >> - if (ret) >> - return ret; >> - >> - vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); >> - if (obj->import_attach) >> - vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); >> - vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot); >> - >> - return 0; >> -} >> - >> -static void *udl_gem_object_vmap(struct drm_gem_object *obj) >> -{ >> - struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj); >> - int ret; >> - >> - ret = mutex_lock_interruptible(&shmem->vmap_lock); >> - if (ret) >> - return ERR_PTR(ret); >> - >> - if (shmem->vmap_use_count++ > 0) >> - goto out; >> - >> - ret = drm_gem_shmem_get_pages(shmem); >> - if (ret) >> - goto err_zero_use; >> - >> - if (obj->import_attach) >> - shmem->vaddr = dma_buf_vmap(obj->import_attach->dmabuf); >> - else >> - shmem->vaddr = vmap(shmem->pages, obj->size >> PAGE_SHIFT, >> - VM_MAP, PAGE_KERNEL); >> - >> - if (!shmem->vaddr) { >> - DRM_DEBUG_KMS("Failed to vmap pages\n"); >> - ret = -ENOMEM; >> - goto err_put_pages; >> - } >> - >> -out: >> - mutex_unlock(&shmem->vmap_lock); >> - return shmem->vaddr; >> - >> -err_put_pages: >> - drm_gem_shmem_put_pages(shmem); >> -err_zero_use: >> - shmem->vmap_use_count = 0; >> - mutex_unlock(&shmem->vmap_lock); >> - return ERR_PTR(ret); >> -} >> - >> -static const struct drm_gem_object_funcs udl_gem_object_funcs = { >> - .free = drm_gem_shmem_free_object, >> - .print_info = drm_gem_shmem_print_info, >> - .pin = drm_gem_shmem_pin, >> - .unpin = drm_gem_shmem_unpin, >> - .get_sg_table = drm_gem_shmem_get_sg_table, >> - .vmap = udl_gem_object_vmap, >> - .vunmap = drm_gem_shmem_vunmap, >> - .mmap = udl_gem_object_mmap, >> -}; >> - >> -/* >> - * Helpers for struct drm_driver >> - */ >> - >> -struct drm_gem_object *udl_driver_gem_create_object(struct drm_device *dev, >> - size_t size) >> -{ >> - struct drm_gem_shmem_object *shmem; >> - struct drm_gem_object *obj; >> - >> - shmem = kzalloc(sizeof(*shmem), GFP_KERNEL); >> - if (!shmem) >> - return NULL; >> - >> - obj = &shmem->base; >> - obj->funcs = &udl_gem_object_funcs; >> - >> - return obj; >> -} >> -- >> 2.26.2 >> > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer [-- Attachment #1.2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] drm/udl: Use GEM vmap/mmap function from SHMEM helpers 2020-05-13 15:03 ` [PATCH 2/2] drm/udl: Use GEM vmap/mmap function from SHMEM helpers Thomas Zimmermann 2020-05-13 15:49 ` Daniel Vetter @ 2020-05-14 12:44 ` Daniel Vetter 2020-05-19 6:36 ` Thomas Zimmermann 1 sibling, 1 reply; 19+ messages in thread From: Daniel Vetter @ 2020-05-14 12:44 UTC (permalink / raw) To: Thomas Zimmermann; +Cc: sam, airlied, emil.l.velikov, dri-devel, kraxel, sean On Wed, May 13, 2020 at 05:03:12PM +0200, Thomas Zimmermann wrote: > The udl driver contains an implementation of GEM vmap and mmap > operations that is identical to the common SHMEM helper; except > that udl's code does not support writecombine mappings. > > Convert udl to regular SHMEM helper functions. There's no reason > to have udl behave differently from all other SHMEM drivers. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> Since I'm not sure how patch 1 works in this series I think keeping the udl_driver_gem_create_object but just let it set the map_cached = true; is all we need? -Daniel > --- > drivers/gpu/drm/udl/Makefile | 2 +- > drivers/gpu/drm/udl/udl_drv.c | 3 - > drivers/gpu/drm/udl/udl_drv.h | 3 - > drivers/gpu/drm/udl/udl_gem.c | 106 ---------------------------------- > 4 files changed, 1 insertion(+), 113 deletions(-) > delete mode 100644 drivers/gpu/drm/udl/udl_gem.c > > diff --git a/drivers/gpu/drm/udl/Makefile b/drivers/gpu/drm/udl/Makefile > index b50179bb4de06..24d61f61d7db2 100644 > --- a/drivers/gpu/drm/udl/Makefile > +++ b/drivers/gpu/drm/udl/Makefile > @@ -1,4 +1,4 @@ > # SPDX-License-Identifier: GPL-2.0-only > -udl-y := udl_drv.o udl_modeset.o udl_connector.o udl_main.o udl_transfer.o udl_gem.o > +udl-y := udl_drv.o udl_modeset.o udl_connector.o udl_main.o udl_transfer.o > > obj-$(CONFIG_DRM_UDL) := udl.o > diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c > index d1aa50fd6d65a..cf5b679bf58bb 100644 > --- a/drivers/gpu/drm/udl/udl_drv.c > +++ b/drivers/gpu/drm/udl/udl_drv.c > @@ -37,9 +37,6 @@ DEFINE_DRM_GEM_FOPS(udl_driver_fops); > static struct drm_driver driver = { > .driver_features = DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET, > > - /* gem hooks */ > - .gem_create_object = udl_driver_gem_create_object, > - > .fops = &udl_driver_fops, > DRM_GEM_SHMEM_DRIVER_OPS, > > diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h > index 2642f94a63fc8..b1461f30780bc 100644 > --- a/drivers/gpu/drm/udl/udl_drv.h > +++ b/drivers/gpu/drm/udl/udl_drv.h > @@ -81,9 +81,6 @@ int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr, > const char *front, char **urb_buf_ptr, > u32 byte_offset, u32 device_byte_offset, u32 byte_width); > > -struct drm_gem_object *udl_driver_gem_create_object(struct drm_device *dev, > - size_t size); > - > int udl_drop_usb(struct drm_device *dev); > > #define CMD_WRITE_RAW8 "\xAF\x60" /**< 8 bit raw write command. */ > diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c > deleted file mode 100644 > index b6e26f98aa0af..0000000000000 > --- a/drivers/gpu/drm/udl/udl_gem.c > +++ /dev/null > @@ -1,106 +0,0 @@ > -// SPDX-License-Identifier: GPL-2.0-only > -/* > - * Copyright (C) 2012 Red Hat > - */ > - > -#include <linux/dma-buf.h> > -#include <linux/vmalloc.h> > - > -#include <drm/drm_drv.h> > -#include <drm/drm_gem_shmem_helper.h> > -#include <drm/drm_mode.h> > -#include <drm/drm_prime.h> > - > -#include "udl_drv.h" > - > -/* > - * GEM object funcs > - */ > - > -static int udl_gem_object_mmap(struct drm_gem_object *obj, > - struct vm_area_struct *vma) > -{ > - int ret; > - > - ret = drm_gem_shmem_mmap(obj, vma); > - if (ret) > - return ret; > - > - vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); > - if (obj->import_attach) > - vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); > - vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot); > - > - return 0; > -} > - > -static void *udl_gem_object_vmap(struct drm_gem_object *obj) > -{ > - struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj); > - int ret; > - > - ret = mutex_lock_interruptible(&shmem->vmap_lock); > - if (ret) > - return ERR_PTR(ret); > - > - if (shmem->vmap_use_count++ > 0) > - goto out; > - > - ret = drm_gem_shmem_get_pages(shmem); > - if (ret) > - goto err_zero_use; > - > - if (obj->import_attach) > - shmem->vaddr = dma_buf_vmap(obj->import_attach->dmabuf); > - else > - shmem->vaddr = vmap(shmem->pages, obj->size >> PAGE_SHIFT, > - VM_MAP, PAGE_KERNEL); > - > - if (!shmem->vaddr) { > - DRM_DEBUG_KMS("Failed to vmap pages\n"); > - ret = -ENOMEM; > - goto err_put_pages; > - } > - > -out: > - mutex_unlock(&shmem->vmap_lock); > - return shmem->vaddr; > - > -err_put_pages: > - drm_gem_shmem_put_pages(shmem); > -err_zero_use: > - shmem->vmap_use_count = 0; > - mutex_unlock(&shmem->vmap_lock); > - return ERR_PTR(ret); > -} > - > -static const struct drm_gem_object_funcs udl_gem_object_funcs = { > - .free = drm_gem_shmem_free_object, > - .print_info = drm_gem_shmem_print_info, > - .pin = drm_gem_shmem_pin, > - .unpin = drm_gem_shmem_unpin, > - .get_sg_table = drm_gem_shmem_get_sg_table, > - .vmap = udl_gem_object_vmap, > - .vunmap = drm_gem_shmem_vunmap, > - .mmap = udl_gem_object_mmap, > -}; > - > -/* > - * Helpers for struct drm_driver > - */ > - > -struct drm_gem_object *udl_driver_gem_create_object(struct drm_device *dev, > - size_t size) > -{ > - struct drm_gem_shmem_object *shmem; > - struct drm_gem_object *obj; > - > - shmem = kzalloc(sizeof(*shmem), GFP_KERNEL); > - if (!shmem) > - return NULL; > - > - obj = &shmem->base; > - obj->funcs = &udl_gem_object_funcs; > - > - return obj; > -} > -- > 2.26.2 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/2] drm/udl: Use GEM vmap/mmap function from SHMEM helpers 2020-05-14 12:44 ` Daniel Vetter @ 2020-05-19 6:36 ` Thomas Zimmermann 0 siblings, 0 replies; 19+ messages in thread From: Thomas Zimmermann @ 2020-05-19 6:36 UTC (permalink / raw) To: Daniel Vetter; +Cc: airlied, sam, emil.l.velikov, dri-devel, kraxel, sean [-- Attachment #1.1.1: Type: text/plain, Size: 6091 bytes --] Hi Am 14.05.20 um 14:44 schrieb Daniel Vetter: > On Wed, May 13, 2020 at 05:03:12PM +0200, Thomas Zimmermann wrote: >> The udl driver contains an implementation of GEM vmap and mmap >> operations that is identical to the common SHMEM helper; except >> that udl's code does not support writecombine mappings. >> >> Convert udl to regular SHMEM helper functions. There's no reason >> to have udl behave differently from all other SHMEM drivers. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > > Since I'm not sure how patch 1 works in this series I think keeping the > udl_driver_gem_create_object but just let it set the map_cached = true; is > all we need? Yes. It tested this localy and it worked. The create function can simply go into udl_drv.c. I'll send out a patch later. Best regards Thomas > -Daniel > >> --- >> drivers/gpu/drm/udl/Makefile | 2 +- >> drivers/gpu/drm/udl/udl_drv.c | 3 - >> drivers/gpu/drm/udl/udl_drv.h | 3 - >> drivers/gpu/drm/udl/udl_gem.c | 106 ---------------------------------- >> 4 files changed, 1 insertion(+), 113 deletions(-) >> delete mode 100644 drivers/gpu/drm/udl/udl_gem.c >> >> diff --git a/drivers/gpu/drm/udl/Makefile b/drivers/gpu/drm/udl/Makefile >> index b50179bb4de06..24d61f61d7db2 100644 >> --- a/drivers/gpu/drm/udl/Makefile >> +++ b/drivers/gpu/drm/udl/Makefile >> @@ -1,4 +1,4 @@ >> # SPDX-License-Identifier: GPL-2.0-only >> -udl-y := udl_drv.o udl_modeset.o udl_connector.o udl_main.o udl_transfer.o udl_gem.o >> +udl-y := udl_drv.o udl_modeset.o udl_connector.o udl_main.o udl_transfer.o >> >> obj-$(CONFIG_DRM_UDL) := udl.o >> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c >> index d1aa50fd6d65a..cf5b679bf58bb 100644 >> --- a/drivers/gpu/drm/udl/udl_drv.c >> +++ b/drivers/gpu/drm/udl/udl_drv.c >> @@ -37,9 +37,6 @@ DEFINE_DRM_GEM_FOPS(udl_driver_fops); >> static struct drm_driver driver = { >> .driver_features = DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET, >> >> - /* gem hooks */ >> - .gem_create_object = udl_driver_gem_create_object, >> - >> .fops = &udl_driver_fops, >> DRM_GEM_SHMEM_DRIVER_OPS, >> >> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h >> index 2642f94a63fc8..b1461f30780bc 100644 >> --- a/drivers/gpu/drm/udl/udl_drv.h >> +++ b/drivers/gpu/drm/udl/udl_drv.h >> @@ -81,9 +81,6 @@ int udl_render_hline(struct drm_device *dev, int log_bpp, struct urb **urb_ptr, >> const char *front, char **urb_buf_ptr, >> u32 byte_offset, u32 device_byte_offset, u32 byte_width); >> >> -struct drm_gem_object *udl_driver_gem_create_object(struct drm_device *dev, >> - size_t size); >> - >> int udl_drop_usb(struct drm_device *dev); >> >> #define CMD_WRITE_RAW8 "\xAF\x60" /**< 8 bit raw write command. */ >> diff --git a/drivers/gpu/drm/udl/udl_gem.c b/drivers/gpu/drm/udl/udl_gem.c >> deleted file mode 100644 >> index b6e26f98aa0af..0000000000000 >> --- a/drivers/gpu/drm/udl/udl_gem.c >> +++ /dev/null >> @@ -1,106 +0,0 @@ >> -// SPDX-License-Identifier: GPL-2.0-only >> -/* >> - * Copyright (C) 2012 Red Hat >> - */ >> - >> -#include <linux/dma-buf.h> >> -#include <linux/vmalloc.h> >> - >> -#include <drm/drm_drv.h> >> -#include <drm/drm_gem_shmem_helper.h> >> -#include <drm/drm_mode.h> >> -#include <drm/drm_prime.h> >> - >> -#include "udl_drv.h" >> - >> -/* >> - * GEM object funcs >> - */ >> - >> -static int udl_gem_object_mmap(struct drm_gem_object *obj, >> - struct vm_area_struct *vma) >> -{ >> - int ret; >> - >> - ret = drm_gem_shmem_mmap(obj, vma); >> - if (ret) >> - return ret; >> - >> - vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); >> - if (obj->import_attach) >> - vma->vm_page_prot = pgprot_writecombine(vma->vm_page_prot); >> - vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot); >> - >> - return 0; >> -} >> - >> -static void *udl_gem_object_vmap(struct drm_gem_object *obj) >> -{ >> - struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj); >> - int ret; >> - >> - ret = mutex_lock_interruptible(&shmem->vmap_lock); >> - if (ret) >> - return ERR_PTR(ret); >> - >> - if (shmem->vmap_use_count++ > 0) >> - goto out; >> - >> - ret = drm_gem_shmem_get_pages(shmem); >> - if (ret) >> - goto err_zero_use; >> - >> - if (obj->import_attach) >> - shmem->vaddr = dma_buf_vmap(obj->import_attach->dmabuf); >> - else >> - shmem->vaddr = vmap(shmem->pages, obj->size >> PAGE_SHIFT, >> - VM_MAP, PAGE_KERNEL); >> - >> - if (!shmem->vaddr) { >> - DRM_DEBUG_KMS("Failed to vmap pages\n"); >> - ret = -ENOMEM; >> - goto err_put_pages; >> - } >> - >> -out: >> - mutex_unlock(&shmem->vmap_lock); >> - return shmem->vaddr; >> - >> -err_put_pages: >> - drm_gem_shmem_put_pages(shmem); >> -err_zero_use: >> - shmem->vmap_use_count = 0; >> - mutex_unlock(&shmem->vmap_lock); >> - return ERR_PTR(ret); >> -} >> - >> -static const struct drm_gem_object_funcs udl_gem_object_funcs = { >> - .free = drm_gem_shmem_free_object, >> - .print_info = drm_gem_shmem_print_info, >> - .pin = drm_gem_shmem_pin, >> - .unpin = drm_gem_shmem_unpin, >> - .get_sg_table = drm_gem_shmem_get_sg_table, >> - .vmap = udl_gem_object_vmap, >> - .vunmap = drm_gem_shmem_vunmap, >> - .mmap = udl_gem_object_mmap, >> -}; >> - >> -/* >> - * Helpers for struct drm_driver >> - */ >> - >> -struct drm_gem_object *udl_driver_gem_create_object(struct drm_device *dev, >> - size_t size) >> -{ >> - struct drm_gem_shmem_object *shmem; >> - struct drm_gem_object *obj; >> - >> - shmem = kzalloc(sizeof(*shmem), GFP_KERNEL); >> - if (!shmem) >> - return NULL; >> - >> - obj = &shmem->base; >> - obj->funcs = &udl_gem_object_funcs; >> - >> - return obj; >> -} >> -- >> 2.26.2 >> > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer [-- Attachment #1.2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 160 bytes --] _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2020-05-19 6:36 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-13 15:03 [PATCH 0/2] Default to cachable mappings for GEM SHMEM Thomas Zimmermann 2020-05-13 15:03 ` [PATCH 1/2] drm/shmem: Use cached mappings by default Thomas Zimmermann 2020-05-13 15:06 ` Thomas Zimmermann 2020-05-14 12:40 ` Daniel Vetter 2020-05-14 15:27 ` Thomas Zimmermann 2020-05-14 20:36 ` Rob Herring 2020-05-15 6:58 ` Thomas Zimmermann 2020-05-15 14:10 ` Daniel Vetter 2020-05-18 8:13 ` Thomas Zimmermann 2020-05-18 8:23 ` Gerd Hoffmann 2020-05-18 8:50 ` Thomas Zimmermann 2020-05-18 10:11 ` Gerd Hoffmann 2020-05-18 14:40 ` Daniel Vetter 2020-05-19 6:31 ` Thomas Zimmermann 2020-05-13 15:03 ` [PATCH 2/2] drm/udl: Use GEM vmap/mmap function from SHMEM helpers Thomas Zimmermann 2020-05-13 15:49 ` Daniel Vetter 2020-05-13 17:19 ` Thomas Zimmermann 2020-05-14 12:44 ` Daniel Vetter 2020-05-19 6:36 ` Thomas Zimmermann
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.