| Message ID | 1440665312-26698-5-git-send-email-sivakumar.thulasimani@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 27 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote: > From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com> > > This patch checks for changes in sink count between short pulse > hpds and forces full detect when there is a change. > > This will allow both detection of hotplug and unplug of panels > through dongles that give only short pulse for such events. > > v2: changed variable type from u8 to bool (Jani) > return immediately if perform_full_detect is set(Siva) The general idea LGTM, however see below. > > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 27 ++++++++++++++++++++++----- > 1 file changed, 22 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 834f513..279e52c 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4370,26 +4370,39 @@ go_again: > * 4. Check link status on receipt of hot-plug interrupt > */ > static void > -intel_dp_check_link_status(struct intel_dp *intel_dp) > +intel_dp_check_link_status(struct intel_dp *intel_dp, bool *perform_full_detect) > { > struct drm_device *dev = intel_dp_to_dev(intel_dp); > struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base; > struct intel_crtc *crtc = > to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc); > u8 sink_irq_vector; > + u8 old_sink_count = intel_dp->sink_count; > + bool ret; > u8 link_status[DP_LINK_STATUS_SIZE]; > > WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); > > + *perform_full_detect = false; > + > /* Try to read receiver status if the link appears to be up */ > if (!intel_dp_get_link_status(intel_dp, link_status)) { > return; > } > > - /* Now read the DPCD to see if it's actually running */ > - if (!intel_dp_get_dpcd(intel_dp)) { > + /* Now read the DPCD to see if it's actually running > + * Don't return immediately if dpcd read failed, > + * if sink count was 1 and dpcd read failed we need > + * to do full detection > + */ > + ret = intel_dp_get_dpcd(intel_dp); > + > + if (old_sink_count != intel_dp->sink_count) > + *perform_full_detect = true; The point I was trying to make earlier is that how can you trust intel_dp->sink_count if intel_dp_get_dpcd failed? You don't know where it failed, and ->sink_count may or may not have changed. Maybe it has changed for real but you didn't have a chance to read it just yet. > + > + /* No need to proceed if we are going to do full detect */ > + if (!ret || *perform_full_detect) > return; > - } So if you're returning because of !ret, *perform_full_detect may be true or false, you don't know. I'd like the code to be explicit about what you want the caller to do when intel_dp_get_dpcd returns false, full detect or not. BR, Jani. > > if (!intel_encoder->base.crtc) > return; > @@ -5031,13 +5044,17 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) > } > > if (!intel_dp->is_mst) { > + bool full_detect; > /* > * we'll check the link status via the normal hot plug path later - > * but for short hpds we should check it now > */ > drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > - intel_dp_check_link_status(intel_dp); > + intel_dp_check_link_status(intel_dp, &full_detect); > drm_modeset_unlock(&dev->mode_config.connection_mutex); > + > + if (full_detect) > + goto put_power; > } > } > > -- > 1.7.9.5 >
On 9/1/2015 4:12 PM, Jani Nikula wrote: > On Thu, 27 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote: >> From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com> >> >> This patch checks for changes in sink count between short pulse >> hpds and forces full detect when there is a change. >> >> This will allow both detection of hotplug and unplug of panels >> through dongles that give only short pulse for such events. >> >> v2: changed variable type from u8 to bool (Jani) >> return immediately if perform_full_detect is set(Siva) > The general idea LGTM, however see below. > >> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> >> --- >> drivers/gpu/drm/i915/intel_dp.c | 27 ++++++++++++++++++++++----- >> 1 file changed, 22 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 834f513..279e52c 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -4370,26 +4370,39 @@ go_again: >> * 4. Check link status on receipt of hot-plug interrupt >> */ >> static void >> -intel_dp_check_link_status(struct intel_dp *intel_dp) >> +intel_dp_check_link_status(struct intel_dp *intel_dp, bool *perform_full_detect) >> { >> struct drm_device *dev = intel_dp_to_dev(intel_dp); >> struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base; >> struct intel_crtc *crtc = >> to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc); >> u8 sink_irq_vector; >> + u8 old_sink_count = intel_dp->sink_count; >> + bool ret; >> u8 link_status[DP_LINK_STATUS_SIZE]; >> >> WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); >> >> + *perform_full_detect = false; >> + >> /* Try to read receiver status if the link appears to be up */ >> if (!intel_dp_get_link_status(intel_dp, link_status)) { >> return; >> } >> >> - /* Now read the DPCD to see if it's actually running */ >> - if (!intel_dp_get_dpcd(intel_dp)) { >> + /* Now read the DPCD to see if it's actually running >> + * Don't return immediately if dpcd read failed, >> + * if sink count was 1 and dpcd read failed we need >> + * to do full detection >> + */ >> + ret = intel_dp_get_dpcd(intel_dp); >> + >> + if (old_sink_count != intel_dp->sink_count) >> + *perform_full_detect = true; > The point I was trying to make earlier is that how can you trust > intel_dp->sink_count if intel_dp_get_dpcd failed? You don't know where > it failed, and ->sink_count may or may not have changed. Maybe it has > changed for real but you didn't have a chance to read it just yet. > >> + >> + /* No need to proceed if we are going to do full detect */ >> + if (!ret || *perform_full_detect) >> return; >> - } > So if you're returning because of !ret, *perform_full_detect may be true > or false, you don't know. I'd like the code to be explicit about what > you want the caller to do when intel_dp_get_dpcd returns false, full > detect or not. > > BR, > Jani. the idea i had in mind was if intel_dp_get_dpcd failed we should do a full detect. my assumption here was that sink_count would be zero so it would take care of this, but now that it seems to be a wrong assumption how about the following code ? this makes it explicit that if dpcd read failed we will do full detect. + if ((old_sink_count != intel_dp->sink_count) || !ret) + *perform_full_detect = true; + + /* No need to proceed if we are going to do full detect */ + if (*perform_full_detect) return; > >> >> if (!intel_encoder->base.crtc) >> return; >> @@ -5031,13 +5044,17 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) >> } >> >> if (!intel_dp->is_mst) { >> + bool full_detect; >> /* >> * we'll check the link status via the normal hot plug path later - >> * but for short hpds we should check it now >> */ >> drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); >> - intel_dp_check_link_status(intel_dp); >> + intel_dp_check_link_status(intel_dp, &full_detect); >> drm_modeset_unlock(&dev->mode_config.connection_mutex); >> + >> + if (full_detect) >> + goto put_power; >> } >> } >> >> -- >> 1.7.9.5 >>
On Tue, 01 Sep 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote: > On 9/1/2015 4:12 PM, Jani Nikula wrote: >> On Thu, 27 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote: >>> From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com> >>> >>> This patch checks for changes in sink count between short pulse >>> hpds and forces full detect when there is a change. >>> >>> This will allow both detection of hotplug and unplug of panels >>> through dongles that give only short pulse for such events. >>> >>> v2: changed variable type from u8 to bool (Jani) >>> return immediately if perform_full_detect is set(Siva) >> The general idea LGTM, however see below. >> >>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_dp.c | 27 ++++++++++++++++++++++----- >>> 1 file changed, 22 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >>> index 834f513..279e52c 100644 >>> --- a/drivers/gpu/drm/i915/intel_dp.c >>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>> @@ -4370,26 +4370,39 @@ go_again: >>> * 4. Check link status on receipt of hot-plug interrupt >>> */ >>> static void >>> -intel_dp_check_link_status(struct intel_dp *intel_dp) >>> +intel_dp_check_link_status(struct intel_dp *intel_dp, bool *perform_full_detect) >>> { >>> struct drm_device *dev = intel_dp_to_dev(intel_dp); >>> struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base; >>> struct intel_crtc *crtc = >>> to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc); >>> u8 sink_irq_vector; >>> + u8 old_sink_count = intel_dp->sink_count; >>> + bool ret; >>> u8 link_status[DP_LINK_STATUS_SIZE]; >>> >>> WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); >>> >>> + *perform_full_detect = false; >>> + >>> /* Try to read receiver status if the link appears to be up */ >>> if (!intel_dp_get_link_status(intel_dp, link_status)) { >>> return; >>> } >>> >>> - /* Now read the DPCD to see if it's actually running */ >>> - if (!intel_dp_get_dpcd(intel_dp)) { >>> + /* Now read the DPCD to see if it's actually running >>> + * Don't return immediately if dpcd read failed, >>> + * if sink count was 1 and dpcd read failed we need >>> + * to do full detection >>> + */ >>> + ret = intel_dp_get_dpcd(intel_dp); >>> + >>> + if (old_sink_count != intel_dp->sink_count) >>> + *perform_full_detect = true; >> The point I was trying to make earlier is that how can you trust >> intel_dp->sink_count if intel_dp_get_dpcd failed? You don't know where >> it failed, and ->sink_count may or may not have changed. Maybe it has >> changed for real but you didn't have a chance to read it just yet. >> >>> + >>> + /* No need to proceed if we are going to do full detect */ >>> + if (!ret || *perform_full_detect) >>> return; >>> - } >> So if you're returning because of !ret, *perform_full_detect may be true >> or false, you don't know. I'd like the code to be explicit about what >> you want the caller to do when intel_dp_get_dpcd returns false, full >> detect or not. >> >> BR, >> Jani. > the idea i had in mind was if intel_dp_get_dpcd failed we should do a > full detect. > > my assumption here was that sink_count would be zero so it would take care > of this, but now that it seems to be a wrong assumption how about the > following code ? > this makes it explicit that if dpcd read failed we will do full detect. > > + if ((old_sink_count != intel_dp->sink_count) || !ret) > + *perform_full_detect = true; > > + > + /* No need to proceed if we are going to do full detect */ > + if (*perform_full_detect) > return; Ok by me. Jani. > >> >>> >>> if (!intel_encoder->base.crtc) >>> return; >>> @@ -5031,13 +5044,17 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) >>> } >>> >>> if (!intel_dp->is_mst) { >>> + bool full_detect; >>> /* >>> * we'll check the link status via the normal hot plug path later - >>> * but for short hpds we should check it now >>> */ >>> drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); >>> - intel_dp_check_link_status(intel_dp); >>> + intel_dp_check_link_status(intel_dp, &full_detect); >>> drm_modeset_unlock(&dev->mode_config.connection_mutex); >>> + >>> + if (full_detect) >>> + goto put_power; >>> } >>> } >>> >>> -- >>> 1.7.9.5 >>> > > -- > regards, > Sivakumar >
On Thu, Aug 27, 2015 at 02:18:32PM +0530, Sivakumar Thulasimani wrote: > From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com> > > This patch checks for changes in sink count between short pulse > hpds and forces full detect when there is a change. > > This will allow both detection of hotplug and unplug of panels > through dongles that give only short pulse for such events. > > v2: changed variable type from u8 to bool (Jani) > return immediately if perform_full_detect is set(Siva) > > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> That's still incomplete since in the hotplug work function we check whether ->detect status has changed. If that doesn't happen (e.g. sink count changes from 1->2 or whatever) then we'll fail to generate the required uevent. I suspect the only clean way to fix this is to merge all the dp hpd handling together into one place and stop using ->detect for some parts of it. Then we would have one place only that decides whether anything changed, and whether we need to send anything to userspace or not. -Daniel > --- > drivers/gpu/drm/i915/intel_dp.c | 27 ++++++++++++++++++++++----- > 1 file changed, 22 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 834f513..279e52c 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4370,26 +4370,39 @@ go_again: > * 4. Check link status on receipt of hot-plug interrupt > */ > static void > -intel_dp_check_link_status(struct intel_dp *intel_dp) > +intel_dp_check_link_status(struct intel_dp *intel_dp, bool *perform_full_detect) > { > struct drm_device *dev = intel_dp_to_dev(intel_dp); > struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base; > struct intel_crtc *crtc = > to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc); > u8 sink_irq_vector; > + u8 old_sink_count = intel_dp->sink_count; > + bool ret; > u8 link_status[DP_LINK_STATUS_SIZE]; > > WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); > > + *perform_full_detect = false; > + > /* Try to read receiver status if the link appears to be up */ > if (!intel_dp_get_link_status(intel_dp, link_status)) { > return; > } > > - /* Now read the DPCD to see if it's actually running */ > - if (!intel_dp_get_dpcd(intel_dp)) { > + /* Now read the DPCD to see if it's actually running > + * Don't return immediately if dpcd read failed, > + * if sink count was 1 and dpcd read failed we need > + * to do full detection > + */ > + ret = intel_dp_get_dpcd(intel_dp); > + > + if (old_sink_count != intel_dp->sink_count) > + *perform_full_detect = true; > + > + /* No need to proceed if we are going to do full detect */ > + if (!ret || *perform_full_detect) > return; > - } > > if (!intel_encoder->base.crtc) > return; > @@ -5031,13 +5044,17 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) > } > > if (!intel_dp->is_mst) { > + bool full_detect; > /* > * we'll check the link status via the normal hot plug path later - > * but for short hpds we should check it now > */ > drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > - intel_dp_check_link_status(intel_dp); > + intel_dp_check_link_status(intel_dp, &full_detect); > drm_modeset_unlock(&dev->mode_config.connection_mutex); > + > + if (full_detect) > + goto put_power; > } > } > > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 9/2/2015 2:43 PM, Daniel Vetter wrote: > On Thu, Aug 27, 2015 at 02:18:32PM +0530, Sivakumar Thulasimani wrote: >> From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com> >> >> This patch checks for changes in sink count between short pulse >> hpds and forces full detect when there is a change. >> >> This will allow both detection of hotplug and unplug of panels >> through dongles that give only short pulse for such events. >> >> v2: changed variable type from u8 to bool (Jani) >> return immediately if perform_full_detect is set(Siva) >> >> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> > That's still incomplete since in the hotplug work function we check > whether ->detect status has changed. If that doesn't happen (e.g. sink > count changes from 1->2 or whatever) then we'll fail to generate the > required uevent. > > I suspect the only clean way to fix this is to merge all the dp hpd > handling together into one place and stop using ->detect for some parts of > it. Then we would have one place only that decides whether anything > changed, and whether we need to send anything to userspace or not. > -Daniel i doubt sink count change from 1->2 is valid. 1) we support only edp and SST displays in intel_dp_detect. we should handle all mst related code in intel_dp_hpd_pulse. 2) check_link_status is called from withing !mst check so i dont see it change from 1 to 2. 3) even if it changes from 1 to 2, for the given port without mst there is no way to switch between first sink to second one outside mst logic. correct me if i am wrong, the only reason to call intel_dp_detect is to see if "one display is connected or disconnected" which i feel is fine with current code. But i do agree that we need to clean up intel_dp_hpd_pulse since it does a lot of things then repeats those same operations in intel_dp_detect. for example we read dpcd 0-14, link status and probe oui only to repeat the same steps in the intel_dp_detect. considering that our dpcd read has 1ms delay this can add up pretty quick. if the intention of intel_dp_hpd_pulse is to handle mst displays, that should be checked first and continue only if display is mst otherwise we should just jump to intel_dp_detect. >> --- >> drivers/gpu/drm/i915/intel_dp.c | 27 ++++++++++++++++++++++----- >> 1 file changed, 22 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 834f513..279e52c 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -4370,26 +4370,39 @@ go_again: >> * 4. Check link status on receipt of hot-plug interrupt >> */ >> static void >> -intel_dp_check_link_status(struct intel_dp *intel_dp) >> +intel_dp_check_link_status(struct intel_dp *intel_dp, bool *perform_full_detect) >> { >> struct drm_device *dev = intel_dp_to_dev(intel_dp); >> struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base; >> struct intel_crtc *crtc = >> to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc); >> u8 sink_irq_vector; >> + u8 old_sink_count = intel_dp->sink_count; >> + bool ret; >> u8 link_status[DP_LINK_STATUS_SIZE]; >> >> WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); >> >> + *perform_full_detect = false; >> + >> /* Try to read receiver status if the link appears to be up */ >> if (!intel_dp_get_link_status(intel_dp, link_status)) { >> return; >> } >> >> - /* Now read the DPCD to see if it's actually running */ >> - if (!intel_dp_get_dpcd(intel_dp)) { >> + /* Now read the DPCD to see if it's actually running >> + * Don't return immediately if dpcd read failed, >> + * if sink count was 1 and dpcd read failed we need >> + * to do full detection >> + */ >> + ret = intel_dp_get_dpcd(intel_dp); >> + >> + if (old_sink_count != intel_dp->sink_count) >> + *perform_full_detect = true; >> + >> + /* No need to proceed if we are going to do full detect */ >> + if (!ret || *perform_full_detect) >> return; >> - } >> >> if (!intel_encoder->base.crtc) >> return; >> @@ -5031,13 +5044,17 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) >> } >> >> if (!intel_dp->is_mst) { >> + bool full_detect; >> /* >> * we'll check the link status via the normal hot plug path later - >> * but for short hpds we should check it now >> */ >> drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); >> - intel_dp_check_link_status(intel_dp); >> + intel_dp_check_link_status(intel_dp, &full_detect); >> drm_modeset_unlock(&dev->mode_config.connection_mutex); >> + >> + if (full_detect) >> + goto put_power; >> } >> } >> >> -- >> 1.7.9.5 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 834f513..279e52c 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4370,26 +4370,39 @@ go_again: * 4. Check link status on receipt of hot-plug interrupt */ static void -intel_dp_check_link_status(struct intel_dp *intel_dp) +intel_dp_check_link_status(struct intel_dp *intel_dp, bool *perform_full_detect) { struct drm_device *dev = intel_dp_to_dev(intel_dp); struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base; struct intel_crtc *crtc = to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc); u8 sink_irq_vector; + u8 old_sink_count = intel_dp->sink_count; + bool ret; u8 link_status[DP_LINK_STATUS_SIZE]; WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); + *perform_full_detect = false; + /* Try to read receiver status if the link appears to be up */ if (!intel_dp_get_link_status(intel_dp, link_status)) { return; } - /* Now read the DPCD to see if it's actually running */ - if (!intel_dp_get_dpcd(intel_dp)) { + /* Now read the DPCD to see if it's actually running + * Don't return immediately if dpcd read failed, + * if sink count was 1 and dpcd read failed we need + * to do full detection + */ + ret = intel_dp_get_dpcd(intel_dp); + + if (old_sink_count != intel_dp->sink_count) + *perform_full_detect = true; + + /* No need to proceed if we are going to do full detect */ + if (!ret || *perform_full_detect) return; - } if (!intel_encoder->base.crtc) return; @@ -5031,13 +5044,17 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) } if (!intel_dp->is_mst) { + bool full_detect; /* * we'll check the link status via the normal hot plug path later - * but for short hpds we should check it now */ drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); - intel_dp_check_link_status(intel_dp); + intel_dp_check_link_status(intel_dp, &full_detect); drm_modeset_unlock(&dev->mode_config.connection_mutex); + + if (full_detect) + goto put_power; } }