Message-ID: <ZjCstbN0IDG0jBjI@phenom.ffwll.local>
Date: Tue, 30 Apr 2024 10:32:53 +0200
From: Daniel Vetter <daniel@...ll.ch>
To: Brandon Pollack <brpol@...omium.org>
Cc: marius.vlad@...labora.com, mairacanal@...eup.net, jshargo@...omium.org,
	hamohammed.sa@...il.com, rodrigosiqueiramelo@...il.com,
	linux-doc@...r.kernel.org, hirono@...omium.org, corbet@....net,
	linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
	melissa.srw@...il.com, mduggan@...omium.org, mripard@...nel.org,
	tzimmermann@...e.de
Subject: Re: [PATCH v6 5/7] drm/vkms: Support enabling ConfigFS devices

On Tue, Aug 29, 2023 at 05:30:57AM +0000, Brandon Pollack wrote:
> From: Jim Shargo <jshargo@...omium.org>
> 
> 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.

I forgot one comment ... I tried to look around really hard, but I'm not
seeing any code which prevents changes to immutable state once the device
is created?

Note that if we go with the design where you can only change enabled from
0 to 1 once it really doesn't matter much whether we block changes
afterwards, since they do not have any impact at all. But it would still
be nice, since for connector hotplug support we must make sure that the
encoders and all that stay around as-is.

Otoh there's no way to prevent dropping a config_item, so maybe we just
have to life with the fact that configfs is a lot more mutable than the
underlying drm objects. In that case I'd much prefer we switch over to the
model where you can only enable a configfs device instance once, otherwise
it's really confusing.
-Sima

> 
> Signed-off-by: Jim Shargo <jshargo@...omium.org>
> Signed-off-by: Brandon Pollack <brpol@...omium.org>
> ---
>  drivers/gpu/drm/vkms/vkms_configfs.c |  37 ++--
>  drivers/gpu/drm/vkms/vkms_crtc.c     |   4 +-
>  drivers/gpu/drm/vkms/vkms_drv.c      |   1 +
>  drivers/gpu/drm/vkms/vkms_drv.h      |   4 +-
>  drivers/gpu/drm/vkms/vkms_output.c   | 282 +++++++++++++++++++++++----
>  drivers/gpu/drm/vkms/vkms_plane.c    |  10 +-
>  6 files changed, 282 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
> index dae2e85d83a1..bc35dcc47585 100644
> --- a/drivers/gpu/drm/vkms/vkms_configfs.c
> +++ b/drivers/gpu/drm/vkms/vkms_configfs.c
> @@ -508,29 +508,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 74bbd675464b..2aa1c5246b7e 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -279,7 +279,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;
> @@ -291,7 +291,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 819e880a8cf7..6e7f20681890 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.c
> +++ b/drivers/gpu/drm/vkms/vkms_drv.c
> @@ -275,6 +275,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 8cdd7949f661..2b9545ada9c2 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -250,13 +250,13 @@ 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);
>  
>  struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> -				   enum drm_plane_type type);
> +				   enum drm_plane_type type, char* name, ...);
>  
>  /* CRC Support */
>  const char *const *vkms_get_crc_sources(struct drm_crtc *crtc,
> diff --git a/drivers/gpu/drm/vkms/vkms_output.c b/drivers/gpu/drm/vkms/vkms_output.c
> index dc69959c5e1d..0ee1f3f4a305 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>
>  
> @@ -60,7 +62,8 @@ vkms_connector_init(struct vkms_device *vkms_device)
>  	return connector;
>  }
>  
> -static struct drm_encoder *vkms_encoder_init(struct vkms_device *vkms_device)
> +static struct drm_encoder *vkms_encoder_init(struct vkms_device *vkms_device,
> +					     char *name)
>  {
>  	struct drm_encoder *encoder;
>  	int ret;
> @@ -71,7 +74,7 @@ static struct drm_encoder *vkms_encoder_init(struct vkms_device *vkms_device)
>  	encoder = &vkms_device->output
>  			   .encoders[vkms_device->output.num_encoders++];
>  	ret = drm_encoder_init(&vkms_device->drm, encoder, &vkms_encoder_funcs,
> -			       DRM_MODE_ENCODER_VIRTUAL, NULL);
> +			       DRM_MODE_ENCODER_VIRTUAL, name);
>  	if (ret) {
>  		memset(encoder, 0, sizeof(*encoder));
>  		vkms_device->output.num_encoders -= 1;
> @@ -82,7 +85,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;
> @@ -92,35 +94,34 @@ int vkms_output_init_default(struct vkms_device *vkmsdev)
>  	int writeback;
>  	unsigned int n;
>  
> -	primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY);
> +	primary = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_PRIMARY,
> +				  "default-primary-plane");
>  	if (IS_ERR(primary))
>  		return PTR_ERR(primary);
>  
>  	if (vkmsdev->config.overlay) {
>  		for (n = 0; n < NUM_OVERLAY_PLANES; n++) {
> -			struct vkms_plane *overlay = vkms_plane_init(
> -				vkmsdev, DRM_PLANE_TYPE_OVERLAY);
> -			if (IS_ERR(overlay)) {
> -				ret = PTR_ERR(overlay);
> -				goto err_planes;
> -			}
> +			struct vkms_plane *overlay =
> +				vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_OVERLAY,
> +						"default-overlay-plane-%d", n);
> +			if (IS_ERR(overlay))
> +				return PTR_ERR(overlay);
>  		}
>  	}
>  
>  	if (vkmsdev->config.cursor) {
> -		cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR);
> -		if (IS_ERR(cursor)) {
> -			ret = PTR_ERR(cursor);
> -			goto err_planes;
> -		}
> +		cursor = vkms_plane_init(vkmsdev, DRM_PLANE_TYPE_CURSOR,
> +					 "default-cursor-plane");
> +		if (IS_ERR(cursor))
> +			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 +132,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);
> +	encoder = vkms_encoder_init(vkmsdev, "encoder-default");
>  	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,26 +157,235 @@ int vkms_output_init_default(struct vkms_device *vkmsdev)
>  	drm_mode_config_reset(dev);
>  
>  	return 0;
> +}
>  
> -err_attach:
> -	drm_encoder_cleanup(encoder);
> -
> -err_encoder:
> -	drm_connector_cleanup(connector);
> +static bool is_object_linked(struct vkms_config_links *links, unsigned long idx)
> +{
> +	return links->linked_object_bitmap & (1 << idx);
> +}
>  
> -err_connector:
> -	drm_crtc_cleanup(&vkms_crtc->base);
> +/**
> +* validate_vkms_configfs_no_dangling_objects - warn on unused objects in vkms
> +* configfs.
> +* @vkmsdev: vkms device
> +*
> +* This gives slightly more visible warning messaging to the user before the drm
> +* system finds the configuration invalid and prints it's debug information.  In
> +* this case the user may have accidentally not included some links, or the user
> +* could be testing this faulty configuration.
> +*/
> +static void
> +validate_vkms_configfs_no_dangling_objects(struct vkms_device *vkmsdev)
> +{
> +	struct vkms_configfs *configfs = vkmsdev->configfs;
> +	struct config_item *item;
> +
> +	// 1. Planes
> +	list_for_each_entry(item, &configfs->planes_group.cg_children,
> +			    ci_entry) {
> +		struct vkms_config_plane *config_plane =
> +			item_to_config_plane(item);
> +		if (config_plane->possible_crtcs.linked_object_bitmap == 0)
> +			DRM_WARN(
> +				"Vkms configfs created plane %s has no linked crtcs",
> +				item->ci_name);
> +	}
>  
> -err_planes:
> -	for (int i = 0; i < output->num_planes; i++)
> -		drm_plane_cleanup(&output->planes[i].base);
> +	// 2. connectors
> +	list_for_each_entry(item, &configfs->connectors_group.cg_children,
> +			    ci_entry) {
> +		struct vkms_config_connector *config_connector =
> +			item_to_config_connector(item);
> +		if (config_connector->possible_encoders.linked_object_bitmap ==
> +		    0) {
> +			DRM_WARN(
> +				"Vkms configfs created connector %s has no linked encoders",
> +				item->ci_name);
> +		}
> +	}
>  
> -	memset(output, 0, sizeof(*output));
> +	// 3. encoders
> +	list_for_each_entry(item, &configfs->encoders_group.cg_children,
> +			    ci_entry) {
> +		struct vkms_config_encoder *config_encoder =
> +			item_to_config_encoder(item);
> +		if (config_encoder->possible_crtcs.linked_object_bitmap == 0) {
> +			DRM_WARN(
> +				"Vkms configfs created encoder %s has no linked crtcs",
> +				item->ci_name);
> +		}
> +	}
>  
> -	return ret;
> +	// 4. crtcs only require a primary plane to function, this is checked during
> +	// output initialization and returns an error.
>  }
>  
>  int vkms_output_init(struct vkms_device *vkmsdev)
>  {
> -	return -EOPNOTSUPP;
> +	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;
> +
> +	// Ensure configfs has no unused objects, and warn if so.
> +	validate_vkms_configfs_no_dangling_objects(vkmsdev);
> +
> +	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, item->ci_name);
> +
> +		if (IS_ERR(plane)) {
> +			DRM_ERROR("Unable to init plane from config: %s",
> +				  item->ci_name);
> +			return PTR_ERR(plane);
> +		}
> +
> +		plane_map[map_idx].config_plane = config_plane;
> +		plane_map[map_idx].plane = plane;
> +		map_idx += 1;
> +	}
> +
> +	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, item->ci_name);
> +
> +		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;
> +		encoder_map[map_idx].encoder = encoder;
> +		map_idx += 1;
> +	}
> +
> +	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);
> +
> +		if (IS_ERR(connector)) {
> +			DRM_ERROR("Failed to init connector from config: %s",
> +				  item->ci_name);
> +			return PTR_ERR(connector);
> +		}
> +
> +		for (int j = 0; j < output->num_encoders; j++) {
> +			struct encoder_map *encoder = &encoder_map[j];
> +
> +			if (is_object_linked(
> +				    &config_connector->possible_encoders,
> +				    encoder->config_encoder
> +					    ->encoder_config_idx)) {
> +				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);
> +		}
> +	}
> +
> +	drm_mode_config_reset(dev);
> +
> +	return 0;
>  }
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index 950e6c930273..3198bf0dca73 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0+
>  
>  #include <linux/iosys-map.h>
> +#include <linux/stdarg.h>
>  
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
> @@ -215,20 +216,25 @@ static const struct drm_plane_helper_funcs vkms_plane_helper_funcs = {
>  };
>  
>  struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
> -				   enum drm_plane_type type)
> +				   enum drm_plane_type type, char *name, ...)
>  {
>  	struct drm_device *dev = &vkmsdev->drm;
>  	struct vkms_output *output = &vkmsdev->output;
>  	struct vkms_plane *plane;
> +	va_list va;
>  	int ret;
>  
>  	if (output->num_planes >= VKMS_MAX_PLANES)
>  		return ERR_PTR(-ENOMEM);
>  
>  	plane = &output->planes[output->num_planes++];
> +
> +	va_start(va, name);
>  	ret = drm_universal_plane_init(dev, &plane->base, 0, &vkms_plane_funcs,
>  				       vkms_formats, ARRAY_SIZE(vkms_formats),
> -				       NULL, type, NULL);
> +				       NULL, type, name, va);
> +	va_end(va);
> +
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> -- 
> 2.42.0.rc2.253.gd59a3bf2b4-goog
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists