| Message ID | 1418255597-4716-3-git-send-email-tprevite@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2014-12-10 21:53 GMT-02:00 Todd Previte <tprevite@gmail.com>: > Move the DPCD read to the top and check for an interrupt from the sink to catch > Displayport automated testing requests necessary to support Displayport compliance > testing. The checks for active connectors and link status are moved below the > check for the interrupt. Why exactly is this needed? > > Adds a check at the top to verify that the device is connected. This is necessary > for DP compliance testing to ensure that test requests are captured and acknowledged. Why exactly? Can you please describe in terms of how the code is executed in each case and what is missing? Since there appears to be 2 different changes, shouldn't this patch be split into 2 different patches? > If a test request is present during a connected->disconnected transition, > the test code will attempt to execute even though the connection has been disabled, > resulting in a faied test. > > Signed-off-by: Todd Previte <tprevite@gmail.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 32 +++++++++++++++++++------------- > 1 file changed, 19 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 3dc92a3..1b452cc 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3890,21 +3890,14 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) > > WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); > > - if (!intel_encoder->connectors_active) > - return; > - > - if (WARN_ON(!intel_encoder->base.crtc)) > - return; > - > - if (!to_intel_crtc(intel_encoder->base.crtc)->active) > - return; > - > - /* Try to read receiver status if the link appears to be up */ > - if (!intel_dp_get_link_status(intel_dp, link_status)) { > + /* Bail if not connected */ Bikeshed: I don't see the reason for the comment above :) Other than that, the patch looks fine. Let's see what PRTS will say about it. > + if (intel_dp->attached_connector->base.status != > + connector_status_connected) { > + DRM_DEBUG_KMS("Not connected\n"); > return; > } > > - /* Now read the DPCD to see if it's actually running */ > + /* Attempt to read the DPCD */ > if (!intel_dp_get_dpcd(intel_dp)) { > return; > } > @@ -3916,13 +3909,26 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) > drm_dp_dpcd_writeb(&intel_dp->aux, > DP_DEVICE_SERVICE_IRQ_VECTOR, > sink_irq_vector); > - > if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST) > intel_dp_handle_test_request(intel_dp); > if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ)) > DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n"); > } > > + if (!intel_encoder->connectors_active) > + return; > + > + if (WARN_ON(!intel_encoder->base.crtc)) > + return; > + > + if (!to_intel_crtc(intel_encoder->base.crtc)->active) > + return; > + > + /* Try to read receiver status if the link appears to be up */ > + if (!intel_dp_get_link_status(intel_dp, link_status)) { > + return; > + } > + > if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) { > DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n", > intel_encoder->base.name); > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 12/15/2014 9:36 AM, Paulo Zanoni wrote: > 2014-12-10 21:53 GMT-02:00 Todd Previte<tprevite@gmail.com>: >> Move the DPCD read to the top and check for an interrupt from the sink to catch >> Displayport automated testing requests necessary to support Displayport compliance >> testing. The checks for active connectors and link status are moved below the >> check for the interrupt. > Why exactly is this needed? The main reason for doing this is to make sure that a test request isn't missed. Checking for the status of the encoder/crtc isn't necessary for some test cases (AUX channel tests are one example) and without moving the check for the interrupt, these tests may not execute if one of those checks fails. Additionally, if reading the DPCD fails, regardless of whether or not testing is happening, there's no way to train the link since configurations and status can't be read, nor can link training parameters be written. >> Adds a check at the top to verify that the device is connected. This is necessary >> for DP compliance testing to ensure that test requests are captured and acknowledged. > Why exactly? Can you please describe in terms of how the code is > executed in each case and what is missing? This patch is actually both a bug fix and a component of compliance testing. Because HPD events are received both on connect and disconnect actions, it's vital that we don't try and train the link when we're transitioning from connected->disconnected. That results in errors and warning in the logs from failed AUX transactions and can trigger the WARN for the check of !base.crtc. By making the check at the beginning to see if the connection is truly active, those problems are avoided and testing / link training will only be attempted when there is a valid Displayport connection. > Since there appears to be 2 different changes, shouldn't this patch be > split into 2 different patches? It can be split into two if that will make it easier to upstream. The changes are separate but related, which is why I grouped them into a single patch. >> If a test request is present during a connected->disconnected transition, >> the test code will attempt to execute even though the connection has been disabled, >> resulting in a faied test. >> >> Signed-off-by: Todd Previte<tprevite@gmail.com> >> --- >> drivers/gpu/drm/i915/intel_dp.c | 32 +++++++++++++++++++------------- >> 1 file changed, 19 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index 3dc92a3..1b452cc 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -3890,21 +3890,14 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) >> >> WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); >> >> - if (!intel_encoder->connectors_active) >> - return; >> - >> - if (WARN_ON(!intel_encoder->base.crtc)) >> - return; >> - >> - if (!to_intel_crtc(intel_encoder->base.crtc)->active) >> - return; >> - >> - /* Try to read receiver status if the link appears to be up */ >> - if (!intel_dp_get_link_status(intel_dp, link_status)) { >> + /* Bail if not connected */ > Bikeshed: I don't see the reason for the comment above :) > > Other than that, the patch looks fine. Let's see what PRTS will say about it. Fair enough. Comment deleted. :) Fix will be in patch V3. The above responses to your questions were also included in the patch notes for further explanation of this patch. >> + if (intel_dp->attached_connector->base.status != >> + connector_status_connected) { >> + DRM_DEBUG_KMS("Not connected\n"); >> return; >> } >> >> - /* Now read the DPCD to see if it's actually running */ >> + /* Attempt to read the DPCD */ >> if (!intel_dp_get_dpcd(intel_dp)) { >> return; >> } >> @@ -3916,13 +3909,26 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) >> drm_dp_dpcd_writeb(&intel_dp->aux, >> DP_DEVICE_SERVICE_IRQ_VECTOR, >> sink_irq_vector); >> - >> if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST) >> intel_dp_handle_test_request(intel_dp); >> if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ)) >> DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n"); >> } >> >> + if (!intel_encoder->connectors_active) >> + return; >> + >> + if (WARN_ON(!intel_encoder->base.crtc)) >> + return; >> + >> + if (!to_intel_crtc(intel_encoder->base.crtc)->active) >> + return; >> + >> + /* Try to read receiver status if the link appears to be up */ >> + if (!intel_dp_get_link_status(intel_dp, link_status)) { >> + return; >> + } >> + >> if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) { >> DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n", >> intel_encoder->base.name); >> -- >> 1.9.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
2015-02-18 14:36 GMT-02:00 Todd Previte <tprevite@gmail.com>: > > On 12/15/2014 9:36 AM, Paulo Zanoni wrote: >> >> 2014-12-10 21:53 GMT-02:00 Todd Previte<tprevite@gmail.com>: >>> >>> Move the DPCD read to the top and check for an interrupt from the sink to >>> catch >>> Displayport automated testing requests necessary to support Displayport >>> compliance >>> testing. The checks for active connectors and link status are moved below >>> the >>> check for the interrupt. >> >> Why exactly is this needed? > > The main reason for doing this is to make sure that a test request isn't > missed. Checking for the status of the encoder/crtc isn't necessary for some > test cases (AUX channel tests are one example) and without moving the check > for the interrupt, these tests may not execute if one of those checks fails. > Additionally, if reading the DPCD fails, regardless of whether or not > testing is happening, there's no way to train the link since configurations > and status can't be read, nor can link training parameters be written. > > >>> Adds a check at the top to verify that the device is connected. This is >>> necessary >>> for DP compliance testing to ensure that test requests are captured and >>> acknowledged. >> >> Why exactly? Can you please describe in terms of how the code is >> executed in each case and what is missing? > > This patch is actually both a bug fix and a component of compliance testing. > Because HPD events are received both on connect and disconnect actions, it's > vital that we don't try and train the link when we're transitioning from > connected->disconnected. That results in errors and warning in the logs from > failed AUX transactions and can trigger the WARN for the check of > !base.crtc. By making the check at the beginning to see if the connection is > truly active, those problems are avoided and testing / link training will > only be attempted when there is a valid Displayport connection. > >> Since there appears to be 2 different changes, shouldn't this patch be >> split into 2 different patches? > > It can be split into two if that will make it easier to upstream. The > changes are separate but related, which is why I grouped them into a single > patch. The big reason for splitting is the bisectability. In case we ever bisect a bug to this patch, we'll probably have to ask the user to test additional patches so we can check which of the logical changes actually introduced the problem. Anyway, since it's still a small patch, I don't think it's a big problem, so splitting is not a hard requirement IMHO. > > >>> If a test request is present during a connected->disconnected transition, >>> the test code will attempt to execute even though the connection has been >>> disabled, >>> resulting in a faied test. >>> >>> Signed-off-by: Todd Previte<tprevite@gmail.com> >>> --- >>> drivers/gpu/drm/i915/intel_dp.c | 32 +++++++++++++++++++------------- >>> 1 file changed, 19 insertions(+), 13 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_dp.c >>> b/drivers/gpu/drm/i915/intel_dp.c >>> index 3dc92a3..1b452cc 100644 >>> --- a/drivers/gpu/drm/i915/intel_dp.c >>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>> @@ -3890,21 +3890,14 @@ intel_dp_check_link_status(struct intel_dp >>> *intel_dp) >>> >>> >>> WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); >>> >>> - if (!intel_encoder->connectors_active) >>> - return; >>> - >>> - if (WARN_ON(!intel_encoder->base.crtc)) >>> - return; >>> - >>> - if (!to_intel_crtc(intel_encoder->base.crtc)->active) >>> - return; >>> - >>> - /* Try to read receiver status if the link appears to be up */ >>> - if (!intel_dp_get_link_status(intel_dp, link_status)) { >>> + /* Bail if not connected */ >> >> Bikeshed: I don't see the reason for the comment above :) >> >> Other than that, the patch looks fine. Let's see what PRTS will say about >> it. > > > Fair enough. Comment deleted. :) Fix will be in patch V3. The above > responses to your questions were also included in the patch notes for > further explanation of this patch. > > >>> + if (intel_dp->attached_connector->base.status != >>> + connector_status_connected) { >>> + DRM_DEBUG_KMS("Not connected\n"); >>> return; >>> } >>> >>> - /* Now read the DPCD to see if it's actually running */ >>> + /* Attempt to read the DPCD */ >>> if (!intel_dp_get_dpcd(intel_dp)) { >>> return; >>> } >>> @@ -3916,13 +3909,26 @@ intel_dp_check_link_status(struct intel_dp >>> *intel_dp) >>> drm_dp_dpcd_writeb(&intel_dp->aux, >>> DP_DEVICE_SERVICE_IRQ_VECTOR, >>> sink_irq_vector); >>> - >>> if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST) >>> intel_dp_handle_test_request(intel_dp); >>> if (sink_irq_vector & (DP_CP_IRQ | >>> DP_SINK_SPECIFIC_IRQ)) >>> DRM_DEBUG_DRIVER("CP or sink specific irq >>> unhandled\n"); >>> } >>> >>> + if (!intel_encoder->connectors_active) >>> + return; >>> + >>> + if (WARN_ON(!intel_encoder->base.crtc)) >>> + return; >>> + >>> + if (!to_intel_crtc(intel_encoder->base.crtc)->active) >>> + return; >>> + >>> + /* Try to read receiver status if the link appears to be up */ >>> + if (!intel_dp_get_link_status(intel_dp, link_status)) { >>> + return; >>> + } >>> + >>> if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) { >>> DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n", >>> intel_encoder->base.name); >>> -- >>> 1.9.1 >>> >>> _______________________________________________ >>> 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 3dc92a3..1b452cc 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3890,21 +3890,14 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); - if (!intel_encoder->connectors_active) - return; - - if (WARN_ON(!intel_encoder->base.crtc)) - return; - - if (!to_intel_crtc(intel_encoder->base.crtc)->active) - return; - - /* Try to read receiver status if the link appears to be up */ - if (!intel_dp_get_link_status(intel_dp, link_status)) { + /* Bail if not connected */ + if (intel_dp->attached_connector->base.status != + connector_status_connected) { + DRM_DEBUG_KMS("Not connected\n"); return; } - /* Now read the DPCD to see if it's actually running */ + /* Attempt to read the DPCD */ if (!intel_dp_get_dpcd(intel_dp)) { return; } @@ -3916,13 +3909,26 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) drm_dp_dpcd_writeb(&intel_dp->aux, DP_DEVICE_SERVICE_IRQ_VECTOR, sink_irq_vector); - if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST) intel_dp_handle_test_request(intel_dp); if (sink_irq_vector & (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ)) DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n"); } + if (!intel_encoder->connectors_active) + return; + + if (WARN_ON(!intel_encoder->base.crtc)) + return; + + if (!to_intel_crtc(intel_encoder->base.crtc)->active) + return; + + /* Try to read receiver status if the link appears to be up */ + if (!intel_dp_get_link_status(intel_dp, link_status)) { + return; + } + if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) { DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n", intel_encoder->base.name);
Move the DPCD read to the top and check for an interrupt from the sink to catch Displayport automated testing requests necessary to support Displayport compliance testing. The checks for active connectors and link status are moved below the check for the interrupt. Adds a check at the top to verify that the device is connected. This is necessary for DP compliance testing to ensure that test requests are captured and acknowledged. If a test request is present during a connected->disconnected transition, the test code will attempt to execute even though the connection has been disabled, resulting in a faied test. Signed-off-by: Todd Previte <tprevite@gmail.com> --- drivers/gpu/drm/i915/intel_dp.c | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-)