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@xxxxxxxxxxxxxxxx> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx> > --- > 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@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
