| Message ID | 20170815145519.1162-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@ideasonboard.com> > Signed-off-by: Daniel Vetter <daniel.vetter@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". > 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
On Wed, Aug 16, 2017 at 4:49 PM, Laurent Pinchart <laurent.pinchart@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@ideasonboard.com> >> Signed-off-by: Daniel Vetter <daniel.vetter@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 <daniel.vetter@ffwll.ch> writes: > Laurent asked for this. > > Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > Signed-off-by: Daniel Vetter <daniel.vetter@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. 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 <eric@anholt.net>
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
Laurent asked for this. Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_connector.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)