Re: [PATCH 2/4] drm/plane: Allocate planes with drm_universal_plane_alloc()

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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

-- 
Regards,

Laurent Pinchart