[PATCH v3 2/3] drm/vram: Add infrastructure for move_notify()

[Thomas Zimmermann]

Hi

Am 06.09.19 um 15:05 schrieb Daniel Vetter:
> On Fri, Sep 6, 2019 at 12:24 PM Thomas Zimmermann <tzimmermann at 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 at 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)
>>
> 
> 

-- 
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)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20190906/a73e404f/attachment.sig>