Re: [PATCH 00/15] Share TTM code among framebuffer drivers

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1.1: Type: text/plain, Size: 7830 bytes --]

Hi

Am 08.04.19 um 13:10 schrieb Koenig, Christian:
> Well first problem is I'm not sure if that is a good idea. Essentially 
> we want to get rid of TTM in the long run.
> 
> On the other hand this work might aid with that goal, so it might be 
> worth a try.

I see. I'm actually interested in porting some of the fbdev driver code
over to DRM. So far, that patch set uses TTM. As there's so much
duplicated code among drivers, I was asked to merge some of the code
into helpers first.

If not for TTM, what would be the alternative? One VMA manager per
memory region per device?

With the patch set applied there are very few TTM calls left in the
drivers and some can even be replaced by helpers. Besides the stronger
cleanup, maybe we can merge these helpers under a different API name?
Something that does not imply a TTM-based implementation? Doing so would
ease a later rewrite with non-TTM code. Suggestions?

> Second is that this might actually not work of hand. The problem is here:
>> +	/* TODO: This test used to be performed by drivers, but can
>> +	 * this actually happen? If so, should we put the check into
>> +	 * drm_gem_ttm_of_gem()? */
>> +	if (!drm_is_gem_ttm(bo))
>> +		return;
> 
> Yeah, this test is mandatory because TTM on itself can present buffer 
> object which doesn't belong to the driver called.
> 
> E.g. we sometimes have BOs which don't belong to the current drivers on 
> a driver specific LRU. A totally brain dead  design if you ask me, but 
> that's how it is.
> 
> Not 100% sure, but by converting all drivers to use a common GEM_TTM 
> backend you might actually break that test.
> 
> I'm not sure if that is actually a problem in the real world, it most 
> likely isn't. But I still won't bet on it without being able to test this.

That's for explaining. So this test should best live in driver-specific
code.

Best regards
Thomas

> Regards,
> Christian.
> 
> Am 08.04.19 um 11:21 schrieb Thomas Zimmermann:
>> Several simple framebuffer drivers copy most of the TTM code from each
>> other. The implementation is always the same; except for the name of
>> some data structures.
>>
>> As recently discussed, this patch set provides generic TTM memory-
>> management code for framebuffers with dedicated video memory. It further
>> converts the respective drivers to the generic code. The shared code
>> is basically the same implementation as the one copied among individual
>> drivers.
>>
>> The patch set contains two major changes: first, it introduces
>> |struct drm_gem_ttm_object| and helpers. It's a GEM object that is
>> backed by TTM-managed memory. The type's purpose is somewhat similar
>> to |struct drm_gem_{cma, shmem}_object|. Second, the patch set
>> introduces |struct drm_simple_ttm| (for the lack of a better name) and
>> helpers. It's an implementation of a basic TTM-based memory manager.
>>
>> Both, GEM TTM and Simple TTM, support VRAM and SYSTEM placements. Support
>> for TT could probably be added if necessary. Both can be used independedly
>> from each other if desired by the DRM driver.
>>
>> Currently ast, bochs, mgag200, vboxvideo and hisilicon/hibmc can use
>> these helpers. Cirrus would also be a candidate, but as it's being
>> rewrtten from scratch, I didn't bother doing the conversion.
>>
>> Future directions: with these changes, the respective drivers can also
>> share some of their mode-setting or fbdev code. GEM TTM could implement
>> PRIME helpers, which would allow for using the generic fbcon.
>>
>> The patch set is against a recent drm-tip.
>>
>> Thomas Zimmermann (15):
>>    drm: Add |struct drm_gem_ttm_object| and helpers
>>    drm: Add |struct drm_gem_ttm_object| callbacks for |struct
>>      ttm_bo_driver|
>>    drm: Add |struct drm_gem_ttm_object| callbacks for |struct drm_driver|
>>    drm: Add drm_gem_ttm_fill_create_dumb() to create dumb buffers
>>    drm: Add Simple TTM, a memory manager for dedicated VRAM
>>    drm/ast: Convert AST driver to |struct drm_gem_ttm_object|
>>    drm/ast: Convert AST driver to Simple TTM
>>    drm/bochs: Convert Bochs driver to |struct drm_gem_ttm_object|
>>    drm/bochs: Convert Bochs driver to Simple TTM
>>    drm/mgag200: Convert mgag200 driver to |struct drm_gem_ttm_object|
>>    drm/mgag200: Convert mgag200 driver to Simple TTM
>>    drm/vboxvideo: Convert vboxvideo driver to |struct drm_gem_ttm_object|
>>    drm/vboxvideo: Convert vboxvideo driver to Simple TTM
>>    drm/hisilicon: Convert hibmc-drm driver to |struct drm_gem_ttm_object|
>>    drm/hisilicon: Convert hibmc-drm driver to Simple TTM
>>
>>   Documentation/gpu/drm-mm.rst                  |  23 +
>>   drivers/gpu/drm/Kconfig                       |  20 +
>>   drivers/gpu/drm/Makefile                      |   5 +
>>   drivers/gpu/drm/ast/Kconfig                   |   3 +-
>>   drivers/gpu/drm/ast/ast_drv.c                 |   4 +-
>>   drivers/gpu/drm/ast/ast_drv.h                 |  58 +-
>>   drivers/gpu/drm/ast/ast_fb.c                  |  18 +-
>>   drivers/gpu/drm/ast/ast_main.c                |  74 +--
>>   drivers/gpu/drm/ast/ast_mode.c                |  78 +--
>>   drivers/gpu/drm/ast/ast_ttm.c                 | 290 +---------
>>   drivers/gpu/drm/bochs/Kconfig                 |   2 +
>>   drivers/gpu/drm/bochs/bochs.h                 |  42 +-
>>   drivers/gpu/drm/bochs/bochs_drv.c             |   4 +-
>>   drivers/gpu/drm/bochs/bochs_kms.c             |  18 +-
>>   drivers/gpu/drm/bochs/bochs_mm.c              | 392 +-------------
>>   drivers/gpu/drm/drm_gem_ttm_helper.c          | 507 ++++++++++++++++++
>>   drivers/gpu/drm/drm_simple_ttm_helper.c       | 191 +++++++
>>   drivers/gpu/drm/drm_ttm_helper_common.c       |   6 +
>>   drivers/gpu/drm/hisilicon/hibmc/Kconfig       |   2 +
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_de.c    |  19 +-
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |   4 +-
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h   |  28 +-
>>   .../gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c |  30 +-
>>   drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c   | 328 +----------
>>   drivers/gpu/drm/mgag200/Kconfig               |   2 +
>>   drivers/gpu/drm/mgag200/mgag200_cursor.c      |  61 ++-
>>   drivers/gpu/drm/mgag200/mgag200_drv.c         |   4 +-
>>   drivers/gpu/drm/mgag200/mgag200_drv.h         |  68 +--
>>   drivers/gpu/drm/mgag200/mgag200_fb.c          |  18 +-
>>   drivers/gpu/drm/mgag200/mgag200_main.c        |  84 +--
>>   drivers/gpu/drm/mgag200/mgag200_mode.c        |  45 +-
>>   drivers/gpu/drm/mgag200/mgag200_ttm.c         | 290 +---------
>>   drivers/gpu/drm/vboxvideo/Kconfig             |   2 +
>>   drivers/gpu/drm/vboxvideo/vbox_drv.c          |   5 +-
>>   drivers/gpu/drm/vboxvideo/vbox_drv.h          |  64 +--
>>   drivers/gpu/drm/vboxvideo/vbox_fb.c           |  22 +-
>>   drivers/gpu/drm/vboxvideo/vbox_main.c         |  70 +--
>>   drivers/gpu/drm/vboxvideo/vbox_mode.c         |  36 +-
>>   drivers/gpu/drm/vboxvideo/vbox_ttm.c          | 344 +-----------
>>   include/drm/drm_gem_ttm_helper.h              | 119 ++++
>>   include/drm/drm_simple_ttm_helper.h           |  74 +++
>>   41 files changed, 1292 insertions(+), 2162 deletions(-)
>>   create mode 100644 drivers/gpu/drm/drm_gem_ttm_helper.c
>>   create mode 100644 drivers/gpu/drm/drm_simple_ttm_helper.c
>>   create mode 100644 drivers/gpu/drm/drm_ttm_helper_common.c
>>   create mode 100644 include/drm/drm_gem_ttm_helper.h
>>   create mode 100644 include/drm/drm_simple_ttm_helper.h
>>
>> --
>> 2.21.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)


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel