drm: Try to document legacy DPMS uapi a bit better
diff mbox

Message ID 20170920225957.16278-1-daniel.vetter@ffwll.ch
State New
Headers show

Commit Message

Daniel Vetter Sept. 20, 2017, 10:59 p.m. UTC
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 <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Sean Paul <seanpaul@chromium.org>
Cc: David Airlie <airlied@linux.ie>
Cc: Eric Anholt <eric@anholt.net>
Reviewed-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/drm_connector.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

Comments

Sean Paul Sept. 20, 2017, 11:03 p.m. UTC | #1
On Wed, Sep 20, 2017 at 3:59 PM, Daniel Vetter <daniel.vetter@ffwll.ch> 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 <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Eric Anholt <eric@anholt.net>
> Reviewed-by: Eric Anholt <eric@anholt.net>

v2 is much better, IMO, thanks.

I'd recommend waiting for Laurent's R-b, but fwiw,

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
>  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 Sept. 26, 2017, 5:13 a.m. UTC | #2
On Wed, Sep 20, 2017 at 04:03:26PM -0700, Sean Paul wrote:
> On Wed, Sep 20, 2017 at 3:59 PM, Daniel Vetter <daniel.vetter@ffwll.ch> 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 <laurent.pinchart+renesas@ideasonboard.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Sean Paul <seanpaul@chromium.org>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: Eric Anholt <eric@anholt.net>
> > Reviewed-by: Eric Anholt <eric@anholt.net>
> 
> v2 is much better, IMO, thanks.
> 
> I'd recommend waiting for Laurent's R-b, but fwiw,
> 
> Reviewed-by: Sean Paul <seanpaul@chromium.org>

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
> >

Patch
diff mbox

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