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

[Thomas Zimmermann]

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/dri-devel/attachments/20220916/68dbdd4b/attachment-0001.sig>