Re: [PATCH RESEND v2 12/32] drm/vkms: Introduce configfs for plane color encoding
From: Louis Chauvet
Date: Fri Dec 19 2025 - 11:40:48 EST
On 12/18/25 18:59, Luca Ceresoli wrote:
On Wed Oct 29, 2025 at 3:36 PM CET, Louis Chauvet wrote:
To allows the userspace to test many hardware configuration, introduce aencodings
new interface to configure the available color encoding per planes. VKMS
supports multiple color encoding, so the userspace can choose any
combination.
The supported color encoding are configured by writing a color encoding
bitmask to the file `supported_color_encoding` and the default color
encoding is chosen by writing a color encoding bitmask to
`default_color_encoding`.
Signed-off-by: Louis Chauvet <louis.chauvet@xxxxxxxxxxx>
---
Documentation/gpu/vkms.rst | 7 ++-
drivers/gpu/drm/vkms/vkms_configfs.c | 98 ++++++++++++++++++++++++++++++++++++
2 files changed, 104 insertions(+), 1 deletion(-)
diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
index eac1a942d6c4..dab6811687a2 100644
--- a/Documentation/gpu/vkms.rst
+++ b/Documentation/gpu/vkms.rst
@@ -87,7 +87,7 @@ Start by creating one or more planes::
sudo mkdir /config/vkms/my-vkms/planes/plane0
-Planes have 4 configurable attributes:
+Planes have 6 configurable attributes:
- type: Plane type: 0 overlay, 1 primary, 2 cursor (same values as those
exposed by the "type" property of a plane)
@@ -97,6 +97,11 @@ Planes have 4 configurable attributes:
(same values as those exposed by the "rotation" property of a plane)
- default_rotation: Default rotation presented to the userspace, same values as
possible_rotations.
+- supported_color_encoding: Available encoding for a plane, as a bitmask:
+ 0x01 YCBCR_BT601, 0x02: YCBCR_BT709, 0x04 YCBCR_BT2020 (same values as those exposed^
Unintended colon? While I think it's nice to have, there is none elsewhere,
even in previous patches, and I'd say we can live happy without.
+ by the COLOR_ENCODING property of a plane)supported_color_encodings
+- default_color_encoding: Default color encoding presented to the userspace, same
+ values as supported_color_encoding
Continue by creating one or more CRTCs::
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index 7cc8ba315ef0..ee2e8d141f9e 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -439,16 +439,114 @@ static ssize_t plane_default_rotation_store(struct config_item *item,
return count;
}
+static ssize_t plane_supported_color_encodings_show(struct config_item *item, char *page)
+{
+ struct vkms_configfs_plane *plane;
+ unsigned int supported_color_encoding;
+
+ plane = plane_item_to_vkms_configfs_plane(item);
As for patch 9, for consistency:
struct vkms_configfs_plane *plane = plane_item_to_vkms_configfs_plane(item);
+
+ scoped_guard(mutex, &plane->dev->lock) {
+ supported_color_encoding = vkms_config_plane_get_supported_color_encodings(plane->config);
+ }
+
+ return sprintf(page, "%u", supported_color_encoding);
+}
+
+static ssize_t plane_supported_color_encodings_store(struct config_item *item,
+ const char *page, size_t count)
+{
+ struct vkms_configfs_plane *plane = plane_item_to_vkms_configfs_plane(item);
+ int ret, val = 0;
+
+ ret = kstrtouint(page, 10, &val);
+ if (ret)
+ return ret;
+
+ /* Should be a supported value */
+ if (val & ~(BIT(DRM_COLOR_YCBCR_BT601) |
+ BIT(DRM_COLOR_YCBCR_BT709) |
+ BIT(DRM_COLOR_YCBCR_BT2020)))
+ return -EINVAL;
+ /* Should at least provide one color range */
+ if ((val & (BIT(DRM_COLOR_YCBCR_BT601) |
+ BIT(DRM_COLOR_YCBCR_BT709) |
+ BIT(DRM_COLOR_YCBCR_BT2020))) == 0)
I'm mentioning here as it comes to mind, but it's valid for other similar
patches in this series: why not adding a
#define DRM_COLOR_ENCODINGS_SUPPORTED ( \
BIT(DRM_COLOR_YCBCR_BT601) |
BIT(DRM_COLOR_YCBCR_BT709) |
BIT(DRM_COLOR_YCBCR_BT2020))
and use it in place of the various bitwise-or sequences?
This would simplify work later on if adding a new color encoding (or color
range, or...).
Somewhat like DRM_MODE_*_MASK.
+ return -EINVAL;
+
+ scoped_guard(mutex, &plane->dev->lock) {
+ /* Ensures that the default rotation is included in supported rotation */
+ if (plane->dev->enabled)
+ return -EINVAL;
And here the comment is definitely wrong. :-)
+
+ vkms_config_plane_set_supported_color_encodings(plane->config, val);
+ }
+
+ return count;
+}
+
+/* Plane default_color_encoding : vkms/<device>/planes/<plane>/default_color_encoding */
There's no such comment in other places, so for consistency remove it (or
add it everywhere?!? ... no, just kidding).
+
+static ssize_t plane_default_color_encoding_show(struct config_item *item, char *page)
+{
+ struct vkms_configfs_plane *plane;
+ unsigned int default_color_encoding;
+
+ plane = plane_item_to_vkms_configfs_plane(item);
+
+ scoped_guard(mutex, &plane->dev->lock) {
+ default_color_encoding = vkms_config_plane_get_default_color_encoding(plane->config);
+ }
+
+ return sprintf(page, "%u", default_color_encoding);
+}
+
+static ssize_t plane_default_color_encoding_store(struct config_item *item,
+ const char *page, size_t count)
+{
+ struct vkms_configfs_plane *plane = plane_item_to_vkms_configfs_plane(item);
+ int ret, val = 0;
+
+ ret = kstrtouint(page, 10, &val);
+ if (ret)
+ return ret;
+
+ /* Should be a supported value */
+ if (val & ~(BIT(DRM_COLOR_YCBCR_BT601) |
+ BIT(DRM_COLOR_YCBCR_BT709) |
+ BIT(DRM_COLOR_YCBCR_BT2020)))
+ return -EINVAL;
+ /* Should at least provide one color range */
+ if ((val & (BIT(DRM_COLOR_YCBCR_BT601) |
+ BIT(DRM_COLOR_YCBCR_BT709) |
+ BIT(DRM_COLOR_YCBCR_BT2020))) == 0)
+ return -EINVAL;
Shouldn't you check that exactly one bit is set? As in patch 9.
Because this code is wrong... the default rotation should be DRM_COLOR_YCBCR_BT601 / DRM_COLOR_YCBCR_BT709 / DRM_COLOR_YCBCR_BT2020
not a bitfield...
+
+ scoped_guard(mutex, &plane->dev->lock) {
+ /* Ensures that the default rotation is included in supported rotation */
+ if (plane->dev->enabled)
+ return -EINVAL;
As before, wrong comment.
Luca
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com