Message ID | 20230108210445.3948344-1-dmitry.osipenko@collabora.com (mailing list archive) |
---|---|
Headers | show |
Series | Add generic memory shrinker to VirtIO-GPU and Panfrost DRM drivers | expand |
Hello Thomas and Gerd, On 1/9/23 00:04, Dmitry Osipenko wrote: > This series: > > 1. Makes minor fixes for drm_gem_lru and Panfrost > 2. Brings refactoring for older code > 3. Adds common drm-shmem memory shrinker > 4. Enables shrinker for VirtIO-GPU driver > 5. Switches Panfrost driver to the common shrinker > > Changelog: > > v10:- Rebased on a recent linux-next. > > - Added Rob's ack to MSM "Prevent blocking within shrinker loop" patch. > > - Added Steven's ack/r-b/t-b for the Panfrost patches. > > - Fixed missing export of the new drm_gem_object_evict() function. > > - Added fixes tags to the first two patches that are making minor fixes, > for consistency. Do you have comments on this version? Otherwise ack will be appreciated. Thanks in advance!
On Thu, Jan 26, 2023 at 01:55:09AM +0300, Dmitry Osipenko wrote: > Hello Thomas and Gerd, > > On 1/9/23 00:04, Dmitry Osipenko wrote: > > This series: > > > > 1. Makes minor fixes for drm_gem_lru and Panfrost > > 2. Brings refactoring for older code > > 3. Adds common drm-shmem memory shrinker > > 4. Enables shrinker for VirtIO-GPU driver > > 5. Switches Panfrost driver to the common shrinker > > > > Changelog: > > > > v10:- Rebased on a recent linux-next. > > > > - Added Rob's ack to MSM "Prevent blocking within shrinker loop" patch. > > > > - Added Steven's ack/r-b/t-b for the Panfrost patches. > > > > - Fixed missing export of the new drm_gem_object_evict() function. > > > > - Added fixes tags to the first two patches that are making minor fixes, > > for consistency. > > Do you have comments on this version? Otherwise ack will be appreciated. > Thanks in advance! Don't feel like signing off on the locking changes, I'm not that familiar with the drm locking rules. So someone else looking at them would be good. Otherwise the series and specifically the virtio changes look good to me. Acked-by: Gerd Hoffmann <kraxel@redhat.com> take care, Gerd
On 1/27/23 11:13, Gerd Hoffmann wrote: > On Thu, Jan 26, 2023 at 01:55:09AM +0300, Dmitry Osipenko wrote: >> Hello Thomas and Gerd, >> >> On 1/9/23 00:04, Dmitry Osipenko wrote: >>> This series: >>> >>> 1. Makes minor fixes for drm_gem_lru and Panfrost >>> 2. Brings refactoring for older code >>> 3. Adds common drm-shmem memory shrinker >>> 4. Enables shrinker for VirtIO-GPU driver >>> 5. Switches Panfrost driver to the common shrinker >>> >>> Changelog: >>> >>> v10:- Rebased on a recent linux-next. >>> >>> - Added Rob's ack to MSM "Prevent blocking within shrinker loop" patch. >>> >>> - Added Steven's ack/r-b/t-b for the Panfrost patches. >>> >>> - Fixed missing export of the new drm_gem_object_evict() function. >>> >>> - Added fixes tags to the first two patches that are making minor fixes, >>> for consistency. >> >> Do you have comments on this version? Otherwise ack will be appreciated. >> Thanks in advance! > > Don't feel like signing off on the locking changes, I'm not that > familiar with the drm locking rules. So someone else looking at them > would be good. Otherwise the series and specifically the virtio changes > look good to me. > > Acked-by: Gerd Hoffmann <kraxel@redhat.com> Thomas was looking at the the DRM core changes. I expect he'll ack them. Thank you for reviewing the virtio patches!
On Mon, Jan 30, 2023 at 03:02:10PM +0300, Dmitry Osipenko wrote: > On 1/27/23 11:13, Gerd Hoffmann wrote: > > On Thu, Jan 26, 2023 at 01:55:09AM +0300, Dmitry Osipenko wrote: > >> Hello Thomas and Gerd, > >> > >> On 1/9/23 00:04, Dmitry Osipenko wrote: > >>> This series: > >>> > >>> 1. Makes minor fixes for drm_gem_lru and Panfrost > >>> 2. Brings refactoring for older code > >>> 3. Adds common drm-shmem memory shrinker > >>> 4. Enables shrinker for VirtIO-GPU driver > >>> 5. Switches Panfrost driver to the common shrinker > >>> > >>> Changelog: > >>> > >>> v10:- Rebased on a recent linux-next. > >>> > >>> - Added Rob's ack to MSM "Prevent blocking within shrinker loop" patch. > >>> > >>> - Added Steven's ack/r-b/t-b for the Panfrost patches. > >>> > >>> - Fixed missing export of the new drm_gem_object_evict() function. > >>> > >>> - Added fixes tags to the first two patches that are making minor fixes, > >>> for consistency. > >> > >> Do you have comments on this version? Otherwise ack will be appreciated. > >> Thanks in advance! > > > > Don't feel like signing off on the locking changes, I'm not that > > familiar with the drm locking rules. So someone else looking at them > > would be good. Otherwise the series and specifically the virtio changes > > look good to me. > > > > Acked-by: Gerd Hoffmann <kraxel@redhat.com> > > Thomas was looking at the the DRM core changes. I expect he'll ack them. > > Thank you for reviewing the virtio patches! I think best-case would be an ack from msm people that this looks good (even better a conversion for msm to start using this). Otherwise I think the locking looks reasonable, I think the tricky bits have been moving the dma-buf rules, but if you want I can try to take another in-depth look. But would need to be in 2 weeks since I'm going on vacations, pls ping me on irc if I'm needed. Otherwise would be great if we can land this soon, so that it can soak the entire linux-next cycle to catch any driver specific issues. -Daniel
Il 16/02/23 13:15, Daniel Vetter ha scritto: > On Mon, Jan 30, 2023 at 03:02:10PM +0300, Dmitry Osipenko wrote: >> On 1/27/23 11:13, Gerd Hoffmann wrote: >>> On Thu, Jan 26, 2023 at 01:55:09AM +0300, Dmitry Osipenko wrote: >>>> Hello Thomas and Gerd, >>>> >>>> On 1/9/23 00:04, Dmitry Osipenko wrote: >>>>> This series: >>>>> >>>>> 1. Makes minor fixes for drm_gem_lru and Panfrost >>>>> 2. Brings refactoring for older code >>>>> 3. Adds common drm-shmem memory shrinker >>>>> 4. Enables shrinker for VirtIO-GPU driver >>>>> 5. Switches Panfrost driver to the common shrinker >>>>> >>>>> Changelog: >>>>> >>>>> v10:- Rebased on a recent linux-next. >>>>> >>>>> - Added Rob's ack to MSM "Prevent blocking within shrinker loop" patch. >>>>> >>>>> - Added Steven's ack/r-b/t-b for the Panfrost patches. >>>>> >>>>> - Fixed missing export of the new drm_gem_object_evict() function. >>>>> >>>>> - Added fixes tags to the first two patches that are making minor fixes, >>>>> for consistency. >>>> >>>> Do you have comments on this version? Otherwise ack will be appreciated. >>>> Thanks in advance! >>> >>> Don't feel like signing off on the locking changes, I'm not that >>> familiar with the drm locking rules. So someone else looking at them >>> would be good. Otherwise the series and specifically the virtio changes >>> look good to me. >>> >>> Acked-by: Gerd Hoffmann <kraxel@redhat.com> >> >> Thomas was looking at the the DRM core changes. I expect he'll ack them. >> >> Thank you for reviewing the virtio patches! > > I think best-case would be an ack from msm people that this looks good > (even better a conversion for msm to start using this). > Dmitry B, Konrad, can you please help with this one? Thanks! Regards, Angelo > Otherwise I think the locking looks reasonable, I think the tricky bits > have been moving the dma-buf rules, but if you want I can try to take > another in-depth look. But would need to be in 2 weeks since I'm going on > vacations, pls ping me on irc if I'm needed. > > Otherwise would be great if we can land this soon, so that it can soak the > entire linux-next cycle to catch any driver specific issues. > -Daniel
On 2/16/23 15:15, Daniel Vetter wrote: > On Mon, Jan 30, 2023 at 03:02:10PM +0300, Dmitry Osipenko wrote: >> On 1/27/23 11:13, Gerd Hoffmann wrote: >>> On Thu, Jan 26, 2023 at 01:55:09AM +0300, Dmitry Osipenko wrote: >>>> Hello Thomas and Gerd, >>>> >>>> On 1/9/23 00:04, Dmitry Osipenko wrote: >>>>> This series: >>>>> >>>>> 1. Makes minor fixes for drm_gem_lru and Panfrost >>>>> 2. Brings refactoring for older code >>>>> 3. Adds common drm-shmem memory shrinker >>>>> 4. Enables shrinker for VirtIO-GPU driver >>>>> 5. Switches Panfrost driver to the common shrinker >>>>> >>>>> Changelog: >>>>> >>>>> v10:- Rebased on a recent linux-next. >>>>> >>>>> - Added Rob's ack to MSM "Prevent blocking within shrinker loop" patch. >>>>> >>>>> - Added Steven's ack/r-b/t-b for the Panfrost patches. >>>>> >>>>> - Fixed missing export of the new drm_gem_object_evict() function. >>>>> >>>>> - Added fixes tags to the first two patches that are making minor fixes, >>>>> for consistency. >>>> >>>> Do you have comments on this version? Otherwise ack will be appreciated. >>>> Thanks in advance! >>> >>> Don't feel like signing off on the locking changes, I'm not that >>> familiar with the drm locking rules. So someone else looking at them >>> would be good. Otherwise the series and specifically the virtio changes >>> look good to me. >>> >>> Acked-by: Gerd Hoffmann <kraxel@redhat.com> >> >> Thomas was looking at the the DRM core changes. I expect he'll ack them. >> >> Thank you for reviewing the virtio patches! > > I think best-case would be an ack from msm people that this looks good > (even better a conversion for msm to start using this). The MSM pretty much isn't touched by this patchset, apart from the minor common shrinker fix. Moving whole MSM to use drm_shmem should be a big change to the driver. The Panfrost and VirtIO-GPU drivers already got the acks. I also tested the Lima driver, which uses drm-shmem helpers. Other DRM drivers should be unaffected by this series. > Otherwise I think the locking looks reasonable, I think the tricky bits > have been moving the dma-buf rules, but if you want I can try to take > another in-depth look. But would need to be in 2 weeks since I'm going on > vacations, pls ping me on irc if I'm needed. The locking conversion is mostly a straightforward replacement of mutex with resv lock for drm-shmem. The dma-buf rules were tricky, another tricky part was fixing the lockdep warning for the bogus report of fs_reclaim vs GEM shrinker at the GEM destroy time where I borrowed the drm_gem_shmem_resv_assert_held() solution from the MSM driver where Rob had a similar issue. > Otherwise would be great if we can land this soon, so that it can soak the > entire linux-next cycle to catch any driver specific issues. That will be great. Was waiting for Thomas to ack the shmem patches since he reviewed the previous versions, but if you or anyone else could ack them, then will be good too. Thanks!
On Thu, Feb 16, 2023 at 11:43:38PM +0300, Dmitry Osipenko wrote: > On 2/16/23 15:15, Daniel Vetter wrote: > > On Mon, Jan 30, 2023 at 03:02:10PM +0300, Dmitry Osipenko wrote: > >> On 1/27/23 11:13, Gerd Hoffmann wrote: > >>> On Thu, Jan 26, 2023 at 01:55:09AM +0300, Dmitry Osipenko wrote: > >>>> Hello Thomas and Gerd, > >>>> > >>>> On 1/9/23 00:04, Dmitry Osipenko wrote: > >>>>> This series: > >>>>> > >>>>> 1. Makes minor fixes for drm_gem_lru and Panfrost > >>>>> 2. Brings refactoring for older code > >>>>> 3. Adds common drm-shmem memory shrinker > >>>>> 4. Enables shrinker for VirtIO-GPU driver > >>>>> 5. Switches Panfrost driver to the common shrinker > >>>>> > >>>>> Changelog: > >>>>> > >>>>> v10:- Rebased on a recent linux-next. > >>>>> > >>>>> - Added Rob's ack to MSM "Prevent blocking within shrinker loop" patch. > >>>>> > >>>>> - Added Steven's ack/r-b/t-b for the Panfrost patches. > >>>>> > >>>>> - Fixed missing export of the new drm_gem_object_evict() function. > >>>>> > >>>>> - Added fixes tags to the first two patches that are making minor fixes, > >>>>> for consistency. > >>>> > >>>> Do you have comments on this version? Otherwise ack will be appreciated. > >>>> Thanks in advance! > >>> > >>> Don't feel like signing off on the locking changes, I'm not that > >>> familiar with the drm locking rules. So someone else looking at them > >>> would be good. Otherwise the series and specifically the virtio changes > >>> look good to me. > >>> > >>> Acked-by: Gerd Hoffmann <kraxel@redhat.com> > >> > >> Thomas was looking at the the DRM core changes. I expect he'll ack them. > >> > >> Thank you for reviewing the virtio patches! > > > > I think best-case would be an ack from msm people that this looks good > > (even better a conversion for msm to start using this). > > The MSM pretty much isn't touched by this patchset, apart from the minor > common shrinker fix. Moving whole MSM to use drm_shmem should be a big > change to the driver. > > The Panfrost and VirtIO-GPU drivers already got the acks. I also tested > the Lima driver, which uses drm-shmem helpers. Other DRM drivers should > be unaffected by this series. Ah that sounds good, I somehow thought that etnaviv also uses the helpers, but there we only had problems with dma-buf. So that's all sorted. > > Otherwise I think the locking looks reasonable, I think the tricky bits > > have been moving the dma-buf rules, but if you want I can try to take > > another in-depth look. But would need to be in 2 weeks since I'm going on > > vacations, pls ping me on irc if I'm needed. > > The locking conversion is mostly a straightforward replacement of mutex > with resv lock for drm-shmem. The dma-buf rules were tricky, another > tricky part was fixing the lockdep warning for the bogus report of > fs_reclaim vs GEM shrinker at the GEM destroy time where I borrowed the > drm_gem_shmem_resv_assert_held() solution from the MSM driver where Rob > had a similar issue. Ah I missed that detail, if msm solved that the same way then I think very high chances it all ends up being compatible. Which is really what matters, not so much whether every last driver actually has converted over. > > Otherwise would be great if we can land this soon, so that it can soak the > > entire linux-next cycle to catch any driver specific issues. > > That will be great. Was waiting for Thomas to ack the shmem patches > since he reviewed the previous versions, but if you or anyone else could > ack them, then will be good too. Thanks! I'm good for an ack, but maybe ping Thomas for a review on irc since I'm out next week. Also maybe Thomas has some series you can help land for cross review. -Daniel
Hi, I looked through the series. Most of the patches should have an r-b or a-b at this point. I can't say much about patch 2 and had questions about others. Maybe you can already land patches 2, and 4 to 6? They look independent from the shrinker changes. You could also attempt to land the locking changes in patch 7. They need to get testing. I'll send you an a-b for the patch. Best regards Thomas Am 08.01.23 um 22:04 schrieb Dmitry Osipenko: > This series: > > 1. Makes minor fixes for drm_gem_lru and Panfrost > 2. Brings refactoring for older code > 3. Adds common drm-shmem memory shrinker > 4. Enables shrinker for VirtIO-GPU driver > 5. Switches Panfrost driver to the common shrinker > > Changelog: > > v10:- Rebased on a recent linux-next. > > - Added Rob's ack to MSM "Prevent blocking within shrinker loop" patch. > > - Added Steven's ack/r-b/t-b for the Panfrost patches. > > - Fixed missing export of the new drm_gem_object_evict() function. > > - Added fixes tags to the first two patches that are making minor fixes, > for consistency. > > v9: - Replaced struct drm_gem_shmem_shrinker with drm_gem_shmem and > moved it to drm_device, like was suggested by Thomas Zimmermann. > > - Replaced drm_gem_shmem_shrinker_register() with drmm_gem_shmem_init(), > like was suggested by Thomas Zimmermann. > > - Moved evict() callback to drm_gem_object_funcs and added common > drm_gem_object_evict() helper, like was suggested by Thomas Zimmermann. > > - The shmem object now is evictable by default, like was suggested by > Thomas Zimmermann. Dropped the set_evictable/purgeble() functions > as well, drivers will decide whether BO is evictable within theirs > madvise IOCTL. > > - Added patches that convert drm-shmem code to use drm_WARN_ON() and > drm_dbg_kms(), like was requested by Thomas Zimmermann. > > - Turned drm_gem_shmem_object booleans into 1-bit bit fields, like was > suggested by Thomas Zimmermann. > > - Switched to use drm_dev->unique for the shmem shrinker name. Drivers > don't need to specify the name explicitly anymore. > > - Re-added dma_resv_test_signaled() that was missing in v8 and also > fixed its argument to DMA_RESV_USAGE_READ. See comment to > dma_resv_usage_rw(). > > - Added new fix for Panfrost driver that silences lockdep warning > caused by shrinker. Both Panfrost old and new shmem shrinkers are > affected. > > v8: - Rebased on top of recent linux-next that now has dma-buf locking > convention patches merged, which was blocking shmem shrinker before. > > - Shmem shrinker now uses new drm_gem_lru helper. > > - Dropped Steven Price t-b from the Panfrost patch because code > changed significantly since v6 and should be re-tested. > > v7: - dma-buf locking convention > > v6: https://lore.kernel.org/dri-devel/20220526235040.678984-1-dmitry.osipenko@collabora.com/ > > Related patches: > > Mesa: https://gitlab.freedesktop.org/digetx/mesa/-/commits/virgl-madvise > igt: https://gitlab.freedesktop.org/digetx/igt-gpu-tools/-/commits/virtio-madvise > https://gitlab.freedesktop.org/digetx/igt-gpu-tools/-/commits/panfrost-madvise > > The Mesa and IGT patches will be sent out once the kernel part will land. > > Dmitry Osipenko (11): > drm/msm/gem: Prevent blocking within shrinker loop > drm/panfrost: Don't sync rpm suspension after mmu flushing > drm/gem: Add evict() callback to drm_gem_object_funcs > drm/shmem: Put booleans in the end of struct drm_gem_shmem_object > drm/shmem: Switch to use drm_* debug helpers > drm/shmem-helper: Don't use vmap_use_count for dma-bufs > drm/shmem-helper: Switch to reservation lock > drm/shmem-helper: Add memory shrinker > drm/gem: Add drm_gem_pin_unlocked() > drm/virtio: Support memory shrinking > drm/panfrost: Switch to generic memory shrinker > > drivers/gpu/drm/drm_gem.c | 54 +- > drivers/gpu/drm/drm_gem_shmem_helper.c | 646 +++++++++++++----- > drivers/gpu/drm/lima/lima_gem.c | 8 +- > drivers/gpu/drm/msm/msm_gem_shrinker.c | 8 +- > drivers/gpu/drm/panfrost/Makefile | 1 - > drivers/gpu/drm/panfrost/panfrost_device.h | 4 - > drivers/gpu/drm/panfrost/panfrost_drv.c | 34 +- > drivers/gpu/drm/panfrost/panfrost_gem.c | 30 +- > drivers/gpu/drm/panfrost/panfrost_gem.h | 9 - > .../gpu/drm/panfrost/panfrost_gem_shrinker.c | 122 ---- > drivers/gpu/drm/panfrost/panfrost_job.c | 18 +- > drivers/gpu/drm/panfrost/panfrost_mmu.c | 21 +- > drivers/gpu/drm/virtio/virtgpu_drv.h | 18 +- > drivers/gpu/drm/virtio/virtgpu_gem.c | 52 ++ > drivers/gpu/drm/virtio/virtgpu_ioctl.c | 37 + > drivers/gpu/drm/virtio/virtgpu_kms.c | 8 + > drivers/gpu/drm/virtio/virtgpu_object.c | 132 +++- > drivers/gpu/drm/virtio/virtgpu_plane.c | 22 +- > drivers/gpu/drm/virtio/virtgpu_vq.c | 40 ++ > include/drm/drm_device.h | 10 +- > include/drm/drm_gem.h | 19 +- > include/drm/drm_gem_shmem_helper.h | 112 +-- > include/uapi/drm/virtgpu_drm.h | 14 + > 23 files changed, 1010 insertions(+), 409 deletions(-) > delete mode 100644 drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c >
On 2/17/23 16:28, Thomas Zimmermann wrote: > Hi, > > I looked through the series. Most of the patches should have an r-b or > a-b at this point. I can't say much about patch 2 and had questions > about others. > > Maybe you can already land patches 2, and 4 to 6? They look independent > from the shrinker changes. You could also attempt to land the locking > changes in patch 7. They need to get testing. I'll send you an a-b for > the patch. Thank you, I'll apply the acked patches and then make v11 with the remaining patches updated. Not sure if it will be possible to split patch 8, but I'll think on it for v11.
On 2/17/23 16:41, Dmitry Osipenko wrote: > On 2/17/23 16:28, Thomas Zimmermann wrote: >> Hi, >> >> I looked through the series. Most of the patches should have an r-b or >> a-b at this point. I can't say much about patch 2 and had questions >> about others. >> >> Maybe you can already land patches 2, and 4 to 6? They look independent >> from the shrinker changes. You could also attempt to land the locking >> changes in patch 7. They need to get testing. I'll send you an a-b for >> the patch. > > Thank you, I'll apply the acked patches and then make v11 with the > remaining patches updated. > > Not sure if it will be possible to split patch 8, but I'll think on it > for v11. > Applied patches 1-2 to misc-fixes and patches 3-7 to misc-next, with the review comments addressed.
On Mon, 27 Feb 2023, Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: > On 2/17/23 16:41, Dmitry Osipenko wrote: >> On 2/17/23 16:28, Thomas Zimmermann wrote: >>> Hi, >>> >>> I looked through the series. Most of the patches should have an r-b or >>> a-b at this point. I can't say much about patch 2 and had questions >>> about others. >>> >>> Maybe you can already land patches 2, and 4 to 6? They look independent >>> from the shrinker changes. You could also attempt to land the locking >>> changes in patch 7. They need to get testing. I'll send you an a-b for >>> the patch. >> >> Thank you, I'll apply the acked patches and then make v11 with the >> remaining patches updated. >> >> Not sure if it will be possible to split patch 8, but I'll think on it >> for v11. >> > > Applied patches 1-2 to misc-fixes and patches 3-7 to misc-next, with the > review comments addressed. Please resolve the drm-tip rebuild conflict [1]. BR, Jani. [1] https://paste.debian.net/1272275/
On 2/27/23 13:37, Jani Nikula wrote: > On Mon, 27 Feb 2023, Dmitry Osipenko <dmitry.osipenko@collabora.com> wrote: >> On 2/17/23 16:41, Dmitry Osipenko wrote: >>> On 2/17/23 16:28, Thomas Zimmermann wrote: >>>> Hi, >>>> >>>> I looked through the series. Most of the patches should have an r-b or >>>> a-b at this point. I can't say much about patch 2 and had questions >>>> about others. >>>> >>>> Maybe you can already land patches 2, and 4 to 6? They look independent >>>> from the shrinker changes. You could also attempt to land the locking >>>> changes in patch 7. They need to get testing. I'll send you an a-b for >>>> the patch. >>> >>> Thank you, I'll apply the acked patches and then make v11 with the >>> remaining patches updated. >>> >>> Not sure if it will be possible to split patch 8, but I'll think on it >>> for v11. >>> >> >> Applied patches 1-2 to misc-fixes and patches 3-7 to misc-next, with the >> review comments addressed. > > Please resolve the drm-tip rebuild conflict [1]. > > BR, > Jani. > > > [1] https://paste.debian.net/1272275/ Don't see that conflict locally, perhaps somebody already fixed it?