Re: [Intel-gfx] [PATCH 01/10] drm/i915: Add automated testing support for Displayport compliance testing
On Thu, Oct 23, 2014 at 09:58:47AM -0700, Todd Previte wrote:
On 10/20/2014 10:48 AM, Paulo Zanoni wrote:
2014-10-09 12:38 GMT-03:00 Todd Previte tprev...@gmail.com:
+/* Displayport compliance testing - PHY pattern testing */
+static uint8_t
+intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp)
+{
+ uint8_t test_result = DP_TEST_NAK;
+ return test_result;
+}
I guess a lot of people would have just made the code return NAK
without even defining/calling these functions above.
It came down to two ways of implementing this initial patch. I could have
NAK'd in the handler and omitted these functions until they were fully
implemented. Or I could do what I did here, which was to put the skeleton in
place and add the flesh to bones when ready. To me, this seems to make more
sense to me because it gets the structure in place and builds on it in
subsequent patches.
Random drive-by comment: Imo rolling out the scaffolding in this patch
here looks ok. Personally I'd probably have done what Paulo suggested
too. Maybe even with a default switch that just yells with a
DRM_ERROR(Test not yet implement: %i\n, testcode) and then fill things
out step-by-step. But that's kinda a bikeshed ;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - 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 01/10] drm/i915: Add automated testing support for Displayport compliance testing
On Thu, Oct 09, 2014 at 08:38:01AM -0700, Todd Previte wrote:
Add the skeleton framework for supporting automation for Displayport
compliance
testing. This patch adds the necessary framework for the source device to
appropriately
respond to test automation requests from a sink device.
Signed-off-by: Todd Previte tprev...@gmail.com
Patch is missing the patch changelog here which details your changes
compared to the earlier version. This is especially important since Paulo
has already started reviewing your patches, so it's good practice to
mention all the things you've changed. And much more important, which
parts you didn't change - that _must_ happen, eithere in the commit
message or with a reply to Paulo's review. Also, please cc any people who
previously commented on a patch so that they have a chance to catch the
next round.
Without all that review iterations are a massive pain for reviewers since
they might miss you resends and have to re-read all the patches again
completely to check that you didn't miss anything.
Thanks, Daniel
---
drivers/gpu/drm/i915/intel_dp.c | 82
-
1 file changed, 80 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 64c8e04..f7d4119 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3959,11 +3959,89 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp,
u8 *sink_irq_vector)
return true;
}
+/* Displayport compliance testing - Link training */
+static uint8_t
+intel_dp_autotest_link_training(struct intel_dp *intel_dp)
+{
+ uint8_t test_result = DP_TEST_NAK;
+ return test_result;
+}
+
+/* Displayport compliance testing - Video pattern testing */
+static uint8_t
+intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
+{
+ uint8_t test_result = DP_TEST_NAK;
+ return test_result;
+}
+
+/* Displayport compliance testing - EDID operations */
+static uint8_t
+intel_dp_autotest_edid(struct intel_dp *intel_dp)
+{
+ uint8_t test_result = DP_TEST_NAK;
+ return test_result;
+}
+
+/* Displayport compliance testing - PHY pattern testing */
+static uint8_t
+intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp)
+{
+ uint8_t test_result = DP_TEST_NAK;
+ return test_result;
+}
+
static void
intel_dp_handle_test_request(struct intel_dp *intel_dp)
{
- /* NAK by default */
- drm_dp_dpcd_writeb(intel_dp-aux, DP_TEST_RESPONSE, DP_TEST_NAK);
+ uint8_t response = DP_TEST_NAK;
+ uint8_t rxdata = 0;
+ int status = 0;
+
+ DRM_DEBUG_KMS(Received automated test request\n);
+ status = drm_dp_dpcd_read(intel_dp-aux, DP_TEST_REQUEST, rxdata, 1);
+
+ /* ACK/NAK response based on test function response
+Unimplemented/unsupported tests will NAK by default */
+ switch (rxdata) {
+ case DP_TEST_LINK_TRAINING:
+ DRM_DEBUG_KMS(Executing LINK_TRAINING request\n);
+ intel_dp-compliance_test_data = DP_TEST_LINK_TRAINING;
+ response = intel_dp_autotest_link_training(intel_dp);
+ break;
+ case DP_TEST_LINK_VIDEO_PATTERN:
+ DRM_DEBUG_KMS(Executing TEST_PATTERN request\n);
+ intel_dp-compliance_test_data = DP_TEST_LINK_VIDEO_PATTERN;
+ response = intel_dp_autotest_video_pattern(intel_dp);
+ break;
+ case DP_TEST_LINK_EDID_READ:
+ DRM_DEBUG_KMS(Executing EDID request\n);
+ intel_dp-compliance_test_data = DP_TEST_LINK_EDID_READ;
+ response = intel_dp_autotest_edid(intel_dp);
+ break;
+ case DP_TEST_LINK_PHY_TEST_PATTERN:
+ DRM_DEBUG_KMS(Executing PHY_PATTERN request\n);
+ intel_dp-compliance_test_data = DP_TEST_LINK_PHY_TEST_PATTERN;
+ response = intel_dp_autotest_phy_pattern(intel_dp);
+ break;
+ /* FAUX is optional in DP 1.2*/
+ case DP_TEST_LINK_FAUX_PATTERN:
+ DRM_DEBUG_KMS(FAUX_PATTERN testing not supported\n);
+ break;
+ /* Unsupported test case or something went wrong */
+ default:
+ DRM_DEBUG_KMS(Unhandled test request\n);
+ break;
+ }
+ if (status != 0) {
+ response = DP_TEST_NAK;
+ DRM_DEBUG_KMS(Error %d processing test request\n, status);
+ }
+ status = drm_dp_dpcd_write(intel_dp-aux,
+DP_TEST_RESPONSE,
+response, 1);
+ intel_dp-compliance_testing_active = 0;
+
}
static int
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
Re: [Intel-gfx] [PATCH 01/10] drm/i915: Add automated testing support for Displayport compliance testing
Version 2 in process now. Responses in line below.
On 10/20/2014 10:48 AM, Paulo Zanoni wrote:
Hi
Since I already commented about the coding style on the previous
reviews, I'll ignore that aspect for the comments below.
I think all the style issues have been resolved. Checkpatch.pl has 0
errors, 0 warnings, so that should be good to go. Excess comments have
also been removed. If I missed anything though, please let me know.
2014-10-09 12:38 GMT-03:00 Todd Previte tprev...@gmail.com:
Add the skeleton framework for supporting automation for Displayport compliance
testing. This patch adds the necessary framework for the source device to
appropriately
respond to test automation requests from a sink device.
Signed-off-by: Todd Previte tprev...@gmail.com
---
drivers/gpu/drm/i915/intel_dp.c | 82 -
1 file changed, 80 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 64c8e04..f7d4119 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3959,11 +3959,89 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8
*sink_irq_vector)
return true;
}
+/* Displayport compliance testing - Link training */
+static uint8_t
+intel_dp_autotest_link_training(struct intel_dp *intel_dp)
+{
+ uint8_t test_result = DP_TEST_NAK;
+ return test_result;
+}
+
+/* Displayport compliance testing - Video pattern testing */
+static uint8_t
+intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
+{
+ uint8_t test_result = DP_TEST_NAK;
+ return test_result;
+}
+
+/* Displayport compliance testing - EDID operations */
+static uint8_t
+intel_dp_autotest_edid(struct intel_dp *intel_dp)
+{
+ uint8_t test_result = DP_TEST_NAK;
+ return test_result;
+}
+
+/* Displayport compliance testing - PHY pattern testing */
+static uint8_t
+intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp)
+{
+ uint8_t test_result = DP_TEST_NAK;
+ return test_result;
+}
I guess a lot of people would have just made the code return NAK
without even defining/calling these functions above.
It came down to two ways of implementing this initial patch. I could
have NAK'd in the handler and omitted these functions until they were
fully implemented. Or I could do what I did here, which was to put the
skeleton in place and add the flesh to bones when ready. To me, this
seems to make more sense to me because it gets the structure in place
and builds on it in subsequent patches.
+
static void
intel_dp_handle_test_request(struct intel_dp *intel_dp)
{
- /* NAK by default */
- drm_dp_dpcd_writeb(intel_dp-aux, DP_TEST_RESPONSE, DP_TEST_NAK);
+ uint8_t response = DP_TEST_NAK;
+ uint8_t rxdata = 0;
+ int status = 0;
+
+ DRM_DEBUG_KMS(Received automated test request\n);
+ status = drm_dp_dpcd_read(intel_dp-aux, DP_TEST_REQUEST, rxdata, 1);
+
+ /* ACK/NAK response based on test function response
+ Unimplemented/unsupported tests will NAK by default */
+ switch (rxdata) {
You're reading rxdata without checking for status first.
Good catch. Fixed. The status check at the end should have been up at
the top as well.
+ case DP_TEST_LINK_TRAINING:
+ DRM_DEBUG_KMS(Executing LINK_TRAINING request\n);
As I said on the previous review of the same patch: we're lying here.
We won't execute anything yet.
I reread the previous comments and after reviewing the code, I can see
how this could be misleading or confusing.
The message has been changed to test name test requested.
+ intel_dp-compliance_test_data = DP_TEST_LINK_TRAINING;
+ response = intel_dp_autotest_link_training(intel_dp);
+ break;
+ case DP_TEST_LINK_VIDEO_PATTERN:
+ DRM_DEBUG_KMS(Executing TEST_PATTERN request\n);
+ intel_dp-compliance_test_data = DP_TEST_LINK_VIDEO_PATTERN;
+ response = intel_dp_autotest_video_pattern(intel_dp);
+ break;
+ case DP_TEST_LINK_EDID_READ:
+ DRM_DEBUG_KMS(Executing EDID request\n);
+ intel_dp-compliance_test_data = DP_TEST_LINK_EDID_READ;
+ response = intel_dp_autotest_edid(intel_dp);
+ break;
+ case DP_TEST_LINK_PHY_TEST_PATTERN:
+ DRM_DEBUG_KMS(Executing PHY_PATTERN request\n);
+ intel_dp-compliance_test_data = DP_TEST_LINK_PHY_TEST_PATTERN;
+ response = intel_dp_autotest_phy_pattern(intel_dp);
+ break;
+ /* FAUX is optional in DP 1.2*/
+ case DP_TEST_LINK_FAUX_PATTERN:
+ DRM_DEBUG_KMS(FAUX_PATTERN testing not supported\n);
+ break;
+ /* Unsupported test case or something went wrong */
Is there a way to differentiate the unsupported and the went wrong
cases on dmesg?
I gave this one some thought and
Re: [Intel-gfx] [PATCH 01/10] drm/i915: Add automated testing support for Displayport compliance testing
Hi
Since I already commented about the coding style on the previous
reviews, I'll ignore that aspect for the comments below.
2014-10-09 12:38 GMT-03:00 Todd Previte tprev...@gmail.com:
Add the skeleton framework for supporting automation for Displayport
compliance
testing. This patch adds the necessary framework for the source device to
appropriately
respond to test automation requests from a sink device.
Signed-off-by: Todd Previte tprev...@gmail.com
---
drivers/gpu/drm/i915/intel_dp.c | 82
-
1 file changed, 80 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 64c8e04..f7d4119 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3959,11 +3959,89 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp,
u8 *sink_irq_vector)
return true;
}
+/* Displayport compliance testing - Link training */
+static uint8_t
+intel_dp_autotest_link_training(struct intel_dp *intel_dp)
+{
+ uint8_t test_result = DP_TEST_NAK;
+ return test_result;
+}
+
+/* Displayport compliance testing - Video pattern testing */
+static uint8_t
+intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
+{
+ uint8_t test_result = DP_TEST_NAK;
+ return test_result;
+}
+
+/* Displayport compliance testing - EDID operations */
+static uint8_t
+intel_dp_autotest_edid(struct intel_dp *intel_dp)
+{
+ uint8_t test_result = DP_TEST_NAK;
+ return test_result;
+}
+
+/* Displayport compliance testing - PHY pattern testing */
+static uint8_t
+intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp)
+{
+ uint8_t test_result = DP_TEST_NAK;
+ return test_result;
+}
I guess a lot of people would have just made the code return NAK
without even defining/calling these functions above.
+
static void
intel_dp_handle_test_request(struct intel_dp *intel_dp)
{
- /* NAK by default */
- drm_dp_dpcd_writeb(intel_dp-aux, DP_TEST_RESPONSE, DP_TEST_NAK);
+ uint8_t response = DP_TEST_NAK;
+ uint8_t rxdata = 0;
+ int status = 0;
+
+ DRM_DEBUG_KMS(Received automated test request\n);
+ status = drm_dp_dpcd_read(intel_dp-aux, DP_TEST_REQUEST, rxdata,
1);
+
+ /* ACK/NAK response based on test function response
+ Unimplemented/unsupported tests will NAK by default */
+ switch (rxdata) {
You're reading rxdata without checking for status first.
+ case DP_TEST_LINK_TRAINING:
+ DRM_DEBUG_KMS(Executing LINK_TRAINING request\n);
As I said on the previous review of the same patch: we're lying here.
We won't execute anything yet.
+ intel_dp-compliance_test_data = DP_TEST_LINK_TRAINING;
+ response = intel_dp_autotest_link_training(intel_dp);
+ break;
+ case DP_TEST_LINK_VIDEO_PATTERN:
+ DRM_DEBUG_KMS(Executing TEST_PATTERN request\n);
+ intel_dp-compliance_test_data = DP_TEST_LINK_VIDEO_PATTERN;
+ response = intel_dp_autotest_video_pattern(intel_dp);
+ break;
+ case DP_TEST_LINK_EDID_READ:
+ DRM_DEBUG_KMS(Executing EDID request\n);
+ intel_dp-compliance_test_data = DP_TEST_LINK_EDID_READ;
+ response = intel_dp_autotest_edid(intel_dp);
+ break;
+ case DP_TEST_LINK_PHY_TEST_PATTERN:
+ DRM_DEBUG_KMS(Executing PHY_PATTERN request\n);
+ intel_dp-compliance_test_data =
DP_TEST_LINK_PHY_TEST_PATTERN;
+ response = intel_dp_autotest_phy_pattern(intel_dp);
+ break;
+ /* FAUX is optional in DP 1.2*/
+ case DP_TEST_LINK_FAUX_PATTERN:
+ DRM_DEBUG_KMS(FAUX_PATTERN testing not supported\n);
+ break;
+ /* Unsupported test case or something went wrong */
Is there a way to differentiate the unsupported and the went wrong
cases on dmesg?
+ default:
+ DRM_DEBUG_KMS(Unhandled test request\n);
+ break;
+ }
+ if (status != 0) {
+ response = DP_TEST_NAK;
+ DRM_DEBUG_KMS(Error %d processing test request\n, status);
+ }
+ status = drm_dp_dpcd_write(intel_dp-aux,
+ DP_TEST_RESPONSE,
+ response, 1);
+ intel_dp-compliance_testing_active = 0;
+
}
And the most important thing: the patch doesn't compile. Please make
sure each patch in the series compiles and works. We do tons of git
bisections on our tree, that's really important.
static int
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
