2020-11-25 16:28:50

by Daniel Vetter

[permalink] [raw]
Subject: [PATCH] drm/ttm: don't set page->mapping

Random observation while trying to review Christian's patch series to
stop looking at struct page for dma-buf imports.

This was originally added in

commit 58aa6622d32af7d2c08d45085f44c54554a16ed7
Author: Thomas Hellstrom <[email protected]>
Date: Fri Jan 3 11:47:23 2014 +0100

drm/ttm: Correctly set page mapping and -index members

Needed for some vm operations; most notably unmap_mapping_range() with
even_cows = 0.

Signed-off-by: Thomas Hellstrom <[email protected]>
Reviewed-by: Brian Paul <[email protected]>

but we do not have a single caller of unmap_mapping_range with
even_cows == 0. And all the gem drivers don't do this, so another
small thing we could standardize between drm and ttm drivers.

Plus I don't really see a need for unamp_mapping_range where we don't
want to indiscriminately shoot down all ptes.

Cc: Thomas Hellstrom <[email protected]>
Cc: Brian Paul <[email protected]>
Signed-off-by: Daniel Vetter <[email protected]>
Cc: Christian Koenig <[email protected]>
Cc: Huang Rui <[email protected]>
---
drivers/gpu/drm/ttm/ttm_tt.c | 12 ------------
1 file changed, 12 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c
index da9eeffe0c6d..5b2eb6d58bb7 100644
--- a/drivers/gpu/drm/ttm/ttm_tt.c
+++ b/drivers/gpu/drm/ttm/ttm_tt.c
@@ -284,17 +284,6 @@ int ttm_tt_swapout(struct ttm_bo_device *bdev, struct ttm_tt *ttm)
return ret;
}

-static void ttm_tt_add_mapping(struct ttm_bo_device *bdev, struct ttm_tt *ttm)
-{
- pgoff_t i;
-
- if (ttm->page_flags & TTM_PAGE_FLAG_SG)
- return;
-
- for (i = 0; i < ttm->num_pages; ++i)
- ttm->pages[i]->mapping = bdev->dev_mapping;
-}
-
int ttm_tt_populate(struct ttm_bo_device *bdev,
struct ttm_tt *ttm, struct ttm_operation_ctx *ctx)
{
@@ -313,7 +302,6 @@ int ttm_tt_populate(struct ttm_bo_device *bdev,
if (ret)
return ret;

- ttm_tt_add_mapping(bdev, ttm);
ttm->page_flags |= TTM_PAGE_FLAG_PRIV_POPULATED;
if (unlikely(ttm->page_flags & TTM_PAGE_FLAG_SWAPPED)) {
ret = ttm_tt_swapin(ttm);
--
2.29.2


2020-11-25 16:30:31

by Daniel Vetter

[permalink] [raw]
Subject: Re: [PATCH] drm/ttm: don't set page->mapping

On Wed, Nov 25, 2020 at 5:25 PM Daniel Vetter <[email protected]> wrote:
>
> Random observation while trying to review Christian's patch series to
> stop looking at struct page for dma-buf imports.
>
> This was originally added in
>
> commit 58aa6622d32af7d2c08d45085f44c54554a16ed7
> Author: Thomas Hellstrom <[email protected]>
> Date: Fri Jan 3 11:47:23 2014 +0100
>
> drm/ttm: Correctly set page mapping and -index members
>
> Needed for some vm operations; most notably unmap_mapping_range() with
> even_cows = 0.
>
> Signed-off-by: Thomas Hellstrom <[email protected]>
> Reviewed-by: Brian Paul <[email protected]>
>
> but we do not have a single caller of unmap_mapping_range with
> even_cows == 0. And all the gem drivers don't do this, so another
> small thing we could standardize between drm and ttm drivers.
>
> Plus I don't really see a need for unamp_mapping_range where we don't
> want to indiscriminately shoot down all ptes.
>
> Cc: Thomas Hellstrom <[email protected]>
> Cc: Brian Paul <[email protected]>
> Signed-off-by: Daniel Vetter <[email protected]>
> Cc: Christian Koenig <[email protected]>
> Cc: Huang Rui <[email protected]>

Apologies again, this shouldn't have been included. But at least I
have an idea now why this patch somehow was included in the git
send-email. Lovely interface :-/
-Daniel

> ---
> drivers/gpu/drm/ttm/ttm_tt.c | 12 ------------
> 1 file changed, 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c
> index da9eeffe0c6d..5b2eb6d58bb7 100644
> --- a/drivers/gpu/drm/ttm/ttm_tt.c
> +++ b/drivers/gpu/drm/ttm/ttm_tt.c
> @@ -284,17 +284,6 @@ int ttm_tt_swapout(struct ttm_bo_device *bdev, struct ttm_tt *ttm)
> return ret;
> }
>
> -static void ttm_tt_add_mapping(struct ttm_bo_device *bdev, struct ttm_tt *ttm)
> -{
> - pgoff_t i;
> -
> - if (ttm->page_flags & TTM_PAGE_FLAG_SG)
> - return;
> -
> - for (i = 0; i < ttm->num_pages; ++i)
> - ttm->pages[i]->mapping = bdev->dev_mapping;
> -}
> -
> int ttm_tt_populate(struct ttm_bo_device *bdev,
> struct ttm_tt *ttm, struct ttm_operation_ctx *ctx)
> {
> @@ -313,7 +302,6 @@ int ttm_tt_populate(struct ttm_bo_device *bdev,
> if (ret)
> return ret;
>
> - ttm_tt_add_mapping(bdev, ttm);
> ttm->page_flags |= TTM_PAGE_FLAG_PRIV_POPULATED;
> if (unlikely(ttm->page_flags & TTM_PAGE_FLAG_SWAPPED)) {
> ret = ttm_tt_swapin(ttm);
> --
> 2.29.2
>


--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

2020-11-25 18:14:38

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH] drm/ttm: don't set page->mapping

On Wed, Nov 25, 2020 at 02:06:06PM -0400, Jason Gunthorpe wrote:
> It uses a empty 'cover-letter' commit and automatically transforms it
> into exactly the right stuff. Keeps track of everything you send in
> git, and there is a little tool to auto-run git range-diff to help
> build change logs..
>
> https://github.com/jgunthorpe/Kernel-Maintainer-Tools/blob/master/gj_tools/cmd_send_patches.py
>
> I've been occasionaly wondering if I should suggest Konstantin add a
> sending side to b4, maybe using some of those ideas..
>
> (careful if you run it, it does autosend without prompting)

The looks pretty fancy. Here is my trivial patchbomb.sh script

----------------------- snip -----------------------
#!/bin/sh

COVERLETTER=$1
PATCHES=$2

git send-email --annotate --to-cover --cc-cover $1 $2
----------------------- snip -----------------------

still needs the git basecommit..endcommit notation, but it fires
up the series for review.

2020-11-25 18:21:19

by Daniel Stone

[permalink] [raw]
Subject: Re: [PATCH] drm/ttm: don't set page->mapping

Hi,

On Wed, 25 Nov 2020 at 18:06, Jason Gunthorpe <[email protected]> wrote:
> On Wed, Nov 25, 2020 at 05:28:32PM +0100, Daniel Vetter wrote:
> > Apologies again, this shouldn't have been included. But at least I
> > have an idea now why this patch somehow was included in the git
> > send-email. Lovely interface :-/
>
> I wrote a bit of a script around this because git send-email just too
> hard to use
>
> The key workflow change I made was to have it prepare all the emails
> to send and open them in an editor for review - exactly as they would
> be sent to the lists.
>
> It uses a empty 'cover-letter' commit and automatically transforms it
> into exactly the right stuff. Keeps track of everything you send in
> git, and there is a little tool to auto-run git range-diff to help
> build change logs..

This sounds a fair bit like patman, which does something similar and
also lets you annotate commit messages with changelogs.

But of course, suggesting different methods of carving patches into
stone tablets to someone who's written their own, is even more of a
windmill tilt than rDMA. ;)

Cheers,
Daniel

2020-11-26 07:14:02

by Jason Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH] drm/ttm: don't set page->mapping

On Wed, Nov 25, 2020 at 05:28:32PM +0100, Daniel Vetter wrote:
> On Wed, Nov 25, 2020 at 5:25 PM Daniel Vetter <[email protected]> wrote:
> >
> > Random observation while trying to review Christian's patch series to
> > stop looking at struct page for dma-buf imports.
> >
> > This was originally added in
> >
> > commit 58aa6622d32af7d2c08d45085f44c54554a16ed7
> > Author: Thomas Hellstrom <[email protected]>
> > Date: Fri Jan 3 11:47:23 2014 +0100
> >
> > drm/ttm: Correctly set page mapping and -index members
> >
> > Needed for some vm operations; most notably unmap_mapping_range() with
> > even_cows = 0.
> >
> > Signed-off-by: Thomas Hellstrom <[email protected]>
> > Reviewed-by: Brian Paul <[email protected]>
> >
> > but we do not have a single caller of unmap_mapping_range with
> > even_cows == 0. And all the gem drivers don't do this, so another
> > small thing we could standardize between drm and ttm drivers.
> >
> > Plus I don't really see a need for unamp_mapping_range where we don't
> > want to indiscriminately shoot down all ptes.
> >
> > Cc: Thomas Hellstrom <[email protected]>
> > Cc: Brian Paul <[email protected]>
> > Signed-off-by: Daniel Vetter <[email protected]>
> > Cc: Christian Koenig <[email protected]>
> > Cc: Huang Rui <[email protected]>
>
> Apologies again, this shouldn't have been included. But at least I
> have an idea now why this patch somehow was included in the git
> send-email. Lovely interface :-/

I wrote a bit of a script around this because git send-email just too
hard to use

The key workflow change I made was to have it prepare all the emails
to send and open them in an editor for review - exactly as they would
be sent to the lists.

It uses a empty 'cover-letter' commit and automatically transforms it
into exactly the right stuff. Keeps track of everything you send in
git, and there is a little tool to auto-run git range-diff to help
build change logs..

https://github.com/jgunthorpe/Kernel-Maintainer-Tools/blob/master/gj_tools/cmd_send_patches.py

I've been occasionaly wondering if I should suggest Konstantin add a
sending side to b4, maybe using some of those ideas..

(careful if you run it, it does autosend without prompting)

Jason

2020-11-26 08:31:14

by Jason Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH] drm/ttm: don't set page->mapping

On Wed, Nov 25, 2020 at 06:11:29PM +0000, Christoph Hellwig wrote:
> On Wed, Nov 25, 2020 at 02:06:06PM -0400, Jason Gunthorpe wrote:
> > It uses a empty 'cover-letter' commit and automatically transforms it
> > into exactly the right stuff. Keeps track of everything you send in
> > git, and there is a little tool to auto-run git range-diff to help
> > build change logs..
> >
> > https://github.com/jgunthorpe/Kernel-Maintainer-Tools/blob/master/gj_tools/cmd_send_patches.py
> >
> > I've been occasionaly wondering if I should suggest Konstantin add a
> > sending side to b4, maybe using some of those ideas..
> >
> > (careful if you run it, it does autosend without prompting)
>
> The looks pretty fancy. Here is my trivial patchbomb.sh script
>
> #!/bin/sh
>
> COVERLETTER=$1
> PATCHES=$2
>
> git send-email --annotate --to-cover --cc-cover $1 $2
>
> still needs the git basecommit..endcommit notation, but it fires
> up the series for review.

annotate is OK, I used that for a long time..

My main gripe was it didn't setup the to/cc until after the annotate
editor closes.

Jason

2020-11-26 10:22:34

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH] drm/ttm: don't set page->mapping

On Wed, Nov 25, 2020 at 07:57:20PM -0400, Jason Gunthorpe wrote:
> annotate is OK, I used that for a long time..
>
> My main gripe was it didn't setup the to/cc until after the annotate
> editor closes.

I put the To/Cc into the cover letter text file.