Hi Thomas.

On Fri, May 22, 2020 at 03:52:26PM +0200, Thomas Zimmermann wrote:

Rename the macro to DRM_GEM_CMA_DRIVER_OPS to align with SHMEM helpers.

This part is fine, I like that the naming is somehow consistent.

An internal version is provided for drivers that override the default .dumb_create callback. Adapt drivers to the changes.

I loathe anything named __foo or __FOO. This __ signals to me that the author was clueless in naming - or some sort. I know that __ is used in some lib headers - but thats not the case here.

But I love that we have a variant that takes a create function. So we do not have to escape from the nice macro. The macro is another way to tell me as rewiewer that this drivers uses all the default helpers for this.

So critizising the name I better suggest something that I personally like better:

DRM_GEM_CMA_DRIVER_OPS_CREATE()

It would look like this: /* GEM Operations */ - DRM_GEM_CMA_VMAP_DRIVER_OPS, - .dumb_create = drm_sun4i_gem_dumb_create, + DRM_GEM_CMA_DRIVER_OPS_CREATE(drm_sun4i_gem_dumb_create),

Please fix zte/zx_drm_drv.c which also uses DRM_GEM_CMA_VMAP_DRIVER_OPS.

The naming is a bikeshedding topic that we may not agree on, soo..

With zte fixed the patch is: Reviewed-by: Sam Ravnborg sam@ravnborg.org

Sam

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

drivers/gpu/drm/drm_gem_cma_helper.c | 2 +- drivers/gpu/drm/sun4i/sun4i_drv.c | 3 +-- drivers/gpu/drm/tidss/tidss_drv.c | 2 +- drivers/gpu/drm/tiny/hx8357d.c | 2 +- drivers/gpu/drm/tiny/ili9225.c | 2 +- drivers/gpu/drm/tiny/ili9341.c | 2 +- drivers/gpu/drm/tiny/ili9486.c | 2 +- drivers/gpu/drm/tiny/mi0283qt.c | 2 +- drivers/gpu/drm/tiny/repaper.c | 2 +- drivers/gpu/drm/tiny/st7586.c | 2 +- drivers/gpu/drm/tiny/st7735r.c | 2 +- include/drm/drm_gem_cma_helper.h | 24 ++++++++++++++++++++---- 12 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index 12e98fb28229d..6fa4d2f2e3987 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -620,7 +620,7 @@ EXPORT_SYMBOL(drm_cma_gem_create_object_default_funcs);

• address set. This address is released when the object is freed.
• This function can be used as the &drm_driver.gem_prime_import_sg_table

• • callback. The DRM_GEM_CMA_VMAP_DRIVER_OPS() macro provides a shortcut to set

• • callback. The &DRM_GEM_CMA_DRIVER_OPS macro provides a shortcut to set
  • the necessary DRM driver operations.
  • Returns:

diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c index 328272ff77d84..012855fd89c24 100644 --- a/drivers/gpu/drm/sun4i/sun4i_drv.c +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c @@ -52,8 +52,7 @@ static struct drm_driver sun4i_drv_driver = { .minor = 0,

/* GEM Operations */

• DRM_GEM_CMA_VMAP_DRIVER_OPS,
• .dumb_create = drm_sun4i_gem_dumb_create,

• __DRM_GEM_CMA_DRIVER_OPS(drm_sun4i_gem_dumb_create),

};

static int sun4i_drv_bind(struct device *dev) diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c index 99edc66ebdef2..1753cdc74ebda 100644 --- a/drivers/gpu/drm/tidss/tidss_drv.c +++ b/drivers/gpu/drm/tidss/tidss_drv.c @@ -112,7 +112,7 @@ static struct drm_driver tidss_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, .fops = &tidss_fops, .release = tidss_release,

• DRM_GEM_CMA_VMAP_DRIVER_OPS,

• DRM_GEM_CMA_DRIVER_OPS, .name = "tidss", .desc = "TI Keystone DSS", .date = "20180215",

diff --git a/drivers/gpu/drm/tiny/hx8357d.c b/drivers/gpu/drm/tiny/hx8357d.c index b4bc358a3269a..592da71d7ca70 100644 --- a/drivers/gpu/drm/tiny/hx8357d.c +++ b/drivers/gpu/drm/tiny/hx8357d.c @@ -196,7 +196,7 @@ DEFINE_DRM_GEM_CMA_FOPS(hx8357d_fops); static struct drm_driver hx8357d_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, .fops = &hx8357d_fops,

• DRM_GEM_CMA_VMAP_DRIVER_OPS,

• DRM_GEM_CMA_DRIVER_OPS, .debugfs_init = mipi_dbi_debugfs_init, .name = "hx8357d", .desc = "HX8357D",

diff --git a/drivers/gpu/drm/tiny/ili9225.c b/drivers/gpu/drm/tiny/ili9225.c index d1a5ab6747d5c..368ff6c8a1efb 100644 --- a/drivers/gpu/drm/tiny/ili9225.c +++ b/drivers/gpu/drm/tiny/ili9225.c @@ -346,7 +346,7 @@ DEFINE_DRM_GEM_CMA_FOPS(ili9225_fops); static struct drm_driver ili9225_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, .fops = &ili9225_fops,

• DRM_GEM_CMA_VMAP_DRIVER_OPS,

• DRM_GEM_CMA_DRIVER_OPS, .name = "ili9225", .desc = "Ilitek ILI9225", .date = "20171106",

diff --git a/drivers/gpu/drm/tiny/ili9341.c b/drivers/gpu/drm/tiny/ili9341.c index bb819f45a5d3b..e1b9043ef7a0a 100644 --- a/drivers/gpu/drm/tiny/ili9341.c +++ b/drivers/gpu/drm/tiny/ili9341.c @@ -152,7 +152,7 @@ DEFINE_DRM_GEM_CMA_FOPS(ili9341_fops); static struct drm_driver ili9341_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, .fops = &ili9341_fops,

• DRM_GEM_CMA_VMAP_DRIVER_OPS,

• DRM_GEM_CMA_DRIVER_OPS, .debugfs_init = mipi_dbi_debugfs_init, .name = "ili9341", .desc = "Ilitek ILI9341",

diff --git a/drivers/gpu/drm/tiny/ili9486.c b/drivers/gpu/drm/tiny/ili9486.c index 2702ea557d297..90a17f40fdf0c 100644 --- a/drivers/gpu/drm/tiny/ili9486.c +++ b/drivers/gpu/drm/tiny/ili9486.c @@ -165,7 +165,7 @@ DEFINE_DRM_GEM_CMA_FOPS(ili9486_fops); static struct drm_driver ili9486_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, .fops = &ili9486_fops,

• DRM_GEM_CMA_VMAP_DRIVER_OPS,

• DRM_GEM_CMA_DRIVER_OPS, .debugfs_init = mipi_dbi_debugfs_init, .name = "ili9486", .desc = "Ilitek ILI9486",

diff --git a/drivers/gpu/drm/tiny/mi0283qt.c b/drivers/gpu/drm/tiny/mi0283qt.c index 08ac549ab0f7f..6624c2098fba2 100644 --- a/drivers/gpu/drm/tiny/mi0283qt.c +++ b/drivers/gpu/drm/tiny/mi0283qt.c @@ -156,7 +156,7 @@ DEFINE_DRM_GEM_CMA_FOPS(mi0283qt_fops); static struct drm_driver mi0283qt_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, .fops = &mi0283qt_fops,

• DRM_GEM_CMA_VMAP_DRIVER_OPS,

• DRM_GEM_CMA_DRIVER_OPS, .debugfs_init = mipi_dbi_debugfs_init, .name = "mi0283qt", .desc = "Multi-Inno MI0283QT",

diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c index 1c0e7169545b4..877dcece25828 100644 --- a/drivers/gpu/drm/tiny/repaper.c +++ b/drivers/gpu/drm/tiny/repaper.c @@ -946,7 +946,7 @@ DEFINE_DRM_GEM_CMA_FOPS(repaper_fops); static struct drm_driver repaper_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, .fops = &repaper_fops,

• DRM_GEM_CMA_VMAP_DRIVER_OPS,

• DRM_GEM_CMA_DRIVER_OPS, .name = "repaper", .desc = "Pervasive Displays RePaper e-ink panels", .date = "20170405",

diff --git a/drivers/gpu/drm/tiny/st7586.c b/drivers/gpu/drm/tiny/st7586.c index 2a1fae422f7a2..ec84bdc51f60d 100644 --- a/drivers/gpu/drm/tiny/st7586.c +++ b/drivers/gpu/drm/tiny/st7586.c @@ -285,7 +285,7 @@ DEFINE_DRM_GEM_CMA_FOPS(st7586_fops); static struct drm_driver st7586_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, .fops = &st7586_fops,

• DRM_GEM_CMA_VMAP_DRIVER_OPS,

• DRM_GEM_CMA_DRIVER_OPS, .debugfs_init = mipi_dbi_debugfs_init, .name = "st7586", .desc = "Sitronix ST7586",

diff --git a/drivers/gpu/drm/tiny/st7735r.c b/drivers/gpu/drm/tiny/st7735r.c index 0af1b15efdf8a..cfd4933f3b30c 100644 --- a/drivers/gpu/drm/tiny/st7735r.c +++ b/drivers/gpu/drm/tiny/st7735r.c @@ -157,7 +157,7 @@ DEFINE_DRM_GEM_CMA_FOPS(st7735r_fops); static struct drm_driver st7735r_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, .fops = &st7735r_fops,

• DRM_GEM_CMA_VMAP_DRIVER_OPS,

• DRM_GEM_CMA_DRIVER_OPS, .debugfs_init = mipi_dbi_debugfs_init, .name = "st7735r", .desc = "Sitronix ST7735R",

diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h index 947ac95eb24a9..917d42603db06 100644 --- a/include/drm/drm_gem_cma_helper.h +++ b/include/drm/drm_gem_cma_helper.h @@ -110,21 +110,37 @@ struct drm_gem_object * drm_cma_gem_create_object_default_funcs(struct drm_device *dev, size_t size);

/**

• • DRM_GEM_CMA_VMAP_DRIVER_OPS - CMA GEM driver operations ensuring a virtual

• •                           address on the buffer
    

• • __DRM_GEM_CMA_DRIVER_OPS - CMA GEM driver operations ensuring a

• •                        virtual address on the buffer
    

• • @__dumb_create: callback function for .dumb_create
  • This macro provides a shortcut for setting the default GEM operations in the
  • &drm_driver structure for drivers that need the virtual address also on
  • imported buffers.
• • This macro is a variant of DRM_GEM_CMA_DRIVER_OPS for drivers that
• • override the default implementation of .dumb_create. Use
• • DRM_GEM_CMA_DRIVER_OPS if possible.
  */

-#define DRM_GEM_CMA_VMAP_DRIVER_OPS \ +#define __DRM_GEM_CMA_DRIVER_OPS(__dumb_create) \ .gem_create_object = drm_cma_gem_create_object_default_funcs, \

• .dumb_create = drm_gem_cma_dumb_create, \

• .dumb_create = (__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_sg_table = drm_gem_cma_prime_import_sg_table_vmap, \ .gem_prime_mmap = drm_gem_prime_mmap

+/**

• • DRM_GEM_CMA_DRIVER_OPS - CMA GEM driver operations ensuring a virtual

• •                      address on the buffer
    

• • This macro provides a shortcut for setting the default GEM operations in the
• • &drm_driver structure for drivers that need the virtual address also on
• • imported buffers.
• */

+#define DRM_GEM_CMA_DRIVER_OPS \

• __DRM_GEM_CMA_DRIVER_OPS(drm_gem_cma_dumb_create)

struct drm_gem_object * drm_gem_cma_prime_import_sg_table_vmap(struct drm_device *drm, struct dma_buf_attachment *attach, -- 2.26.2

Hi Sam

Am 22.05.20 um 19:48 schrieb Sam Ravnborg:

Hi Thomas.

On Fri, May 22, 2020 at 03:52:26PM +0200, Thomas Zimmermann wrote:

...

Rename the macro to DRM_GEM_CMA_DRIVER_OPS to align with SHMEM helpers.

This part is fine, I like that the naming is somehow consistent.

...

An internal version is provided for drivers that override the default .dumb_create callback. Adapt drivers to the changes.

I loathe anything named __foo or __FOO. This __ signals to me that the author was clueless in naming - or some sort. I know that __ is used in some lib headers - but thats not the case here.

I agree with your comment and I've been trying to find a better name before posting the patchset. I considered something like DRM_GEM_CMA_DRIVER_OPS_INTERNAL(), but wasn't happy with that either. In the end, I uses the double underscore to push driver authors towards the other macro. This one's only for the special case of settings a separate implementation for .dumb_create().

But I love that we have a variant that takes a create function. So we do not have to escape from the nice macro. The macro is another way to tell me as rewiewer that this drivers uses all the default helpers for this.

So critizising the name I better suggest something that I personally like better:

DRM_GEM_CMA_DRIVER_OPS_CREATE()

Can we at least use DRM_GEM_CMA_DRIVER_OPS_WITH_DUMB_CREATE ? Because there's also gem_object_create.

I'll update the patches accordingly.

I noticed that most of the affected drivers do some kind of alignment calculation in their dumb_create code. IMHO in the long run, we should move such calculations into the default implementation and put the control paramters into struct drm_mode_config.

Best regards Thomas

It would look like this: /* GEM Operations */

• DRM_GEM_CMA_VMAP_DRIVER_OPS,
• .dumb_create = drm_sun4i_gem_dumb_create,

• DRM_GEM_CMA_DRIVER_OPS_CREATE(drm_sun4i_gem_dumb_create),

Please fix zte/zx_drm_drv.c which also uses DRM_GEM_CMA_VMAP_DRIVER_OPS.

The naming is a bikeshedding topic that we may not agree on, soo..

With zte fixed the patch is: Reviewed-by: Sam Ravnborg sam@ravnborg.org

Sam

...

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

drivers/gpu/drm/drm_gem_cma_helper.c | 2 +- drivers/gpu/drm/sun4i/sun4i_drv.c | 3 +-- drivers/gpu/drm/tidss/tidss_drv.c | 2 +- drivers/gpu/drm/tiny/hx8357d.c | 2 +- drivers/gpu/drm/tiny/ili9225.c | 2 +- drivers/gpu/drm/tiny/ili9341.c | 2 +- drivers/gpu/drm/tiny/ili9486.c | 2 +- drivers/gpu/drm/tiny/mi0283qt.c | 2 +- drivers/gpu/drm/tiny/repaper.c | 2 +- drivers/gpu/drm/tiny/st7586.c | 2 +- drivers/gpu/drm/tiny/st7735r.c | 2 +- include/drm/drm_gem_cma_helper.h | 24 ++++++++++++++++++++---- 12 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index 12e98fb28229d..6fa4d2f2e3987 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -620,7 +620,7 @@ EXPORT_SYMBOL(drm_cma_gem_create_object_default_funcs);

• address set. This address is released when the object is freed.
• This function can be used as the &drm_driver.gem_prime_import_sg_table

• • callback. The DRM_GEM_CMA_VMAP_DRIVER_OPS() macro provides a shortcut to set

• • callback. The &DRM_GEM_CMA_DRIVER_OPS macro provides a shortcut to set
  • the necessary DRM driver operations.
  • Returns:

diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c index 328272ff77d84..012855fd89c24 100644 --- a/drivers/gpu/drm/sun4i/sun4i_drv.c +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c @@ -52,8 +52,7 @@ static struct drm_driver sun4i_drv_driver = { .minor = 0,

/* GEM Operations */

• DRM_GEM_CMA_VMAP_DRIVER_OPS,
• .dumb_create = drm_sun4i_gem_dumb_create,

• __DRM_GEM_CMA_DRIVER_OPS(drm_sun4i_gem_dumb_create),

};

static int sun4i_drv_bind(struct device *dev) diff --git a/drivers/gpu/drm/tidss/tidss_drv.c b/drivers/gpu/drm/tidss/tidss_drv.c index 99edc66ebdef2..1753cdc74ebda 100644 --- a/drivers/gpu/drm/tidss/tidss_drv.c +++ b/drivers/gpu/drm/tidss/tidss_drv.c @@ -112,7 +112,7 @@ static struct drm_driver tidss_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, .fops = &tidss_fops, .release = tidss_release,

• DRM_GEM_CMA_VMAP_DRIVER_OPS,

• DRM_GEM_CMA_DRIVER_OPS, .name = "tidss", .desc = "TI Keystone DSS", .date = "20180215",

diff --git a/drivers/gpu/drm/tiny/hx8357d.c b/drivers/gpu/drm/tiny/hx8357d.c index b4bc358a3269a..592da71d7ca70 100644 --- a/drivers/gpu/drm/tiny/hx8357d.c +++ b/drivers/gpu/drm/tiny/hx8357d.c @@ -196,7 +196,7 @@ DEFINE_DRM_GEM_CMA_FOPS(hx8357d_fops); static struct drm_driver hx8357d_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, .fops = &hx8357d_fops,

• DRM_GEM_CMA_VMAP_DRIVER_OPS,

• DRM_GEM_CMA_DRIVER_OPS, .debugfs_init = mipi_dbi_debugfs_init, .name = "hx8357d", .desc = "HX8357D",

diff --git a/drivers/gpu/drm/tiny/ili9225.c b/drivers/gpu/drm/tiny/ili9225.c index d1a5ab6747d5c..368ff6c8a1efb 100644 --- a/drivers/gpu/drm/tiny/ili9225.c +++ b/drivers/gpu/drm/tiny/ili9225.c @@ -346,7 +346,7 @@ DEFINE_DRM_GEM_CMA_FOPS(ili9225_fops); static struct drm_driver ili9225_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, .fops = &ili9225_fops,

• DRM_GEM_CMA_VMAP_DRIVER_OPS,

• DRM_GEM_CMA_DRIVER_OPS, .name = "ili9225", .desc = "Ilitek ILI9225", .date = "20171106",

diff --git a/drivers/gpu/drm/tiny/ili9341.c b/drivers/gpu/drm/tiny/ili9341.c index bb819f45a5d3b..e1b9043ef7a0a 100644 --- a/drivers/gpu/drm/tiny/ili9341.c +++ b/drivers/gpu/drm/tiny/ili9341.c @@ -152,7 +152,7 @@ DEFINE_DRM_GEM_CMA_FOPS(ili9341_fops); static struct drm_driver ili9341_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, .fops = &ili9341_fops,

• DRM_GEM_CMA_VMAP_DRIVER_OPS,

• DRM_GEM_CMA_DRIVER_OPS, .debugfs_init = mipi_dbi_debugfs_init, .name = "ili9341", .desc = "Ilitek ILI9341",

diff --git a/drivers/gpu/drm/tiny/ili9486.c b/drivers/gpu/drm/tiny/ili9486.c index 2702ea557d297..90a17f40fdf0c 100644 --- a/drivers/gpu/drm/tiny/ili9486.c +++ b/drivers/gpu/drm/tiny/ili9486.c @@ -165,7 +165,7 @@ DEFINE_DRM_GEM_CMA_FOPS(ili9486_fops); static struct drm_driver ili9486_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, .fops = &ili9486_fops,

• DRM_GEM_CMA_VMAP_DRIVER_OPS,

• DRM_GEM_CMA_DRIVER_OPS, .debugfs_init = mipi_dbi_debugfs_init, .name = "ili9486", .desc = "Ilitek ILI9486",

diff --git a/drivers/gpu/drm/tiny/mi0283qt.c b/drivers/gpu/drm/tiny/mi0283qt.c index 08ac549ab0f7f..6624c2098fba2 100644 --- a/drivers/gpu/drm/tiny/mi0283qt.c +++ b/drivers/gpu/drm/tiny/mi0283qt.c @@ -156,7 +156,7 @@ DEFINE_DRM_GEM_CMA_FOPS(mi0283qt_fops); static struct drm_driver mi0283qt_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, .fops = &mi0283qt_fops,

• DRM_GEM_CMA_VMAP_DRIVER_OPS,

• DRM_GEM_CMA_DRIVER_OPS, .debugfs_init = mipi_dbi_debugfs_init, .name = "mi0283qt", .desc = "Multi-Inno MI0283QT",

diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c index 1c0e7169545b4..877dcece25828 100644 --- a/drivers/gpu/drm/tiny/repaper.c +++ b/drivers/gpu/drm/tiny/repaper.c @@ -946,7 +946,7 @@ DEFINE_DRM_GEM_CMA_FOPS(repaper_fops); static struct drm_driver repaper_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, .fops = &repaper_fops,

• DRM_GEM_CMA_VMAP_DRIVER_OPS,

• DRM_GEM_CMA_DRIVER_OPS, .name = "repaper", .desc = "Pervasive Displays RePaper e-ink panels", .date = "20170405",

diff --git a/drivers/gpu/drm/tiny/st7586.c b/drivers/gpu/drm/tiny/st7586.c index 2a1fae422f7a2..ec84bdc51f60d 100644 --- a/drivers/gpu/drm/tiny/st7586.c +++ b/drivers/gpu/drm/tiny/st7586.c @@ -285,7 +285,7 @@ DEFINE_DRM_GEM_CMA_FOPS(st7586_fops); static struct drm_driver st7586_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, .fops = &st7586_fops,

• DRM_GEM_CMA_VMAP_DRIVER_OPS,

• DRM_GEM_CMA_DRIVER_OPS, .debugfs_init = mipi_dbi_debugfs_init, .name = "st7586", .desc = "Sitronix ST7586",

diff --git a/drivers/gpu/drm/tiny/st7735r.c b/drivers/gpu/drm/tiny/st7735r.c index 0af1b15efdf8a..cfd4933f3b30c 100644 --- a/drivers/gpu/drm/tiny/st7735r.c +++ b/drivers/gpu/drm/tiny/st7735r.c @@ -157,7 +157,7 @@ DEFINE_DRM_GEM_CMA_FOPS(st7735r_fops); static struct drm_driver st7735r_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, .fops = &st7735r_fops,

• DRM_GEM_CMA_VMAP_DRIVER_OPS,

• DRM_GEM_CMA_DRIVER_OPS, .debugfs_init = mipi_dbi_debugfs_init, .name = "st7735r", .desc = "Sitronix ST7735R",

diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h index 947ac95eb24a9..917d42603db06 100644 --- a/include/drm/drm_gem_cma_helper.h +++ b/include/drm/drm_gem_cma_helper.h @@ -110,21 +110,37 @@ struct drm_gem_object * drm_cma_gem_create_object_default_funcs(struct drm_device *dev, size_t size);

/**

• • DRM_GEM_CMA_VMAP_DRIVER_OPS - CMA GEM driver operations ensuring a virtual

• •                           address on the buffer
    

• • __DRM_GEM_CMA_DRIVER_OPS - CMA GEM driver operations ensuring a

• •                        virtual address on the buffer
    

• • @__dumb_create: callback function for .dumb_create
  • This macro provides a shortcut for setting the default GEM operations in the
  • &drm_driver structure for drivers that need the virtual address also on
  • imported buffers.
• • This macro is a variant of DRM_GEM_CMA_DRIVER_OPS for drivers that
• • override the default implementation of .dumb_create. Use
• • DRM_GEM_CMA_DRIVER_OPS if possible.
  */

-#define DRM_GEM_CMA_VMAP_DRIVER_OPS \ +#define __DRM_GEM_CMA_DRIVER_OPS(__dumb_create) \ .gem_create_object = drm_cma_gem_create_object_default_funcs, \

• .dumb_create = drm_gem_cma_dumb_create, \

• .dumb_create = (__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_sg_table = drm_gem_cma_prime_import_sg_table_vmap, \ .gem_prime_mmap = drm_gem_prime_mmap

+/**

• • DRM_GEM_CMA_DRIVER_OPS - CMA GEM driver operations ensuring a virtual

• •                      address on the buffer
    

• • This macro provides a shortcut for setting the default GEM operations in the
• • &drm_driver structure for drivers that need the virtual address also on
• • imported buffers.
• */

+#define DRM_GEM_CMA_DRIVER_OPS \

• __DRM_GEM_CMA_DRIVER_OPS(drm_gem_cma_dumb_create)

struct drm_gem_object * drm_gem_cma_prime_import_sg_table_vmap(struct drm_device *drm, struct dma_buf_attachment *attach, -- 2.26.2

---

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

Hi

Am 09.10.20 um 09:54 schrieb Joel Stanley:

On Fri, 22 May 2020 at 13:52, Thomas Zimmermann tzimmermann@suse.de wrote:

...

DRM_GEM_CMA_DRIVER_OPS sets the functions in struct drm_driver to their defaults. No functional changes are made.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

I just found this in my inbox. I assume it has not been applied as you were after a review.

The patchset as a whole got an A-b and was merged a while ago.

Reviewed-by: Joel Stanley joel@jms.id.au

Nevertheless thanks for taking a look at the patch.

Best regards Thomas

I will apply it to drm-misc-next.

Cheers,

Joel

...

---

drivers/gpu/drm/aspeed/aspeed_gfx_drv.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c index 6b27242b9ee3c..1167ff78e24a3 100644 --- a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c +++ b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c @@ -188,12 +188,7 @@ DEFINE_DRM_GEM_CMA_FOPS(fops);

static struct drm_driver aspeed_gfx_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,

•   .gem_create_object      = drm_cma_gem_create_object_default_funcs,
  

•   .dumb_create            = drm_gem_cma_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_sg_table = drm_gem_cma_prime_import_sg_table,
  

•   .gem_prime_mmap         = drm_gem_prime_mmap,
  

•   DRM_GEM_CMA_DRIVER_OPS,
    .fops = &fops,
    .name = "aspeed-gfx-drm",
    .desc = "ASPEED GFX DRM",
  

-- 2.26.2

Hi

Am 09.10.20 um 10:06 schrieb Joel Stanley:

On Fri, 9 Oct 2020 at 08:01, Thomas Zimmermann tzimmermann@suse.de wrote:

...

Hi

Am 09.10.20 um 09:54 schrieb Joel Stanley:

...

On Fri, 22 May 2020 at 13:52, Thomas Zimmermann tzimmermann@suse.de wrote:

...

DRM_GEM_CMA_DRIVER_OPS sets the functions in struct drm_driver to their defaults. No functional changes are made.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

I just found this in my inbox. I assume it has not been applied as you were after a review.

The patchset as a whole got an A-b and was merged a while ago.

I'm a bit confused, I couldn't see it in any tree. The aspeed one seemed to have been skipped when applying the series.

I looked at today's linux-next and drm-misc-next.

Indeed. The other patches are in drm-misc-next, but not this one. I must have lost it during the merge process.

...

...

Reviewed-by: Joel Stanley joel@jms.id.au

Nevertheless thanks for taking a look at the patch.

Best regards Thomas

...

I will apply it to drm-misc-next.

So please ahead and apply it. There's also an ack by Emil, which you may want to add as well.

Acked-by: Emil Velikov emil.velikov@collabora.com

Sorry about missing the patch and thanks for taking care.

Best regards Thomas

...

...

Cheers,

Joel

...

---

drivers/gpu/drm/aspeed/aspeed_gfx_drv.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c index 6b27242b9ee3c..1167ff78e24a3 100644 --- a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c +++ b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c @@ -188,12 +188,7 @@ DEFINE_DRM_GEM_CMA_FOPS(fops);

static struct drm_driver aspeed_gfx_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,

•   .gem_create_object      = drm_cma_gem_create_object_default_funcs,
  

•   .dumb_create            = drm_gem_cma_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_sg_table = drm_gem_cma_prime_import_sg_table,
  

•   .gem_prime_mmap         = drm_gem_prime_mmap,
  

•   DRM_GEM_CMA_DRIVER_OPS,
    .fops = &fops,
    .name = "aspeed-gfx-drm",
    .desc = "ASPEED GFX DRM",
  

-- 2.26.2

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

---

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

Hi

Am 22.05.20 um 20:08 schrieb Sam Ravnborg:

Hi Thomas.

On Fri, May 22, 2020 at 03:52:30PM +0200, Thomas Zimmermann wrote:

...

The atmel-hlcdc driver uses the default implementation for CMA functions. The DRM_GEM_CMA_DRIVER_OPS macro now sets these defaults in struct drm_driver. All remaining operations are provided by CMA GEM object functions.

A nice side-effect of introducing the defualt implementation of CMA functions is that this driver is now migrated over to use drm_gem_object_funcs, which is why we can replace all these helpers with a simple macro that defined only 6 helpers.

With an improved changelog: Reviewed-by: Sam Ravnborg sam@ravnborg.org

And as I said in the other mail, this is really nice. It is now much more obvious that this drivers uses all the default helpers for CMA.

And I can drop one item from my TODO list on top of that.

There's still more to do. The current macro still sets .gem_object_create to a CMA default. But that pointer is the interface where drivers can override some of the CMA object defaults, so it should not be set by CMA helpers.

After the other CMA drivers have been converted to GEM object functions, this setting can be kept to zero and __drm_gem_cma_create() can set the default object functions. SHMEM and VRAM helpers already work this way.

Best regards Thomas

Sam

...

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c index 112aa5066ceed..871293d1aeeba 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c @@ -821,16 +821,7 @@ static struct drm_driver atmel_hlcdc_dc_driver = { .irq_preinstall = atmel_hlcdc_dc_irq_uninstall, .irq_postinstall = atmel_hlcdc_dc_irq_postinstall, .irq_uninstall = atmel_hlcdc_dc_irq_uninstall,

• .gem_free_object_unlocked = drm_gem_cma_free_object,
• .gem_vm_ops = &drm_gem_cma_vm_ops,
• .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 = drm_gem_cma_prime_get_sg_table,
• .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
• .gem_prime_vmap = drm_gem_cma_prime_vmap,
• .gem_prime_vunmap = drm_gem_cma_prime_vunmap,
• .gem_prime_mmap = drm_gem_cma_prime_mmap,
• .dumb_create = drm_gem_cma_dumb_create,

• DRM_GEM_CMA_DRIVER_OPS, .fops = &fops, .name = "atmel-hlcdc", .desc = "Atmel HLCD Controller DRM",

-- 2.26.2

---

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

Hi

Am 22.05.20 um 21:25 schrieb Sam Ravnborg:

Hi Thomas.

On Fri, May 22, 2020 at 03:52:30PM +0200, Thomas Zimmermann wrote:

...

The atmel-hlcdc driver uses the default implementation for CMA functions. The DRM_GEM_CMA_DRIVER_OPS macro now sets these defaults in struct drm_driver. All remaining operations are provided by CMA GEM object functions.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c index 112aa5066ceed..871293d1aeeba 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c @@ -821,16 +821,7 @@ static struct drm_driver atmel_hlcdc_dc_driver = { .irq_preinstall = atmel_hlcdc_dc_irq_uninstall, .irq_postinstall = atmel_hlcdc_dc_irq_postinstall, .irq_uninstall = atmel_hlcdc_dc_irq_uninstall,

• .gem_free_object_unlocked = drm_gem_cma_free_object,
• .gem_vm_ops = &drm_gem_cma_vm_ops,
• .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 = drm_gem_cma_prime_get_sg_table,
• .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
• .gem_prime_vmap = drm_gem_cma_prime_vmap,
• .gem_prime_vunmap = drm_gem_cma_prime_vunmap,

...

• .gem_prime_mmap = drm_gem_cma_prime_mmap,

When using DRM_GEM_CMA_DRIVER_OPS gem_prime_mmap is set to drm_gem_prime_mmap. Why is this the same as drm_gem_cma_prime_mmap?

Maybe this is all obvious when you know all the CMA stuff, but this puzzeled me.

Following through the calls is far from easy.

I took the macro from the aspeed driver. I had some doubts about the mmap code, but expected the driver to be working correctly. Maybe we should set that field to drm_gem_cma_prime_mmap or implement the mmap object function.

Best regards Thomas

Sam

...

• .dumb_create = drm_gem_cma_dumb_create,

• DRM_GEM_CMA_DRIVER_OPS, .fops = &fops, .name = "atmel-hlcdc", .desc = "Atmel HLCD Controller DRM",

-- 2.26.2

Hi Emil

Am 22.05.20 um 20:11 schrieb Emil Velikov:

Hi Thomas,

On Fri, 22 May 2020 at 14:53, Thomas Zimmermann tzimmermann@suse.de wrote:

...

The kirin driver uses the default implementation for CMA functions; except for the .dumb_create callback. The __DRM_GEM_CMA_DRIVER_OPS macro now sets these defaults and .dumb_create in struct drm_driver. All remaining operations are provided by CMA GEM object functions.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c index c339e632522a9..b1ffd7d43e562 100644 --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c @@ -921,17 +921,7 @@ DEFINE_DRM_GEM_CMA_FOPS(ade_fops); static struct drm_driver ade_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, .fops = &ade_fops,

•   .gem_free_object_unlocked = drm_gem_cma_free_object,
  

•   .gem_vm_ops = &drm_gem_cma_vm_ops,
  

•   .dumb_create = drm_gem_cma_dumb_create_internal,
  

This doesn't seem right. The _internal documentation explicitly says that this should _not_ be used as .dumb_create. Instead drivers should use it to implement their callback.

Since it yields the same result as drm_gem_cma_dumb_create we can use the default macro below.

I noticed this and thought that the driver authors probably had their reasons. Changing the driver to the default macro is probably still a good idea.

Best regards Thomas

Weather to the .dumb_create in separate patch, or squash it here - I'll leave to you. In case of the latter, please mentioned it in the commit message.

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

Hi Thomas,

Le ven. 22 mai 2020 à 15:52, Thomas Zimmermann tzimmermann@suse.de a écrit :

The ingenic driver uses the default implementation for CMA functions. The DRM_GEM_CMA_DRIVER_OPS macro now sets these defaults in struct drm_driver. All remaining operations are provided by CMA GEM object functions.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

Tested-by: Paul Cercueil paul@crapouillou.net Reviewed-by: Paul Cercueil paul@crapouillou.net

Cheers, -Paul

---

drivers/gpu/drm/ingenic/ingenic-drm.c | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm.c b/drivers/gpu/drm/ingenic/ingenic-drm.c index eff57a1f70fb0..1c1cee367b752 100644 --- a/drivers/gpu/drm/ingenic/ingenic-drm.c +++ b/drivers/gpu/drm/ingenic/ingenic-drm.c @@ -519,18 +519,7 @@ static struct drm_driver ingenic_drm_driver_data = { .patchlevel = 0,

.fops = &ingenic_drm_fops,

• .dumb_create = drm_gem_cma_dumb_create,
• .gem_free_object_unlocked = drm_gem_cma_free_object,
• .gem_vm_ops = &drm_gem_cma_vm_ops,
• .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 = drm_gem_cma_prime_get_sg_table,
• .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
• .gem_prime_vmap = drm_gem_cma_prime_vmap,
• .gem_prime_vunmap = drm_gem_cma_prime_vunmap,
• .gem_prime_mmap = drm_gem_cma_prime_mmap,

• DRM_GEM_CMA_DRIVER_OPS,

  .irq_handler = ingenic_drm_irq_handler,

};

2.26.2

On Fri, May 22, 2020 at 03:52:35PM +0200, Thomas Zimmermann wrote:

The komeda driver uses the default implementation for CMA functions; except for the .dumb_create callback. The __DRM_GEM_CMA_DRIVER_OPS macro now sets these defaults and .dumb_create in struct drm_driver. All remaining operations are provided by CMA GEM object functions.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

Acked-by: Liviu Dudau liviu.dudau@arm.com

Best regards, Liviu

---

drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c index 6b85d5f4caa85..bdfbcbc416260 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c @@ -61,16 +61,7 @@ static irqreturn_t komeda_kms_irq_handler(int irq, void *data) static struct drm_driver komeda_kms_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, .lastclose = drm_fb_helper_lastclose,

• .gem_free_object_unlocked = drm_gem_cma_free_object,
• .gem_vm_ops = &drm_gem_cma_vm_ops,
• .dumb_create = komeda_gem_cma_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 = drm_gem_cma_prime_get_sg_table,
• .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
• .gem_prime_vmap = drm_gem_cma_prime_vmap,
• .gem_prime_vunmap = drm_gem_cma_prime_vunmap,
• .gem_prime_mmap = drm_gem_cma_prime_mmap,

• __DRM_GEM_CMA_DRIVER_OPS(komeda_gem_cma_dumb_create), .fops = &komeda_cma_fops, .name = "komeda", .desc = "Arm Komeda Display Processor driver",

-- 2.26.2

On Fri, May 22, 2020 at 03:52:36PM +0200, Thomas Zimmermann wrote:

The malidp driver uses the default implementation for CMA functions; except for the .dumb_create callback. The __DRM_GEM_CMA_DRIVER_OPS macro now sets these defaults and .dumb_create in struct drm_driver. All remaining operations are provided by CMA GEM object functions.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

Acked-by: Liviu Dudau liviu.dudau@arm.com

Best regards, Liviu

---

drivers/gpu/drm/arm/malidp_drv.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c index def8c9ffafcaf..92e0bca6aa2f4 100644 --- a/drivers/gpu/drm/arm/malidp_drv.c +++ b/drivers/gpu/drm/arm/malidp_drv.c @@ -563,16 +563,7 @@ static void malidp_debugfs_init(struct drm_minor *minor)

static struct drm_driver malidp_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,

• .gem_free_object_unlocked = drm_gem_cma_free_object,
• .gem_vm_ops = &drm_gem_cma_vm_ops,
• .dumb_create = malidp_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 = drm_gem_cma_prime_get_sg_table,
• .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
• .gem_prime_vmap = drm_gem_cma_prime_vmap,
• .gem_prime_vunmap = drm_gem_cma_prime_vunmap,
• .gem_prime_mmap = drm_gem_cma_prime_mmap,

• __DRM_GEM_CMA_DRIVER_OPS(malidp_dumb_create),

#ifdef CONFIG_DEBUG_FS .debugfs_init = malidp_debugfs_init,

#endif

2.26.2

Hi

Am 22.05.20 um 22:12 schrieb Laurent Pinchart:

Hi Thomas,

Thank you for the patch.

On Fri, May 22, 2020 at 03:52:40PM +0200, Thomas Zimmermann wrote:

...

The rcar-du driver uses the default implementation for CMA functions; except for the .dumb_create callback. The __DRM_GEM_CMA_DRIVER_OPS macro now sets these defaults and .dumb_create in struct drm_driver. All remaining operations are provided by CMA GEM object functions.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

drivers/gpu/drm/rcar-du/rcar_du_drv.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 3e67cf70f0402..3728038cec1d1 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c @@ -476,16 +476,7 @@ DEFINE_DRM_GEM_CMA_FOPS(rcar_du_fops);

static struct drm_driver rcar_du_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,

• .gem_free_object_unlocked = drm_gem_cma_free_object,
• .gem_vm_ops = &drm_gem_cma_vm_ops,
• .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 = drm_gem_cma_prime_get_sg_table,
• .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
• .gem_prime_vmap = drm_gem_cma_prime_vmap,
• .gem_prime_vunmap = drm_gem_cma_prime_vunmap,
• .gem_prime_mmap = drm_gem_cma_prime_mmap,
• .dumb_create = rcar_du_dumb_create,

• __DRM_GEM_CMA_DRIVER_OPS(rcar_du_dumb_create),

Your __DRM_GEM_CMA_DRIVER_OPS is defined as

#define __DRM_GEM_CMA_DRIVER_OPS(__dumb_create) \ .gem_create_object = drm_cma_gem_create_object_default_funcs, \ .dumb_create = (__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_sg_table = drm_gem_cma_prime_import_sg_table_vmap, \ .gem_prime_mmap = drm_gem_prime_mmap

The patch thus introduces several changes:

• drm_gem_cma_prime_import_sg_table_vmap() is used instead of drm_gem_cma_prime_import_sg_table() combined with .gem_prime_vmap() and .gem_prime_vunmap(). I believe that's fine, but splitting that change in a separate commit, or at the very least explaining it in details in the commit message, would make review easier.

• .gem_create_object() is now set. That seems to be OK, but I'm not sure to grasp all the implications. This should also be explained in the commit message, and ideally split to a separate patch.

That's relevant during object creation and sets the object functions. See one of my other replies for how this can go away after all CMA drivers have been updated to GEM object functions.

• drm_gem_cma_prime_mmap() is replaced with drm_gem_prime_mmap(). Same comments :-)

I relied on the aspeed driver to be correct. After Sam's comment on that, I read the code once again several times. The original implementation clears VM_PFNMAP. And I cannot find that code any longer. Going back to the original function might be better.

This patch hides way too many changes in what is documented as just innocent refactoring. It seems other drivers are affected too.

Could you test the patchset? I don't have the HW.

Best regards Thomas

...

.fops = &rcar_du_fops, .name = "rcar-du", .desc = "Renesas R-Car Display Unit",

Hi

Am 25.05.20 um 17:38 schrieb Kieran Bingham:

On 25/05/2020 13:49, Thomas Zimmermann wrote:

...

Hi

Am 22.05.20 um 22:12 schrieb Laurent Pinchart:

...

Hi Thomas,

Thank you for the patch.

On Fri, May 22, 2020 at 03:52:40PM +0200, Thomas Zimmermann wrote:

...

The rcar-du driver uses the default implementation for CMA functions; except for the .dumb_create callback. The __DRM_GEM_CMA_DRIVER_OPS macro now sets these defaults and .dumb_create in struct drm_driver. All remaining operations are provided by CMA GEM object functions.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

drivers/gpu/drm/rcar-du/rcar_du_drv.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 3e67cf70f0402..3728038cec1d1 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c @@ -476,16 +476,7 @@ DEFINE_DRM_GEM_CMA_FOPS(rcar_du_fops);

static struct drm_driver rcar_du_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,

• .gem_free_object_unlocked = drm_gem_cma_free_object,
• .gem_vm_ops = &drm_gem_cma_vm_ops,
• .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 = drm_gem_cma_prime_get_sg_table,
• .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
• .gem_prime_vmap = drm_gem_cma_prime_vmap,
• .gem_prime_vunmap = drm_gem_cma_prime_vunmap,
• .gem_prime_mmap = drm_gem_cma_prime_mmap,
• .dumb_create = rcar_du_dumb_create,

• __DRM_GEM_CMA_DRIVER_OPS(rcar_du_dumb_create),

Your __DRM_GEM_CMA_DRIVER_OPS is defined as

#define __DRM_GEM_CMA_DRIVER_OPS(__dumb_create) \ .gem_create_object = drm_cma_gem_create_object_default_funcs, \ .dumb_create = (__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_sg_table = drm_gem_cma_prime_import_sg_table_vmap, \ .gem_prime_mmap = drm_gem_prime_mmap

The patch thus introduces several changes:

• drm_gem_cma_prime_import_sg_table_vmap() is used instead of drm_gem_cma_prime_import_sg_table() combined with .gem_prime_vmap() and .gem_prime_vunmap(). I believe that's fine, but splitting that change in a separate commit, or at the very least explaining it in details in the commit message, would make review easier.

• .gem_create_object() is now set. That seems to be OK, but I'm not sure to grasp all the implications. This should also be explained in the commit message, and ideally split to a separate patch.

That's relevant during object creation and sets the object functions. See one of my other replies for how this can go away after all CMA drivers have been updated to GEM object functions.

...

• drm_gem_cma_prime_mmap() is replaced with drm_gem_prime_mmap(). Same comments :-)

I relied on the aspeed driver to be correct. After Sam's comment on that, I read the code once again several times. The original implementation clears VM_PFNMAP. And I cannot find that code any longer. Going back to the original function might be better.

...

This patch hides way too many changes in what is documented as just innocent refactoring. It seems other drivers are affected too.

Could you test the patchset? I don't have the HW.

Digging out the branch you provided elsewhere in this thread:

...

...

...

Could you boot-test with the patchset applied?

Yes, if you have a git branch I can just build and boot I can do it quickly!

Fantastic! It's the cma-objfuncs branch of

https://gitlab.freedesktop.org/tzimmermann/linux.git

I have successfully run our display tests with your patches here on an R-Car H3 Salvator-XS(-es2).

Tested-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com

Thanks for testing.

...

Best regards Thomas

...

...

.fops = &rcar_du_fops, .name = "rcar-du", .desc = "Renesas R-Car Display Unit",

On 5/22/20 3:52 PM, Thomas Zimmermann wrote:

The stm driver uses the default implementation for CMA functions; except for the .dumb_create callback. The __DRM_GEM_CMA_DRIVER_OPS macro now sets these defaults and .dumb_create in struct drm_driver. All remaining operations are provided by CMA GEM object functions.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

drivers/gpu/drm/stm/drv.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c index 0f85dd86cafa7..d6238c71fabf6 100644 --- a/drivers/gpu/drm/stm/drv.c +++ b/drivers/gpu/drm/stm/drv.c @@ -62,16 +62,7 @@ static struct drm_driver drv_driver = { .minor = 0, .patchlevel = 0, .fops = &drv_driver_fops,

• .dumb_create = stm_gem_cma_dumb_create,
• .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
• .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
• .gem_free_object_unlocked = drm_gem_cma_free_object,
• .gem_vm_ops = &drm_gem_cma_vm_ops,
• .gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table,
• .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
• .gem_prime_vmap = drm_gem_cma_prime_vmap,
• .gem_prime_vunmap = drm_gem_cma_prime_vunmap,
• .gem_prime_mmap = drm_gem_cma_prime_mmap,

• __DRM_GEM_CMA_DRIVER_OPS(stm_gem_cma_dumb_create), };

  static int drv_load(struct drm_device *ddev)

Hi Thomas, Thank you for the patch. Reviewed-by: Philippe Cornu philippe.cornu@st.com Philippe :-)

Hi Thomas.

On Fri, May 22, 2020 at 03:52:46PM +0200, Thomas Zimmermann wrote:

The zte driver uses the default implementation for CMA functions. The DRM_GEM_CMA_DRIVER_OPS macro now sets these defaults in struct drm_driver. All remaining operations are provided by CMA GEM object functions.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

drivers/gpu/drm/zte/zx_drm_drv.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/zte/zx_drm_drv.c b/drivers/gpu/drm/zte/zx_drm_drv.c index 1141c1ed1ed04..42c59eae0ea03 100644 --- a/drivers/gpu/drm/zte/zx_drm_drv.c +++ b/drivers/gpu/drm/zte/zx_drm_drv.c @@ -36,16 +36,7 @@ DEFINE_DRM_GEM_CMA_FOPS(zx_drm_fops);

static struct drm_driver zx_drm_driver = { .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,

• .gem_free_object_unlocked = drm_gem_cma_free_object,
• .gem_vm_ops = &drm_gem_cma_vm_ops,
• .dumb_create = drm_gem_cma_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 = drm_gem_cma_prime_get_sg_table,
• .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
• .gem_prime_vmap = drm_gem_cma_prime_vmap,
• .gem_prime_vunmap = drm_gem_cma_prime_vunmap,
• .gem_prime_mmap = drm_gem_cma_prime_mmap,

• DRM_GEM_CMA_VMAP_DRIVER_OPS,

s/_VMAP// as pointed out by Emil.

Sam

.fops = &zx_drm_fops, .name = "zx-vou", .desc = "ZTE VOU Controller DRM", -- 2.26.2