| Message ID | 20190906085214.11677-3-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Implement lazy unmapping for GEM VRAM buffers | expand |
On Fri, Sep 06, 2019 at 10:52:13AM +0200, Thomas Zimmermann wrote: > This patch prepares VRAM helpers for lazy unmapping of buffer objects. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/drm_vram_mm_helper.c | 12 ++++++++++++ > include/drm/drm_vram_mm_helper.h | 4 ++++ > 2 files changed, 16 insertions(+) > > diff --git a/drivers/gpu/drm/drm_vram_mm_helper.c b/drivers/gpu/drm/drm_vram_mm_helper.c > index c911781d6728..31984690d5f3 100644 > --- a/drivers/gpu/drm/drm_vram_mm_helper.c > +++ b/drivers/gpu/drm/drm_vram_mm_helper.c > @@ -98,6 +98,17 @@ static int bo_driver_verify_access(struct ttm_buffer_object *bo, > return vmm->funcs->verify_access(bo, filp); > } > > +static void bo_driver_move_notify(struct ttm_buffer_object *bo, > + bool evict, > + struct ttm_mem_reg *new_mem) > +{ > + struct drm_vram_mm *vmm = drm_vram_mm_of_bdev(bo->bdev); > + > + if (!vmm->funcs || !vmm->funcs->move_notify) > + return; > + vmm->funcs->move_notify(bo, evict, new_mem); > +} > + > static int bo_driver_io_mem_reserve(struct ttm_bo_device *bdev, > struct ttm_mem_reg *mem) > { > @@ -140,6 +151,7 @@ static struct ttm_bo_driver bo_driver = { > .eviction_valuable = ttm_bo_eviction_valuable, > .evict_flags = bo_driver_evict_flags, > .verify_access = bo_driver_verify_access, > + .move_notify = bo_driver_move_notify, > .io_mem_reserve = bo_driver_io_mem_reserve, > .io_mem_free = bo_driver_io_mem_free, > }; > diff --git a/include/drm/drm_vram_mm_helper.h b/include/drm/drm_vram_mm_helper.h > index 2aacfb1ccfae..7fb8700f45fe 100644 > --- a/include/drm/drm_vram_mm_helper.h > +++ b/include/drm/drm_vram_mm_helper.h > @@ -15,6 +15,8 @@ struct drm_device; > &ttm_bo_driver.evict_flags > * @verify_access: Provides an implementation for \ > struct &ttm_bo_driver.verify_access > + * @move_notify: Provides an implementation for > + * struct &ttm_bo_driver.move_notify > * > * These callback function integrate VRAM MM with TTM buffer objects. New > * functions can be added if necessary. > @@ -23,6 +25,8 @@ struct drm_vram_mm_funcs { > void (*evict_flags)(struct ttm_buffer_object *bo, > struct ttm_placement *placement); > int (*verify_access)(struct ttm_buffer_object *bo, struct file *filp); > + void (*move_notify)(struct ttm_buffer_object *bo, bool evict, > + struct ttm_mem_reg *new_mem); Is this indirection really worth it? We have a grand total of 1 driver which isn't using gem (vmwgfx), and I don't think that one will ever switch over to vram helpers. I'd fold that all in. Helpers don't need to be flexible enough to support every possible use-case (that's just the job of the core), they can be opinionated to get cleaner code for most cases. For this case here if you fold this in you can unexport 3 EXPORT_SYMBOLs (4 with this patch here), which I think is a neat simplification of the complexity exposed to driver writers. Just put the necessary declarations into a drm_vram_helper_internal.h so that drivers don't get at it by accident (like the other drm*internal.h helpers we have). -Daniel > }; > > /** > -- > 2.23.0 >
Hi Am 06.09.19 um 11:28 schrieb Daniel Vetter: > On Fri, Sep 06, 2019 at 10:52:13AM +0200, Thomas Zimmermann wrote: >> This patch prepares VRAM helpers for lazy unmapping of buffer objects. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/gpu/drm/drm_vram_mm_helper.c | 12 ++++++++++++ >> include/drm/drm_vram_mm_helper.h | 4 ++++ >> 2 files changed, 16 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_vram_mm_helper.c b/drivers/gpu/drm/drm_vram_mm_helper.c >> index c911781d6728..31984690d5f3 100644 >> --- a/drivers/gpu/drm/drm_vram_mm_helper.c >> +++ b/drivers/gpu/drm/drm_vram_mm_helper.c >> @@ -98,6 +98,17 @@ static int bo_driver_verify_access(struct ttm_buffer_object *bo, >> return vmm->funcs->verify_access(bo, filp); >> } >> >> +static void bo_driver_move_notify(struct ttm_buffer_object *bo, >> + bool evict, >> + struct ttm_mem_reg *new_mem) >> +{ >> + struct drm_vram_mm *vmm = drm_vram_mm_of_bdev(bo->bdev); >> + >> + if (!vmm->funcs || !vmm->funcs->move_notify) >> + return; >> + vmm->funcs->move_notify(bo, evict, new_mem); >> +} >> + >> static int bo_driver_io_mem_reserve(struct ttm_bo_device *bdev, >> struct ttm_mem_reg *mem) >> { >> @@ -140,6 +151,7 @@ static struct ttm_bo_driver bo_driver = { >> .eviction_valuable = ttm_bo_eviction_valuable, >> .evict_flags = bo_driver_evict_flags, >> .verify_access = bo_driver_verify_access, >> + .move_notify = bo_driver_move_notify, >> .io_mem_reserve = bo_driver_io_mem_reserve, >> .io_mem_free = bo_driver_io_mem_free, >> }; >> diff --git a/include/drm/drm_vram_mm_helper.h b/include/drm/drm_vram_mm_helper.h >> index 2aacfb1ccfae..7fb8700f45fe 100644 >> --- a/include/drm/drm_vram_mm_helper.h >> +++ b/include/drm/drm_vram_mm_helper.h >> @@ -15,6 +15,8 @@ struct drm_device; >> &ttm_bo_driver.evict_flags >> * @verify_access: Provides an implementation for \ >> struct &ttm_bo_driver.verify_access >> + * @move_notify: Provides an implementation for >> + * struct &ttm_bo_driver.move_notify >> * >> * These callback function integrate VRAM MM with TTM buffer objects. New >> * functions can be added if necessary. >> @@ -23,6 +25,8 @@ struct drm_vram_mm_funcs { >> void (*evict_flags)(struct ttm_buffer_object *bo, >> struct ttm_placement *placement); >> int (*verify_access)(struct ttm_buffer_object *bo, struct file *filp); >> + void (*move_notify)(struct ttm_buffer_object *bo, bool evict, >> + struct ttm_mem_reg *new_mem); > > Is this indirection really worth it? We have a grand total of 1 driver > which isn't using gem (vmwgfx), and I don't think that one will ever > switch over to vram helpers. > > I'd fold that all in. Helpers don't need to be flexible enough to support > every possible use-case (that's just the job of the core), they can be > opinionated to get cleaner code for most cases. > The original idea was to make this as flexible as possible; probably more flexible than necessary. I wouldn't mind merging everything into one file, but please not in this patch set, can we keep it for now and I send you a cleanup next? Best regards Thomas > For this case here if you fold this in you can unexport 3 EXPORT_SYMBOLs > (4 with this patch here), which I think is a neat simplification of the > complexity exposed to driver writers. Just put the necessary declarations > into a drm_vram_helper_internal.h so that drivers don't get at it by > accident (like the other drm*internal.h helpers we have). > -Daniel > >> }; >> >> /** >> -- >> 2.23.0 >> >
On Fri, Sep 6, 2019 at 12:24 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi > > Am 06.09.19 um 11:28 schrieb Daniel Vetter: > > On Fri, Sep 06, 2019 at 10:52:13AM +0200, Thomas Zimmermann wrote: > >> This patch prepares VRAM helpers for lazy unmapping of buffer objects. > >> > >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > >> --- > >> drivers/gpu/drm/drm_vram_mm_helper.c | 12 ++++++++++++ > >> include/drm/drm_vram_mm_helper.h | 4 ++++ > >> 2 files changed, 16 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/drm_vram_mm_helper.c b/drivers/gpu/drm/drm_vram_mm_helper.c > >> index c911781d6728..31984690d5f3 100644 > >> --- a/drivers/gpu/drm/drm_vram_mm_helper.c > >> +++ b/drivers/gpu/drm/drm_vram_mm_helper.c > >> @@ -98,6 +98,17 @@ static int bo_driver_verify_access(struct ttm_buffer_object *bo, > >> return vmm->funcs->verify_access(bo, filp); > >> } > >> > >> +static void bo_driver_move_notify(struct ttm_buffer_object *bo, > >> + bool evict, > >> + struct ttm_mem_reg *new_mem) > >> +{ > >> + struct drm_vram_mm *vmm = drm_vram_mm_of_bdev(bo->bdev); > >> + > >> + if (!vmm->funcs || !vmm->funcs->move_notify) > >> + return; > >> + vmm->funcs->move_notify(bo, evict, new_mem); > >> +} > >> + > >> static int bo_driver_io_mem_reserve(struct ttm_bo_device *bdev, > >> struct ttm_mem_reg *mem) > >> { > >> @@ -140,6 +151,7 @@ static struct ttm_bo_driver bo_driver = { > >> .eviction_valuable = ttm_bo_eviction_valuable, > >> .evict_flags = bo_driver_evict_flags, > >> .verify_access = bo_driver_verify_access, > >> + .move_notify = bo_driver_move_notify, > >> .io_mem_reserve = bo_driver_io_mem_reserve, > >> .io_mem_free = bo_driver_io_mem_free, > >> }; > >> diff --git a/include/drm/drm_vram_mm_helper.h b/include/drm/drm_vram_mm_helper.h > >> index 2aacfb1ccfae..7fb8700f45fe 100644 > >> --- a/include/drm/drm_vram_mm_helper.h > >> +++ b/include/drm/drm_vram_mm_helper.h > >> @@ -15,6 +15,8 @@ struct drm_device; > >> &ttm_bo_driver.evict_flags > >> * @verify_access: Provides an implementation for \ > >> struct &ttm_bo_driver.verify_access > >> + * @move_notify: Provides an implementation for > >> + * struct &ttm_bo_driver.move_notify > >> * > >> * These callback function integrate VRAM MM with TTM buffer objects. New > >> * functions can be added if necessary. > >> @@ -23,6 +25,8 @@ struct drm_vram_mm_funcs { > >> void (*evict_flags)(struct ttm_buffer_object *bo, > >> struct ttm_placement *placement); > >> int (*verify_access)(struct ttm_buffer_object *bo, struct file *filp); > >> + void (*move_notify)(struct ttm_buffer_object *bo, bool evict, > >> + struct ttm_mem_reg *new_mem); > > > > Is this indirection really worth it? We have a grand total of 1 driver > > which isn't using gem (vmwgfx), and I don't think that one will ever > > switch over to vram helpers. > > > > I'd fold that all in. Helpers don't need to be flexible enough to support > > every possible use-case (that's just the job of the core), they can be > > opinionated to get cleaner code for most cases. > > > > The original idea was to make this as flexible as possible; probably > more flexible than necessary. I wouldn't mind merging everything into > one file, but please not in this patch set, can we keep it for now and I > send you a cleanup next? Oh sure, I just wondered about why this flexibility exists and realized there's not really a user for it. And pondering this more I didn't come up with a use-case where it might reasonably be needed, and you don't want to roll your own complete, and couldn't come up with anything. Aside from the locking question on patch 1 I think this looks like a really tidy solution for the fbdev mapping issue, happy to ack once we've figured out the locking thing. -Daniel > > Best regards > Thomas > > > For this case here if you fold this in you can unexport 3 EXPORT_SYMBOLs > > (4 with this patch here), which I think is a neat simplification of the > > complexity exposed to driver writers. Just put the necessary declarations > > into a drm_vram_helper_internal.h so that drivers don't get at it by > > accident (like the other drm*internal.h helpers we have). > > -Daniel > > > >> }; > >> > >> /** > >> -- > >> 2.23.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) >
Hi Am 06.09.19 um 15:05 schrieb Daniel Vetter: > On Fri, Sep 6, 2019 at 12:24 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: >> >> Hi >> >> Am 06.09.19 um 11:28 schrieb Daniel Vetter: >>> On Fri, Sep 06, 2019 at 10:52:13AM +0200, Thomas Zimmermann wrote: >>>> This patch prepares VRAM helpers for lazy unmapping of buffer objects. >>>> >>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>>> --- >>>> drivers/gpu/drm/drm_vram_mm_helper.c | 12 ++++++++++++ >>>> include/drm/drm_vram_mm_helper.h | 4 ++++ >>>> 2 files changed, 16 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/drm_vram_mm_helper.c b/drivers/gpu/drm/drm_vram_mm_helper.c >>>> index c911781d6728..31984690d5f3 100644 >>>> --- a/drivers/gpu/drm/drm_vram_mm_helper.c >>>> +++ b/drivers/gpu/drm/drm_vram_mm_helper.c >>>> @@ -98,6 +98,17 @@ static int bo_driver_verify_access(struct ttm_buffer_object *bo, >>>> return vmm->funcs->verify_access(bo, filp); >>>> } >>>> >>>> +static void bo_driver_move_notify(struct ttm_buffer_object *bo, >>>> + bool evict, >>>> + struct ttm_mem_reg *new_mem) >>>> +{ >>>> + struct drm_vram_mm *vmm = drm_vram_mm_of_bdev(bo->bdev); >>>> + >>>> + if (!vmm->funcs || !vmm->funcs->move_notify) >>>> + return; >>>> + vmm->funcs->move_notify(bo, evict, new_mem); >>>> +} >>>> + >>>> static int bo_driver_io_mem_reserve(struct ttm_bo_device *bdev, >>>> struct ttm_mem_reg *mem) >>>> { >>>> @@ -140,6 +151,7 @@ static struct ttm_bo_driver bo_driver = { >>>> .eviction_valuable = ttm_bo_eviction_valuable, >>>> .evict_flags = bo_driver_evict_flags, >>>> .verify_access = bo_driver_verify_access, >>>> + .move_notify = bo_driver_move_notify, >>>> .io_mem_reserve = bo_driver_io_mem_reserve, >>>> .io_mem_free = bo_driver_io_mem_free, >>>> }; >>>> diff --git a/include/drm/drm_vram_mm_helper.h b/include/drm/drm_vram_mm_helper.h >>>> index 2aacfb1ccfae..7fb8700f45fe 100644 >>>> --- a/include/drm/drm_vram_mm_helper.h >>>> +++ b/include/drm/drm_vram_mm_helper.h >>>> @@ -15,6 +15,8 @@ struct drm_device; >>>> &ttm_bo_driver.evict_flags >>>> * @verify_access: Provides an implementation for \ >>>> struct &ttm_bo_driver.verify_access >>>> + * @move_notify: Provides an implementation for >>>> + * struct &ttm_bo_driver.move_notify >>>> * >>>> * These callback function integrate VRAM MM with TTM buffer objects. New >>>> * functions can be added if necessary. >>>> @@ -23,6 +25,8 @@ struct drm_vram_mm_funcs { >>>> void (*evict_flags)(struct ttm_buffer_object *bo, >>>> struct ttm_placement *placement); >>>> int (*verify_access)(struct ttm_buffer_object *bo, struct file *filp); >>>> + void (*move_notify)(struct ttm_buffer_object *bo, bool evict, >>>> + struct ttm_mem_reg *new_mem); >>> >>> Is this indirection really worth it? We have a grand total of 1 driver >>> which isn't using gem (vmwgfx), and I don't think that one will ever >>> switch over to vram helpers. >>> >>> I'd fold that all in. Helpers don't need to be flexible enough to support >>> every possible use-case (that's just the job of the core), they can be >>> opinionated to get cleaner code for most cases. >>> >> >> The original idea was to make this as flexible as possible; probably >> more flexible than necessary. I wouldn't mind merging everything into >> one file, but please not in this patch set, can we keep it for now and I >> send you a cleanup next? > > Oh sure, I just wondered about why this flexibility exists and > realized there's not really a user for it. And pondering this more I > didn't come up with a use-case where it might reasonably be needed, > and you don't want to roll your own complete, and couldn't come up > with anything. Aside from the locking question on patch 1 I think this > looks like a really tidy solution for the fbdev mapping issue, happy > to ack once we've figured out the locking thing. Great. There's a v4 of the patch set that should resolve the locking problem. Best regards Thomas > -Daniel > >> >> Best regards >> Thomas >> >>> For this case here if you fold this in you can unexport 3 EXPORT_SYMBOLs >>> (4 with this patch here), which I think is a neat simplification of the >>> complexity exposed to driver writers. Just put the necessary declarations >>> into a drm_vram_helper_internal.h so that drivers don't get at it by >>> accident (like the other drm*internal.h helpers we have). >>> -Daniel >>> >>>> }; >>>> >>>> /** >>>> -- >>>> 2.23.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) >> > >
diff --git a/drivers/gpu/drm/drm_vram_mm_helper.c b/drivers/gpu/drm/drm_vram_mm_helper.c index c911781d6728..31984690d5f3 100644 --- a/drivers/gpu/drm/drm_vram_mm_helper.c +++ b/drivers/gpu/drm/drm_vram_mm_helper.c @@ -98,6 +98,17 @@ static int bo_driver_verify_access(struct ttm_buffer_object *bo, return vmm->funcs->verify_access(bo, filp); } +static void bo_driver_move_notify(struct ttm_buffer_object *bo, + bool evict, + struct ttm_mem_reg *new_mem) +{ + struct drm_vram_mm *vmm = drm_vram_mm_of_bdev(bo->bdev); + + if (!vmm->funcs || !vmm->funcs->move_notify) + return; + vmm->funcs->move_notify(bo, evict, new_mem); +} + static int bo_driver_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_mem_reg *mem) { @@ -140,6 +151,7 @@ static struct ttm_bo_driver bo_driver = { .eviction_valuable = ttm_bo_eviction_valuable, .evict_flags = bo_driver_evict_flags, .verify_access = bo_driver_verify_access, + .move_notify = bo_driver_move_notify, .io_mem_reserve = bo_driver_io_mem_reserve, .io_mem_free = bo_driver_io_mem_free, }; diff --git a/include/drm/drm_vram_mm_helper.h b/include/drm/drm_vram_mm_helper.h index 2aacfb1ccfae..7fb8700f45fe 100644 --- a/include/drm/drm_vram_mm_helper.h +++ b/include/drm/drm_vram_mm_helper.h @@ -15,6 +15,8 @@ struct drm_device; &ttm_bo_driver.evict_flags * @verify_access: Provides an implementation for \ struct &ttm_bo_driver.verify_access + * @move_notify: Provides an implementation for + * struct &ttm_bo_driver.move_notify * * These callback function integrate VRAM MM with TTM buffer objects. New * functions can be added if necessary. @@ -23,6 +25,8 @@ struct drm_vram_mm_funcs { void (*evict_flags)(struct ttm_buffer_object *bo, struct ttm_placement *placement); int (*verify_access)(struct ttm_buffer_object *bo, struct file *filp); + void (*move_notify)(struct ttm_buffer_object *bo, bool evict, + struct ttm_mem_reg *new_mem); }; /**
This patch prepares VRAM helpers for lazy unmapping of buffer objects. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/drm_vram_mm_helper.c | 12 ++++++++++++ include/drm/drm_vram_mm_helper.h | 4 ++++ 2 files changed, 16 insertions(+)