| Message ID | 20170920225957.16278-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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 > >
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