| Message ID | 1440503438-13470-4-git-send-email-sivakumar.thulasimani@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 25 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 > result in compliance test case 4.2.2.8 passing since it tests > for this behavior. > > This will allow both detection of hotplug and unplug of panels > through dongles that give only short pulse for such events. > > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 834f513..eb3ab41 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4370,26 +4370,34 @@ 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) You could turn perform_full_detect into a boolean return value which this function doesn't have. > { > 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; > + u8 ret; Isn't this a bool, really? > 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)) { > + ret = intel_dp_get_dpcd(intel_dp); > + > + if (old_sink_count != intel_dp->sink_count) > + *perform_full_detect = true; Can't trust intel_dp->sink_count without checking the return value. So I guess you can call intel_dp_get_dpcd the way it's done now, and move the sink count check below it. BR, Jani. > + > + if (!ret) > return; > - } > > if (!intel_encoder->base.crtc) > return; > @@ -5031,13 +5039,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 8/26/2015 3:32 PM, Jani Nikula wrote: > On Tue, 25 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 >> result in compliance test case 4.2.2.8 passing since it tests >> for this behavior. >> >> This will allow both detection of hotplug and unplug of panels >> through dongles that give only short pulse for such events. >> >> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> >> --- >> drivers/gpu/drm/i915/intel_dp.c | 20 ++++++++++++++++---- >> 1 file changed, 16 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 834f513..eb3ab41 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -4370,26 +4370,34 @@ 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) > You could turn perform_full_detect into a boolean return value which > this function doesn't have. sure will do. > >> { >> 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; >> + u8 ret; > Isn't this a bool, really? > >> 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)) { >> + ret = intel_dp_get_dpcd(intel_dp); >> + >> + if (old_sink_count != intel_dp->sink_count) >> + *perform_full_detect = true; > Can't trust intel_dp->sink_count without checking the return value. So I > guess you can call intel_dp_get_dpcd the way it's done now, and move the > sink count check below it. > > BR, > Jani. sure will fix this. > >> + >> + if (!ret) >> return; >> - } >> >> if (!intel_encoder->base.crtc) >> return; >> @@ -5031,13 +5039,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 8/26/2015 3:32 PM, Jani Nikula wrote: > On Tue, 25 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 >> result in compliance test case 4.2.2.8 passing since it tests >> for this behavior. >> >> This will allow both detection of hotplug and unplug of panels >> through dongles that give only short pulse for such events. >> >> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> >> --- >> drivers/gpu/drm/i915/intel_dp.c | 20 ++++++++++++++++---- >> 1 file changed, 16 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 834f513..eb3ab41 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -4370,26 +4370,34 @@ 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) > You could turn perform_full_detect into a boolean return value which > this function doesn't have. ok thinking a few mins brings back the reason for this change :). we return from this function for the following reasons, > dpcd read failed > sink count change > crtc is not active > ATR was requested and it was handled/ignored > link status was good or if bad we retrained it. and it might have passed or failed. lets say i return true or false. what does that mean ? i'll need full detection only if display was already present and is not anymore which can be due to dpcd read failure while the display was enabled ? (basically sink count change from 1 to 0) or sink count changed from 0 to 1. so it is better to have an explicit parameter rather than > >> { >> 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; >> + u8 ret; > Isn't this a bool, really? > >> 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)) { >> + ret = intel_dp_get_dpcd(intel_dp); >> + >> + if (old_sink_count != intel_dp->sink_count) >> + *perform_full_detect = true; > Can't trust intel_dp->sink_count without checking the return value. So I > guess you can call intel_dp_get_dpcd the way it's done now, and move the > sink count check below it. > > BR, > Jani. > >> + >> + if (!ret) >> return; >> - } >> >> if (!intel_encoder->base.crtc) >> return; >> @@ -5031,13 +5039,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 >>
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 834f513..eb3ab41 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4370,26 +4370,34 @@ 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; + u8 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)) { + ret = intel_dp_get_dpcd(intel_dp); + + if (old_sink_count != intel_dp->sink_count) + *perform_full_detect = true; + + if (!ret) return; - } if (!intel_encoder->base.crtc) return; @@ -5031,13 +5039,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; } }