[PATCH] drm: Try to document legacy DPMS uapi a bit better

[PATCH] drm: Try to document legacy DPMS uapi a bit better

[Daniel Vetter]

Laurent asked for this.

Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_connector.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index ba9f36cef68c..b458eb488128 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -720,6 +720,25 @@ DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
  * 	callback. For atomic drivers the remapping to the "ACTIVE" property is
  * 	implemented in the DRM core.  This is the only standard connector
  * 	property that userspace can change.
+ *
+ * 	WARNING:
+ *
+ * 	For userspace also running on legacy drivers the "DPMS" semantics are a
+ * 	lot more complicated. First, userspace cannot rely on the "DPMS" value
+ * 	returned by the GETCONNECTOR actually reflecting reality, because many
+ * 	drivers fail to update it. For atomic drivers this is taken care of in
+ * 	drm_atomic_helper_update_legacy_modeset_state().
+ *
+ * 	The second issue is that the DPMS state is only relevant when the
+ * 	connector is connected to a CRTC. In atomic the DRM core enforces that
+ * 	"ACTIVE" is off in such a case, no such checks exists for "DPMS".
+ * 	Finally, when enabling an output using the legacy SETCONFIG ioctl then
+ * 	"DPMS" is forced to ON. But see above, that might not be reflected in
+ * 	the software value.
+ *
+ * 	Summarizing: Only set "DPMS" when the connector is known to be enabled,
+ * 	assume that a successful SETCONFIG call also set "DPMS" to on, and never
+ * 	read back the value of "DPMS" because it can be incorrect.
  * PATH:
  * 	Connector path property to identify how this sink is physically
  * 	connected. Used by DP MST. This should be set by calling
-- 
2.13.3

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

Re: [PATCH] drm: Try to document legacy DPMS uapi a bit better

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

On Tuesday 15 Aug 2017 16:55:19 Daniel Vetter wrote:
> Laurent asked for this.

While this is true, I'm not sure it makes a proper commit message :-)

> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_connector.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_connector.c
> b/drivers/gpu/drm/drm_connector.c index ba9f36cef68c..b458eb488128 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -720,6 +720,25 @@ DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
>   * 	callback. For atomic drivers the remapping to the "ACTIVE" property is
>   * 	implemented in the DRM core.  This is the only standard connector
>   * 	property that userspace can change.
> + *
> + * 	WARNING:
> + *
> + * 	For userspace also running on legacy drivers the "DPMS" semantics are
> + * 	a lot more complicated.

What is "userspace also running on legacy drivers" ? Is that userspace that is 
atomic-aware and have different codes paths for atomic and non-atomic drivers, 
or userspace that uses the legacy API regardless of the driver ? I assume you 
mean the latter, in which case I would write it as "userspace using the legacy 
non-atomic API with atomic drivers".

> First, userspace cannot rely on the "DPMS"
> + * 	value returned by the GETCONNECTOR actually reflecting reality,
> + * 	because many drivers fail to update it. For atomic drivers this is
> + * 	taken care of in drm_atomic_helper_update_legacy_modeset_state().

Are you talking about atomic drivers not using 
drm_atomic_helper_update_legacy_modeset_state() (directly or indirectly 
through the atomic commit helpers) ? Are there many of those ? They should be 
fixed, I don't think we should consider this as the normal behaviour. I'd 
rather explain how the connector DPMS interacts with the connector CRTC_ID and 
the CRTC ACTIVE properties when the drivers get it right, and then possibly 
add a warning that some drivers don't implement it correctly.

I think "reflecting reality" is also vague. What do you mean by reality ? The 
fact the the DPMS property should reflect whether the connector is linked to 
an active CRTC (as explained in the existing DPMS property documentation) ?

> + * 	The second issue is that the DPMS state is only relevant when the
> + * 	connector is connected to a CRTC. In atomic the DRM core enforces that
> + * 	"ACTIVE" is off in such a case, no such checks exists for "DPMS".

What is "such a case" ? A connector not connected to a CRTC ?

> + * 	Finally, when enabling an output using the legacy SETCONFIG ioctl then
> + * 	"DPMS" is forced to ON. But see above, that might not be reflected in
> + * 	the software value.
> + *
> + * 	Summarizing: Only set "DPMS" when the connector is known to be
> + * 	enabled, assume that a successful SETCONFIG call also set "DPMS" to
> + * 	on, and never read back the value of "DPMS" because it can be
> + * 	incorrect.

The need to summarize two paragraphs in a third one indicates to me that the 
documentation is confusing.

>   * PATH:
>   * 	Connector path property to identify how this sink is physically
>   * 	connected. Used by DP MST. This should be set by calling

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH] drm: Try to document legacy DPMS uapi a bit better

[Daniel Vetter]

On Wed, Aug 16, 2017 at 4:49 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Daniel,
>
> Thank you for the patch.
>
> On Tuesday 15 Aug 2017 16:55:19 Daniel Vetter wrote:
>> Laurent asked for this.
>
> While this is true, I'm not sure it makes a proper commit message :-)
>
>> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>> ---
>>  drivers/gpu/drm/drm_connector.c | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_connector.c
>> b/drivers/gpu/drm/drm_connector.c index ba9f36cef68c..b458eb488128 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -720,6 +720,25 @@ DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
>>   *   callback. For atomic drivers the remapping to the "ACTIVE" property is
>>   *   implemented in the DRM core.  This is the only standard connector
>>   *   property that userspace can change.
>> + *
>> + *   WARNING:
>> + *
>> + *   For userspace also running on legacy drivers the "DPMS" semantics are
>> + *   a lot more complicated.
>
> What is "userspace also running on legacy drivers" ? Is that userspace that is
> atomic-aware and have different codes paths for atomic and non-atomic drivers,
> or userspace that uses the legacy API regardless of the driver ? I assume you
> mean the latter, in which case I would write it as "userspace using the legacy
> non-atomic API with atomic drivers".

Legacy DPMS on atomic drivers has well-defined semantics. Legacy DPMS
on legacy drivers is much worse, which is what this WARNING describes.
The intro para only defines how DPMS works for atomic drivers.

I'm not exactly sure how I can make this clearer, please give a
proposal. I thought it's pretty obvious that this is all about legacy
drivers, but somehow you read the entire thing as applying to atomic
drivers.

>> First, userspace cannot rely on the "DPMS"
>> + *   value returned by the GETCONNECTOR actually reflecting reality,
>> + *   because many drivers fail to update it. For atomic drivers this is
>> + *   taken care of in drm_atomic_helper_update_legacy_modeset_state().
>
> Are you talking about atomic drivers not using
> drm_atomic_helper_update_legacy_modeset_state() (directly or indirectly
> through the atomic commit helpers) ? Are there many of those ? They should be
> fixed, I don't think we should consider this as the normal behaviour. I'd
> rather explain how the connector DPMS interacts with the connector CRTC_ID and
> the CRTC ACTIVE properties when the drivers get it right, and then possibly
> add a warning that some drivers don't implement it correctly.

Again this is about legacy drivers.

> I think "reflecting reality" is also vague. What do you mean by reality ? The
> fact the the DPMS property should reflect whether the connector is linked to
> an active CRTC (as explained in the existing DPMS property documentation) ?

The reality of the non-atomic drivers we still ship. Where is the
"existing DPMS property documentation" btw? This here should be
everything, and it makes it clear DPMS is not very well linked to
anything.

>> + *   The second issue is that the DPMS state is only relevant when the
>> + *   connector is connected to a CRTC. In atomic the DRM core enforces that
>> + *   "ACTIVE" is off in such a case, no such checks exists for "DPMS".
>
> What is "such a case" ? A connector not connected to a CRTC ?

Yes. I guess I can hammer this home by repetition :-)

>> + *   Finally, when enabling an output using the legacy SETCONFIG ioctl then
>> + *   "DPMS" is forced to ON. But see above, that might not be reflected in
>> + *   the software value.
>> + *
>> + *   Summarizing: Only set "DPMS" when the connector is known to be
>> + *   enabled, assume that a successful SETCONFIG call also set "DPMS" to
>> + *   on, and never read back the value of "DPMS" because it can be
>> + *   incorrect.
>
> The need to summarize two paragraphs in a third one indicates to me that the
> documentation is confusing.

The idea is to give userspace writers guidelines what they should do
with DPMS. Should I make clearer what the audience of that summary is,
or is there no value in that?
-Daniel

>
>>   * PATH:
>>   *   Connector path property to identify how this sink is physically
>>   *   connected. Used by DP MST. This should be set by calling
>
> --
> Regards,
>
> Laurent Pinchart
>



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Try to document legacy DPMS uapi a bit better

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 2009 bytes --]

Daniel Vetter <daniel.vetter@ffwll.ch> writes:

> Laurent asked for this.
>
> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/drm_connector.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index ba9f36cef68c..b458eb488128 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -720,6 +720,25 @@ DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
>   * 	callback. For atomic drivers the remapping to the "ACTIVE" property is
>   * 	implemented in the DRM core.  This is the only standard connector
>   * 	property that userspace can change.
> + *
> + * 	WARNING:
> + *
> + * 	For userspace also running on legacy drivers the "DPMS" semantics are a
> + * 	lot more complicated. First, userspace cannot rely on the "DPMS" value
> + * 	returned by the GETCONNECTOR actually reflecting reality, because many
> + * 	drivers fail to update it. For atomic drivers this is taken care of in
> + * 	drm_atomic_helper_update_legacy_modeset_state().
> + *
> + * 	The second issue is that the DPMS state is only relevant when the

s/relevant/defined/ maybe?

> + * 	connector is connected to a CRTC. In atomic the DRM core enforces that
> + * 	"ACTIVE" is off in such a case, no such checks exists for "DPMS".

Maybe another newline here?

> + * 	Finally, when enabling an output using the legacy SETCONFIG ioctl then
> + * 	"DPMS" is forced to ON. But see above, that might not be reflected in
> + * 	the software value.

Maybe add " on legacy drivers"?

> + *
> + * 	Summarizing: Only set "DPMS" when the connector is known to be enabled,
> + * 	assume that a successful SETCONFIG call also set "DPMS" to on, and never

"also sets"

> + * 	read back the value of "DPMS" because it can be incorrect.

With whatever set of my suggestions you like,

Reviewed-by: Eric Anholt <eric@anholt.net>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

[PATCH] drm: Try to document legacy DPMS uapi a bit better

[Daniel Vetter]

Due to inconsistency of how various legacy drivers implemented DPMS
the DPMS uabi has a lot of quirks. Atomic standardizes this, but
drivers using the DPMS support can't rely on that since legacy drivers
still exist.

Laurent asked for this.

v2:
Improve commit message and explain that DPMS doesn't really exist for
the atomic ioctl (it's "ACTIVE" on the CRTC instead).

Text polish from Eric's review.

Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: David Airlie <airlied@linux.ie>
Cc: Eric Anholt <eric@anholt.net>
Reviewed-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/drm_connector.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index bb2e60f5feb6..d8ca526ca4ee 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -719,6 +719,29 @@ DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
  * 	callback. For atomic drivers the remapping to the "ACTIVE" property is
  * 	implemented in the DRM core.  This is the only standard connector
  * 	property that userspace can change.
+ *
+ * 	Note that this property cannot be set through the MODE_ATOMIC ioctl,
+ * 	userspace must use "ACTIVE" on the CRTC instead.
+ *
+ * 	WARNING:
+ *
+ * 	For userspace also running on legacy drivers the "DPMS" semantics are a
+ * 	lot more complicated. First, userspace cannot rely on the "DPMS" value
+ * 	returned by the GETCONNECTOR actually reflecting reality, because many
+ * 	drivers fail to update it. For atomic drivers this is taken care of in
+ * 	drm_atomic_helper_update_legacy_modeset_state().
+ *
+ * 	The second issue is that the DPMS state is only well-defined when the
+ * 	connector is connected to a CRTC. In atomic the DRM core enforces that
+ * 	"ACTIVE" is off in such a case, no such checks exists for "DPMS".
+ *
+ * 	Finally, when enabling an output using the legacy SETCONFIG ioctl then
+ * 	"DPMS" is forced to ON. But see above, that might not be reflected in
+ * 	the software value on legacy drivers.
+ *
+ * 	Summarizing: Only set "DPMS" when the connector is known to be enabled,
+ * 	assume that a successful SETCONFIG call also sets "DPMS" to on, and
+ * 	never read back the value of "DPMS" because it can be incorrect.
  * PATH:
  * 	Connector path property to identify how this sink is physically
  * 	connected. Used by DP MST. This should be set by calling
-- 
2.9.5

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

Re: [PATCH] drm: Try to document legacy DPMS uapi a bit better

[Sean Paul]

On Wed, Sep 20, 2017 at 3:59 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Due to inconsistency of how various legacy drivers implemented DPMS
> the DPMS uabi has a lot of quirks. Atomic standardizes this, but
> drivers using the DPMS support can't rely on that since legacy drivers
> still exist.
>
> Laurent asked for this.
>
> v2:
> Improve commit message and explain that DPMS doesn't really exist for
> the atomic ioctl (it's "ACTIVE" on the CRTC instead).
>
> Text polish from Eric's review.
>
> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Eric Anholt <eric@anholt.net>
> Reviewed-by: Eric Anholt <eric@anholt.net>

v2 is much better, IMO, thanks.

I'd recommend waiting for Laurent's R-b, but fwiw,

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
>  drivers/gpu/drm/drm_connector.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index bb2e60f5feb6..d8ca526ca4ee 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -719,6 +719,29 @@ DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
>   *     callback. For atomic drivers the remapping to the "ACTIVE" property is
>   *     implemented in the DRM core.  This is the only standard connector
>   *     property that userspace can change.
> + *
> + *     Note that this property cannot be set through the MODE_ATOMIC ioctl,
> + *     userspace must use "ACTIVE" on the CRTC instead.
> + *
> + *     WARNING:
> + *
> + *     For userspace also running on legacy drivers the "DPMS" semantics are a
> + *     lot more complicated. First, userspace cannot rely on the "DPMS" value
> + *     returned by the GETCONNECTOR actually reflecting reality, because many
> + *     drivers fail to update it. For atomic drivers this is taken care of in
> + *     drm_atomic_helper_update_legacy_modeset_state().
> + *
> + *     The second issue is that the DPMS state is only well-defined when the
> + *     connector is connected to a CRTC. In atomic the DRM core enforces that
> + *     "ACTIVE" is off in such a case, no such checks exists for "DPMS".
> + *
> + *     Finally, when enabling an output using the legacy SETCONFIG ioctl then
> + *     "DPMS" is forced to ON. But see above, that might not be reflected in
> + *     the software value on legacy drivers.
> + *
> + *     Summarizing: Only set "DPMS" when the connector is known to be enabled,
> + *     assume that a successful SETCONFIG call also sets "DPMS" to on, and
> + *     never read back the value of "DPMS" because it can be incorrect.
>   * PATH:
>   *     Connector path property to identify how this sink is physically
>   *     connected. Used by DP MST. This should be set by calling
> --
> 2.9.5
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Try to document legacy DPMS uapi a bit better

[Daniel Vetter]

On Wed, Sep 20, 2017 at 04:03:26PM -0700, Sean Paul wrote:
> On Wed, Sep 20, 2017 at 3:59 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > Due to inconsistency of how various legacy drivers implemented DPMS
> > the DPMS uabi has a lot of quirks. Atomic standardizes this, but
> > drivers using the DPMS support can't rely on that since legacy drivers
> > still exist.
> >
> > Laurent asked for this.
> >
> > v2:
> > Improve commit message and explain that DPMS doesn't really exist for
> > the atomic ioctl (it's "ACTIVE" on the CRTC instead).
> >
> > Text polish from Eric's review.
> >
> > Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Sean Paul <seanpaul@chromium.org>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Eric Anholt <eric@anholt.net>
> > Reviewed-by: Eric Anholt <eric@anholt.net>
> 
> v2 is much better, IMO, thanks.
> 
> I'd recommend waiting for Laurent's R-b, but fwiw,
> 
> Reviewed-by: Sean Paul <seanpaul@chromium.org>

Few days waiting, I'll fix it up with a follow-up if Laurent has more
things for me to polish.

Applied, thanks for the reviews.
-Daniel

> 
> > ---
> >  drivers/gpu/drm/drm_connector.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> > index bb2e60f5feb6..d8ca526ca4ee 100644
> > --- a/drivers/gpu/drm/drm_connector.c
> > +++ b/drivers/gpu/drm/drm_connector.c
> > @@ -719,6 +719,29 @@ DRM_ENUM_NAME_FN(drm_get_tv_subconnector_name,
> >   *     callback. For atomic drivers the remapping to the "ACTIVE" property is
> >   *     implemented in the DRM core.  This is the only standard connector
> >   *     property that userspace can change.
> > + *
> > + *     Note that this property cannot be set through the MODE_ATOMIC ioctl,
> > + *     userspace must use "ACTIVE" on the CRTC instead.
> > + *
> > + *     WARNING:
> > + *
> > + *     For userspace also running on legacy drivers the "DPMS" semantics are a
> > + *     lot more complicated. First, userspace cannot rely on the "DPMS" value
> > + *     returned by the GETCONNECTOR actually reflecting reality, because many
> > + *     drivers fail to update it. For atomic drivers this is taken care of in
> > + *     drm_atomic_helper_update_legacy_modeset_state().
> > + *
> > + *     The second issue is that the DPMS state is only well-defined when the
> > + *     connector is connected to a CRTC. In atomic the DRM core enforces that
> > + *     "ACTIVE" is off in such a case, no such checks exists for "DPMS".
> > + *
> > + *     Finally, when enabling an output using the legacy SETCONFIG ioctl then
> > + *     "DPMS" is forced to ON. But see above, that might not be reflected in
> > + *     the software value on legacy drivers.
> > + *
> > + *     Summarizing: Only set "DPMS" when the connector is known to be enabled,
> > + *     assume that a successful SETCONFIG call also sets "DPMS" to on, and
> > + *     never read back the value of "DPMS" because it can be incorrect.
> >   * PATH:
> >   *     Connector path property to identify how this sink is physically
> >   *     connected. Used by DP MST. This should be set by calling
> > --
> > 2.9.5
> >

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