Re: [Intel-gfx] [PATCH v5 4/5] drm: Add library for shmem backed GEM objects
Den 29.01.2019 01.19, skrev Eric Anholt:
> Noralf Trønnes writes:
>
>> Den 28.01.2019 21.57, skrev Rob Herring:
>>> On Sun, Dec 2, 2018 at 9:59 AM Noralf Trønnes wrote:
Den 30.11.2018 00.58, skrev Eric Anholt:
> Daniel Vetter writes:
>
>> On Wed, Nov 28, 2018 at 01:52:56PM -0800, Eric Anholt wrote:
>>> Daniel Vetter writes:
>>>
On Tue, Nov 27, 2018 at 12:38:44PM -0800, Eric Anholt wrote:
> Daniel Vetter writes:
>
>> On Mon, Nov 26, 2018 at 04:36:21PM -0800, Eric Anholt wrote:
>>> Noralf Trønnes 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.
>>>
>>> FWIW, I think etnaviv at least could use this too.
>>>
>>> I'm starting to look at panfrost and lima drivers and was trying to
>>> figure out where to start with the GEM code. So I've been comparing
>>> etnaviv, freedreno, and vgem implementations. They are all pretty
>>> similar from what I see. The per driver GEM obj structs certainly are.
>>> I can't bring myself to just copy etnaviv code over and do a
>>> s/etnaviv/panfrost/. So searching around a bit, I ended up on this
>>> thread. This seems to be what I need for panfrost (based on my brief
>>> study).
>>>
>>
>> I gave up on this due to problems with SPI DMA.
>> Eric tried to use it with vkms, but it failed. On his blog he speculates
>> that it might be due to cached CPU mappings:
>> https://anholt.github.io/twivc4/2018/12/03/twiv/
>>
>> For tinydrm I wanted cached mappings, but it might not work that well
>> with shmem. Maybe that's why I had problems with SPI DMA.
>
> Actually, for tinydrm buffers that are dma-buf exported through prime, I
> really want tinydrm using WC mappings so that vc4 or v3d rendering (now
> supported on Mesa master with kmsro) works.
>
I thought that the buffer is created by the GPU driver when using PRIME?
And them imported into the tinydrm driver. In which case it doesn't
matter how tinydrm creates it's dumb buffers.
That said, I will stay with CMA buffers for the SPI drivers. It just
works. The reason I looked into shmem, is that tindyrm would then be
able to import buffers that isn't contiguous in memory, thus making it
work with more GPU's. And the reason for cached buffers is that with
partial flushing the rectangle is copied to a separate buffer for
transfer and CPU acccess to WC memory is quite slow. But ofc for these
small buffers it's not that big a deal.
Noralf.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v5 4/5] drm: Add library for shmem backed GEM objects
Noralf Trønnes writes:
> Den 28.01.2019 21.57, skrev Rob Herring:
>> On Sun, Dec 2, 2018 at 9:59 AM Noralf Trønnes wrote:
>>>
>>>
>>> Den 30.11.2018 00.58, skrev Eric Anholt:
Daniel Vetter writes:
> On Wed, Nov 28, 2018 at 01:52:56PM -0800, Eric Anholt wrote:
>> Daniel Vetter writes:
>>
>>> On Tue, Nov 27, 2018 at 12:38:44PM -0800, Eric Anholt wrote:
Daniel Vetter writes:
> On Mon, Nov 26, 2018 at 04:36:21PM -0800, Eric Anholt wrote:
>> Noralf Trønnes 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.
>>
>> FWIW, I think etnaviv at least could use this too.
>>
>> I'm starting to look at panfrost and lima drivers and was trying to
>> figure out where to start with the GEM code. So I've been comparing
>> etnaviv, freedreno, and vgem implementations. They are all pretty
>> similar from what I see. The per driver GEM obj structs certainly are.
>> I can't bring myself to just copy etnaviv code over and do a
>> s/etnaviv/panfrost/. So searching around a bit, I ended up on this
>> thread. This seems to be what I need for panfrost (based on my brief
>> study).
>>
>
> I gave up on this due to problems with SPI DMA.
> Eric tried to use it with vkms, but it failed. On his blog he speculates
> that it might be due to cached CPU mappings:
> https://anholt.github.io/twivc4/2018/12/03/twiv/
>
> For tinydrm I wanted cached mappings, but it might not work that well
> with shmem. Maybe that's why I had problems with SPI DMA.
Actually, for tinydrm buffers that are dma-buf exported through prime, I
really want tinydrm using WC mappings so that vc4 or v3d rendering (now
supported on Mesa master with kmsro) works.
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v5 4/5] drm: Add library for shmem backed GEM objects
On Mon, Jan 28, 2019 at 3:26 PM Noralf Trønnes wrote:
>
>
>
> Den 28.01.2019 21.57, skrev Rob Herring:
> > On Sun, Dec 2, 2018 at 9:59 AM Noralf Trønnes wrote:
> >>
> >>
> >> Den 30.11.2018 00.58, skrev Eric Anholt:
> >>> Daniel Vetter writes:
> >>>
> On Wed, Nov 28, 2018 at 01:52:56PM -0800, Eric Anholt wrote:
> > Daniel Vetter writes:
> >
> >> On Tue, Nov 27, 2018 at 12:38:44PM -0800, Eric Anholt wrote:
> >>> Daniel Vetter writes:
> >>>
> On Mon, Nov 26, 2018 at 04:36:21PM -0800, Eric Anholt wrote:
> > Noralf Trønnes 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.
> >
> > FWIW, I think etnaviv at least could use this too.
> >
> > I'm starting to look at panfrost and lima drivers and was trying to
> > figure out where to start with the GEM code. So I've been comparing
> > etnaviv, freedreno, and vgem implementations. They are all pretty
> > similar from what I see. The per driver GEM obj structs certainly are.
> > I can't bring myself to just copy etnaviv code over and do a
> > s/etnaviv/panfrost/. So searching around a bit, I ended up on this
> > thread. This seems to be what I need for panfrost (based on my brief
> > study).
> >
>
> I gave up on this due to problems with SPI DMA.
> Eric tried to use it with vkms, but it failed. On his blog he speculates
> that it might be due to cached CPU mappings:
> https://anholt.github.io/twivc4/2018/12/03/twiv/
>
> For tinydrm I wanted cached mappings, but it might not work that well
> with shmem. Maybe that's why I had problems with SPI DMA.
I think for most ARM systems, a cached mapping is not coherent. So
there will need to be cache flushes using the dma API. I see there's
been some discussion around that too.
> Maybe this change to drm_gem_shmem_mmap() is all it takes:
>
> - vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> + vma->vm_page_prot =
> pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
Seems like at least this part needs some flexibility. etnaviv,
freedreno, and omap at least all appear to support cached, uncached,
and writecombined based on flags in the GEM object.
> The memory subsystem is really complicated and I have kind of given up
> on trying to decipher it.
Yes. All the more reason to not let each driver figure out what to do.
Rob
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v5 4/5] drm: Add library for shmem backed GEM objects
Den 28.01.2019 21.57, skrev Rob Herring:
> On Sun, Dec 2, 2018 at 9:59 AM Noralf Trønnes wrote:
>>
>>
>> Den 30.11.2018 00.58, skrev Eric Anholt:
>>> Daniel Vetter writes:
>>>
On Wed, Nov 28, 2018 at 01:52:56PM -0800, Eric Anholt wrote:
> Daniel Vetter writes:
>
>> On Tue, Nov 27, 2018 at 12:38:44PM -0800, Eric Anholt wrote:
>>> Daniel Vetter writes:
>>>
On Mon, Nov 26, 2018 at 04:36:21PM -0800, Eric Anholt wrote:
> Noralf Trønnes 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.
>
> FWIW, I think etnaviv at least could use this too.
>
> I'm starting to look at panfrost and lima drivers and was trying to
> figure out where to start with the GEM code. So I've been comparing
> etnaviv, freedreno, and vgem implementations. They are all pretty
> similar from what I see. The per driver GEM obj structs certainly are.
> I can't bring myself to just copy etnaviv code over and do a
> s/etnaviv/panfrost/. So searching around a bit, I ended up on this
> thread. This seems to be what I need for panfrost (based on my brief
> study).
>
I gave up on this due to problems with SPI DMA.
Eric tried to use it with vkms, but it failed. On his blog he speculates
that it might be due to cached CPU mappings:
https://anholt.github.io/twivc4/2018/12/03/twiv/
For tinydrm I wanted cached mappings, but it might not work that well
with shmem. Maybe that's why I had problems with SPI DMA.
Maybe this change to drm_gem_shmem_mmap() is all it takes:
- vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
+ vma->vm_page_prot =
pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
The memory subsystem is really complicated and I have kind of given up
on trying to decipher it.
Noralf.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v5 4/5] drm: Add library for shmem backed GEM objects
On Sun, Dec 2, 2018 at 9:59 AM Noralf Trønnes wrote:
>
>
> Den 30.11.2018 00.58, skrev Eric Anholt:
> > Daniel Vetter writes:
> >
> >> On Wed, Nov 28, 2018 at 01:52:56PM -0800, Eric Anholt wrote:
> >>> Daniel Vetter writes:
> >>>
> On Tue, Nov 27, 2018 at 12:38:44PM -0800, Eric Anholt wrote:
> > Daniel Vetter writes:
> >
> >> On Mon, Nov 26, 2018 at 04:36:21PM -0800, Eric Anholt wrote:
> >>> Noralf Trønnes 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.
FWIW, I think etnaviv at least could use this too.
I'm starting to look at panfrost and lima drivers and was trying to
figure out where to start with the GEM code. So I've been comparing
etnaviv, freedreno, and vgem implementations. They are all pretty
similar from what I see. The per driver GEM obj structs certainly are.
I can't bring myself to just copy etnaviv code over and do a
s/etnaviv/panfrost/. So searching around a bit, I ended up on this
thread. This seems to be what I need for panfrost (based on my brief
study).
Rob
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v5 4/5] drm: Add library for shmem backed GEM objects
Den 30.11.2018 00.58, skrev Eric Anholt:
Daniel Vetter writes:
On Wed, Nov 28, 2018 at 01:52:56PM -0800, Eric Anholt wrote:
Daniel Vetter writes:
On Tue, Nov 27, 2018 at 12:38:44PM -0800, Eric Anholt wrote:
Daniel Vetter writes:
On Mon, Nov 26, 2018 at 04:36:21PM -0800, Eric Anholt wrote:
Noralf Trønnes 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
Re: [Intel-gfx] [PATCH v5 4/5] drm: Add library for shmem backed GEM objects
Daniel Vetter writes:
> On Wed, Nov 28, 2018 at 01:52:56PM -0800, Eric Anholt wrote:
>> Daniel Vetter writes:
>>
>> > On Tue, Nov 27, 2018 at 12:38:44PM -0800, Eric Anholt wrote:
>> >> Daniel Vetter writes:
>> >>
>> >> > On Mon, Nov 26, 2018 at 04:36:21PM -0800, Eric Anholt wrote:
>> >> >> Noralf Trønnes 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. :/
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v5 4/5] drm: Add library for shmem backed GEM objects
On Wed, Nov 28, 2018 at 01:52:56PM -0800, Eric Anholt wrote:
> Daniel Vetter writes:
>
> > On Tue, Nov 27, 2018 at 12:38:44PM -0800, Eric Anholt wrote:
> >> Daniel Vetter writes:
> >>
> >> > On Mon, Nov 26, 2018 at 04:36:21PM -0800, Eric Anholt wrote:
> >> >> Noralf Trønnes 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).
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v5 4/5] drm: Add library for shmem backed GEM objects
Daniel Vetter writes:
> On Tue, Nov 27, 2018 at 12:38:44PM -0800, Eric Anholt wrote:
>> Daniel Vetter writes:
>>
>> > On Mon, Nov 26, 2018 at 04:36:21PM -0800, Eric Anholt wrote:
>> >> Noralf Trønnes 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?
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v5 4/5] drm: Add library for shmem backed GEM objects
On Tue, Nov 27, 2018 at 12:38:44PM -0800, Eric Anholt wrote:
> Daniel Vetter writes:
>
> > On Mon, Nov 26, 2018 at 04:36:21PM -0800, Eric Anholt wrote:
> >> Noralf Trønnes 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 :-/
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v5 4/5] drm: Add library for shmem backed GEM objects
Daniel Vetter writes:
> On Mon, Nov 26, 2018 at 04:36:21PM -0800, Eric Anholt wrote:
>> Noralf Trønnes 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.
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v5 4/5] drm: Add library for shmem backed GEM objects
On Mon, Nov 26, 2018 at 04:36:21PM -0800, Eric Anholt wrote:
> Noralf Trønnes 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.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v5 4/5] drm: Add library for shmem backed GEM objects
Noralf Trønnes 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().
signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v5 4/5] drm: Add library for shmem backed GEM objects
On Mon, Oct 22, 2018 at 04:15:48PM +0200, Noralf Trønnes wrote:
>
> Den 17.10.2018 17.46, skrev Daniel Vetter:
> > On Wed, Oct 17, 2018 at 03:04:53PM +0200, Noralf Trønnes wrote:
> > > This adds a library for shmem backed GEM objects.
> > >
> > > 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
> > > ---
> > > Documentation/gpu/drm-kms-helpers.rst | 12 +
> > > drivers/gpu/drm/Kconfig| 6 +
> > > drivers/gpu/drm/Makefile | 1 +
> > > drivers/gpu/drm/drm_gem_shmem_helper.c | 551
> > > +
> > > include/drm/drm_gem_shmem_helper.h | 153 +
> > > 5 files changed, 723 insertions(+)
> > > create mode 100644 drivers/gpu/drm/drm_gem_shmem_helper.c
> > > create mode 100644 include/drm/drm_gem_shmem_helper.h
> > >
>
>
>
> > > +static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf)
> > > +{
> > > + struct vm_area_struct *vma = vmf->vma;
> > > + struct drm_gem_object *obj = vma->vm_private_data;
> > > + struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
> > > + loff_t num_pages = obj->size >> PAGE_SHIFT;
> > > + struct page *page;
> > > +
> > > + if (vmf->pgoff > num_pages || WARN_ON_ONCE(!shmem->pages))
> > > + return VM_FAULT_SIGBUS;
> > > +
> > > + page = shmem->pages[vmf->pgoff];
> > > +
> > > + return vmf_insert_page(vma, vmf->address, page);
> > > +}
> > > +
> > > +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);
> > Imbalance get/put_pages here, if someone forks (which is what calls
> > vm_ops->open on an existing vma).
> >
> > > + 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);
> > > +
> > > +/**
> > > + * drm_gem_shmem_mmap - Memory-map a shmem GEM object
> > > + * @filp: File object
> > > + * @vma: VMA for the area to be mapped
> > > + *
> > > + * This function implements an augmented version of the GEM DRM file mmap
> > > + * operation for shmem objects. Drivers which employ the shmem helpers
> > > should
> > > + * use this function as their _operations.mmap handler in the DRM
> > > device file's
> > > + * file_operations structure.
> > > + *
> > > + * Instead of directly referencing this function, drivers should use the
> > > + * DEFINE_DRM_GEM_SHMEM_FOPS() macro.
> > > + *
> > > + * Returns:
> > > + * 0 on success or a negative error code on failure.
> > > + */
> > Between cma helpers, gem shmem helpers and the drm_gem_mmap we now have
> > quite a bit a confusion of mmap functions. I think it'd be good to update
> > the kerneldoc for drm_gem_mmap() to point at these here (both the shmem
> > and cma version), so that people prefer to use these helpers here. Direct
> > call to drm_gem_mmap would only be needed if you want to use your own
> > vm_ops.
> >
> > > +int drm_gem_shmem_mmap(struct file *filp, struct vm_area_struct *vma)
> > > +{
> > > + struct drm_gem_shmem_object *shmem;
> > > + int ret;
> > > +
> > > + ret = drm_gem_mmap(filp, vma);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + shmem = to_drm_gem_shmem_obj(vma->vm_private_data);
> > > +
> > > + ret = drm_gem_shmem_get_pages(shmem);
> > > + if (ret) {
> > > + drm_gem_vm_close(vma);
> > > + return ret;
> > > + }
> > > +
> > > + /* VM_PFNMAP was set by drm_gem_mmap() */
> > > + vma->vm_flags &= ~VM_PFNMAP;
> > > + vma->vm_flags |= VM_MIXEDMAP;
> > > + vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
> > > +
> > > + fput(vma->vm_file);
> > > + vma->vm_file = get_file(shmem->base.filp);
> > > + /* Remove the fake offset */
> > > + vma->vm_pgoff -= drm_vma_node_start(>base.vma_node);
> > > +
> > > + return 0;
> > > +}
> > > +EXPORT_SYMBOL_GPL(drm_gem_shmem_mmap);
> > Looking through your mmap code there's 2 bits:
> > - shmem isn't coherent,
>
> You saved me in the last minute with that comment Daniel.
>
> I just remembered that on the Raspberry Pi I need to swap the bytes (RGB565,
> SPI big endian) before transfer because its SPI can't do 16-bit transfers.
> So I removed the swapping and sent the shmem buffer straight through to SPI.
> This resulted
Re: [Intel-gfx] [PATCH v5 4/5] drm: Add library for shmem backed GEM objects
Den 17.10.2018 17.46, skrev Daniel Vetter:
On Wed, Oct 17, 2018 at 03:04:53PM +0200, Noralf Trønnes wrote:
This adds a library for shmem backed GEM objects.
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
---
Documentation/gpu/drm-kms-helpers.rst | 12 +
drivers/gpu/drm/Kconfig| 6 +
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/drm_gem_shmem_helper.c | 551 +
include/drm/drm_gem_shmem_helper.h | 153 +
5 files changed, 723 insertions(+)
create mode 100644 drivers/gpu/drm/drm_gem_shmem_helper.c
create mode 100644 include/drm/drm_gem_shmem_helper.h
+static vm_fault_t drm_gem_shmem_fault(struct vm_fault *vmf)
+{
+ struct vm_area_struct *vma = vmf->vma;
+ struct drm_gem_object *obj = vma->vm_private_data;
+ struct drm_gem_shmem_object *shmem = to_drm_gem_shmem_obj(obj);
+ loff_t num_pages = obj->size >> PAGE_SHIFT;
+ struct page *page;
+
+ if (vmf->pgoff > num_pages || WARN_ON_ONCE(!shmem->pages))
+ return VM_FAULT_SIGBUS;
+
+ page = shmem->pages[vmf->pgoff];
+
+ return vmf_insert_page(vma, vmf->address, page);
+}
+
+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);
Imbalance get/put_pages here, if someone forks (which is what calls
vm_ops->open on an existing vma).
+ 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);
+
+/**
+ * drm_gem_shmem_mmap - Memory-map a shmem GEM object
+ * @filp: File object
+ * @vma: VMA for the area to be mapped
+ *
+ * This function implements an augmented version of the GEM DRM file mmap
+ * operation for shmem objects. Drivers which employ the shmem helpers should
+ * use this function as their _operations.mmap handler in the DRM device
file's
+ * file_operations structure.
+ *
+ * Instead of directly referencing this function, drivers should use the
+ * DEFINE_DRM_GEM_SHMEM_FOPS() macro.
+ *
+ * Returns:
+ * 0 on success or a negative error code on failure.
+ */
Between cma helpers, gem shmem helpers and the drm_gem_mmap we now have
quite a bit a confusion of mmap functions. I think it'd be good to update
the kerneldoc for drm_gem_mmap() to point at these here (both the shmem
and cma version), so that people prefer to use these helpers here. Direct
call to drm_gem_mmap would only be needed if you want to use your own
vm_ops.
+int drm_gem_shmem_mmap(struct file *filp, struct vm_area_struct *vma)
+{
+ struct drm_gem_shmem_object *shmem;
+ int ret;
+
+ ret = drm_gem_mmap(filp, vma);
+ if (ret)
+ return ret;
+
+ shmem = to_drm_gem_shmem_obj(vma->vm_private_data);
+
+ ret = drm_gem_shmem_get_pages(shmem);
+ if (ret) {
+ drm_gem_vm_close(vma);
+ return ret;
+ }
+
+ /* VM_PFNMAP was set by drm_gem_mmap() */
+ vma->vm_flags &= ~VM_PFNMAP;
+ vma->vm_flags |= VM_MIXEDMAP;
+ vma->vm_page_prot = vm_get_page_prot(vma->vm_flags);
+
+ fput(vma->vm_file);
+ vma->vm_file = get_file(shmem->base.filp);
+ /* Remove the fake offset */
+ vma->vm_pgoff -= drm_vma_node_start(>base.vma_node);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(drm_gem_shmem_mmap);
Looking through your mmap code there's 2 bits:
- shmem isn't coherent,
You saved me in the last minute with that comment Daniel.
I just remembered that on the Raspberry Pi I need to swap the bytes
(RGB565,
SPI big endian) before transfer because its SPI can't do 16-bit transfers.
So I removed the swapping and sent the shmem buffer straight through to SPI.
This resulted in fbcon looking erratic and "distorted", so it didn't work
with the generic fbdev emulation shadow buffer being copied by the cpu to
the shmem buffer which is DMA'ed out over SPI. Even though the SPI core is
using the streaming DMA API.
(apparently I've worked so much on DRM helper refactoring that I've
forgotten how the tinydrm drivers work...)
So it looks like I can't use shmem for the current tinydrm drivers, which
all use SPI. Unless there's a solution to my problem, I guess I'll I have
to postpone this shmem work until I can use it in a driver that I'm
Re: [Intel-gfx] [PATCH v5 4/5] drm: Add library for shmem backed GEM objects
On Wed, Oct 17, 2018 at 03:04:53PM +0200, Noralf Trønnes wrote:
> This adds a library for shmem backed GEM objects.
>
> 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
> ---
> Documentation/gpu/drm-kms-helpers.rst | 12 +
> drivers/gpu/drm/Kconfig| 6 +
> drivers/gpu/drm/Makefile | 1 +
> drivers/gpu/drm/drm_gem_shmem_helper.c | 551
> +
> include/drm/drm_gem_shmem_helper.h | 153 +
> 5 files changed, 723 insertions(+)
> create mode 100644 drivers/gpu/drm/drm_gem_shmem_helper.c
> create mode 100644 include/drm/drm_gem_shmem_helper.h
>
> diff --git a/Documentation/gpu/drm-kms-helpers.rst
> b/Documentation/gpu/drm-kms-helpers.rst
> index 4b4dc236ef6f..8305d3566928 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -335,3 +335,15 @@ Legacy CRTC/Modeset Helper Functions Reference
>
> .. kernel-doc:: drivers/gpu/drm/drm_crtc_helper.c
> :export:
> +
> +SHMEM GEM Helper Reference
> +==
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_gem_shmem_helper.c
> + :doc: overview
> +
> +.. kernel-doc:: include/drm/drm_gem_shmem_helper.h
> + :internal:
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_gem_shmem_helper.c
> + :export:
This doesn't make sense here imo - put them right next to the cma helpers
in drm-mm.rst instead?
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 736b7e67e4ec..46090a36f52e 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -157,6 +157,12 @@ config DRM_KMS_CMA_HELPER
> help
> Choose this if you need the KMS CMA helper functions
>
> +config DRM_GEM_SHMEM_HELPER
> + bool
> + depends on DRM
> + help
> + Choose this if you need the GEM shmem helper functions
> +
> config DRM_VM
> bool
> depends on DRM && MMU
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 576ba985e138..8733ceb41292 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -25,6 +25,7 @@ drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
> drm-$(CONFIG_DRM_VM) += drm_vm.o
> drm-$(CONFIG_COMPAT) += drm_ioc32.o
> drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
> +drm-$(CONFIG_DRM_GEM_SHMEM_HELPER) += drm_gem_shmem_helper.o
> drm-$(CONFIG_PCI) += ati_pcigart.o
> drm-$(CONFIG_DRM_PANEL) += drm_panel.o
> drm-$(CONFIG_OF) += drm_of.o
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
> b/drivers/gpu/drm/drm_gem_shmem_helper.c
> new file mode 100644
> index ..c4eec51d3282
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -0,0 +1,551 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2018 Noralf Trønnes
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/**
> + * DOC: overview
> + *
> + * This library provides helpers for GEM objects backed by shmem buffers
> + * allocated using anonymous pageable memory.
Would be neat to insert a link to the cma version here (and to the gem
version in the cma helpers) and spend a few words on when to use each.
> + */
> +
> +static const struct drm_gem_object_funcs drm_gem_shmem_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 = drm_gem_shmem_vmap,
> + .vunmap = drm_gem_shmem_vunmap,
> + .vm_ops = _gem_shmem_vm_ops,
> +};
> +
> +/**
> + * drm_gem_shmem_create - Allocate an object with the given size
> + * @dev: DRM device
> + * @size: Size of the object to allocate
> + *
> + * This function creates a shmem GEM object.
> + *
> + * Returns:
> + * A struct drm_gem_shmem_object * on success or an ERR_PTR()-encoded
> negative
> + * error code on failure.
> + */
> +struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev,
> size_t size)
> +{
> + struct drm_gem_shmem_object *shmem;
> + struct drm_gem_object *obj;
> + int ret;
> +
> + size = PAGE_ALIGN(size);
> +
> + if (dev->driver->gem_create_object)
> + obj = dev->driver->gem_create_object(dev, size);
> + else
> + obj = kzalloc(sizeof(*shmem), GFP_KERNEL);
> + if (!obj)
> + return ERR_PTR(-ENOMEM);
> +
> + if (!obj->funcs)
> + obj->funcs =
[Intel-gfx] [PATCH v5 4/5] drm: Add library for shmem backed GEM objects
This adds a library for shmem backed GEM objects.
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
---
Documentation/gpu/drm-kms-helpers.rst | 12 +
drivers/gpu/drm/Kconfig| 6 +
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/drm_gem_shmem_helper.c | 551 +
include/drm/drm_gem_shmem_helper.h | 153 +
5 files changed, 723 insertions(+)
create mode 100644 drivers/gpu/drm/drm_gem_shmem_helper.c
create mode 100644 include/drm/drm_gem_shmem_helper.h
diff --git a/Documentation/gpu/drm-kms-helpers.rst
b/Documentation/gpu/drm-kms-helpers.rst
index 4b4dc236ef6f..8305d3566928 100644
--- a/Documentation/gpu/drm-kms-helpers.rst
+++ b/Documentation/gpu/drm-kms-helpers.rst
@@ -335,3 +335,15 @@ Legacy CRTC/Modeset Helper Functions Reference
.. kernel-doc:: drivers/gpu/drm/drm_crtc_helper.c
:export:
+
+SHMEM GEM Helper Reference
+==
+
+.. kernel-doc:: drivers/gpu/drm/drm_gem_shmem_helper.c
+ :doc: overview
+
+.. kernel-doc:: include/drm/drm_gem_shmem_helper.h
+ :internal:
+
+.. kernel-doc:: drivers/gpu/drm/drm_gem_shmem_helper.c
+ :export:
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 736b7e67e4ec..46090a36f52e 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -157,6 +157,12 @@ config DRM_KMS_CMA_HELPER
help
Choose this if you need the KMS CMA helper functions
+config DRM_GEM_SHMEM_HELPER
+ bool
+ depends on DRM
+ help
+ Choose this if you need the GEM shmem helper functions
+
config DRM_VM
bool
depends on DRM && MMU
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 576ba985e138..8733ceb41292 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -25,6 +25,7 @@ drm-$(CONFIG_DRM_LIB_RANDOM) += lib/drm_random.o
drm-$(CONFIG_DRM_VM) += drm_vm.o
drm-$(CONFIG_COMPAT) += drm_ioc32.o
drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
+drm-$(CONFIG_DRM_GEM_SHMEM_HELPER) += drm_gem_shmem_helper.o
drm-$(CONFIG_PCI) += ati_pcigart.o
drm-$(CONFIG_DRM_PANEL) += drm_panel.o
drm-$(CONFIG_OF) += drm_of.o
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
b/drivers/gpu/drm/drm_gem_shmem_helper.c
new file mode 100644
index ..c4eec51d3282
--- /dev/null
+++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
@@ -0,0 +1,551 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2018 Noralf Trønnes
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+#include
+#include
+
+/**
+ * DOC: overview
+ *
+ * This library provides helpers for GEM objects backed by shmem buffers
+ * allocated using anonymous pageable memory.
+ */
+
+static const struct drm_gem_object_funcs drm_gem_shmem_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 = drm_gem_shmem_vmap,
+ .vunmap = drm_gem_shmem_vunmap,
+ .vm_ops = _gem_shmem_vm_ops,
+};
+
+/**
+ * drm_gem_shmem_create - Allocate an object with the given size
+ * @dev: DRM device
+ * @size: Size of the object to allocate
+ *
+ * This function creates a shmem GEM object.
+ *
+ * Returns:
+ * A struct drm_gem_shmem_object * on success or an ERR_PTR()-encoded negative
+ * error code on failure.
+ */
+struct drm_gem_shmem_object *drm_gem_shmem_create(struct drm_device *dev,
size_t size)
+{
+ struct drm_gem_shmem_object *shmem;
+ struct drm_gem_object *obj;
+ int ret;
+
+ size = PAGE_ALIGN(size);
+
+ if (dev->driver->gem_create_object)
+ obj = dev->driver->gem_create_object(dev, size);
+ else
+ obj = kzalloc(sizeof(*shmem), GFP_KERNEL);
+ if (!obj)
+ return ERR_PTR(-ENOMEM);
+
+ if (!obj->funcs)
+ obj->funcs = _gem_shmem_funcs;
+
+ ret = drm_gem_object_init(dev, obj, size);
+ if (ret)
+ goto err_free;
+
+ ret = drm_gem_create_mmap_offset(obj);
+ if (ret)
+ goto err_release;
+
+ shmem = to_drm_gem_shmem_obj(obj);
+ mutex_init(>pages_lock);
+ mutex_init(>vmap_lock);
+
+ return shmem;
+
+err_release:
+ drm_gem_object_release(obj);
+err_free:
+ kfree(shmem);
+
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(drm_gem_shmem_create);
+
+/**
+ * drm_gem_shmem_free_object - Free
