[Intel-gfx] [PATCH 2/5] drm/i915: Cleaning up intel_dp_hpd_pulse
Ander Conselvan De Oliveira
conselvan2 at gmail.com
Mon Feb 1 10:13:08 CET 2016
On Mon, 2016-02-01 at 11:50 +0530, Thulasimani, Sivakumar wrote:
>
> On 1/29/2016 5:33 PM, Ander Conselvan De Oliveira wrote:
> > On Fri, 2016-01-29 at 14:31 +0530, Shubhangi Shrivastava wrote:
> > > On Tuesday 26 January 2016 06:52 PM, Ander Conselvan De Oliveira wrote:
> > > > On Tue, 2016-01-19 at 16:07 +0530, Shubhangi Shrivastava wrote:
> > > > > Current DP detection has DPCD operations split across
> > > > > intel_dp_hpd_pulse and intel_dp_detect which contains
> > > > > duplicates as well. Also intel_dp_detect is called
> > > > > during modes enumeration as well which will result
> > > > > in multiple dpcd operations. So this patch tries
> > > > > to solve both these by bringing all DPCD operations
> > > > > in one single function and make intel_dp_detect
> > > > > use existing values instead of repeating same steps.
> > > > >
> > > > > v2: Pulled in a hunk from last patch of the series to
> > > > > this patch. (Ander)
> > > > > v3: Added MST hotplug handling. (Ander)
> > > > >
> > > > > Tested-by: Nathan D Ciobanu <nathan.d.ciobanu at intel.com>
> > > > > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani at intel.com>
> > > > > Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava at intel.com>
> > > > > ---
> > > > > drivers/gpu/drm/i915/intel_dp.c | 71 +++++++++++++++++++++++++-----
> > > > > -----
> > > > > -----
> > > > > -
> > > > > 1 file changed, 44 insertions(+), 27 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index 8969ff9..82ee18d 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > [...]
> > > >
> > > > > @@ -4693,7 +4717,8 @@ intel_dp_detect(struct drm_connector *connector,
> > > > > bool
> > > > > force)
> > > > > return connector_status_disconnected;
> > > > > }
> > > > >
> > > > > - intel_dp_long_pulse(intel_dp->attached_connector);
> > > > > + if (force)
> > > > > + intel_dp_long_pulse(intel_dp->attached_connector);
> > > > I didn't notice this at first, but force is not the right thing to check
> > > > for
> > > > here. It is basically intended to avoid doing load detection (see
> > > > intel_get_load_detect_pipe()) on automated polling. But we still have to
> > > > try
> > > > detection here when force = false, otherwise this will cause a
> > > > regression.
> > > >
> > > > If you plug in a DP device while suspended, that device won't get
> > > > detected,
> > > > since we don't get an HPD for it. Previously, the call do
> > > > intel_dp_detect()
> > > > with
> > > > force = false from intel_drm_resume() (via drm_helper_hpd_irq_event())
> > > > would
> > > > cause a full detection.
> > > >
> > > > To avoid the repeated DPCD operations, I think we need a more explicit
> > > > mechanism
> > > > to signal that we already handled the long pulse via the HPD handler. In
> > > > intel_dp_hpd_pulse() we could set a flag that tells we just handled a
> > > > long
> > > > pulse
> > > > for the given port. The call to intel_dp_long_pulse() in
> > > > intel_dp_detect()
> > > > would
> > > > then be dependent on that flag.
> > > >
> > > > For that reason I have to retract my R-b from this patch.
> > > >
> > > > Ander
> > > Call to intel_dp_detect() from get_modes is with force set to true while
> > > from resume the call is with force set to false.. It should be in the
> > > opposite manner as get_modes should not require full detection whereas
> > > resume should. So, this needs to be cleaned up there. After merge of
> > > these patches, will look into cleaning up that part of the code.
> > That really depends on what the force parameter is supposed to mean. The
> > documentation states that "force is set to false whilst polling, true when
> > checking the connector due to a user request". A look through git history
> > shows
> > the parameter was added to reduce time wasted doing load detection (doing a
> > mode
> > set in order to check if there is a device connected) for hardware that
> > needs it
> > (commit 7b334fcb45b7).
> >
> > As far as I can tell, across all the drm drivers, that parameter is only
> > used to
> > avoid doing load detection.
> >
> > Another thing to consider is that the driver may switch to polling if it
> > detects
> > an HPD storm. When the detect calls come from polling, the code in this
> > patch
> > will simply avoid any detection.
> >
> hmm i think this discussion will prolong for a while :)
> how about we call intel_dp_long_pulse() always for now.
> this will be a compromise to not break any of the existing code
> but will still result in getting a clean detection code, which
> will can then be improved upon in the next iteration ?
> i.e post the change it should look like. i.e skip this change alone
>
> intel_dp_long_pulse(intel_dp->attached_connector);
Sounds good. The code gets cleaner and there is no regression in terms of
repeated DPCD operations.
Ander
>
>
> regards,
> Sivakumar
> > > Moreover, intel_dp_detect() will be called from
> > > drm_helper_hpd_irq_event() in polling scenarios only (when
> > > DRM_CONNECTOR_POLL_HPD flag is set in connector->polled). So, seems like
> > > this code here, doesn't really create a regression for realtime scenarios.
> > I don't know what you mean by realtime scenarios, but the regression is very
> > real. Using a kernel with your patches applied, suspend while there is no DP
> > monitor attached, attach the monitor while suspended and then wake up.
> > Notice
> > how the connector state doesn't change. You can check the i915_display_info
> > file
> > in debugfs, for instance.
> >
> >
> > Ander
> >
> > > >
> > > > > if (intel_connector->detect_edid)
> > > > > return connector_status_connected;
> > > > > @@ -5026,25 +5051,25 @@ intel_dp_hpd_pulse(struct intel_digital_port
> > > > > *intel_dig_port, bool long_hpd)
> > > > > /* indicate that we need to restart link training
> > > > > */
> > > > > intel_dp->train_set_valid = false;
> > > > >
> > > > > - if (!intel_digital_port_connected(dev_priv,
> > > > > intel_dig_port))
> > > > > - goto mst_fail;
> > > > > -
> > > > > - if (!intel_dp_get_dpcd(intel_dp)) {
> > > > > - goto mst_fail;
> > > > > - }
> > > > > -
> > > > > - intel_dp_probe_oui(intel_dp);
> > > > > + intel_dp_long_pulse(intel_dp->attached_connector);
> > > > > + if (intel_dp->is_mst)
> > > > > + ret = IRQ_HANDLED;
> > > > > + goto put_power;
> > > > >
> > > > > - if (!intel_dp_probe_mst(intel_dp)) {
> > > > > - drm_modeset_lock(&dev
> > > > > ->mode_config.connection_mutex,
> > > > > NULL);
> > > > > - intel_dp_check_link_status(intel_dp);
> > > > > - drm_modeset_unlock(&dev
> > > > > ->mode_config.connection_mutex);
> > > > > - goto mst_fail;
> > > > > - }
> > > > > } else {
> > > > > if (intel_dp->is_mst) {
> > > > > - if (intel_dp_check_mst_status(intel_dp) ==
> > > > > -EINVAL)
> > > > > - goto mst_fail;
> > > > > + if (intel_dp_check_mst_status(intel_dp) ==
> > > > > -EINVAL) {
> > > > > + /*
> > > > > + * If we were in MST mode, and device
> > > > > is
> > > > > not
> > > > > + * there, get out of MST mode
> > > > > + */
> > > > > + DRM_DEBUG_KMS("MST device may have
> > > > > disappeared %d vs %d\n",
> > > > > + intel_dp->is_mst, intel_dp
> > > > > ->mst_mgr.mst_state);
> > > > > + intel_dp->is_mst = false;
> > > > > + drm_dp_mst_topology_mgr_set_mst(&inte
> > > > > l_dp
> > > > > ->mst_mgr,
> > > > > + intel
> > > > > _dp
> > > > > ->is_mst);
> > > > > + goto put_power;
> > > > > + }
> > > > > }
> > > > >
> > > > > if (!intel_dp->is_mst) {
> > > > > @@ -5056,14 +5081,6 @@ intel_dp_hpd_pulse(struct intel_digital_port
> > > > > *intel_dig_port, bool long_hpd)
> > > > >
> > > > > ret = IRQ_HANDLED;
> > > > >
> > > > > - goto put_power;
> > > > > -mst_fail:
> > > > > - /* if we were in MST mode, and device is not there get out of
> > > > > MST
> > > > > mode */
> > > > > - if (intel_dp->is_mst) {
> > > > > - DRM_DEBUG_KMS("MST device may have disappeared %d vs
> > > > > %d\n",
> > > > > intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
> > > > > - intel_dp->is_mst = false;
> > > > > - drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> > > > > intel_dp
> > > > > ->is_mst);
> > > > > - }
> > > > > put_power:
> > > > > intel_display_power_put(dev_priv, power_domain);
> > > > >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
More information about the Intel-gfx
mailing list