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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[...]

+__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@xxxxxxxxxx>


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

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux