[PATCH] drm: Add DPMS ioctl

[PATCH] drm: Add DPMS ioctl

[Alex Ivanov]

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
-- 
2.14.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Add DPMS ioctl

[Daniel Vetter]

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:
> 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>

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Add DPMS ioctl

[Alex Ivanov]

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.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Add DPMS ioctl

[Jani Nikula]

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.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Add DPMS ioctl

[Daniel Stone]

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:
> 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.

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Add DPMS ioctl

[Alex Ivanov]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Add DPMS ioctl

[Daniel Stone]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Add DPMS ioctl

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel