* [PATCH v2] Remove custom dumb_map_offset implementation in msm driver @ 2023-11-15 14:19 Dipam Turkar 2023-11-15 15:07 ` Dmitry Baryshkov 0 siblings, 1 reply; 7+ messages in thread From: Dipam Turkar @ 2023-11-15 14:19 UTC (permalink / raw) To: robdclark Cc: quic_abhinavk, dmitry.baryshkov, sean, marijn.suijten, airlied, daniel, linux-arm-msm, dri-devel, freedreno, linux-kernel, Dipam Turkar Make msm use drm_gem_create_map_offset() instead of its custom implementation for associating GEM object with a fake offset. Since, we already have this generic implementation, we don't need the custom implementation and it is better to standardize the code for GEM based drivers. This also removes the outdated locking leftovers. Signed-off-by: Dipam Turkar <dipamt1729@gmail.com> --- drivers/gpu/drm/msm/msm_drv.c | 2 +- drivers/gpu/drm/msm/msm_gem.c | 21 --------------------- drivers/gpu/drm/msm/msm_gem.h | 2 -- 3 files changed, 1 insertion(+), 24 deletions(-) Changes in v2: Modify commit message to include the absence of internal locking leftovers around allocating a fake offset in msm_gem_mmap_offset() in the generic implementation drm_gem_create_map_offset(). diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index a428951ee539..86a15992c717 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -1085,7 +1085,7 @@ static const struct drm_driver msm_driver = { .open = msm_open, .postclose = msm_postclose, .dumb_create = msm_gem_dumb_create, - .dumb_map_offset = msm_gem_dumb_map_offset, + .dumb_map_offset = drm_gem_dumb_map_offset, .gem_prime_import_sg_table = msm_gem_prime_import_sg_table, #ifdef CONFIG_DEBUG_FS .debugfs_init = msm_debugfs_init, diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c index db1e748daa75..489694ef79cb 100644 --- a/drivers/gpu/drm/msm/msm_gem.c +++ b/drivers/gpu/drm/msm/msm_gem.c @@ -671,27 +671,6 @@ int msm_gem_dumb_create(struct drm_file *file, struct drm_device *dev, MSM_BO_SCANOUT | MSM_BO_WC, &args->handle, "dumb"); } -int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, - uint32_t handle, uint64_t *offset) -{ - struct drm_gem_object *obj; - int ret = 0; - - /* GEM does all our handle to object mapping */ - obj = drm_gem_object_lookup(file, handle); - if (obj == NULL) { - ret = -ENOENT; - goto fail; - } - - *offset = msm_gem_mmap_offset(obj); - - drm_gem_object_put(obj); - -fail: - return ret; -} - static void *get_vaddr(struct drm_gem_object *obj, unsigned madv) { struct msm_gem_object *msm_obj = to_msm_bo(obj); diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h index 8ddef5443140..dc74a0ef865d 100644 --- a/drivers/gpu/drm/msm/msm_gem.h +++ b/drivers/gpu/drm/msm/msm_gem.h @@ -139,8 +139,6 @@ struct page **msm_gem_pin_pages(struct drm_gem_object *obj); void msm_gem_unpin_pages(struct drm_gem_object *obj); int msm_gem_dumb_create(struct drm_file *file, struct drm_device *dev, struct drm_mode_create_dumb *args); -int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, - uint32_t handle, uint64_t *offset); void *msm_gem_get_vaddr_locked(struct drm_gem_object *obj); void *msm_gem_get_vaddr(struct drm_gem_object *obj); void *msm_gem_get_vaddr_active(struct drm_gem_object *obj); -- 2.34.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] Remove custom dumb_map_offset implementation in msm driver 2023-11-15 14:19 [PATCH v2] Remove custom dumb_map_offset implementation in msm driver Dipam Turkar @ 2023-11-15 15:07 ` Dmitry Baryshkov [not found] ` <CALHmwsoC5h7_w9OzpUS_-xM6x5WF5V-vFExLEf4y99b2eCcqGQ@mail.gmail.com> 0 siblings, 1 reply; 7+ messages in thread From: Dmitry Baryshkov @ 2023-11-15 15:07 UTC (permalink / raw) To: Dipam Turkar Cc: robdclark, quic_abhinavk, sean, marijn.suijten, airlied, daniel, linux-arm-msm, dri-devel, freedreno, linux-kernel On Wed, 15 Nov 2023 at 16:30, Dipam Turkar <dipamt1729@gmail.com> wrote: > > Make msm use drm_gem_create_map_offset() instead of its custom > implementation for associating GEM object with a fake offset. Since, > we already have this generic implementation, we don't need the custom > implementation and it is better to standardize the code for GEM based > drivers. This also removes the outdated locking leftovers. Why are they outdated? > > Signed-off-by: Dipam Turkar <dipamt1729@gmail.com> > --- > drivers/gpu/drm/msm/msm_drv.c | 2 +- > drivers/gpu/drm/msm/msm_gem.c | 21 --------------------- > drivers/gpu/drm/msm/msm_gem.h | 2 -- > 3 files changed, 1 insertion(+), 24 deletions(-) > > Changes in v2: > Modify commit message to include the absence of internal locking leftovers > around allocating a fake offset in msm_gem_mmap_offset() in the generic > implementation drm_gem_create_map_offset(). > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > index a428951ee539..86a15992c717 100644 > --- a/drivers/gpu/drm/msm/msm_drv.c > +++ b/drivers/gpu/drm/msm/msm_drv.c > @@ -1085,7 +1085,7 @@ static const struct drm_driver msm_driver = { > .open = msm_open, > .postclose = msm_postclose, > .dumb_create = msm_gem_dumb_create, > - .dumb_map_offset = msm_gem_dumb_map_offset, > + .dumb_map_offset = drm_gem_dumb_map_offset, > .gem_prime_import_sg_table = msm_gem_prime_import_sg_table, > #ifdef CONFIG_DEBUG_FS > .debugfs_init = msm_debugfs_init, > diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c > index db1e748daa75..489694ef79cb 100644 > --- a/drivers/gpu/drm/msm/msm_gem.c > +++ b/drivers/gpu/drm/msm/msm_gem.c > @@ -671,27 +671,6 @@ int msm_gem_dumb_create(struct drm_file *file, struct drm_device *dev, > MSM_BO_SCANOUT | MSM_BO_WC, &args->handle, "dumb"); > } > > -int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, > - uint32_t handle, uint64_t *offset) > -{ > - struct drm_gem_object *obj; > - int ret = 0; > - > - /* GEM does all our handle to object mapping */ > - obj = drm_gem_object_lookup(file, handle); > - if (obj == NULL) { > - ret = -ENOENT; > - goto fail; > - } > - > - *offset = msm_gem_mmap_offset(obj); > - > - drm_gem_object_put(obj); > - > -fail: > - return ret; > -} > - > static void *get_vaddr(struct drm_gem_object *obj, unsigned madv) > { > struct msm_gem_object *msm_obj = to_msm_bo(obj); > diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h > index 8ddef5443140..dc74a0ef865d 100644 > --- a/drivers/gpu/drm/msm/msm_gem.h > +++ b/drivers/gpu/drm/msm/msm_gem.h > @@ -139,8 +139,6 @@ struct page **msm_gem_pin_pages(struct drm_gem_object *obj); > void msm_gem_unpin_pages(struct drm_gem_object *obj); > int msm_gem_dumb_create(struct drm_file *file, struct drm_device *dev, > struct drm_mode_create_dumb *args); > -int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, > - uint32_t handle, uint64_t *offset); > void *msm_gem_get_vaddr_locked(struct drm_gem_object *obj); > void *msm_gem_get_vaddr(struct drm_gem_object *obj); > void *msm_gem_get_vaddr_active(struct drm_gem_object *obj); > -- > 2.34.1 > -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 7+ messages in thread
[parent not found: <CALHmwsoC5h7_w9OzpUS_-xM6x5WF5V-vFExLEf4y99b2eCcqGQ@mail.gmail.com>]
* Re: [PATCH v2] Remove custom dumb_map_offset implementation in msm driver [not found] ` <CALHmwsoC5h7_w9OzpUS_-xM6x5WF5V-vFExLEf4y99b2eCcqGQ@mail.gmail.com> @ 2023-11-15 19:33 ` Dmitry Baryshkov 2023-11-21 2:26 ` Rob Clark 0 siblings, 1 reply; 7+ messages in thread From: Dmitry Baryshkov @ 2023-11-15 19:33 UTC (permalink / raw) To: Dipam Turkar Cc: robdclark, quic_abhinavk, sean, marijn.suijten, airlied, daniel, linux-arm-msm, dri-devel, freedreno, linux-kernel On Wed, 15 Nov 2023 at 20:46, Dipam Turkar <dipamt1729@gmail.com> wrote: > > They are not outdated, my bad. I went through the locks' code and saw that they have been updated. But they are probably not necessary here as most of the drivers do not use any form of locking in their implementations. The generic implementations drm_gem_dumb_map_offset() and drm_gem_ttm_dumb_map_offset() do not have any locking mechanisms either. Excuse me, but this doesn't sound right to me. There are different drivers with different implementations. So either we'd need a good explanation of why it is not necessary, or this patch is NAKed. > > Thanks and regards > Dipam Turkar > > On Wed, Nov 15, 2023 at 8:37 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: >> >> On Wed, 15 Nov 2023 at 16:30, Dipam Turkar <dipamt1729@gmail.com> wrote: >> > >> > Make msm use drm_gem_create_map_offset() instead of its custom >> > implementation for associating GEM object with a fake offset. Since, >> > we already have this generic implementation, we don't need the custom >> > implementation and it is better to standardize the code for GEM based >> > drivers. This also removes the outdated locking leftovers. >> >> Why are they outdated? >> >> > >> > Signed-off-by: Dipam Turkar <dipamt1729@gmail.com> >> > --- >> > drivers/gpu/drm/msm/msm_drv.c | 2 +- >> > drivers/gpu/drm/msm/msm_gem.c | 21 --------------------- >> > drivers/gpu/drm/msm/msm_gem.h | 2 -- >> > 3 files changed, 1 insertion(+), 24 deletions(-) >> > >> > Changes in v2: >> > Modify commit message to include the absence of internal locking leftovers >> > around allocating a fake offset in msm_gem_mmap_offset() in the generic >> > implementation drm_gem_create_map_offset(). >> > >> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c >> > index a428951ee539..86a15992c717 100644 >> > --- a/drivers/gpu/drm/msm/msm_drv.c >> > +++ b/drivers/gpu/drm/msm/msm_drv.c >> > @@ -1085,7 +1085,7 @@ static const struct drm_driver msm_driver = { >> > .open = msm_open, >> > .postclose = msm_postclose, >> > .dumb_create = msm_gem_dumb_create, >> > - .dumb_map_offset = msm_gem_dumb_map_offset, >> > + .dumb_map_offset = drm_gem_dumb_map_offset, >> > .gem_prime_import_sg_table = msm_gem_prime_import_sg_table, >> > #ifdef CONFIG_DEBUG_FS >> > .debugfs_init = msm_debugfs_init, >> > diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c >> > index db1e748daa75..489694ef79cb 100644 >> > --- a/drivers/gpu/drm/msm/msm_gem.c >> > +++ b/drivers/gpu/drm/msm/msm_gem.c >> > @@ -671,27 +671,6 @@ int msm_gem_dumb_create(struct drm_file *file, struct drm_device *dev, >> > MSM_BO_SCANOUT | MSM_BO_WC, &args->handle, "dumb"); >> > } >> > >> > -int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, >> > - uint32_t handle, uint64_t *offset) >> > -{ >> > - struct drm_gem_object *obj; >> > - int ret = 0; >> > - >> > - /* GEM does all our handle to object mapping */ >> > - obj = drm_gem_object_lookup(file, handle); >> > - if (obj == NULL) { >> > - ret = -ENOENT; >> > - goto fail; >> > - } >> > - >> > - *offset = msm_gem_mmap_offset(obj); >> > - >> > - drm_gem_object_put(obj); >> > - >> > -fail: >> > - return ret; >> > -} >> > - >> > static void *get_vaddr(struct drm_gem_object *obj, unsigned madv) >> > { >> > struct msm_gem_object *msm_obj = to_msm_bo(obj); >> > diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h >> > index 8ddef5443140..dc74a0ef865d 100644 >> > --- a/drivers/gpu/drm/msm/msm_gem.h >> > +++ b/drivers/gpu/drm/msm/msm_gem.h >> > @@ -139,8 +139,6 @@ struct page **msm_gem_pin_pages(struct drm_gem_object *obj); >> > void msm_gem_unpin_pages(struct drm_gem_object *obj); >> > int msm_gem_dumb_create(struct drm_file *file, struct drm_device *dev, >> > struct drm_mode_create_dumb *args); >> > -int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, >> > - uint32_t handle, uint64_t *offset); >> > void *msm_gem_get_vaddr_locked(struct drm_gem_object *obj); >> > void *msm_gem_get_vaddr(struct drm_gem_object *obj); >> > void *msm_gem_get_vaddr_active(struct drm_gem_object *obj); >> > -- >> > 2.34.1 >> > >> >> >> -- >> With best wishes >> Dmitry -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] Remove custom dumb_map_offset implementation in msm driver 2023-11-15 19:33 ` Dmitry Baryshkov @ 2023-11-21 2:26 ` Rob Clark 2023-11-21 13:14 ` Dmitry Baryshkov 0 siblings, 1 reply; 7+ messages in thread From: Rob Clark @ 2023-11-21 2:26 UTC (permalink / raw) To: Dmitry Baryshkov Cc: Dipam Turkar, quic_abhinavk, sean, marijn.suijten, airlied, daniel, linux-arm-msm, dri-devel, freedreno, linux-kernel On Wed, Nov 15, 2023 at 11:33 AM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Wed, 15 Nov 2023 at 20:46, Dipam Turkar <dipamt1729@gmail.com> wrote: > > > > They are not outdated, my bad. I went through the locks' code and saw that they have been updated. But they are probably not necessary here as most of the drivers do not use any form of locking in their implementations. The generic implementations drm_gem_dumb_map_offset() and drm_gem_ttm_dumb_map_offset() do not have any locking mechanisms either. > > Excuse me, but this doesn't sound right to me. There are different > drivers with different implementations. So either we'd need a good > explanation of why it is not necessary, or this patch is NAKed. Digging a bit thru history, it looks like commit 0de23977cfeb ("drm/gem: convert to new unified vma manager") made external locking unnecessary, since the vma mgr already had it's own internal locking. BR, -R > > > > Thanks and regards > > Dipam Turkar > > > > On Wed, Nov 15, 2023 at 8:37 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > >> > >> On Wed, 15 Nov 2023 at 16:30, Dipam Turkar <dipamt1729@gmail.com> wrote: > >> > > >> > Make msm use drm_gem_create_map_offset() instead of its custom > >> > implementation for associating GEM object with a fake offset. Since, > >> > we already have this generic implementation, we don't need the custom > >> > implementation and it is better to standardize the code for GEM based > >> > drivers. This also removes the outdated locking leftovers. > >> > >> Why are they outdated? > >> > >> > > >> > Signed-off-by: Dipam Turkar <dipamt1729@gmail.com> > >> > --- > >> > drivers/gpu/drm/msm/msm_drv.c | 2 +- > >> > drivers/gpu/drm/msm/msm_gem.c | 21 --------------------- > >> > drivers/gpu/drm/msm/msm_gem.h | 2 -- > >> > 3 files changed, 1 insertion(+), 24 deletions(-) > >> > > >> > Changes in v2: > >> > Modify commit message to include the absence of internal locking leftovers > >> > around allocating a fake offset in msm_gem_mmap_offset() in the generic > >> > implementation drm_gem_create_map_offset(). > >> > > >> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > >> > index a428951ee539..86a15992c717 100644 > >> > --- a/drivers/gpu/drm/msm/msm_drv.c > >> > +++ b/drivers/gpu/drm/msm/msm_drv.c > >> > @@ -1085,7 +1085,7 @@ static const struct drm_driver msm_driver = { > >> > .open = msm_open, > >> > .postclose = msm_postclose, > >> > .dumb_create = msm_gem_dumb_create, > >> > - .dumb_map_offset = msm_gem_dumb_map_offset, > >> > + .dumb_map_offset = drm_gem_dumb_map_offset, > >> > .gem_prime_import_sg_table = msm_gem_prime_import_sg_table, > >> > #ifdef CONFIG_DEBUG_FS > >> > .debugfs_init = msm_debugfs_init, > >> > diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c > >> > index db1e748daa75..489694ef79cb 100644 > >> > --- a/drivers/gpu/drm/msm/msm_gem.c > >> > +++ b/drivers/gpu/drm/msm/msm_gem.c > >> > @@ -671,27 +671,6 @@ int msm_gem_dumb_create(struct drm_file *file, struct drm_device *dev, > >> > MSM_BO_SCANOUT | MSM_BO_WC, &args->handle, "dumb"); > >> > } > >> > > >> > -int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, > >> > - uint32_t handle, uint64_t *offset) > >> > -{ > >> > - struct drm_gem_object *obj; > >> > - int ret = 0; > >> > - > >> > - /* GEM does all our handle to object mapping */ > >> > - obj = drm_gem_object_lookup(file, handle); > >> > - if (obj == NULL) { > >> > - ret = -ENOENT; > >> > - goto fail; > >> > - } > >> > - > >> > - *offset = msm_gem_mmap_offset(obj); > >> > - > >> > - drm_gem_object_put(obj); > >> > - > >> > -fail: > >> > - return ret; > >> > -} > >> > - > >> > static void *get_vaddr(struct drm_gem_object *obj, unsigned madv) > >> > { > >> > struct msm_gem_object *msm_obj = to_msm_bo(obj); > >> > diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h > >> > index 8ddef5443140..dc74a0ef865d 100644 > >> > --- a/drivers/gpu/drm/msm/msm_gem.h > >> > +++ b/drivers/gpu/drm/msm/msm_gem.h > >> > @@ -139,8 +139,6 @@ struct page **msm_gem_pin_pages(struct drm_gem_object *obj); > >> > void msm_gem_unpin_pages(struct drm_gem_object *obj); > >> > int msm_gem_dumb_create(struct drm_file *file, struct drm_device *dev, > >> > struct drm_mode_create_dumb *args); > >> > -int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, > >> > - uint32_t handle, uint64_t *offset); > >> > void *msm_gem_get_vaddr_locked(struct drm_gem_object *obj); > >> > void *msm_gem_get_vaddr(struct drm_gem_object *obj); > >> > void *msm_gem_get_vaddr_active(struct drm_gem_object *obj); > >> > -- > >> > 2.34.1 > >> > > >> > >> > >> -- > >> With best wishes > >> Dmitry > > > > -- > With best wishes > Dmitry ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] Remove custom dumb_map_offset implementation in msm driver 2023-11-21 2:26 ` Rob Clark @ 2023-11-21 13:14 ` Dmitry Baryshkov 2023-11-27 20:52 ` Rob Clark 0 siblings, 1 reply; 7+ messages in thread From: Dmitry Baryshkov @ 2023-11-21 13:14 UTC (permalink / raw) To: Rob Clark Cc: Dipam Turkar, quic_abhinavk, sean, marijn.suijten, airlied, daniel, linux-arm-msm, dri-devel, freedreno, linux-kernel On Tue, 21 Nov 2023 at 04:26, Rob Clark <robdclark@gmail.com> wrote: > > On Wed, Nov 15, 2023 at 11:33 AM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On Wed, 15 Nov 2023 at 20:46, Dipam Turkar <dipamt1729@gmail.com> wrote: > > > > > > They are not outdated, my bad. I went through the locks' code and saw that they have been updated. But they are probably not necessary here as most of the drivers do not use any form of locking in their implementations. The generic implementations drm_gem_dumb_map_offset() and drm_gem_ttm_dumb_map_offset() do not have any locking mechanisms either. > > > > Excuse me, but this doesn't sound right to me. There are different > > drivers with different implementations. So either we'd need a good > > explanation of why it is not necessary, or this patch is NAKed. > > Digging a bit thru history, it looks like commit 0de23977cfeb > ("drm/gem: convert to new unified vma manager") made external locking > unnecessary, since the vma mgr already had it's own internal locking. So, should we drop our own locking system? > > BR, > -R > > > > > > > Thanks and regards > > > Dipam Turkar > > > > > > On Wed, Nov 15, 2023 at 8:37 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > >> > > >> On Wed, 15 Nov 2023 at 16:30, Dipam Turkar <dipamt1729@gmail.com> wrote: > > >> > > > >> > Make msm use drm_gem_create_map_offset() instead of its custom > > >> > implementation for associating GEM object with a fake offset. Since, > > >> > we already have this generic implementation, we don't need the custom > > >> > implementation and it is better to standardize the code for GEM based > > >> > drivers. This also removes the outdated locking leftovers. > > >> > > >> Why are they outdated? > > >> > > >> > > > >> > Signed-off-by: Dipam Turkar <dipamt1729@gmail.com> > > >> > --- > > >> > drivers/gpu/drm/msm/msm_drv.c | 2 +- > > >> > drivers/gpu/drm/msm/msm_gem.c | 21 --------------------- > > >> > drivers/gpu/drm/msm/msm_gem.h | 2 -- > > >> > 3 files changed, 1 insertion(+), 24 deletions(-) > > >> > > > >> > Changes in v2: > > >> > Modify commit message to include the absence of internal locking leftovers > > >> > around allocating a fake offset in msm_gem_mmap_offset() in the generic > > >> > implementation drm_gem_create_map_offset(). > > >> > > > >> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > > >> > index a428951ee539..86a15992c717 100644 > > >> > --- a/drivers/gpu/drm/msm/msm_drv.c > > >> > +++ b/drivers/gpu/drm/msm/msm_drv.c > > >> > @@ -1085,7 +1085,7 @@ static const struct drm_driver msm_driver = { > > >> > .open = msm_open, > > >> > .postclose = msm_postclose, > > >> > .dumb_create = msm_gem_dumb_create, > > >> > - .dumb_map_offset = msm_gem_dumb_map_offset, > > >> > + .dumb_map_offset = drm_gem_dumb_map_offset, > > >> > .gem_prime_import_sg_table = msm_gem_prime_import_sg_table, > > >> > #ifdef CONFIG_DEBUG_FS > > >> > .debugfs_init = msm_debugfs_init, > > >> > diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c > > >> > index db1e748daa75..489694ef79cb 100644 > > >> > --- a/drivers/gpu/drm/msm/msm_gem.c > > >> > +++ b/drivers/gpu/drm/msm/msm_gem.c > > >> > @@ -671,27 +671,6 @@ int msm_gem_dumb_create(struct drm_file *file, struct drm_device *dev, > > >> > MSM_BO_SCANOUT | MSM_BO_WC, &args->handle, "dumb"); > > >> > } > > >> > > > >> > -int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, > > >> > - uint32_t handle, uint64_t *offset) > > >> > -{ > > >> > - struct drm_gem_object *obj; > > >> > - int ret = 0; > > >> > - > > >> > - /* GEM does all our handle to object mapping */ > > >> > - obj = drm_gem_object_lookup(file, handle); > > >> > - if (obj == NULL) { > > >> > - ret = -ENOENT; > > >> > - goto fail; > > >> > - } > > >> > - > > >> > - *offset = msm_gem_mmap_offset(obj); > > >> > - > > >> > - drm_gem_object_put(obj); > > >> > - > > >> > -fail: > > >> > - return ret; > > >> > -} > > >> > - > > >> > static void *get_vaddr(struct drm_gem_object *obj, unsigned madv) > > >> > { > > >> > struct msm_gem_object *msm_obj = to_msm_bo(obj); > > >> > diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h > > >> > index 8ddef5443140..dc74a0ef865d 100644 > > >> > --- a/drivers/gpu/drm/msm/msm_gem.h > > >> > +++ b/drivers/gpu/drm/msm/msm_gem.h > > >> > @@ -139,8 +139,6 @@ struct page **msm_gem_pin_pages(struct drm_gem_object *obj); > > >> > void msm_gem_unpin_pages(struct drm_gem_object *obj); > > >> > int msm_gem_dumb_create(struct drm_file *file, struct drm_device *dev, > > >> > struct drm_mode_create_dumb *args); > > >> > -int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, > > >> > - uint32_t handle, uint64_t *offset); > > >> > void *msm_gem_get_vaddr_locked(struct drm_gem_object *obj); > > >> > void *msm_gem_get_vaddr(struct drm_gem_object *obj); > > >> > void *msm_gem_get_vaddr_active(struct drm_gem_object *obj); > > >> > -- > > >> > 2.34.1 > > >> > > > >> > > >> > > >> -- > > >> With best wishes > > >> Dmitry > > > > > > > > -- > > With best wishes > > Dmitry -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] Remove custom dumb_map_offset implementation in msm driver 2023-11-21 13:14 ` Dmitry Baryshkov @ 2023-11-27 20:52 ` Rob Clark 2023-11-27 21:06 ` Dmitry Baryshkov 0 siblings, 1 reply; 7+ messages in thread From: Rob Clark @ 2023-11-27 20:52 UTC (permalink / raw) To: Dmitry Baryshkov Cc: Dipam Turkar, quic_abhinavk, sean, marijn.suijten, airlied, daniel, linux-arm-msm, dri-devel, freedreno, linux-kernel On Tue, Nov 21, 2023 at 5:14 AM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Tue, 21 Nov 2023 at 04:26, Rob Clark <robdclark@gmail.com> wrote: > > > > On Wed, Nov 15, 2023 at 11:33 AM Dmitry Baryshkov > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > On Wed, 15 Nov 2023 at 20:46, Dipam Turkar <dipamt1729@gmail.com> wrote: > > > > > > > > They are not outdated, my bad. I went through the locks' code and saw that they have been updated. But they are probably not necessary here as most of the drivers do not use any form of locking in their implementations. The generic implementations drm_gem_dumb_map_offset() and drm_gem_ttm_dumb_map_offset() do not have any locking mechanisms either. > > > > > > Excuse me, but this doesn't sound right to me. There are different > > > drivers with different implementations. So either we'd need a good > > > explanation of why it is not necessary, or this patch is NAKed. > > > > Digging a bit thru history, it looks like commit 0de23977cfeb > > ("drm/gem: convert to new unified vma manager") made external locking > > unnecessary, since the vma mgr already had it's own internal locking. > > So, should we drop our own locking system? specifically for _just_ vma_offset_manager/vma_node, we could. But I think that only amounts to mmap_offset(). BR, -R > > > > BR, > > -R > > > > > > > > > > Thanks and regards > > > > Dipam Turkar > > > > > > > > On Wed, Nov 15, 2023 at 8:37 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > > >> > > > >> On Wed, 15 Nov 2023 at 16:30, Dipam Turkar <dipamt1729@gmail.com> wrote: > > > >> > > > > >> > Make msm use drm_gem_create_map_offset() instead of its custom > > > >> > implementation for associating GEM object with a fake offset. Since, > > > >> > we already have this generic implementation, we don't need the custom > > > >> > implementation and it is better to standardize the code for GEM based > > > >> > drivers. This also removes the outdated locking leftovers. > > > >> > > > >> Why are they outdated? > > > >> > > > >> > > > > >> > Signed-off-by: Dipam Turkar <dipamt1729@gmail.com> > > > >> > --- > > > >> > drivers/gpu/drm/msm/msm_drv.c | 2 +- > > > >> > drivers/gpu/drm/msm/msm_gem.c | 21 --------------------- > > > >> > drivers/gpu/drm/msm/msm_gem.h | 2 -- > > > >> > 3 files changed, 1 insertion(+), 24 deletions(-) > > > >> > > > > >> > Changes in v2: > > > >> > Modify commit message to include the absence of internal locking leftovers > > > >> > around allocating a fake offset in msm_gem_mmap_offset() in the generic > > > >> > implementation drm_gem_create_map_offset(). > > > >> > > > > >> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > > > >> > index a428951ee539..86a15992c717 100644 > > > >> > --- a/drivers/gpu/drm/msm/msm_drv.c > > > >> > +++ b/drivers/gpu/drm/msm/msm_drv.c > > > >> > @@ -1085,7 +1085,7 @@ static const struct drm_driver msm_driver = { > > > >> > .open = msm_open, > > > >> > .postclose = msm_postclose, > > > >> > .dumb_create = msm_gem_dumb_create, > > > >> > - .dumb_map_offset = msm_gem_dumb_map_offset, > > > >> > + .dumb_map_offset = drm_gem_dumb_map_offset, > > > >> > .gem_prime_import_sg_table = msm_gem_prime_import_sg_table, > > > >> > #ifdef CONFIG_DEBUG_FS > > > >> > .debugfs_init = msm_debugfs_init, > > > >> > diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c > > > >> > index db1e748daa75..489694ef79cb 100644 > > > >> > --- a/drivers/gpu/drm/msm/msm_gem.c > > > >> > +++ b/drivers/gpu/drm/msm/msm_gem.c > > > >> > @@ -671,27 +671,6 @@ int msm_gem_dumb_create(struct drm_file *file, struct drm_device *dev, > > > >> > MSM_BO_SCANOUT | MSM_BO_WC, &args->handle, "dumb"); > > > >> > } > > > >> > > > > >> > -int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, > > > >> > - uint32_t handle, uint64_t *offset) > > > >> > -{ > > > >> > - struct drm_gem_object *obj; > > > >> > - int ret = 0; > > > >> > - > > > >> > - /* GEM does all our handle to object mapping */ > > > >> > - obj = drm_gem_object_lookup(file, handle); > > > >> > - if (obj == NULL) { > > > >> > - ret = -ENOENT; > > > >> > - goto fail; > > > >> > - } > > > >> > - > > > >> > - *offset = msm_gem_mmap_offset(obj); > > > >> > - > > > >> > - drm_gem_object_put(obj); > > > >> > - > > > >> > -fail: > > > >> > - return ret; > > > >> > -} > > > >> > - > > > >> > static void *get_vaddr(struct drm_gem_object *obj, unsigned madv) > > > >> > { > > > >> > struct msm_gem_object *msm_obj = to_msm_bo(obj); > > > >> > diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h > > > >> > index 8ddef5443140..dc74a0ef865d 100644 > > > >> > --- a/drivers/gpu/drm/msm/msm_gem.h > > > >> > +++ b/drivers/gpu/drm/msm/msm_gem.h > > > >> > @@ -139,8 +139,6 @@ struct page **msm_gem_pin_pages(struct drm_gem_object *obj); > > > >> > void msm_gem_unpin_pages(struct drm_gem_object *obj); > > > >> > int msm_gem_dumb_create(struct drm_file *file, struct drm_device *dev, > > > >> > struct drm_mode_create_dumb *args); > > > >> > -int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, > > > >> > - uint32_t handle, uint64_t *offset); > > > >> > void *msm_gem_get_vaddr_locked(struct drm_gem_object *obj); > > > >> > void *msm_gem_get_vaddr(struct drm_gem_object *obj); > > > >> > void *msm_gem_get_vaddr_active(struct drm_gem_object *obj); > > > >> > -- > > > >> > 2.34.1 > > > >> > > > > >> > > > >> > > > >> -- > > > >> With best wishes > > > >> Dmitry > > > > > > > > > > > > -- > > > With best wishes > > > Dmitry > > > > -- > With best wishes > Dmitry ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] Remove custom dumb_map_offset implementation in msm driver 2023-11-27 20:52 ` Rob Clark @ 2023-11-27 21:06 ` Dmitry Baryshkov 0 siblings, 0 replies; 7+ messages in thread From: Dmitry Baryshkov @ 2023-11-27 21:06 UTC (permalink / raw) To: Rob Clark Cc: Dipam Turkar, quic_abhinavk, sean, marijn.suijten, airlied, daniel, linux-arm-msm, dri-devel, freedreno, linux-kernel On Mon, 27 Nov 2023 at 22:52, Rob Clark <robdclark@gmail.com> wrote: > > On Tue, Nov 21, 2023 at 5:14 AM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On Tue, 21 Nov 2023 at 04:26, Rob Clark <robdclark@gmail.com> wrote: > > > > > > On Wed, Nov 15, 2023 at 11:33 AM Dmitry Baryshkov > > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > > > On Wed, 15 Nov 2023 at 20:46, Dipam Turkar <dipamt1729@gmail.com> wrote: > > > > > > > > > > They are not outdated, my bad. I went through the locks' code and saw that they have been updated. But they are probably not necessary here as most of the drivers do not use any form of locking in their implementations. The generic implementations drm_gem_dumb_map_offset() and drm_gem_ttm_dumb_map_offset() do not have any locking mechanisms either. > > > > > > > > Excuse me, but this doesn't sound right to me. There are different > > > > drivers with different implementations. So either we'd need a good > > > > explanation of why it is not necessary, or this patch is NAKed. > > > > > > Digging a bit thru history, it looks like commit 0de23977cfeb > > > ("drm/gem: convert to new unified vma manager") made external locking > > > unnecessary, since the vma mgr already had it's own internal locking. > > > > So, should we drop our own locking system? > > specifically for _just_ vma_offset_manager/vma_node, we could. But I > think that only amounts to mmap_offset(). I see. I'll try digging into the mentioned commit. In the meantime, this looks like an R-B from you, doesn't it? > > BR, > -R > > > > > > > BR, > > > -R > > > > > > > > > > > > > Thanks and regards > > > > > Dipam Turkar > > > > > > > > > > On Wed, Nov 15, 2023 at 8:37 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > > > >> > > > > >> On Wed, 15 Nov 2023 at 16:30, Dipam Turkar <dipamt1729@gmail.com> wrote: > > > > >> > > > > > >> > Make msm use drm_gem_create_map_offset() instead of its custom > > > > >> > implementation for associating GEM object with a fake offset. Since, > > > > >> > we already have this generic implementation, we don't need the custom > > > > >> > implementation and it is better to standardize the code for GEM based > > > > >> > drivers. This also removes the outdated locking leftovers. > > > > >> > > > > >> Why are they outdated? > > > > >> > > > > >> > > > > > >> > Signed-off-by: Dipam Turkar <dipamt1729@gmail.com> > > > > >> > --- > > > > >> > drivers/gpu/drm/msm/msm_drv.c | 2 +- > > > > >> > drivers/gpu/drm/msm/msm_gem.c | 21 --------------------- > > > > >> > drivers/gpu/drm/msm/msm_gem.h | 2 -- > > > > >> > 3 files changed, 1 insertion(+), 24 deletions(-) > > > > >> > > > > > >> > Changes in v2: > > > > >> > Modify commit message to include the absence of internal locking leftovers > > > > >> > around allocating a fake offset in msm_gem_mmap_offset() in the generic > > > > >> > implementation drm_gem_create_map_offset(). > > > > >> > > > > > >> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c > > > > >> > index a428951ee539..86a15992c717 100644 > > > > >> > --- a/drivers/gpu/drm/msm/msm_drv.c > > > > >> > +++ b/drivers/gpu/drm/msm/msm_drv.c > > > > >> > @@ -1085,7 +1085,7 @@ static const struct drm_driver msm_driver = { > > > > >> > .open = msm_open, > > > > >> > .postclose = msm_postclose, > > > > >> > .dumb_create = msm_gem_dumb_create, > > > > >> > - .dumb_map_offset = msm_gem_dumb_map_offset, > > > > >> > + .dumb_map_offset = drm_gem_dumb_map_offset, > > > > >> > .gem_prime_import_sg_table = msm_gem_prime_import_sg_table, > > > > >> > #ifdef CONFIG_DEBUG_FS > > > > >> > .debugfs_init = msm_debugfs_init, > > > > >> > diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c > > > > >> > index db1e748daa75..489694ef79cb 100644 > > > > >> > --- a/drivers/gpu/drm/msm/msm_gem.c > > > > >> > +++ b/drivers/gpu/drm/msm/msm_gem.c > > > > >> > @@ -671,27 +671,6 @@ int msm_gem_dumb_create(struct drm_file *file, struct drm_device *dev, > > > > >> > MSM_BO_SCANOUT | MSM_BO_WC, &args->handle, "dumb"); > > > > >> > } > > > > >> > > > > > >> > -int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, > > > > >> > - uint32_t handle, uint64_t *offset) > > > > >> > -{ > > > > >> > - struct drm_gem_object *obj; > > > > >> > - int ret = 0; > > > > >> > - > > > > >> > - /* GEM does all our handle to object mapping */ > > > > >> > - obj = drm_gem_object_lookup(file, handle); > > > > >> > - if (obj == NULL) { > > > > >> > - ret = -ENOENT; > > > > >> > - goto fail; > > > > >> > - } > > > > >> > - > > > > >> > - *offset = msm_gem_mmap_offset(obj); > > > > >> > - > > > > >> > - drm_gem_object_put(obj); > > > > >> > - > > > > >> > -fail: > > > > >> > - return ret; > > > > >> > -} > > > > >> > - > > > > >> > static void *get_vaddr(struct drm_gem_object *obj, unsigned madv) > > > > >> > { > > > > >> > struct msm_gem_object *msm_obj = to_msm_bo(obj); > > > > >> > diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h > > > > >> > index 8ddef5443140..dc74a0ef865d 100644 > > > > >> > --- a/drivers/gpu/drm/msm/msm_gem.h > > > > >> > +++ b/drivers/gpu/drm/msm/msm_gem.h > > > > >> > @@ -139,8 +139,6 @@ struct page **msm_gem_pin_pages(struct drm_gem_object *obj); > > > > >> > void msm_gem_unpin_pages(struct drm_gem_object *obj); > > > > >> > int msm_gem_dumb_create(struct drm_file *file, struct drm_device *dev, > > > > >> > struct drm_mode_create_dumb *args); > > > > >> > -int msm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, > > > > >> > - uint32_t handle, uint64_t *offset); > > > > >> > void *msm_gem_get_vaddr_locked(struct drm_gem_object *obj); > > > > >> > void *msm_gem_get_vaddr(struct drm_gem_object *obj); > > > > >> > void *msm_gem_get_vaddr_active(struct drm_gem_object *obj); > > > > >> > -- > > > > >> > 2.34.1 > > > > >> > > > > > >> > > > > >> > > > > >> -- > > > > >> With best wishes > > > > >> Dmitry > > > > > > > > > > > > > > > > -- > > > > With best wishes > > > > Dmitry > > > > > > > > -- > > With best wishes > > Dmitry -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-11-27 21:07 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-15 14:19 [PATCH v2] Remove custom dumb_map_offset implementation in msm driver Dipam Turkar
2023-11-15 15:07 ` Dmitry Baryshkov
[not found] ` <CALHmwsoC5h7_w9OzpUS_-xM6x5WF5V-vFExLEf4y99b2eCcqGQ@mail.gmail.com>
2023-11-15 19:33 ` Dmitry Baryshkov
2023-11-21 2:26 ` Rob Clark
2023-11-21 13:14 ` Dmitry Baryshkov
2023-11-27 20:52 ` Rob Clark
2023-11-27 21:06 ` Dmitry Baryshkov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®