Re: [PATCH 06/10] drm/client: Add a way to set modeset, properties and rotation
Den 03.05.2020 10.47, skrev Sam Ravnborg:
> Hi Noralf.
>
> Some comments in the following - partly because I still do not fully
> grasp modeset etc.
I still don't fully understand it either. I have wandered inside the
atomic machinery countless times to track what happens to this or that
state/property and 2 weeks later the insight is gone. It just doesn't
stick. What makes it difficult for me I believe is that I have never
looked much at the userspace side, how this is all used.
>
> 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
>> ---
>> 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().
This is legacy behaviour that was moved over from drm_fb_helper. It
cleared the modeset before probing for a new useable modeset. So the
same applies when the client sets the modeset itself, we need to clear
out the previous modeset.
If drm_client was written from scratch, the state/modeset would not be
stored in drm_client_dev, but stored by the client.
With the current situation a client can't check a configuration while
retaining a working config/state. It looses the working state setting up
one for testing.
This is not a problem for my usecase, but it can be for a future
usecase. Since it works for me I decided to keep it as-is instead of
trying to fix this without knowing what the future usecase might be.
>
>> }
>>
>> 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?
Not needed here because drm_client_modeset_commit() supports
legacy/non-atomic.
> Or maybe I just do not get all this atomic stuff yet..
>
>> +mutex_lock(>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?
Ah, I haven't documented this. This actually clears the modesets. If
this is commited, it results in all outputs being turned off.
Maybe I should do a drm_client_modeset_clear() instead.
>
>> +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;
>>
Re: [PATCH 06/10] drm/client: Add a way to set modeset, properties and rotation
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
> ---
> 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(>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(>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(>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
[PATCH 06/10] drm/client: Add a way to set modeset, properties and rotation
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
---
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;
}
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;
+
+ mutex_lock(>modeset_mutex);
+
+ drm_client_modeset_release(client);
+
+ if (!connector || !mode || !fb) {
+ ret = 0;
+ 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(>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.
+ *
+ * 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;
+
+ mutex_lock(>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;
+ properties[client->num_properties].value = value;
+ client->properties = properties;
+ client->num_properties++;
+unlock:
+ mutex_unlock(>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(>modeset_mutex);
+ drm_client_for_each_modeset(modeset, client) {
+ if (modeset->mode) {
+ plane = modeset->crtc->primary;
+ break;
+ }
+ }
+ mutex_unlock(>modeset_mutex);
+
+ if (!plane)
+ return -ENOENT;
+
+ return drm_client_modeset_set_property(client, >base,
+
