| Message ID | 20221117075433.222968-1-contact@emersion.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/atomic: add quirks for blind save/restore | expand |
On Thu, 17 Nov 2022 07:54:40 +0000 Simon Ser <contact@emersion.fr> wrote: > Two quirks to make blind atomic save/restore [1] work correctly: > > - Mark the DPMS property as immutable for atomic clients, since > atomic clients cannot change it. > - Allow user-space to set content protection to "enabled", interpret > it as "desired". > > [1]: https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/3794 > > Signed-off-by: Simon Ser <contact@emersion.fr> That seems to be the sensible thing to do to these properties. Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com> I'm really to happy see wlroots doing this. This has been in my mind to do in Weston for years, but never had the chance to implement it yet. I have just one little detail different: I would snapshot the "original" KMS state at Weston start-up only once, and then use that snapshot as the base every time Weston needs to sanitize the KMS state, like when re-gaining DRM master. It's a bit more complicated, but it goes better with the idea of resetting KMS state or starting from clean state, in case KMS ever gains UAPI for that. Thanks, pq > --- > > I don't have the motivation to write IGT tests for this. > > drivers/gpu/drm/drm_atomic_uapi.c | 5 +++-- > drivers/gpu/drm/drm_property.c | 7 +++++++ > 2 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c > index c06d0639d552..95363aac7f69 100644 > --- a/drivers/gpu/drm/drm_atomic_uapi.c > +++ b/drivers/gpu/drm/drm_atomic_uapi.c > @@ -741,8 +741,9 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, > state->scaling_mode = val; > } else if (property == config->content_protection_property) { > if (val == DRM_MODE_CONTENT_PROTECTION_ENABLED) { > - drm_dbg_kms(dev, "only drivers can set CP Enabled\n"); > - return -EINVAL; > + /* Degrade ENABLED to DESIRED so that blind atomic > + * save/restore works as intended. */ > + val = DRM_MODE_CONTENT_PROTECTION_DESIRED; > } > state->content_protection = val; > } else if (property == config->hdcp_content_type_property) { > diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c > index dfec479830e4..dde42986f8cb 100644 > --- a/drivers/gpu/drm/drm_property.c > +++ b/drivers/gpu/drm/drm_property.c > @@ -474,7 +474,14 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, > return -ENOENT; > > strscpy_pad(out_resp->name, property->name, DRM_PROP_NAME_LEN); > + > out_resp->flags = property->flags; > + if (file_priv->atomic && property == dev->mode_config.dpms_property) { > + /* Quirk: indicate that the legacy DPMS property is not > + * writable from atomic user-space, so that blind atomic > + * save/restore works as intended. */ > + out_resp->flags |= DRM_MODE_PROP_IMMUTABLE; > + } > > value_count = property->num_values; > values_ptr = u64_to_user_ptr(out_resp->values_ptr);
On Thu, Nov 17, 2022 at 07:54:40AM +0000, Simon Ser wrote: > Two quirks to make blind atomic save/restore [1] work correctly: > > - Mark the DPMS property as immutable for atomic clients, since > atomic clients cannot change it. > - Allow user-space to set content protection to "enabled", interpret > it as "desired". > > [1]: https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/3794 > > Signed-off-by: Simon Ser <contact@emersion.fr> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> I think a doc patch which documents the guarantees we're trying to make here and that they're uapi would be really nice. Maybe somewhere in the KMS properties section in the docs. -Daniel > --- > > I don't have the motivation to write IGT tests for this. > > drivers/gpu/drm/drm_atomic_uapi.c | 5 +++-- > drivers/gpu/drm/drm_property.c | 7 +++++++ > 2 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c > index c06d0639d552..95363aac7f69 100644 > --- a/drivers/gpu/drm/drm_atomic_uapi.c > +++ b/drivers/gpu/drm/drm_atomic_uapi.c > @@ -741,8 +741,9 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, > state->scaling_mode = val; > } else if (property == config->content_protection_property) { > if (val == DRM_MODE_CONTENT_PROTECTION_ENABLED) { > - drm_dbg_kms(dev, "only drivers can set CP Enabled\n"); > - return -EINVAL; > + /* Degrade ENABLED to DESIRED so that blind atomic > + * save/restore works as intended. */ > + val = DRM_MODE_CONTENT_PROTECTION_DESIRED; > } > state->content_protection = val; > } else if (property == config->hdcp_content_type_property) { > diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c > index dfec479830e4..dde42986f8cb 100644 > --- a/drivers/gpu/drm/drm_property.c > +++ b/drivers/gpu/drm/drm_property.c > @@ -474,7 +474,14 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, > return -ENOENT; > > strscpy_pad(out_resp->name, property->name, DRM_PROP_NAME_LEN); > + > out_resp->flags = property->flags; > + if (file_priv->atomic && property == dev->mode_config.dpms_property) { > + /* Quirk: indicate that the legacy DPMS property is not > + * writable from atomic user-space, so that blind atomic > + * save/restore works as intended. */ > + out_resp->flags |= DRM_MODE_PROP_IMMUTABLE; > + } > > value_count = property->num_values; > values_ptr = u64_to_user_ptr(out_resp->values_ptr); > -- > 2.38.1 > >
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index c06d0639d552..95363aac7f69 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -741,8 +741,9 @@ static int drm_atomic_connector_set_property(struct drm_connector *connector, state->scaling_mode = val; } else if (property == config->content_protection_property) { if (val == DRM_MODE_CONTENT_PROTECTION_ENABLED) { - drm_dbg_kms(dev, "only drivers can set CP Enabled\n"); - return -EINVAL; + /* Degrade ENABLED to DESIRED so that blind atomic + * save/restore works as intended. */ + val = DRM_MODE_CONTENT_PROTECTION_DESIRED; } state->content_protection = val; } else if (property == config->hdcp_content_type_property) { diff --git a/drivers/gpu/drm/drm_property.c b/drivers/gpu/drm/drm_property.c index dfec479830e4..dde42986f8cb 100644 --- a/drivers/gpu/drm/drm_property.c +++ b/drivers/gpu/drm/drm_property.c @@ -474,7 +474,14 @@ int drm_mode_getproperty_ioctl(struct drm_device *dev, return -ENOENT; strscpy_pad(out_resp->name, property->name, DRM_PROP_NAME_LEN); + out_resp->flags = property->flags; + if (file_priv->atomic && property == dev->mode_config.dpms_property) { + /* Quirk: indicate that the legacy DPMS property is not + * writable from atomic user-space, so that blind atomic + * save/restore works as intended. */ + out_resp->flags |= DRM_MODE_PROP_IMMUTABLE; + } value_count = property->num_values; values_ptr = u64_to_user_ptr(out_resp->values_ptr);
Two quirks to make blind atomic save/restore [1] work correctly: - Mark the DPMS property as immutable for atomic clients, since atomic clients cannot change it. - Allow user-space to set content protection to "enabled", interpret it as "desired". [1]: https://gitlab.freedesktop.org/wlroots/wlroots/-/merge_requests/3794 Signed-off-by: Simon Ser <contact@emersion.fr> --- I don't have the motivation to write IGT tests for this. drivers/gpu/drm/drm_atomic_uapi.c | 5 +++-- drivers/gpu/drm/drm_property.c | 7 +++++++ 2 files changed, 10 insertions(+), 2 deletions(-)