[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 at 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 at ideasonboard.com>
>> Signed-off-by: Daniel Vetter <daniel.vetter at 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