Re: [PATCH 06/10] drm/client: Add a way to set modeset, properties and rotation

[Sam Ravnborg <sam@ravnborg.org>]

Hi Noralf.

Some comments in the following - partly because I still do not fully
grasp modeset etc.

	Sam

On Wed, Apr 29, 2020 at 02:48:26PM +0200, Noralf Trønnes wrote:
> This adds functions for clients that need more control over the
> configuration than what's setup by drm_client_modeset_probe().
> Connector, fb and display mode can be set using drm_client_modeset_set().
> Plane rotation can be set using drm_client_modeset_set_rotation() and
> other properties using drm_client_modeset_set_property(). Property
> setting is only implemented for atomic drivers.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  drivers/gpu/drm/drm_client_modeset.c | 139 +++++++++++++++++++++++++++
>  include/drm/drm_client.h             |  38 +++++++-
>  2 files changed, 176 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_client_modeset.c b/drivers/gpu/drm/drm_client_modeset.c
> index 177158ff2a40..1eef6869cae1 100644
> --- a/drivers/gpu/drm/drm_client_modeset.c
> +++ b/drivers/gpu/drm/drm_client_modeset.c
> @@ -83,6 +83,10 @@ static void drm_client_modeset_release(struct drm_client_dev *client)
>  		}
>  		modeset->num_connectors = 0;
>  	}
> +
> +	kfree(client->properties);
> +	client->properties = NULL;
> +	client->num_properties = 0;

I failed to see why this code is in drm_client_modeset_release()
and not in drm_client_modeset_free().
In other words - why do we need to free properties in drm_client_modeset_probe()
which is the only other user of drm_client_modeset_release().

>  }
>  
>  void drm_client_modeset_free(struct drm_client_dev *client)
> @@ -879,6 +883,132 @@ int drm_client_modeset_probe(struct drm_client_dev *client, unsigned int width,
>  }
>  EXPORT_SYMBOL(drm_client_modeset_probe);
>  
> +/**
> + * drm_client_modeset_set() - Set modeset configuration
> + * @client: DRM client
> + * @connector: Connector
> + * @mode: Display mode
> + * @fb: Framebuffer
> + *
> + * This function releases any current modeset info, including properties, and
> + * sets the new modeset in the client's modeset array.
> + *
> + * Returns:
> + * Zero on success or negative error code on failure.
> + */
> +int drm_client_modeset_set(struct drm_client_dev *client, struct drm_connector *connector,
> +			   struct drm_display_mode *mode, struct drm_framebuffer *fb)
> +{
> +	struct drm_mode_set *modeset;
> +	int ret = -ENOENT;
> +
Need to check if atomic is supported?
Or maybe I just do not get all this atomic stuff yet..

> +	mutex_lock(&client->modeset_mutex);
> +
> +	drm_client_modeset_release(client);
If the check below fails - is it then correct to release modeset?
> +
> +	if (!connector || !mode || !fb) {
> +		ret = 0;
Error out, it is not a success if we cannot do anything?

> +		goto unlock;
> +	}
> +
> +	drm_client_for_each_modeset(modeset, client) {
> +		if (!connector_has_possible_crtc(connector, modeset->crtc))
> +			continue;
> +
> +		modeset->mode = drm_mode_duplicate(client->dev, mode);
> +		if (!modeset->mode) {
> +			ret = -ENOMEM;
> +			break;
> +		}
> +
> +		drm_mode_set_crtcinfo(modeset->mode, CRTC_INTERLACE_HALVE_V);
> +
> +		drm_connector_get(connector);
> +		modeset->connectors[modeset->num_connectors++] = connector;
> +
> +		modeset->fb = fb;
> +		ret = 0;
> +		break;
> +	}
> +unlock:
> +	mutex_unlock(&client->modeset_mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_client_modeset_set);
> +
> +/**
> + * drm_client_modeset_set_property() - Set a property on the current configuration
> + * @client: DRM client
> + * @obj: DRM Mode Object
> + * @prop: DRM Property
> + * @value: Property value
> + *
> + * Note: Currently only implemented for atomic drivers.
Are there any reason to in the future support legacy (non-atomic)
drivers.
If not then reword - as the above reads like it is on a TODO list to
support legacy drivers.

> + *
> + * Returns:
> + * Zero on success or negative error code on failure.
> + */
> +int drm_client_modeset_set_property(struct drm_client_dev *client, struct drm_mode_object *obj,
> +				    struct drm_property *prop, u64 value)
> +{
> +	struct drm_client_property *properties;
> +	int ret = 0;
> +
> +	if (!prop)
> +		return -EINVAL;
> +
Need to check if atomic is supported?
Or maybe I just do not get all this atomic stuff yet..

> +	mutex_lock(&client->modeset_mutex);
> +
> +	properties = krealloc(client->properties,
> +			      (client->num_properties + 1) * sizeof(*properties), GFP_KERNEL);
> +	if (!properties) {
> +		ret = -ENOMEM;
> +		goto unlock;
> +	}
> +
> +	properties[client->num_properties].obj = obj;
> +	properties[client->num_properties].prop = prop;
The drm_client_modeset_set_property() take over ownership of prop.
This looks wrong - should this be a copy of prop?
properties[].prop should not be a pointer but a full drm_property
struct?

> +	properties[client->num_properties].value = value;
> +	client->properties = properties;
> +	client->num_properties++;
> +unlock:
> +	mutex_unlock(&client->modeset_mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_client_modeset_set_property);
> +
> +/**
> + * drm_client_modeset_set_rotation() - Set rotation on the current configuration
> + * @client: DRM client
> + * @value: Rotation value
> + *
> + * Returns:
> + * Zero on success or negative error code on failure.
> + */
> +int drm_client_modeset_set_rotation(struct drm_client_dev *client, u64 value)
> +{
> +	struct drm_plane *plane = NULL;
> +	struct drm_mode_set *modeset;
> +
> +	mutex_lock(&client->modeset_mutex);
> +	drm_client_for_each_modeset(modeset, client) {
> +		if (modeset->mode) {
> +			plane = modeset->crtc->primary;
> +			break;
> +		}
> +	}
> +	mutex_unlock(&client->modeset_mutex);
> +
> +	if (!plane)
> +		return -ENOENT;
> +
> +	return drm_client_modeset_set_property(client, &plane->base,
> +					       plane->rotation_property, value);
> +}
> +EXPORT_SYMBOL(drm_client_modeset_set_rotation);
> +
>  /**
>   * drm_client_rotation() - Check the initial rotation value
>   * @modeset: DRM modeset
> @@ -973,6 +1103,7 @@ static int drm_client_modeset_commit_atomic(struct drm_client_dev *client, bool
>  	struct drm_atomic_state *state;
>  	struct drm_modeset_acquire_ctx ctx;
>  	struct drm_mode_set *mode_set;
> +	unsigned int i;
>  	int ret;
>  
>  	drm_modeset_acquire_init(&ctx, 0);
> @@ -1033,6 +1164,14 @@ static int drm_client_modeset_commit_atomic(struct drm_client_dev *client, bool
>  		}
>  	}
>  
> +	for (i = 0; i < client->num_properties; i++) {
> +		struct drm_client_property *prop = &client->properties[i];
> +
> +		ret = drm_atomic_set_property(state, NULL, prop->obj, prop->prop, prop->value);
> +		if (ret)
> +			goto out_state;
> +	}
> +
With the code above drm_atomic_set_property() is called also when check
is true. I had expected that check would not change anything.

>  	if (check)
>  		ret = drm_atomic_check_only(state);
>  	else
> diff --git a/include/drm/drm_client.h b/include/drm/drm_client.h
> index b6ffa4863e45..4b266741ec0e 100644
> --- a/include/drm/drm_client.h
> +++ b/include/drm/drm_client.h
> @@ -16,6 +16,7 @@ struct drm_file;
>  struct drm_framebuffer;
>  struct drm_gem_object;
>  struct drm_minor;
> +struct drm_property;
>  struct module;
>  
>  /**
> @@ -64,6 +65,26 @@ struct drm_client_funcs {
>  	int (*hotplug)(struct drm_client_dev *client);
>  };
>  
> +/**
> + * struct drm_client_property - DRM client property
> + */
> +struct drm_client_property {
> +	/**
> +	 * @obj: DRM Mode Object to which the property belongs.
> +	 */
> +	struct drm_mode_object *obj;
> +
> +	/**
> +	 * @prop: DRM Property.
> +	 */
> +	struct drm_property *prop;
> +
> +	/**
> +	 * @value: Property value.
> +	 */
> +	u64 value;
> +};
> +
>  /**
>   * struct drm_client_dev - DRM client instance
>   */
> @@ -97,7 +118,7 @@ struct drm_client_dev {
>  	struct drm_file *file;
>  
>  	/**
> -	 * @modeset_mutex: Protects @modesets.
> +	 * @modeset_mutex: Protects @modesets and @properties.
>  	 */
>  	struct mutex modeset_mutex;
>  
> @@ -105,6 +126,16 @@ struct drm_client_dev {
>  	 * @modesets: CRTC configurations
>  	 */
>  	struct drm_mode_set *modesets;
> +
> +	/**
> +	 * @properties: DRM properties attached to the configuration.
> +	 */
> +	struct drm_client_property *properties;
> +
> +	/**
> +	 * @num_properties: Number of attached properties.
> +	 */
> +	unsigned int num_properties;
>  };
>  
>  int drm_client_init(struct drm_device *dev, struct drm_client_dev *client,
> @@ -163,6 +194,11 @@ void drm_client_buffer_vunmap(struct drm_client_buffer *buffer);
>  int drm_client_modeset_create(struct drm_client_dev *client);
>  void drm_client_modeset_free(struct drm_client_dev *client);
>  int drm_client_modeset_probe(struct drm_client_dev *client, unsigned int width, unsigned int height);
> +int drm_client_modeset_set(struct drm_client_dev *client, struct drm_connector *connector,
> +			   struct drm_display_mode *mode, struct drm_framebuffer *fb);
> +int drm_client_modeset_set_property(struct drm_client_dev *client, struct drm_mode_object *obj,
> +				    struct drm_property *prop, u64 value);
> +int drm_client_modeset_set_rotation(struct drm_client_dev *client, u64 value);
>  bool drm_client_rotation(struct drm_mode_set *modeset, unsigned int *rotation);
>  int drm_client_modeset_check(struct drm_client_dev *client);
>  int drm_client_modeset_commit_locked(struct drm_client_dev *client);
> -- 
> 2.23.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel