Re: [PATCH v5 4/5] drm: Add library for shmem backed GEM objects

["Noralf Trønnes" <noralf@tronnes.org>]

Den 30.11.2018 00.58, skrev Eric Anholt:
> Daniel Vetter <daniel@ffwll.ch> writes:
>
>> On Wed, Nov 28, 2018 at 01:52:56PM -0800, Eric Anholt wrote:
>>> Daniel Vetter <daniel@ffwll.ch> writes:
>>>
>>>> On Tue, Nov 27, 2018 at 12:38:44PM -0800, Eric Anholt wrote:
>>>>> Daniel Vetter <daniel@ffwll.ch> writes:
>>>>>
>>>>>> On Mon, Nov 26, 2018 at 04:36:21PM -0800, Eric Anholt wrote:
>>>>>>> Noralf Trønnes <noralf@tronnes.org> writes:
>>>>>>>> +static void drm_gem_shmem_vm_close(struct vm_area_struct *vma)
>>>>>>>> +{
>>>>>>>> +	struct drm_gem_object *obj = vma->vm_private_data;
>>>>>>>> +	struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
>>>>>>>> +
>>>>>>>> +	drm_gem_shmem_put_pages(shmem);
>>>>>>>> +	drm_gem_vm_close(vma);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +const struct vm_operations_struct drm_gem_shmem_vm_ops = {
>>>>>>>> +	.fault = drm_gem_shmem_fault,
>>>>>>>> +	.open = drm_gem_vm_open,
>>>>>>>> +	.close = drm_gem_shmem_vm_close,
>>>>>>>> +};
>>>>>>>> +EXPORT_SYMBOL_GPL(drm_gem_shmem_vm_ops);
>>>>>>> I just saw a warning from drm_gem_shmem_put_pages() for
>>>>>>> !shmem->pages_use_count -- I think drm_gem_vm_open() needs to
>>>>>>> drm_gem_shmem_get_pages().
>>>>>> Yeah we need a drm_gem_shmem_vm_open here.
>>>>> Adding one of those fixed my refcounting issues, so I've sent out a v6
>>>>> with it.
>>>> Just realized that I've reviewed this patch already, spotted that vma
>>>> management issue there too. Plus a pile of other things. From reading that
>>>> other thread discussion with Noralf concluded with "not yet ready for
>>>> prime time" unfortunately :-/
>>> I saw stuff about how it wasn't usable for SPI because SPI does weird
>>> things with DMA mapping.  Was there something else?
>> Looking through that mail it was a bunch of comments to improve the
>> kerneldoc. Plus a note that buffer sharing/mmap is going to be all
>> incoherent and horrible (but I guess for vkms we don't care that much).
>> I'm just kinda vary of generic buffer handling that turns out to not be
>> actually all that useful. We have lots of deadends and kinda-midlayers in
>> this area already (but this one here definitely smells plenty better than
>> lots of older ones).
> FWIW, I really want shmem helpers for v3d.  The fault handling in
> particular has magic I don't understand, and this is not my first fault
> handler. :/


If you can use it for a "real" hw driver like v3d, I think it makes a lot
sense to have it as a helper. I believe udl and a future simpledrm can
also make use of it.

I have an idea about a usb driver that I hope to work on somewhere down
the line that will need this kind of code. So my plan was to resurrect
this code when I got there.

I agree that fault handling looks a bit like magic. I looked at all the
drivers that uses shmem buffers to see what they where doing. When Daniel
put me on the right track with the fake offsets, the fault handler ended
up being quite small.

Having a helper like this that can actually be used for real hw drivers
(if it can, that is), increases the chance of getting this -mm stuff
right. And hopefully someone down the line having domain knowledge can
audit this code. It's less likely that this will happen with code tucked
away in a driver, especially the smaller ones.

Initially I hoped that I could make the helper compatible with vgem, so I
could convert vgem to use this helper. That would give the helper a lot
of testing, making it and keeping it solid. However vgem can get pages
one by one in the fault handler if all pages hasn't been fetched. I
didn't see an easy way to handle that together with page use counting.
The main reason for having page counting was to make it easy to bolt on a
shrinker, but I have no experience nor any knowledge about that, so I
don't know if it can be easily done.

Noralf.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx