[Intel-gfx] [PATCH 1/3] drm: Add a new connector atomic property for link status
On Mon, Dec 05, 2016 at 12:33:28PM -0800, Manasi Navare wrote: > On Fri, Dec 02, 2016 at 05:26:35PM +0100, Daniel Vetter wrote: > > Hm, tiny new bikeshed: I'd drop the _property postfix here. The function > > name is already really long as-is, and the fact that the link status is > > exposed as a property is kinda irrelevant for kernel driver writers. But > > up to you. > > -Daniel > > > > I decided to name it as drm_mode_connector_set_link_status_property() > looking at names of other set_property functions like > drm_mode_connector_update_edid_property() or > drm_mode_connector_set_path_property(). > Thats why I think it makes sense to append it with property just to be > consistent. Yeah it's fine either way, maybe we could just drop _property from all of them. Really doesn't add much, but these long function names make reasonable code layout a pain. Anyway, nothing that must be done here, agreed. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[Intel-gfx] [PATCH 1/3] drm: Add a new connector atomic property for link status
On Fri, Dec 02, 2016 at 05:26:35PM +0100, Daniel Vetter wrote:
> On Tue, Nov 29, 2016 at 11:30:31PM -0800, Manasi Navare wrote:
> > At the time userspace does setcrtc, we've already promised the mode
> > would work. The promise is based on the theoretical capabilities of
> > the link, but it's possible we can't reach this in practice. The DP
> > spec describes how the link should be reduced, but we can't reduce
> > the link below the requirements of the mode. Black screen follows.
> >
> > One idea would be to have setcrtc return a failure. However, it
> > already should not fail as the atomic checks have passed. It would
> > also conflict with the idea of making setcrtc asynchronous in the
> > future, returning before the actual mode setting and link training.
> >
> > Another idea is to train the link "upfront" at hotplug time, before
> > pruning the mode list, so that we can do the pruning based on
> > practical not theoretical capabilities. However, the changes for link
> > training are pretty drastic, all for the sake of error handling and
> > DP compliance, when the most common happy day scenario is the current
> > approach of link training at mode setting time, using the optimal
> > parameters for the mode. It is also not certain all hardware could do
> > this without the pipe on; not even all our hardware can do this. Some
> > of this can be solved, but not trivially.
> >
> > Both of the above ideas also fail to address link degradation *during*
> > operation.
> >
> > The solution is to add a new "link-status" connector property in order
> > to address link training failure in a way that:
> > a) changes the current happy day scenario as little as possible, to
> > avoid regressions, b) can be implemented the same way by all drm
> > drivers, c) is still opt-in for the drivers and userspace, and opting
> > out doesn't regress the user experience, d) doesn't prevent drivers
> > from implementing better or alternate approaches, possibly without
> > userspace involvement. And, of course, handles all the issues presented.
> > In the usual happy day scenario, this is always "good". If something
> > fails during or after a mode set, the kernel driver can set the link
> > status to "bad" and issue a hotplug uevent for userspace to have it
> > re-check the valid modes through GET_CONNECTOR IOCTL, and try modeset
> > again. If the theoretical capabilities of the link can't be reached,
> > the mode list is trimmed based on that.
> >
> > v4:
> > * Add comments in kernel-doc format (Daniel Vetter)
> > * Update the kernel-doc for link-status (Sean Paul)
> > v3:
> > * Fixed a build error (Jani Saarinen)
> > v2:
> > * Removed connector->link_status (Daniel Vetter)
> > * Set connector->state->link_status in
> > drm_mode_connector_set_link_status_property
> > (Daniel Vetter)
> > * Set the connector_changed flag to true if connector->state->link_status
> > changed.
> > * Reset link_status to GOOD in update_output_state (Daniel Vetter)
> > * Never allow userspace to set link status from Good To Bad (Daniel Vetter)
> >
> > Acked-by: Tony Cheng
> > Acked-by: Harry Wentland
> > Cc: Jani Nikula
> > Cc: Daniel Vetter
> > Cc: Ville Syrjala
> > Cc: Chris Wilson
> > Cc: Sean Paul
> > Signed-off-by: Manasi Navare
> > ---
> > drivers/gpu/drm/drm_atomic.c| 10 +++
> > drivers/gpu/drm/drm_atomic_helper.c | 8 ++
> > drivers/gpu/drm/drm_connector.c | 54
> > -
> > include/drm/drm_connector.h | 19 +
> > include/drm/drm_mode_config.h | 5
> > include/uapi/drm/drm_mode.h | 4 +++
> > 6 files changed, 99 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 89737e4..990f013 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1087,6 +1087,14 @@ int drm_atomic_connector_set_property(struct
> > drm_connector *connector,
> > * now?) atomic writes to DPMS property:
> > */
> > return -EINVAL;
> > + } else if (property == config->link_status_property) {
> > + /* Never downgrade from GOOD to BAD on userspace's request here,
> > +* only hw issues can do that.
> > +*/
> > + if (state->link_status == DRM_LINK_STATUS_GOOD)
> > + return 0;
> > + state->link_status = val;
> > + return 0;
> > } else if (connector->funcs->atomic_set_property) {
> > return connector->funcs->atomic_set_property(connector,
> > state, property, val);
> > @@ -1135,6 +1143,8 @@ static void drm_atomic_connector_print_state(struct
> > drm_printer *p,
> > *val = (state->crtc) ? state->crtc->base.id : 0;
> > } else if (property == config->dpms_property) {
> > *val = connector->dpms;
> > + } else if (property == config->link_status_property) {
> > + *val =
[Intel-gfx] [PATCH 1/3] drm: Add a new connector atomic property for link status
On Fri, Dec 02, 2016 at 09:48:59AM -0800, Manasi Navare wrote:
> On Fri, Dec 02, 2016 at 05:26:35PM +0100, Daniel Vetter wrote:
> > On Tue, Nov 29, 2016 at 11:30:31PM -0800, Manasi Navare wrote:
> > > At the time userspace does setcrtc, we've already promised the mode
> > > would work. The promise is based on the theoretical capabilities of
> > > the link, but it's possible we can't reach this in practice. The DP
> > > spec describes how the link should be reduced, but we can't reduce
> > > the link below the requirements of the mode. Black screen follows.
> > >
> > > One idea would be to have setcrtc return a failure. However, it
> > > already should not fail as the atomic checks have passed. It would
> > > also conflict with the idea of making setcrtc asynchronous in the
> > > future, returning before the actual mode setting and link training.
> > >
> > > Another idea is to train the link "upfront" at hotplug time, before
> > > pruning the mode list, so that we can do the pruning based on
> > > practical not theoretical capabilities. However, the changes for link
> > > training are pretty drastic, all for the sake of error handling and
> > > DP compliance, when the most common happy day scenario is the current
> > > approach of link training at mode setting time, using the optimal
> > > parameters for the mode. It is also not certain all hardware could do
> > > this without the pipe on; not even all our hardware can do this. Some
> > > of this can be solved, but not trivially.
> > >
> > > Both of the above ideas also fail to address link degradation *during*
> > > operation.
> > >
> > > The solution is to add a new "link-status" connector property in order
> > > to address link training failure in a way that:
> > > a) changes the current happy day scenario as little as possible, to
> > > avoid regressions, b) can be implemented the same way by all drm
> > > drivers, c) is still opt-in for the drivers and userspace, and opting
> > > out doesn't regress the user experience, d) doesn't prevent drivers
> > > from implementing better or alternate approaches, possibly without
> > > userspace involvement. And, of course, handles all the issues presented.
> > > In the usual happy day scenario, this is always "good". If something
> > > fails during or after a mode set, the kernel driver can set the link
> > > status to "bad" and issue a hotplug uevent for userspace to have it
> > > re-check the valid modes through GET_CONNECTOR IOCTL, and try modeset
> > > again. If the theoretical capabilities of the link can't be reached,
> > > the mode list is trimmed based on that.
> > >
> > > v4:
> > > * Add comments in kernel-doc format (Daniel Vetter)
> > > * Update the kernel-doc for link-status (Sean Paul)
> > > v3:
> > > * Fixed a build error (Jani Saarinen)
> > > v2:
> > > * Removed connector->link_status (Daniel Vetter)
> > > * Set connector->state->link_status in
> > > drm_mode_connector_set_link_status_property
> > > (Daniel Vetter)
> > > * Set the connector_changed flag to true if connector->state->link_status
> > > changed.
> > > * Reset link_status to GOOD in update_output_state (Daniel Vetter)
> > > * Never allow userspace to set link status from Good To Bad (Daniel
> > > Vetter)
> > >
> > > Acked-by: Tony Cheng
> > > Acked-by: Harry Wentland
> > > Cc: Jani Nikula
> > > Cc: Daniel Vetter
> > > Cc: Ville Syrjala
> > > Cc: Chris Wilson
> > > Cc: Sean Paul
> > > Signed-off-by: Manasi Navare
> > > ---
> > > drivers/gpu/drm/drm_atomic.c| 10 +++
> > > drivers/gpu/drm/drm_atomic_helper.c | 8 ++
> > > drivers/gpu/drm/drm_connector.c | 54
> > > -
> > > include/drm/drm_connector.h | 19 +
> > > include/drm/drm_mode_config.h | 5
> > > include/uapi/drm/drm_mode.h | 4 +++
> > > 6 files changed, 99 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index 89737e4..990f013 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -1087,6 +1087,14 @@ int drm_atomic_connector_set_property(struct
> > > drm_connector *connector,
> > >* now?) atomic writes to DPMS property:
> > >*/
> > > return -EINVAL;
> > > + } else if (property == config->link_status_property) {
> > > + /* Never downgrade from GOOD to BAD on userspace's request here,
> > > + * only hw issues can do that.
> > > + */
> > > + if (state->link_status == DRM_LINK_STATUS_GOOD)
> > > + return 0;
> > > + state->link_status = val;
> > > + return 0;
> > > } else if (connector->funcs->atomic_set_property) {
> > > return connector->funcs->atomic_set_property(connector,
> > > state, property, val);
> > > @@ -1135,6 +1143,8 @@ static void drm_atomic_connector_print_state(struct
> > > drm_printer *p,
> > >
[Intel-gfx] [PATCH 1/3] drm: Add a new connector atomic property for link status
On Tue, Nov 29, 2016 at 11:30:31PM -0800, Manasi Navare wrote:
> At the time userspace does setcrtc, we've already promised the mode
> would work. The promise is based on the theoretical capabilities of
> the link, but it's possible we can't reach this in practice. The DP
> spec describes how the link should be reduced, but we can't reduce
> the link below the requirements of the mode. Black screen follows.
>
> One idea would be to have setcrtc return a failure. However, it
> already should not fail as the atomic checks have passed. It would
> also conflict with the idea of making setcrtc asynchronous in the
> future, returning before the actual mode setting and link training.
>
> Another idea is to train the link "upfront" at hotplug time, before
> pruning the mode list, so that we can do the pruning based on
> practical not theoretical capabilities. However, the changes for link
> training are pretty drastic, all for the sake of error handling and
> DP compliance, when the most common happy day scenario is the current
> approach of link training at mode setting time, using the optimal
> parameters for the mode. It is also not certain all hardware could do
> this without the pipe on; not even all our hardware can do this. Some
> of this can be solved, but not trivially.
>
> Both of the above ideas also fail to address link degradation *during*
> operation.
>
> The solution is to add a new "link-status" connector property in order
> to address link training failure in a way that:
> a) changes the current happy day scenario as little as possible, to
> avoid regressions, b) can be implemented the same way by all drm
> drivers, c) is still opt-in for the drivers and userspace, and opting
> out doesn't regress the user experience, d) doesn't prevent drivers
> from implementing better or alternate approaches, possibly without
> userspace involvement. And, of course, handles all the issues presented.
> In the usual happy day scenario, this is always "good". If something
> fails during or after a mode set, the kernel driver can set the link
> status to "bad" and issue a hotplug uevent for userspace to have it
> re-check the valid modes through GET_CONNECTOR IOCTL, and try modeset
> again. If the theoretical capabilities of the link can't be reached,
> the mode list is trimmed based on that.
>
> v4:
> * Add comments in kernel-doc format (Daniel Vetter)
> * Update the kernel-doc for link-status (Sean Paul)
> v3:
> * Fixed a build error (Jani Saarinen)
> v2:
> * Removed connector->link_status (Daniel Vetter)
> * Set connector->state->link_status in
> drm_mode_connector_set_link_status_property
> (Daniel Vetter)
> * Set the connector_changed flag to true if connector->state->link_status
> changed.
> * Reset link_status to GOOD in update_output_state (Daniel Vetter)
> * Never allow userspace to set link status from Good To Bad (Daniel Vetter)
>
> Acked-by: Tony Cheng
> Acked-by: Harry Wentland
> Cc: Jani Nikula
> Cc: Daniel Vetter
> Cc: Ville Syrjala
> Cc: Chris Wilson
> Cc: Sean Paul
> Signed-off-by: Manasi Navare
> ---
> drivers/gpu/drm/drm_atomic.c| 10 +++
> drivers/gpu/drm/drm_atomic_helper.c | 8 ++
> drivers/gpu/drm/drm_connector.c | 54
> -
> include/drm/drm_connector.h | 19 +
> include/drm/drm_mode_config.h | 5
> include/uapi/drm/drm_mode.h | 4 +++
> 6 files changed, 99 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 89737e4..990f013 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1087,6 +1087,14 @@ int drm_atomic_connector_set_property(struct
> drm_connector *connector,
>* now?) atomic writes to DPMS property:
>*/
> return -EINVAL;
> + } else if (property == config->link_status_property) {
> + /* Never downgrade from GOOD to BAD on userspace's request here,
> + * only hw issues can do that.
> + */
> + if (state->link_status == DRM_LINK_STATUS_GOOD)
> + return 0;
> + state->link_status = val;
> + return 0;
> } else if (connector->funcs->atomic_set_property) {
> return connector->funcs->atomic_set_property(connector,
> state, property, val);
> @@ -1135,6 +1143,8 @@ static void drm_atomic_connector_print_state(struct
> drm_printer *p,
> *val = (state->crtc) ? state->crtc->base.id : 0;
> } else if (property == config->dpms_property) {
> *val = connector->dpms;
> + } else if (property == config->link_status_property) {
> + *val = state->link_status;
> } else if (connector->funcs->atomic_get_property) {
> return connector->funcs->atomic_get_property(connector,
> state, property, val);
> diff --git
[Intel-gfx] [PATCH 1/3] drm: Add a new connector atomic property for link status
On Fri, Dec 02, 2016 at 05:26:35PM +0100, Daniel Vetter wrote:
> On Tue, Nov 29, 2016 at 11:30:31PM -0800, Manasi Navare wrote:
> > At the time userspace does setcrtc, we've already promised the mode
> > would work. The promise is based on the theoretical capabilities of
> > the link, but it's possible we can't reach this in practice. The DP
> > spec describes how the link should be reduced, but we can't reduce
> > the link below the requirements of the mode. Black screen follows.
> >
> > One idea would be to have setcrtc return a failure. However, it
> > already should not fail as the atomic checks have passed. It would
> > also conflict with the idea of making setcrtc asynchronous in the
> > future, returning before the actual mode setting and link training.
> >
> > Another idea is to train the link "upfront" at hotplug time, before
> > pruning the mode list, so that we can do the pruning based on
> > practical not theoretical capabilities. However, the changes for link
> > training are pretty drastic, all for the sake of error handling and
> > DP compliance, when the most common happy day scenario is the current
> > approach of link training at mode setting time, using the optimal
> > parameters for the mode. It is also not certain all hardware could do
> > this without the pipe on; not even all our hardware can do this. Some
> > of this can be solved, but not trivially.
> >
> > Both of the above ideas also fail to address link degradation *during*
> > operation.
> >
> > The solution is to add a new "link-status" connector property in order
> > to address link training failure in a way that:
> > a) changes the current happy day scenario as little as possible, to
> > avoid regressions, b) can be implemented the same way by all drm
> > drivers, c) is still opt-in for the drivers and userspace, and opting
> > out doesn't regress the user experience, d) doesn't prevent drivers
> > from implementing better or alternate approaches, possibly without
> > userspace involvement. And, of course, handles all the issues presented.
> > In the usual happy day scenario, this is always "good". If something
> > fails during or after a mode set, the kernel driver can set the link
> > status to "bad" and issue a hotplug uevent for userspace to have it
> > re-check the valid modes through GET_CONNECTOR IOCTL, and try modeset
> > again. If the theoretical capabilities of the link can't be reached,
> > the mode list is trimmed based on that.
> >
> > v4:
> > * Add comments in kernel-doc format (Daniel Vetter)
> > * Update the kernel-doc for link-status (Sean Paul)
> > v3:
> > * Fixed a build error (Jani Saarinen)
> > v2:
> > * Removed connector->link_status (Daniel Vetter)
> > * Set connector->state->link_status in
> > drm_mode_connector_set_link_status_property
> > (Daniel Vetter)
> > * Set the connector_changed flag to true if connector->state->link_status
> > changed.
> > * Reset link_status to GOOD in update_output_state (Daniel Vetter)
> > * Never allow userspace to set link status from Good To Bad (Daniel Vetter)
> >
> > Acked-by: Tony Cheng
> > Acked-by: Harry Wentland
> > Cc: Jani Nikula
> > Cc: Daniel Vetter
> > Cc: Ville Syrjala
> > Cc: Chris Wilson
> > Cc: Sean Paul
> > Signed-off-by: Manasi Navare
> > ---
> > drivers/gpu/drm/drm_atomic.c| 10 +++
> > drivers/gpu/drm/drm_atomic_helper.c | 8 ++
> > drivers/gpu/drm/drm_connector.c | 54
> > -
> > include/drm/drm_connector.h | 19 +
> > include/drm/drm_mode_config.h | 5
> > include/uapi/drm/drm_mode.h | 4 +++
> > 6 files changed, 99 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 89737e4..990f013 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1087,6 +1087,14 @@ int drm_atomic_connector_set_property(struct
> > drm_connector *connector,
> > * now?) atomic writes to DPMS property:
> > */
> > return -EINVAL;
> > + } else if (property == config->link_status_property) {
> > + /* Never downgrade from GOOD to BAD on userspace's request here,
> > +* only hw issues can do that.
> > +*/
> > + if (state->link_status == DRM_LINK_STATUS_GOOD)
> > + return 0;
> > + state->link_status = val;
> > + return 0;
> > } else if (connector->funcs->atomic_set_property) {
> > return connector->funcs->atomic_set_property(connector,
> > state, property, val);
> > @@ -1135,6 +1143,8 @@ static void drm_atomic_connector_print_state(struct
> > drm_printer *p,
> > *val = (state->crtc) ? state->crtc->base.id : 0;
> > } else if (property == config->dpms_property) {
> > *val = connector->dpms;
> > + } else if (property == config->link_status_property) {
> > + *val =
