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

[Marius Vlad <marius.vlad@xxxxxxxxxxxxx>]

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