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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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