[Nouveau] I wish to get actively involved in nouveau driver development

[Chinmay Manas]

Hello everyone
I'm a 2nd year Computer Science student in India.
I'm proficient in C++, Data Structures and Algorithms, Linux and git.
I've been using Linux for about 2 years now and shifted to Arch Linux with
i3wm a year ago.

I wish to get involved in nouveau driver development.
Any kind of suggestions on how to get started with development would be
great.

Thank you.
Regards
- Chinmay Manas


[image: Mailtrack]

Sender
notified by
Mailtrack

17/09/20,
09:09:27 am
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 14/21] drm/tegra: Introduce GEM object functions

[Thierry Reding]

On Tue, Sep 15, 2020 at 04:59:51PM +0200, Thomas Zimmermann wrote:
> GEM object functions deprecate several similar callback interfaces in
> struct drm_driver. This patch replaces the per-driver callbacks with
> per-instance callbacks in tegra.
> 
> Signed-off-by: Thomas Zimmermann 
> ---
>  drivers/gpu/drm/tegra/drm.c | 4 
>  drivers/gpu/drm/tegra/gem.c | 8 
>  2 files changed, 8 insertions(+), 4 deletions(-)

Acked-by: Thierry Reding 


signature.asc
Description: PGP signature
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH] mm: remove extra ZONE_DEVICE struct page refcount

[Dan Williams]

On Wed, Sep 16, 2020 at 5:29 PM Ralph Campbell  wrote:
>
>
> On 9/15/20 10:36 PM, Christoph Hellwig wrote:
> > On Tue, Sep 15, 2020 at 09:39:47AM -0700, Ralph Campbell wrote:
> >>> I don't think any of the three ->page_free instances even cares about
> >>> the page refcount.
> >>>
> >> Not true. The page_free() callback records the page is free by setting
> >> a bit or putting the page on a free list but when it allocates a free
> >> device private struct page to be used with migrate_vma_setup(), it needs to
> >> increment the refcount.
> >>
> >> For the ZONE_DEVICE MEMORY_DEVICE_GENERIC and MEMORY_DEVICE_PCI_P2PDMA
> >> struct pages, I think you are correct because they don't define page_free()
> >> and from what I can see, don't decrement the page refcount to zero.
> >
> > Umm, the whole point of ZONE_DEVICE is to have a struct page for
> > something that is not system memory.  For both the ppc kvm case (magic
> > hypervisor pool) and Noveau (device internal) memory that clear is the
> > case.  But looks like test_hmm uses normal pages to fake this up, so
> > I was wrong about the third caller.  But I think we can just call
> > set_page_count just before freeing the page there with a comment
> > explaining what is goin on.
>
> Dan Williams thought that having the ZONE_DEVICE struct pages
> be on a free list with a refcount of one was a bit strange and
> that the driver should handle the zero to one transition.
> But, that would mean a bit more invasive change to the 3 drivers
> to set the reference count to zero after calling memremap_pages()
> and setting the reference count to one when allocating a struct
> page. What you are suggesting is what I also proposed in v1.

IIUC, isn't what Christoph recommending is that drivers handle
set_page_count() directly rather than the core since some are prepared
for it to be zero on entry?
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 07/21] drm/mediatek: Introduce GEM object functions

[Daniel Vetter]

On Tue, Sep 15, 2020 at 04:59:44PM +0200, Thomas Zimmermann wrote:
> GEM object functions deprecate several similar callback interfaces in
> struct drm_driver. This patch replaces the per-driver callbacks with
> per-instance callbacks in mediatek. The only exception is gem_prime_mmap,
> which is non-trivial to convert.
> 
> Signed-off-by: Thomas Zimmermann 
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_drv.c |  5 -
>  drivers/gpu/drm/mediatek/mtk_drm_gem.c | 11 +++
>  2 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c 
> b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> index 040a8f393fe2..2f8d0043fca7 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> @@ -301,18 +301,13 @@ struct drm_gem_object *mtk_drm_gem_prime_import(struct 
> drm_device *dev,
>  static struct drm_driver mtk_drm_driver = {
>   .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
>  
> - .gem_free_object_unlocked = mtk_drm_gem_free_object,
> - .gem_vm_ops = _gem_cma_vm_ops,
>   .dumb_create = mtk_drm_gem_dumb_create,
>  
>   .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>   .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>   .gem_prime_import = mtk_drm_gem_prime_import,
> - .gem_prime_get_sg_table = mtk_gem_prime_get_sg_table,
>   .gem_prime_import_sg_table = mtk_gem_prime_import_sg_table,
>   .gem_prime_mmap = mtk_drm_gem_mmap_buf,
> - .gem_prime_vmap = mtk_drm_gem_prime_vmap,
> - .gem_prime_vunmap = mtk_drm_gem_prime_vunmap,
>   .fops = _drm_fops,
>  
>   .name = DRIVER_NAME,
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_gem.c 
> b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> index 6190cc3b7b0d..591b90410e4a 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_gem.c
> @@ -8,11 +8,20 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  
>  #include "mtk_drm_drv.h"
>  #include "mtk_drm_gem.h"
>  
> +static const struct drm_gem_object_funcs mtk_drm_gem_object_funcs = {
> + .free = mtk_drm_gem_free_object,
> + .get_sg_table = mtk_gem_prime_get_sg_table,
> + .vmap = mtk_drm_gem_prime_vmap,
> + .vunmap = mtk_drm_gem_prime_vunmap,
> + .vm_ops = _gem_cma_vm_ops,
> +};
> +
>  static struct mtk_drm_gem_obj *mtk_drm_gem_init(struct drm_device *dev,
>   unsigned long size)
>  {
> @@ -25,6 +34,8 @@ static struct mtk_drm_gem_obj *mtk_drm_gem_init(struct 
> drm_device *dev,
>   if (!mtk_gem_obj)
>   return ERR_PTR(-ENOMEM);
>  
> + mtk_gem_obj->base.funcs = _drm_gem_object_funcs;
> +
>   ret = drm_gem_object_init(dev, _gem_obj->base, size);
>   if (ret < 0) {
>   DRM_ERROR("failed to initialize gem object\n");
> -- 
> 2.28.0

Reviewed-by: Daniel Vetter 

> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 04/21] drm/exynos: Introduce GEM object functions

[Daniel Vetter]

On Tue, Sep 15, 2020 at 04:59:41PM +0200, Thomas Zimmermann wrote:
> GEM object functions deprecate several similar callback interfaces in
> struct drm_driver. This patch replaces the per-driver callbacks with
> per-instance callbacks in exynos. The only exception is gem_prime_mmap,
> which is non-trivial to convert.
> 
> Signed-off-by: Thomas Zimmermann 
> ---
>  drivers/gpu/drm/exynos/exynos_drm_drv.c | 10 --
>  drivers/gpu/drm/exynos/exynos_drm_gem.c | 15 +++
>  2 files changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c 
> b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index dbd80f1e4c78..fe46680ca208 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -75,11 +75,6 @@ static void exynos_drm_postclose(struct drm_device *dev, 
> struct drm_file *file)
>   file->driver_priv = NULL;
>  }
>  
> -static const struct vm_operations_struct exynos_drm_gem_vm_ops = {
> - .open = drm_gem_vm_open,
> - .close = drm_gem_vm_close,
> -};
> -
>  static const struct drm_ioctl_desc exynos_ioctls[] = {
>   DRM_IOCTL_DEF_DRV(EXYNOS_GEM_CREATE, exynos_drm_gem_create_ioctl,
>   DRM_RENDER_ALLOW),
> @@ -124,16 +119,11 @@ static struct drm_driver exynos_drm_driver = {
>   .open   = exynos_drm_open,
>   .lastclose  = drm_fb_helper_lastclose,
>   .postclose  = exynos_drm_postclose,
> - .gem_free_object_unlocked = exynos_drm_gem_free_object,
> - .gem_vm_ops = _drm_gem_vm_ops,
>   .dumb_create= exynos_drm_gem_dumb_create,
>   .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>   .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>   .gem_prime_import   = exynos_drm_gem_prime_import,
> - .gem_prime_get_sg_table = exynos_drm_gem_prime_get_sg_table,
>   .gem_prime_import_sg_table  = exynos_drm_gem_prime_import_sg_table,
> - .gem_prime_vmap = exynos_drm_gem_prime_vmap,
> - .gem_prime_vunmap   = exynos_drm_gem_prime_vunmap,
>   .gem_prime_mmap = exynos_drm_gem_prime_mmap,
>   .ioctls = exynos_ioctls,
>   .num_ioctls = ARRAY_SIZE(exynos_ioctls),
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c 
> b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> index efa476858db5..69a5cf28b4ae 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> @@ -129,6 +129,19 @@ void exynos_drm_gem_destroy(struct exynos_drm_gem 
> *exynos_gem)
>   kfree(exynos_gem);
>  }
>  
> +static const struct vm_operations_struct exynos_drm_gem_vm_ops = {
> + .open = drm_gem_vm_open,
> + .close = drm_gem_vm_close,
> +};

Hm moving the drm_gem_cma_vm_ops into drm_gem.h or so and maybe calling
them drm_gem_simple_ops or so would remove a pile of these. But perhaps a
quick follow up series.

Reviewed-by: Daniel Vetter 

> +
> +static const struct drm_gem_object_funcs exynos_drm_gem_object_funcs = {
> + .free = exynos_drm_gem_free_object,
> + .get_sg_table = exynos_drm_gem_prime_get_sg_table,
> + .vmap = exynos_drm_gem_prime_vmap,
> + .vunmap = exynos_drm_gem_prime_vunmap,
> + .vm_ops = _drm_gem_vm_ops,
> +};
> +
>  static struct exynos_drm_gem *exynos_drm_gem_init(struct drm_device *dev,
> unsigned long size)
>  {
> @@ -143,6 +156,8 @@ static struct exynos_drm_gem *exynos_drm_gem_init(struct 
> drm_device *dev,
>   exynos_gem->size = size;
>   obj = _gem->base;
>  
> + obj->funcs = _drm_gem_object_funcs;
> +
>   ret = drm_gem_object_init(dev, obj, size);
>   if (ret < 0) {
>   DRM_DEV_ERROR(dev->dev, "failed to initialize gem object\n");
> -- 
> 2.28.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 04/21] drm/exynos: Introduce GEM object functions

[Thomas Zimmermann]

Hi

Am 16.09.20 um 12:03 schrieb Daniel Vetter:
> On Tue, Sep 15, 2020 at 04:59:41PM +0200, Thomas Zimmermann wrote:
>> GEM object functions deprecate several similar callback interfaces in
>> struct drm_driver. This patch replaces the per-driver callbacks with
>> per-instance callbacks in exynos. The only exception is gem_prime_mmap,
>> which is non-trivial to convert.
>>
>> Signed-off-by: Thomas Zimmermann 
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_drv.c | 10 --
>>  drivers/gpu/drm/exynos/exynos_drm_gem.c | 15 +++
>>  2 files changed, 15 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c 
>> b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> index dbd80f1e4c78..fe46680ca208 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> @@ -75,11 +75,6 @@ static void exynos_drm_postclose(struct drm_device *dev, 
>> struct drm_file *file)
>>  file->driver_priv = NULL;
>>  }
>>  
>> -static const struct vm_operations_struct exynos_drm_gem_vm_ops = {
>> -.open = drm_gem_vm_open,
>> -.close = drm_gem_vm_close,
>> -};
>> -
>>  static const struct drm_ioctl_desc exynos_ioctls[] = {
>>  DRM_IOCTL_DEF_DRV(EXYNOS_GEM_CREATE, exynos_drm_gem_create_ioctl,
>>  DRM_RENDER_ALLOW),
>> @@ -124,16 +119,11 @@ static struct drm_driver exynos_drm_driver = {
>>  .open   = exynos_drm_open,
>>  .lastclose  = drm_fb_helper_lastclose,
>>  .postclose  = exynos_drm_postclose,
>> -.gem_free_object_unlocked = exynos_drm_gem_free_object,
>> -.gem_vm_ops = _drm_gem_vm_ops,
>>  .dumb_create= exynos_drm_gem_dumb_create,
>>  .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>>  .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>>  .gem_prime_import   = exynos_drm_gem_prime_import,
>> -.gem_prime_get_sg_table = exynos_drm_gem_prime_get_sg_table,
>>  .gem_prime_import_sg_table  = exynos_drm_gem_prime_import_sg_table,
>> -.gem_prime_vmap = exynos_drm_gem_prime_vmap,
>> -.gem_prime_vunmap   = exynos_drm_gem_prime_vunmap,
>>  .gem_prime_mmap = exynos_drm_gem_prime_mmap,
>>  .ioctls = exynos_ioctls,
>>  .num_ioctls = ARRAY_SIZE(exynos_ioctls),
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c 
>> b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>> index efa476858db5..69a5cf28b4ae 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
>> @@ -129,6 +129,19 @@ void exynos_drm_gem_destroy(struct exynos_drm_gem 
>> *exynos_gem)
>>  kfree(exynos_gem);
>>  }
>>  
>> +static const struct vm_operations_struct exynos_drm_gem_vm_ops = {
>> +.open = drm_gem_vm_open,
>> +.close = drm_gem_vm_close,
>> +};
> 
> Hm moving the drm_gem_cma_vm_ops into drm_gem.h or so and maybe calling
> them drm_gem_simple_ops or so would remove a pile of these. But perhaps a
> quick follow up series.

Good idea. Several interfaces use the term 'default' in their name, so
something like drm_gem_default_vm_ops seems appropriate.

BTW is there a reason why we have file operations like
DEFINE_DRM_GEM_CMA_FOPS() in each module? It seems like this could also
be provided by the rsp memory-manager library.

Best regards
Thomas

> 
> Reviewed-by: Daniel Vetter 
> 
>> +
>> +static const struct drm_gem_object_funcs exynos_drm_gem_object_funcs = {
>> +.free = exynos_drm_gem_free_object,
>> +.get_sg_table = exynos_drm_gem_prime_get_sg_table,
>> +.vmap = exynos_drm_gem_prime_vmap,
>> +.vunmap = exynos_drm_gem_prime_vunmap,
>> +.vm_ops = _drm_gem_vm_ops,
>> +};
>> +
>>  static struct exynos_drm_gem *exynos_drm_gem_init(struct drm_device *dev,
>>unsigned long size)
>>  {
>> @@ -143,6 +156,8 @@ static struct exynos_drm_gem *exynos_drm_gem_init(struct 
>> drm_device *dev,
>>  exynos_gem->size = size;
>>  obj = _gem->base;
>>  
>> +obj->funcs = _drm_gem_object_funcs;
>> +
>>  ret = drm_gem_object_init(dev, obj, size);
>>  if (ret < 0) {
>>  DRM_DEV_ERROR(dev->dev, "failed to initialize gem object\n");
>> -- 
>> 2.28.0
>>
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer



signature.asc
Description: OpenPGP digital signature
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH] mm: remove extra ZONE_DEVICE struct page refcount

[Christoph Hellwig]

On Mon, Sep 14, 2020 at 04:10:38PM -0700, Dan Williams wrote:
> You also need to fix up ext4_break_layouts() and
> xfs_break_dax_layouts() to expect ->_refcount is 0 instead of 1. This
> also needs some fstests exposure.

While we're at it, can we add a wait_fsdax_unref helper macro that hides
the _refcount access from the file systems?
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 16/21] drm/vgem: Introduce GEM object functions

[Melissa Wen]

Hi Thomas,

On 09/15, Thomas Zimmermann wrote:
> GEM object functions deprecate several similar callback interfaces in
> struct drm_driver. This patch replaces the per-driver callbacks with
> per-instance callbacks in vgem. The only exception is gem_prime_mmap,
> which is non-trivial to convert.
> 
> Signed-off-by: Thomas Zimmermann 

Thanks here again.

This drv file is little tumultuous to me.
I mean, I took a while to sort functions in my head.

However, finally, I got it, and the change looks good.

Reviewed-by: Melissa Wen 

> ---
>  drivers/gpu/drm/vgem/vgem_drv.c | 21 ++---
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
> index cb884c890065..fa54a6d1403d 100644
> --- a/drivers/gpu/drm/vgem/vgem_drv.c
> +++ b/drivers/gpu/drm/vgem/vgem_drv.c
> @@ -50,6 +50,8 @@
>  #define DRIVER_MAJOR 1
>  #define DRIVER_MINOR 0
>  
> +static const struct drm_gem_object_funcs vgem_gem_object_funcs;
> +
>  static struct vgem_device {
>   struct drm_device drm;
>   struct platform_device *platform;
> @@ -167,6 +169,8 @@ static struct drm_vgem_gem_object 
> *__vgem_gem_create(struct drm_device *dev,
>   if (!obj)
>   return ERR_PTR(-ENOMEM);
>  
> + obj->base.funcs = _gem_object_funcs;
> +
>   ret = drm_gem_object_init(dev, >base, roundup(size, PAGE_SIZE));
>   if (ret) {
>   kfree(obj);
> @@ -401,12 +405,20 @@ static int vgem_prime_mmap(struct drm_gem_object *obj,
>   return 0;
>  }
>  
> +static const struct drm_gem_object_funcs vgem_gem_object_funcs = {
> + .free = vgem_gem_free_object,
> + .pin = vgem_prime_pin,
> + .unpin = vgem_prime_unpin,
> + .get_sg_table = vgem_prime_get_sg_table,
> + .vmap = vgem_prime_vmap,
> + .vunmap = vgem_prime_vunmap,
> + .vm_ops = _gem_vm_ops,
> +};
> +
>  static struct drm_driver vgem_driver = {
>   .driver_features= DRIVER_GEM | DRIVER_RENDER,
>   .open   = vgem_open,
>   .postclose  = vgem_postclose,
> - .gem_free_object_unlocked   = vgem_gem_free_object,
> - .gem_vm_ops = _gem_vm_ops,
>   .ioctls = vgem_ioctls,
>   .num_ioctls = ARRAY_SIZE(vgem_ioctls),
>   .fops   = _driver_fops,
> @@ -415,13 +427,8 @@ static struct drm_driver vgem_driver = {
>  
>   .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>   .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> - .gem_prime_pin = vgem_prime_pin,
> - .gem_prime_unpin = vgem_prime_unpin,
>   .gem_prime_import = vgem_prime_import,
>   .gem_prime_import_sg_table = vgem_prime_import_sg_table,
> - .gem_prime_get_sg_table = vgem_prime_get_sg_table,
> - .gem_prime_vmap = vgem_prime_vmap,
> - .gem_prime_vunmap = vgem_prime_vunmap,
>   .gem_prime_mmap = vgem_prime_mmap,
>  
>   .name   = DRIVER_NAME,
> -- 
> 2.28.0
> 
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 03/21] drm/etnaviv: Introduce GEM object functions

[Daniel Vetter]

On Tue, Sep 15, 2020 at 04:59:40PM +0200, Thomas Zimmermann wrote:
> GEM object functions deprecate several similar callback interfaces in
> struct drm_driver. This patch replaces the per-driver callbacks with
> per-instance callbacks in etnaviv. The only exception is gem_prime_mmap,
> which is non-trivial to convert.
> 
> Signed-off-by: Thomas Zimmermann 
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_drv.c | 13 -
>  drivers/gpu/drm/etnaviv/etnaviv_drv.h |  1 -
>  drivers/gpu/drm/etnaviv/etnaviv_gem.c | 19 ++-
>  3 files changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c 
> b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> index a9a3afaef9a1..aa270b79e585 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> @@ -468,12 +468,6 @@ static const struct drm_ioctl_desc etnaviv_ioctls[] = {
>   ETNA_IOCTL(PM_QUERY_SIG, pm_query_sig, DRM_RENDER_ALLOW),
>  };
>  
> -static const struct vm_operations_struct vm_ops = {
> - .fault = etnaviv_gem_fault,
> - .open = drm_gem_vm_open,
> - .close = drm_gem_vm_close,
> -};
> -
>  static const struct file_operations fops = {
>   .owner  = THIS_MODULE,
>   .open   = drm_open,
> @@ -490,16 +484,9 @@ static struct drm_driver etnaviv_drm_driver = {
>   .driver_features= DRIVER_GEM | DRIVER_RENDER,
>   .open   = etnaviv_open,
>   .postclose   = etnaviv_postclose,
> - .gem_free_object_unlocked = etnaviv_gem_free_object,
> - .gem_vm_ops = _ops,
>   .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>   .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> - .gem_prime_pin  = etnaviv_gem_prime_pin,
> - .gem_prime_unpin= etnaviv_gem_prime_unpin,
> - .gem_prime_get_sg_table = etnaviv_gem_prime_get_sg_table,
>   .gem_prime_import_sg_table = etnaviv_gem_prime_import_sg_table,
> - .gem_prime_vmap = etnaviv_gem_prime_vmap,
> - .gem_prime_vunmap   = etnaviv_gem_prime_vunmap,
>   .gem_prime_mmap = etnaviv_gem_prime_mmap,
>  #ifdef CONFIG_DEBUG_FS
>   .debugfs_init   = etnaviv_debugfs_init,
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h 
> b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> index 4d8dc9236e5f..914f0867ff71 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> @@ -49,7 +49,6 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void 
> *data,
>   struct drm_file *file);
>  
>  int etnaviv_gem_mmap(struct file *filp, struct vm_area_struct *vma);
> -vm_fault_t etnaviv_gem_fault(struct vm_fault *vmf);
>  int etnaviv_gem_mmap_offset(struct drm_gem_object *obj, u64 *offset);
>  struct sg_table *etnaviv_gem_prime_get_sg_table(struct drm_gem_object *obj);
>  void *etnaviv_gem_prime_vmap(struct drm_gem_object *obj);
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c 
> b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> index ea19f1d27275..312e9d58d5a7 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> @@ -171,7 +171,7 @@ int etnaviv_gem_mmap(struct file *filp, struct 
> vm_area_struct *vma)
>   return obj->ops->mmap(obj, vma);
>  }
>  
> -vm_fault_t etnaviv_gem_fault(struct vm_fault *vmf)
> +static vm_fault_t etnaviv_gem_fault(struct vm_fault *vmf)
>  {
>   struct vm_area_struct *vma = vmf->vma;
>   struct drm_gem_object *obj = vma->vm_private_data;
> @@ -561,6 +561,22 @@ void etnaviv_gem_obj_add(struct drm_device *dev, struct 
> drm_gem_object *obj)
>   mutex_unlock(>gem_lock);
>  }
>  
> +static const struct vm_operations_struct vm_ops = {
> + .fault = etnaviv_gem_fault,
> + .open = drm_gem_vm_open,
> + .close = drm_gem_vm_close,
> +};
> +
> +static const struct drm_gem_object_funcs etnaviv_gem_object_funcs = {
> + .free = etnaviv_gem_free_object,
> + .pin = etnaviv_gem_prime_pin,
> + .unpin = etnaviv_gem_prime_unpin,
> + .get_sg_table = etnaviv_gem_prime_get_sg_table,
> + .vmap = etnaviv_gem_prime_vmap,
> + .vunmap = etnaviv_gem_prime_vunmap,
> + .vm_ops = _ops,
> +};

Reviewed-by: Daniel Vetter 

> +
>  static int etnaviv_gem_new_impl(struct drm_device *dev, u32 size, u32 flags,
>   const struct etnaviv_gem_ops *ops, struct drm_gem_object **obj)
>  {
> @@ -595,6 +611,7 @@ static int etnaviv_gem_new_impl(struct drm_device *dev, 
> u32 size, u32 flags,
>   INIT_LIST_HEAD(_obj->vram_list);
>  
>   *obj = _obj->base;
> + (*obj)->funcs = _gem_object_funcs;
>  
>   return 0;
>  }
> -- 
> 2.28.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 08/21] drm/msm: Introduce GEM object funcs

[Daniel Vetter]

On Tue, Sep 15, 2020 at 04:59:45PM +0200, Thomas Zimmermann wrote:
> GEM object functions deprecate several similar callback interfaces in
> struct drm_driver. This patch replaces the per-driver callbacks with
> per-instance callbacks in msm. The only exception is gem_prime_mmap,
> which is non-trivial to convert.
> 
> Signed-off-by: Thomas Zimmermann 

Reviewed-by: Daniel Vetter 

> ---
>  drivers/gpu/drm/msm/msm_drv.c | 13 -
>  drivers/gpu/drm/msm/msm_drv.h |  1 -
>  drivers/gpu/drm/msm/msm_gem.c | 19 ++-
>  3 files changed, 18 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 79333842f70a..5952767ea478 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -978,12 +978,6 @@ static const struct drm_ioctl_desc msm_ioctls[] = {
>   DRM_IOCTL_DEF_DRV(MSM_SUBMITQUEUE_QUERY, msm_ioctl_submitqueue_query, 
> DRM_RENDER_ALLOW),
>  };
>  
> -static const struct vm_operations_struct vm_ops = {
> - .fault = msm_gem_fault,
> - .open = drm_gem_vm_open,
> - .close = drm_gem_vm_close,
> -};
> -
>  static const struct file_operations fops = {
>   .owner  = THIS_MODULE,
>   .open   = drm_open,
> @@ -1009,18 +1003,11 @@ static struct drm_driver msm_driver = {
>   .irq_preinstall = msm_irq_preinstall,
>   .irq_postinstall= msm_irq_postinstall,
>   .irq_uninstall  = msm_irq_uninstall,
> - .gem_free_object_unlocked = msm_gem_free_object,
> - .gem_vm_ops = _ops,
>   .dumb_create= msm_gem_dumb_create,
>   .dumb_map_offset= msm_gem_dumb_map_offset,
>   .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>   .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> - .gem_prime_pin  = msm_gem_prime_pin,
> - .gem_prime_unpin= msm_gem_prime_unpin,
> - .gem_prime_get_sg_table = msm_gem_prime_get_sg_table,
>   .gem_prime_import_sg_table = msm_gem_prime_import_sg_table,
> - .gem_prime_vmap = msm_gem_prime_vmap,
> - .gem_prime_vunmap   = msm_gem_prime_vunmap,
>   .gem_prime_mmap = msm_gem_prime_mmap,
>  #ifdef CONFIG_DEBUG_FS
>   .debugfs_init   = msm_debugfs_init,
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index af259b0573ea..7bcea10be81f 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -269,7 +269,6 @@ void msm_gem_shrinker_cleanup(struct drm_device *dev);
>  int msm_gem_mmap_obj(struct drm_gem_object *obj,
>   struct vm_area_struct *vma);
>  int msm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
> -vm_fault_t msm_gem_fault(struct vm_fault *vmf);
>  uint64_t msm_gem_mmap_offset(struct drm_gem_object *obj);
>  int msm_gem_get_iova(struct drm_gem_object *obj,
>   struct msm_gem_address_space *aspace, uint64_t *iova);
> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index b4553caaa196..de915ff6f4b4 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -247,7 +247,7 @@ int msm_gem_mmap(struct file *filp, struct vm_area_struct 
> *vma)
>   return msm_gem_mmap_obj(vma->vm_private_data, vma);
>  }
>  
> -vm_fault_t msm_gem_fault(struct vm_fault *vmf)
> +static vm_fault_t msm_gem_fault(struct vm_fault *vmf)
>  {
>   struct vm_area_struct *vma = vmf->vma;
>   struct drm_gem_object *obj = vma->vm_private_data;
> @@ -994,6 +994,22 @@ int msm_gem_new_handle(struct drm_device *dev, struct 
> drm_file *file,
>   return ret;
>  }
>  
> +static const struct vm_operations_struct vm_ops = {
> + .fault = msm_gem_fault,
> + .open = drm_gem_vm_open,
> + .close = drm_gem_vm_close,
> +};
> +
> +static const struct drm_gem_object_funcs msm_gem_object_funcs = {
> + .free = msm_gem_free_object,
> + .pin = msm_gem_prime_pin,
> + .unpin = msm_gem_prime_unpin,
> + .get_sg_table = msm_gem_prime_get_sg_table,
> + .vmap = msm_gem_prime_vmap,
> + .vunmap = msm_gem_prime_vunmap,
> + .vm_ops = _ops,
> +};
> +
>  static int msm_gem_new_impl(struct drm_device *dev,
>   uint32_t size, uint32_t flags,
>   struct drm_gem_object **obj)
> @@ -1024,6 +1040,7 @@ static int msm_gem_new_impl(struct drm_device *dev,
>   INIT_LIST_HEAD(_obj->vmas);
>  
>   *obj = _obj->base;
> + (*obj)->funcs = _gem_object_funcs;
>  
>   return 0;
>  }
> -- 
> 2.28.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 05/21] drm/gma500: Introduce GEM object functions

[Daniel Vetter]

On Tue, Sep 15, 2020 at 04:59:42PM +0200, Thomas Zimmermann wrote:
> GEM object functions deprecate several similar callback interfaces in
> struct drm_driver. This patch replaces the per-driver callbacks with
> per-instance callbacks in gma500.
> 
> Signed-off-by: Thomas Zimmermann 

Reviewed-by: Daniel Vetter 

> ---
>  drivers/gpu/drm/gma500/framebuffer.c |  2 ++
>  drivers/gpu/drm/gma500/gem.c | 18 --
>  drivers/gpu/drm/gma500/gem.h |  3 +++
>  drivers/gpu/drm/gma500/psb_drv.c |  9 -
>  drivers/gpu/drm/gma500/psb_drv.h |  2 --
>  5 files changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/gma500/framebuffer.c 
> b/drivers/gpu/drm/gma500/framebuffer.c
> index 54d9876b5305..5ede24fb44ae 100644
> --- a/drivers/gpu/drm/gma500/framebuffer.c
> +++ b/drivers/gpu/drm/gma500/framebuffer.c
> @@ -24,6 +24,7 @@
>  #include 
>  
>  #include "framebuffer.h"
> +#include "gem.h"
>  #include "gtt.h"
>  #include "psb_drv.h"
>  #include "psb_intel_drv.h"
> @@ -285,6 +286,7 @@ static struct gtt_range *psbfb_alloc(struct drm_device 
> *dev, int aligned_size)
>   /* Begin by trying to use stolen memory backing */
>   backing = psb_gtt_alloc_range(dev, aligned_size, "fb", 1, PAGE_SIZE);
>   if (backing) {
> + backing->gem.funcs = _gem_object_funcs;
>   drm_gem_private_object_init(dev, >gem, aligned_size);
>   return backing;
>   }
> diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
> index f9c4b1d76f56..8f07de83b6fb 100644
> --- a/drivers/gpu/drm/gma500/gem.c
> +++ b/drivers/gpu/drm/gma500/gem.c
> @@ -18,7 +18,9 @@
>  
>  #include "psb_drv.h"
>  
> -void psb_gem_free_object(struct drm_gem_object *obj)
> +static vm_fault_t psb_gem_fault(struct vm_fault *vmf);
> +
> +static void psb_gem_free_object(struct drm_gem_object *obj)
>  {
>   struct gtt_range *gtt = container_of(obj, struct gtt_range, gem);
>  
> @@ -36,6 +38,17 @@ int psb_gem_get_aperture(struct drm_device *dev, void 
> *data,
>   return -EINVAL;
>  }
>  
> +static const struct vm_operations_struct psb_gem_vm_ops = {
> + .fault = psb_gem_fault,
> + .open = drm_gem_vm_open,
> + .close = drm_gem_vm_close,
> +};
> +
> +const struct drm_gem_object_funcs psb_gem_object_funcs = {
> + .free = psb_gem_free_object,
> + .vm_ops = _gem_vm_ops,
> +};
> +
>  /**
>   *   psb_gem_create  -   create a mappable object
>   *   @file: the DRM file of the client
> @@ -63,6 +76,7 @@ int psb_gem_create(struct drm_file *file, struct drm_device 
> *dev, u64 size,
>   dev_err(dev->dev, "no memory for %lld byte GEM object\n", size);
>   return -ENOSPC;
>   }
> + r->gem.funcs = _gem_object_funcs;
>   /* Initialize the extra goodies GEM needs to do all the hard work */
>   if (drm_gem_object_init(dev, >gem, size) != 0) {
>   psb_gtt_free_range(dev, r);
> @@ -123,7 +137,7 @@ int psb_gem_dumb_create(struct drm_file *file, struct 
> drm_device *dev,
>   *   vma->vm_private_data points to the GEM object that is backing this
>   *   mapping.
>   */
> -vm_fault_t psb_gem_fault(struct vm_fault *vmf)
> +static vm_fault_t psb_gem_fault(struct vm_fault *vmf)
>  {
>   struct vm_area_struct *vma = vmf->vma;
>   struct drm_gem_object *obj;
> diff --git a/drivers/gpu/drm/gma500/gem.h b/drivers/gpu/drm/gma500/gem.h
> index 4a74dc623b6b..3741a711b9fd 100644
> --- a/drivers/gpu/drm/gma500/gem.h
> +++ b/drivers/gpu/drm/gma500/gem.h
> @@ -8,6 +8,9 @@
>  #ifndef _GEM_H
>  #define _GEM_H
>  
> +extern const struct drm_gem_object_funcs psb_gem_object_funcs;
> +
>  extern int psb_gem_create(struct drm_file *file, struct drm_device *dev,
> u64 size, u32 *handlep, int stolen, u32 align);
> +
>  #endif
> diff --git a/drivers/gpu/drm/gma500/psb_drv.c 
> b/drivers/gpu/drm/gma500/psb_drv.c
> index 34b4aae9a15e..b13376a6fb91 100644
> --- a/drivers/gpu/drm/gma500/psb_drv.c
> +++ b/drivers/gpu/drm/gma500/psb_drv.c
> @@ -480,12 +480,6 @@ static const struct dev_pm_ops psb_pm_ops = {
>   .runtime_idle = psb_runtime_idle,
>  };
>  
> -static const struct vm_operations_struct psb_gem_vm_ops = {
> - .fault = psb_gem_fault,
> - .open = drm_gem_vm_open,
> - .close = drm_gem_vm_close,
> -};
> -
>  static const struct file_operations psb_gem_fops = {
>   .owner = THIS_MODULE,
>   .open = drm_open,
> @@ -507,9 +501,6 @@ static struct drm_driver driver = {
>   .irq_uninstall = psb_irq_uninstall,
>   .irq_handler = psb_irq_handler,
>  
> - .gem_free_object_unlocked = psb_gem_free_object,
> - .gem_vm_ops = _gem_vm_ops,
> -
>   .dumb_create = psb_gem_dumb_create,
>   .ioctls = psb_ioctls,
>   .fops = _gem_fops,
> diff --git a/drivers/gpu/drm/gma500/psb_drv.h 
> b/drivers/gpu/drm/gma500/psb_drv.h
> index 956926341316..c71a5a4e912c 100644
> --- a/drivers/gpu/drm/gma500/psb_drv.h
> +++ b/drivers/gpu/drm/gma500/psb_drv.h
> 

Re: [Nouveau] [PATCH v2 18/21] drm/vkms: Introduce GEM object functions

[Melissa Wen]

Hi Thomas,

On 09/15, Thomas Zimmermann wrote:
> GEM object functions deprecate several similar callback interfaces in
> struct drm_driver. This patch replaces the per-driver callbacks with
> per-instance callbacks in vkms.
> 
> Signed-off-by: Thomas Zimmermann 

Thanks! Looks fine.

Reviewed-by: Melissa Wen 

> ---
>  drivers/gpu/drm/vkms/vkms_drv.c |  8 
>  drivers/gpu/drm/vkms/vkms_gem.c | 13 +
>  2 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
> index cb0b6230c22c..726801ab44d4 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -51,12 +51,6 @@ static const struct file_operations vkms_driver_fops = {
>   .release= drm_release,
>  };
>  
> -static const struct vm_operations_struct vkms_gem_vm_ops = {
> - .fault = vkms_gem_fault,
> - .open = drm_gem_vm_open,
> - .close = drm_gem_vm_close,
> -};
> -
>  static void vkms_release(struct drm_device *dev)
>  {
>   struct vkms_device *vkms = container_of(dev, struct vkms_device, drm);
> @@ -98,8 +92,6 @@ static struct drm_driver vkms_driver = {
>   .release= vkms_release,
>   .fops   = _driver_fops,
>   .dumb_create= vkms_dumb_create,
> - .gem_vm_ops = _gem_vm_ops,
> - .gem_free_object_unlocked = vkms_gem_free_object,
>   .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>   .gem_prime_import_sg_table = vkms_prime_import_sg_table,
>  
> diff --git a/drivers/gpu/drm/vkms/vkms_gem.c b/drivers/gpu/drm/vkms/vkms_gem.c
> index a017fc59905e..19a0e260a4df 100644
> --- a/drivers/gpu/drm/vkms/vkms_gem.c
> +++ b/drivers/gpu/drm/vkms/vkms_gem.c
> @@ -7,6 +7,17 @@
>  
>  #include "vkms_drv.h"
>  
> +static const struct vm_operations_struct vkms_gem_vm_ops = {
> + .fault = vkms_gem_fault,
> + .open = drm_gem_vm_open,
> + .close = drm_gem_vm_close,
> +};
> +
> +static const struct drm_gem_object_funcs vkms_gem_object_funcs = {
> + .free = vkms_gem_free_object,
> + .vm_ops = _gem_vm_ops,
> +};
> +
>  static struct vkms_gem_object *__vkms_gem_create(struct drm_device *dev,
>u64 size)
>  {
> @@ -17,6 +28,8 @@ static struct vkms_gem_object *__vkms_gem_create(struct 
> drm_device *dev,
>   if (!obj)
>   return ERR_PTR(-ENOMEM);
>  
> + obj->gem.funcs = _gem_object_funcs;
> +
>   size = roundup(size, PAGE_SIZE);
>   ret = drm_gem_object_init(dev, >gem, size);
>   if (ret) {
> -- 
> 2.28.0
> 
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 00/21] Convert all remaining drivers to GEM object functions

[Thomas Zimmermann]

Hi

Am 15.09.20 um 17:25 schrieb Christian König:
> Added my rb to the amdgpu and radeon patches.
> 
> Should we pick those up through the amd branches or do you want to push
> everything to drm-misc-next?
> 
> I think the later since this should result in much merge clash.

Yes, preferable, I'd merge it all through drm-misc.

Best regards
Thomas

> 
> Christian.
> 
> Am 15.09.20 um 16:59 schrieb Thomas Zimmermann:
>> The GEM and PRIME related callbacks in struct drm_driver are
>> deprecated in
>> favor of GEM object functions in struct drm_gem_object_funcs. This
>> patchset
>> converts the remaining drivers to object functions and removes most of
>> the
>> obsolete interfaces.
>>
>> Patches #1 to #16 and #18 to #19 convert DRM drivers to GEM object
>> functions,
>> one by one. Each patch moves existing callbacks from struct drm_driver
>> to an
>> instance of struct drm_gem_object_funcs, and sets these funcs when the
>> GEM
>> object is initialized. The expection is .gem_prime_mmap. There are
>> different
>> ways of how drivers implement the callback, and moving it to GEM object
>> functions requires a closer review for each.
>>
>> Patch #17 fixes virtgpu to use GEM object functions where possible. The
>> driver recently introduced a function for one of the deprecated
>> callbacks.
>>
>> Patch #20 converts xlnx to CMA helper macros. There's no apparent reason
>> why the driver does the GEM setup on it's own. Using CMA helper macros
>> adds GEM object functions implicitly.
>>
>> With most of the GEM and PRIME moved to GEM object functions, related
>> code
>> in struct drm_driver and in the DRM core/helpers is being removed by
>> patch
>> #21.
>>
>> Further testing is welcome. I tested the drivers for which I have HW
>> available. These are gma500, i915, nouveau, radeon and vc4. The console,
>> Weston and Xorg apparently work with the patches applied.
>>
>> v2:
>> * moved code in amdgpu and radeon
>> * made several functions static in various drivers
>> * updated TODO-list item
>> * fix virtgpu
>>
>> Thomas Zimmermann (21):
>>    drm/amdgpu: Introduce GEM object functions
>>    drm/armada: Introduce GEM object functions
>>    drm/etnaviv: Introduce GEM object functions
>>    drm/exynos: Introduce GEM object functions
>>    drm/gma500: Introduce GEM object functions
>>    drm/i915: Introduce GEM object functions
>>    drm/mediatek: Introduce GEM object functions
>>    drm/msm: Introduce GEM object funcs
>>    drm/nouveau: Introduce GEM object functions
>>    drm/omapdrm: Introduce GEM object functions
>>    drm/pl111: Introduce GEM object functions
>>    drm/radeon: Introduce GEM object functions
>>    drm/rockchip: Convert to drm_gem_object_funcs
>>    drm/tegra: Introduce GEM object functions
>>    drm/vc4: Introduce GEM object functions
>>    drm/vgem: Introduce GEM object functions
>>    drm/virtgpu: Set PRIME export function in struct drm_gem_object_funcs
>>    drm/vkms: Introduce GEM object functions
>>    drm/xen: Introduce GEM object functions
>>    drm/xlnx: Initialize DRM driver instance with CMA helper macro
>>    drm: Remove obsolete GEM and PRIME callbacks from struct drm_driver
>>
>>   Documentation/gpu/todo.rst    |  7 +-
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c   |  6 --
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c   | 23 +++--
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h   |  5 --
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c    |  1 +
>>   drivers/gpu/drm/armada/armada_drv.c   |  3 -
>>   drivers/gpu/drm/armada/armada_gem.c   | 12 ++-
>>   drivers/gpu/drm/armada/armada_gem.h   |  2 -
>>   drivers/gpu/drm/drm_gem.c | 35 ++--
>>   drivers/gpu/drm/drm_gem_cma_helper.c  |  6 +-
>>   drivers/gpu/drm/drm_prime.c   | 17 ++--
>>   drivers/gpu/drm/etnaviv/etnaviv_drv.c | 13 ---
>>   drivers/gpu/drm/etnaviv/etnaviv_drv.h |  1 -
>>   drivers/gpu/drm/etnaviv/etnaviv_gem.c | 19 -
>>   drivers/gpu/drm/exynos/exynos_drm_drv.c   | 10 ---
>>   drivers/gpu/drm/exynos/exynos_drm_gem.c   | 15 
>>   drivers/gpu/drm/gma500/framebuffer.c  |  2 +
>>   drivers/gpu/drm/gma500/gem.c  | 18 +++-
>>   drivers/gpu/drm/gma500/gem.h  |  3 +
>>   drivers/gpu/drm/gma500/psb_drv.c  |  9 --
>>   drivers/gpu/drm/gma500/psb_drv.h  |  2 -
>>   drivers/gpu/drm/i915/gem/i915_gem_object.c    | 21 -
>>   drivers/gpu/drm/i915/gem/i915_gem_object.h    |  3 -
>>   drivers/gpu/drm/i915/i915_drv.c   |  4 -
>>   .../gpu/drm/i915/selftests/mock_gem_device.c  |  3 -
>>   drivers/gpu/drm/mediatek/mtk_drm_drv.c    |  5 --
>>   drivers/gpu/drm/mediatek/mtk_drm_gem.c    | 11 +++
>>   drivers/gpu/drm/msm/msm_drv.c | 13 ---
>>   drivers/gpu/drm/msm/msm_drv.h |  1 -
>>   drivers/gpu/drm/msm/msm_gem.c | 19 -
>>   drivers/gpu/drm/nouveau/nouveau_drm.c

Re: [Nouveau] [oss-drivers] [trivial PATCH] treewide: Convert switch/case fallthrough; to break;

[Simon Horman]

On Wed, Sep 09, 2020 at 01:06:39PM -0700, Joe Perches wrote:
> fallthrough to a separate case/default label break; isn't very readable.
> 
> Convert pseudo-keyword fallthrough; statements to a simple break; when
> the next label is case or default and the only statement in the next
> label block is break;
> 
> Found using:
> 
> $ grep-2.5.4 -rP --include=*.[ch] -n 
> "fallthrough;(\s*(case\s+\w+|default)\s*:\s*){1,7}break;" *
> 
> Miscellanea:
> 
> o Move or coalesce a couple label blocks above a default: block.
> 
> Signed-off-by: Joe Perches 

...

> diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c 
> b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c
> index 252fe06f58aa..1d5b87079104 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp6000_pcie.c
> @@ -345,7 +345,7 @@ static int matching_bar(struct nfp_bar *bar, u32 tgt, u32 
> act, u32 tok,
>   baract = NFP_CPP_ACTION_RW;
>   if (act == 0)
>   act = NFP_CPP_ACTION_RW;
> - fallthrough;
> + break;
>   case NFP_PCIE_BAR_PCIE2CPP_MapType_FIXED:
>   break;
>   default:

This is a cascading fall-through handling all map types.
I don't think this change improves readability.

...
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 04/21] drm/exynos: Introduce GEM object functions

[Daniel Vetter]

On Wed, Sep 16, 2020 at 12:36:28PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 16.09.20 um 12:03 schrieb Daniel Vetter:
> > On Tue, Sep 15, 2020 at 04:59:41PM +0200, Thomas Zimmermann wrote:
> >> GEM object functions deprecate several similar callback interfaces in
> >> struct drm_driver. This patch replaces the per-driver callbacks with
> >> per-instance callbacks in exynos. The only exception is gem_prime_mmap,
> >> which is non-trivial to convert.
> >>
> >> Signed-off-by: Thomas Zimmermann 
> >> ---
> >>  drivers/gpu/drm/exynos/exynos_drm_drv.c | 10 --
> >>  drivers/gpu/drm/exynos/exynos_drm_gem.c | 15 +++
> >>  2 files changed, 15 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c 
> >> b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> >> index dbd80f1e4c78..fe46680ca208 100644
> >> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> >> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> >> @@ -75,11 +75,6 @@ static void exynos_drm_postclose(struct drm_device 
> >> *dev, struct drm_file *file)
> >>file->driver_priv = NULL;
> >>  }
> >>  
> >> -static const struct vm_operations_struct exynos_drm_gem_vm_ops = {
> >> -  .open = drm_gem_vm_open,
> >> -  .close = drm_gem_vm_close,
> >> -};
> >> -
> >>  static const struct drm_ioctl_desc exynos_ioctls[] = {
> >>DRM_IOCTL_DEF_DRV(EXYNOS_GEM_CREATE, exynos_drm_gem_create_ioctl,
> >>DRM_RENDER_ALLOW),
> >> @@ -124,16 +119,11 @@ static struct drm_driver exynos_drm_driver = {
> >>.open   = exynos_drm_open,
> >>.lastclose  = drm_fb_helper_lastclose,
> >>.postclose  = exynos_drm_postclose,
> >> -  .gem_free_object_unlocked = exynos_drm_gem_free_object,
> >> -  .gem_vm_ops = _drm_gem_vm_ops,
> >>.dumb_create= exynos_drm_gem_dumb_create,
> >>.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
> >>.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> >>.gem_prime_import   = exynos_drm_gem_prime_import,
> >> -  .gem_prime_get_sg_table = exynos_drm_gem_prime_get_sg_table,
> >>.gem_prime_import_sg_table  = exynos_drm_gem_prime_import_sg_table,
> >> -  .gem_prime_vmap = exynos_drm_gem_prime_vmap,
> >> -  .gem_prime_vunmap   = exynos_drm_gem_prime_vunmap,
> >>.gem_prime_mmap = exynos_drm_gem_prime_mmap,
> >>.ioctls = exynos_ioctls,
> >>.num_ioctls = ARRAY_SIZE(exynos_ioctls),
> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c 
> >> b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> >> index efa476858db5..69a5cf28b4ae 100644
> >> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> >> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> >> @@ -129,6 +129,19 @@ void exynos_drm_gem_destroy(struct exynos_drm_gem 
> >> *exynos_gem)
> >>kfree(exynos_gem);
> >>  }
> >>  
> >> +static const struct vm_operations_struct exynos_drm_gem_vm_ops = {
> >> +  .open = drm_gem_vm_open,
> >> +  .close = drm_gem_vm_close,
> >> +};
> > 
> > Hm moving the drm_gem_cma_vm_ops into drm_gem.h or so and maybe calling
> > them drm_gem_simple_ops or so would remove a pile of these. But perhaps a
> > quick follow up series.
> 
> Good idea. Several interfaces use the term 'default' in their name, so
> something like drm_gem_default_vm_ops seems appropriate.

Default sounds like a fine naming choice too.

> BTW is there a reason why we have file operations like
> DEFINE_DRM_GEM_CMA_FOPS() in each module? It seems like this could also
> be provided by the rsp memory-manager library.

It's for the module reference counting of the underlying file. So
file_operations need this.
-Daniel


> 
> Best regards
> Thomas
> 
> > 
> > Reviewed-by: Daniel Vetter 
> > 
> >> +
> >> +static const struct drm_gem_object_funcs exynos_drm_gem_object_funcs = {
> >> +  .free = exynos_drm_gem_free_object,
> >> +  .get_sg_table = exynos_drm_gem_prime_get_sg_table,
> >> +  .vmap = exynos_drm_gem_prime_vmap,
> >> +  .vunmap = exynos_drm_gem_prime_vunmap,
> >> +  .vm_ops = _drm_gem_vm_ops,
> >> +};
> >> +
> >>  static struct exynos_drm_gem *exynos_drm_gem_init(struct drm_device *dev,
> >>  unsigned long size)
> >>  {
> >> @@ -143,6 +156,8 @@ static struct exynos_drm_gem 
> >> *exynos_drm_gem_init(struct drm_device *dev,
> >>exynos_gem->size = size;
> >>obj = _gem->base;
> >>  
> >> +  obj->funcs = _drm_gem_object_funcs;
> >> +
> >>ret = drm_gem_object_init(dev, obj, size);
> >>if (ret < 0) {
> >>DRM_DEV_ERROR(dev->dev, "failed to initialize gem object\n");
> >> -- 
> >> 2.28.0
> >>
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
> 




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Nouveau 

Re: [Nouveau] [PATCH v2 21/21] drm: Remove obsolete GEM and PRIME callbacks from struct drm_driver

[Thomas Zimmermann]

Am 15.09.20 um 16:59 schrieb Thomas Zimmermann:
> Several GEM and PRIME callbacks have been deprecated in favor of
> per-instance GEM object functions. Remove the callbacks as they are
> now unused. The only exception is .gem_prime_mmap, which is still
> in use by several drivers.
> 
> What is also gone is gem_vm_ops in struct drm_driver. All drivers now
> use struct drm_gem_object_funcs.vm_ops instead.
> 
> While at it, the patch also improves error handling around calls
> to .free and .get_sg_table callbacks.
> 
> v2:
>   * update related TODO item (Sam)
> 
> Signed-off-by: Thomas Zimmermann 
> ---
>  Documentation/gpu/todo.rst   |  7 +--
>  drivers/gpu/drm/drm_gem.c| 35 +++-
>  drivers/gpu/drm/drm_gem_cma_helper.c |  6 +-
>  drivers/gpu/drm/drm_prime.c  | 17 +++---
>  include/drm/drm_drv.h| 85 ++--
>  5 files changed, 25 insertions(+), 125 deletions(-)
> 
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index b0ea17da8ff6..0fc6bc222392 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -289,11 +289,8 @@ struct drm_gem_object_funcs
>  ---
>  
>  GEM objects can now have a function table instead of having the callbacks on 
> the
> -DRM driver struct. This is now the preferred way and drivers can be moved 
> over.
> -
> -We also need a 2nd version of the CMA define that doesn't require the
> -vmapping to be present (different hook for prime importing). Plus this needs 
> to
> -be rolled out to all drivers using their own implementations, too.
> +DRM driver struct. This is now the preferred way. Callbacks in drivers have 
> been
> +converted, except for struct drm_driver.gem_prime_mmap.
>  
>  Level: Intermediate
>  
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 19d73868490e..96945bed8291 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -247,12 +247,9 @@ drm_gem_object_release_handle(int id, void *ptr, void 
> *data)
>  {
>   struct drm_file *file_priv = data;
>   struct drm_gem_object *obj = ptr;
> - struct drm_device *dev = obj->dev;
>  
>   if (obj->funcs && obj->funcs->close)
>   obj->funcs->close(obj, file_priv);
> - else if (dev->driver->gem_close_object)
> - dev->driver->gem_close_object(obj, file_priv);
>  
>   drm_gem_remove_prime_handles(obj, file_priv);
>   drm_vma_node_revoke(>vma_node, file_priv);
> @@ -407,10 +404,6 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
>   ret = obj->funcs->open(obj, file_priv);
>   if (ret)
>   goto err_revoke;
> - } else if (dev->driver->gem_open_object) {
> - ret = dev->driver->gem_open_object(obj, file_priv);
> - if (ret)
> - goto err_revoke;
>   }
>  
>   *handlep = handle;
> @@ -982,12 +975,11 @@ drm_gem_object_free(struct kref *kref)
>  {
>   struct drm_gem_object *obj =
>   container_of(kref, struct drm_gem_object, refcount);
> - struct drm_device *dev = obj->dev;
>  
> - if (obj->funcs)
> - obj->funcs->free(obj);
> - else if (dev->driver->gem_free_object_unlocked)
> - dev->driver->gem_free_object_unlocked(obj);
> + if (drm_WARN_ON_ONCE(obj->dev, !obj->funcs || !obj->funcs->free))
> + return;
> +
> + obj->funcs->free(obj);
>  }
>  EXPORT_SYMBOL(drm_gem_object_free);
>  
> @@ -1049,9 +1041,9 @@ EXPORT_SYMBOL(drm_gem_vm_close);
>   * @obj_size: the object size to be mapped, in bytes
>   * @vma: VMA for the area to be mapped
>   *
> - * Set up the VMA to prepare mapping of the GEM object using the gem_vm_ops
> - * provided by the driver. Depending on their requirements, drivers can 
> either
> - * provide a fault handler in their gem_vm_ops (in which case any accesses to
> + * Set up the VMA to prepare mapping of the GEM object using the GEM object's
> + * vm_ops. Depending on their requirements, GEM objects can either
> + * provide a fault handler in their vm_ops (in which case any accesses to
>   * the object will be trapped, to perform migration, GTT binding, surface
>   * register allocation, or performance monitoring), or mmap the buffer memory
>   * synchronously after calling drm_gem_mmap_obj.
> @@ -1065,12 +1057,11 @@ EXPORT_SYMBOL(drm_gem_vm_close);
>   * callers must verify access restrictions before calling this helper.
>   *
>   * Return 0 or success or -EINVAL if the object size is smaller than the VMA
> - * size, or if no gem_vm_ops are provided.
> + * size, or if no vm_ops are provided.
>   */
>  int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>struct vm_area_struct *vma)
>  {
> - struct drm_device *dev = obj->dev;
>   int ret;
>  
>   /* Check for valid size. */
> @@ -1095,8 +1086,6 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, 
> 

Re: [Nouveau] [PATCH] mm: remove extra ZONE_DEVICE struct page refcount

[Ralph Campbell]

On 9/15/20 10:36 PM, Christoph Hellwig wrote:

On Tue, Sep 15, 2020 at 09:39:47AM -0700, Ralph Campbell wrote:

I don't think any of the three ->page_free instances even cares about
the page refcount.


Not true. The page_free() callback records the page is free by setting
a bit or putting the page on a free list but when it allocates a free
device private struct page to be used with migrate_vma_setup(), it needs to
increment the refcount.

For the ZONE_DEVICE MEMORY_DEVICE_GENERIC and MEMORY_DEVICE_PCI_P2PDMA
struct pages, I think you are correct because they don't define page_free()
and from what I can see, don't decrement the page refcount to zero.


Umm, the whole point of ZONE_DEVICE is to have a struct page for
something that is not system memory.  For both the ppc kvm case (magic
hypervisor pool) and Noveau (device internal) memory that clear is the
case.  But looks like test_hmm uses normal pages to fake this up, so
I was wrong about the third caller.  But I think we can just call
set_page_count just before freeing the page there with a comment
explaining what is goin on.


Dan Williams thought that having the ZONE_DEVICE struct pages
be on a free list with a refcount of one was a bit strange and
that the driver should handle the zero to one transition.
But, that would mean a bit more invasive change to the 3 drivers
to set the reference count to zero after calling memremap_pages()
and setting the reference count to one when allocating a struct
page. What you are suggesting is what I also proposed in v1.
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 17/21] drm/virtgpu: Set PRIME export function in struct drm_gem_object_funcs

[Daniel Vetter]

On Tue, Sep 15, 2020 at 04:59:54PM +0200, Thomas Zimmermann wrote:
> GEM object functions deprecate several similar callback interfaces in
> struct drm_driver. This patch replaces virtgpu's per-driver PRIME export
> function with a per-object function.
> 
> Signed-off-by: Thomas Zimmermann 

Reviewed-by: Daniel Vetter 

> ---
>  drivers/gpu/drm/virtio/virtgpu_drv.c| 1 -
>  drivers/gpu/drm/virtio/virtgpu_object.c | 1 +
>  2 files changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c 
> b/drivers/gpu/drm/virtio/virtgpu_drv.c
> index b039f493bda9..1f8d6ed11d21 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> @@ -203,7 +203,6 @@ static struct drm_driver driver = {
>   .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>   .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>   .gem_prime_mmap = drm_gem_prime_mmap,
> - .gem_prime_export = virtgpu_gem_prime_export,
>   .gem_prime_import = virtgpu_gem_prime_import,
>   .gem_prime_import_sg_table = virtgpu_gem_prime_import_sg_table,
>  
> diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c 
> b/drivers/gpu/drm/virtio/virtgpu_object.c
> index 842f8b61aa89..4f7d7ea8194c 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_object.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_object.c
> @@ -108,6 +108,7 @@ static const struct drm_gem_object_funcs 
> virtio_gpu_shmem_funcs = {
>   .close = virtio_gpu_gem_object_close,
>  
>   .print_info = drm_gem_shmem_print_info,
> + .export = virtgpu_gem_prime_export,
>   .pin = drm_gem_shmem_pin,
>   .unpin = drm_gem_shmem_unpin,
>   .get_sg_table = drm_gem_shmem_get_sg_table,
> -- 
> 2.28.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 21/21] drm: Remove obsolete GEM and PRIME callbacks from struct drm_driver

[Daniel Vetter]

On Tue, Sep 15, 2020 at 04:59:58PM +0200, Thomas Zimmermann wrote:
> Several GEM and PRIME callbacks have been deprecated in favor of
> per-instance GEM object functions. Remove the callbacks as they are
> now unused. The only exception is .gem_prime_mmap, which is still
> in use by several drivers.
> 
> What is also gone is gem_vm_ops in struct drm_driver. All drivers now
> use struct drm_gem_object_funcs.vm_ops instead.
> 
> While at it, the patch also improves error handling around calls
> to .free and .get_sg_table callbacks.
> 
> v2:
>   * update related TODO item (Sam)
> 
> Signed-off-by: Thomas Zimmermann 

Nice work!

Acked-by: Daniel Vetter 

> ---
>  Documentation/gpu/todo.rst   |  7 +--
>  drivers/gpu/drm/drm_gem.c| 35 +++-
>  drivers/gpu/drm/drm_gem_cma_helper.c |  6 +-
>  drivers/gpu/drm/drm_prime.c  | 17 +++---
>  include/drm/drm_drv.h| 85 ++--
>  5 files changed, 25 insertions(+), 125 deletions(-)
> 
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index b0ea17da8ff6..0fc6bc222392 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -289,11 +289,8 @@ struct drm_gem_object_funcs
>  ---
>  
>  GEM objects can now have a function table instead of having the callbacks on 
> the
> -DRM driver struct. This is now the preferred way and drivers can be moved 
> over.
> -
> -We also need a 2nd version of the CMA define that doesn't require the
> -vmapping to be present (different hook for prime importing). Plus this needs 
> to
> -be rolled out to all drivers using their own implementations, too.
> +DRM driver struct. This is now the preferred way. Callbacks in drivers have 
> been
> +converted, except for struct drm_driver.gem_prime_mmap.
>  
>  Level: Intermediate
>  
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 19d73868490e..96945bed8291 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -247,12 +247,9 @@ drm_gem_object_release_handle(int id, void *ptr, void 
> *data)
>  {
>   struct drm_file *file_priv = data;
>   struct drm_gem_object *obj = ptr;
> - struct drm_device *dev = obj->dev;
>  
>   if (obj->funcs && obj->funcs->close)
>   obj->funcs->close(obj, file_priv);
> - else if (dev->driver->gem_close_object)
> - dev->driver->gem_close_object(obj, file_priv);
>  
>   drm_gem_remove_prime_handles(obj, file_priv);
>   drm_vma_node_revoke(>vma_node, file_priv);
> @@ -407,10 +404,6 @@ drm_gem_handle_create_tail(struct drm_file *file_priv,
>   ret = obj->funcs->open(obj, file_priv);
>   if (ret)
>   goto err_revoke;
> - } else if (dev->driver->gem_open_object) {
> - ret = dev->driver->gem_open_object(obj, file_priv);
> - if (ret)
> - goto err_revoke;
>   }
>  
>   *handlep = handle;
> @@ -982,12 +975,11 @@ drm_gem_object_free(struct kref *kref)
>  {
>   struct drm_gem_object *obj =
>   container_of(kref, struct drm_gem_object, refcount);
> - struct drm_device *dev = obj->dev;
>  
> - if (obj->funcs)
> - obj->funcs->free(obj);
> - else if (dev->driver->gem_free_object_unlocked)
> - dev->driver->gem_free_object_unlocked(obj);
> + if (drm_WARN_ON_ONCE(obj->dev, !obj->funcs || !obj->funcs->free))
> + return;
> +
> + obj->funcs->free(obj);
>  }
>  EXPORT_SYMBOL(drm_gem_object_free);
>  
> @@ -1049,9 +1041,9 @@ EXPORT_SYMBOL(drm_gem_vm_close);
>   * @obj_size: the object size to be mapped, in bytes
>   * @vma: VMA for the area to be mapped
>   *
> - * Set up the VMA to prepare mapping of the GEM object using the gem_vm_ops
> - * provided by the driver. Depending on their requirements, drivers can 
> either
> - * provide a fault handler in their gem_vm_ops (in which case any accesses to
> + * Set up the VMA to prepare mapping of the GEM object using the GEM object's
> + * vm_ops. Depending on their requirements, GEM objects can either
> + * provide a fault handler in their vm_ops (in which case any accesses to
>   * the object will be trapped, to perform migration, GTT binding, surface
>   * register allocation, or performance monitoring), or mmap the buffer memory
>   * synchronously after calling drm_gem_mmap_obj.
> @@ -1065,12 +1057,11 @@ EXPORT_SYMBOL(drm_gem_vm_close);
>   * callers must verify access restrictions before calling this helper.
>   *
>   * Return 0 or success or -EINVAL if the object size is smaller than the VMA
> - * size, or if no gem_vm_ops are provided.
> + * size, or if no vm_ops are provided.
>   */
>  int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
>struct vm_area_struct *vma)
>  {
> - struct drm_device *dev = obj->dev;
>   int ret;
>  
>   /* Check for valid size. */
> @@ -1095,8 +1086,6 @@ 

Re: [Nouveau] [trivial PATCH] treewide: Convert switch/case fallthrough; to break;

[Jacob Keller]

On 9/9/2020 1:55 PM, Keith Busch wrote:
> On Wed, Sep 09, 2020 at 01:06:39PM -0700, Joe Perches wrote:
>> diff --git a/crypto/tcrypt.c b/crypto/tcrypt.c
>> index eea0f453cfb6..8aac5bc60f4c 100644
>> --- a/crypto/tcrypt.c
>> +++ b/crypto/tcrypt.c
>> @@ -2464,7 +2464,7 @@ static int do_test(const char *alg, u32 type, u32 
>> mask, int m, u32 num_mb)
>>  test_hash_speed("streebog512", sec,
>>  generic_hash_speed_template);
>>  if (mode > 300 && mode < 400) break;
>> -fallthrough;
>> +break;
>>  case 399:
>>  break;
> 
> Just imho, this change makes the preceding 'if' look even more
> pointless. Maybe the fallthrough was a deliberate choice? Not that my
> opinion matters here as I don't know this module, but it looked a bit
> odd to me.
> 

Yea this does look very odd..
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 13/21] drm/rockchip: Convert to drm_gem_object_funcs

[Daniel Vetter]

On Tue, Sep 15, 2020 at 04:59:50PM +0200, Thomas Zimmermann wrote:
> GEM object functions deprecate several similar callback interfaces in
> struct drm_driver. This patch replaces the per-driver callbacks with
> per-instance callbacks in rockchip. The only exception is gem_prime_mmap,
> which is non-trivial to convert.
> 
> Signed-off-by: Thomas Zimmermann 

Reviewed-by: Daniel Vetter 
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_drv.c |  5 -
>  drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 10 ++
>  2 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c 
> b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> index 0f3eb392fe39..b7654f5e4225 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
> @@ -212,15 +212,10 @@ static const struct file_operations 
> rockchip_drm_driver_fops = {
>  static struct drm_driver rockchip_drm_driver = {
>   .driver_features= DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
>   .lastclose  = drm_fb_helper_lastclose,
> - .gem_vm_ops = _gem_cma_vm_ops,
> - .gem_free_object_unlocked = rockchip_gem_free_object,
>   .dumb_create= rockchip_gem_dumb_create,
>   .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>   .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> - .gem_prime_get_sg_table = rockchip_gem_prime_get_sg_table,
>   .gem_prime_import_sg_table  = rockchip_gem_prime_import_sg_table,
> - .gem_prime_vmap = rockchip_gem_prime_vmap,
> - .gem_prime_vunmap   = rockchip_gem_prime_vunmap,
>   .gem_prime_mmap = rockchip_gem_mmap_buf,
>   .fops   = _drm_driver_fops,
>   .name   = DRIVER_NAME,
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c 
> b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> index 0055d86576f7..bddc7d99efe3 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_gem.c
> @@ -296,6 +296,14 @@ static void rockchip_gem_release_object(struct 
> rockchip_gem_object *rk_obj)
>   kfree(rk_obj);
>  }
>  
> +static const struct drm_gem_object_funcs rockchip_gem_object_funcs = {
> + .free = rockchip_gem_free_object,
> + .get_sg_table = rockchip_gem_prime_get_sg_table,
> + .vmap = rockchip_gem_prime_vmap,
> + .vunmap = rockchip_gem_prime_vunmap,
> + .vm_ops = _gem_cma_vm_ops,
> +};
> +
>  static struct rockchip_gem_object *
>   rockchip_gem_alloc_object(struct drm_device *drm, unsigned int size)
>  {
> @@ -310,6 +318,8 @@ static struct rockchip_gem_object *
>  
>   obj = _obj->base;
>  
> + obj->funcs = _gem_object_funcs;
> +
>   drm_gem_object_init(drm, obj, size);
>  
>   return rk_obj;
> -- 
> 2.28.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 01/21] drm/amdgpu: Introduce GEM object functions

[Thomas Zimmermann]

Hi

Am 15.09.20 um 17:05 schrieb Christian König:
> Am 15.09.20 um 16:59 schrieb Thomas Zimmermann:
>> GEM object functions deprecate several similar callback interfaces in
>> struct drm_driver. This patch replaces the per-driver callbacks with
>> per-instance callbacks in amdgpu. The only exception is gem_prime_mmap,
>> which is non-trivial to convert.
>>
>> v2:
>> * move object-function instance to amdgpu_gem.c (Christian)
>> * set callbacks in amdgpu_gem_object_create() (Christian)
>>
>> Signed-off-by: Thomas Zimmermann 
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |  6 --
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c    | 23 +-
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h    |  5 -
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c |  1 +
>>   4 files changed, 19 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> index 6edde2b9e402..840ca8f9c1e1 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>> @@ -1505,19 +1505,13 @@ static struct drm_driver kms_driver = {
>>   .lastclose = amdgpu_driver_lastclose_kms,
>>   .irq_handler = amdgpu_irq_handler,
>>   .ioctls = amdgpu_ioctls_kms,
>> -    .gem_free_object_unlocked = amdgpu_gem_object_free,
>> -    .gem_open_object = amdgpu_gem_object_open,
>> -    .gem_close_object = amdgpu_gem_object_close,
>>   .dumb_create = amdgpu_mode_dumb_create,
>>   .dumb_map_offset = amdgpu_mode_dumb_mmap,
>>   .fops = _driver_kms_fops,
>>     .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>>   .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>> -    .gem_prime_export = amdgpu_gem_prime_export,
>>   .gem_prime_import = amdgpu_gem_prime_import,
>> -    .gem_prime_vmap = amdgpu_gem_prime_vmap,
>> -    .gem_prime_vunmap = amdgpu_gem_prime_vunmap,
>>   .gem_prime_mmap = amdgpu_gem_prime_mmap,
>>     .name = DRIVER_NAME,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index aa7f230c71bf..aeecd5dc3ce4 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -36,9 +36,12 @@
>>     #include "amdgpu.h"
>>   #include "amdgpu_display.h"
>> +#include "amdgpu_dma_buf.h"
>>   #include "amdgpu_xgmi.h"
>>   -void amdgpu_gem_object_free(struct drm_gem_object *gobj)
>> +static const struct drm_gem_object_funcs amdgpu_gem_object_funcs;
>> +
>> +static void amdgpu_gem_object_free(struct drm_gem_object *gobj)
>>   {
>>   struct amdgpu_bo *robj = gem_to_amdgpu_bo(gobj);
>>   @@ -87,6 +90,7 @@ int amdgpu_gem_object_create(struct amdgpu_device
>> *adev, unsigned long size,
>>   return r;
>>   }
>>   *obj = >tbo.base;
>> +    (*obj)->funcs = _gem_object_funcs;
>>     return 0;
>>   }
>> @@ -119,8 +123,8 @@ void amdgpu_gem_force_release(struct amdgpu_device
>> *adev)
>>    * Call from drm_gem_handle_create which appear in both new and open
>> ioctl
>>    * case.
>>    */
>> -int amdgpu_gem_object_open(struct drm_gem_object *obj,
>> -   struct drm_file *file_priv)
>> +static int amdgpu_gem_object_open(struct drm_gem_object *obj,
>> +  struct drm_file *file_priv)
>>   {
>>   struct amdgpu_bo *abo = gem_to_amdgpu_bo(obj);
>>   struct amdgpu_device *adev = amdgpu_ttm_adev(abo->tbo.bdev);
>> @@ -152,8 +156,8 @@ int amdgpu_gem_object_open(struct drm_gem_object
>> *obj,
>>   return 0;
>>   }
>>   -void amdgpu_gem_object_close(struct drm_gem_object *obj,
>> - struct drm_file *file_priv)
>> +static void amdgpu_gem_object_close(struct drm_gem_object *obj,
>> +    struct drm_file *file_priv)
>>   {
>>   struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
>>   struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
>> @@ -211,6 +215,15 @@ void amdgpu_gem_object_close(struct
>> drm_gem_object *obj,
>>   ttm_eu_backoff_reservation(, );
>>   }
>>   +static const struct drm_gem_object_funcs amdgpu_gem_object_funcs = {
>> +    .free = amdgpu_gem_object_free,
>> +    .open = amdgpu_gem_object_open,
>> +    .close = amdgpu_gem_object_close,
>> +    .export = amdgpu_gem_prime_export,
>> +    .vmap = amdgpu_gem_prime_vmap,
>> +    .vunmap = amdgpu_gem_prime_vunmap,
>> +};
>> +
>>   /*
>>    * GEM ioctls.
>>    */
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h
>> index e0f025dd1b14..637bf51dbf06 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h
>> @@ -33,11 +33,6 @@
>>   #define AMDGPU_GEM_DOMAIN_MAX    0x3
>>   #define gem_to_amdgpu_bo(gobj) container_of((gobj), struct
>> amdgpu_bo, tbo.base)
>>   -void amdgpu_gem_object_free(struct drm_gem_object *obj);
>> -int amdgpu_gem_object_open(struct drm_gem_object *obj,
>> -    struct drm_file *file_priv);
>> -void 

Re: [Nouveau] [PATCH v2 09/21] drm/nouveau: Introduce GEM object functions

[Daniel Vetter]

On Tue, Sep 15, 2020 at 04:59:46PM +0200, Thomas Zimmermann wrote:
> GEM object functions deprecate several similar callback interfaces in
> struct drm_driver. This patch replaces the per-driver callbacks with
> per-instance callbacks in nouveau.
> 
> Signed-off-by: Thomas Zimmermann 

Hm ttm and gem mmap world still quite disjoint ... Anyway that's an
entirely different thing.

Reviewed-by: Daniel Vetter 

> ---
>  drivers/gpu/drm/nouveau/nouveau_drm.c   |  9 -
>  drivers/gpu/drm/nouveau/nouveau_gem.c   | 13 +
>  drivers/gpu/drm/nouveau/nouveau_gem.h   |  2 ++
>  drivers/gpu/drm/nouveau/nouveau_prime.c |  2 ++
>  4 files changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c 
> b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 42fc5c813a9b..72640bca1617 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -1207,16 +1207,7 @@ driver_stub = {
>  
>   .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>   .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
> - .gem_prime_pin = nouveau_gem_prime_pin,
> - .gem_prime_unpin = nouveau_gem_prime_unpin,
> - .gem_prime_get_sg_table = nouveau_gem_prime_get_sg_table,
>   .gem_prime_import_sg_table = nouveau_gem_prime_import_sg_table,
> - .gem_prime_vmap = nouveau_gem_prime_vmap,
> - .gem_prime_vunmap = nouveau_gem_prime_vunmap,
> -
> - .gem_free_object_unlocked = nouveau_gem_object_del,
> - .gem_open_object = nouveau_gem_object_open,
> - .gem_close_object = nouveau_gem_object_close,
>  
>   .dumb_create = nouveau_display_dumb_create,
>   .dumb_map_offset = nouveau_display_dumb_map_offset,
> diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c 
> b/drivers/gpu/drm/nouveau/nouveau_gem.c
> index 89adadf4706b..28e0cbb00876 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_gem.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c
> @@ -169,6 +169,17 @@ nouveau_gem_object_close(struct drm_gem_object *gem, 
> struct drm_file *file_priv)
>   ttm_bo_unreserve(>bo);
>  }
>  
> +const struct drm_gem_object_funcs nouveau_gem_object_funcs = {
> + .free = nouveau_gem_object_del,
> + .open = nouveau_gem_object_open,
> + .close = nouveau_gem_object_close,
> + .pin = nouveau_gem_prime_pin,
> + .unpin = nouveau_gem_prime_unpin,
> + .get_sg_table = nouveau_gem_prime_get_sg_table,
> + .vmap = nouveau_gem_prime_vmap,
> + .vunmap = nouveau_gem_prime_vunmap,
> +};
> +
>  int
>  nouveau_gem_new(struct nouveau_cli *cli, u64 size, int align, uint32_t 
> domain,
>   uint32_t tile_mode, uint32_t tile_flags,
> @@ -186,6 +197,8 @@ nouveau_gem_new(struct nouveau_cli *cli, u64 size, int 
> align, uint32_t domain,
>   if (IS_ERR(nvbo))
>   return PTR_ERR(nvbo);
>  
> + nvbo->bo.base.funcs = _gem_object_funcs;
> +
>   /* Initialize the embedded gem-object. We return a single gem-reference
>* to the caller, instead of a normal nouveau_bo ttm reference. */
>   ret = drm_gem_object_init(drm->dev, >bo.base, size);
> diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.h 
> b/drivers/gpu/drm/nouveau/nouveau_gem.h
> index 978e07591990..b35c180322e2 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_gem.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_gem.h
> @@ -5,6 +5,8 @@
>  #include "nouveau_drv.h"
>  #include "nouveau_bo.h"
>  
> +extern const struct drm_gem_object_funcs nouveau_gem_object_funcs;
> +
>  static inline struct nouveau_bo *
>  nouveau_gem_object(struct drm_gem_object *gem)
>  {
> diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c 
> b/drivers/gpu/drm/nouveau/nouveau_prime.c
> index b2ecb91f8ddc..a8264aebf3d4 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_prime.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c
> @@ -77,6 +77,8 @@ struct drm_gem_object 
> *nouveau_gem_prime_import_sg_table(struct drm_device *dev,
>  
>   nvbo->valid_domains = NOUVEAU_GEM_DOMAIN_GART;
>  
> + nvbo->bo.base.funcs = _gem_object_funcs;
> +
>   /* Initialize the embedded gem-object. We return a single gem-reference
>* to the caller, instead of a normal nouveau_bo ttm reference. */
>   ret = drm_gem_object_init(dev, >bo.base, size);
> -- 
> 2.28.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH] mm: remove extra ZONE_DEVICE struct page refcount

[Christoph Hellwig]

> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 517751310dd2..5a82037a4b26 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1093,34 +1093,6 @@ static inline bool is_zone_device_page(const struct 
> page *page)
>  #ifdef CONFIG_DEV_PAGEMAP_OPS
>  void free_devmap_managed_page(struct page *page);
>  DECLARE_STATIC_KEY_FALSE(devmap_managed_key);

The export for devmap_managed_key can be dropped now.  In fact I think
we can remove devmap_managed_key entirely now - it is only checked in
the actual page free path instead of for each refcount manipulation,
so a good old unlikely is probably enough.

Also free_devmap_managed_page can move to mm/internal.h.

> +#ifdef CONFIG_DEV_PAGEMAP_OPS
> +static void __put_devmap_managed_page(struct page *page)
> +{
> + if (!static_branch_unlikely(_managed_key))
> + return;
> +
> + switch (page->pgmap->type) {
> + case MEMORY_DEVICE_PRIVATE:
> + case MEMORY_DEVICE_FS_DAX:
> + free_devmap_managed_page(page);
> + break;
> + default:
> + break;
> + }
> +}
> +#else
> +static inline void __put_devmap_managed_page(struct page *page)
> +{
> +}
> +#endif

I think this should be moved to mm/memremap.c or even better
actually be folded into free_devmap_managed_page, which would need
a new name like free_zone_device_page().

Something like this incremental patch:


diff --git a/include/linux/mm.h b/include/linux/mm.h
index 7bb9e93cf86cde..29350dc27cd0cd 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1090,11 +1090,6 @@ static inline bool is_zone_device_page(const struct page 
*page)
 }
 #endif
 
-#ifdef CONFIG_DEV_PAGEMAP_OPS
-void free_devmap_managed_page(struct page *page);
-DECLARE_STATIC_KEY_FALSE(devmap_managed_key);
-#endif /* CONFIG_DEV_PAGEMAP_OPS */
-
 static inline bool is_device_private_page(const struct page *page)
 {
return IS_ENABLED(CONFIG_DEV_PAGEMAP_OPS) &&
diff --git a/mm/internal.h b/mm/internal.h
index 6345b08ce86ccf..629959a6f26d7c 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -618,4 +618,12 @@ struct migration_target_control {
gfp_t gfp_mask;
 };
 
+#ifdef CONFIG_DEV_PAGEMAP_OPS
+void free_zone_device_page(struct page *page);
+#else
+static inline void free_zone_device_page(struct page *page)
+{
+}
+#endif
+
 #endif /* __MM_INTERNAL_H */
diff --git a/mm/memremap.c b/mm/memremap.c
index d549e3733f4098..b15ad2264a4f1c 100644
--- a/mm/memremap.c
+++ b/mm/memremap.c
@@ -12,6 +12,7 @@
 #include 
 #include 
 #include 
+#include "internal.h"
 
 static DEFINE_XARRAY(pgmap_array);
 
@@ -37,36 +38,6 @@ unsigned long memremap_compat_align(void)
 EXPORT_SYMBOL_GPL(memremap_compat_align);
 #endif
 
-#ifdef CONFIG_DEV_PAGEMAP_OPS
-DEFINE_STATIC_KEY_FALSE(devmap_managed_key);
-EXPORT_SYMBOL(devmap_managed_key);
-
-static void devmap_managed_enable_put(void)
-{
-   static_branch_dec(_managed_key);
-}
-
-static int devmap_managed_enable_get(struct dev_pagemap *pgmap)
-{
-   if (pgmap->type == MEMORY_DEVICE_PRIVATE &&
-   (!pgmap->ops || !pgmap->ops->page_free)) {
-   WARN(1, "Missing page_free method\n");
-   return -EINVAL;
-   }
-
-   static_branch_inc(_managed_key);
-   return 0;
-}
-#else
-static int devmap_managed_enable_get(struct dev_pagemap *pgmap)
-{
-   return -EINVAL;
-}
-static void devmap_managed_enable_put(void)
-{
-}
-#endif /* CONFIG_DEV_PAGEMAP_OPS */
-
 static void pgmap_array_delete(struct range *range)
 {
xa_store_range(_array, PHYS_PFN(range->start), 
PHYS_PFN(range->end),
@@ -181,7 +152,6 @@ void memunmap_pages(struct dev_pagemap *pgmap)
pageunmap_range(pgmap, i);
 
WARN_ONCE(pgmap->altmap.alloc, "failed to free all reserved pages\n");
-   devmap_managed_enable_put();
 }
 EXPORT_SYMBOL_GPL(memunmap_pages);
 
@@ -319,7 +289,6 @@ void *memremap_pages(struct dev_pagemap *pgmap, int nid)
.pgprot = PAGE_KERNEL,
};
const int nr_range = pgmap->nr_range;
-   bool need_devmap_managed = true;
int error, i;
 
if (WARN_ONCE(!nr_range, "nr_range must be specified\n"))
@@ -331,8 +300,9 @@ void *memremap_pages(struct dev_pagemap *pgmap, int nid)
WARN(1, "Device private memory not supported\n");
return ERR_PTR(-EINVAL);
}
-   if (!pgmap->ops || !pgmap->ops->migrate_to_ram) {
-   WARN(1, "Missing migrate_to_ram method\n");
+   if (!pgmap->ops ||
+   !pgmap->ops->migrate_to_ram || !pgmap->ops->page_free) {
+   WARN(1, "Missing ops\n");
return ERR_PTR(-EINVAL);
}
if (!pgmap->owner) {
@@ -348,11 +318,9 @@ void *memremap_pages(struct dev_pagemap *pgmap, int nid)
}
break;
case MEMORY_DEVICE_GENERIC:
-   need_devmap_managed = false;
break;

Re: [Nouveau] [Intel-gfx] [PATCH v5 14/20] drm/nouveau/kms/nv50-: Use downstream DP clock limits for mode validation

[Ville Syrjälä]

On Wed, Aug 26, 2020 at 02:24:50PM -0400, Lyude Paul wrote:
> This adds support for querying the maximum clock rate of a downstream
> port on a DisplayPort connection. Generally, downstream ports refer to
> active dongles which can have their own pixel clock limits.
> 
> Note as well, we also start marking the connector as disconnected if we
> can't read the DPCD, since we wouldn't be able to do anything without
> DPCD access anyway.
> 
> Signed-off-by: Lyude Paul 
> Reviewed-by: Ben Skeggs 
> ---
>  drivers/gpu/drm/nouveau/dispnv50/disp.c   |  3 +++
>  drivers/gpu/drm/nouveau/nouveau_dp.c  | 15 +++
>  drivers/gpu/drm/nouveau/nouveau_encoder.h |  1 +
>  3 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c 
> b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index 8e1effb10425d..d2141ca16107b 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> @@ -1258,7 +1258,10 @@ nv50_mstc_detect(struct drm_connector *connector,
>  
>   ret = drm_dp_mst_detect_port(connector, ctx, mstc->port->mgr,
>mstc->port);
> + if (ret != connector_status_connected)
> + goto out;
>  
> +out:
>   pm_runtime_mark_last_busy(connector->dev->dev);
>   pm_runtime_put_autosuspend(connector->dev->dev);
>   return ret;
> diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c 
> b/drivers/gpu/drm/nouveau/nouveau_dp.c
> index 005750aeb6d4f..ad852e572cfec 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_dp.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_dp.c
> @@ -61,6 +61,11 @@ nouveau_dp_probe_dpcd(struct nouveau_connector 
> *nv_connector,
>   mstm->can_mst = drm_dp_read_mst_cap(aux, dpcd);
>   }
>  
> + ret = drm_dp_read_downstream_info(aux, dpcd,
> +   outp->dp.downstream_ports);
> + if (ret < 0)
> + return connector_status_disconnected;
> +
>   return connector_status_connected;
>  }
>  
> @@ -176,8 +181,6 @@ void nouveau_dp_irq(struct nouveau_drm *drm,
>  /* TODO:
>   * - Use the minimum possible BPC here, once we add support for the max bpc
>   *   property.
> - * - Validate the mode against downstream port caps (see
> - *   drm_dp_downstream_max_clock())
>   * - Validate against the DP caps advertised by the GPU (we don't check these
>   *   yet)
>   */
> @@ -188,15 +191,19 @@ nv50_dp_mode_valid(struct drm_connector *connector,
>  unsigned *out_clock)
>  {
>   const unsigned min_clock = 25000;
> - unsigned max_clock, clock;
> + unsigned max_clock, ds_clock, clock;
>   enum drm_mode_status ret;
>  
>   if (mode->flags & DRM_MODE_FLAG_INTERLACE && !outp->caps.dp_interlace)
>   return MODE_NO_INTERLACE;

I stumbled on this code when applying my big DFP series (sorry that
I forgot to read this while it was on the list).

Anyways, this code appears somewhat confused about the different
clocks.

>  
>   max_clock = outp->dp.link_nr * outp->dp.link_bw;

That I presume is the max symbol rate of the link.

> - clock = mode->clock * (connector->display_info.bpc * 3) / 10;
> + ds_clock = drm_dp_downstream_max_clock(outp->dp.dpcd,
> +outp->dp.downstream_ports);

That is the maximum dotclock (also the max TMDS clock before my DFP
series landed) the DFP supports.

> + if (ds_clock)
> + max_clock = min(max_clock, ds_clock);

max() between the symbol rate and dotclock doesn't really
make sense. One is the amount of symbols (or data in other
words), the other is amount of pixels (which depending on
bpc can result in various amounts of symbols/data).

>  
> + clock = mode->clock * (connector->display_info.bpc * 3) / 10;

I presume this trying to compute the symbol rate we require.
Due to the 8b/10b encoding each 10bit symbol carries 8 bits of
actual data, so the /10 should be /8. IIRC we had this same
bug in i915, but it was fixed years ago.

This is also calculating things based on display_info.bpc which
IIRC is the max bpc the display supports. Using the max may be
overly pessimistic in case a) the driver/hw doesn't even support
bpc that high, b) the driver can dynamically reduce the bpc in
order to fit the mode onto the link. In i915 we take the opposite
approach and just use the min bpc (==6) in mode_valid(). During
modeset we start from the max bpc and keep reducing it until
things fit.


So I suspect what you want here is something like:

max_clock = outp->dp.link_nr * outp->dp.link_bw;
clock = mode->clock * (connector->display_info.bpc * 3) / 8; // or maybe just 
use 6 for bpc
if (clock > max_clock)
reurn CLOCK_HIGH;

ds_clock = drm_dp_downstream_max_dotclock();
if (ds_clock && mode->clock > ds_clock)
return CLOCK_HIGH;

+ a bit more if you want to also deal with the TMDS
clock limits sensibly. That also requires some though
as to which bpc to use. In i915 

Re: [Nouveau] [PATCH v2 00/18] drm/i915: Pimp DP DFP handling

[Ville Syrjälä]

On Tue, Sep 08, 2020 at 02:34:24PM -0400, Lyude Paul wrote:
> With the nitpicks addressed (note there were a couple of other spots where we
> wanted to use Return: in the kdocs, but I didn't bother pointing all of them
> out), all but patch 07 is:
> 
> Reviewed-by: Lyude Paul 

Thanks for the review. I fixed up the missing/bad docs and 
pushed the lot to drm-intel-next-queued (with Daniel's irc ack).

PS.
Had to s/drm_dp_downstream_max_clock/drm_dp_downstream_max_dotclock/
in nouveau_dp.c to keep it in a buildable shape. I hope I didn't step
on too many toes with this...

-- 
Ville Syrjälä
Intel
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [Nouveau] [PATCH v2 1/2] drm/nouveau: return temperatures in temp_get() via parameter

[Karol Herbst]

On Thu, Sep 17, 2020 at 4:11 PM Jeremy Cline  wrote:
>
> On Wed, Sep 16, 2020 at 10:03:22PM +0200, Karol Herbst wrote:
> > On Wed, Sep 16, 2020 at 10:01 PM Karol Herbst  wrote:
> > >
> > > On Wed, Sep 16, 2020 at 9:47 PM Jeremy Cline  wrote:
> > > >
> > > > The temp_get() function currently returns negative error numbers or a
> > > > temperature. However, the thermal sensors can (in theory) measure
> > > > negative temperatures. Some implementations of temp_get() correctly
> > > > clamp negative temperature readings to 0 so that users do not mistake
> > > > them for errors, but some, like gp100_temp_get(), do not.
> > > >
> > > > Rather than relying on implementations remembering to clamp values,
> > > > dedicate the function return value to error codes and accept a pointer
> > > > to an integer where the temperature reading should be stored.
> > > >
> > > > Signed-off-by: Jeremy Cline 
> > > > ---
> > > >  .../drm/nouveau/include/nvkm/subdev/therm.h   |  2 +-
> > > >  drivers/gpu/drm/nouveau/nouveau_hwmon.c   | 12 ++--
> > > >  .../gpu/drm/nouveau/nvkm/subdev/therm/base.c  | 19 ++-
> > > >  .../gpu/drm/nouveau/nvkm/subdev/therm/g84.c   | 11 ++-
> > > >  .../gpu/drm/nouveau/nvkm/subdev/therm/gp100.c | 11 ++-
> > > >  .../gpu/drm/nouveau/nvkm/subdev/therm/nv40.c  |  9 +++--
> > > >  .../gpu/drm/nouveau/nvkm/subdev/therm/nv50.c  |  9 +++--
> > > >  .../gpu/drm/nouveau/nvkm/subdev/therm/priv.h  | 17 +++--
> > > >  .../gpu/drm/nouveau/nvkm/subdev/therm/temp.c  | 12 
> > > >  9 files changed, 62 insertions(+), 40 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h 
> > > > b/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h
> > > > index 62c34f98c930..bfe9779216fc 100644
> > > > --- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h
> > > > +++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h
> > > > @@ -99,7 +99,7 @@ struct nvkm_therm {
> > > > bool clkgating_enabled;
> > > >  };
> > > >
> > > > -int nvkm_therm_temp_get(struct nvkm_therm *);
> > > > +int nvkm_therm_temp_get(struct nvkm_therm *therm, int *temp);
> > > >  int nvkm_therm_fan_sense(struct nvkm_therm *);
> > > >  int nvkm_therm_cstate(struct nvkm_therm *, int, int);
> > > >  void nvkm_therm_clkgate_init(struct nvkm_therm *,
> > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_hwmon.c 
> > > > b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> > > > index 1c3104d20571..aff6aa296678 100644
> > > > --- a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> > > > +++ b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> > > > @@ -325,7 +325,7 @@ nouveau_temp_is_visible(const void *data, u32 attr, 
> > > > int channel)
> > > > struct nouveau_drm *drm = nouveau_drm((struct drm_device 
> > > > *)data);
> > > > struct nvkm_therm *therm = nvxx_therm(>client.device);
> > > >
> > > > -   if (!therm || !therm->attr_get || nvkm_therm_temp_get(therm) < 
> > > > 0)
> > > > +   if (!therm || !therm->attr_get || nvkm_therm_temp_get(therm, 
> > > > NULL) < 0)
> > > > return 0;
> > > >
> > > > switch (attr) {
> > > > @@ -419,7 +419,7 @@ nouveau_temp_read(struct device *dev, u32 attr, int 
> > > > channel, long *val)
> > > > struct drm_device *drm_dev = dev_get_drvdata(dev);
> > > > struct nouveau_drm *drm = nouveau_drm(drm_dev);
> > > > struct nvkm_therm *therm = nvxx_therm(>client.device);
> > > > -   int ret;
> > > > +   int ret = 0, temp;
> > > >
> > > > if (!therm || !therm->attr_get)
> > > > return -EOPNOTSUPP;
> > > > @@ -428,8 +428,8 @@ nouveau_temp_read(struct device *dev, u32 attr, int 
> > > > channel, long *val)
> > > > case hwmon_temp_input:
> > > > if (drm_dev->switch_power_state != DRM_SWITCH_POWER_ON)
> > > > return -EINVAL;
> > > > -   ret = nvkm_therm_temp_get(therm);
> > > > -   *val = ret < 0 ? ret : (ret * 1000);
> > > > +   ret = nvkm_therm_temp_get(therm, );
> > > > +   *val = temp * 1000;
> > > > break;
> > > > case hwmon_temp_max:
> > > > *val = therm->attr_get(therm, 
> > > > NVKM_THERM_ATTR_THRS_DOWN_CLK)
> > > > @@ -459,7 +459,7 @@ nouveau_temp_read(struct device *dev, u32 attr, int 
> > > > channel, long *val)
> > > > return -EOPNOTSUPP;
> > > > }
> > > >
> > > > -   return 0;
> > > > +   return ret;
> > > >  }
> > > >
> > > >  static int
> > > > @@ -735,7 +735,7 @@ nouveau_hwmon_init(struct drm_device *dev)
> > > > hwmon->dev = dev;
> > > >
> > > > if (therm && therm->attr_get && therm->attr_set) {
> > > > -   if (nvkm_therm_temp_get(therm) >= 0)
> > > > +   if (nvkm_therm_temp_get(therm, NULL) >= 0)
> > > > special_groups[i++] = 
> > > > _auto_point_sensor_group;
> > > > if 

Re: [Nouveau] [PATCH v2 1/2] drm/nouveau: return temperatures in temp_get() via parameter

[Jeremy Cline]

On Wed, Sep 16, 2020 at 10:03:22PM +0200, Karol Herbst wrote:
> On Wed, Sep 16, 2020 at 10:01 PM Karol Herbst  wrote:
> >
> > On Wed, Sep 16, 2020 at 9:47 PM Jeremy Cline  wrote:
> > >
> > > The temp_get() function currently returns negative error numbers or a
> > > temperature. However, the thermal sensors can (in theory) measure
> > > negative temperatures. Some implementations of temp_get() correctly
> > > clamp negative temperature readings to 0 so that users do not mistake
> > > them for errors, but some, like gp100_temp_get(), do not.
> > >
> > > Rather than relying on implementations remembering to clamp values,
> > > dedicate the function return value to error codes and accept a pointer
> > > to an integer where the temperature reading should be stored.
> > >
> > > Signed-off-by: Jeremy Cline 
> > > ---
> > >  .../drm/nouveau/include/nvkm/subdev/therm.h   |  2 +-
> > >  drivers/gpu/drm/nouveau/nouveau_hwmon.c   | 12 ++--
> > >  .../gpu/drm/nouveau/nvkm/subdev/therm/base.c  | 19 ++-
> > >  .../gpu/drm/nouveau/nvkm/subdev/therm/g84.c   | 11 ++-
> > >  .../gpu/drm/nouveau/nvkm/subdev/therm/gp100.c | 11 ++-
> > >  .../gpu/drm/nouveau/nvkm/subdev/therm/nv40.c  |  9 +++--
> > >  .../gpu/drm/nouveau/nvkm/subdev/therm/nv50.c  |  9 +++--
> > >  .../gpu/drm/nouveau/nvkm/subdev/therm/priv.h  | 17 +++--
> > >  .../gpu/drm/nouveau/nvkm/subdev/therm/temp.c  | 12 
> > >  9 files changed, 62 insertions(+), 40 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h 
> > > b/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h
> > > index 62c34f98c930..bfe9779216fc 100644
> > > --- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h
> > > +++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/therm.h
> > > @@ -99,7 +99,7 @@ struct nvkm_therm {
> > > bool clkgating_enabled;
> > >  };
> > >
> > > -int nvkm_therm_temp_get(struct nvkm_therm *);
> > > +int nvkm_therm_temp_get(struct nvkm_therm *therm, int *temp);
> > >  int nvkm_therm_fan_sense(struct nvkm_therm *);
> > >  int nvkm_therm_cstate(struct nvkm_therm *, int, int);
> > >  void nvkm_therm_clkgate_init(struct nvkm_therm *,
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_hwmon.c 
> > > b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> > > index 1c3104d20571..aff6aa296678 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_hwmon.c
> > > @@ -325,7 +325,7 @@ nouveau_temp_is_visible(const void *data, u32 attr, 
> > > int channel)
> > > struct nouveau_drm *drm = nouveau_drm((struct drm_device *)data);
> > > struct nvkm_therm *therm = nvxx_therm(>client.device);
> > >
> > > -   if (!therm || !therm->attr_get || nvkm_therm_temp_get(therm) < 0)
> > > +   if (!therm || !therm->attr_get || nvkm_therm_temp_get(therm, 
> > > NULL) < 0)
> > > return 0;
> > >
> > > switch (attr) {
> > > @@ -419,7 +419,7 @@ nouveau_temp_read(struct device *dev, u32 attr, int 
> > > channel, long *val)
> > > struct drm_device *drm_dev = dev_get_drvdata(dev);
> > > struct nouveau_drm *drm = nouveau_drm(drm_dev);
> > > struct nvkm_therm *therm = nvxx_therm(>client.device);
> > > -   int ret;
> > > +   int ret = 0, temp;
> > >
> > > if (!therm || !therm->attr_get)
> > > return -EOPNOTSUPP;
> > > @@ -428,8 +428,8 @@ nouveau_temp_read(struct device *dev, u32 attr, int 
> > > channel, long *val)
> > > case hwmon_temp_input:
> > > if (drm_dev->switch_power_state != DRM_SWITCH_POWER_ON)
> > > return -EINVAL;
> > > -   ret = nvkm_therm_temp_get(therm);
> > > -   *val = ret < 0 ? ret : (ret * 1000);
> > > +   ret = nvkm_therm_temp_get(therm, );
> > > +   *val = temp * 1000;
> > > break;
> > > case hwmon_temp_max:
> > > *val = therm->attr_get(therm, 
> > > NVKM_THERM_ATTR_THRS_DOWN_CLK)
> > > @@ -459,7 +459,7 @@ nouveau_temp_read(struct device *dev, u32 attr, int 
> > > channel, long *val)
> > > return -EOPNOTSUPP;
> > > }
> > >
> > > -   return 0;
> > > +   return ret;
> > >  }
> > >
> > >  static int
> > > @@ -735,7 +735,7 @@ nouveau_hwmon_init(struct drm_device *dev)
> > > hwmon->dev = dev;
> > >
> > > if (therm && therm->attr_get && therm->attr_set) {
> > > -   if (nvkm_therm_temp_get(therm) >= 0)
> > > +   if (nvkm_therm_temp_get(therm, NULL) >= 0)
> > > special_groups[i++] = 
> > > _auto_point_sensor_group;
> > > if (therm->fan_get && therm->fan_get(therm) >= 0)
> > > special_groups[i++] = _fan_sensor_group;
> > > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/therm/base.c 
> > > b/drivers/gpu/drm/nouveau/nvkm/subdev/therm/base.c
> > > index 4a4d1e224126..e7ffea1512e6 

Re: [Nouveau] [PATCH] drm/nouveau: Drop mutex_lock_nested for atomic

[Daniel Vetter]

Ben, did you have a chance to look at this?
-Daniel

On Mon, Aug 3, 2020 at 1:22 PM Maarten Lankhorst
 wrote:
>
> Op 02-08-2020 om 20:18 schreef Daniel Vetter:
> > Purely conjecture, but I think the original locking inversion with the
> > legacy page flip code between flipping and ttm's bo move function
> > shoudn't exist anymore with atomic: With atomic the bo pinning and
> > actual modeset commit is completely separated in the code patsh.
> >
> > This annotation was originally added in
> >
> > commit 060810d7abaabcab282e062c595871d661561400
> > Author: Ben Skeggs 
> > Date:   Mon Jul 8 14:15:51 2013 +1000
> >
> > drm/nouveau: fix locking issues in page flipping paths
> >
> > due to
> >
> > commit b580c9e2b7ba5030a795aa2fb73b796523d65a78
> > Author: Maarten Lankhorst 
> > Date:   Thu Jun 27 13:48:18 2013 +0200
> >
> > drm/nouveau: make flipping lockdep safe
> >
> > Signed-off-by: Daniel Vetter 
> > Cc: Maarten Lankhorst 
> > Cc: Ben Skeggs 
> > Cc: Dave Airlie 
> > Cc: nouveau@lists.freedesktop.org
> > ---
> > I might be totally wrong, so this definitely needs testing :-)
> >
> > Cheers, Daniel
> > ---
> >  drivers/gpu/drm/nouveau/nouveau_bo.c | 6 +-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c 
> > b/drivers/gpu/drm/nouveau/nouveau_bo.c
> > index 7806278dce57..a7b2a9bb0ffe 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_bo.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
> > @@ -776,7 +776,11 @@ nouveau_bo_move_m2mf(struct ttm_buffer_object *bo, int 
> > evict, bool intr,
> >   return ret;
> >   }
> >
> > - mutex_lock_nested(>mutex, SINGLE_DEPTH_NESTING);
> > + if (drm_drv_uses_atomic_modeset(drm->dev))
> > + mutex_lock(>mutex);
> > + else
> > + mutex_lock_nested(>mutex, SINGLE_DEPTH_NESTING);
> > +
> >   ret = nouveau_fence_sync(nouveau_bo(bo), chan, true, intr);
> >   if (ret == 0) {
> >   ret = drm->ttm.move(chan, bo, >mem, new_reg);
>
> Well if you're certain it works now. :)
>
> Reviewed-by: Maarten Lankhorst 
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau