On Mon, Feb 01, 2016 at 11:13:08AM +0200, Ander Conselvan De Oliveira wrote: > 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@xxxxxxxxx> > > > > > > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@xxxxxxxxx> > > > > > > Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@xxxxxxxxx> > > > > > > --- > > > > > > 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. So this conversation seems to have had little bearing on reality, especially in terms of how intel_dp_detect() is supposed to behave. This patch is causing WARNs as it presumed that intel_dp_detect() is only called after a modeset. Should we send a revert to stable? -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
