Re: [PATCH] drm: Try to document legacy DPMS uapi a bit better
On Wed, Sep 20, 2017 at 04:03:26PM -0700, Sean Paul wrote: > On Wed, Sep 20, 2017 at 3:59 PM, Daniel Vetter 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 > > Signed-off-by: Daniel Vetter > > Cc: Daniel Vetter > > Cc: Jani Nikula > > Cc: Sean Paul > > Cc: David Airlie > > Cc: Eric Anholt > > Reviewed-by: Eric Anholt > > v2 is much better, IMO, thanks. > > I'd recommend waiting for Laurent's R-b, but fwiw, > > Reviewed-by: Sean Paul 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
Re: [PATCH] drm: Try to document legacy DPMS uapi a bit better
On Wed, Sep 20, 2017 at 3:59 PM, Daniel Vetter 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 > Signed-off-by: Daniel Vetter > Cc: Daniel Vetter > Cc: Jani Nikula > Cc: Sean Paul > Cc: David Airlie > Cc: Eric Anholt > Reviewed-by: Eric Anholt v2 is much better, IMO, thanks. I'd recommend waiting for Laurent's R-b, but fwiw, Reviewed-by: Sean Paul > --- > 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
[PATCH] drm: Try to document legacy DPMS uapi a bit better
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 Signed-off-by: Daniel Vetter Cc: Daniel Vetter Cc: Jani Nikula Cc: Sean Paul Cc: David Airlie Cc: Eric Anholt Reviewed-by: Eric Anholt --- 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 writes: > Laurent asked for this. > > Cc: Laurent Pinchart > Signed-off-by: Daniel Vetter > --- > 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 signature.asc Description: PGP signature ___ 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
On Wed, Aug 16, 2017 at 4:49 PM, Laurent Pinchart 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 >> Signed-off-by: Daniel Vetter >> --- >> 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
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 > Signed-off-by: Daniel Vetter > --- > 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
[PATCH] drm: Try to document legacy DPMS uapi a bit better
Laurent asked for this. Cc: Laurent Pinchart Signed-off-by: Daniel Vetter --- 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
