Patchwork drm: Try to document legacy DPMS uapi a bit better

login
register
mail settings
Submitter Daniel Vetter
Date Aug. 15, 2017, 2:55 p.m.
Message ID <20170815145519.1162-1-daniel.vetter@ffwll.ch>
Download mbox | patch
Permalink /patch/9902007/
State New
Headers show

Comments

Daniel Vetter - Aug. 15, 2017, 2:55 p.m.
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(+)
Laurent Pinchart - Aug. 16, 2017, 2:49 p.m.
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
Daniel Vetter - Aug. 16, 2017, 4:45 p.m.
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
>
Eric Anholt - Sept. 20, 2017, 10:48 p.m.
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>

Patch

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