mbox series

[0/3] Various clean-up patches for GEM VRAM

Message ID 20190521110831.20200-1-tzimmermann@suse.de (mailing list archive)
Headers show
Series Various clean-up patches for GEM VRAM | expand

Message

Thomas Zimmermann May 21, 2019, 11:08 a.m. UTC
Replacing drm_gem_vram_push_to_system() moves policy from drivers back
to the memory manager. Now, unused BOs are only evicted when the space
is required.

The lock/unlock-renaming patch aligns the interface with other names
in DRM. No functional changes are done.

Finally, there's now a lockdep assert that ensures we don't call the
GEM VRAM _locked() functions with an unlocked BO.

Patches are against a recent drm-tip and tested on mgag200 and ast HW.

Thomas Zimmermann (3):
  drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin
  drm: Rename reserve/unreserve to lock/unlock in GEM VRAM helpers
  drm: Assert that BO is locked in drm_gem_vram_{pin,unpin}_locked()

 drivers/gpu/drm/ast/ast_fb.c             | 11 ++-
 drivers/gpu/drm/ast/ast_mode.c           | 26 ++++---
 drivers/gpu/drm/drm_gem_vram_helper.c    | 86 ++++++------------------
 drivers/gpu/drm/drm_vram_helper_common.c |  2 -
 drivers/gpu/drm/mgag200/mgag200_cursor.c | 40 +++++------
 drivers/gpu/drm/mgag200/mgag200_fb.c     | 11 ++-
 drivers/gpu/drm/mgag200/mgag200_mode.c   | 15 +++--
 include/drm/drm_gem_vram_helper.h        |  9 ++-
 8 files changed, 80 insertions(+), 120 deletions(-)

--
2.21.0

Comments

Daniel Vetter May 21, 2019, 12:40 p.m. UTC | #1
On Tue, May 21, 2019 at 01:08:28PM +0200, Thomas Zimmermann wrote:
> Replacing drm_gem_vram_push_to_system() moves policy from drivers back
> to the memory manager. Now, unused BOs are only evicted when the space
> is required.
> 
> The lock/unlock-renaming patch aligns the interface with other names
> in DRM. No functional changes are done.
> 
> Finally, there's now a lockdep assert that ensures we don't call the
> GEM VRAM _locked() functions with an unlocked BO.
> 
> Patches are against a recent drm-tip and tested on mgag200 and ast HW.
> 
> Thomas Zimmermann (3):
>   drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin
>   drm: Rename reserve/unreserve to lock/unlock in GEM VRAM helpers
>   drm: Assert that BO is locked in drm_gem_vram_{pin,unpin}_locked()

Awesome, thanks a lot for quickly working on this. On the series:

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

But definitely get someone with more knowledge of the details to check
this all again.

Aside: Do you plan to continue working on drm drivers, i.e. any need for
drm-misc commit rights?

Cheers, Daniel

> 
>  drivers/gpu/drm/ast/ast_fb.c             | 11 ++-
>  drivers/gpu/drm/ast/ast_mode.c           | 26 ++++---
>  drivers/gpu/drm/drm_gem_vram_helper.c    | 86 ++++++------------------
>  drivers/gpu/drm/drm_vram_helper_common.c |  2 -
>  drivers/gpu/drm/mgag200/mgag200_cursor.c | 40 +++++------
>  drivers/gpu/drm/mgag200/mgag200_fb.c     | 11 ++-
>  drivers/gpu/drm/mgag200/mgag200_mode.c   | 15 +++--
>  include/drm/drm_gem_vram_helper.h        |  9 ++-
>  8 files changed, 80 insertions(+), 120 deletions(-)
> 
> --
> 2.21.0
>
Thomas Zimmermann May 21, 2019, 2:25 p.m. UTC | #2
Hi

Am 21.05.19 um 14:40 schrieb Daniel Vetter:
> On Tue, May 21, 2019 at 01:08:28PM +0200, Thomas Zimmermann wrote:
>> Replacing drm_gem_vram_push_to_system() moves policy from drivers back
>> to the memory manager. Now, unused BOs are only evicted when the space
>> is required.
>>
>> The lock/unlock-renaming patch aligns the interface with other names
>> in DRM. No functional changes are done.
>>
>> Finally, there's now a lockdep assert that ensures we don't call the
>> GEM VRAM _locked() functions with an unlocked BO.
>>
>> Patches are against a recent drm-tip and tested on mgag200 and ast HW.
>>
>> Thomas Zimmermann (3):
>>   drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin
>>   drm: Rename reserve/unreserve to lock/unlock in GEM VRAM helpers
>>   drm: Assert that BO is locked in drm_gem_vram_{pin,unpin}_locked()
> 
> Awesome, thanks a lot for quickly working on this. On the series:
> 
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> But definitely get someone with more knowledge of the details to check
> this all again.
> 
> Aside: Do you plan to continue working on drm drivers,

Yes, that's my job at SUSE.

> i.e. any need for
> drm-misc commit rights?

Sure. Thank you for your trust. From what I could found online, I guess
[1] and [2] applies?

Best regards
Thomas

[1]
https://drm.pages.freedesktop.org/maintainer-tools/committer-drm-misc.html
[2] https://drm.pages.freedesktop.org/maintainer-tools/commit-access.html

> Cheers, Daniel
> 
>>
>>  drivers/gpu/drm/ast/ast_fb.c             | 11 ++-
>>  drivers/gpu/drm/ast/ast_mode.c           | 26 ++++---
>>  drivers/gpu/drm/drm_gem_vram_helper.c    | 86 ++++++------------------
>>  drivers/gpu/drm/drm_vram_helper_common.c |  2 -
>>  drivers/gpu/drm/mgag200/mgag200_cursor.c | 40 +++++------
>>  drivers/gpu/drm/mgag200/mgag200_fb.c     | 11 ++-
>>  drivers/gpu/drm/mgag200/mgag200_mode.c   | 15 +++--
>>  include/drm/drm_gem_vram_helper.h        |  9 ++-
>>  8 files changed, 80 insertions(+), 120 deletions(-)
>>
>> --
>> 2.21.0
>>
>
Daniel Vetter May 21, 2019, 2:29 p.m. UTC | #3
On Tue, May 21, 2019 at 4:26 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 21.05.19 um 14:40 schrieb Daniel Vetter:
> > On Tue, May 21, 2019 at 01:08:28PM +0200, Thomas Zimmermann wrote:
> >> Replacing drm_gem_vram_push_to_system() moves policy from drivers back
> >> to the memory manager. Now, unused BOs are only evicted when the space
> >> is required.
> >>
> >> The lock/unlock-renaming patch aligns the interface with other names
> >> in DRM. No functional changes are done.
> >>
> >> Finally, there's now a lockdep assert that ensures we don't call the
> >> GEM VRAM _locked() functions with an unlocked BO.
> >>
> >> Patches are against a recent drm-tip and tested on mgag200 and ast HW.
> >>
> >> Thomas Zimmermann (3):
> >>   drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin
> >>   drm: Rename reserve/unreserve to lock/unlock in GEM VRAM helpers
> >>   drm: Assert that BO is locked in drm_gem_vram_{pin,unpin}_locked()
> >
> > Awesome, thanks a lot for quickly working on this. On the series:
> >
> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> > But definitely get someone with more knowledge of the details to check
> > this all again.
> >
> > Aside: Do you plan to continue working on drm drivers,
>
> Yes, that's my job at SUSE.
>
> > i.e. any need for
> > drm-misc commit rights?
>
> Sure. Thank you for your trust. From what I could found online, I guess
> [1] and [2] applies?
>
> Best regards
> Thomas
>
> [1]
> https://drm.pages.freedesktop.org/maintainer-tools/committer-drm-misc.html
> [2] https://drm.pages.freedesktop.org/maintainer-tools/commit-access.html

The getting started page is pretty useful too:

https://drm.pages.freedesktop.org/maintainer-tools/getting-started.html

Wrt account you need a legacy ssh account from here:

https://www.freedesktop.org/wiki/AccountRequests/

Just highlighting that, it's all in the docs too, but at least myself
I glossed over the right link a few times :-)

Cheers, Daniel

>
> > Cheers, Daniel
> >
> >>
> >>  drivers/gpu/drm/ast/ast_fb.c             | 11 ++-
> >>  drivers/gpu/drm/ast/ast_mode.c           | 26 ++++---
> >>  drivers/gpu/drm/drm_gem_vram_helper.c    | 86 ++++++------------------
> >>  drivers/gpu/drm/drm_vram_helper_common.c |  2 -
> >>  drivers/gpu/drm/mgag200/mgag200_cursor.c | 40 +++++------
> >>  drivers/gpu/drm/mgag200/mgag200_fb.c     | 11 ++-
> >>  drivers/gpu/drm/mgag200/mgag200_mode.c   | 15 +++--
> >>  include/drm/drm_gem_vram_helper.h        |  9 ++-
> >>  8 files changed, 80 insertions(+), 120 deletions(-)
> >>
> >> --
> >> 2.21.0
> >>
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
> HRB 21284 (AG Nürnberg)
>
Gerd Hoffmann May 22, 2019, 10:49 a.m. UTC | #4
On Tue, May 21, 2019 at 02:40:22PM +0200, Daniel Vetter wrote:
> On Tue, May 21, 2019 at 01:08:28PM +0200, Thomas Zimmermann wrote:
> > Replacing drm_gem_vram_push_to_system() moves policy from drivers back
> > to the memory manager. Now, unused BOs are only evicted when the space
> > is required.
> > 
> > The lock/unlock-renaming patch aligns the interface with other names
> > in DRM. No functional changes are done.
> > 
> > Finally, there's now a lockdep assert that ensures we don't call the
> > GEM VRAM _locked() functions with an unlocked BO.
> > 
> > Patches are against a recent drm-tip and tested on mgag200 and ast HW.
> > 
> > Thomas Zimmermann (3):
> >   drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin
> >   drm: Rename reserve/unreserve to lock/unlock in GEM VRAM helpers
> >   drm: Assert that BO is locked in drm_gem_vram_{pin,unpin}_locked()
> 
> Awesome, thanks a lot for quickly working on this. On the series:
> 
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> But definitely get someone with more knowledge of the details to check
> this all again.

Done & pushed to drm-misc-next.

thanks,
  Gerd
Daniel Vetter May 29, 2019, 1:45 p.m. UTC | #5
On Tue, May 21, 2019 at 01:08:28PM +0200, Thomas Zimmermann wrote:
> Replacing drm_gem_vram_push_to_system() moves policy from drivers back
> to the memory manager. Now, unused BOs are only evicted when the space
> is required.
> 
> The lock/unlock-renaming patch aligns the interface with other names
> in DRM. No functional changes are done.
> 
> Finally, there's now a lockdep assert that ensures we don't call the
> GEM VRAM _locked() functions with an unlocked BO.
> 
> Patches are against a recent drm-tip and tested on mgag200 and ast HW.
> 
> Thomas Zimmermann (3):
>   drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin
>   drm: Rename reserve/unreserve to lock/unlock in GEM VRAM helpers
>   drm: Assert that BO is locked in drm_gem_vram_{pin,unpin}_locked()

$ make htmldocs

results in new warnings:

./drivers/gpu/drm/drm_gem_vram_helper.c:595: warning: cannot understand function prototype: 'const struct drm_vram_mm_funcs drm_gem_vram_mm_funcs = '
./drivers/gpu/drm/drm_gem_vram_helper.c:596: warning: cannot understand function prototype: 'const struct drm_vram_mm_funcs drm_gem_vram_mm_funcs = '

You might want to review the entire output and make sure all the links
work and everything looks pretty and reasonable.

Thanks, Daniel
> 
>  drivers/gpu/drm/ast/ast_fb.c             | 11 ++-
>  drivers/gpu/drm/ast/ast_mode.c           | 26 ++++---
>  drivers/gpu/drm/drm_gem_vram_helper.c    | 86 ++++++------------------
>  drivers/gpu/drm/drm_vram_helper_common.c |  2 -
>  drivers/gpu/drm/mgag200/mgag200_cursor.c | 40 +++++------
>  drivers/gpu/drm/mgag200/mgag200_fb.c     | 11 ++-
>  drivers/gpu/drm/mgag200/mgag200_mode.c   | 15 +++--
>  include/drm/drm_gem_vram_helper.h        |  9 ++-
>  8 files changed, 80 insertions(+), 120 deletions(-)
> 
> --
> 2.21.0
>