Re: [PATCH 0/5] Clean up TTM mmap offsets

Re: [PATCH 0/5] Clean up TTM mmap offsets

[Koenig, Christian]

Am 07.02.19 um 09:59 schrieb Thomas Zimmermann:
> Almost all TTM-based drivers use the same values for the mmap-able
> range of BO addresses. Each driver therefore duplicates the
> DRM_FILE_PAGE_OFFSET constant. OTOH, the mmap range's size is not
> configurable by drivers.
>
> This patch set replaces driver-specific configuration with a single
> setup. All code is located within TTM. TTM and GEM share the same
> range for mmap-able BOs.
>
> Thomas Zimmermann (5):
>    staging/vboxvideo: Use same BO mmap offset as other drivers
>    drm/ttm: Define a single DRM_FILE_PAGE_OFFSET constant
>    drm/ttm: Remove file_page_offset parameter from ttm_bo_device_init()
>    drm/ttm: Quick-test mmap offset in ttm_bo_mmap()
>    drm: Use the same mmap-range offset and size for GEM and TTM

Reviewed-by: Christian König <christian.koenig@amd.com> for the whole 
series.

Nice cleanup! Thanks,
Christian.

>
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c     | 12 ++----------
>   drivers/gpu/drm/ast/ast_drv.h               |  2 --
>   drivers/gpu/drm/ast/ast_ttm.c               | 10 ++--------
>   drivers/gpu/drm/bochs/bochs.h               |  2 --
>   drivers/gpu/drm/bochs/bochs_mm.c            | 10 ++--------
>   drivers/gpu/drm/cirrus/cirrus_drv.h         |  1 -
>   drivers/gpu/drm/cirrus/cirrus_ttm.c         | 10 ++--------
>   drivers/gpu/drm/drm_gem.c                   | 17 -----------------
>   drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 12 ++----------
>   drivers/gpu/drm/mgag200/mgag200_drv.h       |  1 -
>   drivers/gpu/drm/mgag200/mgag200_ttm.c       | 10 ++--------
>   drivers/gpu/drm/nouveau/nouveau_drv.h       |  2 --
>   drivers/gpu/drm/nouveau/nouveau_ttm.c       |  4 ----
>   drivers/gpu/drm/qxl/qxl_drv.h               |  3 ---
>   drivers/gpu/drm/qxl/qxl_ttm.c               | 11 +++--------
>   drivers/gpu/drm/radeon/radeon_ttm.c         | 13 ++-----------
>   drivers/gpu/drm/ttm/ttm_bo.c                |  6 +++---
>   drivers/gpu/drm/ttm/ttm_bo_vm.c             |  3 +++
>   drivers/gpu/drm/virtio/virtgpu_ttm.c        |  4 +---
>   drivers/gpu/drm/vmwgfx/vmwgfx_drv.c         |  1 -
>   drivers/gpu/drm/vmwgfx/vmwgfx_drv.h         |  1 -
>   drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c    | 11 ++---------
>   drivers/staging/vboxvideo/vbox_drv.h        |  2 --
>   drivers/staging/vboxvideo/vbox_ttm.c        | 12 +++---------
>   include/drm/drm_vma_manager.h               | 12 ++++++++++++
>   include/drm/ttm/ttm_bo_driver.h             |  2 +-
>   26 files changed, 42 insertions(+), 132 deletions(-)
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 2/5] drm/ttm: Define a single DRM_FILE_PAGE_OFFSET constant

[Thomas Hellstrom via Virtualization]

On Thu, 2019-02-07 at 09:59 +0100, Thomas Zimmermann wrote:
> Most TTM drivers define the constant DRM_FILE_PAGE_OFFSET of the same
> value. The only exception is vboxvideo, which is being converted to
> the
> new offset by this patch. Unifying the constants in a single place
> simplifies the driver code.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c     | 2 --
>  drivers/gpu/drm/ast/ast_drv.h               | 2 --
>  drivers/gpu/drm/bochs/bochs.h               | 2 --
>  drivers/gpu/drm/cirrus/cirrus_drv.h         | 1 -
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 2 --
>  drivers/gpu/drm/mgag200/mgag200_drv.h       | 1 -
>  drivers/gpu/drm/nouveau/nouveau_drv.h       | 2 --
>  drivers/gpu/drm/qxl/qxl_drv.h               | 3 ---
>  drivers/gpu/drm/radeon/radeon_ttm.c         | 2 --
>  drivers/gpu/drm/virtio/virtgpu_ttm.c        | 2 --
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c         | 2 +-
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.h         | 1 -
>  drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c    | 2 +-
>  drivers/staging/vboxvideo/vbox_drv.h        | 2 --
>  include/drm/ttm/ttm_bo_driver.h             | 2 ++
>  15 files changed, 4 insertions(+), 24 deletions(-)
> 
> 

...

> diff --git a/include/drm/ttm/ttm_bo_driver.h
> b/include/drm/ttm/ttm_bo_driver.h
> index cbf3180cb612..c0bed72492f3 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -49,6 +49,8 @@
>  #define TTM_MEMTYPE_FLAG_MAPPABLE      (1 << 1)	/* Memory
> mappable */
>  #define TTM_MEMTYPE_FLAG_CMA           (1 << 3)	/* Can't map
> aperture */
>  
> +#define DRM_FILE_PAGE_OFFSET (0x100000000ULL >> PAGE_SHIFT)
> +

IMO strictly, the place in the drm file address space where a driver
chooses to insert the TTM bo address space is to be determined by DRM,
(as also the define indicates) rather than TTM. So I think the right
place for this define is a drm header, not a ttm header.

Thanks,
Thomas


>  struct ttm_mem_type_manager;
>  
>  struct ttm_mem_type_manager_func {

Re: [PATCH 0/5] Clean up TTM mmap offsets

[Hans de Goede]

Hi,

On 2/7/19 9:59 AM, Thomas Zimmermann wrote:
> Almost all TTM-based drivers use the same values for the mmap-able
> range of BO addresses. Each driver therefore duplicates the
> DRM_FILE_PAGE_OFFSET constant. OTOH, the mmap range's size is not
> configurable by drivers.
> 
> This patch set replaces driver-specific configuration with a single
> setup. All code is located within TTM. TTM and GEM share the same
> range for mmap-able BOs.
> 
> Thomas Zimmermann (5):
>    staging/vboxvideo: Use same BO mmap offset as other drivers
>    drm/ttm: Define a single DRM_FILE_PAGE_OFFSET constant
>    drm/ttm: Remove file_page_offset parameter from ttm_bo_device_init()
>    drm/ttm: Quick-test mmap offset in ttm_bo_mmap()
>    drm: Use the same mmap-range offset and size for GEM and TTM

The first patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

The vboxvideo bits in the other patches look good to me to:

Acked-by: Hans de Goede <hdegoede@redhat.com>

For the other patches in the series.

Regards,

Hans

Re: [PATCH 0/5] Clean up TTM mmap offsets

[Hans de Goede]

Hi,

On 07-02-19 09:59, Thomas Zimmermann wrote:
> Almost all TTM-based drivers use the same values for the mmap-able
> range of BO addresses. Each driver therefore duplicates the
> DRM_FILE_PAGE_OFFSET constant. OTOH, the mmap range's size is not
> configurable by drivers.
> 
> This patch set replaces driver-specific configuration with a single
> setup. All code is located within TTM. TTM and GEM share the same
> range for mmap-able BOs.
> 
> Thomas Zimmermann (5):
>    staging/vboxvideo: Use same BO mmap offset as other drivers
>    drm/ttm: Define a single DRM_FILE_PAGE_OFFSET constant
>    drm/ttm: Remove file_page_offset parameter from ttm_bo_device_init()
>    drm/ttm: Quick-test mmap offset in ttm_bo_mmap()
>    drm: Use the same mmap-range offset and size for GEM and TTM

Note I'm about to push a patch-series to drm-misc-next which moves
vboxvideo out of staging and I see that this series has not landed
in drm-misc-next yet, so it will needs to be rebased.

Regards,

Hans

Re: [PATCH 0/5] Clean up TTM mmap offsets

[Hans de Goede]

Hi,

On 11-03-19 17:51, Christian König wrote:
> Am 11.03.19 um 17:39 schrieb Hans de Goede:
>> Hi,
>>
>> On 07-02-19 09:59, Thomas Zimmermann wrote:
>>> Almost all TTM-based drivers use the same values for the mmap-able
>>> range of BO addresses. Each driver therefore duplicates the
>>> DRM_FILE_PAGE_OFFSET constant. OTOH, the mmap range's size is not
>>> configurable by drivers.
>>>
>>> This patch set replaces driver-specific configuration with a single
>>> setup. All code is located within TTM. TTM and GEM share the same
>>> range for mmap-able BOs.
>>>
>>> Thomas Zimmermann (5):
>>>    staging/vboxvideo: Use same BO mmap offset as other drivers
>>>    drm/ttm: Define a single DRM_FILE_PAGE_OFFSET constant
>>>    drm/ttm: Remove file_page_offset parameter from ttm_bo_device_init()
>>>    drm/ttm: Quick-test mmap offset in ttm_bo_mmap()
>>>    drm: Use the same mmap-range offset and size for GEM and TTM
>>
>> Note I'm about to push a patch-series to drm-misc-next which moves
>> vboxvideo out of staging and I see that this series has not landed
>> in drm-misc-next yet, so it will needs to be rebased.
> 
> Mhm, TTM is usually not pushed upstream through drm-misc-next, so that will certainly collide with the next TTM pull request.

Ugh, I didn't realize that this series would not be going through drm-misc-next.

> So can you wait with that or should I make an exception and merge this change though drm-misc-next?

I've already pushed it now :| My mail was more intended as a headsup then
that I expected an objection, sorry.

I see 2 possible solutions:

1) Merge drm-misc-next into the ttm tree (probably the cleanest)
2) Push your series through drm-misc-next

Regards,

Hans
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH 0/5] Clean up TTM mmap offsets

[Daniel Vetter]

On Mon, Mar 11, 2019 at 05:51:39PM +0100, Christian König wrote:
> Am 11.03.19 um 17:39 schrieb Hans de Goede:
> > Hi,
> > 
> > On 07-02-19 09:59, Thomas Zimmermann wrote:
> > > Almost all TTM-based drivers use the same values for the mmap-able
> > > range of BO addresses. Each driver therefore duplicates the
> > > DRM_FILE_PAGE_OFFSET constant. OTOH, the mmap range's size is not
> > > configurable by drivers.
> > > 
> > > This patch set replaces driver-specific configuration with a single
> > > setup. All code is located within TTM. TTM and GEM share the same
> > > range for mmap-able BOs.
> > > 
> > > Thomas Zimmermann (5):
> > >    staging/vboxvideo: Use same BO mmap offset as other drivers
> > >    drm/ttm: Define a single DRM_FILE_PAGE_OFFSET constant
> > >    drm/ttm: Remove file_page_offset parameter from ttm_bo_device_init()
> > >    drm/ttm: Quick-test mmap offset in ttm_bo_mmap()
> > >    drm: Use the same mmap-range offset and size for GEM and TTM
> > 
> > Note I'm about to push a patch-series to drm-misc-next which moves
> > vboxvideo out of staging and I see that this series has not landed
> > in drm-misc-next yet, so it will needs to be rebased.
> 
> Mhm, TTM is usually not pushed upstream through drm-misc-next, so that will
> certainly collide with the next TTM pull request.
> 
> So can you wait with that or should I make an exception and merge this
> change though drm-misc-next?

Other options:
- Get amdgpu added to drm-tip and linux-next so we have a heads-up about
  the conflict. That's usually good enough to avoid the broken merge
  conflict.
- Do a topic branch, pull it into both trees.
- Really stuff ttm into a shared tree if it's meant to be shared
  infrastructure :-P

Waiting+rebasing is imo the worst option, and usually not needed.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch