Re: [PATCH v2 5/6] drm/vkms: Support enabling ConfigFS devices

From: Marius Vlad
Date: Fri Aug 18 2023 - 02:55:12 EST


Hi,

Seem my comments below.

On 8/18/23 08:24, Brandon Ross Pollack wrote:

On 8/15/23 23:01, Marius Vlad wrote:
Hi,

See below some minor comments:

On Fri, Jun 23, 2023 at 06:23:47PM -0400, Jim Shargo wrote:
VKMS now supports creating and using virtual devices!

In addition to the enabling logic, this commit also prevents users from
adding new objects once a card is registered.

Signed-off-by: Jim Shargo <jshargo@xxxxxxxxxxxx>
---
  drivers/gpu/drm/vkms/vkms_configfs.c |  37 +++--
  drivers/gpu/drm/vkms/vkms_crtc.c     |   4 +-
  drivers/gpu/drm/vkms/vkms_drv.c      |   3 +-
  drivers/gpu/drm/vkms/vkms_drv.h      |   2 +-
  drivers/gpu/drm/vkms/vkms_output.c   | 201 +++++++++++++++++++++++----
  5 files changed, 202 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index 544024735d19..f5eed6d23dcd 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -504,29 +504,40 @@ static ssize_t device_enabled_store(struct config_item *item, const char *buf,
  {
      struct vkms_configfs *configfs = item_to_configfs(item);
      struct vkms_device *device;
-    int value, ret;
+    int enabled, ret;
-    ret = kstrtoint(buf, 0, &value);
+    ret = kstrtoint(buf, 0, &enabled);
      if (ret)
          return ret;
-    if (value != 1)
-        return -EINVAL;
-
-    mutex_lock(&configfs->lock);
-
-    if (configfs->vkms_device) {
+    if (enabled == 0) {
+        mutex_lock(&configfs->lock);
+        if (configfs->vkms_device) {
+            vkms_remove_device(configfs->vkms_device);
+            configfs->vkms_device = NULL;
+        }
          mutex_unlock(&configfs->lock);
+
          return len;
      }
-    device = vkms_add_device(configfs);
-    mutex_unlock(&configfs->lock);
+    if (enabled == 1) {
+        mutex_lock(&configfs->lock);
+        if (!configfs->vkms_device) {
+            device = vkms_add_device(configfs);
+            if (IS_ERR(device)) {
+                mutex_unlock(&configfs->lock);
+                return -PTR_ERR(device);
+            }
+
+            configfs->vkms_device = device;
+        }
+        mutex_unlock(&configfs->lock);
-    if (IS_ERR(device))
-        return -PTR_ERR(device);
+        return len;
+    }
-    return len;
+    return -EINVAL;
  }
  CONFIGFS_ATTR(device_, enabled);
diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index d91e49c53adc..5ebb5264f6ef 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -278,7 +278,7 @@ static const struct drm_crtc_helper_funcs vkms_crtc_helper_funcs = {
  struct vkms_crtc *vkms_crtc_init(struct vkms_device *vkmsdev,
                   struct drm_plane *primary,
-                 struct drm_plane *cursor)
+                 struct drm_plane *cursor, const char *name)
  {
      struct drm_device *dev = &vkmsdev->drm;
      struct vkms_crtc *vkms_crtc;
@@ -290,7 +290,7 @@ struct vkms_crtc *vkms_crtc_init(struct vkms_device *vkmsdev,
      vkms_crtc = &vkmsdev->output.crtcs[vkmsdev->output.num_crtcs++];
      ret = drmm_crtc_init_with_planes(dev, &vkms_crtc->base, primary, cursor,
-                     &vkms_crtc_funcs, NULL);
+                     &vkms_crtc_funcs, name);
      if (ret) {
          DRM_ERROR("Failed to init CRTC\n");
          goto out_error;
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 1b5b7143792f..314a04659c5f 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -210,7 +210,7 @@ static int vkms_platform_probe(struct platform_device *pdev)
      ret = drm_dev_register(&vkms_device->drm, 0);
      if (ret) {
          DRM_ERROR("Unable to register device with id %d\n", pdev->id);
-        return ret;
+        goto out_release_group;
      }
      drm_fbdev_generic_setup(&vkms_device->drm, 0);
@@ -256,6 +256,7 @@ struct vkms_device *vkms_add_device(struct vkms_configfs *configfs)
              dev, &vkms_platform_driver.driver))) {
          pdev = to_platform_device(dev);
          max_id = max(max_id, pdev->id);
+        put_device(dev);
      }
      pdev = platform_device_register_data(NULL, DRIVER_NAME, max_id + 1,
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 3634eeeb4548..3d592d085f49 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -239,7 +239,7 @@ void vkms_remove_device(struct vkms_device *vkms_device);
  /* CRTC */
  struct vkms_crtc *vkms_crtc_init(struct vkms_device *vkmsdev,
                   struct drm_plane *primary,
-                 struct drm_plane *cursor);
+                 struct drm_plane *cursor, const char *name);
  int vkms_output_init(struct vkms_device *vkmsdev);
  int vkms_output_init_default(struct vkms_device *vkmsdev);
diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
index c9e6c653de19..806ff01954ad 100644
--- a/drivers/gpu/drm/vkms/vkms_output.c
+++ b/drivers/gpu/drm/vkms/vkms_output.c
@@ -2,8 +2,10 @@
  #include <drm/drm_atomic_helper.h>
  #include <drm/drm_connector.h>
+#include <drm/drm_crtc.h>
  #include <drm/drm_edid.h>
  #include <drm/drm_encoder.h>
+#include <drm/drm_plane.h>
  #include <drm/drm_probe_helper.h>
  #include <drm/drm_simple_kms_helper.h>
@@ -82,7 +84,6 @@ static struct drm_encoder *vkms_encoder_init(struct vkms_device *vkms_device)
  int vkms_output_init_default(struct vkms_device *vkmsdev)
  {
-    struct vkms_output *output = &vkmsdev->output;
      struct drm_device *dev = &vkmsdev->drm;
      struct drm_connector *connector;
      struct drm_encoder *encoder;
@@ -101,8 +102,7 @@ int vkms_output_init_default(struct vkms_device *vkmsdev)
              struct vkms_plane *overlay = vkms_plane_init(
                  vkmsdev, DRM_PLANE_TYPE_OVERLAY);
              if (IS_ERR(overlay)) {
-                ret = PTR_ERR(overlay);
-                goto err_planes;
+                return PTR_ERR(overlay);
              }
          }
      }
@@ -110,17 +110,16 @@ int vkms_output_init_default(struct vkms_device *vkmsdev)
      if (vkmsdev->config.cursor) {
          cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR);
          if (IS_ERR(cursor)) {
-            ret = PTR_ERR(cursor);
-            goto err_planes;
+            return PTR_ERR(cursor);
          }
      }
      vkms_crtc = vkms_crtc_init(vkmsdev, &primary->base,
-                   cursor ? &cursor->base : NULL);
+                   cursor ? &cursor->base : NULL,
+                   "crtc-default");
      if (IS_ERR(vkms_crtc)) {
          DRM_ERROR("Failed to init crtc\n");
-        ret = PTR_ERR(vkms_crtc);
-        goto err_planes;
+        return PTR_ERR(vkms_crtc);
      }
      for (int i = 0; i < vkmsdev->output.num_planes; i++) {
@@ -131,22 +130,20 @@ int vkms_output_init_default(struct vkms_device *vkmsdev)
      connector = vkms_connector_init(vkmsdev);
      if (IS_ERR(connector)) {
          DRM_ERROR("Failed to init connector\n");
-        ret = PTR_ERR(connector);
-        goto err_connector;
+        return PTR_ERR(connector);
      }
      encoder = vkms_encoder_init(vkmsdev);
      if (IS_ERR(encoder)) {
          DRM_ERROR("Failed to init encoder\n");
-        ret = PTR_ERR(encoder);
-        goto err_encoder;
+        return PTR_ERR(encoder);
      }
      encoder->possible_crtcs |= drm_crtc_mask(&vkms_crtc->base);
      ret = drm_connector_attach_encoder(connector, encoder);
      if (ret) {
          DRM_ERROR("Failed to attach connector to encoder\n");
-        goto err_attach;
+        return ret;
      }
      if (vkmsdev->config.writeback) {
@@ -158,27 +155,175 @@ int vkms_output_init_default(struct vkms_device *vkmsdev)
      drm_mode_config_reset(dev);
      return 0;
+}
-err_attach:
-    drm_encoder_cleanup(encoder);
+static bool is_object_linked(struct vkms_config_links *links, unsigned long idx)
+{
+    return links->linked_object_bitmap & (1 << idx);
+}
-err_encoder:
-    drm_connector_cleanup(connector);
+int vkms_output_init(struct vkms_device *vkmsdev)
+{
+    struct drm_device *dev = &vkmsdev->drm;
+    struct vkms_configfs *configfs = vkmsdev->configfs;
+    struct vkms_output *output = &vkmsdev->output;
+    struct plane_map {
+        struct vkms_config_plane *config_plane;
+        struct vkms_plane *plane;
+    } plane_map[VKMS_MAX_PLANES] = { 0 };
+    struct encoder_map {
+        struct vkms_config_encoder *config_encoder;
+        struct drm_encoder *encoder;
+    } encoder_map[VKMS_MAX_OUTPUT_OBJECTS] = { 0 };
+    struct config_item *item;
+    int map_idx = 0;
+
+    list_for_each_entry(item, &configfs->planes_group.cg_children,
+                ci_entry) {
+        struct vkms_config_plane *config_plane =
+            item_to_config_plane(item);
+        struct vkms_plane *plane =
+            vkms_plane_init(vkmsdev, config_plane->type);
+
+        if (IS_ERR(plane)) {
+            DRM_ERROR("Unable to init plane from config: %s",
+                  item->ci_name);
+            return PTR_ERR(plane);
+        }
-err_connector:
-    drm_crtc_cleanup(&vkms_crtc->base);
+        plane_map[map_idx].config_plane = config_plane;
+        plane_map[map_idx].plane = plane;
+        map_idx += 1;
+    }
-err_planes:
-    for (int i = 0; i < output->num_planes; i++) {
-        drm_plane_cleanup(&output->planes[i].base);
+    map_idx = 0;
+    list_for_each_entry(item, &configfs->encoders_group.cg_children,
+                ci_entry) {
+        struct vkms_config_encoder *config_encoder =
+            item_to_config_encoder(item);
+        struct drm_encoder *encoder = vkms_encoder_init(vkmsdev);
+
+        if (IS_ERR(encoder)) {
+            DRM_ERROR("Failed to init config encoder: %s",
+                  item->ci_name);
+            return PTR_ERR(encoder);
+        }
+        encoder_map[map_idx].config_encoder = config_encoder;
A two connector configuration without an encoder would have the
potential of blowing up if we don't create a second_encoder.

What a catch!!! I tested this and sure enough BOOM!

Switched to limiting based on output->num_encoders as it should be.

+        encoder_map[map_idx].encoder = encoder;
+        map_idx += 1;
      }
-    memset(output, 0, sizeof(*output));
+    list_for_each_entry(item, &configfs->connectors_group.cg_children,
+                ci_entry) {
+        struct vkms_config_connector *config_connector =
+            item_to_config_connector(item);
+        struct drm_connector *connector = vkms_connector_init(vkmsdev);
-    return ret;
-}
+        if (IS_ERR(connector)) {
+            DRM_ERROR("Failed to init connector from config: %s",
+                  item->ci_name);
+            return PTR_ERR(connector);
+        }
-int vkms_output_init(struct vkms_device *vkmsdev)
-{
-    return -ENOTSUPP;
+        for (int j = 0; j < output->num_connectors; j++) {
+            struct encoder_map *encoder = &encoder_map[j];
+
+            if (is_object_linked(
+                    &config_connector->possible_encoders,
+                    encoder->config_encoder
+                        ->encoder_config_idx)) {
Here encoder->config_encoder for two connectors but a single encoder
will give back a empty encoder.
+                drm_connector_attach_encoder(connector,
+                                 encoder->encoder);
+            }
+        }
+    }
+
+    list_for_each_entry(item, &configfs->crtcs_group.cg_children,
+                ci_entry) {
+        struct vkms_config_crtc *config_crtc =
+            item_to_config_crtc(item);
+        struct vkms_crtc *vkms_crtc;
+        struct drm_plane *primary = NULL, *cursor = NULL;
+
+        for (int j = 0; j < output->num_planes; j++) {
+            struct plane_map *plane_entry = &plane_map[j];
+            struct drm_plane *plane = &plane_entry->plane->base;
+
+            if (!is_object_linked(
+                    &plane_entry->config_plane->possible_crtcs,
+                    config_crtc->crtc_config_idx)) {
+                continue;
+            }
+
+            if (plane->type == DRM_PLANE_TYPE_PRIMARY) {
+                if (primary) {
+                    DRM_WARN(
+                        "Too many primary planes found for crtc %s.",
+                        item->ci_name);
+                    return EINVAL;
+                }
+                primary = plane;
+            } else if (plane->type == DRM_PLANE_TYPE_CURSOR) {
+                if (cursor) {
+                    DRM_WARN(
+                        "Too many cursor planes found for crtc %s.",
+                        item->ci_name);
+                    return EINVAL;
+                }
+                cursor = plane;
+            }
+        }
+
+        if (!primary) {
+            DRM_WARN("No primary plane configured for crtc %s",
+                 item->ci_name);
+            return EINVAL;
+        }
+
+        vkms_crtc =
+            vkms_crtc_init(vkmsdev, primary, cursor, item->ci_name);
+        if (IS_ERR(vkms_crtc)) {
+            DRM_WARN("Unable to init crtc from config: %s",
+                 item->ci_name);
+            return PTR_ERR(vkms_crtc);
+        }
+
+        for (int j = 0; j < output->num_planes; j++) {
+            struct plane_map *plane_entry = &plane_map[j];
+
+            if (!plane_entry->plane)
+                break;
+
+            if (is_object_linked(
+                    &plane_entry->config_plane->possible_crtcs,
+                    config_crtc->crtc_config_idx)) {
+                plane_entry->plane->base.possible_crtcs |=
+                    drm_crtc_mask(&vkms_crtc->base);
+            }
+        }
+
+        for (int j = 0; j < output->num_encoders; j++) {
+            struct encoder_map *encoder_entry = &encoder_map[j];
+
+            if (is_object_linked(&encoder_entry->config_encoder
+                              ->possible_crtcs,
+                         config_crtc->crtc_config_idx)) {
+                encoder_entry->encoder->possible_crtcs |=
+                    drm_crtc_mask(&vkms_crtc->base);
+            }
+        }
+
+        if (vkmsdev->config.writeback) {
+            int ret = vkms_enable_writeback_connector(vkmsdev,
+                                  vkms_crtc);
+            if (ret)
+                DRM_WARN(
+                    "Failed to init writeback connector for config crtc: %s. Error code %d",
+                    item->ci_name, ret);
+        }
+    }
I think we might be needing here a test for missing symlinks - invalid
configurations, like unused crtcs, encoders, connectors. I
suppose anything that's not matched with is_object_linked(),
such we avoid invalid configuration found by drm_mode_config_validate()
and inform users about missing them.

An example here would be to create a second encoder, connector, crtc and
not symlink them to their possible_encoders,possible_crtcs mask. An
i-g-t test for this very thing would be needed to stress different kind
of combinations.

I wonder about this.  Shouldn't a user be free to create dangling files to simulate some scenario?

Problem is we end up with invalid pipelines which would be invalided by `drm_mode_config_validate()`, further more leading to kernel warns/errors. That seems to be case now. If the system is still usable -- while still having these missing bits, and possibly inform the user about an invalid pipeline would be ideal.


I could see a further development to publish a warning in the log, but disallowing it seems overly restrictive.

Sure, I think ideally this should be the case, if we can do that in the first place, given that we operate on a transaction model, only when enable it we go over all items that were created/added by the user. Dangling or not, my impression is that these invalid bits need to be dropped entirely when submitted back to the DRM helpers.

If the user configures 3 pipelines, two correctly, and one not, still allow those two pipelines to work and inform that it might have an invalid one, rather than invalidating the entire config, I think that's the point.

Either way we could accomplish this pretty easily by adding a flag to each vkms object in the form of `bool is_linked` and set it when we detect it's linked.  Then at the end iterate through all planes, encoders, connectors, crtcs that can be linked and if they are not just publish a warning saying

kwarn: "crtc/plane/encoder $NAME is created but unlinked"
Yeah, that would be really good.

IGT tests to test that that was done make sense to me.

+
+    drm_mode_config_reset(dev);
+
+    return 0;
  }
--
2.41.0.162.gfafddb0af9-goog