Re: [Nouveau] [PATCH 2/4] drm/plane: Allocate planes with drm_universal_plane_alloc()
On 9/16/22 13:41, Thomas Zimmermann wrote:
[...]
>>
>>> + * @dev: DRM device
>>> + * @type: the type of the struct which contains struct _plane
>>> + * @member: the name of the _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.
>
Ah, never mind. I misread the function name definition and thought that
you had a drmm_ suffix but, now on second read I see that is only drm_
so just ignore my comment about using managed memory alloc / release.
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [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
> ---
[...]
>
> +__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.
> + * @dev: DRM device
> + * @type: the type of the struct which contains struct _plane
> + * @member: the name of the _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
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Re: [Nouveau] [PATCH 2/4] drm/plane: Allocate planes with drm_universal_plane_alloc()
Hi Thomas,
On Fri, Sep 16, 2022 at 01:31:25PM +0200, Thomas Zimmermann wrote:
> Am 16.09.22 um 13:06 schrieb Laurent Pinchart:
> > 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 ?
>
> It's used by nouveau and drm_modeset_helper.c. Since the code is
> duplicated, it seems generally better to have it located and documented
> in a central place.
>
> Although it may look somewhat pointless now, the helper will get useful
> in the future. The affected code in drm_modeset_helper is in
> drm_crtc_init(), which is also a deprecated interface; only used by
> non-atomic drivers. The function is a good candidate to be inlined into
> calling drivers. Getting drm_crtc_init() removed will allow us to
> correct these drivers' color-format handling. Once that happened,
> several more drivers will call drm_univeral_plane_alloc().
OK, works for me.
> >> Signed-off-by: Thomas Zimmermann
> >> ---
> >> 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,
> >> - _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,
> >> +_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
> >>
Re: [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
---
[...]
+__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 _plane
+ * @member: the name of the _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
--
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
OpenPGP_signature
Description: OpenPGP digital signature
Re: [Nouveau] [PATCH 2/4] drm/plane: Allocate planes with drm_universal_plane_alloc()
Hi
Am 16.09.22 um 13:06 schrieb Laurent Pinchart:
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 ?
It's used by nouveau and drm_modeset_helper.c. Since the code is
duplicated, it seems generally better to have it located and documented
in a central place.
Although it may look somewhat pointless now, the helper will get useful
in the future. The affected code in drm_modeset_helper is in
drm_crtc_init(), which is also a deprecated interface; only used by
non-atomic drivers. The function is a good candidate to be inlined into
calling drivers. Getting drm_crtc_init() removed will allow us to
correct these drivers' color-format handling. Once that happened,
several more drivers will call drm_univeral_plane_alloc().
Best regards
Thomas
Signed-off-by: Thomas Zimmermann
---
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,
- _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,
+ _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
Re: [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
> ---
> 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,
> -_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,
> + _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);
> +
> +
