Thomas Zimmermann
2022-Sep-09 10:59 UTC
[Nouveau] [PATCH 2/4] drm/plane: Allocate planes with drm_universal_plane_alloc()
Provide drm_univeral_plane_alloc(), which allocated an initializes a
plane. Code for non-atomic drivers uses this pattern. Convert it to
the new function. The modeset helpers contain a quirk for handling their
color formats differently. Set the flag outside plane allocation.
The new function is already deprecated to some extend. Drivers should
rather use drmm_univeral_plane_alloc() or drm_universal_plane_init().
Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
---
drivers/gpu/drm/drm_modeset_helper.c | 61 +++++++++++--------------
drivers/gpu/drm/drm_plane.c | 38 +++++++++++++++
drivers/gpu/drm/nouveau/dispnv04/crtc.c | 41 ++++++-----------
include/drm/drm_plane.h | 44 ++++++++++++++++++
4 files changed, 121 insertions(+), 63 deletions(-)
diff --git a/drivers/gpu/drm/drm_modeset_helper.c
b/drivers/gpu/drm/drm_modeset_helper.c
index 611dd01fb604..38040eebfa16 100644
--- a/drivers/gpu/drm/drm_modeset_helper.c
+++ b/drivers/gpu/drm/drm_modeset_helper.c
@@ -113,38 +113,6 @@ static const struct drm_plane_funcs primary_plane_funcs = {
.destroy = drm_plane_helper_destroy,
};
-static struct drm_plane *create_primary_plane(struct drm_device *dev)
-{
- struct drm_plane *primary;
- int ret;
-
- primary = kzalloc(sizeof(*primary), GFP_KERNEL);
- if (primary == NULL) {
- DRM_DEBUG_KMS("Failed to allocate primary plane\n");
- return NULL;
- }
-
- /*
- * Remove the format_default field from drm_plane when dropping
- * this helper.
- */
- primary->format_default = true;
-
- /* possible_crtc's will be filled in later by crtc_init */
- ret = drm_universal_plane_init(dev, primary, 0,
- &primary_plane_funcs,
- safe_modeset_formats,
- ARRAY_SIZE(safe_modeset_formats),
- NULL,
- DRM_PLANE_TYPE_PRIMARY, NULL);
- if (ret) {
- kfree(primary);
- primary = NULL;
- }
-
- return primary;
-}
-
/**
* drm_crtc_init - Legacy CRTC initialization function
* @dev: DRM device
@@ -176,10 +144,33 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc
*crtc,
const struct drm_crtc_funcs *funcs)
{
struct drm_plane *primary;
+ int ret;
+
+ /* possible_crtc's will be filled in later by crtc_init */
+ primary = __drm_universal_plane_alloc(dev, sizeof(*primary), 0, 0,
+ &primary_plane_funcs,
+ safe_modeset_formats,
+ ARRAY_SIZE(safe_modeset_formats),
+ NULL, DRM_PLANE_TYPE_PRIMARY, NULL);
+ if (IS_ERR(primary))
+ return PTR_ERR(primary);
- primary = create_primary_plane(dev);
- return drm_crtc_init_with_planes(dev, crtc, primary, NULL, funcs,
- NULL);
+ /*
+ * Remove the format_default field from drm_plane when dropping
+ * this helper.
+ */
+ primary->format_default = true;
+
+ ret = drm_crtc_init_with_planes(dev, crtc, primary, NULL, funcs, NULL);
+ if (ret)
+ goto err_drm_plane_cleanup;
+
+ return 0;
+
+err_drm_plane_cleanup:
+ drm_plane_cleanup(primary);
+ kfree(primary);
+ return ret;
}
EXPORT_SYMBOL(drm_crtc_init);
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
index 0f14b4d3bb10..33357629a7f5 100644
--- a/drivers/gpu/drm/drm_plane.c
+++ b/drivers/gpu/drm/drm_plane.c
@@ -448,6 +448,44 @@ void *__drmm_universal_plane_alloc(struct drm_device *dev,
size_t size,
}
EXPORT_SYMBOL(__drmm_universal_plane_alloc);
+void *__drm_universal_plane_alloc(struct drm_device *dev, size_t size,
+ size_t offset, uint32_t possible_crtcs,
+ const struct drm_plane_funcs *funcs,
+ const uint32_t *formats, unsigned int format_count,
+ const uint64_t *format_modifiers,
+ enum drm_plane_type type,
+ const char *name, ...)
+{
+ void *container;
+ struct drm_plane *plane;
+ va_list ap;
+ int ret;
+
+ if (drm_WARN_ON(dev, !funcs))
+ return ERR_PTR(-EINVAL);
+
+ container = kzalloc(size, GFP_KERNEL);
+ if (!container)
+ return ERR_PTR(-ENOMEM);
+
+ plane = container + offset;
+
+ va_start(ap, name);
+ ret = __drm_universal_plane_init(dev, plane, possible_crtcs, funcs,
+ formats, format_count, format_modifiers,
+ type, name, ap);
+ va_end(ap);
+ if (ret)
+ goto err_kfree;
+
+ return container;
+
+err_kfree:
+ kfree(container);
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL(__drm_universal_plane_alloc);
+
int drm_plane_register_all(struct drm_device *dev)
{
unsigned int num_planes = 0;
diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c
b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
index 660c4cbc0b3d..6b8a014b5e97 100644
--- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c
+++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c
@@ -1281,32 +1281,6 @@ static const struct drm_plane_funcs
nv04_primary_plane_funcs = {
.destroy = drm_plane_helper_destroy,
};
-static struct drm_plane *
-create_primary_plane(struct drm_device *dev)
-{
- struct drm_plane *primary;
- int ret;
-
- primary = kzalloc(sizeof(*primary), GFP_KERNEL);
- if (primary == NULL) {
- DRM_DEBUG_KMS("Failed to allocate primary plane\n");
- return NULL;
- }
-
- /* possible_crtc's will be filled in later by crtc_init */
- ret = drm_universal_plane_init(dev, primary, 0,
- &nv04_primary_plane_funcs,
- modeset_formats,
- ARRAY_SIZE(modeset_formats), NULL,
- DRM_PLANE_TYPE_PRIMARY, NULL);
- if (ret) {
- kfree(primary);
- primary = NULL;
- }
-
- return primary;
-}
-
static int nv04_crtc_vblank_handler(struct nvif_notify *notify)
{
struct nouveau_crtc *nv_crtc @@ -1321,6 +1295,7 @@ nv04_crtc_create(struct
drm_device *dev, int crtc_num)
{
struct nouveau_display *disp = nouveau_display(dev);
struct nouveau_crtc *nv_crtc;
+ struct drm_plane *primary;
int ret;
nv_crtc = kzalloc(sizeof(*nv_crtc), GFP_KERNEL);
@@ -1335,8 +1310,18 @@ nv04_crtc_create(struct drm_device *dev, int crtc_num)
nv_crtc->save = nv_crtc_save;
nv_crtc->restore = nv_crtc_restore;
- drm_crtc_init_with_planes(dev, &nv_crtc->base,
- create_primary_plane(dev), NULL,
+ primary = __drm_universal_plane_alloc(dev, sizeof(*primary), 0, 0,
+ &nv04_primary_plane_funcs,
+ modeset_formats,
+ ARRAY_SIZE(modeset_formats), NULL,
+ DRM_PLANE_TYPE_PRIMARY, NULL);
+ if (IS_ERR(primary)) {
+ ret = PTR_ERR(primary);
+ kfree(nv_crtc);
+ return ret;
+ }
+
+ drm_crtc_init_with_planes(dev, &nv_crtc->base, primary, NULL,
&nv04_crtc_funcs, NULL);
drm_crtc_helper_add(&nv_crtc->base, &nv04_crtc_helper_funcs);
drm_mode_crtc_set_gamma_size(&nv_crtc->base, 256);
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 910cb941f3d5..21dfa7f97948 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -809,6 +809,50 @@ void *__drmm_universal_plane_alloc(struct drm_device *dev,
format_count, format_modifiers, \
plane_type, name, ##__VA_ARGS__))
+__printf(10, 11)
+void *__drm_universal_plane_alloc(struct drm_device *dev,
+ size_t size, size_t offset,
+ uint32_t possible_crtcs,
+ const struct drm_plane_funcs *funcs,
+ const uint32_t *formats,
+ unsigned int format_count,
+ const uint64_t *format_modifiers,
+ enum drm_plane_type plane_type,
+ const char *name, ...);
+
+/**
+ * drm_universal_plane_alloc - Allocate and initialize an universal plane
object
+ * @dev: DRM device
+ * @type: the type of the struct which contains struct &drm_plane
+ * @member: the name of the &drm_plane within @type
+ * @possible_crtcs: bitmask of possible CRTCs
+ * @funcs: callbacks for the new plane
+ * @formats: array of supported formats (DRM_FORMAT\_\*)
+ * @format_count: number of elements in @formats
+ * @format_modifiers: array of struct drm_format modifiers terminated by
+ * DRM_FORMAT_MOD_INVALID
+ * @plane_type: type of plane (overlay, primary, cursor)
+ * @name: printf style format string for the plane name, or NULL for default
name
+ *
+ * Allocates and initializes a plane object of type @type. The caller
+ * is responsible for releasing the allocated memory with kfree().
+ *
+ * Drivers are encouraged to use drmm_universal_plane_alloc() instead.
+ *
+ * Drivers that only support the DRM_FORMAT_MOD_LINEAR modifier support may set
+ * @format_modifiers to NULL. The plane will advertise the linear modifier.
+ *
+ * Returns:
+ * Pointer to new plane, or ERR_PTR on failure.
+ */
+#define drm_universal_plane_alloc(dev, type, member, possible_crtcs, funcs,
formats, \
+ format_count, format_modifiers, plane_type, name, ...) \
+ ((type *)__drm_universal_plane_alloc(dev, sizeof(type), \
+ offsetof(type, member), \
+ possible_crtcs, funcs, formats, \
+ format_count, format_modifiers, \
+ plane_type, name, ##__VA_ARGS__))
+
/**
* drm_plane_index - find the index of a registered plane
* @plane: plane to find index for
--
2.37.2
Laurent Pinchart
2022-Sep-16 11:06 UTC
[Nouveau] [PATCH 2/4] drm/plane: Allocate planes with drm_universal_plane_alloc()
Hi Thomas, Thank you for the patch. On Fri, Sep 09, 2022 at 12:59:45PM +0200, Thomas Zimmermann wrote:> Provide drm_univeral_plane_alloc(), which allocated an initializes a > plane. Code for non-atomic drivers uses this pattern. Convert it to > the new function. The modeset helpers contain a quirk for handling their > color formats differently. Set the flag outside plane allocation. > > The new function is already deprecated to some extend. Drivers should > rather use drmm_univeral_plane_alloc() or drm_universal_plane_init().If this is already deprecated and used by a single driver, what is the point ?> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de> > --- > drivers/gpu/drm/drm_modeset_helper.c | 61 +++++++++++-------------- > drivers/gpu/drm/drm_plane.c | 38 +++++++++++++++ > drivers/gpu/drm/nouveau/dispnv04/crtc.c | 41 ++++++----------- > include/drm/drm_plane.h | 44 ++++++++++++++++++ > 4 files changed, 121 insertions(+), 63 deletions(-) > > diff --git a/drivers/gpu/drm/drm_modeset_helper.c b/drivers/gpu/drm/drm_modeset_helper.c > index 611dd01fb604..38040eebfa16 100644 > --- a/drivers/gpu/drm/drm_modeset_helper.c > +++ b/drivers/gpu/drm/drm_modeset_helper.c > @@ -113,38 +113,6 @@ static const struct drm_plane_funcs primary_plane_funcs = { > .destroy = drm_plane_helper_destroy, > }; > > -static struct drm_plane *create_primary_plane(struct drm_device *dev) > -{ > - struct drm_plane *primary; > - int ret; > - > - primary = kzalloc(sizeof(*primary), GFP_KERNEL); > - if (primary == NULL) { > - DRM_DEBUG_KMS("Failed to allocate primary plane\n"); > - return NULL; > - } > - > - /* > - * Remove the format_default field from drm_plane when dropping > - * this helper. > - */ > - primary->format_default = true; > - > - /* possible_crtc's will be filled in later by crtc_init */ > - ret = drm_universal_plane_init(dev, primary, 0, > - &primary_plane_funcs, > - safe_modeset_formats, > - ARRAY_SIZE(safe_modeset_formats), > - NULL, > - DRM_PLANE_TYPE_PRIMARY, NULL); > - if (ret) { > - kfree(primary); > - primary = NULL; > - } > - > - return primary; > -} > - > /** > * drm_crtc_init - Legacy CRTC initialization function > * @dev: DRM device > @@ -176,10 +144,33 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc, > const struct drm_crtc_funcs *funcs) > { > struct drm_plane *primary; > + int ret; > + > + /* possible_crtc's will be filled in later by crtc_init */ > + primary = __drm_universal_plane_alloc(dev, sizeof(*primary), 0, 0, > + &primary_plane_funcs, > + safe_modeset_formats, > + ARRAY_SIZE(safe_modeset_formats), > + NULL, DRM_PLANE_TYPE_PRIMARY, NULL); > + if (IS_ERR(primary)) > + return PTR_ERR(primary); > > - primary = create_primary_plane(dev); > - return drm_crtc_init_with_planes(dev, crtc, primary, NULL, funcs, > - NULL); > + /* > + * Remove the format_default field from drm_plane when dropping > + * this helper. > + */ > + primary->format_default = true; > + > + ret = drm_crtc_init_with_planes(dev, crtc, primary, NULL, funcs, NULL); > + if (ret) > + goto err_drm_plane_cleanup; > + > + return 0; > + > +err_drm_plane_cleanup: > + drm_plane_cleanup(primary); > + kfree(primary); > + return ret; > } > EXPORT_SYMBOL(drm_crtc_init); > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index 0f14b4d3bb10..33357629a7f5 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -448,6 +448,44 @@ void *__drmm_universal_plane_alloc(struct drm_device *dev, size_t size, > } > EXPORT_SYMBOL(__drmm_universal_plane_alloc); > > +void *__drm_universal_plane_alloc(struct drm_device *dev, size_t size, > + size_t offset, uint32_t possible_crtcs, > + const struct drm_plane_funcs *funcs, > + const uint32_t *formats, unsigned int format_count, > + const uint64_t *format_modifiers, > + enum drm_plane_type type, > + const char *name, ...) > +{ > + void *container; > + struct drm_plane *plane; > + va_list ap; > + int ret; > + > + if (drm_WARN_ON(dev, !funcs)) > + return ERR_PTR(-EINVAL); > + > + container = kzalloc(size, GFP_KERNEL); > + if (!container) > + return ERR_PTR(-ENOMEM); > + > + plane = container + offset; > + > + va_start(ap, name); > + ret = __drm_universal_plane_init(dev, plane, possible_crtcs, funcs, > + formats, format_count, format_modifiers, > + type, name, ap); > + va_end(ap); > + if (ret) > + goto err_kfree; > + > + return container; > + > +err_kfree: > + kfree(container); > + return ERR_PTR(ret); > +} > +EXPORT_SYMBOL(__drm_universal_plane_alloc); > + > int drm_plane_register_all(struct drm_device *dev) > { > unsigned int num_planes = 0; > diff --git a/drivers/gpu/drm/nouveau/dispnv04/crtc.c b/drivers/gpu/drm/nouveau/dispnv04/crtc.c > index 660c4cbc0b3d..6b8a014b5e97 100644 > --- a/drivers/gpu/drm/nouveau/dispnv04/crtc.c > +++ b/drivers/gpu/drm/nouveau/dispnv04/crtc.c > @@ -1281,32 +1281,6 @@ static const struct drm_plane_funcs nv04_primary_plane_funcs = { > .destroy = drm_plane_helper_destroy, > }; > > -static struct drm_plane * > -create_primary_plane(struct drm_device *dev) > -{ > - struct drm_plane *primary; > - int ret; > - > - primary = kzalloc(sizeof(*primary), GFP_KERNEL); > - if (primary == NULL) { > - DRM_DEBUG_KMS("Failed to allocate primary plane\n"); > - return NULL; > - } > - > - /* possible_crtc's will be filled in later by crtc_init */ > - ret = drm_universal_plane_init(dev, primary, 0, > - &nv04_primary_plane_funcs, > - modeset_formats, > - ARRAY_SIZE(modeset_formats), NULL, > - DRM_PLANE_TYPE_PRIMARY, NULL); > - if (ret) { > - kfree(primary); > - primary = NULL; > - } > - > - return primary; > -} > - > static int nv04_crtc_vblank_handler(struct nvif_notify *notify) > { > struct nouveau_crtc *nv_crtc > @@ -1321,6 +1295,7 @@ nv04_crtc_create(struct drm_device *dev, int crtc_num) > { > struct nouveau_display *disp = nouveau_display(dev); > struct nouveau_crtc *nv_crtc; > + struct drm_plane *primary; > int ret; > > nv_crtc = kzalloc(sizeof(*nv_crtc), GFP_KERNEL); > @@ -1335,8 +1310,18 @@ nv04_crtc_create(struct drm_device *dev, int crtc_num) > nv_crtc->save = nv_crtc_save; > nv_crtc->restore = nv_crtc_restore; > > - drm_crtc_init_with_planes(dev, &nv_crtc->base, > - create_primary_plane(dev), NULL, > + primary = __drm_universal_plane_alloc(dev, sizeof(*primary), 0, 0, > + &nv04_primary_plane_funcs, > + modeset_formats, > + ARRAY_SIZE(modeset_formats), NULL, > + DRM_PLANE_TYPE_PRIMARY, NULL); > + if (IS_ERR(primary)) { > + ret = PTR_ERR(primary); > + kfree(nv_crtc); > + return ret; > + } > + > + drm_crtc_init_with_planes(dev, &nv_crtc->base, primary, NULL, > &nv04_crtc_funcs, NULL); > drm_crtc_helper_add(&nv_crtc->base, &nv04_crtc_helper_funcs); > drm_mode_crtc_set_gamma_size(&nv_crtc->base, 256); > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > index 910cb941f3d5..21dfa7f97948 100644 > --- a/include/drm/drm_plane.h > +++ b/include/drm/drm_plane.h > @@ -809,6 +809,50 @@ void *__drmm_universal_plane_alloc(struct drm_device *dev, > format_count, format_modifiers, \ > plane_type, name, ##__VA_ARGS__)) > > +__printf(10, 11) > +void *__drm_universal_plane_alloc(struct drm_device *dev, > + size_t size, size_t offset, > + uint32_t possible_crtcs, > + const struct drm_plane_funcs *funcs, > + const uint32_t *formats, > + unsigned int format_count, > + const uint64_t *format_modifiers, > + enum drm_plane_type plane_type, > + const char *name, ...); > + > +/** > + * drm_universal_plane_alloc - Allocate and initialize an universal plane object > + * @dev: DRM device > + * @type: the type of the struct which contains struct &drm_plane > + * @member: the name of the &drm_plane within @type > + * @possible_crtcs: bitmask of possible CRTCs > + * @funcs: callbacks for the new plane > + * @formats: array of supported formats (DRM_FORMAT\_\*) > + * @format_count: number of elements in @formats > + * @format_modifiers: array of struct drm_format modifiers terminated by > + * DRM_FORMAT_MOD_INVALID > + * @plane_type: type of plane (overlay, primary, cursor) > + * @name: printf style format string for the plane name, or NULL for default name > + * > + * Allocates and initializes a plane object of type @type. The caller > + * is responsible for releasing the allocated memory with kfree(). > + * > + * Drivers are encouraged to use drmm_universal_plane_alloc() instead. > + * > + * Drivers that only support the DRM_FORMAT_MOD_LINEAR modifier support may set > + * @format_modifiers to NULL. The plane will advertise the linear modifier. > + * > + * Returns: > + * Pointer to new plane, or ERR_PTR on failure. > + */ > +#define drm_universal_plane_alloc(dev, type, member, possible_crtcs, funcs, formats, \ > + format_count, format_modifiers, plane_type, name, ...) \ > + ((type *)__drm_universal_plane_alloc(dev, sizeof(type), \ > + offsetof(type, member), \ > + possible_crtcs, funcs, formats, \ > + format_count, format_modifiers, \ > + plane_type, name, ##__VA_ARGS__)) > + > /** > * drm_plane_index - find the index of a registered plane > * @plane: plane to find index for > -- > 2.37.2 >-- Regards, Laurent Pinchart
Javier Martinez Canillas
2022-Sep-16 11:22 UTC
[Nouveau] [PATCH 2/4] drm/plane: Allocate planes with drm_universal_plane_alloc()
On 9/9/22 12:59, Thomas Zimmermann wrote:> Provide drm_univeral_plane_alloc(), which allocated an initializes a > plane. Code for non-atomic drivers uses this pattern. Convert it to > the new function. The modeset helpers contain a quirk for handling their > color formats differently. Set the flag outside plane allocation. > > The new function is already deprecated to some extend. Drivers should > rather use drmm_univeral_plane_alloc() or drm_universal_plane_init(). > > Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de> > ---[...]> > +__printf(10, 11) > +void *__drm_universal_plane_alloc(struct drm_device *dev, > + size_t size, size_t offset, > + uint32_t possible_crtcs, > + const struct drm_plane_funcs *funcs, > + const uint32_t *formats, > + unsigned int format_count, > + const uint64_t *format_modifiers, > + enum drm_plane_type plane_type, > + const char *name, ...); > + > +/** > + * drm_universal_plane_alloc - Allocate and initialize an universal plane objectShould functions kernel-doc definitions use parenthesis or not? I see in https://elixir.bootlin.com/linux/latest/source/Documentation/doc-guide/kernel-doc.rst#L59 that the example has it but notice that there is not consistency in DRM about this.> + * @dev: DRM device > + * @type: the type of the struct which contains struct &drm_plane > + * @member: the name of the &drm_plane within @type > + * @possible_crtcs: bitmask of possible CRTCs > + * @funcs: callbacks for the new plane > + * @formats: array of supported formats (DRM_FORMAT\_\*) > + * @format_count: number of elements in @formats > + * @format_modifiers: array of struct drm_format modifiers terminated by > + * DRM_FORMAT_MOD_INVALID > + * @plane_type: type of plane (overlay, primary, cursor) > + * @name: printf style format string for the plane name, or NULL for default name > + * > + * Allocates and initializes a plane object of type @type. The caller > + * is responsible for releasing the allocated memory with kfree(). > + *I thought that all the drmm_*_alloc() managed interfaces should use the drmm_k{z,m}alloc() helpers, so that the memory is automatically freed on the last drm_dev_put() call ? Other than those two nits, the patch looks good to me. Reviewed-by: Javier Martinez Canillas <javierm at redhat.com> -- Best regards, Javier Martinez Canillas Core Platforms Red Hat
Thomas Zimmermann
2022-Sep-16 11:41 UTC
[Nouveau] [PATCH 2/4] drm/plane: Allocate planes with drm_universal_plane_alloc()
Hi Am 16.09.22 um 13:22 schrieb Javier Martinez Canillas:> On 9/9/22 12:59, Thomas Zimmermann wrote: >> Provide drm_univeral_plane_alloc(), which allocated an initializes a >> plane. Code for non-atomic drivers uses this pattern. Convert it to >> the new function. The modeset helpers contain a quirk for handling their >> color formats differently. Set the flag outside plane allocation. >> >> The new function is already deprecated to some extend. Drivers should >> rather use drmm_univeral_plane_alloc() or drm_universal_plane_init(). >> >> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de> >> --- > > [...] > >> >> +__printf(10, 11) >> +void *__drm_universal_plane_alloc(struct drm_device *dev, >> + size_t size, size_t offset, >> + uint32_t possible_crtcs, >> + const struct drm_plane_funcs *funcs, >> + const uint32_t *formats, >> + unsigned int format_count, >> + const uint64_t *format_modifiers, >> + enum drm_plane_type plane_type, >> + const char *name, ...); >> + >> +/** >> + * drm_universal_plane_alloc - Allocate and initialize an universal plane object > > Should functions kernel-doc definitions use parenthesis or not? I see in > https://elixir.bootlin.com/linux/latest/source/Documentation/doc-guide/kernel-doc.rst#L59 > that the example has it but notice that there is not consistency in DRM > about this.A wasn't aware of this convention.> >> + * @dev: DRM device >> + * @type: the type of the struct which contains struct &drm_plane >> + * @member: the name of the &drm_plane within @type >> + * @possible_crtcs: bitmask of possible CRTCs >> + * @funcs: callbacks for the new plane >> + * @formats: array of supported formats (DRM_FORMAT\_\*) >> + * @format_count: number of elements in @formats >> + * @format_modifiers: array of struct drm_format modifiers terminated by >> + * DRM_FORMAT_MOD_INVALID >> + * @plane_type: type of plane (overlay, primary, cursor) >> + * @name: printf style format string for the plane name, or NULL for default name >> + * >> + * Allocates and initializes a plane object of type @type. The caller >> + * is responsible for releasing the allocated memory with kfree(). >> + * > > I thought that all the drmm_*_alloc() managed interfaces should use the > drmm_k{z,m}alloc() helpers, so that the memory is automatically freed > on the last drm_dev_put() call ?For new drivers, managed cleanup is preferred. But this is an existing unmanaged cleanup. I don't know if drmm_ changes the semantics in unexpected ways (well, probably not). With managed memory releases, I was worried that plane memory might stay around longer than expected. And we'd have to fix the plane-destroy function in each user as well. Adding the existing code as a new function is the trivial solution and allows to address each driver individually. Also see my reply to Laurent's question on that topic. Best regards Thomas> > Other than those two nits, the patch looks good to me. > > Reviewed-by: Javier Martinez Canillas <javierm at redhat.com> >-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 N?rnberg, Germany (HRB 36809, AG N?rnberg) Gesch?ftsf?hrer: Ivo Totev -------------- next part -------------- A non-text attachment was scrubbed... Name: OpenPGP_signature Type: application/pgp-signature Size: 840 bytes Desc: OpenPGP digital signature URL: <https://lists.freedesktop.org/archives/nouveau/attachments/20220916/68dbdd4b/attachment-0001.sig>