* [PATCH 0/5] Handle link training failure for DDI platforms @ 2016-10-21 23:45 Manasi Navare 2016-10-21 23:45 ` [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode Manasi Navare ` (5 more replies) 0 siblings, 6 replies; 33+ messages in thread From: Manasi Navare @ 2016-10-21 23:45 UTC (permalink / raw) To: intel-gfx According to the DP spec 1.2, link training failure needs to be handled by lowering the link rate and retraining the link. These patches implement this link rate fallback. Currently the driver trains the link in atomic commit. This could fail if Clock Recovery or Channel EQ fails during actual link training. In this case, we track the link parameters at which it failed, validate the mode list based on new link constraints and redo a modeset within the kernel if the current mode is still valid but on lower link rate and lower bpp else send a hotplug uevent to notify userspace to try a different mode. This has been tested only on DDI platforms now using DPR120 DP Compliance tests. More patches will be submitted to scale this to older non DDI platforms. Manasi Navare (4): drm: Add atomic helper to redo a modeset on current mode drm: Define a work struct for scheduling a uevent for modeset retry drm/i915; Add a function to return index of link rate drm/i915: Link Rate fallback on Link training failure Navare, Manasi D (1): drm/i915: Change the placement of some static functions in intel_dp.c drivers/gpu/drm/drm_atomic_helper.c | 58 +++++++ drivers/gpu/drm/i915/intel_ddi.c | 15 +- drivers/gpu/drm/i915/intel_dp.c | 234 +++++++++++++++++--------- drivers/gpu/drm/i915/intel_dp_link_training.c | 12 +- drivers/gpu/drm/i915/intel_drv.h | 6 +- include/drm/drm_atomic_helper.h | 1 + include/drm/drm_connector.h | 5 + 7 files changed, 249 insertions(+), 82 deletions(-) -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode 2016-10-21 23:45 [PATCH 0/5] Handle link training failure for DDI platforms Manasi Navare @ 2016-10-21 23:45 ` Manasi Navare 2016-10-22 8:47 ` Daniel Vetter 2016-10-25 12:09 ` Jani Nikula 2016-10-21 23:45 ` [PATCH 2/5] drm: Define a work struct for scheduling a uevent for modeset retry Manasi Navare ` (4 subsequent siblings) 5 siblings, 2 replies; 33+ messages in thread From: Manasi Navare @ 2016-10-21 23:45 UTC (permalink / raw) To: intel-gfx; +Cc: Daniel Vetter, dri-devel This function provides a way for the driver to redo a modeset on the current mode and retry the link training at a lower link rate/lane count/bpp. This will get called incase the link training fails during the current modeset. Cc: dri-devel@lists.freedesktop.org Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Daniel Vetter <daniel.vetter@intel.com> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> --- drivers/gpu/drm/drm_atomic_helper.c | 58 +++++++++++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 1 + 2 files changed, 59 insertions(+) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index f936276..0c1614e 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2895,6 +2895,64 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector, EXPORT_SYMBOL(drm_atomic_helper_connector_dpms); /** + * drm_atomic_helper_connector_modeset - Force a modeset on a connector + * @connector: DRM connector + * + * Provides a way to redo a modeset with the current mode so that it can + * drop the bpp, link rate/lane count and retry the link training. + * + * Returns: + * Returns 0 on success, negative errno numbers on failure. + */ +int +drm_atomic_helper_connector_modeset(struct drm_connector *connector) +{ + struct drm_device *dev = connector->dev; + struct drm_modeset_acquire_ctx ctx; + struct drm_atomic_state *state; + struct drm_connector_state *connector_state; + struct drm_crtc_state *crtc_state; + int ret = 0; + + drm_modeset_acquire_init(&ctx, 0); + state = drm_atomic_state_alloc(dev); + if (!state) { + ret = -ENOMEM; + goto fail; + } + state->acquire_ctx = &ctx; +retry: + ret = 0; + connector_state = drm_atomic_get_connector_state(state, connector); + if (IS_ERR(connector_state)) { + ret = PTR_ERR(connector_state); + goto fail; + } + if (!connector_state->crtc) + goto fail; + + crtc_state = drm_atomic_get_existing_crtc_state(state, + connector_state->crtc); + crtc_state->connectors_changed = true; + ret = drm_atomic_commit(state); +fail: + if (ret == -EDEADLK) { + drm_atomic_state_clear(state); + drm_modeset_backoff(&ctx); + goto retry; + } + + if (state) + drm_atomic_state_put(state); + + drm_modeset_drop_locks(&ctx); + drm_modeset_acquire_fini(&ctx); + + return ret; +} +EXPORT_SYMBOL(drm_atomic_helper_connector_modeset); + +/** * drm_atomic_helper_best_encoder - Helper for &drm_connector_helper_funcs * ->best_encoder callback * @connector: Connector control structure diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 7ff92b0..8de24dc 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -126,6 +126,7 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc, uint32_t flags); int drm_atomic_helper_connector_dpms(struct drm_connector *connector, int mode); +int drm_atomic_helper_connector_modeset(struct drm_connector *connector); struct drm_encoder * drm_atomic_helper_best_encoder(struct drm_connector *connector); -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode 2016-10-21 23:45 ` [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode Manasi Navare @ 2016-10-22 8:47 ` Daniel Vetter 2016-10-22 14:01 ` [Intel-gfx] " Daniel Vetter 2016-10-25 12:09 ` Jani Nikula 1 sibling, 1 reply; 33+ messages in thread From: Daniel Vetter @ 2016-10-22 8:47 UTC (permalink / raw) To: Manasi Navare; +Cc: Daniel Vetter, intel-gfx, dri-devel On Fri, Oct 21, 2016 at 04:45:39PM -0700, Manasi Navare wrote: > This function provides a way for the driver to redo a > modeset on the current mode and retry the link training > at a lower link rate/lane count/bpp. This will get called > incase the link training fails during the current modeset. > > Cc: dri-devel@lists.freedesktop.org > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > drivers/gpu/drm/drm_atomic_helper.c | 58 +++++++++++++++++++++++++++++++++++++ > include/drm/drm_atomic_helper.h | 1 + > 2 files changed, 59 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index f936276..0c1614e 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -2895,6 +2895,64 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector, > EXPORT_SYMBOL(drm_atomic_helper_connector_dpms); > > /** > + * drm_atomic_helper_connector_modeset - Force a modeset on a connector > + * @connector: DRM connector > + * > + * Provides a way to redo a modeset with the current mode so that it can > + * drop the bpp, link rate/lane count and retry the link training. > + * > + * Returns: > + * Returns 0 on success, negative errno numbers on failure. > + */ > +int > +drm_atomic_helper_connector_modeset(struct drm_connector *connector) > +{ > + struct drm_device *dev = connector->dev; > + struct drm_modeset_acquire_ctx ctx; > + struct drm_atomic_state *state; > + struct drm_connector_state *connector_state; > + struct drm_crtc_state *crtc_state; > + int ret = 0; > + > + drm_modeset_acquire_init(&ctx, 0); > + state = drm_atomic_state_alloc(dev); > + if (!state) { > + ret = -ENOMEM; > + goto fail; > + } > + state->acquire_ctx = &ctx; > +retry: > + ret = 0; > + connector_state = drm_atomic_get_connector_state(state, connector); > + if (IS_ERR(connector_state)) { > + ret = PTR_ERR(connector_state); > + goto fail; > + } > + if (!connector_state->crtc) > + goto fail; > + > + crtc_state = drm_atomic_get_existing_crtc_state(state, > + connector_state->crtc); > + crtc_state->connectors_changed = true; This line here only works if the driver uses the helpers for checking and commit, and when it doesn't overwrite connectors_changed. I think the kerneldoc should mention this. The other issue: You cannot call this from modeset code itself because of recursion. I think this also must be mentioned. -Daniel > + ret = drm_atomic_commit(state); > +fail: > + if (ret == -EDEADLK) { > + drm_atomic_state_clear(state); > + drm_modeset_backoff(&ctx); > + goto retry; > + } > + > + if (state) > + drm_atomic_state_put(state); > + > + drm_modeset_drop_locks(&ctx); > + drm_modeset_acquire_fini(&ctx); > + > + return ret; > +} > +EXPORT_SYMBOL(drm_atomic_helper_connector_modeset); > + > +/** > * drm_atomic_helper_best_encoder - Helper for &drm_connector_helper_funcs > * ->best_encoder callback > * @connector: Connector control structure > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h > index 7ff92b0..8de24dc 100644 > --- a/include/drm/drm_atomic_helper.h > +++ b/include/drm/drm_atomic_helper.h > @@ -126,6 +126,7 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc, > uint32_t flags); > int drm_atomic_helper_connector_dpms(struct drm_connector *connector, > int mode); > +int drm_atomic_helper_connector_modeset(struct drm_connector *connector); > struct drm_encoder * > drm_atomic_helper_best_encoder(struct drm_connector *connector); > > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Intel-gfx] [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode 2016-10-22 8:47 ` Daniel Vetter @ 2016-10-22 14:01 ` Daniel Vetter 2016-10-22 14:46 ` Ville Syrjälä 0 siblings, 1 reply; 33+ messages in thread From: Daniel Vetter @ 2016-10-22 14:01 UTC (permalink / raw) To: Manasi Navare; +Cc: Daniel Vetter, intel-gfx, dri-devel On Sat, Oct 22, 2016 at 10:47:25AM +0200, Daniel Vetter wrote: > On Fri, Oct 21, 2016 at 04:45:39PM -0700, Manasi Navare wrote: > > This function provides a way for the driver to redo a > > modeset on the current mode and retry the link training > > at a lower link rate/lane count/bpp. This will get called > > incase the link training fails during the current modeset. > > > > Cc: dri-devel@lists.freedesktop.org > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > --- > > drivers/gpu/drm/drm_atomic_helper.c | 58 +++++++++++++++++++++++++++++++++++++ > > include/drm/drm_atomic_helper.h | 1 + > > 2 files changed, 59 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > index f936276..0c1614e 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -2895,6 +2895,64 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector, > > EXPORT_SYMBOL(drm_atomic_helper_connector_dpms); > > > > /** > > + * drm_atomic_helper_connector_modeset - Force a modeset on a connector > > + * @connector: DRM connector > > + * > > + * Provides a way to redo a modeset with the current mode so that it can > > + * drop the bpp, link rate/lane count and retry the link training. > > + * > > + * Returns: > > + * Returns 0 on success, negative errno numbers on failure. > > + */ > > +int > > +drm_atomic_helper_connector_modeset(struct drm_connector *connector) > > +{ > > + struct drm_device *dev = connector->dev; > > + struct drm_modeset_acquire_ctx ctx; > > + struct drm_atomic_state *state; > > + struct drm_connector_state *connector_state; > > + struct drm_crtc_state *crtc_state; > > + int ret = 0; > > + > > + drm_modeset_acquire_init(&ctx, 0); > > + state = drm_atomic_state_alloc(dev); > > + if (!state) { > > + ret = -ENOMEM; > > + goto fail; > > + } > > + state->acquire_ctx = &ctx; > > +retry: > > + ret = 0; > > + connector_state = drm_atomic_get_connector_state(state, connector); > > + if (IS_ERR(connector_state)) { > > + ret = PTR_ERR(connector_state); > > + goto fail; > > + } > > + if (!connector_state->crtc) > > + goto fail; > > + > > + crtc_state = drm_atomic_get_existing_crtc_state(state, > > + connector_state->crtc); > > + crtc_state->connectors_changed = true; > > This line here only works if the driver uses the helpers for checking and > commit, and when it doesn't overwrite connectors_changed. I think the > kerneldoc should mention this. > > The other issue: You cannot call this from modeset code itself because of > recursion. I think this also must be mentioned. And if this indeed does a modeset then we have again the same issue as with upfront link training when the pipe is supposed to be on: Userspace can observe the kernel playing tricks because the vblank support will be temporarily disabled. Not sure how to best fix this, but might be good to grab the crtc->mutex in the vblank code for stuff like this. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode 2016-10-22 14:01 ` [Intel-gfx] " Daniel Vetter @ 2016-10-22 14:46 ` Ville Syrjälä 2016-10-24 6:00 ` Daniel Vetter 0 siblings, 1 reply; 33+ messages in thread From: Ville Syrjälä @ 2016-10-22 14:46 UTC (permalink / raw) To: Daniel Vetter; +Cc: Daniel Vetter, intel-gfx, dri-devel On Sat, Oct 22, 2016 at 04:01:14PM +0200, Daniel Vetter wrote: > On Sat, Oct 22, 2016 at 10:47:25AM +0200, Daniel Vetter wrote: > > On Fri, Oct 21, 2016 at 04:45:39PM -0700, Manasi Navare wrote: > > > This function provides a way for the driver to redo a > > > modeset on the current mode and retry the link training > > > at a lower link rate/lane count/bpp. This will get called > > > incase the link training fails during the current modeset. > > > > > > Cc: dri-devel@lists.freedesktop.org > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > --- > > > drivers/gpu/drm/drm_atomic_helper.c | 58 +++++++++++++++++++++++++++++++++++++ > > > include/drm/drm_atomic_helper.h | 1 + > > > 2 files changed, 59 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > > index f936276..0c1614e 100644 > > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > > @@ -2895,6 +2895,64 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector, > > > EXPORT_SYMBOL(drm_atomic_helper_connector_dpms); > > > > > > /** > > > + * drm_atomic_helper_connector_modeset - Force a modeset on a connector > > > + * @connector: DRM connector > > > + * > > > + * Provides a way to redo a modeset with the current mode so that it can > > > + * drop the bpp, link rate/lane count and retry the link training. > > > + * > > > + * Returns: > > > + * Returns 0 on success, negative errno numbers on failure. > > > + */ > > > +int > > > +drm_atomic_helper_connector_modeset(struct drm_connector *connector) > > > +{ > > > + struct drm_device *dev = connector->dev; > > > + struct drm_modeset_acquire_ctx ctx; > > > + struct drm_atomic_state *state; > > > + struct drm_connector_state *connector_state; > > > + struct drm_crtc_state *crtc_state; > > > + int ret = 0; > > > + > > > + drm_modeset_acquire_init(&ctx, 0); > > > + state = drm_atomic_state_alloc(dev); > > > + if (!state) { > > > + ret = -ENOMEM; > > > + goto fail; > > > + } > > > + state->acquire_ctx = &ctx; > > > +retry: > > > + ret = 0; > > > + connector_state = drm_atomic_get_connector_state(state, connector); > > > + if (IS_ERR(connector_state)) { > > > + ret = PTR_ERR(connector_state); > > > + goto fail; > > > + } > > > + if (!connector_state->crtc) > > > + goto fail; > > > + > > > + crtc_state = drm_atomic_get_existing_crtc_state(state, > > > + connector_state->crtc); > > > + crtc_state->connectors_changed = true; > > > > This line here only works if the driver uses the helpers for checking and > > commit, and when it doesn't overwrite connectors_changed. I think the > > kerneldoc should mention this. > > > > The other issue: You cannot call this from modeset code itself because of > > recursion. I think this also must be mentioned. > > And if this indeed does a modeset then we have again the same issue as > with upfront link training when the pipe is supposed to be on: Userspace > can observe the kernel playing tricks because the vblank support will be > temporarily disabled. > > Not sure how to best fix this, but might be good to grab the crtc->mutex > in the vblank code for stuff like this. We're going to have the same problem with async modeset as well. -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode 2016-10-22 14:46 ` Ville Syrjälä @ 2016-10-24 6:00 ` Daniel Vetter 2016-10-24 6:12 ` Manasi Navare 0 siblings, 1 reply; 33+ messages in thread From: Daniel Vetter @ 2016-10-24 6:00 UTC (permalink / raw) To: Ville Syrjälä; +Cc: Daniel Vetter, intel-gfx, dri-devel On Sat, Oct 22, 2016 at 05:46:30PM +0300, Ville Syrjälä wrote: > On Sat, Oct 22, 2016 at 04:01:14PM +0200, Daniel Vetter wrote: > > On Sat, Oct 22, 2016 at 10:47:25AM +0200, Daniel Vetter wrote: > > > On Fri, Oct 21, 2016 at 04:45:39PM -0700, Manasi Navare wrote: > > > > This function provides a way for the driver to redo a > > > > modeset on the current mode and retry the link training > > > > at a lower link rate/lane count/bpp. This will get called > > > > incase the link training fails during the current modeset. > > > > > > > > Cc: dri-devel@lists.freedesktop.org > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > > --- > > > > drivers/gpu/drm/drm_atomic_helper.c | 58 +++++++++++++++++++++++++++++++++++++ > > > > include/drm/drm_atomic_helper.h | 1 + > > > > 2 files changed, 59 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > > > index f936276..0c1614e 100644 > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > > > @@ -2895,6 +2895,64 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector, > > > > EXPORT_SYMBOL(drm_atomic_helper_connector_dpms); > > > > > > > > /** > > > > + * drm_atomic_helper_connector_modeset - Force a modeset on a connector > > > > + * @connector: DRM connector > > > > + * > > > > + * Provides a way to redo a modeset with the current mode so that it can > > > > + * drop the bpp, link rate/lane count and retry the link training. > > > > + * > > > > + * Returns: > > > > + * Returns 0 on success, negative errno numbers on failure. > > > > + */ > > > > +int > > > > +drm_atomic_helper_connector_modeset(struct drm_connector *connector) > > > > +{ > > > > + struct drm_device *dev = connector->dev; > > > > + struct drm_modeset_acquire_ctx ctx; > > > > + struct drm_atomic_state *state; > > > > + struct drm_connector_state *connector_state; > > > > + struct drm_crtc_state *crtc_state; > > > > + int ret = 0; > > > > + > > > > + drm_modeset_acquire_init(&ctx, 0); > > > > + state = drm_atomic_state_alloc(dev); > > > > + if (!state) { > > > > + ret = -ENOMEM; > > > > + goto fail; > > > > + } > > > > + state->acquire_ctx = &ctx; > > > > +retry: > > > > + ret = 0; > > > > + connector_state = drm_atomic_get_connector_state(state, connector); > > > > + if (IS_ERR(connector_state)) { > > > > + ret = PTR_ERR(connector_state); > > > > + goto fail; > > > > + } > > > > + if (!connector_state->crtc) > > > > + goto fail; > > > > + > > > > + crtc_state = drm_atomic_get_existing_crtc_state(state, > > > > + connector_state->crtc); > > > > + crtc_state->connectors_changed = true; > > > > > > This line here only works if the driver uses the helpers for checking and > > > commit, and when it doesn't overwrite connectors_changed. I think the > > > kerneldoc should mention this. > > > > > > The other issue: You cannot call this from modeset code itself because of > > > recursion. I think this also must be mentioned. > > > > And if this indeed does a modeset then we have again the same issue as > > with upfront link training when the pipe is supposed to be on: Userspace > > can observe the kernel playing tricks because the vblank support will be > > temporarily disabled. > > > > Not sure how to best fix this, but might be good to grab the crtc->mutex > > in the vblank code for stuff like this. > > We're going to have the same problem with async modeset as well. Async modeset is ok because userspace expects that the pipe will go off/on, and the kernel will send out an event to signal when the pipe is guaranteed to be on and can be started to be used. It's random modesets afterwards I'm concerned about. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode 2016-10-24 6:00 ` Daniel Vetter @ 2016-10-24 6:12 ` Manasi Navare 2016-10-24 6:33 ` Daniel Vetter 0 siblings, 1 reply; 33+ messages in thread From: Manasi Navare @ 2016-10-24 6:12 UTC (permalink / raw) To: Daniel Vetter; +Cc: Daniel Vetter, intel-gfx, dri-devel On Mon, Oct 24, 2016 at 08:00:10AM +0200, Daniel Vetter wrote: > On Sat, Oct 22, 2016 at 05:46:30PM +0300, Ville Syrjälä wrote: > > On Sat, Oct 22, 2016 at 04:01:14PM +0200, Daniel Vetter wrote: > > > On Sat, Oct 22, 2016 at 10:47:25AM +0200, Daniel Vetter wrote: > > > > On Fri, Oct 21, 2016 at 04:45:39PM -0700, Manasi Navare wrote: > > > > > This function provides a way for the driver to redo a > > > > > modeset on the current mode and retry the link training > > > > > at a lower link rate/lane count/bpp. This will get called > > > > > incase the link training fails during the current modeset. > > > > > > > > > > Cc: dri-devel@lists.freedesktop.org > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > > > --- > > > > > drivers/gpu/drm/drm_atomic_helper.c | 58 +++++++++++++++++++++++++++++++++++++ > > > > > include/drm/drm_atomic_helper.h | 1 + > > > > > 2 files changed, 59 insertions(+) > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > > > > index f936276..0c1614e 100644 > > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > > > > @@ -2895,6 +2895,64 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector, > > > > > EXPORT_SYMBOL(drm_atomic_helper_connector_dpms); > > > > > > > > > > /** > > > > > + * drm_atomic_helper_connector_modeset - Force a modeset on a connector > > > > > + * @connector: DRM connector > > > > > + * > > > > > + * Provides a way to redo a modeset with the current mode so that it can > > > > > + * drop the bpp, link rate/lane count and retry the link training. > > > > > + * > > > > > + * Returns: > > > > > + * Returns 0 on success, negative errno numbers on failure. > > > > > + */ > > > > > +int > > > > > +drm_atomic_helper_connector_modeset(struct drm_connector *connector) > > > > > +{ > > > > > + struct drm_device *dev = connector->dev; > > > > > + struct drm_modeset_acquire_ctx ctx; > > > > > + struct drm_atomic_state *state; > > > > > + struct drm_connector_state *connector_state; > > > > > + struct drm_crtc_state *crtc_state; > > > > > + int ret = 0; > > > > > + > > > > > + drm_modeset_acquire_init(&ctx, 0); > > > > > + state = drm_atomic_state_alloc(dev); > > > > > + if (!state) { > > > > > + ret = -ENOMEM; > > > > > + goto fail; > > > > > + } > > > > > + state->acquire_ctx = &ctx; > > > > > +retry: > > > > > + ret = 0; > > > > > + connector_state = drm_atomic_get_connector_state(state, connector); > > > > > + if (IS_ERR(connector_state)) { > > > > > + ret = PTR_ERR(connector_state); > > > > > + goto fail; > > > > > + } > > > > > + if (!connector_state->crtc) > > > > > + goto fail; > > > > > + > > > > > + crtc_state = drm_atomic_get_existing_crtc_state(state, > > > > > + connector_state->crtc); > > > > > + crtc_state->connectors_changed = true; > > > > > > > > This line here only works if the driver uses the helpers for checking and > > > > commit, and when it doesn't overwrite connectors_changed. I think the > > > > kerneldoc should mention this. > > > > > > > > The other issue: You cannot call this from modeset code itself because of > > > > recursion. I think this also must be mentioned. I will add this to the kernel doc. In our case, we call this in a work func that will get scheduled after the current modeset is completed. > > > > > > And if this indeed does a modeset then we have again the same issue as > > > with upfront link training when the pipe is supposed to be on: Userspace > > > can observe the kernel playing tricks because the vblank support will be > > > temporarily disabled. > > > > > > Not sure how to best fix this, but might be good to grab the crtc->mutex > > > in the vblank code for stuff like this. > > > > We're going to have the same problem with async modeset as well. > > Async modeset is ok because userspace expects that the pipe will go > off/on, and the kernel will send out an event to signal when the pipe is > guaranteed to be on and can be started to be used. It's random modesets > afterwards I'm concerned about. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch These wont be random modesets, this will occur only in case of link training failure in which case if the mode has not changed/pruned, it will attempt modeset at lower link rate. Manasi _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode 2016-10-24 6:12 ` Manasi Navare @ 2016-10-24 6:33 ` Daniel Vetter 2016-10-24 7:00 ` Manasi Navare 0 siblings, 1 reply; 33+ messages in thread From: Daniel Vetter @ 2016-10-24 6:33 UTC (permalink / raw) To: Manasi Navare; +Cc: dri-devel, Daniel Vetter, intel-gfx On Sun, Oct 23, 2016 at 11:12:31PM -0700, Manasi Navare wrote: > On Mon, Oct 24, 2016 at 08:00:10AM +0200, Daniel Vetter wrote: > > On Sat, Oct 22, 2016 at 05:46:30PM +0300, Ville Syrjälä wrote: > > > On Sat, Oct 22, 2016 at 04:01:14PM +0200, Daniel Vetter wrote: > > > > On Sat, Oct 22, 2016 at 10:47:25AM +0200, Daniel Vetter wrote: > > > > > On Fri, Oct 21, 2016 at 04:45:39PM -0700, Manasi Navare wrote: > > > > > > This function provides a way for the driver to redo a > > > > > > modeset on the current mode and retry the link training > > > > > > at a lower link rate/lane count/bpp. This will get called > > > > > > incase the link training fails during the current modeset. > > > > > > > > > > > > Cc: dri-devel@lists.freedesktop.org > > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > > > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > > > > --- > > > > > > drivers/gpu/drm/drm_atomic_helper.c | 58 +++++++++++++++++++++++++++++++++++++ > > > > > > include/drm/drm_atomic_helper.h | 1 + > > > > > > 2 files changed, 59 insertions(+) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > > > > > index f936276..0c1614e 100644 > > > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > > > > > @@ -2895,6 +2895,64 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector, > > > > > > EXPORT_SYMBOL(drm_atomic_helper_connector_dpms); > > > > > > > > > > > > /** > > > > > > + * drm_atomic_helper_connector_modeset - Force a modeset on a connector > > > > > > + * @connector: DRM connector > > > > > > + * > > > > > > + * Provides a way to redo a modeset with the current mode so that it can > > > > > > + * drop the bpp, link rate/lane count and retry the link training. > > > > > > + * > > > > > > + * Returns: > > > > > > + * Returns 0 on success, negative errno numbers on failure. > > > > > > + */ > > > > > > +int > > > > > > +drm_atomic_helper_connector_modeset(struct drm_connector *connector) > > > > > > +{ > > > > > > + struct drm_device *dev = connector->dev; > > > > > > + struct drm_modeset_acquire_ctx ctx; > > > > > > + struct drm_atomic_state *state; > > > > > > + struct drm_connector_state *connector_state; > > > > > > + struct drm_crtc_state *crtc_state; > > > > > > + int ret = 0; > > > > > > + > > > > > > + drm_modeset_acquire_init(&ctx, 0); > > > > > > + state = drm_atomic_state_alloc(dev); > > > > > > + if (!state) { > > > > > > + ret = -ENOMEM; > > > > > > + goto fail; > > > > > > + } > > > > > > + state->acquire_ctx = &ctx; > > > > > > +retry: > > > > > > + ret = 0; > > > > > > + connector_state = drm_atomic_get_connector_state(state, connector); > > > > > > + if (IS_ERR(connector_state)) { > > > > > > + ret = PTR_ERR(connector_state); > > > > > > + goto fail; > > > > > > + } > > > > > > + if (!connector_state->crtc) > > > > > > + goto fail; > > > > > > + > > > > > > + crtc_state = drm_atomic_get_existing_crtc_state(state, > > > > > > + connector_state->crtc); > > > > > > + crtc_state->connectors_changed = true; > > > > > > > > > > This line here only works if the driver uses the helpers for checking and > > > > > commit, and when it doesn't overwrite connectors_changed. I think the > > > > > kerneldoc should mention this. > > > > > > > > > > The other issue: You cannot call this from modeset code itself because of > > > > > recursion. I think this also must be mentioned. > > I will add this to the kernel doc. > In our case, we call this in a work func that will get scheduled after > the current modeset is completed. > > > > > > > > > And if this indeed does a modeset then we have again the same issue as > > > > with upfront link training when the pipe is supposed to be on: Userspace > > > > can observe the kernel playing tricks because the vblank support will be > > > > temporarily disabled. > > > > > > > > Not sure how to best fix this, but might be good to grab the crtc->mutex > > > > in the vblank code for stuff like this. > > > > > > We're going to have the same problem with async modeset as well. > > > > Async modeset is ok because userspace expects that the pipe will go > > off/on, and the kernel will send out an event to signal when the pipe is > > guaranteed to be on and can be started to be used. It's random modesets > > afterwards I'm concerned about. > > -Daniel > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > > These wont be random modesets, this will occur only in case of link training > failure in which case if the mode has not changed/pruned, it will attempt > modeset at lower link rate. "Cat ate the cable" is very much random from userspace's pov. It's not random from the kernel's pov, because the kernel is aware of what's going on - it just tried to (re)train the link. But userspace has no idea at all. Note that link training failures can not just happen right after a modeset, but also: - anytime the sink feels like the link is bad and asks us to retrain (yay, same issue there with having to stop the pipe). - system suspend/resume might also result in link train fail (the screen might not even be there any more), but the link should still keep running. I guess we just need to do some additional work on top to make sure the vblank ioctl can't see invalid state. Which would then again make upfront link trainig possible ... -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode 2016-10-24 6:33 ` Daniel Vetter @ 2016-10-24 7:00 ` Manasi Navare 2016-10-24 7:12 ` Daniel Vetter 0 siblings, 1 reply; 33+ messages in thread From: Manasi Navare @ 2016-10-24 7:00 UTC (permalink / raw) To: Daniel Vetter; +Cc: Daniel Vetter, intel-gfx, dri-devel On Mon, Oct 24, 2016 at 08:33:10AM +0200, Daniel Vetter wrote: > On Sun, Oct 23, 2016 at 11:12:31PM -0700, Manasi Navare wrote: > > On Mon, Oct 24, 2016 at 08:00:10AM +0200, Daniel Vetter wrote: > > > On Sat, Oct 22, 2016 at 05:46:30PM +0300, Ville Syrjälä wrote: > > > > On Sat, Oct 22, 2016 at 04:01:14PM +0200, Daniel Vetter wrote: > > > > > On Sat, Oct 22, 2016 at 10:47:25AM +0200, Daniel Vetter wrote: > > > > > > On Fri, Oct 21, 2016 at 04:45:39PM -0700, Manasi Navare wrote: > > > > > > > This function provides a way for the driver to redo a > > > > > > > modeset on the current mode and retry the link training > > > > > > > at a lower link rate/lane count/bpp. This will get called > > > > > > > incase the link training fails during the current modeset. > > > > > > > > > > > > > > Cc: dri-devel@lists.freedesktop.org > > > > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > > > > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > > > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > > > > > --- > > > > > > > drivers/gpu/drm/drm_atomic_helper.c | 58 +++++++++++++++++++++++++++++++++++++ > > > > > > > include/drm/drm_atomic_helper.h | 1 + > > > > > > > 2 files changed, 59 insertions(+) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > > > > > > index f936276..0c1614e 100644 > > > > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > > > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > > > > > > @@ -2895,6 +2895,64 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector, > > > > > > > EXPORT_SYMBOL(drm_atomic_helper_connector_dpms); > > > > > > > > > > > > > > /** > > > > > > > + * drm_atomic_helper_connector_modeset - Force a modeset on a connector > > > > > > > + * @connector: DRM connector > > > > > > > + * > > > > > > > + * Provides a way to redo a modeset with the current mode so that it can > > > > > > > + * drop the bpp, link rate/lane count and retry the link training. > > > > > > > + * > > > > > > > + * Returns: > > > > > > > + * Returns 0 on success, negative errno numbers on failure. > > > > > > > + */ > > > > > > > +int > > > > > > > +drm_atomic_helper_connector_modeset(struct drm_connector *connector) > > > > > > > +{ > > > > > > > + struct drm_device *dev = connector->dev; > > > > > > > + struct drm_modeset_acquire_ctx ctx; > > > > > > > + struct drm_atomic_state *state; > > > > > > > + struct drm_connector_state *connector_state; > > > > > > > + struct drm_crtc_state *crtc_state; > > > > > > > + int ret = 0; > > > > > > > + > > > > > > > + drm_modeset_acquire_init(&ctx, 0); > > > > > > > + state = drm_atomic_state_alloc(dev); > > > > > > > + if (!state) { > > > > > > > + ret = -ENOMEM; > > > > > > > + goto fail; > > > > > > > + } > > > > > > > + state->acquire_ctx = &ctx; > > > > > > > +retry: > > > > > > > + ret = 0; > > > > > > > + connector_state = drm_atomic_get_connector_state(state, connector); > > > > > > > + if (IS_ERR(connector_state)) { > > > > > > > + ret = PTR_ERR(connector_state); > > > > > > > + goto fail; > > > > > > > + } > > > > > > > + if (!connector_state->crtc) > > > > > > > + goto fail; > > > > > > > + > > > > > > > + crtc_state = drm_atomic_get_existing_crtc_state(state, > > > > > > > + connector_state->crtc); > > > > > > > + crtc_state->connectors_changed = true; > > > > > > > > > > > > This line here only works if the driver uses the helpers for checking and > > > > > > commit, and when it doesn't overwrite connectors_changed. I think the > > > > > > kerneldoc should mention this. > > > > > > > > > > > > The other issue: You cannot call this from modeset code itself because of > > > > > > recursion. I think this also must be mentioned. > > > > I will add this to the kernel doc. > > In our case, we call this in a work func that will get scheduled after > > the current modeset is completed. > > > > > > > > > > > > And if this indeed does a modeset then we have again the same issue as > > > > > with upfront link training when the pipe is supposed to be on: Userspace > > > > > can observe the kernel playing tricks because the vblank support will be > > > > > temporarily disabled. > > > > > > > > > > Not sure how to best fix this, but might be good to grab the crtc->mutex > > > > > in the vblank code for stuff like this. > > > > > > > > We're going to have the same problem with async modeset as well. > > > > > > Async modeset is ok because userspace expects that the pipe will go > > > off/on, and the kernel will send out an event to signal when the pipe is > > > guaranteed to be on and can be started to be used. It's random modesets > > > afterwards I'm concerned about. > > > -Daniel > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch > > > > These wont be random modesets, this will occur only in case of link training > > failure in which case if the mode has not changed/pruned, it will attempt > > modeset at lower link rate. > > "Cat ate the cable" is very much random from userspace's pov. It's not > random from the kernel's pov, because the kernel is aware of what's going > on - it just tried to (re)train the link. But userspace has no idea at > all. Note that link training failures can not just happen right after a > modeset, but also: > - anytime the sink feels like the link is bad and asks us to retrain (yay, > same issue there with having to stop the pipe). > - system suspend/resume might also result in link train fail (the screen > might not even be there any more), but the link should still keep > running. > > I guess we just need to do some additional work on top to make sure the > vblank ioctl can't see invalid state. Which would then again make upfront > link trainig possible ... > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch Could you share some more details on what work would need to be done for vblank? Then I can investigate it a little bit more tomor during the day. Manasi _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode 2016-10-24 7:00 ` Manasi Navare @ 2016-10-24 7:12 ` Daniel Vetter 2016-10-24 18:38 ` [Intel-gfx] " Sean Paul 2016-10-24 22:08 ` Manasi Navare 0 siblings, 2 replies; 33+ messages in thread From: Daniel Vetter @ 2016-10-24 7:12 UTC (permalink / raw) To: Manasi Navare; +Cc: Daniel Vetter, intel-gfx, dri-devel On Mon, Oct 24, 2016 at 9:00 AM, Manasi Navare <manasi.d.navare@intel.com> wrote: >> I guess we just need to do some additional work on top to make sure the >> vblank ioctl can't see invalid state. Which would then again make upfront >> link trainig possible ... > > Could you share some more details on what work would need to be done > for vblank? Then I can investigate it a little bit more tomor during the day. So the trouble is that drm_irq.c is still from the old pre-kms days. Which means it deals in int pipe instead of struct drm_crtc *, among other things. But we can't simply switch over because some old drivers (pre-kms ones) still depend upon that old code. Hence we need: 1. Duplicate most of the code in drm_irq.c into a new drm_crtc_vblank.c, which is only used for kms drivers. And in the ioctl code have a DRIVER_MODESET check and either call the new kms version (in drm_crtc_vblank.c) or the old one (still in drm_irq.c). 2. Convert the int pipe into a drm_crtc pointer in the new kms code. For prettiness we could (try) to do that as much as possible. 3. Grab the drm_crtc->mutex in the ioctl code to block these races. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Intel-gfx] [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode 2016-10-24 7:12 ` Daniel Vetter @ 2016-10-24 18:38 ` Sean Paul 2016-10-25 6:35 ` Daniel Vetter 2016-10-24 22:08 ` Manasi Navare 1 sibling, 1 reply; 33+ messages in thread From: Sean Paul @ 2016-10-24 18:38 UTC (permalink / raw) To: Daniel Vetter; +Cc: Manasi Navare, Daniel Vetter, intel-gfx, dri-devel On Mon, Oct 24, 2016 at 3:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Oct 24, 2016 at 9:00 AM, Manasi Navare > <manasi.d.navare@intel.com> wrote: >>> I guess we just need to do some additional work on top to make sure the >>> vblank ioctl can't see invalid state. Which would then again make upfront >>> link trainig possible ... >> >> Could you share some more details on what work would need to be done >> for vblank? Then I can investigate it a little bit more tomor during the day. > > So the trouble is that drm_irq.c is still from the old pre-kms days. > Which means it deals in int pipe instead of struct drm_crtc *, among > other things. But we can't simply switch over because some old drivers > (pre-kms ones) still depend upon that old code. Hence we need: > > 1. Duplicate most of the code in drm_irq.c into a new > drm_crtc_vblank.c, which is only used for kms drivers. Why duplicate all of this code? That seems a bit wasteful. Can't we just add the inverse of drm_crtc_pipe() (coincidentally, I also suggested we introduce this function in "drm: zte: add initial vou drm driver"), and use those to map between pipe and crtc where appropriate? The crtc locks could be acquired/dropped based on DRIVER_MODESET in drm_irq.c Unrelated to the vblank discussion: This functionality is pretty similar to what happens on suspend/resume. Any chance we can share? Sean > And in the > ioctl code have a DRIVER_MODESET check and either call the new kms > version (in drm_crtc_vblank.c) or the old one (still in drm_irq.c). > > 2. Convert the int pipe into a drm_crtc pointer in the new kms code. > For prettiness we could (try) to do that as much as possible. > > 3. Grab the drm_crtc->mutex in the ioctl code to block these races. > > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode 2016-10-24 18:38 ` [Intel-gfx] " Sean Paul @ 2016-10-25 6:35 ` Daniel Vetter 0 siblings, 0 replies; 33+ messages in thread From: Daniel Vetter @ 2016-10-25 6:35 UTC (permalink / raw) To: Sean Paul; +Cc: Daniel Vetter, intel-gfx, dri-devel On Mon, Oct 24, 2016 at 02:38:17PM -0400, Sean Paul wrote: > On Mon, Oct 24, 2016 at 3:12 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Mon, Oct 24, 2016 at 9:00 AM, Manasi Navare > > <manasi.d.navare@intel.com> wrote: > >>> I guess we just need to do some additional work on top to make sure the > >>> vblank ioctl can't see invalid state. Which would then again make upfront > >>> link trainig possible ... > >> > >> Could you share some more details on what work would need to be done > >> for vblank? Then I can investigate it a little bit more tomor during the day. > > > > So the trouble is that drm_irq.c is still from the old pre-kms days. > > Which means it deals in int pipe instead of struct drm_crtc *, among > > other things. But we can't simply switch over because some old drivers > > (pre-kms ones) still depend upon that old code. Hence we need: > > > > 1. Duplicate most of the code in drm_irq.c into a new > > drm_crtc_vblank.c, which is only used for kms drivers. > > Why duplicate all of this code? That seems a bit wasteful. > > Can't we just add the inverse of drm_crtc_pipe() (coincidentally, I > also suggested we introduce this function in "drm: zte: add initial > vou drm driver"), and use those to map between pipe and crtc where > appropriate? The crtc locks could be acquired/dropped based on > DRIVER_MODESET in drm_irq.c Only if you have a DRIVER_MODESET. For DRIVER_LEGACY this code all still needs to work, without there ever being a drm_crtc. If you try to squeeze kms and non-kms paths into one function that will be seriously ugly, and a complete maintenance pain. Copypasting is imo the sound approach here. That way we can let the old non-kms code rot unchanged until it's really dead. > Unrelated to the vblank discussion: This functionality is pretty > similar to what happens on suspend/resume. Any chance we can share? Hm, what do you mean? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode 2016-10-24 7:12 ` Daniel Vetter 2016-10-24 18:38 ` [Intel-gfx] " Sean Paul @ 2016-10-24 22:08 ` Manasi Navare 2016-10-25 6:40 ` [Intel-gfx] " Daniel Vetter 1 sibling, 1 reply; 33+ messages in thread From: Manasi Navare @ 2016-10-24 22:08 UTC (permalink / raw) To: Daniel Vetter; +Cc: Daniel Vetter, intel-gfx, dri-devel On Mon, Oct 24, 2016 at 09:12:35AM +0200, Daniel Vetter wrote: > On Mon, Oct 24, 2016 at 9:00 AM, Manasi Navare > <manasi.d.navare@intel.com> wrote: > >> I guess we just need to do some additional work on top to make sure the > >> vblank ioctl can't see invalid state. Which would then again make upfront > >> link trainig possible ... Thanks for the explanation. So the atomic_commit code in the driver calls drm_crtc_send_vblank_event(crtc, crtc->state->event);, is this what needs to be filtered to be seen by Vblank IOCTL? > > > > Could you share some more details on what work would need to be done > > for vblank? Then I can investigate it a little bit more tomor during the day. > > So the trouble is that drm_irq.c is still from the old pre-kms days. > Which means it deals in int pipe instead of struct drm_crtc *, among > other things. But we can't simply switch over because some old drivers > (pre-kms ones) still depend upon that old code. Hence we need: > > 1. Duplicate most of the code in drm_irq.c into a new > drm_crtc_vblank.c, which is only used for kms drivers. And in the > ioctl code have a DRIVER_MODESET check and either call the new kms > version (in drm_crtc_vblank.c) or the old one (still in drm_irq.c). What functions need to be duplicated? Which IOCTL are we talking about here? Do you mean in the atomic_commit_tail where we check for driver modeset, is where we should then call the new kms version of drm_crtc_send_vblank_event()? > > 2. Convert the int pipe into a drm_crtc pointer in the new kms code. > For prettiness we could (try) to do that as much as possible. > > 3. Grab the drm_crtc->mutex in the ioctl code to block these races. Again which IOCTL needs to grab the drm_crtc->mutex? What is the end goal of thsi task, what race conditions are we trying to avoid in case of these modesets during link training failures? Manasi > > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Intel-gfx] [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode 2016-10-24 22:08 ` Manasi Navare @ 2016-10-25 6:40 ` Daniel Vetter 0 siblings, 0 replies; 33+ messages in thread From: Daniel Vetter @ 2016-10-25 6:40 UTC (permalink / raw) To: Manasi Navare; +Cc: dri-devel, Daniel Vetter, intel-gfx On Mon, Oct 24, 2016 at 03:08:48PM -0700, Manasi Navare wrote: > On Mon, Oct 24, 2016 at 09:12:35AM +0200, Daniel Vetter wrote: > > On Mon, Oct 24, 2016 at 9:00 AM, Manasi Navare > > <manasi.d.navare@intel.com> wrote: > > >> I guess we just need to do some additional work on top to make sure the > > >> vblank ioctl can't see invalid state. Which would then again make upfront > > >> link trainig possible ... > > Thanks for the explanation. So the atomic_commit code in the driver > calls drm_crtc_send_vblank_event(crtc, crtc->state->event);, is this > what needs to be filtered to be seen by Vblank IOCTL? > > > > > > > > Could you share some more details on what work would need to be done > > > for vblank? Then I can investigate it a little bit more tomor during the day. > > > > So the trouble is that drm_irq.c is still from the old pre-kms days. > > Which means it deals in int pipe instead of struct drm_crtc *, among > > other things. But we can't simply switch over because some old drivers > > (pre-kms ones) still depend upon that old code. Hence we need: > > > > 1. Duplicate most of the code in drm_irq.c into a new > > drm_crtc_vblank.c, which is only used for kms drivers. And in the > > ioctl code have a DRIVER_MODESET check and either call the new kms > > version (in drm_crtc_vblank.c) or the old one (still in drm_irq.c). > > What functions need to be duplicated? Figuring out the exact list is way this is painful. Since we don't need everything, but just enough to make the new drm_crtc_vblank.c work. And then as a second step we probably should move all the functions starting with drm_crtc_vblank_ which are exported to drivers over to that file too. > Which IOCTL are we talking about here? Do you mean in the atomic_commit_tail > where we check for driver modeset, is where we should then call the new kms version > of drm_crtc_send_vblank_event()? > > > > 2. Convert the int pipe into a drm_crtc pointer in the new kms code. > > For prettiness we could (try) to do that as much as possible. > > > > 3. Grab the drm_crtc->mutex in the ioctl code to block these races. > > Again which IOCTL needs to grab the drm_crtc->mutex? The vblank ioctl in drm_irq.c, implemented by drm_wait_vblank. > What is the end goal of thsi task, what race conditions are we trying to avoid > in case of these modesets during link training failures? Modeset code calls drm_crtc_vblank_on/off when doing a full modeset. That changes the vblank status, which can be observed through the vblank ioctl by userspace: In between the calls to _off/_on it will return -EINVAL, instead of the last vblank timestamp (for a query) or doing the vblank wait (otherwise), which is what it should do. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode 2016-10-21 23:45 ` [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode Manasi Navare 2016-10-22 8:47 ` Daniel Vetter @ 2016-10-25 12:09 ` Jani Nikula 2016-10-25 22:28 ` Manasi Navare 2016-10-25 22:38 ` Rodrigo Vivi 1 sibling, 2 replies; 33+ messages in thread From: Jani Nikula @ 2016-10-25 12:09 UTC (permalink / raw) To: Manasi Navare, intel-gfx; +Cc: Daniel Vetter, dri-devel On Sat, 22 Oct 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > This function provides a way for the driver to redo a > modeset on the current mode and retry the link training > at a lower link rate/lane count/bpp. This will get called > incase the link training fails during the current modeset. Based on discussions on #intel-gfx, I would dodge all the problems here by having the userspace do the modeset. If we add a connector property to indicate the link status (and this does not have to be DP or link training specific, really), we can set that to "bad", and fire off the hotplug uevent. Then userspace has the information to force a modeset on that connector even if the mode has not changed. (Credits to Ville for the idea.) If we find a solution later on that allows us to handle all of this in kernel, we can do so, and remove the property or always report "good". Drivers can choose different approaches, depending on the capabilities of the hardware, for instance. Deferring this to the userspace does not regress anything now, because currently we just completely fail and end up with a black screen. The property would allow an enlightened userspace to fix that. And userspace can't rely on the property being there, as it's currently not there. BR, Jani. > > Cc: dri-devel@lists.freedesktop.org > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > drivers/gpu/drm/drm_atomic_helper.c | 58 +++++++++++++++++++++++++++++++++++++ > include/drm/drm_atomic_helper.h | 1 + > 2 files changed, 59 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index f936276..0c1614e 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -2895,6 +2895,64 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector, > EXPORT_SYMBOL(drm_atomic_helper_connector_dpms); > > /** > + * drm_atomic_helper_connector_modeset - Force a modeset on a connector > + * @connector: DRM connector > + * > + * Provides a way to redo a modeset with the current mode so that it can > + * drop the bpp, link rate/lane count and retry the link training. > + * > + * Returns: > + * Returns 0 on success, negative errno numbers on failure. > + */ > +int > +drm_atomic_helper_connector_modeset(struct drm_connector *connector) > +{ > + struct drm_device *dev = connector->dev; > + struct drm_modeset_acquire_ctx ctx; > + struct drm_atomic_state *state; > + struct drm_connector_state *connector_state; > + struct drm_crtc_state *crtc_state; > + int ret = 0; > + > + drm_modeset_acquire_init(&ctx, 0); > + state = drm_atomic_state_alloc(dev); > + if (!state) { > + ret = -ENOMEM; > + goto fail; > + } > + state->acquire_ctx = &ctx; > +retry: > + ret = 0; > + connector_state = drm_atomic_get_connector_state(state, connector); > + if (IS_ERR(connector_state)) { > + ret = PTR_ERR(connector_state); > + goto fail; > + } > + if (!connector_state->crtc) > + goto fail; > + > + crtc_state = drm_atomic_get_existing_crtc_state(state, > + connector_state->crtc); > + crtc_state->connectors_changed = true; > + ret = drm_atomic_commit(state); > +fail: > + if (ret == -EDEADLK) { > + drm_atomic_state_clear(state); > + drm_modeset_backoff(&ctx); > + goto retry; > + } > + > + if (state) > + drm_atomic_state_put(state); > + > + drm_modeset_drop_locks(&ctx); > + drm_modeset_acquire_fini(&ctx); > + > + return ret; > +} > +EXPORT_SYMBOL(drm_atomic_helper_connector_modeset); > + > +/** > * drm_atomic_helper_best_encoder - Helper for &drm_connector_helper_funcs > * ->best_encoder callback > * @connector: Connector control structure > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h > index 7ff92b0..8de24dc 100644 > --- a/include/drm/drm_atomic_helper.h > +++ b/include/drm/drm_atomic_helper.h > @@ -126,6 +126,7 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc, > uint32_t flags); > int drm_atomic_helper_connector_dpms(struct drm_connector *connector, > int mode); > +int drm_atomic_helper_connector_modeset(struct drm_connector *connector); > struct drm_encoder * > drm_atomic_helper_best_encoder(struct drm_connector *connector); -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode 2016-10-25 12:09 ` Jani Nikula @ 2016-10-25 22:28 ` Manasi Navare 2016-10-25 22:38 ` Rodrigo Vivi 1 sibling, 0 replies; 33+ messages in thread From: Manasi Navare @ 2016-10-25 22:28 UTC (permalink / raw) To: Jani Nikula; +Cc: Daniel Vetter, intel-gfx, dri-devel On Tue, Oct 25, 2016 at 03:09:39PM +0300, Jani Nikula wrote: > On Sat, 22 Oct 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > > This function provides a way for the driver to redo a > > modeset on the current mode and retry the link training > > at a lower link rate/lane count/bpp. This will get called > > incase the link training fails during the current modeset. > > Based on discussions on #intel-gfx, I would dodge all the problems here > by having the userspace do the modeset. > > If we add a connector property to indicate the link status (and this > does not have to be DP or link training specific, really), we can set > that to "bad", and fire off the hotplug uevent. Then userspace has the > information to force a modeset on that connector even if the mode has > not changed. (Credits to Ville for the idea.) > > If we find a solution later on that allows us to handle all of this in > kernel, we can do so, and remove the property or always report > "good". Drivers can choose different approaches, depending on the > capabilities of the hardware, for instance. > > Deferring this to the userspace does not regress anything now, because > currently we just completely fail and end up with a black screen. The > property would allow an enlightened userspace to fix that. And userspace > can't rely on the property being there, as it's currently not there. > > > BR, > Jani. > > Thanks for your feedback Jani. I will try implementing this, but isn't this going to require changes in all the userspace drivers (modesetting driver, ChromeOS) to make use of this new property to trigger another modeset? How easy would it be to have all the userspace drivers adopt this change? Regards Manasi > > > > Cc: dri-devel@lists.freedesktop.org > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > --- > > drivers/gpu/drm/drm_atomic_helper.c | 58 +++++++++++++++++++++++++++++++++++++ > > include/drm/drm_atomic_helper.h | 1 + > > 2 files changed, 59 insertions(+) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > index f936276..0c1614e 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -2895,6 +2895,64 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector, > > EXPORT_SYMBOL(drm_atomic_helper_connector_dpms); > > > > /** > > + * drm_atomic_helper_connector_modeset - Force a modeset on a connector > > + * @connector: DRM connector > > + * > > + * Provides a way to redo a modeset with the current mode so that it can > > + * drop the bpp, link rate/lane count and retry the link training. > > + * > > + * Returns: > > + * Returns 0 on success, negative errno numbers on failure. > > + */ > > +int > > +drm_atomic_helper_connector_modeset(struct drm_connector *connector) > > +{ > > + struct drm_device *dev = connector->dev; > > + struct drm_modeset_acquire_ctx ctx; > > + struct drm_atomic_state *state; > > + struct drm_connector_state *connector_state; > > + struct drm_crtc_state *crtc_state; > > + int ret = 0; > > + > > + drm_modeset_acquire_init(&ctx, 0); > > + state = drm_atomic_state_alloc(dev); > > + if (!state) { > > + ret = -ENOMEM; > > + goto fail; > > + } > > + state->acquire_ctx = &ctx; > > +retry: > > + ret = 0; > > + connector_state = drm_atomic_get_connector_state(state, connector); > > + if (IS_ERR(connector_state)) { > > + ret = PTR_ERR(connector_state); > > + goto fail; > > + } > > + if (!connector_state->crtc) > > + goto fail; > > + > > + crtc_state = drm_atomic_get_existing_crtc_state(state, > > + connector_state->crtc); > > + crtc_state->connectors_changed = true; > > + ret = drm_atomic_commit(state); > > +fail: > > + if (ret == -EDEADLK) { > > + drm_atomic_state_clear(state); > > + drm_modeset_backoff(&ctx); > > + goto retry; > > + } > > + > > + if (state) > > + drm_atomic_state_put(state); > > + > > + drm_modeset_drop_locks(&ctx); > > + drm_modeset_acquire_fini(&ctx); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL(drm_atomic_helper_connector_modeset); > > + > > +/** > > * drm_atomic_helper_best_encoder - Helper for &drm_connector_helper_funcs > > * ->best_encoder callback > > * @connector: Connector control structure > > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h > > index 7ff92b0..8de24dc 100644 > > --- a/include/drm/drm_atomic_helper.h > > +++ b/include/drm/drm_atomic_helper.h > > @@ -126,6 +126,7 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc, > > uint32_t flags); > > int drm_atomic_helper_connector_dpms(struct drm_connector *connector, > > int mode); > > +int drm_atomic_helper_connector_modeset(struct drm_connector *connector); > > struct drm_encoder * > > drm_atomic_helper_best_encoder(struct drm_connector *connector); > > -- > Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode 2016-10-25 12:09 ` Jani Nikula 2016-10-25 22:28 ` Manasi Navare @ 2016-10-25 22:38 ` Rodrigo Vivi 1 sibling, 0 replies; 33+ messages in thread From: Rodrigo Vivi @ 2016-10-25 22:38 UTC (permalink / raw) To: Jani Nikula; +Cc: Daniel Vetter, intel-gfx, DRI mailing list On Tue, Oct 25, 2016 at 5:09 AM, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Sat, 22 Oct 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: >> This function provides a way for the driver to redo a >> modeset on the current mode and retry the link training >> at a lower link rate/lane count/bpp. This will get called >> incase the link training fails during the current modeset. > > Based on discussions on #intel-gfx, I would dodge all the problems here > by having the userspace do the modeset. This is not like going back to UMS times? > If we add a connector property to indicate the link status (and this > does not have to be DP or link training specific, really), we can set > that to "bad", and fire off the hotplug uevent. Then userspace has the > information to force a modeset on that connector even if the mode has > not changed. (Credits to Ville for the idea.) This would require changes in all user space drivers out there right? Ok, maybe faster than fix vblank layer for good... > > If we find a solution later on that allows us to handle all of this in > kernel, we can do so, and remove the property or always report > "good". Drivers can choose different approaches, depending on the > capabilities of the hardware, for instance. So, is this temporary then? While we update the vblank layer? > Deferring this to the userspace does not regress anything now, because > currently we just completely fail and end up with a black screen. The > property would allow an enlightened userspace to fix that. And userspace > can't rely on the property being there, as it's currently not there. Also this discussion remind me about the PSR where we refused to give control to userspace or create any property and kmd should control all cases. Should we revisit that and change userspace to control PSR? Thanks, Rodrigo. > > > BR, > Jani. > > >> >> Cc: dri-devel@lists.freedesktop.org >> Cc: Jani Nikula <jani.nikula@linux.intel.com> >> Cc: Daniel Vetter <daniel.vetter@intel.com> >> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> >> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> >> --- >> drivers/gpu/drm/drm_atomic_helper.c | 58 +++++++++++++++++++++++++++++++++++++ >> include/drm/drm_atomic_helper.h | 1 + >> 2 files changed, 59 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >> index f936276..0c1614e 100644 >> --- a/drivers/gpu/drm/drm_atomic_helper.c >> +++ b/drivers/gpu/drm/drm_atomic_helper.c >> @@ -2895,6 +2895,64 @@ int drm_atomic_helper_connector_dpms(struct drm_connector *connector, >> EXPORT_SYMBOL(drm_atomic_helper_connector_dpms); >> >> /** >> + * drm_atomic_helper_connector_modeset - Force a modeset on a connector >> + * @connector: DRM connector >> + * >> + * Provides a way to redo a modeset with the current mode so that it can >> + * drop the bpp, link rate/lane count and retry the link training. >> + * >> + * Returns: >> + * Returns 0 on success, negative errno numbers on failure. >> + */ >> +int >> +drm_atomic_helper_connector_modeset(struct drm_connector *connector) >> +{ >> + struct drm_device *dev = connector->dev; >> + struct drm_modeset_acquire_ctx ctx; >> + struct drm_atomic_state *state; >> + struct drm_connector_state *connector_state; >> + struct drm_crtc_state *crtc_state; >> + int ret = 0; >> + >> + drm_modeset_acquire_init(&ctx, 0); >> + state = drm_atomic_state_alloc(dev); >> + if (!state) { >> + ret = -ENOMEM; >> + goto fail; >> + } >> + state->acquire_ctx = &ctx; >> +retry: >> + ret = 0; >> + connector_state = drm_atomic_get_connector_state(state, connector); >> + if (IS_ERR(connector_state)) { >> + ret = PTR_ERR(connector_state); >> + goto fail; >> + } >> + if (!connector_state->crtc) >> + goto fail; >> + >> + crtc_state = drm_atomic_get_existing_crtc_state(state, >> + connector_state->crtc); >> + crtc_state->connectors_changed = true; >> + ret = drm_atomic_commit(state); >> +fail: >> + if (ret == -EDEADLK) { >> + drm_atomic_state_clear(state); >> + drm_modeset_backoff(&ctx); >> + goto retry; >> + } >> + >> + if (state) >> + drm_atomic_state_put(state); >> + >> + drm_modeset_drop_locks(&ctx); >> + drm_modeset_acquire_fini(&ctx); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL(drm_atomic_helper_connector_modeset); >> + >> +/** >> * drm_atomic_helper_best_encoder - Helper for &drm_connector_helper_funcs >> * ->best_encoder callback >> * @connector: Connector control structure >> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h >> index 7ff92b0..8de24dc 100644 >> --- a/include/drm/drm_atomic_helper.h >> +++ b/include/drm/drm_atomic_helper.h >> @@ -126,6 +126,7 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc, >> uint32_t flags); >> int drm_atomic_helper_connector_dpms(struct drm_connector *connector, >> int mode); >> +int drm_atomic_helper_connector_modeset(struct drm_connector *connector); >> struct drm_encoder * >> drm_atomic_helper_best_encoder(struct drm_connector *connector); > > -- > Jani Nikula, Intel Open Source Technology Center > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Rodrigo Vivi Blog: http://blog.vivi.eng.br _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 2/5] drm: Define a work struct for scheduling a uevent for modeset retry 2016-10-21 23:45 [PATCH 0/5] Handle link training failure for DDI platforms Manasi Navare 2016-10-21 23:45 ` [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode Manasi Navare @ 2016-10-21 23:45 ` Manasi Navare 2016-10-22 8:48 ` Daniel Vetter 2016-10-21 23:45 ` [PATCH 3/5] drm/i915: Change the placement of some static functions in intel_dp.c Manasi Navare ` (3 subsequent siblings) 5 siblings, 1 reply; 33+ messages in thread From: Manasi Navare @ 2016-10-21 23:45 UTC (permalink / raw) To: intel-gfx; +Cc: Daniel Vetter, dri-devel This work struct will be used to schedule a uevent on a separate thread. This will be scheduled after a link train failure during modeset to indicate a modeset retry request. It will get executed after the current modeset is complete and all locks are released. This was required to avoid deadlock. Cc: dri-devel@lists.freedesktop.org Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Daniel Vetter <daniel.vetter@intel.com> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> --- include/drm/drm_connector.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h index ac9d7d8..fcf6b97 100644 --- a/include/drm/drm_connector.h +++ b/include/drm/drm_connector.h @@ -682,6 +682,11 @@ struct drm_connector { uint8_t num_h_tile, num_v_tile; uint8_t tile_h_loc, tile_v_loc; uint16_t tile_h_size, tile_v_size; + + /* Work struct to schedule a uevent on link train failure for + * DisplayPort. + */ + struct work_struct i915_modeset_retry_work; }; #define obj_to_connector(x) container_of(x, struct drm_connector, base) -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 2/5] drm: Define a work struct for scheduling a uevent for modeset retry 2016-10-21 23:45 ` [PATCH 2/5] drm: Define a work struct for scheduling a uevent for modeset retry Manasi Navare @ 2016-10-22 8:48 ` Daniel Vetter 2016-10-25 6:28 ` Manasi Navare 0 siblings, 1 reply; 33+ messages in thread From: Daniel Vetter @ 2016-10-22 8:48 UTC (permalink / raw) To: Manasi Navare; +Cc: Daniel Vetter, intel-gfx, dri-devel On Fri, Oct 21, 2016 at 04:45:40PM -0700, Manasi Navare wrote: > This work struct will be used to schedule a uevent on a separate > thread. This will be scheduled after a link train failure during modeset > to indicate a modeset retry request. It will get executed after the > current modeset is complete and all locks are released. This was > required to avoid deadlock. > > Cc: dri-devel@lists.freedesktop.org > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > include/drm/drm_connector.h | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > index ac9d7d8..fcf6b97 100644 > --- a/include/drm/drm_connector.h > +++ b/include/drm/drm_connector.h > @@ -682,6 +682,11 @@ struct drm_connector { > uint8_t num_h_tile, num_v_tile; > uint8_t tile_h_loc, tile_v_loc; > uint16_t tile_h_size, tile_v_size; > + > + /* Work struct to schedule a uevent on link train failure for > + * DisplayPort. > + */ > + struct work_struct i915_modeset_retry_work; You cannot put i915 stuff into core structs. -Daniel > }; > > #define obj_to_connector(x) container_of(x, struct drm_connector, base) > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/5] drm: Define a work struct for scheduling a uevent for modeset retry 2016-10-22 8:48 ` Daniel Vetter @ 2016-10-25 6:28 ` Manasi Navare 2016-10-25 6:30 ` Pandiyan, Dhinakaran 0 siblings, 1 reply; 33+ messages in thread From: Manasi Navare @ 2016-10-25 6:28 UTC (permalink / raw) To: Daniel Vetter; +Cc: Daniel Vetter, intel-gfx, dri-devel On Sat, Oct 22, 2016 at 10:48:13AM +0200, Daniel Vetter wrote: > On Fri, Oct 21, 2016 at 04:45:40PM -0700, Manasi Navare wrote: > > This work struct will be used to schedule a uevent on a separate > > thread. This will be scheduled after a link train failure during modeset > > to indicate a modeset retry request. It will get executed after the > > current modeset is complete and all locks are released. This was > > required to avoid deadlock. > > > > Cc: dri-devel@lists.freedesktop.org > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > --- > > include/drm/drm_connector.h | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > > index ac9d7d8..fcf6b97 100644 > > --- a/include/drm/drm_connector.h > > +++ b/include/drm/drm_connector.h > > @@ -682,6 +682,11 @@ struct drm_connector { > > uint8_t num_h_tile, num_v_tile; > > uint8_t tile_h_loc, tile_v_loc; > > uint16_t tile_h_size, tile_v_size; > > + > > + /* Work struct to schedule a uevent on link train failure for > > + * DisplayPort. > > + */ > > + struct work_struct i915_modeset_retry_work; > > You cannot put i915 stuff into core structs. > -Daniel > > > }; I will rename it to use modeset_retry_work instead. Manasi > > > > #define obj_to_connector(x) container_of(x, struct drm_connector, base) > > -- > > 1.9.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/5] drm: Define a work struct for scheduling a uevent for modeset retry 2016-10-25 6:28 ` Manasi Navare @ 2016-10-25 6:30 ` Pandiyan, Dhinakaran 2016-10-25 6:45 ` [Intel-gfx] " Daniel Vetter 0 siblings, 1 reply; 33+ messages in thread From: Pandiyan, Dhinakaran @ 2016-10-25 6:30 UTC (permalink / raw) To: Navare, Manasi D; +Cc: Vetter, Daniel, intel-gfx, dri-devel On Mon, 2016-10-24 at 23:28 -0700, Manasi Navare wrote: > On Sat, Oct 22, 2016 at 10:48:13AM +0200, Daniel Vetter wrote: > > On Fri, Oct 21, 2016 at 04:45:40PM -0700, Manasi Navare wrote: > > > This work struct will be used to schedule a uevent on a separate > > > thread. This will be scheduled after a link train failure during modeset > > > to indicate a modeset retry request. It will get executed after the > > > current modeset is complete and all locks are released. This was > > > required to avoid deadlock. > > > > > > Cc: dri-devel@lists.freedesktop.org > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > --- > > > include/drm/drm_connector.h | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > > > index ac9d7d8..fcf6b97 100644 > > > --- a/include/drm/drm_connector.h > > > +++ b/include/drm/drm_connector.h > > > @@ -682,6 +682,11 @@ struct drm_connector { > > > uint8_t num_h_tile, num_v_tile; > > > uint8_t tile_h_loc, tile_v_loc; > > > uint16_t tile_h_size, tile_v_size; > > > + > > > + /* Work struct to schedule a uevent on link train failure for > > > + * DisplayPort. > > > + */ > > > + struct work_struct i915_modeset_retry_work; > > > > You cannot put i915 stuff into core structs. > > -Daniel > > > > > }; > > I will rename it to use modeset_retry_work instead. > > Manasi Why not move it struct intel_connector? The work function is defined in intel_dp.c afaict -DK > > > > > > #define obj_to_connector(x) container_of(x, struct drm_connector, base) > > > -- > > > 1.9.1 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [Intel-gfx] [PATCH 2/5] drm: Define a work struct for scheduling a uevent for modeset retry 2016-10-25 6:30 ` Pandiyan, Dhinakaran @ 2016-10-25 6:45 ` Daniel Vetter 0 siblings, 0 replies; 33+ messages in thread From: Daniel Vetter @ 2016-10-25 6:45 UTC (permalink / raw) To: Pandiyan, Dhinakaran Cc: Navare, Manasi D, Vetter, Daniel, intel-gfx, dri-devel On Tue, Oct 25, 2016 at 06:30:29AM +0000, Pandiyan, Dhinakaran wrote: > On Mon, 2016-10-24 at 23:28 -0700, Manasi Navare wrote: > > On Sat, Oct 22, 2016 at 10:48:13AM +0200, Daniel Vetter wrote: > > > On Fri, Oct 21, 2016 at 04:45:40PM -0700, Manasi Navare wrote: > > > > This work struct will be used to schedule a uevent on a separate > > > > thread. This will be scheduled after a link train failure during modeset > > > > to indicate a modeset retry request. It will get executed after the > > > > current modeset is complete and all locks are released. This was > > > > required to avoid deadlock. > > > > > > > > Cc: dri-devel@lists.freedesktop.org > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > > > > > > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > > > --- > > > > include/drm/drm_connector.h | 5 +++++ > > > > 1 file changed, 5 insertions(+) > > > > > > > > diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h > > > > index ac9d7d8..fcf6b97 100644 > > > > --- a/include/drm/drm_connector.h > > > > +++ b/include/drm/drm_connector.h > > > > @@ -682,6 +682,11 @@ struct drm_connector { > > > > uint8_t num_h_tile, num_v_tile; > > > > uint8_t tile_h_loc, tile_v_loc; > > > > uint16_t tile_h_size, tile_v_size; > > > > + > > > > + /* Work struct to schedule a uevent on link train failure for > > > > + * DisplayPort. > > > > + */ > > > > + struct work_struct i915_modeset_retry_work; > > > > > > You cannot put i915 stuff into core structs. > > > -Daniel > > > > > > > }; > > > > I will rename it to use modeset_retry_work instead. > > > > Manasi > > > Why not move it struct intel_connector? The work function is defined in > intel_dp.c afaict Yeah, the place is wrong, not just the name. -Daniel > > -DK > > > > > > > > #define obj_to_connector(x) container_of(x, struct drm_connector, base) > > > > -- > > > > 1.9.1 > > > > > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 3/5] drm/i915: Change the placement of some static functions in intel_dp.c 2016-10-21 23:45 [PATCH 0/5] Handle link training failure for DDI platforms Manasi Navare 2016-10-21 23:45 ` [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode Manasi Navare 2016-10-21 23:45 ` [PATCH 2/5] drm: Define a work struct for scheduling a uevent for modeset retry Manasi Navare @ 2016-10-21 23:45 ` Manasi Navare 2016-10-21 23:45 ` [PATCH 4/5] drm/i915; Add a function to return index of link rate Manasi Navare ` (2 subsequent siblings) 5 siblings, 0 replies; 33+ messages in thread From: Manasi Navare @ 2016-10-21 23:45 UTC (permalink / raw) To: intel-gfx; +Cc: Daniel Vetter From: "Navare, Manasi D" <manasi.d.navare@intel.com> These static helper functions are required to be used during fallback link rate implemnetation so they need to be placed at the top of the file. v3: * Add cleanup to other patch (Mika Kahola) v2: * Dont move around functions declared in intel_drv.h (Rodrigo Vivi) Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Daniel Vetter <daniel.vetter@intel.com> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> Reviewed-by: Mika Kahola <mika.kahola@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 150 ++++++++++++++++++++-------------------- 1 file changed, 75 insertions(+), 75 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index f30db8f..c499ec1 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -213,6 +213,81 @@ static u8 intel_dp_max_lane_count(struct intel_dp *intel_dp) return max_dotclk; } +static int +intel_dp_sink_rates(struct intel_dp *intel_dp, const int **sink_rates) +{ + if (intel_dp->num_sink_rates) { + *sink_rates = intel_dp->sink_rates; + return intel_dp->num_sink_rates; + } + + *sink_rates = default_rates; + + return (intel_dp_max_link_bw(intel_dp) >> 3) + 1; +} + +static int +intel_dp_source_rates(struct intel_dp *intel_dp, const int **source_rates) +{ + struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); + int size; + + if (IS_BROXTON(dev_priv)) { + *source_rates = bxt_rates; + size = ARRAY_SIZE(bxt_rates); + } else if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) { + *source_rates = skl_rates; + size = ARRAY_SIZE(skl_rates); + } else { + *source_rates = default_rates; + size = ARRAY_SIZE(default_rates); + } + + /* This depends on the fact that 5.4 is last value in the array */ + if (!intel_dp_source_supports_hbr2(intel_dp)) + size--; + + return size; +} + +static int intersect_rates(const int *source_rates, int source_len, + const int *sink_rates, int sink_len, + int *common_rates) +{ + int i = 0, j = 0, k = 0; + + while (i < source_len && j < sink_len) { + if (source_rates[i] == sink_rates[j]) { + if (WARN_ON(k >= DP_MAX_SUPPORTED_RATES)) + return k; + common_rates[k] = source_rates[i]; + ++k; + ++i; + ++j; + } else if (source_rates[i] < sink_rates[j]) { + ++i; + } else { + ++j; + } + } + return k; +} + +static int intel_dp_common_rates(struct intel_dp *intel_dp, + int *common_rates) +{ + const int *source_rates, *sink_rates; + int source_len, sink_len; + + sink_len = intel_dp_sink_rates(intel_dp, &sink_rates); + source_len = intel_dp_source_rates(intel_dp, &source_rates); + + return intersect_rates(source_rates, source_len, + sink_rates, sink_len, + common_rates); +} + static enum drm_mode_status intel_dp_mode_valid(struct drm_connector *connector, struct drm_display_mode *mode) @@ -1291,19 +1366,6 @@ static void intel_aux_reg_init(struct intel_dp *intel_dp) intel_dp->aux.transfer = intel_dp_aux_transfer; } -static int -intel_dp_sink_rates(struct intel_dp *intel_dp, const int **sink_rates) -{ - if (intel_dp->num_sink_rates) { - *sink_rates = intel_dp->sink_rates; - return intel_dp->num_sink_rates; - } - - *sink_rates = default_rates; - - return (intel_dp_max_link_bw(intel_dp) >> 3) + 1; -} - bool intel_dp_source_supports_hbr2(struct intel_dp *intel_dp) { struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); @@ -1316,31 +1378,6 @@ bool intel_dp_source_supports_hbr2(struct intel_dp *intel_dp) return false; } -static int -intel_dp_source_rates(struct intel_dp *intel_dp, const int **source_rates) -{ - struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp); - struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); - int size; - - if (IS_BROXTON(dev_priv)) { - *source_rates = bxt_rates; - size = ARRAY_SIZE(bxt_rates); - } else if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv)) { - *source_rates = skl_rates; - size = ARRAY_SIZE(skl_rates); - } else { - *source_rates = default_rates; - size = ARRAY_SIZE(default_rates); - } - - /* This depends on the fact that 5.4 is last value in the array */ - if (!intel_dp_source_supports_hbr2(intel_dp)) - size--; - - return size; -} - static void intel_dp_set_clock(struct intel_encoder *encoder, struct intel_crtc_state *pipe_config) @@ -1375,43 +1412,6 @@ bool intel_dp_source_supports_hbr2(struct intel_dp *intel_dp) } } -static int intersect_rates(const int *source_rates, int source_len, - const int *sink_rates, int sink_len, - int *common_rates) -{ - int i = 0, j = 0, k = 0; - - while (i < source_len && j < sink_len) { - if (source_rates[i] == sink_rates[j]) { - if (WARN_ON(k >= DP_MAX_SUPPORTED_RATES)) - return k; - common_rates[k] = source_rates[i]; - ++k; - ++i; - ++j; - } else if (source_rates[i] < sink_rates[j]) { - ++i; - } else { - ++j; - } - } - return k; -} - -static int intel_dp_common_rates(struct intel_dp *intel_dp, - int *common_rates) -{ - const int *source_rates, *sink_rates; - int source_len, sink_len; - - sink_len = intel_dp_sink_rates(intel_dp, &sink_rates); - source_len = intel_dp_source_rates(intel_dp, &source_rates); - - return intersect_rates(source_rates, source_len, - sink_rates, sink_len, - common_rates); -} - static void snprintf_int_array(char *str, size_t len, const int *array, int nelem) { -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 4/5] drm/i915; Add a function to return index of link rate 2016-10-21 23:45 [PATCH 0/5] Handle link training failure for DDI platforms Manasi Navare ` (2 preceding siblings ...) 2016-10-21 23:45 ` [PATCH 3/5] drm/i915: Change the placement of some static functions in intel_dp.c Manasi Navare @ 2016-10-21 23:45 ` Manasi Navare 2016-10-25 6:33 ` Pandiyan, Dhinakaran 2016-10-21 23:45 ` [PATCH 5/5] drm/i915: Link Rate fallback on Link training failure Manasi Navare 2016-10-22 0:16 ` ✗ Fi.CI.BAT: warning for Handle link training failure for DDI platforms Patchwork 5 siblings, 1 reply; 33+ messages in thread From: Manasi Navare @ 2016-10-21 23:45 UTC (permalink / raw) To: intel-gfx; +Cc: Daniel Vetter This is required to return the index of link rate into common_rates array. This gets used to retry the link training at lower link rate. Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Daniel Vetter <daniel.vetter@intel.com> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index c499ec1..c192e18 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -288,6 +288,21 @@ static int intel_dp_common_rates(struct intel_dp *intel_dp, common_rates); } +static int intel_dp_link_rate_index(struct intel_dp *intel_dp, + int *common_rates, int link_rate) +{ + int common_len; + int index; + + common_len = intel_dp_common_rates(intel_dp, common_rates); + for (index = 0; index < common_len; index++) { + if (link_rate == common_rates[common_len - index - 1]) + return common_len - index - 1; + } + + return -1; +} + static enum drm_mode_status intel_dp_mode_valid(struct drm_connector *connector, struct drm_display_mode *mode) -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 4/5] drm/i915; Add a function to return index of link rate 2016-10-21 23:45 ` [PATCH 4/5] drm/i915; Add a function to return index of link rate Manasi Navare @ 2016-10-25 6:33 ` Pandiyan, Dhinakaran 0 siblings, 0 replies; 33+ messages in thread From: Pandiyan, Dhinakaran @ 2016-10-25 6:33 UTC (permalink / raw) To: Navare, Manasi D; +Cc: Vetter, Daniel, intel-gfx On Fri, 2016-10-21 at 16:45 -0700, Manasi Navare wrote: > This is required to return the index of link rate into > common_rates array. This gets used to retry the link > training at lower link rate. > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index c499ec1..c192e18 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -288,6 +288,21 @@ static int intel_dp_common_rates(struct intel_dp *intel_dp, > common_rates); > } > > +static int intel_dp_link_rate_index(struct intel_dp *intel_dp, > + int *common_rates, int link_rate) > +{ > + int common_len; > + int index; > + > + common_len = intel_dp_common_rates(intel_dp, common_rates); > + for (index = 0; index < common_len; index++) { Running the loop in the opposite direction will eliminate the unnecessary array index computations. > + if (link_rate == common_rates[common_len - index - 1]) > + return common_len - index - 1; > + } > + > + return -1; > +} > + > static enum drm_mode_status > intel_dp_mode_valid(struct drm_connector *connector, > struct drm_display_mode *mode) _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 5/5] drm/i915: Link Rate fallback on Link training failure 2016-10-21 23:45 [PATCH 0/5] Handle link training failure for DDI platforms Manasi Navare ` (3 preceding siblings ...) 2016-10-21 23:45 ` [PATCH 4/5] drm/i915; Add a function to return index of link rate Manasi Navare @ 2016-10-21 23:45 ` Manasi Navare 2016-10-24 17:53 ` Jim Bride ` (2 more replies) 2016-10-22 0:16 ` ✗ Fi.CI.BAT: warning for Handle link training failure for DDI platforms Patchwork 5 siblings, 3 replies; 33+ messages in thread From: Manasi Navare @ 2016-10-21 23:45 UTC (permalink / raw) To: intel-gfx; +Cc: Daniel Vetter If link training at a link rate optimal for a particular mode fails during modeset's atomic commit phase, then we let the modeset complete and then retry. We save the link rate value at which link training failed and use a lower link rate to prune the modes. It will redo the modeset on the current mode at lower link rate or if the current mode gets pruned due to lower link constraints then, it will send a hotplug uevent for userspace to handle it. This is also required to pass DP CTS tests 4.3.1.3, 4.3.1.4, 4.3.1.6. Cc: Jani Nikula <jani.nikula@linux.intel.com> Cc: Daniel Vetter <daniel.vetter@intel.com> Cc: Ville Syrjala <ville.syrjala@linux.intel.com> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> --- drivers/gpu/drm/i915/intel_ddi.c | 15 +++++- drivers/gpu/drm/i915/intel_dp.c | 69 ++++++++++++++++++++++++++- drivers/gpu/drm/i915/intel_dp_link_training.c | 12 +++-- drivers/gpu/drm/i915/intel_drv.h | 6 ++- 4 files changed, 95 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index fb18d69..451433b 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -1712,6 +1712,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); enum port port = intel_ddi_get_encoder_port(encoder); + struct intel_connector *intel_connector = intel_dp->attached_connector; + struct drm_connector *connector = &intel_connector->base; intel_dp_set_link_params(intel_dp, link_rate, lane_count, link_mst); @@ -1722,7 +1724,18 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, intel_prepare_dp_ddi_buffers(encoder); intel_ddi_init_dp_buf_reg(encoder); intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); - intel_dp_start_link_train(intel_dp); + if (!intel_dp_start_link_train(intel_dp)) { + DRM_ERROR("Link Training failed at link rate = %d, lane count = %d\n", + link_rate, lane_count); + intel_dp->link_train_failed = true; + intel_dp->link_train_failed_link_rate = link_rate; + intel_dp->link_train_failed_lane_count = lane_count; + /* Schedule a Hotplug Uevent to userspace to start modeset */ + schedule_work(&connector->i915_modeset_retry_work); + } else { + intel_dp->link_train_failed = false; + } + if (port != PORT_A || INTEL_GEN(dev_priv) >= 9) intel_dp_stop_link_train(intel_dp); } diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index c192e18..5d5f4a7 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -313,6 +313,7 @@ static int intel_dp_link_rate_index(struct intel_dp *intel_dp, int target_clock = mode->clock; int max_rate, mode_rate, max_lanes, max_link_clock; int max_dotclk; + int common_rates[DP_MAX_SUPPORTED_RATES] = {}; max_dotclk = intel_dp_downstream_max_dotclock(intel_dp); @@ -326,8 +327,27 @@ static int intel_dp_link_rate_index(struct intel_dp *intel_dp, target_clock = fixed_mode->clock; } - max_link_clock = intel_dp_max_link_rate(intel_dp); - max_lanes = intel_dp_max_lane_count(intel_dp); + /* Prune the modes based on the link rate that failed */ + if (intel_dp->link_train_failed_link_rate) { + intel_dp->link_rate_index = intel_dp_link_rate_index(intel_dp, + common_rates, + intel_dp->link_train_failed_link_rate); + if (intel_dp->link_rate_index > 0) { + max_link_clock = common_rates[intel_dp->link_rate_index - 1]; + max_lanes = intel_dp_max_lane_count(intel_dp); + } else { + /* Here we can lower the lane count, but that will be + * added for DP Spec 1.3 + */ + DRM_ERROR("No Valid Mode Supported for this Link\n"); + intel_dp->link_train_failed_link_rate = 0; + intel_dp->link_rate_index = -1; + intel_dp->link_train_failed = false; + } + } else { + max_link_clock = intel_dp_max_link_rate(intel_dp); + max_lanes = intel_dp_max_lane_count(intel_dp); + } max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes); mode_rate = intel_dp_link_required(target_clock, 18); @@ -1619,6 +1639,14 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp, if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) return false; + /* Fall back to lower link rate in case of failure in previous modeset */ + if (intel_dp->link_train_failed_link_rate) { + min_lane_count = max_lane_count; + min_clock = max_clock = intel_dp->link_rate_index - 1; + intel_dp->link_train_failed_link_rate = 0; + intel_dp->link_rate_index = -1; + } + DRM_DEBUG_KMS("DP link computation with max lane count %i " "max bw %d pixel clock %iKHz\n", max_lane_count, common_rates[max_clock], @@ -5689,6 +5717,39 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, return false; } +static void intel_dp_modeset_retry_work_fn(struct work_struct *work) +{ + struct drm_connector *connector; + struct intel_dp *intel_dp; + struct drm_display_mode *mode; + bool verbose_prune = true; + bool reprobe = false; + + connector = container_of(work, typeof(*connector), + i915_modeset_retry_work); + intel_dp = intel_attached_dp(connector); + + /* Grab the locks before changing connector property*/ + mutex_lock(&connector->dev->mode_config.mutex); + DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, + connector->name); + list_for_each_entry(mode, &connector->modes, head) { + mode->status = intel_dp_mode_valid(connector, + mode); + if (mode->status != MODE_OK) + reprobe = true; + } + drm_mode_prune_invalid(connector->dev, &connector->modes, + verbose_prune); + mutex_unlock(&connector->dev->mode_config.mutex); + if (reprobe) { + /* Send Hotplug uevent so userspace can reprobe */ + drm_kms_helper_hotplug_event(connector->dev); + } + if (intel_dp->link_train_failed) + drm_atomic_helper_connector_modeset(connector); +} + bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port, struct intel_connector *intel_connector) @@ -5701,6 +5762,10 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, enum port port = intel_dig_port->port; int type; + /* Initialize the work for modeset in case of link train failure */ + INIT_WORK(&connector->i915_modeset_retry_work, + intel_dp_modeset_retry_work_fn); + if (WARN(intel_dig_port->max_lanes < 1, "Not enough lanes (%d) for DP on port %c\n", intel_dig_port->max_lanes, port_name(port))) diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c index 0048b52..10f81ab 100644 --- a/drivers/gpu/drm/i915/intel_dp_link_training.c +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c @@ -310,9 +310,15 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp) DP_TRAINING_PATTERN_DISABLE); } -void +bool intel_dp_start_link_train(struct intel_dp *intel_dp) { - intel_dp_link_training_clock_recovery(intel_dp); - intel_dp_link_training_channel_equalization(intel_dp); + bool ret; + + if (intel_dp_link_training_clock_recovery(intel_dp)) { + ret = intel_dp_link_training_channel_equalization(intel_dp); + if (ret) + return true; + } + return false; } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 4e90b07..d3fcffc 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -890,6 +890,10 @@ struct intel_dp { uint32_t DP; int link_rate; uint8_t lane_count; + int link_train_failed_link_rate; + uint8_t link_train_failed_lane_count; + int link_rate_index; + bool link_train_failed; uint8_t sink_count; bool link_mst; bool has_audio; @@ -1394,7 +1398,7 @@ bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port, void intel_dp_set_link_params(struct intel_dp *intel_dp, int link_rate, uint8_t lane_count, bool link_mst); -void intel_dp_start_link_train(struct intel_dp *intel_dp); +bool intel_dp_start_link_train(struct intel_dp *intel_dp); void intel_dp_stop_link_train(struct intel_dp *intel_dp); void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode); void intel_dp_encoder_reset(struct drm_encoder *encoder); -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 5/5] drm/i915: Link Rate fallback on Link training failure 2016-10-21 23:45 ` [PATCH 5/5] drm/i915: Link Rate fallback on Link training failure Manasi Navare @ 2016-10-24 17:53 ` Jim Bride 2016-10-25 6:23 ` Pandiyan, Dhinakaran 2016-10-25 12:17 ` Jani Nikula 2 siblings, 0 replies; 33+ messages in thread From: Jim Bride @ 2016-10-24 17:53 UTC (permalink / raw) To: Manasi Navare; +Cc: Daniel Vetter, intel-gfx On Fri, Oct 21, 2016 at 04:45:43PM -0700, Manasi Navare wrote: > If link training at a link rate optimal for a particular > mode fails during modeset's atomic commit phase, then we > let the modeset complete and then retry. We save the link rate > value at which link training failed and use a lower link rate > to prune the modes. It will redo the modeset on the current mode > at lower link rate or if the current mode gets pruned due to lower > link constraints then, it will send a hotplug uevent for userspace > to handle it. > > This is also required to pass DP CTS tests 4.3.1.3, 4.3.1.4, > 4.3.1.6. > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 15 +++++- > drivers/gpu/drm/i915/intel_dp.c | 69 ++++++++++++++++++++++++++- > drivers/gpu/drm/i915/intel_dp_link_training.c | 12 +++-- > drivers/gpu/drm/i915/intel_drv.h | 6 ++- > 4 files changed, 95 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index fb18d69..451433b 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -1712,6 +1712,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > enum port port = intel_ddi_get_encoder_port(encoder); > + struct intel_connector *intel_connector = intel_dp->attached_connector; > + struct drm_connector *connector = &intel_connector->base; > > intel_dp_set_link_params(intel_dp, link_rate, lane_count, > link_mst); > @@ -1722,7 +1724,18 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > intel_prepare_dp_ddi_buffers(encoder); > intel_ddi_init_dp_buf_reg(encoder); > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); > - intel_dp_start_link_train(intel_dp); > + if (!intel_dp_start_link_train(intel_dp)) { > + DRM_ERROR("Link Training failed at link rate = %d, lane count = %d\n", > + link_rate, lane_count); I don't think this should be a DRM_ERROR, since it's a part of the normal fallback mechanism. > + intel_dp->link_train_failed = true; > + intel_dp->link_train_failed_link_rate = link_rate; > + intel_dp->link_train_failed_lane_count = lane_count; > + /* Schedule a Hotplug Uevent to userspace to start modeset */ Minor nit, no caps needed for "Hotplug" or "Uevent." Jim > + schedule_work(&connector->i915_modeset_retry_work); > + } else { > + intel_dp->link_train_failed = false; > + } > + > if (port != PORT_A || INTEL_GEN(dev_priv) >= 9) > intel_dp_stop_link_train(intel_dp); > } > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index c192e18..5d5f4a7 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -313,6 +313,7 @@ static int intel_dp_link_rate_index(struct intel_dp *intel_dp, > int target_clock = mode->clock; > int max_rate, mode_rate, max_lanes, max_link_clock; > int max_dotclk; > + int common_rates[DP_MAX_SUPPORTED_RATES] = {}; > > max_dotclk = intel_dp_downstream_max_dotclock(intel_dp); > > @@ -326,8 +327,27 @@ static int intel_dp_link_rate_index(struct intel_dp *intel_dp, > target_clock = fixed_mode->clock; > } > > - max_link_clock = intel_dp_max_link_rate(intel_dp); > - max_lanes = intel_dp_max_lane_count(intel_dp); > + /* Prune the modes based on the link rate that failed */ > + if (intel_dp->link_train_failed_link_rate) { > + intel_dp->link_rate_index = intel_dp_link_rate_index(intel_dp, > + common_rates, > + intel_dp->link_train_failed_link_rate); > + if (intel_dp->link_rate_index > 0) { > + max_link_clock = common_rates[intel_dp->link_rate_index - 1]; > + max_lanes = intel_dp_max_lane_count(intel_dp); > + } else { > + /* Here we can lower the lane count, but that will be > + * added for DP Spec 1.3 > + */ > + DRM_ERROR("No Valid Mode Supported for this Link\n"); > + intel_dp->link_train_failed_link_rate = 0; > + intel_dp->link_rate_index = -1; > + intel_dp->link_train_failed = false; > + } > + } else { > + max_link_clock = intel_dp_max_link_rate(intel_dp); > + max_lanes = intel_dp_max_lane_count(intel_dp); > + } > > max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes); > mode_rate = intel_dp_link_required(target_clock, 18); > @@ -1619,6 +1639,14 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp, > if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) > return false; > > + /* Fall back to lower link rate in case of failure in previous modeset */ > + if (intel_dp->link_train_failed_link_rate) { > + min_lane_count = max_lane_count; > + min_clock = max_clock = intel_dp->link_rate_index - 1; > + intel_dp->link_train_failed_link_rate = 0; > + intel_dp->link_rate_index = -1; > + } > + > DRM_DEBUG_KMS("DP link computation with max lane count %i " > "max bw %d pixel clock %iKHz\n", > max_lane_count, common_rates[max_clock], > @@ -5689,6 +5717,39 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > return false; > } > > +static void intel_dp_modeset_retry_work_fn(struct work_struct *work) > +{ > + struct drm_connector *connector; > + struct intel_dp *intel_dp; > + struct drm_display_mode *mode; > + bool verbose_prune = true; > + bool reprobe = false; > + > + connector = container_of(work, typeof(*connector), > + i915_modeset_retry_work); > + intel_dp = intel_attached_dp(connector); > + > + /* Grab the locks before changing connector property*/ > + mutex_lock(&connector->dev->mode_config.mutex); > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, > + connector->name); > + list_for_each_entry(mode, &connector->modes, head) { > + mode->status = intel_dp_mode_valid(connector, > + mode); > + if (mode->status != MODE_OK) > + reprobe = true; > + } > + drm_mode_prune_invalid(connector->dev, &connector->modes, > + verbose_prune); > + mutex_unlock(&connector->dev->mode_config.mutex); > + if (reprobe) { > + /* Send Hotplug uevent so userspace can reprobe */ > + drm_kms_helper_hotplug_event(connector->dev); > + } > + if (intel_dp->link_train_failed) > + drm_atomic_helper_connector_modeset(connector); > +} > + > bool > intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > struct intel_connector *intel_connector) > @@ -5701,6 +5762,10 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > enum port port = intel_dig_port->port; > int type; > > + /* Initialize the work for modeset in case of link train failure */ > + INIT_WORK(&connector->i915_modeset_retry_work, > + intel_dp_modeset_retry_work_fn); > + > if (WARN(intel_dig_port->max_lanes < 1, > "Not enough lanes (%d) for DP on port %c\n", > intel_dig_port->max_lanes, port_name(port))) > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c > index 0048b52..10f81ab 100644 > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c > @@ -310,9 +310,15 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp) > DP_TRAINING_PATTERN_DISABLE); > } > > -void > +bool > intel_dp_start_link_train(struct intel_dp *intel_dp) > { > - intel_dp_link_training_clock_recovery(intel_dp); > - intel_dp_link_training_channel_equalization(intel_dp); > + bool ret; > + > + if (intel_dp_link_training_clock_recovery(intel_dp)) { > + ret = intel_dp_link_training_channel_equalization(intel_dp); > + if (ret) > + return true; > + } > + return false; > } > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 4e90b07..d3fcffc 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -890,6 +890,10 @@ struct intel_dp { > uint32_t DP; > int link_rate; > uint8_t lane_count; > + int link_train_failed_link_rate; > + uint8_t link_train_failed_lane_count; > + int link_rate_index; > + bool link_train_failed; > uint8_t sink_count; > bool link_mst; > bool has_audio; > @@ -1394,7 +1398,7 @@ bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > void intel_dp_set_link_params(struct intel_dp *intel_dp, > int link_rate, uint8_t lane_count, > bool link_mst); > -void intel_dp_start_link_train(struct intel_dp *intel_dp); > +bool intel_dp_start_link_train(struct intel_dp *intel_dp); > void intel_dp_stop_link_train(struct intel_dp *intel_dp); > void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode); > void intel_dp_encoder_reset(struct drm_encoder *encoder); > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 5/5] drm/i915: Link Rate fallback on Link training failure 2016-10-21 23:45 ` [PATCH 5/5] drm/i915: Link Rate fallback on Link training failure Manasi Navare 2016-10-24 17:53 ` Jim Bride @ 2016-10-25 6:23 ` Pandiyan, Dhinakaran 2016-10-25 18:32 ` Manasi Navare 2016-10-25 12:17 ` Jani Nikula 2 siblings, 1 reply; 33+ messages in thread From: Pandiyan, Dhinakaran @ 2016-10-25 6:23 UTC (permalink / raw) To: Navare, Manasi D; +Cc: Vetter, Daniel, intel-gfx On Fri, 2016-10-21 at 16:45 -0700, Manasi Navare wrote: > If link training at a link rate optimal for a particular > mode fails during modeset's atomic commit phase, then we > let the modeset complete and then retry. We save the link rate > value at which link training failed and use a lower link rate > to prune the modes. It will redo the modeset on the current mode > at lower link rate or if the current mode gets pruned due to lower > link constraints then, it will send a hotplug uevent for userspace > to handle it. > > This is also required to pass DP CTS tests 4.3.1.3, 4.3.1.4, > 4.3.1.6. > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 15 +++++- > drivers/gpu/drm/i915/intel_dp.c | 69 ++++++++++++++++++++++++++- > drivers/gpu/drm/i915/intel_dp_link_training.c | 12 +++-- > drivers/gpu/drm/i915/intel_drv.h | 6 ++- > 4 files changed, 95 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index fb18d69..451433b 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -1712,6 +1712,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > enum port port = intel_ddi_get_encoder_port(encoder); > + struct intel_connector *intel_connector = intel_dp->attached_connector; > + struct drm_connector *connector = &intel_connector->base; > > intel_dp_set_link_params(intel_dp, link_rate, lane_count, > link_mst); > @@ -1722,7 +1724,18 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > intel_prepare_dp_ddi_buffers(encoder); > intel_ddi_init_dp_buf_reg(encoder); > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); > - intel_dp_start_link_train(intel_dp); > + if (!intel_dp_start_link_train(intel_dp)) { > + DRM_ERROR("Link Training failed at link rate = %d, lane count = %d\n", > + link_rate, lane_count); > + intel_dp->link_train_failed = true; > + intel_dp->link_train_failed_link_rate = link_rate; > + intel_dp->link_train_failed_lane_count = lane_count; > + /* Schedule a Hotplug Uevent to userspace to start modeset */ > + schedule_work(&connector->i915_modeset_retry_work); > + } else { > + intel_dp->link_train_failed = false; > + } > + > if (port != PORT_A || INTEL_GEN(dev_priv) >= 9) > intel_dp_stop_link_train(intel_dp); > } > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index c192e18..5d5f4a7 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -313,6 +313,7 @@ static int intel_dp_link_rate_index(struct intel_dp *intel_dp, > int target_clock = mode->clock; > int max_rate, mode_rate, max_lanes, max_link_clock; > int max_dotclk; > + int common_rates[DP_MAX_SUPPORTED_RATES] = {}; > > max_dotclk = intel_dp_downstream_max_dotclock(intel_dp); > > @@ -326,8 +327,27 @@ static int intel_dp_link_rate_index(struct intel_dp *intel_dp, > target_clock = fixed_mode->clock; > } > > - max_link_clock = intel_dp_max_link_rate(intel_dp); > - max_lanes = intel_dp_max_lane_count(intel_dp); > + /* Prune the modes based on the link rate that failed */ > + if (intel_dp->link_train_failed_link_rate) { Shouldn't the bool link_train_failed that you are adding be used here? The naming indicates a check like this should use that. > + intel_dp->link_rate_index = intel_dp_link_rate_index(intel_dp, > + common_rates, > + intel_dp->link_train_failed_link_rate); > + if (intel_dp->link_rate_index > 0) { > + max_link_clock = common_rates[intel_dp->link_rate_index - 1]; > + max_lanes = intel_dp_max_lane_count(intel_dp); > + } else { > + /* Here we can lower the lane count, but that will be > + * added for DP Spec 1.3 > + */ > + DRM_ERROR("No Valid Mode Supported for this Link\n"); > + intel_dp->link_train_failed_link_rate = 0; > + intel_dp->link_rate_index = -1; > + intel_dp->link_train_failed = false; 1. You are not setting max_link_clock and max_lanes here. Won't this end up computing the max_rate based on uninitialized locals? 2. Also, what is the correct return value in this case? 3. You are resetting all values when a link rate cannot be found, what link rate is the subsequent modeset expected to start from? -DK > + } > + } else { > + max_link_clock = intel_dp_max_link_rate(intel_dp); > + max_lanes = intel_dp_max_lane_count(intel_dp); > + } > > max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes); > mode_rate = intel_dp_link_required(target_clock, 18); > @@ -1619,6 +1639,14 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp, > if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) > return false; > > + /* Fall back to lower link rate in case of failure in previous modeset */ > + if (intel_dp->link_train_failed_link_rate) { > + min_lane_count = max_lane_count; > + min_clock = max_clock = intel_dp->link_rate_index - 1; 1. Why is min_clock same as max_clock? This forces the clock to a value instead of iterating it based on the mode rate. 2. Why is min_lane_count set to max_lane_count? Is this done to be Spec compliant? 3. What about bpp? Looks like the we will still be iterating over bpp to find the link rate. -DK > + intel_dp->link_train_failed_link_rate = 0; > + intel_dp->link_rate_index = -1; > + } > + > DRM_DEBUG_KMS("DP link computation with max lane count %i " > "max bw %d pixel clock %iKHz\n", > max_lane_count, common_rates[max_clock], > @@ -5689,6 +5717,39 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > return false; > } > > +static void intel_dp_modeset_retry_work_fn(struct work_struct *work) > +{ > + struct drm_connector *connector; > + struct intel_dp *intel_dp; > + struct drm_display_mode *mode; > + bool verbose_prune = true; > + bool reprobe = false; > + > + connector = container_of(work, typeof(*connector), > + i915_modeset_retry_work); > + intel_dp = intel_attached_dp(connector); > + > + /* Grab the locks before changing connector property*/ > + mutex_lock(&connector->dev->mode_config.mutex); > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, > + connector->name); > + list_for_each_entry(mode, &connector->modes, head) { > + mode->status = intel_dp_mode_valid(connector, > + mode); > + if (mode->status != MODE_OK) > + reprobe = true; > + } > + drm_mode_prune_invalid(connector->dev, &connector->modes, > + verbose_prune); > + mutex_unlock(&connector->dev->mode_config.mutex); > + if (reprobe) { > + /* Send Hotplug uevent so userspace can reprobe */ > + drm_kms_helper_hotplug_event(connector->dev); > + } > + if (intel_dp->link_train_failed) > + drm_atomic_helper_connector_modeset(connector); > +} > + > bool > intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > struct intel_connector *intel_connector) > @@ -5701,6 +5762,10 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > enum port port = intel_dig_port->port; > int type; > > + /* Initialize the work for modeset in case of link train failure */ > + INIT_WORK(&connector->i915_modeset_retry_work, > + intel_dp_modeset_retry_work_fn); > + > if (WARN(intel_dig_port->max_lanes < 1, > "Not enough lanes (%d) for DP on port %c\n", > intel_dig_port->max_lanes, port_name(port))) > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c > index 0048b52..10f81ab 100644 > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c > @@ -310,9 +310,15 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp) > DP_TRAINING_PATTERN_DISABLE); > } > > -void > +bool > intel_dp_start_link_train(struct intel_dp *intel_dp) > { > - intel_dp_link_training_clock_recovery(intel_dp); > - intel_dp_link_training_channel_equalization(intel_dp); > + bool ret; > + > + if (intel_dp_link_training_clock_recovery(intel_dp)) { > + ret = intel_dp_link_training_channel_equalization(intel_dp); > + if (ret) > + return true; > + } > + return false; > } > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 4e90b07..d3fcffc 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -890,6 +890,10 @@ struct intel_dp { > uint32_t DP; > int link_rate; > uint8_t lane_count; > + int link_train_failed_link_rate; > + uint8_t link_train_failed_lane_count; > + int link_rate_index; > + bool link_train_failed; > uint8_t sink_count; > bool link_mst; > bool has_audio; > @@ -1394,7 +1398,7 @@ bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > void intel_dp_set_link_params(struct intel_dp *intel_dp, > int link_rate, uint8_t lane_count, > bool link_mst); > -void intel_dp_start_link_train(struct intel_dp *intel_dp); > +bool intel_dp_start_link_train(struct intel_dp *intel_dp); > void intel_dp_stop_link_train(struct intel_dp *intel_dp); > void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode); > void intel_dp_encoder_reset(struct drm_encoder *encoder); _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 5/5] drm/i915: Link Rate fallback on Link training failure 2016-10-25 6:23 ` Pandiyan, Dhinakaran @ 2016-10-25 18:32 ` Manasi Navare 0 siblings, 0 replies; 33+ messages in thread From: Manasi Navare @ 2016-10-25 18:32 UTC (permalink / raw) To: Pandiyan, Dhinakaran; +Cc: Vetter, Daniel, intel-gfx On Mon, Oct 24, 2016 at 11:23:39PM -0700, Pandiyan, Dhinakaran wrote: > On Fri, 2016-10-21 at 16:45 -0700, Manasi Navare wrote: > > If link training at a link rate optimal for a particular > > mode fails during modeset's atomic commit phase, then we > > let the modeset complete and then retry. We save the link rate > > value at which link training failed and use a lower link rate > > to prune the modes. It will redo the modeset on the current mode > > at lower link rate or if the current mode gets pruned due to lower > > link constraints then, it will send a hotplug uevent for userspace > > to handle it. > > > > This is also required to pass DP CTS tests 4.3.1.3, 4.3.1.4, > > 4.3.1.6. > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 15 +++++- > > drivers/gpu/drm/i915/intel_dp.c | 69 ++++++++++++++++++++++++++- > > drivers/gpu/drm/i915/intel_dp_link_training.c | 12 +++-- > > drivers/gpu/drm/i915/intel_drv.h | 6 ++- > > 4 files changed, 95 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > index fb18d69..451433b 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -1712,6 +1712,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > > struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > > enum port port = intel_ddi_get_encoder_port(encoder); > > + struct intel_connector *intel_connector = intel_dp->attached_connector; > > + struct drm_connector *connector = &intel_connector->base; > > > > intel_dp_set_link_params(intel_dp, link_rate, lane_count, > > link_mst); > > @@ -1722,7 +1724,18 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > > intel_prepare_dp_ddi_buffers(encoder); > > intel_ddi_init_dp_buf_reg(encoder); > > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); > > - intel_dp_start_link_train(intel_dp); > > + if (!intel_dp_start_link_train(intel_dp)) { > > + DRM_ERROR("Link Training failed at link rate = %d, lane count = %d\n", > > + link_rate, lane_count); > > + intel_dp->link_train_failed = true; > > + intel_dp->link_train_failed_link_rate = link_rate; > > + intel_dp->link_train_failed_lane_count = lane_count; > > + /* Schedule a Hotplug Uevent to userspace to start modeset */ > > + schedule_work(&connector->i915_modeset_retry_work); > > + } else { > > + intel_dp->link_train_failed = false; > > + } > > + > > if (port != PORT_A || INTEL_GEN(dev_priv) >= 9) > > intel_dp_stop_link_train(intel_dp); > > } > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index c192e18..5d5f4a7 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -313,6 +313,7 @@ static int intel_dp_link_rate_index(struct intel_dp *intel_dp, > > int target_clock = mode->clock; > > int max_rate, mode_rate, max_lanes, max_link_clock; > > int max_dotclk; > > + int common_rates[DP_MAX_SUPPORTED_RATES] = {}; > > > > max_dotclk = intel_dp_downstream_max_dotclock(intel_dp); > > > > @@ -326,8 +327,27 @@ static int intel_dp_link_rate_index(struct intel_dp *intel_dp, > > target_clock = fixed_mode->clock; > > } > > > > - max_link_clock = intel_dp_max_link_rate(intel_dp); > > - max_lanes = intel_dp_max_lane_count(intel_dp); > > + /* Prune the modes based on the link rate that failed */ > > + if (intel_dp->link_train_failed_link_rate) { > > Shouldn't the bool link_train_failed that you are adding be used here? > The naming indicates a check like this should use that. Either that or the link_train_failed_link_rate can be used, both get initialized and reset the same time. But using link_train_failed would be more appropriate I guess, I will look at changing that. Manasi > > > > + intel_dp->link_rate_index = intel_dp_link_rate_index(intel_dp, > > + common_rates, > > + intel_dp->link_train_failed_link_rate); > > + if (intel_dp->link_rate_index > 0) { > > + max_link_clock = common_rates[intel_dp->link_rate_index - 1]; > > + max_lanes = intel_dp_max_lane_count(intel_dp); > > + } else { > > + /* Here we can lower the lane count, but that will be > > + * added for DP Spec 1.3 > > + */ > > + DRM_ERROR("No Valid Mode Supported for this Link\n"); > > + intel_dp->link_train_failed_link_rate = 0; > > + intel_dp->link_rate_index = -1; > > + intel_dp->link_train_failed = false; > > 1. You are not setting max_link_clock and max_lanes here. Won't this end > up computing the max_rate based on uninitialized locals? > > 2. Also, what is the correct return value in this case? > > 3. You are resetting all values when a link rate cannot be found, what > link rate is the subsequent modeset expected to start from? > > > -DK The idea is to just fail the link training in this case since we cannot further reduce the link rate/lane count so it shoudl just fail the modeset and send a uevent to userspace. I think this logic will need further tuning. > > + } > > + } else { > > + max_link_clock = intel_dp_max_link_rate(intel_dp); > > + max_lanes = intel_dp_max_lane_count(intel_dp); > > + } > > > > max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes); > > mode_rate = intel_dp_link_required(target_clock, 18); > > @@ -1619,6 +1639,14 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp, > > if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) > > return false; > > > > + /* Fall back to lower link rate in case of failure in previous modeset */ > > + if (intel_dp->link_train_failed_link_rate) { > > + min_lane_count = max_lane_count; > > + min_clock = max_clock = intel_dp->link_rate_index - 1; > > 1. Why is min_clock same as max_clock? This forces the clock to a value > instead of iterating it based on the mode rate. > > 2. Why is min_lane_count set to max_lane_count? Is this done to be Spec > compliant? > > 3. What about bpp? Looks like the we will still be iterating over bpp to > find the link rate. > > -DK > > + intel_dp->link_train_failed_link_rate = 0; > > + intel_dp->link_rate_index = -1; > > + } > > + > > DRM_DEBUG_KMS("DP link computation with max lane count %i " > > "max bw %d pixel clock %iKHz\n", > > max_lane_count, common_rates[max_clock], > > @@ -5689,6 +5717,39 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > > return false; > > } > > > > +static void intel_dp_modeset_retry_work_fn(struct work_struct *work) > > +{ > > + struct drm_connector *connector; > > + struct intel_dp *intel_dp; > > + struct drm_display_mode *mode; > > + bool verbose_prune = true; > > + bool reprobe = false; > > + > > + connector = container_of(work, typeof(*connector), > > + i915_modeset_retry_work); > > + intel_dp = intel_attached_dp(connector); > > + > > + /* Grab the locks before changing connector property*/ > > + mutex_lock(&connector->dev->mode_config.mutex); > > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, > > + connector->name); > > + list_for_each_entry(mode, &connector->modes, head) { > > + mode->status = intel_dp_mode_valid(connector, > > + mode); > > + if (mode->status != MODE_OK) > > + reprobe = true; > > + } > > + drm_mode_prune_invalid(connector->dev, &connector->modes, > > + verbose_prune); > > + mutex_unlock(&connector->dev->mode_config.mutex); > > + if (reprobe) { > > + /* Send Hotplug uevent so userspace can reprobe */ > > + drm_kms_helper_hotplug_event(connector->dev); > > + } > > + if (intel_dp->link_train_failed) > > + drm_atomic_helper_connector_modeset(connector); > > +} > > + > > bool > > intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > > struct intel_connector *intel_connector) > > @@ -5701,6 +5762,10 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > > enum port port = intel_dig_port->port; > > int type; > > > > + /* Initialize the work for modeset in case of link train failure */ > > + INIT_WORK(&connector->i915_modeset_retry_work, > > + intel_dp_modeset_retry_work_fn); > > + > > if (WARN(intel_dig_port->max_lanes < 1, > > "Not enough lanes (%d) for DP on port %c\n", > > intel_dig_port->max_lanes, port_name(port))) > > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c > > index 0048b52..10f81ab 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c > > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c > > @@ -310,9 +310,15 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp) > > DP_TRAINING_PATTERN_DISABLE); > > } > > > > -void > > +bool > > intel_dp_start_link_train(struct intel_dp *intel_dp) > > { > > - intel_dp_link_training_clock_recovery(intel_dp); > > - intel_dp_link_training_channel_equalization(intel_dp); > > + bool ret; > > + > > + if (intel_dp_link_training_clock_recovery(intel_dp)) { > > + ret = intel_dp_link_training_channel_equalization(intel_dp); > > + if (ret) > > + return true; > > + } > > + return false; > > } > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index 4e90b07..d3fcffc 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -890,6 +890,10 @@ struct intel_dp { > > uint32_t DP; > > int link_rate; > > uint8_t lane_count; > > + int link_train_failed_link_rate; > > + uint8_t link_train_failed_lane_count; > > + int link_rate_index; > > + bool link_train_failed; > > uint8_t sink_count; > > bool link_mst; > > bool has_audio; > > @@ -1394,7 +1398,7 @@ bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > > void intel_dp_set_link_params(struct intel_dp *intel_dp, > > int link_rate, uint8_t lane_count, > > bool link_mst); > > -void intel_dp_start_link_train(struct intel_dp *intel_dp); > > +bool intel_dp_start_link_train(struct intel_dp *intel_dp); > > void intel_dp_stop_link_train(struct intel_dp *intel_dp); > > void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode); > > void intel_dp_encoder_reset(struct drm_encoder *encoder); > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 5/5] drm/i915: Link Rate fallback on Link training failure 2016-10-21 23:45 ` [PATCH 5/5] drm/i915: Link Rate fallback on Link training failure Manasi Navare 2016-10-24 17:53 ` Jim Bride 2016-10-25 6:23 ` Pandiyan, Dhinakaran @ 2016-10-25 12:17 ` Jani Nikula 2016-10-25 18:00 ` Jim Bride 2016-10-25 18:37 ` Manasi Navare 2 siblings, 2 replies; 33+ messages in thread From: Jani Nikula @ 2016-10-25 12:17 UTC (permalink / raw) To: Manasi Navare, intel-gfx; +Cc: Daniel Vetter On Sat, 22 Oct 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > If link training at a link rate optimal for a particular > mode fails during modeset's atomic commit phase, then we > let the modeset complete and then retry. We save the link rate > value at which link training failed and use a lower link rate > to prune the modes. It will redo the modeset on the current mode > at lower link rate or if the current mode gets pruned due to lower > link constraints then, it will send a hotplug uevent for userspace > to handle it. > > This is also required to pass DP CTS tests 4.3.1.3, 4.3.1.4, > 4.3.1.6. > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Daniel Vetter <daniel.vetter@intel.com> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 15 +++++- > drivers/gpu/drm/i915/intel_dp.c | 69 ++++++++++++++++++++++++++- > drivers/gpu/drm/i915/intel_dp_link_training.c | 12 +++-- > drivers/gpu/drm/i915/intel_drv.h | 6 ++- > 4 files changed, 95 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index fb18d69..451433b 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -1712,6 +1712,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > enum port port = intel_ddi_get_encoder_port(encoder); > + struct intel_connector *intel_connector = intel_dp->attached_connector; > + struct drm_connector *connector = &intel_connector->base; > > intel_dp_set_link_params(intel_dp, link_rate, lane_count, > link_mst); > @@ -1722,7 +1724,18 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > intel_prepare_dp_ddi_buffers(encoder); > intel_ddi_init_dp_buf_reg(encoder); > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); > - intel_dp_start_link_train(intel_dp); > + if (!intel_dp_start_link_train(intel_dp)) { > + DRM_ERROR("Link Training failed at link rate = %d, lane count = %d\n", > + link_rate, lane_count); > + intel_dp->link_train_failed = true; > + intel_dp->link_train_failed_link_rate = link_rate; > + intel_dp->link_train_failed_lane_count = lane_count; I think eventually you'll need to store a list (array) of failing link rate, lane count pairs, not just the last that failed. Now you restrict the link config computation to only reducing the link rate. But currently (for whatever reason, it's flip-flopped too many times) we start with wide & slow, meaning that in many cases we've already exhausted the option to go slower. If optimal fails, maybe we need to try narrow & fast instead. BR, Jani. > + /* Schedule a Hotplug Uevent to userspace to start modeset */ > + schedule_work(&connector->i915_modeset_retry_work); > + } else { > + intel_dp->link_train_failed = false; > + } > + > if (port != PORT_A || INTEL_GEN(dev_priv) >= 9) > intel_dp_stop_link_train(intel_dp); > } > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index c192e18..5d5f4a7 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -313,6 +313,7 @@ static int intel_dp_link_rate_index(struct intel_dp *intel_dp, > int target_clock = mode->clock; > int max_rate, mode_rate, max_lanes, max_link_clock; > int max_dotclk; > + int common_rates[DP_MAX_SUPPORTED_RATES] = {}; > > max_dotclk = intel_dp_downstream_max_dotclock(intel_dp); > > @@ -326,8 +327,27 @@ static int intel_dp_link_rate_index(struct intel_dp *intel_dp, > target_clock = fixed_mode->clock; > } > > - max_link_clock = intel_dp_max_link_rate(intel_dp); > - max_lanes = intel_dp_max_lane_count(intel_dp); > + /* Prune the modes based on the link rate that failed */ > + if (intel_dp->link_train_failed_link_rate) { > + intel_dp->link_rate_index = intel_dp_link_rate_index(intel_dp, > + common_rates, > + intel_dp->link_train_failed_link_rate); > + if (intel_dp->link_rate_index > 0) { > + max_link_clock = common_rates[intel_dp->link_rate_index - 1]; > + max_lanes = intel_dp_max_lane_count(intel_dp); > + } else { > + /* Here we can lower the lane count, but that will be > + * added for DP Spec 1.3 > + */ > + DRM_ERROR("No Valid Mode Supported for this Link\n"); > + intel_dp->link_train_failed_link_rate = 0; > + intel_dp->link_rate_index = -1; > + intel_dp->link_train_failed = false; > + } > + } else { > + max_link_clock = intel_dp_max_link_rate(intel_dp); > + max_lanes = intel_dp_max_lane_count(intel_dp); > + } > > max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes); > mode_rate = intel_dp_link_required(target_clock, 18); > @@ -1619,6 +1639,14 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp, > if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) > return false; > > + /* Fall back to lower link rate in case of failure in previous modeset */ > + if (intel_dp->link_train_failed_link_rate) { > + min_lane_count = max_lane_count; > + min_clock = max_clock = intel_dp->link_rate_index - 1; > + intel_dp->link_train_failed_link_rate = 0; > + intel_dp->link_rate_index = -1; > + } > + > DRM_DEBUG_KMS("DP link computation with max lane count %i " > "max bw %d pixel clock %iKHz\n", > max_lane_count, common_rates[max_clock], > @@ -5689,6 +5717,39 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > return false; > } > > +static void intel_dp_modeset_retry_work_fn(struct work_struct *work) > +{ > + struct drm_connector *connector; > + struct intel_dp *intel_dp; > + struct drm_display_mode *mode; > + bool verbose_prune = true; > + bool reprobe = false; > + > + connector = container_of(work, typeof(*connector), > + i915_modeset_retry_work); > + intel_dp = intel_attached_dp(connector); > + > + /* Grab the locks before changing connector property*/ > + mutex_lock(&connector->dev->mode_config.mutex); > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, > + connector->name); > + list_for_each_entry(mode, &connector->modes, head) { > + mode->status = intel_dp_mode_valid(connector, > + mode); > + if (mode->status != MODE_OK) > + reprobe = true; > + } > + drm_mode_prune_invalid(connector->dev, &connector->modes, > + verbose_prune); > + mutex_unlock(&connector->dev->mode_config.mutex); > + if (reprobe) { > + /* Send Hotplug uevent so userspace can reprobe */ > + drm_kms_helper_hotplug_event(connector->dev); > + } > + if (intel_dp->link_train_failed) > + drm_atomic_helper_connector_modeset(connector); > +} > + > bool > intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > struct intel_connector *intel_connector) > @@ -5701,6 +5762,10 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > enum port port = intel_dig_port->port; > int type; > > + /* Initialize the work for modeset in case of link train failure */ > + INIT_WORK(&connector->i915_modeset_retry_work, > + intel_dp_modeset_retry_work_fn); > + > if (WARN(intel_dig_port->max_lanes < 1, > "Not enough lanes (%d) for DP on port %c\n", > intel_dig_port->max_lanes, port_name(port))) > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c > index 0048b52..10f81ab 100644 > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c > @@ -310,9 +310,15 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp) > DP_TRAINING_PATTERN_DISABLE); > } > > -void > +bool > intel_dp_start_link_train(struct intel_dp *intel_dp) > { > - intel_dp_link_training_clock_recovery(intel_dp); > - intel_dp_link_training_channel_equalization(intel_dp); > + bool ret; > + > + if (intel_dp_link_training_clock_recovery(intel_dp)) { > + ret = intel_dp_link_training_channel_equalization(intel_dp); > + if (ret) > + return true; > + } > + return false; > } > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 4e90b07..d3fcffc 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -890,6 +890,10 @@ struct intel_dp { > uint32_t DP; > int link_rate; > uint8_t lane_count; > + int link_train_failed_link_rate; > + uint8_t link_train_failed_lane_count; > + int link_rate_index; > + bool link_train_failed; > uint8_t sink_count; > bool link_mst; > bool has_audio; > @@ -1394,7 +1398,7 @@ bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > void intel_dp_set_link_params(struct intel_dp *intel_dp, > int link_rate, uint8_t lane_count, > bool link_mst); > -void intel_dp_start_link_train(struct intel_dp *intel_dp); > +bool intel_dp_start_link_train(struct intel_dp *intel_dp); > void intel_dp_stop_link_train(struct intel_dp *intel_dp); > void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode); > void intel_dp_encoder_reset(struct drm_encoder *encoder); -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 5/5] drm/i915: Link Rate fallback on Link training failure 2016-10-25 12:17 ` Jani Nikula @ 2016-10-25 18:00 ` Jim Bride 2016-10-25 18:37 ` Manasi Navare 1 sibling, 0 replies; 33+ messages in thread From: Jim Bride @ 2016-10-25 18:00 UTC (permalink / raw) To: Jani Nikula; +Cc: Daniel Vetter, intel-gfx On Tue, Oct 25, 2016 at 03:17:47PM +0300, Jani Nikula wrote: > On Sat, 22 Oct 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > > If link training at a link rate optimal for a particular > > mode fails during modeset's atomic commit phase, then we > > let the modeset complete and then retry. We save the link rate > > value at which link training failed and use a lower link rate > > to prune the modes. It will redo the modeset on the current mode > > at lower link rate or if the current mode gets pruned due to lower > > link constraints then, it will send a hotplug uevent for userspace > > to handle it. > > > > This is also required to pass DP CTS tests 4.3.1.3, 4.3.1.4, > > 4.3.1.6. > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 15 +++++- > > drivers/gpu/drm/i915/intel_dp.c | 69 ++++++++++++++++++++++++++- > > drivers/gpu/drm/i915/intel_dp_link_training.c | 12 +++-- > > drivers/gpu/drm/i915/intel_drv.h | 6 ++- > > 4 files changed, 95 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > index fb18d69..451433b 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -1712,6 +1712,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > > struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > > enum port port = intel_ddi_get_encoder_port(encoder); > > + struct intel_connector *intel_connector = intel_dp->attached_connector; > > + struct drm_connector *connector = &intel_connector->base; > > > > intel_dp_set_link_params(intel_dp, link_rate, lane_count, > > link_mst); > > @@ -1722,7 +1724,18 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > > intel_prepare_dp_ddi_buffers(encoder); > > intel_ddi_init_dp_buf_reg(encoder); > > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); > > - intel_dp_start_link_train(intel_dp); > > + if (!intel_dp_start_link_train(intel_dp)) { > > + DRM_ERROR("Link Training failed at link rate = %d, lane count = %d\n", > > + link_rate, lane_count); > > + intel_dp->link_train_failed = true; > > + intel_dp->link_train_failed_link_rate = link_rate; > > + intel_dp->link_train_failed_lane_count = lane_count; > > I think eventually you'll need to store a list (array) of failing link > rate, lane count pairs, not just the last that failed. Now you restrict > the link config computation to only reducing the link rate. But > currently (for whatever reason, it's flip-flopped too many times) we > start with wide & slow, meaning that in many cases we've already > exhausted the option to go slower. If optimal fails, maybe we need to > try narrow & fast instead. The DP spec specifically calls out that lane count shouldn't be reduced until all speeds with the current lane configuration fail. Even if we start at an "optimal" configuration I believe we still need to follow the reduction pattern that the spec calls out. Jim > > BR, > Jani. > > > > + /* Schedule a Hotplug Uevent to userspace to start modeset */ > > + schedule_work(&connector->i915_modeset_retry_work); > > + } else { > > + intel_dp->link_train_failed = false; > > + } > > + > > if (port != PORT_A || INTEL_GEN(dev_priv) >= 9) > > intel_dp_stop_link_train(intel_dp); > > } > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index c192e18..5d5f4a7 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -313,6 +313,7 @@ static int intel_dp_link_rate_index(struct intel_dp *intel_dp, > > int target_clock = mode->clock; > > int max_rate, mode_rate, max_lanes, max_link_clock; > > int max_dotclk; > > + int common_rates[DP_MAX_SUPPORTED_RATES] = {}; > > > > max_dotclk = intel_dp_downstream_max_dotclock(intel_dp); > > > > @@ -326,8 +327,27 @@ static int intel_dp_link_rate_index(struct intel_dp *intel_dp, > > target_clock = fixed_mode->clock; > > } > > > > - max_link_clock = intel_dp_max_link_rate(intel_dp); > > - max_lanes = intel_dp_max_lane_count(intel_dp); > > + /* Prune the modes based on the link rate that failed */ > > + if (intel_dp->link_train_failed_link_rate) { > > + intel_dp->link_rate_index = intel_dp_link_rate_index(intel_dp, > > + common_rates, > > + intel_dp->link_train_failed_link_rate); > > + if (intel_dp->link_rate_index > 0) { > > + max_link_clock = common_rates[intel_dp->link_rate_index - 1]; > > + max_lanes = intel_dp_max_lane_count(intel_dp); > > + } else { > > + /* Here we can lower the lane count, but that will be > > + * added for DP Spec 1.3 > > + */ > > + DRM_ERROR("No Valid Mode Supported for this Link\n"); > > + intel_dp->link_train_failed_link_rate = 0; > > + intel_dp->link_rate_index = -1; > > + intel_dp->link_train_failed = false; > > + } > > + } else { > > + max_link_clock = intel_dp_max_link_rate(intel_dp); > > + max_lanes = intel_dp_max_lane_count(intel_dp); > > + } > > > > max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes); > > mode_rate = intel_dp_link_required(target_clock, 18); > > @@ -1619,6 +1639,14 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp, > > if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) > > return false; > > > > + /* Fall back to lower link rate in case of failure in previous modeset */ > > + if (intel_dp->link_train_failed_link_rate) { > > + min_lane_count = max_lane_count; > > + min_clock = max_clock = intel_dp->link_rate_index - 1; > > + intel_dp->link_train_failed_link_rate = 0; > > + intel_dp->link_rate_index = -1; > > + } > > + > > DRM_DEBUG_KMS("DP link computation with max lane count %i " > > "max bw %d pixel clock %iKHz\n", > > max_lane_count, common_rates[max_clock], > > @@ -5689,6 +5717,39 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > > return false; > > } > > > > +static void intel_dp_modeset_retry_work_fn(struct work_struct *work) > > +{ > > + struct drm_connector *connector; > > + struct intel_dp *intel_dp; > > + struct drm_display_mode *mode; > > + bool verbose_prune = true; > > + bool reprobe = false; > > + > > + connector = container_of(work, typeof(*connector), > > + i915_modeset_retry_work); > > + intel_dp = intel_attached_dp(connector); > > + > > + /* Grab the locks before changing connector property*/ > > + mutex_lock(&connector->dev->mode_config.mutex); > > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, > > + connector->name); > > + list_for_each_entry(mode, &connector->modes, head) { > > + mode->status = intel_dp_mode_valid(connector, > > + mode); > > + if (mode->status != MODE_OK) > > + reprobe = true; > > + } > > + drm_mode_prune_invalid(connector->dev, &connector->modes, > > + verbose_prune); > > + mutex_unlock(&connector->dev->mode_config.mutex); > > + if (reprobe) { > > + /* Send Hotplug uevent so userspace can reprobe */ > > + drm_kms_helper_hotplug_event(connector->dev); > > + } > > + if (intel_dp->link_train_failed) > > + drm_atomic_helper_connector_modeset(connector); > > +} > > + > > bool > > intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > > struct intel_connector *intel_connector) > > @@ -5701,6 +5762,10 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > > enum port port = intel_dig_port->port; > > int type; > > > > + /* Initialize the work for modeset in case of link train failure */ > > + INIT_WORK(&connector->i915_modeset_retry_work, > > + intel_dp_modeset_retry_work_fn); > > + > > if (WARN(intel_dig_port->max_lanes < 1, > > "Not enough lanes (%d) for DP on port %c\n", > > intel_dig_port->max_lanes, port_name(port))) > > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c > > index 0048b52..10f81ab 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c > > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c > > @@ -310,9 +310,15 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp) > > DP_TRAINING_PATTERN_DISABLE); > > } > > > > -void > > +bool > > intel_dp_start_link_train(struct intel_dp *intel_dp) > > { > > - intel_dp_link_training_clock_recovery(intel_dp); > > - intel_dp_link_training_channel_equalization(intel_dp); > > + bool ret; > > + > > + if (intel_dp_link_training_clock_recovery(intel_dp)) { > > + ret = intel_dp_link_training_channel_equalization(intel_dp); > > + if (ret) > > + return true; > > + } > > + return false; > > } > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index 4e90b07..d3fcffc 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -890,6 +890,10 @@ struct intel_dp { > > uint32_t DP; > > int link_rate; > > uint8_t lane_count; > > + int link_train_failed_link_rate; > > + uint8_t link_train_failed_lane_count; > > + int link_rate_index; > > + bool link_train_failed; > > uint8_t sink_count; > > bool link_mst; > > bool has_audio; > > @@ -1394,7 +1398,7 @@ bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > > void intel_dp_set_link_params(struct intel_dp *intel_dp, > > int link_rate, uint8_t lane_count, > > bool link_mst); > > -void intel_dp_start_link_train(struct intel_dp *intel_dp); > > +bool intel_dp_start_link_train(struct intel_dp *intel_dp); > > void intel_dp_stop_link_train(struct intel_dp *intel_dp); > > void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode); > > void intel_dp_encoder_reset(struct drm_encoder *encoder); > > -- > Jani Nikula, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 5/5] drm/i915: Link Rate fallback on Link training failure 2016-10-25 12:17 ` Jani Nikula 2016-10-25 18:00 ` Jim Bride @ 2016-10-25 18:37 ` Manasi Navare 1 sibling, 0 replies; 33+ messages in thread From: Manasi Navare @ 2016-10-25 18:37 UTC (permalink / raw) To: Jani Nikula; +Cc: Daniel Vetter, intel-gfx On Tue, Oct 25, 2016 at 03:17:47PM +0300, Jani Nikula wrote: > On Sat, 22 Oct 2016, Manasi Navare <manasi.d.navare@intel.com> wrote: > > If link training at a link rate optimal for a particular > > mode fails during modeset's atomic commit phase, then we > > let the modeset complete and then retry. We save the link rate > > value at which link training failed and use a lower link rate > > to prune the modes. It will redo the modeset on the current mode > > at lower link rate or if the current mode gets pruned due to lower > > link constraints then, it will send a hotplug uevent for userspace > > to handle it. > > > > This is also required to pass DP CTS tests 4.3.1.3, 4.3.1.4, > > 4.3.1.6. > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > Cc: Daniel Vetter <daniel.vetter@intel.com> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com> > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 15 +++++- > > drivers/gpu/drm/i915/intel_dp.c | 69 ++++++++++++++++++++++++++- > > drivers/gpu/drm/i915/intel_dp_link_training.c | 12 +++-- > > drivers/gpu/drm/i915/intel_drv.h | 6 ++- > > 4 files changed, 95 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > index fb18d69..451433b 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -1712,6 +1712,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > > struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > > enum port port = intel_ddi_get_encoder_port(encoder); > > + struct intel_connector *intel_connector = intel_dp->attached_connector; > > + struct drm_connector *connector = &intel_connector->base; > > > > intel_dp_set_link_params(intel_dp, link_rate, lane_count, > > link_mst); > > @@ -1722,7 +1724,18 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > > intel_prepare_dp_ddi_buffers(encoder); > > intel_ddi_init_dp_buf_reg(encoder); > > intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); > > - intel_dp_start_link_train(intel_dp); > > + if (!intel_dp_start_link_train(intel_dp)) { > > + DRM_ERROR("Link Training failed at link rate = %d, lane count = %d\n", > > + link_rate, lane_count); > > + intel_dp->link_train_failed = true; > > + intel_dp->link_train_failed_link_rate = link_rate; > > + intel_dp->link_train_failed_lane_count = lane_count; > > I think eventually you'll need to store a list (array) of failing link > rate, lane count pairs, not just the last that failed. Now you restrict > the link config computation to only reducing the link rate. But > currently (for whatever reason, it's flip-flopped too many times) we > start with wide & slow, meaning that in many cases we've already > exhausted the option to go slower. If optimal fails, maybe we need to > try narrow & fast instead. > > BR, > Jani. > > So the DP 1.2 spec says that we need to first try reducing the link rate all the way to RBR without reducing the lane count and if that fails then just fail the link training. DP spec 1.3 also reduces the lane count after reducing the link rate all the way to RBR, then it should try lower lane count and highest link rate again. But I havent expanded this to include spec 1.3 yet, this can be added later. The DP compliance only follows CTS according tO DP Spec 1.2 and they would be including lane count reduction later for CTS 1.3. But in either case I dont think we need an array, at link train failure, we just need to know the failed link rate and lane count based on which the next lower link rate/lane count value can be decided here during mode validation and be used during compute config. Manasi > > + /* Schedule a Hotplug Uevent to userspace to start modeset */ > > + schedule_work(&connector->i915_modeset_retry_work); > > + } else { > > + intel_dp->link_train_failed = false; > > + } > > + > > if (port != PORT_A || INTEL_GEN(dev_priv) >= 9) > > intel_dp_stop_link_train(intel_dp); > > } > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index c192e18..5d5f4a7 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -313,6 +313,7 @@ static int intel_dp_link_rate_index(struct intel_dp *intel_dp, > > int target_clock = mode->clock; > > int max_rate, mode_rate, max_lanes, max_link_clock; > > int max_dotclk; > > + int common_rates[DP_MAX_SUPPORTED_RATES] = {}; > > > > max_dotclk = intel_dp_downstream_max_dotclock(intel_dp); > > > > @@ -326,8 +327,27 @@ static int intel_dp_link_rate_index(struct intel_dp *intel_dp, > > target_clock = fixed_mode->clock; > > } > > > > - max_link_clock = intel_dp_max_link_rate(intel_dp); > > - max_lanes = intel_dp_max_lane_count(intel_dp); > > + /* Prune the modes based on the link rate that failed */ > > + if (intel_dp->link_train_failed_link_rate) { > > + intel_dp->link_rate_index = intel_dp_link_rate_index(intel_dp, > > + common_rates, > > + intel_dp->link_train_failed_link_rate); > > + if (intel_dp->link_rate_index > 0) { > > + max_link_clock = common_rates[intel_dp->link_rate_index - 1]; > > + max_lanes = intel_dp_max_lane_count(intel_dp); > > + } else { > > + /* Here we can lower the lane count, but that will be > > + * added for DP Spec 1.3 > > + */ > > + DRM_ERROR("No Valid Mode Supported for this Link\n"); > > + intel_dp->link_train_failed_link_rate = 0; > > + intel_dp->link_rate_index = -1; > > + intel_dp->link_train_failed = false; > > + } > > + } else { > > + max_link_clock = intel_dp_max_link_rate(intel_dp); > > + max_lanes = intel_dp_max_lane_count(intel_dp); > > + } > > > > max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes); > > mode_rate = intel_dp_link_required(target_clock, 18); > > @@ -1619,6 +1639,14 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp, > > if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) > > return false; > > > > + /* Fall back to lower link rate in case of failure in previous modeset */ > > + if (intel_dp->link_train_failed_link_rate) { > > + min_lane_count = max_lane_count; > > + min_clock = max_clock = intel_dp->link_rate_index - 1; > > + intel_dp->link_train_failed_link_rate = 0; > > + intel_dp->link_rate_index = -1; > > + } > > + > > DRM_DEBUG_KMS("DP link computation with max lane count %i " > > "max bw %d pixel clock %iKHz\n", > > max_lane_count, common_rates[max_clock], > > @@ -5689,6 +5717,39 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > > return false; > > } > > > > +static void intel_dp_modeset_retry_work_fn(struct work_struct *work) > > +{ > > + struct drm_connector *connector; > > + struct intel_dp *intel_dp; > > + struct drm_display_mode *mode; > > + bool verbose_prune = true; > > + bool reprobe = false; > > + > > + connector = container_of(work, typeof(*connector), > > + i915_modeset_retry_work); > > + intel_dp = intel_attached_dp(connector); > > + > > + /* Grab the locks before changing connector property*/ > > + mutex_lock(&connector->dev->mode_config.mutex); > > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, > > + connector->name); > > + list_for_each_entry(mode, &connector->modes, head) { > > + mode->status = intel_dp_mode_valid(connector, > > + mode); > > + if (mode->status != MODE_OK) > > + reprobe = true; > > + } > > + drm_mode_prune_invalid(connector->dev, &connector->modes, > > + verbose_prune); > > + mutex_unlock(&connector->dev->mode_config.mutex); > > + if (reprobe) { > > + /* Send Hotplug uevent so userspace can reprobe */ > > + drm_kms_helper_hotplug_event(connector->dev); > > + } > > + if (intel_dp->link_train_failed) > > + drm_atomic_helper_connector_modeset(connector); > > +} > > + > > bool > > intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > > struct intel_connector *intel_connector) > > @@ -5701,6 +5762,10 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp, > > enum port port = intel_dig_port->port; > > int type; > > > > + /* Initialize the work for modeset in case of link train failure */ > > + INIT_WORK(&connector->i915_modeset_retry_work, > > + intel_dp_modeset_retry_work_fn); > > + > > if (WARN(intel_dig_port->max_lanes < 1, > > "Not enough lanes (%d) for DP on port %c\n", > > intel_dig_port->max_lanes, port_name(port))) > > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c > > index 0048b52..10f81ab 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c > > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c > > @@ -310,9 +310,15 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp) > > DP_TRAINING_PATTERN_DISABLE); > > } > > > > -void > > +bool > > intel_dp_start_link_train(struct intel_dp *intel_dp) > > { > > - intel_dp_link_training_clock_recovery(intel_dp); > > - intel_dp_link_training_channel_equalization(intel_dp); > > + bool ret; > > + > > + if (intel_dp_link_training_clock_recovery(intel_dp)) { > > + ret = intel_dp_link_training_channel_equalization(intel_dp); > > + if (ret) > > + return true; > > + } > > + return false; > > } > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index 4e90b07..d3fcffc 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -890,6 +890,10 @@ struct intel_dp { > > uint32_t DP; > > int link_rate; > > uint8_t lane_count; > > + int link_train_failed_link_rate; > > + uint8_t link_train_failed_lane_count; > > + int link_rate_index; > > + bool link_train_failed; > > uint8_t sink_count; > > bool link_mst; > > bool has_audio; > > @@ -1394,7 +1398,7 @@ bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > > void intel_dp_set_link_params(struct intel_dp *intel_dp, > > int link_rate, uint8_t lane_count, > > bool link_mst); > > -void intel_dp_start_link_train(struct intel_dp *intel_dp); > > +bool intel_dp_start_link_train(struct intel_dp *intel_dp); > > void intel_dp_stop_link_train(struct intel_dp *intel_dp); > > void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode); > > void intel_dp_encoder_reset(struct drm_encoder *encoder); > > -- > Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 33+ messages in thread
* ✗ Fi.CI.BAT: warning for Handle link training failure for DDI platforms 2016-10-21 23:45 [PATCH 0/5] Handle link training failure for DDI platforms Manasi Navare ` (4 preceding siblings ...) 2016-10-21 23:45 ` [PATCH 5/5] drm/i915: Link Rate fallback on Link training failure Manasi Navare @ 2016-10-22 0:16 ` Patchwork 5 siblings, 0 replies; 33+ messages in thread From: Patchwork @ 2016-10-22 0:16 UTC (permalink / raw) To: Navare, Manasi D; +Cc: intel-gfx == Series Details == Series: Handle link training failure for DDI platforms URL : https://patchwork.freedesktop.org/series/14192/ State : warning == Summary == Series 14192v1 Handle link training failure for DDI platforms https://patchwork.freedesktop.org/api/1.0/series/14192/revisions/1/mbox/ Test drv_module_reload_basic: skip -> PASS (fi-skl-6770hq) Test gem_exec_suspend: Subgroup basic-s3: pass -> DMESG-WARN (fi-skl-6700k) Test kms_busy: Subgroup basic-flip-default-a: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Subgroup basic-flip-default-b: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Subgroup basic-flip-default-c: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Test kms_cursor_legacy: Subgroup basic-busy-flip-before-cursor-legacy: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Subgroup basic-busy-flip-before-cursor-varying-size: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Subgroup basic-flip-after-cursor-legacy: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Subgroup basic-flip-after-cursor-varying-size: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Subgroup basic-flip-before-cursor-legacy: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Subgroup basic-flip-before-cursor-varying-size: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Test kms_flip: Subgroup basic-flip-vs-dpms: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Subgroup basic-flip-vs-modeset: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Subgroup basic-flip-vs-wf_vblank: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Subgroup basic-plain-flip: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Test kms_frontbuffer_tracking: Subgroup basic: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Test kms_pipe_crc_basic: Subgroup hang-read-crc-pipe-a: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Subgroup hang-read-crc-pipe-b: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Subgroup hang-read-crc-pipe-c: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Subgroup nonblocking-crc-pipe-a: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Subgroup nonblocking-crc-pipe-a-frame-sequence: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Subgroup nonblocking-crc-pipe-b: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Subgroup nonblocking-crc-pipe-b-frame-sequence: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Subgroup nonblocking-crc-pipe-c: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Subgroup nonblocking-crc-pipe-c-frame-sequence: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Subgroup read-crc-pipe-a: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Subgroup read-crc-pipe-a-frame-sequence: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Subgroup read-crc-pipe-b: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Subgroup read-crc-pipe-b-frame-sequence: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Subgroup read-crc-pipe-c: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) Subgroup read-crc-pipe-c-frame-sequence: pass -> DMESG-WARN (fi-skl-6700k) pass -> DMESG-WARN (fi-skl-6770hq) WARNING: Long output truncated Results at /archive/results/CI_IGT_test/Patchwork_2795/ fe893e34141bcdf5e145cc79e3b17e3b8f6b340a drm-intel-nightly: 2016y-10m-21d-20h-04m-02s UTC integration manifest d5f2db02 drm/i915: Link Rate fallback on Link training failure 8539f6e drm/i915; Add a function to return index of link rate 01ec3b0 drm/i915: Change the placement of some static functions in intel_dp.c bf4d7cf drm: Define a work struct for scheduling a uevent for modeset retry 30a31b5 drm: Add atomic helper to redo a modeset on current mode _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2016-10-25 22:38 UTC | newest] Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-10-21 23:45 [PATCH 0/5] Handle link training failure for DDI platforms Manasi Navare 2016-10-21 23:45 ` [PATCH 1/5] drm: Add atomic helper to redo a modeset on current mode Manasi Navare 2016-10-22 8:47 ` Daniel Vetter 2016-10-22 14:01 ` [Intel-gfx] " Daniel Vetter 2016-10-22 14:46 ` Ville Syrjälä 2016-10-24 6:00 ` Daniel Vetter 2016-10-24 6:12 ` Manasi Navare 2016-10-24 6:33 ` Daniel Vetter 2016-10-24 7:00 ` Manasi Navare 2016-10-24 7:12 ` Daniel Vetter 2016-10-24 18:38 ` [Intel-gfx] " Sean Paul 2016-10-25 6:35 ` Daniel Vetter 2016-10-24 22:08 ` Manasi Navare 2016-10-25 6:40 ` [Intel-gfx] " Daniel Vetter 2016-10-25 12:09 ` Jani Nikula 2016-10-25 22:28 ` Manasi Navare 2016-10-25 22:38 ` Rodrigo Vivi 2016-10-21 23:45 ` [PATCH 2/5] drm: Define a work struct for scheduling a uevent for modeset retry Manasi Navare 2016-10-22 8:48 ` Daniel Vetter 2016-10-25 6:28 ` Manasi Navare 2016-10-25 6:30 ` Pandiyan, Dhinakaran 2016-10-25 6:45 ` [Intel-gfx] " Daniel Vetter 2016-10-21 23:45 ` [PATCH 3/5] drm/i915: Change the placement of some static functions in intel_dp.c Manasi Navare 2016-10-21 23:45 ` [PATCH 4/5] drm/i915; Add a function to return index of link rate Manasi Navare 2016-10-25 6:33 ` Pandiyan, Dhinakaran 2016-10-21 23:45 ` [PATCH 5/5] drm/i915: Link Rate fallback on Link training failure Manasi Navare 2016-10-24 17:53 ` Jim Bride 2016-10-25 6:23 ` Pandiyan, Dhinakaran 2016-10-25 18:32 ` Manasi Navare 2016-10-25 12:17 ` Jani Nikula 2016-10-25 18:00 ` Jim Bride 2016-10-25 18:37 ` Manasi Navare 2016-10-22 0:16 ` ✗ Fi.CI.BAT: warning for Handle link training failure for DDI platforms Patchwork
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.