* [PATCH] drm/msm: Ensure mmap offset is initialized
@ 2022-05-31 20:08 ` Rob Clark
0 siblings, 0 replies; 8+ messages in thread
From: Rob Clark @ 2022-05-31 20:08 UTC (permalink / raw)
To: dri-devel
Cc: freedreno, linux-arm-msm, Rob Clark, Rob Clark, Abhinav Kumar,
Dmitry Baryshkov, Sean Paul, David Airlie, Daniel Vetter,
Gerd Hoffmann, open list
From: Rob Clark <robdclark@chromium.org>
If a GEM object is allocated, and then exported as a dma-buf fd which is
mmap'd before or without the GEM buffer being directly mmap'd, the
vma_node could be unitialized. This leads to a situation where the CPU
mapping is not correctly torn down in drm_vma_node_unmap().
Fixes: e5516553999f ("drm: call drm_gem_object_funcs.mmap with fake offset")
Signed-off-by: Rob Clark <robdclark@chromium.org>
---
drivers/gpu/drm/msm/msm_drv.c | 2 +-
drivers/gpu/drm/msm/msm_drv.h | 1 +
drivers/gpu/drm/msm/msm_gem_prime.c | 15 +++++++++++++++
3 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 44485363f37a..14ab9a627d8b 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -964,7 +964,7 @@ static const struct drm_driver msm_driver = {
.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
.gem_prime_import_sg_table = msm_gem_prime_import_sg_table,
- .gem_prime_mmap = drm_gem_prime_mmap,
+ .gem_prime_mmap = msm_gem_prime_mmap,
#ifdef CONFIG_DEBUG_FS
.debugfs_init = msm_debugfs_init,
#endif
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index bb052071b16d..090b8074fec7 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -275,6 +275,7 @@ unsigned long msm_gem_shrinker_shrink(struct drm_device *dev, unsigned long nr_t
void msm_gem_shrinker_init(struct drm_device *dev);
void msm_gem_shrinker_cleanup(struct drm_device *dev);
+int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma);
struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj);
int msm_gem_prime_vmap(struct drm_gem_object *obj, struct iosys_map *map);
void msm_gem_prime_vunmap(struct drm_gem_object *obj, struct iosys_map *map);
diff --git a/drivers/gpu/drm/msm/msm_gem_prime.c b/drivers/gpu/drm/msm/msm_gem_prime.c
index 94ab705e9b8a..dcc8a573bc76 100644
--- a/drivers/gpu/drm/msm/msm_gem_prime.c
+++ b/drivers/gpu/drm/msm/msm_gem_prime.c
@@ -11,6 +11,21 @@
#include "msm_drv.h"
#include "msm_gem.h"
+int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
+{
+ int ret;
+
+ /* Ensure the mmap offset is initialized. We lazily initialize it,
+ * so if it has not been first mmap'd directly as a GEM object, the
+ * mmap offset will not be already initialized.
+ */
+ ret = drm_gem_create_mmap_offset(obj);
+ if (ret)
+ return ret;
+
+ return drm_gem_prime_mmap(obj, vma);
+}
+
struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj)
{
struct msm_gem_object *msm_obj = to_msm_bo(obj);
--
2.36.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] drm/msm: Ensure mmap offset is initialized
@ 2022-05-31 20:08 ` Rob Clark
0 siblings, 0 replies; 8+ messages in thread
From: Rob Clark @ 2022-05-31 20:08 UTC (permalink / raw)
To: dri-devel
Cc: Rob Clark, David Airlie, linux-arm-msm, Abhinav Kumar, open list,
Sean Paul, Gerd Hoffmann, Dmitry Baryshkov, freedreno
From: Rob Clark <robdclark@chromium.org>
If a GEM object is allocated, and then exported as a dma-buf fd which is
mmap'd before or without the GEM buffer being directly mmap'd, the
vma_node could be unitialized. This leads to a situation where the CPU
mapping is not correctly torn down in drm_vma_node_unmap().
Fixes: e5516553999f ("drm: call drm_gem_object_funcs.mmap with fake offset")
Signed-off-by: Rob Clark <robdclark@chromium.org>
---
drivers/gpu/drm/msm/msm_drv.c | 2 +-
drivers/gpu/drm/msm/msm_drv.h | 1 +
drivers/gpu/drm/msm/msm_gem_prime.c | 15 +++++++++++++++
3 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 44485363f37a..14ab9a627d8b 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -964,7 +964,7 @@ static const struct drm_driver msm_driver = {
.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
.gem_prime_import_sg_table = msm_gem_prime_import_sg_table,
- .gem_prime_mmap = drm_gem_prime_mmap,
+ .gem_prime_mmap = msm_gem_prime_mmap,
#ifdef CONFIG_DEBUG_FS
.debugfs_init = msm_debugfs_init,
#endif
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index bb052071b16d..090b8074fec7 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -275,6 +275,7 @@ unsigned long msm_gem_shrinker_shrink(struct drm_device *dev, unsigned long nr_t
void msm_gem_shrinker_init(struct drm_device *dev);
void msm_gem_shrinker_cleanup(struct drm_device *dev);
+int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma);
struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj);
int msm_gem_prime_vmap(struct drm_gem_object *obj, struct iosys_map *map);
void msm_gem_prime_vunmap(struct drm_gem_object *obj, struct iosys_map *map);
diff --git a/drivers/gpu/drm/msm/msm_gem_prime.c b/drivers/gpu/drm/msm/msm_gem_prime.c
index 94ab705e9b8a..dcc8a573bc76 100644
--- a/drivers/gpu/drm/msm/msm_gem_prime.c
+++ b/drivers/gpu/drm/msm/msm_gem_prime.c
@@ -11,6 +11,21 @@
#include "msm_drv.h"
#include "msm_gem.h"
+int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
+{
+ int ret;
+
+ /* Ensure the mmap offset is initialized. We lazily initialize it,
+ * so if it has not been first mmap'd directly as a GEM object, the
+ * mmap offset will not be already initialized.
+ */
+ ret = drm_gem_create_mmap_offset(obj);
+ if (ret)
+ return ret;
+
+ return drm_gem_prime_mmap(obj, vma);
+}
+
struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj)
{
struct msm_gem_object *msm_obj = to_msm_bo(obj);
--
2.36.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/msm: Ensure mmap offset is initialized
2022-05-31 20:08 ` Rob Clark
@ 2022-05-31 20:34 ` Dmitry Baryshkov
-1 siblings, 0 replies; 8+ messages in thread
From: Dmitry Baryshkov @ 2022-05-31 20:34 UTC (permalink / raw)
To: Rob Clark
Cc: dri-devel, freedreno, linux-arm-msm, Rob Clark, Abhinav Kumar,
Sean Paul, David Airlie, Daniel Vetter, Gerd Hoffmann, open list
On Tue, 31 May 2022 at 23:08, Rob Clark <robdclark@gmail.com> wrote:
>
> From: Rob Clark <robdclark@chromium.org>
>
> If a GEM object is allocated, and then exported as a dma-buf fd which is
> mmap'd before or without the GEM buffer being directly mmap'd, the
> vma_node could be unitialized. This leads to a situation where the CPU
> mapping is not correctly torn down in drm_vma_node_unmap().
>
> Fixes: e5516553999f ("drm: call drm_gem_object_funcs.mmap with fake offset")
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
> drivers/gpu/drm/msm/msm_drv.c | 2 +-
> drivers/gpu/drm/msm/msm_drv.h | 1 +
> drivers/gpu/drm/msm/msm_gem_prime.c | 15 +++++++++++++++
> 3 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 44485363f37a..14ab9a627d8b 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -964,7 +964,7 @@ static const struct drm_driver msm_driver = {
> .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> .gem_prime_import_sg_table = msm_gem_prime_import_sg_table,
> - .gem_prime_mmap = drm_gem_prime_mmap,
> + .gem_prime_mmap = msm_gem_prime_mmap,
> #ifdef CONFIG_DEBUG_FS
> .debugfs_init = msm_debugfs_init,
> #endif
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index bb052071b16d..090b8074fec7 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -275,6 +275,7 @@ unsigned long msm_gem_shrinker_shrink(struct drm_device *dev, unsigned long nr_t
> void msm_gem_shrinker_init(struct drm_device *dev);
> void msm_gem_shrinker_cleanup(struct drm_device *dev);
>
> +int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma);
> struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj);
> int msm_gem_prime_vmap(struct drm_gem_object *obj, struct iosys_map *map);
> void msm_gem_prime_vunmap(struct drm_gem_object *obj, struct iosys_map *map);
> diff --git a/drivers/gpu/drm/msm/msm_gem_prime.c b/drivers/gpu/drm/msm/msm_gem_prime.c
> index 94ab705e9b8a..dcc8a573bc76 100644
> --- a/drivers/gpu/drm/msm/msm_gem_prime.c
> +++ b/drivers/gpu/drm/msm/msm_gem_prime.c
> @@ -11,6 +11,21 @@
> #include "msm_drv.h"
> #include "msm_gem.h"
>
> +int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
> +{
> + int ret;
> +
> + /* Ensure the mmap offset is initialized. We lazily initialize it,
> + * so if it has not been first mmap'd directly as a GEM object, the
> + * mmap offset will not be already initialized.
> + */
> + ret = drm_gem_create_mmap_offset(obj);
> + if (ret)
> + return ret;
Wouldn't it be better to have this call directly in the
drm_gem_prime_mmap() ? This way all drivers can be lazy.
> +
> + return drm_gem_prime_mmap(obj, vma);
> +}
> +
> struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> --
> 2.36.1
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/msm: Ensure mmap offset is initialized
@ 2022-05-31 20:34 ` Dmitry Baryshkov
0 siblings, 0 replies; 8+ messages in thread
From: Dmitry Baryshkov @ 2022-05-31 20:34 UTC (permalink / raw)
To: Rob Clark
Cc: Rob Clark, freedreno, David Airlie, linux-arm-msm, Abhinav Kumar,
dri-devel, open list, Gerd Hoffmann, Sean Paul
On Tue, 31 May 2022 at 23:08, Rob Clark <robdclark@gmail.com> wrote:
>
> From: Rob Clark <robdclark@chromium.org>
>
> If a GEM object is allocated, and then exported as a dma-buf fd which is
> mmap'd before or without the GEM buffer being directly mmap'd, the
> vma_node could be unitialized. This leads to a situation where the CPU
> mapping is not correctly torn down in drm_vma_node_unmap().
>
> Fixes: e5516553999f ("drm: call drm_gem_object_funcs.mmap with fake offset")
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
> drivers/gpu/drm/msm/msm_drv.c | 2 +-
> drivers/gpu/drm/msm/msm_drv.h | 1 +
> drivers/gpu/drm/msm/msm_gem_prime.c | 15 +++++++++++++++
> 3 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 44485363f37a..14ab9a627d8b 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -964,7 +964,7 @@ static const struct drm_driver msm_driver = {
> .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> .gem_prime_import_sg_table = msm_gem_prime_import_sg_table,
> - .gem_prime_mmap = drm_gem_prime_mmap,
> + .gem_prime_mmap = msm_gem_prime_mmap,
> #ifdef CONFIG_DEBUG_FS
> .debugfs_init = msm_debugfs_init,
> #endif
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index bb052071b16d..090b8074fec7 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -275,6 +275,7 @@ unsigned long msm_gem_shrinker_shrink(struct drm_device *dev, unsigned long nr_t
> void msm_gem_shrinker_init(struct drm_device *dev);
> void msm_gem_shrinker_cleanup(struct drm_device *dev);
>
> +int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma);
> struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj);
> int msm_gem_prime_vmap(struct drm_gem_object *obj, struct iosys_map *map);
> void msm_gem_prime_vunmap(struct drm_gem_object *obj, struct iosys_map *map);
> diff --git a/drivers/gpu/drm/msm/msm_gem_prime.c b/drivers/gpu/drm/msm/msm_gem_prime.c
> index 94ab705e9b8a..dcc8a573bc76 100644
> --- a/drivers/gpu/drm/msm/msm_gem_prime.c
> +++ b/drivers/gpu/drm/msm/msm_gem_prime.c
> @@ -11,6 +11,21 @@
> #include "msm_drv.h"
> #include "msm_gem.h"
>
> +int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
> +{
> + int ret;
> +
> + /* Ensure the mmap offset is initialized. We lazily initialize it,
> + * so if it has not been first mmap'd directly as a GEM object, the
> + * mmap offset will not be already initialized.
> + */
> + ret = drm_gem_create_mmap_offset(obj);
> + if (ret)
> + return ret;
Wouldn't it be better to have this call directly in the
drm_gem_prime_mmap() ? This way all drivers can be lazy.
> +
> + return drm_gem_prime_mmap(obj, vma);
> +}
> +
> struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj)
> {
> struct msm_gem_object *msm_obj = to_msm_bo(obj);
> --
> 2.36.1
>
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/msm: Ensure mmap offset is initialized
2022-05-31 20:34 ` Dmitry Baryshkov
@ 2022-05-31 20:37 ` Rob Clark
-1 siblings, 0 replies; 8+ messages in thread
From: Rob Clark @ 2022-05-31 20:37 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: dri-devel, freedreno, linux-arm-msm, Rob Clark, Abhinav Kumar,
Sean Paul, David Airlie, Daniel Vetter, Gerd Hoffmann, open list
On Tue, May 31, 2022 at 1:34 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Tue, 31 May 2022 at 23:08, Rob Clark <robdclark@gmail.com> wrote:
> >
> > From: Rob Clark <robdclark@chromium.org>
> >
> > If a GEM object is allocated, and then exported as a dma-buf fd which is
> > mmap'd before or without the GEM buffer being directly mmap'd, the
> > vma_node could be unitialized. This leads to a situation where the CPU
> > mapping is not correctly torn down in drm_vma_node_unmap().
> >
> > Fixes: e5516553999f ("drm: call drm_gem_object_funcs.mmap with fake offset")
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> > drivers/gpu/drm/msm/msm_drv.c | 2 +-
> > drivers/gpu/drm/msm/msm_drv.h | 1 +
> > drivers/gpu/drm/msm/msm_gem_prime.c | 15 +++++++++++++++
> > 3 files changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > index 44485363f37a..14ab9a627d8b 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -964,7 +964,7 @@ static const struct drm_driver msm_driver = {
> > .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> > .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> > .gem_prime_import_sg_table = msm_gem_prime_import_sg_table,
> > - .gem_prime_mmap = drm_gem_prime_mmap,
> > + .gem_prime_mmap = msm_gem_prime_mmap,
> > #ifdef CONFIG_DEBUG_FS
> > .debugfs_init = msm_debugfs_init,
> > #endif
> > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> > index bb052071b16d..090b8074fec7 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.h
> > +++ b/drivers/gpu/drm/msm/msm_drv.h
> > @@ -275,6 +275,7 @@ unsigned long msm_gem_shrinker_shrink(struct drm_device *dev, unsigned long nr_t
> > void msm_gem_shrinker_init(struct drm_device *dev);
> > void msm_gem_shrinker_cleanup(struct drm_device *dev);
> >
> > +int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma);
> > struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj);
> > int msm_gem_prime_vmap(struct drm_gem_object *obj, struct iosys_map *map);
> > void msm_gem_prime_vunmap(struct drm_gem_object *obj, struct iosys_map *map);
> > diff --git a/drivers/gpu/drm/msm/msm_gem_prime.c b/drivers/gpu/drm/msm/msm_gem_prime.c
> > index 94ab705e9b8a..dcc8a573bc76 100644
> > --- a/drivers/gpu/drm/msm/msm_gem_prime.c
> > +++ b/drivers/gpu/drm/msm/msm_gem_prime.c
> > @@ -11,6 +11,21 @@
> > #include "msm_drv.h"
> > #include "msm_gem.h"
> >
> > +int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
> > +{
> > + int ret;
> > +
> > + /* Ensure the mmap offset is initialized. We lazily initialize it,
> > + * so if it has not been first mmap'd directly as a GEM object, the
> > + * mmap offset will not be already initialized.
> > + */
> > + ret = drm_gem_create_mmap_offset(obj);
> > + if (ret)
> > + return ret;
>
> Wouldn't it be better to have this call directly in the
> drm_gem_prime_mmap() ? This way all drivers can be lazy.
>
yes.. that was my first[1] proposal. But there are differences of
opinion, and in the mean time I want to get this particular issue
fixed ;-)
BR,
-R
[1] https://patchwork.freedesktop.org/patch/487597/
>
> > +
> > + return drm_gem_prime_mmap(obj, vma);
> > +}
> > +
> > struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj)
> > {
> > struct msm_gem_object *msm_obj = to_msm_bo(obj);
> > --
> > 2.36.1
> >
>
>
> --
> With best wishes
> Dmitry
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/msm: Ensure mmap offset is initialized
@ 2022-05-31 20:37 ` Rob Clark
0 siblings, 0 replies; 8+ messages in thread
From: Rob Clark @ 2022-05-31 20:37 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Rob Clark, freedreno, David Airlie, linux-arm-msm, Abhinav Kumar,
dri-devel, open list, Gerd Hoffmann, Sean Paul
On Tue, May 31, 2022 at 1:34 PM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Tue, 31 May 2022 at 23:08, Rob Clark <robdclark@gmail.com> wrote:
> >
> > From: Rob Clark <robdclark@chromium.org>
> >
> > If a GEM object is allocated, and then exported as a dma-buf fd which is
> > mmap'd before or without the GEM buffer being directly mmap'd, the
> > vma_node could be unitialized. This leads to a situation where the CPU
> > mapping is not correctly torn down in drm_vma_node_unmap().
> >
> > Fixes: e5516553999f ("drm: call drm_gem_object_funcs.mmap with fake offset")
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> > drivers/gpu/drm/msm/msm_drv.c | 2 +-
> > drivers/gpu/drm/msm/msm_drv.h | 1 +
> > drivers/gpu/drm/msm/msm_gem_prime.c | 15 +++++++++++++++
> > 3 files changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > index 44485363f37a..14ab9a627d8b 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -964,7 +964,7 @@ static const struct drm_driver msm_driver = {
> > .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> > .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> > .gem_prime_import_sg_table = msm_gem_prime_import_sg_table,
> > - .gem_prime_mmap = drm_gem_prime_mmap,
> > + .gem_prime_mmap = msm_gem_prime_mmap,
> > #ifdef CONFIG_DEBUG_FS
> > .debugfs_init = msm_debugfs_init,
> > #endif
> > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> > index bb052071b16d..090b8074fec7 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.h
> > +++ b/drivers/gpu/drm/msm/msm_drv.h
> > @@ -275,6 +275,7 @@ unsigned long msm_gem_shrinker_shrink(struct drm_device *dev, unsigned long nr_t
> > void msm_gem_shrinker_init(struct drm_device *dev);
> > void msm_gem_shrinker_cleanup(struct drm_device *dev);
> >
> > +int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma);
> > struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj);
> > int msm_gem_prime_vmap(struct drm_gem_object *obj, struct iosys_map *map);
> > void msm_gem_prime_vunmap(struct drm_gem_object *obj, struct iosys_map *map);
> > diff --git a/drivers/gpu/drm/msm/msm_gem_prime.c b/drivers/gpu/drm/msm/msm_gem_prime.c
> > index 94ab705e9b8a..dcc8a573bc76 100644
> > --- a/drivers/gpu/drm/msm/msm_gem_prime.c
> > +++ b/drivers/gpu/drm/msm/msm_gem_prime.c
> > @@ -11,6 +11,21 @@
> > #include "msm_drv.h"
> > #include "msm_gem.h"
> >
> > +int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
> > +{
> > + int ret;
> > +
> > + /* Ensure the mmap offset is initialized. We lazily initialize it,
> > + * so if it has not been first mmap'd directly as a GEM object, the
> > + * mmap offset will not be already initialized.
> > + */
> > + ret = drm_gem_create_mmap_offset(obj);
> > + if (ret)
> > + return ret;
>
> Wouldn't it be better to have this call directly in the
> drm_gem_prime_mmap() ? This way all drivers can be lazy.
>
yes.. that was my first[1] proposal. But there are differences of
opinion, and in the mean time I want to get this particular issue
fixed ;-)
BR,
-R
[1] https://patchwork.freedesktop.org/patch/487597/
>
> > +
> > + return drm_gem_prime_mmap(obj, vma);
> > +}
> > +
> > struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj)
> > {
> > struct msm_gem_object *msm_obj = to_msm_bo(obj);
> > --
> > 2.36.1
> >
>
>
> --
> With best wishes
> Dmitry
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/msm: Ensure mmap offset is initialized
2022-05-31 20:37 ` Rob Clark
@ 2022-05-31 21:11 ` Dmitry Baryshkov
-1 siblings, 0 replies; 8+ messages in thread
From: Dmitry Baryshkov @ 2022-05-31 21:11 UTC (permalink / raw)
To: Rob Clark
Cc: dri-devel, freedreno, linux-arm-msm, Rob Clark, Abhinav Kumar,
Sean Paul, David Airlie, Daniel Vetter, Gerd Hoffmann, open list
On Tue, 31 May 2022 at 23:37, Rob Clark <robdclark@gmail.com> wrote:
>
> On Tue, May 31, 2022 at 1:34 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Tue, 31 May 2022 at 23:08, Rob Clark <robdclark@gmail.com> wrote:
> > >
> > > From: Rob Clark <robdclark@chromium.org>
> > >
> > > If a GEM object is allocated, and then exported as a dma-buf fd which is
> > > mmap'd before or without the GEM buffer being directly mmap'd, the
> > > vma_node could be unitialized. This leads to a situation where the CPU
> > > mapping is not correctly torn down in drm_vma_node_unmap().
> > >
> > > Fixes: e5516553999f ("drm: call drm_gem_object_funcs.mmap with fake offset")
> > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > ---
> > > drivers/gpu/drm/msm/msm_drv.c | 2 +-
> > > drivers/gpu/drm/msm/msm_drv.h | 1 +
> > > drivers/gpu/drm/msm/msm_gem_prime.c | 15 +++++++++++++++
> > > 3 files changed, 17 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > > index 44485363f37a..14ab9a627d8b 100644
> > > --- a/drivers/gpu/drm/msm/msm_drv.c
> > > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > > @@ -964,7 +964,7 @@ static const struct drm_driver msm_driver = {
> > > .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> > > .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> > > .gem_prime_import_sg_table = msm_gem_prime_import_sg_table,
> > > - .gem_prime_mmap = drm_gem_prime_mmap,
> > > + .gem_prime_mmap = msm_gem_prime_mmap,
> > > #ifdef CONFIG_DEBUG_FS
> > > .debugfs_init = msm_debugfs_init,
> > > #endif
> > > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> > > index bb052071b16d..090b8074fec7 100644
> > > --- a/drivers/gpu/drm/msm/msm_drv.h
> > > +++ b/drivers/gpu/drm/msm/msm_drv.h
> > > @@ -275,6 +275,7 @@ unsigned long msm_gem_shrinker_shrink(struct drm_device *dev, unsigned long nr_t
> > > void msm_gem_shrinker_init(struct drm_device *dev);
> > > void msm_gem_shrinker_cleanup(struct drm_device *dev);
> > >
> > > +int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma);
> > > struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj);
> > > int msm_gem_prime_vmap(struct drm_gem_object *obj, struct iosys_map *map);
> > > void msm_gem_prime_vunmap(struct drm_gem_object *obj, struct iosys_map *map);
> > > diff --git a/drivers/gpu/drm/msm/msm_gem_prime.c b/drivers/gpu/drm/msm/msm_gem_prime.c
> > > index 94ab705e9b8a..dcc8a573bc76 100644
> > > --- a/drivers/gpu/drm/msm/msm_gem_prime.c
> > > +++ b/drivers/gpu/drm/msm/msm_gem_prime.c
> > > @@ -11,6 +11,21 @@
> > > #include "msm_drv.h"
> > > #include "msm_gem.h"
> > >
> > > +int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
> > > +{
> > > + int ret;
> > > +
> > > + /* Ensure the mmap offset is initialized. We lazily initialize it,
> > > + * so if it has not been first mmap'd directly as a GEM object, the
> > > + * mmap offset will not be already initialized.
> > > + */
> > > + ret = drm_gem_create_mmap_offset(obj);
> > > + if (ret)
> > > + return ret;
> >
> > Wouldn't it be better to have this call directly in the
> > drm_gem_prime_mmap() ? This way all drivers can be lazy.
> >
>
> yes.. that was my first[1] proposal. But there are differences of
> opinion, and in the mean time I want to get this particular issue
> fixed ;-)
Ack, excuse me, I probably skipped the thread.
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
> BR,
> -R
>
> [1] https://patchwork.freedesktop.org/patch/487597/
>
> >
> > > +
> > > + return drm_gem_prime_mmap(obj, vma);
> > > +}
> > > +
> > > struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj)
> > > {
> > > struct msm_gem_object *msm_obj = to_msm_bo(obj);
> > > --
> > > 2.36.1
> > >
> >
> >
> > --
> > With best wishes
> > Dmitry
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm/msm: Ensure mmap offset is initialized
@ 2022-05-31 21:11 ` Dmitry Baryshkov
0 siblings, 0 replies; 8+ messages in thread
From: Dmitry Baryshkov @ 2022-05-31 21:11 UTC (permalink / raw)
To: Rob Clark
Cc: Rob Clark, freedreno, David Airlie, linux-arm-msm, Abhinav Kumar,
dri-devel, open list, Gerd Hoffmann, Sean Paul
On Tue, 31 May 2022 at 23:37, Rob Clark <robdclark@gmail.com> wrote:
>
> On Tue, May 31, 2022 at 1:34 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Tue, 31 May 2022 at 23:08, Rob Clark <robdclark@gmail.com> wrote:
> > >
> > > From: Rob Clark <robdclark@chromium.org>
> > >
> > > If a GEM object is allocated, and then exported as a dma-buf fd which is
> > > mmap'd before or without the GEM buffer being directly mmap'd, the
> > > vma_node could be unitialized. This leads to a situation where the CPU
> > > mapping is not correctly torn down in drm_vma_node_unmap().
> > >
> > > Fixes: e5516553999f ("drm: call drm_gem_object_funcs.mmap with fake offset")
> > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > ---
> > > drivers/gpu/drm/msm/msm_drv.c | 2 +-
> > > drivers/gpu/drm/msm/msm_drv.h | 1 +
> > > drivers/gpu/drm/msm/msm_gem_prime.c | 15 +++++++++++++++
> > > 3 files changed, 17 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > > index 44485363f37a..14ab9a627d8b 100644
> > > --- a/drivers/gpu/drm/msm/msm_drv.c
> > > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > > @@ -964,7 +964,7 @@ static const struct drm_driver msm_driver = {
> > > .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> > > .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> > > .gem_prime_import_sg_table = msm_gem_prime_import_sg_table,
> > > - .gem_prime_mmap = drm_gem_prime_mmap,
> > > + .gem_prime_mmap = msm_gem_prime_mmap,
> > > #ifdef CONFIG_DEBUG_FS
> > > .debugfs_init = msm_debugfs_init,
> > > #endif
> > > diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> > > index bb052071b16d..090b8074fec7 100644
> > > --- a/drivers/gpu/drm/msm/msm_drv.h
> > > +++ b/drivers/gpu/drm/msm/msm_drv.h
> > > @@ -275,6 +275,7 @@ unsigned long msm_gem_shrinker_shrink(struct drm_device *dev, unsigned long nr_t
> > > void msm_gem_shrinker_init(struct drm_device *dev);
> > > void msm_gem_shrinker_cleanup(struct drm_device *dev);
> > >
> > > +int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma);
> > > struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj);
> > > int msm_gem_prime_vmap(struct drm_gem_object *obj, struct iosys_map *map);
> > > void msm_gem_prime_vunmap(struct drm_gem_object *obj, struct iosys_map *map);
> > > diff --git a/drivers/gpu/drm/msm/msm_gem_prime.c b/drivers/gpu/drm/msm/msm_gem_prime.c
> > > index 94ab705e9b8a..dcc8a573bc76 100644
> > > --- a/drivers/gpu/drm/msm/msm_gem_prime.c
> > > +++ b/drivers/gpu/drm/msm/msm_gem_prime.c
> > > @@ -11,6 +11,21 @@
> > > #include "msm_drv.h"
> > > #include "msm_gem.h"
> > >
> > > +int msm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
> > > +{
> > > + int ret;
> > > +
> > > + /* Ensure the mmap offset is initialized. We lazily initialize it,
> > > + * so if it has not been first mmap'd directly as a GEM object, the
> > > + * mmap offset will not be already initialized.
> > > + */
> > > + ret = drm_gem_create_mmap_offset(obj);
> > > + if (ret)
> > > + return ret;
> >
> > Wouldn't it be better to have this call directly in the
> > drm_gem_prime_mmap() ? This way all drivers can be lazy.
> >
>
> yes.. that was my first[1] proposal. But there are differences of
> opinion, and in the mean time I want to get this particular issue
> fixed ;-)
Ack, excuse me, I probably skipped the thread.
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
> BR,
> -R
>
> [1] https://patchwork.freedesktop.org/patch/487597/
>
> >
> > > +
> > > + return drm_gem_prime_mmap(obj, vma);
> > > +}
> > > +
> > > struct sg_table *msm_gem_prime_get_sg_table(struct drm_gem_object *obj)
> > > {
> > > struct msm_gem_object *msm_obj = to_msm_bo(obj);
> > > --
> > > 2.36.1
> > >
> >
> >
> > --
> > With best wishes
> > Dmitry
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-05-31 21:11 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-31 20:08 [PATCH] drm/msm: Ensure mmap offset is initialized Rob Clark
2022-05-31 20:08 ` Rob Clark
2022-05-31 20:34 ` Dmitry Baryshkov
2022-05-31 20:34 ` Dmitry Baryshkov
2022-05-31 20:37 ` Rob Clark
2022-05-31 20:37 ` Rob Clark
2022-05-31 21:11 ` Dmitry Baryshkov
2022-05-31 21:11 ` Dmitry Baryshkov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.