Re: [Intel-gfx] [PATCH 3/3] drm/i915: force full detect on sink count change
On 8/26/2015 3:32 PM, Jani Nikula wrote: On Tue, 25 Aug 2015, Sivakumar Thulasimani wrote: From: "Thulasimani,Sivakumar" 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 --- 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 -- regards, Sivakumar ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm/i915: force full detect on sink count change
On 8/26/2015 3:32 PM, Jani Nikula wrote: On Tue, 25 Aug 2015, Sivakumar Thulasimani wrote: From: "Thulasimani,Sivakumar" 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 --- 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 -- regards, Sivakumar ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm/i915: force full detect on sink count change
On Tue, 25 Aug 2015, Sivakumar Thulasimani wrote: > From: "Thulasimani,Sivakumar" > > 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 > --- > 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 > -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx