Messages in this thread | | | Date | Fri, 18 Aug 2023 14:24:12 +0900 | Subject | Re: [PATCH v2 5/6] drm/vkms: Support enabling ConfigFS devices | From | Brandon Ross Pollack <> |
| |
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@chromium.org> >> --- >> 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?
I could see a further development to publish a warning in the log, but disallowing it seems overly restrictive.
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"
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 >>
| |