Re: [Intel-gfx] [PATCH 1/3] drm: Add a new connector atomic property for link status

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 <tony.cheng@xxxxxxx>
> > > Acked-by: Harry Wentland <harry.wentland@xxxxxxx>
> > > Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > > Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > > Cc: Sean Paul <seanpaul@xxxxxxxxxxxx>
> > > Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx>
> > > ---
> > >  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 a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 494680c..962ed66 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -519,6 +519,13 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
> > >  					       connector_state);
> > >  		if (ret)
> > >  			return ret;
> > > +		if (connector->state->crtc) {
> > > +			crtc_state = drm_atomic_get_existing_crtc_state(state,
> > > +									connector->state->crtc);
> > > +			if (connector->state->link_status !=
> > > +			    connector_state->link_status)
> > > +				crtc_state->connectors_changed = true;
> > > +		}
> > 
> > See my previous review, this looks should imo be moved to a more obvious
> > place than hidden deep down in the callchain.
> > 
> > >  	}
> 
> Yes I have looked at your previous review and this is inside drm_atomic_helper_check_modeset()
> function just inside the connectors loop. Its not in the handle_conflicting_encoders function.
> Its weird that in the diff it is showing up here. 
> If you pull the patch you will see that it is written as follows:
> 
> for_each_connector_in_state(state, connector, connector_state, i) {
>                 /*
>                  * This only sets crtc->connectors_changed for routing changes,
>                  * drivers must set crtc->connectors_changed themselves when
>                  * connector properties need to be updated.
>                  */
>                 ret = update_connector_routing(state, connector,
>                                                connector_state);
>                 if (ret)
>                         return ret;
>                 if (connector->state->crtc) {
>                         crtc_state = drm_atomic_get_existing_crtc_state(state,
>                                                                         connector->state->crtc);
>                         if (connector->state->link_status !=
>                             connector_state->link_status)
>                                 crtc_state->connectors_changed = true;
>                 }
>         }
> 
> Is this correct?

Yup, double checked myself, no idea what git diff is doing here thinking
it's in a different function ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux