[PATCH v2 5/6] drm/vkms: Support enabling ConfigFS devices
Marius Vlad
marius.vlad at collabora.com
Fri Aug 18 06:54:00 UTC 2023
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 at 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?
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
>>>
More information about the dri-devel
mailing list