Re: [Intel-gfx] [PATCH 4/4] drm/i915: force full detect on sink count change
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"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 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 = _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(>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(>mode_config.connection_mutex, NULL); - intel_dp_check_link_status(intel_dp); + intel_dp_check_link_status(intel_dp, _detect); drm_modeset_unlock(>mode_config.connection_mutex); + + if (full_detect) + goto put_power; } } -- 1.7.9.5 ___
Re: [Intel-gfx] [PATCH 4/4] drm/i915: force full detect on sink count change
On Thu, Aug 27, 2015 at 02:18:32PM +0530, 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 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 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 = _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(>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(>mode_config.connection_mutex, > NULL); > - intel_dp_check_link_status(intel_dp); > + intel_dp_check_link_status(intel_dp, _detect); > drm_modeset_unlock(>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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/4] drm/i915: force full detect on sink count change
On Thu, 27 Aug 2015, Sivakumar Thulasimaniwrote: > 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 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 > --- > 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 = _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(>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(>mode_config.connection_mutex, > NULL); > - intel_dp_check_link_status(intel_dp); > + intel_dp_check_link_status(intel_dp, _detect); > drm_modeset_unlock(>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
Re: [Intel-gfx] [PATCH 4/4] drm/i915: force full detect on sink count change
On 9/1/2015 4:12 PM, Jani Nikula wrote: On Thu, 27 Aug 2015, Sivakumar Thulasimaniwrote: 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 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 --- 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 = _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(>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(>mode_config.connection_mutex, NULL); - intel_dp_check_link_status(intel_dp); + intel_dp_check_link_status(intel_dp, _detect); drm_modeset_unlock(>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 4/4] drm/i915: force full detect on sink count change
On Tue, 01 Sep 2015, Sivakumar Thulasimaniwrote: > On 9/1/2015 4:12 PM, Jani Nikula wrote: >> On Thu, 27 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 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 >>> --- >>> 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 = _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(>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(>mode_config.connection_mutex, >>> NULL); >>> - intel_dp_check_link_status(intel_dp); >>> + intel_dp_check_link_status(intel_dp, _detect); >>> drm_modeset_unlock(>mode_config.connection_mutex); >>> + >>> + if (full_detect) >>> + goto put_power; >>> } >>> } >>> >>> -- >>> 1.7.9.5 >>> > > -- > regards, > Sivakumar > -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing