[PATCH v3 17/33] drm/vkms: Introduce config for plane format

Louis Chauvet posted 33 patches 1 month, 2 weeks ago
[PATCH v3 17/33] drm/vkms: Introduce config for plane format
Posted by Louis Chauvet 1 month, 2 weeks ago
VKMS driver supports all the pixel formats for planes, but for testing it
can be useful to only advertise few of them. This new configuration
interface will allow configuring the pixel format per planes.

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_config.c | 124 +++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_config.h |  51 +++++++++++++++
 drivers/gpu/drm/vkms/vkms_plane.c  |  39 +-----------
 3 files changed, 177 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
index 5591e0baf1a1..1c97f952bf9e 100644
--- a/drivers/gpu/drm/vkms/vkms_config.c
+++ b/drivers/gpu/drm/vkms/vkms_config.c
@@ -12,6 +12,42 @@
 
 #include "vkms_config.h"
 
+static const u32 vkms_supported_plane_formats[] = {
+	DRM_FORMAT_ARGB8888,
+	DRM_FORMAT_ABGR8888,
+	DRM_FORMAT_BGRA8888,
+	DRM_FORMAT_RGBA8888,
+	DRM_FORMAT_XRGB8888,
+	DRM_FORMAT_XBGR8888,
+	DRM_FORMAT_RGB888,
+	DRM_FORMAT_BGR888,
+	DRM_FORMAT_XRGB16161616,
+	DRM_FORMAT_XBGR16161616,
+	DRM_FORMAT_ARGB16161616,
+	DRM_FORMAT_ABGR16161616,
+	DRM_FORMAT_RGB565,
+	DRM_FORMAT_BGR565,
+	DRM_FORMAT_NV12,
+	DRM_FORMAT_NV16,
+	DRM_FORMAT_NV24,
+	DRM_FORMAT_NV21,
+	DRM_FORMAT_NV61,
+	DRM_FORMAT_NV42,
+	DRM_FORMAT_YUV420,
+	DRM_FORMAT_YUV422,
+	DRM_FORMAT_YUV444,
+	DRM_FORMAT_YVU420,
+	DRM_FORMAT_YVU422,
+	DRM_FORMAT_YVU444,
+	DRM_FORMAT_P010,
+	DRM_FORMAT_P012,
+	DRM_FORMAT_P016,
+	DRM_FORMAT_R1,
+	DRM_FORMAT_R2,
+	DRM_FORMAT_R4,
+	DRM_FORMAT_R8,
+};
+
 struct vkms_config *vkms_config_create(const char *dev_name)
 {
 	struct vkms_config *config;
@@ -441,6 +477,15 @@ static void show_bitfield(struct seq_file *m, uint32_t value, const char *callba
 	}
 }
 
+static void show_formats(struct seq_file *m, u32 *formats, size_t formats_count)
+{
+	for (int i = 0; i < formats_count; i++) {
+		seq_printf(m, "%p4cc", &formats[i]);
+		if (i != formats_count - 1)
+			seq_puts(m, ", ");
+	}
+}
+
 static int vkms_config_show(struct seq_file *m, void *data)
 {
 	struct drm_debugfs_entry *entry = m->private;
@@ -482,6 +527,10 @@ static int vkms_config_show(struct seq_file *m, void *data)
 		seq_puts(m, "\n");
 		seq_printf(m, "\tdefault color range=%s\n",
 			   drm_get_color_range_name(vkms_config_plane_get_default_color_range(plane_cfg)));
+		seq_puts(m, "\tsupported formats=");
+		show_formats(m, vkms_config_plane_get_supported_formats(plane_cfg),
+			     vkms_config_plane_get_supported_formats_count(plane_cfg));
+		seq_puts(m, "\n");
 	}
 
 	vkms_config_for_each_crtc(vkmsdev->config, crtc_cfg) {
@@ -520,6 +569,11 @@ struct vkms_config_plane *vkms_config_create_plane(struct vkms_config *config)
 	if (!plane_cfg)
 		return ERR_PTR(-ENOMEM);
 
+	if (vkms_config_plane_add_all_formats(plane_cfg)) {
+		kfree(plane_cfg);
+		return ERR_PTR(-ENOMEM);
+	}
+
 	plane_cfg->config = config;
 	plane_cfg->default_pipeline = false;
 	vkms_config_plane_set_type(plane_cfg, DRM_PLANE_TYPE_OVERLAY);
@@ -650,6 +704,76 @@ static struct vkms_config_plane *vkms_config_crtc_get_plane(const struct vkms_co
 	return NULL;
 }
 
+int __must_check vkms_config_plane_add_all_formats(struct vkms_config_plane *plane_cfg)
+{
+	u32 *ret = krealloc_array(plane_cfg->supported_formats,
+				  ARRAY_SIZE(vkms_supported_plane_formats),
+				  sizeof(uint32_t), GFP_KERNEL);
+	if (!ret)
+		return -ENOMEM;
+	plane_cfg->supported_formats = ret;
+
+	memcpy(plane_cfg->supported_formats, vkms_supported_plane_formats,
+	       sizeof(vkms_supported_plane_formats));
+	plane_cfg->supported_formats_count = ARRAY_SIZE(vkms_supported_plane_formats);
+	return 0;
+}
+
+static bool is_supported_format(u32 drm_format)
+{
+	for (int i = 0; i < ARRAY_SIZE(vkms_supported_plane_formats); i++) {
+		if (vkms_supported_plane_formats[i] == drm_format)
+			return true;
+	}
+
+	return false;
+}
+
+static bool vkms_config_plane_has_format(struct vkms_config_plane *plane_cfg, u32 drm_format)
+{
+	for (unsigned int i = 0; i < plane_cfg->supported_formats_count; i++) {
+		if (plane_cfg->supported_formats[i] == drm_format)
+			return true;
+	}
+	return false;
+}
+
+int __must_check vkms_config_plane_add_format(struct vkms_config_plane *plane_cfg, u32 drm_format)
+{
+	if (!is_supported_format(drm_format))
+		return -EINVAL;
+
+	if (vkms_config_plane_has_format(plane_cfg, drm_format))
+		return 0;
+
+	u32 *new_ptr = krealloc_array(plane_cfg->supported_formats,
+				      plane_cfg->supported_formats_count + 1,
+				      sizeof(*plane_cfg->supported_formats), GFP_KERNEL);
+	if (!new_ptr)
+		return -ENOMEM;
+
+	plane_cfg->supported_formats = new_ptr;
+	plane_cfg->supported_formats[plane_cfg->supported_formats_count] = drm_format;
+	plane_cfg->supported_formats_count++;
+
+	return 0;
+}
+
+void vkms_config_plane_remove_all_formats(struct vkms_config_plane *plane_cfg)
+{
+	plane_cfg->supported_formats_count = 0;
+}
+
+void vkms_config_plane_remove_format(struct vkms_config_plane *plane_cfg, u32 drm_format)
+{
+	for (unsigned int i = 0; i < plane_cfg->supported_formats_count; i++) {
+		if (plane_cfg->supported_formats[i] == drm_format) {
+			plane_cfg->supported_formats[i] = plane_cfg->supported_formats[plane_cfg->supported_formats_count - 1];
+			plane_cfg->supported_formats_count--;
+		}
+	}
+}
+
 struct vkms_config_plane *vkms_config_crtc_primary_plane(const struct vkms_config *config,
 							 struct vkms_config_crtc *crtc_cfg)
 {
diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
index bb2721a18d61..f268c3172217 100644
--- a/drivers/gpu/drm/vkms/vkms_config.h
+++ b/drivers/gpu/drm/vkms/vkms_config.h
@@ -49,6 +49,8 @@ struct vkms_config {
  * @supported_color_encodings: Color encodings that this plane will support
  * @default_color_range: Default color range that should be used by this plane
  * @supported_color_ranges: Color ranges that this plane will support
+ * @supported_formats: List of supported formats
+ * @supported_formats_count: Length of @supported_formats
  */
 struct vkms_config_plane {
 	struct list_head link;
@@ -62,6 +64,8 @@ struct vkms_config_plane {
 	unsigned int supported_color_encodings;
 	enum drm_color_range default_color_range;
 	unsigned int supported_color_ranges;
+	u32 *supported_formats;
+	unsigned int supported_formats_count;
 	struct xarray possible_crtcs;
 	bool default_pipeline;
 
@@ -474,6 +478,53 @@ bool vkms_config_valid_plane_color_range(const struct vkms_config *config,
 					 const struct vkms_config_plane *plane_cfg);
 #endif
 
+static inline u32 *
+vkms_config_plane_get_supported_formats(const struct vkms_config_plane *plane_cfg)
+{
+	return plane_cfg->supported_formats;
+}
+
+static inline unsigned int
+vkms_config_plane_get_supported_formats_count(struct vkms_config_plane *plane_cfg)
+{
+	return plane_cfg->supported_formats_count;
+}
+
+/** vkms_config_plane_add_format - Add a format to the list of supported format of a plane
+ *
+ * The passed drm_format can already be present in the list. This may fail if the allocation of a
+ * bigger array fails.
+ *
+ * @plane_cfg: Plane to add the format to
+ * @drm_format: Format to add to this plane
+ *
+ * Returns: 0 on success, -ENOMEM if array allocation fails, -EINVAL if the format is not supported
+ * by VKMS
+ */
+int __must_check vkms_config_plane_add_format(struct vkms_config_plane *plane_cfg, u32 drm_format);
+
+/**
+ * vkms_config_plane_add_all_formats - Helper to quickly add all the supported formats
+ * @plane_cfg: Plane to add the formats to
+ *
+ * Returns: 0 on success, -ENOMEM if array allocation fails, -EINVAL if the format is not supported
+ * by VKMS
+ */
+int __must_check vkms_config_plane_add_all_formats(struct vkms_config_plane *plane_cfg);
+
+/**
+ * vkms_config_plane_remove_format - Remove a specific format from a plane
+ * @plane_cfg: Plane to remove the format to
+ * @drm_format: Format to remove
+ */
+void vkms_config_plane_remove_format(struct vkms_config_plane *plane_cfg, u32 drm_format);
+
+/**
+ * vkms_config_plane_remove_all_formats - Remove all formats from a plane
+ * @plane_cfg: Plane to remove the formats from
+ */
+void vkms_config_plane_remove_all_formats(struct vkms_config_plane *plane_cfg);
+
 /**
  * vkms_config_plane_set_name() - Set the plane name
  * @plane_cfg: Plane to set the name to
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 6461cf025a27..c2854b2117d1 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -15,42 +15,6 @@
 #include "vkms_drv.h"
 #include "vkms_formats.h"
 
-static const u32 vkms_formats[] = {
-	DRM_FORMAT_ARGB8888,
-	DRM_FORMAT_ABGR8888,
-	DRM_FORMAT_BGRA8888,
-	DRM_FORMAT_RGBA8888,
-	DRM_FORMAT_XRGB8888,
-	DRM_FORMAT_XBGR8888,
-	DRM_FORMAT_RGB888,
-	DRM_FORMAT_BGR888,
-	DRM_FORMAT_XRGB16161616,
-	DRM_FORMAT_XBGR16161616,
-	DRM_FORMAT_ARGB16161616,
-	DRM_FORMAT_ABGR16161616,
-	DRM_FORMAT_RGB565,
-	DRM_FORMAT_BGR565,
-	DRM_FORMAT_NV12,
-	DRM_FORMAT_NV16,
-	DRM_FORMAT_NV24,
-	DRM_FORMAT_NV21,
-	DRM_FORMAT_NV61,
-	DRM_FORMAT_NV42,
-	DRM_FORMAT_YUV420,
-	DRM_FORMAT_YUV422,
-	DRM_FORMAT_YUV444,
-	DRM_FORMAT_YVU420,
-	DRM_FORMAT_YVU422,
-	DRM_FORMAT_YVU444,
-	DRM_FORMAT_P010,
-	DRM_FORMAT_P012,
-	DRM_FORMAT_P016,
-	DRM_FORMAT_R1,
-	DRM_FORMAT_R2,
-	DRM_FORMAT_R4,
-	DRM_FORMAT_R8,
-};
-
 static struct drm_plane_state *
 vkms_plane_duplicate_state(struct drm_plane *plane)
 {
@@ -227,7 +191,8 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
 
 	plane = drmm_universal_plane_alloc(dev, struct vkms_plane, base, 0,
 					   &vkms_plane_funcs,
-					   vkms_formats, ARRAY_SIZE(vkms_formats),
+					   vkms_config_plane_get_supported_formats(plane_cfg),
+					   vkms_config_plane_get_supported_formats_count(plane_cfg),
 					   NULL, vkms_config_plane_get_type(plane_cfg),
 					   vkms_config_plane_get_name(plane_cfg));
 	if (IS_ERR(plane))

-- 
2.51.2
Re: [PATCH v3 17/33] drm/vkms: Introduce config for plane format
Posted by Luca Ceresoli 1 month, 2 weeks ago
On Mon Dec 22, 2025 at 11:11 AM CET, Louis Chauvet wrote:
> VKMS driver supports all the pixel formats for planes, but for testing it
> can be useful to only advertise few of them. This new configuration
> interface will allow configuring the pixel format per planes.
>
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>

> --- a/drivers/gpu/drm/vkms/vkms_config.c
> +++ b/drivers/gpu/drm/vkms/vkms_config.c

> +void vkms_config_plane_remove_all_formats(struct vkms_config_plane *plane_cfg)
> +{
> +	plane_cfg->supported_formats_count = 0;

Why not kfree(plane_cfg->supported_formats) too? You will be (re)allocating
anyway if adding one or all formats later on, so the buffer you're not
freeing won't be reused anyway.

> +}
> +
> +void vkms_config_plane_remove_format(struct vkms_config_plane *plane_cfg, u32 drm_format)
> +{
> +	for (unsigned int i = 0; i < plane_cfg->supported_formats_count; i++) {
> +		if (plane_cfg->supported_formats[i] == drm_format) {
> +			plane_cfg->supported_formats[i] = plane_cfg->supported_formats[plane_cfg->supported_formats_count - 1];

I doubt these are less than 100 chars. ;-)

> --- a/drivers/gpu/drm/vkms/vkms_config.h
> +++ b/drivers/gpu/drm/vkms/vkms_config.h
> @@ -49,6 +49,8 @@ struct vkms_config {
>   * @supported_color_encodings: Color encodings that this plane will support
>   * @default_color_range: Default color range that should be used by this plane
>   * @supported_color_ranges: Color ranges that this plane will support
> + * @supported_formats: List of supported formats
> + * @supported_formats_count: Length of @supported_formats

Nitpick: this description is not adding much. "Number of elements in
@supported_formats" would be more useful, as it would clarify it's number
of (4-bytes) elements and not size in bytes.

> +int __must_check vkms_config_plane_add_format(struct vkms_config_plane *plane_cfg, u32 drm_format);
> +
> +/**
> + * vkms_config_plane_add_all_formats - Helper to quickly add all the supported formats
> + * @plane_cfg: Plane to add the formats to
> + *
> + * Returns: 0 on success, -ENOMEM if array allocation fails, -EINVAL if the format is not supported
> + * by VKMS

The "-EINVAL ... VKMS" part of the sentence does not apply here.

> +/**
> + * vkms_config_plane_remove_format - Remove a specific format from a plane
> + * @plane_cfg: Plane to remove the format to
                                             ^^ from

Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Re: [PATCH v3 17/33] drm/vkms: Introduce config for plane format
Posted by Louis Chauvet 1 month, 1 week ago

On 12/23/25 14:58, Luca Ceresoli wrote:
> On Mon Dec 22, 2025 at 11:11 AM CET, Louis Chauvet wrote:
>> VKMS driver supports all the pixel formats for planes, but for testing it
>> can be useful to only advertise few of them. This new configuration
>> interface will allow configuring the pixel format per planes.
>>
>> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> 
>> --- a/drivers/gpu/drm/vkms/vkms_config.c
>> +++ b/drivers/gpu/drm/vkms/vkms_config.c
> 
>> +void vkms_config_plane_remove_all_formats(struct vkms_config_plane *plane_cfg)
>> +{
>> +	plane_cfg->supported_formats_count = 0;
> 
> Why not kfree(plane_cfg->supported_formats) too? You will be (re)allocating
> anyway if adding one or all formats later on, so the buffer you're not
> freeing won't be reused anyway.

IIRC realloc reuse the existing allocated buffer if it is big enough to fit the new
size. But I think this is probably an oversight here and I will add the free to clarify the situation :-)

>> +}
>> +
>> +void vkms_config_plane_remove_format(struct vkms_config_plane *plane_cfg, u32 drm_format)
>> +{
>> +	for (unsigned int i = 0; i < plane_cfg->supported_formats_count; i++) {
>> +		if (plane_cfg->supported_formats[i] == drm_format) {
>> +			plane_cfg->supported_formats[i] = plane_cfg->supported_formats[plane_cfg->supported_formats_count - 1];
> 
> I doubt these are less than 100 chars. ;-)

yes, 127, but I don't know how to format it so it is readable.

I tried

plane_cfg->supported_formats[i] =
	plane_cfg->supported_formats[plane_cfg->supported_formats_count - 1];

But the second line is 101 chars...

Checkpatch don't complain if I do:

plane_cfg->supported_formats[i] =
	plane_cfg->supported_formats[
		plane_cfg->supported_formats_count - 1];

or

plane_cfg->supported_formats[i] =
	plane_cfg->supported_formats[plane_cfg->supported_formats_count-
		1];

But that very ugly and not readable...

>> --- a/drivers/gpu/drm/vkms/vkms_config.h
>> +++ b/drivers/gpu/drm/vkms/vkms_config.h
>> @@ -49,6 +49,8 @@ struct vkms_config {
>>    * @supported_color_encodings: Color encodings that this plane will support
>>    * @default_color_range: Default color range that should be used by this plane
>>    * @supported_color_ranges: Color ranges that this plane will support
>> + * @supported_formats: List of supported formats
>> + * @supported_formats_count: Length of @supported_formats
> 
> Nitpick: this description is not adding much. "Number of elements in
> @supported_formats" would be more useful, as it would clarify it's number
> of (4-bytes) elements and not size in bytes.
> 
>> +int __must_check vkms_config_plane_add_format(struct vkms_config_plane *plane_cfg, u32 drm_format);
>> +
>> +/**
>> + * vkms_config_plane_add_all_formats - Helper to quickly add all the supported formats
>> + * @plane_cfg: Plane to add the formats to
>> + *
>> + * Returns: 0 on success, -ENOMEM if array allocation fails, -EINVAL if the format is not supported
>> + * by VKMS
> 
> The "-EINVAL ... VKMS" part of the sentence does not apply here.

True!

>> +/**
>> + * vkms_config_plane_remove_format - Remove a specific format from a plane
>> + * @plane_cfg: Plane to remove the format to
>                                               ^^ from
> 
> Luca
> 
> --
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Re: [PATCH v3 17/33] drm/vkms: Introduce config for plane format
Posted by Luca Ceresoli 1 month, 1 week ago
Hi Louis,

On Mon Dec 29, 2025 at 4:29 PM CET, Louis Chauvet wrote:
>>> --- a/drivers/gpu/drm/vkms/vkms_config.c
>>> +++ b/drivers/gpu/drm/vkms/vkms_config.c
>>
>>> +void vkms_config_plane_remove_all_formats(struct vkms_config_plane *plane_cfg)
>>> +{
>>> +	plane_cfg->supported_formats_count = 0;
>>
>> Why not kfree(plane_cfg->supported_formats) too? You will be (re)allocating
>> anyway if adding one or all formats later on, so the buffer you're not
>> freeing won't be reused anyway.
>
> IIRC realloc reuse the existing allocated buffer if it is big enough to fit the new
> size. But I think this is probably an oversight here and I will add the free to clarify the situation :-)

Thanks. My point is that keeping the buffer allocated for future use is not
great because if it is not reused it will take memory for all the remaining
uptime.

>>> +}
>>> +
>>> +void vkms_config_plane_remove_format(struct vkms_config_plane *plane_cfg, u32 drm_format)
>>> +{
>>> +	for (unsigned int i = 0; i < plane_cfg->supported_formats_count; i++) {
>>> +		if (plane_cfg->supported_formats[i] == drm_format) {
>>> +			plane_cfg->supported_formats[i] = plane_cfg->supported_formats[plane_cfg->supported_formats_count - 1];
>>
>> I doubt these are less than 100 chars. ;-)
>
> yes, 127, but I don't know how to format it so it is readable.
>
> I tried
>
> plane_cfg->supported_formats[i] =
> 	plane_cfg->supported_formats[plane_cfg->supported_formats_count - 1];
>
> But the second line is 101 chars...

I vote for this one. 101 is close enough to 100, and I see no reaonable way
to format better (and I agree the two below are ugly).

> Checkpatch don't complain if I do:
>
> plane_cfg->supported_formats[i] =
> 	plane_cfg->supported_formats[
> 		plane_cfg->supported_formats_count - 1];
>
> or
>
> plane_cfg->supported_formats[i] =
> 	plane_cfg->supported_formats[plane_cfg->supported_formats_count-
> 		1];
>
> But that very ugly and not readable...

Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com