Display power management should be available to user no matter whether display server implements it or not and should be made server agnostic (X.Org, Wayland compositor), without need of TTY change.
Reasons: 1. User implements an own universal power management daemon. He has no interest to deal with server or switch to a free TTY, he wants to turn off the monitor in a simple way, like he controls power of other system components. 2. Some applications under X.Org like Electron-based ones like to prevent screen goning off giving user no way to fix that. 3. Many fresh new Wayland compositors don't implement DPMS at all and at the same time have TTY switch broken.
Signed-off-by: Alex Ivanov gnidorah@ya.ru --- drivers/gpu/drm/drm_crtc_internal.h | 2 ++ drivers/gpu/drm/drm_ioctl.c | 1 + drivers/gpu/drm/drm_mode_object.c | 41 +++++++++++++++++++++++++++++++++++++ include/uapi/drm/drm.h | 1 + 4 files changed, 45 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h index a43582076b20..120dd40a5210 100644 --- a/drivers/gpu/drm/drm_crtc_internal.h +++ b/drivers/gpu/drm/drm_crtc_internal.h @@ -122,6 +122,8 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); +int drm_mode_connector_dpms_ioctl(struct drm_device *dev, + void *data, struct drm_file *file_priv);
/* drm_encoder.c */ int drm_encoder_register_all(struct drm_device *dev); diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index a9ae6dd2d593..2cec1a382c7b 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -632,6 +632,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = { DRM_IOCTL_DEF(DRM_IOCTL_MODE_DETACHMODE, drm_noop, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPROPERTY, drm_mode_getproperty_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_SETPROPERTY, drm_mode_connector_property_set_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), + DRM_IOCTL_DEF(DRM_IOCTL_MODE_DPMS, drm_mode_connector_dpms_ioctl, DRM_ROOT_ONLY|DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPROPBLOB, drm_mode_getblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETFB, drm_mode_getfb, DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_ADDFB, drm_mode_addfb, DRM_CONTROL_ALLOW|DRM_UNLOCKED), diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c index 1055533792f3..9006cca4f7f1 100644 --- a/drivers/gpu/drm/drm_mode_object.c +++ b/drivers/gpu/drm/drm_mode_object.c @@ -500,3 +500,44 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data, drm_mode_object_put(arg_obj); return ret; } + +int drm_mode_connector_dpms_ioctl(struct drm_device *dev, + void *data, struct drm_file *file_priv) +{ + struct drm_mode_connector_set_property *conn_set_prop = data; + struct drm_mode_obj_set_property obj_set_prop = { + .value = conn_set_prop->value, + .obj_id = conn_set_prop->connector_id, + .obj_type = DRM_MODE_OBJECT_CONNECTOR + }; + + struct drm_mode_obj_set_property *arg = &obj_set_prop; + struct drm_mode_object *arg_obj; + struct drm_property *property; + int ret = -EINVAL; + + struct drm_connector *connector; + + if (!drm_core_check_feature(dev, DRIVER_MODESET)) + return -EINVAL; + + arg_obj = drm_mode_object_find(dev, arg->obj_id, arg->obj_type); + if (!arg_obj) + return -ENOENT; + + if (!arg_obj->properties) + goto out_unref; + + /* XXX atomic */ + connector = obj_to_connector(arg_obj); + property = connector->dev->mode_config.dpms_property; + + if (drm_drv_uses_atomic_modeset(property->dev)) + ret = set_property_atomic(arg_obj, property, arg->value); + else + ret = set_property_legacy(arg_obj, property, arg->value); + +out_unref: + drm_mode_object_put(arg_obj); + return ret; +} diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 97677cd6964d..f60c14af1bcc 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -862,6 +862,7 @@ extern "C" { #define DRM_IOCTL_SYNCOBJ_WAIT DRM_IOWR(0xC3, struct drm_syncobj_wait) #define DRM_IOCTL_SYNCOBJ_RESET DRM_IOWR(0xC4, struct drm_syncobj_array) #define DRM_IOCTL_SYNCOBJ_SIGNAL DRM_IOWR(0xC5, struct drm_syncobj_array) +#define DRM_IOCTL_MODE_DPMS DRM_IOWR(0xC6, struct drm_mode_connector_set_property)
/** * Device specific ioctls should only be in their respective headers
On Wed, Oct 11, 2017 at 10:46:44AM +0300, Alex Ivanov wrote:
Display power management should be available to user no matter whether display server implements it or not and should be made server agnostic (X.Org, Wayland compositor), without need of TTY change.
Reasons:
- User implements an own universal power management daemon. He has no
interest to deal with server or switch to a free TTY, he wants to turn off the monitor in a simple way, like he controls power of other system components. 2. Some applications under X.Org like Electron-based ones like to prevent screen goning off giving user no way to fix that. 3. Many fresh new Wayland compositors don't implement DPMS at all and at the same time have TTY switch broken.
Signed-off-by: Alex Ivanov gnidorah@ya.ru
Why exactly is the existing dpms ioctl not good enough? it's implemented as the "DPMS" property attached to connectors, but that's the only difference compared to your code here.
Also, that one doesn't have any XXX about atomic like yours.
I also have no idea how this fixes any of your above problems. -Daniel
drivers/gpu/drm/drm_crtc_internal.h | 2 ++ drivers/gpu/drm/drm_ioctl.c | 1 + drivers/gpu/drm/drm_mode_object.c | 41 +++++++++++++++++++++++++++++++++++++ include/uapi/drm/drm.h | 1 + 4 files changed, 45 insertions(+)
diff --git a/drivers/gpu/drm/drm_crtc_internal.h b/drivers/gpu/drm/drm_crtc_internal.h index a43582076b20..120dd40a5210 100644 --- a/drivers/gpu/drm/drm_crtc_internal.h +++ b/drivers/gpu/drm/drm_crtc_internal.h @@ -122,6 +122,8 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); +int drm_mode_connector_dpms_ioctl(struct drm_device *dev,
void *data, struct drm_file *file_priv);/* drm_encoder.c */ int drm_encoder_register_all(struct drm_device *dev); diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index a9ae6dd2d593..2cec1a382c7b 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -632,6 +632,7 @@ static const struct drm_ioctl_desc drm_ioctls[] = { DRM_IOCTL_DEF(DRM_IOCTL_MODE_DETACHMODE, drm_noop, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPROPERTY, drm_mode_getproperty_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_SETPROPERTY, drm_mode_connector_property_set_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
- DRM_IOCTL_DEF(DRM_IOCTL_MODE_DPMS, drm_mode_connector_dpms_ioctl, DRM_ROOT_ONLY|DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETPROPBLOB, drm_mode_getblob_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_GETFB, drm_mode_getfb, DRM_CONTROL_ALLOW|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_ADDFB, drm_mode_addfb, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c index 1055533792f3..9006cca4f7f1 100644 --- a/drivers/gpu/drm/drm_mode_object.c +++ b/drivers/gpu/drm/drm_mode_object.c @@ -500,3 +500,44 @@ int drm_mode_obj_set_property_ioctl(struct drm_device *dev, void *data, drm_mode_object_put(arg_obj); return ret; }
+int drm_mode_connector_dpms_ioctl(struct drm_device *dev,
void *data, struct drm_file *file_priv)+{
- struct drm_mode_connector_set_property *conn_set_prop = data;
- struct drm_mode_obj_set_property obj_set_prop = {
.value = conn_set_prop->value,.obj_id = conn_set_prop->connector_id,.obj_type = DRM_MODE_OBJECT_CONNECTOR- };
- struct drm_mode_obj_set_property *arg = &obj_set_prop;
- struct drm_mode_object *arg_obj;
- struct drm_property *property;
- int ret = -EINVAL;
- struct drm_connector *connector;
- if (!drm_core_check_feature(dev, DRIVER_MODESET))
return -EINVAL;- arg_obj = drm_mode_object_find(dev, arg->obj_id, arg->obj_type);
- if (!arg_obj)
return -ENOENT;- if (!arg_obj->properties)
goto out_unref;- /* XXX atomic */
- connector = obj_to_connector(arg_obj);
- property = connector->dev->mode_config.dpms_property;
- if (drm_drv_uses_atomic_modeset(property->dev))
ret = set_property_atomic(arg_obj, property, arg->value);- else
ret = set_property_legacy(arg_obj, property, arg->value);+out_unref:
- drm_mode_object_put(arg_obj);
- return ret;
+} diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h index 97677cd6964d..f60c14af1bcc 100644 --- a/include/uapi/drm/drm.h +++ b/include/uapi/drm/drm.h @@ -862,6 +862,7 @@ extern "C" { #define DRM_IOCTL_SYNCOBJ_WAIT DRM_IOWR(0xC3, struct drm_syncobj_wait) #define DRM_IOCTL_SYNCOBJ_RESET DRM_IOWR(0xC4, struct drm_syncobj_array) #define DRM_IOCTL_SYNCOBJ_SIGNAL DRM_IOWR(0xC5, struct drm_syncobj_array) +#define DRM_IOCTL_MODE_DPMS DRM_IOWR(0xC6, struct drm_mode_connector_set_property)
/**
- Device specific ioctls should only be in their respective headers
-- 2.14.2
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 11.10.2017 12:04, Daniel Vetter wrote:
Why exactly is the existing dpms ioctl not good enough? it's implemented as the "DPMS" property attached to connectors, but that's the only difference compared to your code here.
Also, that one doesn't have any XXX about atomic like yours.
I also have no idea how this fixes any of your above problems. -Daniel
It's not good enough because it will only succeed if caller is current DRM master. Which means it will work only in console, since X.Org and Wayland hold DRM_MASTER.
But as a security measure this new separate ioctl requires calling process to be root one.
I will fix XXX atomic in second version of this patch, it's pretty mechanical change. Current stage is to get acceptance by public.
On Wed, 11 Oct 2017, Alex Ivanov gnidorah@ya.ru wrote:
On 11.10.2017 12:04, Daniel Vetter wrote:
Why exactly is the existing dpms ioctl not good enough? it's implemented as the "DPMS" property attached to connectors, but that's the only difference compared to your code here.
Also, that one doesn't have any XXX about atomic like yours.
I also have no idea how this fixes any of your above problems. -Daniel
It's not good enough because it will only succeed if caller is current DRM master. Which means it will work only in console, since X.Org and Wayland hold DRM_MASTER.
As said elsewhere in the thread, this is a feature, not a bug.
BR, Jani.
Hi,
On 11 October 2017 at 08:46, Alex Ivanov gnidorah@ya.ru wrote:
Display power management should be available to user no matter whether display server implements it or not and should be made server agnostic (X.Org, Wayland compositor), without need of TTY change.
Reasons:
- User implements an own universal power management daemon. He has no
interest to deal with server or switch to a free TTY, he wants to turn off the monitor in a simple way, like he controls power of other system components. 2. Some applications under X.Org like Electron-based ones like to prevent screen goning off giving user no way to fix that. 3. Many fresh new Wayland compositors don't implement DPMS at all and at the same time have TTY switch broken.
I'm sorry, but this is not the right way to go about things. If Xorg or GNOME Shell or Weston or whatever thinks the monitor is DPMS-off, the fact that someone else has forcibly switched it back on will not make them continue painting. Conversely, if they are painting and someone does DPMS-off from underneath them, they'll keep painting anyway: if they're using the legacy API that means the display will switch straight back on, and if they're using atomic that means their commits will suddenly fail for an unknown reason and they'll be very confused.
If people are doing fancy new compositors without DPMS support, then I recommend they use an existing compositor base, such as libweston (others are also available) which actually implement DPMS. For Electron apps, there are a number of idle-inhibition options available for X11, and also for Wayland when that support lands in Electron. Unfortunately I don't think there is any way to just route around the compositor as this patch does.
Cheers, Daniel
On 11.10.2017 13:13, Daniel Stone wrote:
I'm sorry, but this is not the right way to go about things. If Xorg or GNOME Shell or Weston or whatever thinks the monitor is DPMS-off, the fact that someone else has forcibly switched it back on will not make them continue painting.
User should disable DPMS management of display server before using this ioctl.
Conversely, if they are painting and someone does DPMS-off from underneath them, they'll keep painting anyway: if they're using the legacy API that means the display will switch straight back on, and if they're using atomic that means their commits will suddenly fail for an unknown reason and they'll be very confused.
That's sad to hear. Is that the reason why xf86-video-intel driver switches the display back on, while modesetting driver doesn't do anything? DPMS is disabled in X.Org config.
If people are doing fancy new compositors without DPMS support, then I recommend they use an existing compositor base, such as libweston (others are also available) which actually implement DPMS. For Electron apps, there are a number of idle-inhibition options available for X11, and also for Wayland when that support lands in Electron. Unfortunately I don't think there is any way to just route around the compositor as this patch does.
libweston is still too tied to Weston at this moment, and other libraries are still underway.
Your phrase is a great marker of how much complexity and trickery for user is added into display power management in comparison to power management of other components.
Cheers, Daniel
Hi Alex,
On 11 October 2017 at 12:22, Alex Ivanov gnidorah@ya.ru wrote:
On 11.10.2017 13:13, Daniel Stone wrote:
If people are doing fancy new compositors without DPMS support, then I recommend they use an existing compositor base, such as libweston (others are also available) which actually implement DPMS. For Electron apps, there are a number of idle-inhibition options available for X11, and also for Wayland when that support lands in Electron. Unfortunately I don't think there is any way to just route around the compositor as this patch does.
libweston is still too tied to Weston at this moment, and other libraries are still underway.
Your phrase is a great marker of how much complexity and trickery for user is added into display power management in comparison to power management of other components.
Unfortunately it is indeed quite complex, but this approach doesn't fundamentally remove complexity, but instead _add_ it. I'd much rather fix these problems at the source - add idle-inhibition or DPMS support where required to compositors and clients - than try to route around it.
Cheers, Daniel
On Wed, Oct 11, 2017 at 12:56:33PM +0100, Daniel Stone wrote:
Hi Alex,
On 11 October 2017 at 12:22, Alex Ivanov gnidorah@ya.ru wrote:
On 11.10.2017 13:13, Daniel Stone wrote:
If people are doing fancy new compositors without DPMS support, then I recommend they use an existing compositor base, such as libweston (others are also available) which actually implement DPMS. For Electron apps, there are a number of idle-inhibition options available for X11, and also for Wayland when that support lands in Electron. Unfortunately I don't think there is any way to just route around the compositor as this patch does.
libweston is still too tied to Weston at this moment, and other libraries are still underway.
Your phrase is a great marker of how much complexity and trickery for user is added into display power management in comparison to power management of other components.
Unfortunately it is indeed quite complex, but this approach doesn't fundamentally remove complexity, but instead _add_ it. I'd much rather fix these problems at the source - add idle-inhibition or DPMS support where required to compositors and clients - than try to route around it.
Yes now that I understand that your goal is to just bypass the compositor it's clear to me we don't want this. Implementing a compositor is hard, hacking around broken compositors is worse. As Daniel explained, changing kms state behind the compositor's back is not going to work reliably either.
Please fix your userspace.
Thanks, Daniel
dri-devel@lists.freedesktop.org
-
Alex Ivanov -
Daniel Stone -
Daniel Vetter -
Jani Nikula