Re: [PATCH v7] drm: Add library for shmem backed GEM objects

[Eric Anholt <eric@anholt.net>]

[-- Attachment #1.1: Type: text/plain, Size: 3267 bytes --]

Rob Herring <robh@kernel.org> writes:

> On Wed, Mar 6, 2019 at 6:50 PM Eric Anholt <eric@anholt.net> wrote:
>>
>> Rob Herring <robh@kernel.org> writes:
>>
>> > From: Noralf Trønnes <noralf@tronnes.org>
>> >
>> > This adds a library for shmem backed GEM objects.
>> >
>> > v7:
>> > - Use write-combine for mmap instead. This is the more common
>> >   case. (robher)
>> >
>> > v6:
>> > - Fix uninitialized variable issue in an error path (anholt).
>> > - Add a drm_gem_shmem_vm_open() to the fops to get proper refcounting
>> >   of the pages (anholt).
>> >
>> > v5:
>> > - Drop drm_gem_shmem_prime_mmap() (Daniel Vetter)
>> > - drm_gem_shmem_mmap(): Subtract drm_vma_node_start() to get the real
>> >   vma->vm_pgoff
>> > - drm_gem_shmem_fault(): Use vmf->pgoff now that vma->vm_pgoff is correct
>> >
>> > v4:
>> > - Drop cache modes (Thomas Hellstrom)
>> > - Add a GEM attached vtable
>> >
>> > v3:
>> > - Grammar (Sam Ravnborg)
>> > - s/drm_gem_shmem_put_pages_unlocked/drm_gem_shmem_put_pages_locked/
>> >   (Sam Ravnborg)
>> > - Add debug output in error path (Sam Ravnborg)
>> >
>> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> > Signed-off-by: Eric Anholt <eric@anholt.net>
>> > Signed-off-by: Rob Herring <robh@kernel.org>
>> > ---
>> > I don't have a user to submit just yet, but we are using this for
>> > panfrost which can be seen here[1]. I expect we'll be ready to merge
>> > panfrost for 5.2.
>
> I need to add exporting drm_gem_shmem_create_with_handle() as well. I
> was thinking I was just using that in my rockchip conversion attempt,
> but I need it for panfrost too.
>
>> Excellent.  I'm taking a look at this for v3d, and I see that on the
>> panfrost side you're allocating shmem->sgt and doing dma_map_sg() in
>> your MMU map code, with no error handling.  And, on MMU unmap I see
>> dma_unmap_sg() unconditionally (won't that unbalance for a prime import
>> which will presumably do its own unmapping?), but it also looks like the
>> sgt is not freed.
>
> Indeed, I'll need to split those.
>
> Error handling? Panfrost doesn't do that. ;)
>
>> Can we do something nicer for handling the driver's desire for the sgt
>> to fill its PTEs, regardless of where the BO came from?
>
> Not sure I follow. You mean do something in shmem helpers? I'm
> assuming when we pin and dma map will evolve from at BO creation time
> to on demand.

Yeah, basically my driver wants not drm_gem_shmem_get_pages() as its
interface but drm_shmem_get_sgt().  I'm imagining a helper for that
which has its own refcount that does a get_pages and then, for
non-dmabuf-import, the sgt allocation and dma mapping.

>> I also hope we can plug this into vkms and turn on prime sharing for it.
>
> Doesn't seem too hard. I'd assume vkms would be okay with WC mappings
> as it looks like they are currently cacheable? We could add a dma
> attrs field to let users customize the mapping, but I'd rather avoid
> that if possible.

WC was my intent.  It would make vkms look more like other drivers from
a perf perspective, which I think would be reasonable (and then you
don't have CPU cache issues when sharing buffers).  I've got a patch
laying around for converting to the helpers.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel