[PATCH v2] Displayport compliance testing

[PATCH v2] Displayport compliance testing

[Todd Previte]

>This patch set adds the foundational support for Displayport compliance testing in the
>i915 driver. It implements the framework for automated test support that preclude the
>need (most) for operator input during testing. Tests for AUX transactions, EDID reads
>and basic link training have also been included, along with any support and utility
>functions required. Some changes have also been made to existing code to accommodate
>compliance testing.

V2:
- Addressed review feedback from the mailing list
- Broke up patches into smaller, easily managed chunks
- Reordered the patches such that they can be applied in order
- Fixed checkpatch.pl errors across the patchset  
- Updated and enhanced functionality for the EDID test function
- Completely revamped the mode set operations for compliance testing

[PATCH 01/12] drm/i915: Add automated testing support for Displayport compliance testing

[Todd Previte]

Add the skeleton framework for supporting automation for Displayport compliance testing. This patch
adds the necessary framework for the source device to appropriately responded to test automation
requests from a sink device.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 84 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 82 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b5ec489..0d11145 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3328,11 +3328,91 @@ intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
 				       sink_irq_vector, 1) == 1;
 }
 
+/* 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;
+}
+
+/* Displayport compliance testing - Fast AUX transactions (optional) */
+static uint8_t
+intel_dp_autotest_faux_pattern(struct intel_dp *intel_dp)
+{
+	uint8_t test_result = DP_TEST_NAK;
+	DRM_DEBUG_KMS("Displayport: Fast AUX (FAUX) not supported\n");
+	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 ret = 0;
+
+	DRM_DEBUG_KMS("Displayport: Received automated test request\n");
+	/* Read DP_TEST_REQUEST register to identify the requested test */
+	ret = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1);
+
+	/* Determine which test has been requested */
+	switch (rxdata) {
+	/* ACK/NAK response based on test function response
+	   Unimplemented/unsupported tests will NAK */
+	case DP_TEST_LINK_TRAINING:
+		DRM_DEBUG_KMS("Displayport: Executing LINK_TRAINING request\n");
+		response = intel_dp_autotest_link_training(intel_dp);
+		break;
+	case DP_TEST_LINK_VIDEO_PATTERN:
+		DRM_DEBUG_KMS("Displayport: Executing TEST_PATTERN request\n");
+		response = intel_dp_autotest_video_pattern(intel_dp);
+		break;
+	case DP_TEST_LINK_EDID_READ:
+		DRM_DEBUG_KMS("Displayport: Executing EDID request\n");
+		response = intel_dp_autotest_edid(intel_dp);
+		break;
+	case DP_TEST_LINK_PHY_TEST_PATTERN:
+		DRM_DEBUG_KMS("Displayport: Executing PHY_PATTERN request\n");
+		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("Displayport: Executing FAUX_PATTERN request\n");
+		response = intel_dp_autotest_faux_pattern(intel_dp);
+		break;
+	/* Unsupported test case or something went wrong */
+	default:
+		/* Log error here for unhandled test request */
+		DRM_DEBUG_KMS("Displayport: Unhandled test request\n");
+		break;
+	}
+	/* Send the response from the test result */
+	ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TEST_RESPONSE, &response, 1);
 }
 
 /*
-- 
1.9.1

Re: [PATCH 01/12] drm/i915: Add automated testing support for Displayport compliance testing

[Paulo Zanoni]

Hi

2014-07-14 16:10 GMT-03:00 Todd Previte <tprevite@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 responded to test automation
> requests from a sink device.

These commit messages contain some long lines. Most of the patches I
see go up to 70 or at most 80 columns on each line on the commit
message, but your patches seem to have a 100-column limit.


>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 84 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 82 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b5ec489..0d11145 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3328,11 +3328,91 @@ intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>                                        sink_irq_vector, 1) == 1;
>  }
>
> +/* Displayport compliance testing - Link training */

Does the line above contain the formal name of the test from a given
specification, or is it just a description? If it's the exact name of
the test, then please state that it is the name, and also name the
specification it comes from. But if it's just a description, I don't
really see the value of a comment containing "DP compliance testing -
link training", when the function is already called
intel_dp_autotest_link_training(): that would be just duplicate
information.

In your patch series I see many other examples of comments that don't
add any new information. For example, on patch 7 we have:

    /* Reset the NACK/DEFER counters */
    intel_dp->aux.i2c_nack_count = 0;
    intel_dp->aux.i2c_defer_count = 0;

You don't really need to say you're about to reset a counter if the
code is already saying that. Also, if someone ever changes the code
but forgets to fix the comment, you will have an inconsistency that
will lead code readers confused.

But think about the good side: if the comments are useless, it's
because the code is readable :). And that should be the goal: code
that is clear enough to not really need comments. Please read
Documentation/CodingStyle, chapter 8 inside the Kernel source tree.

For this and the next patches of the series, I won't go and list every
single comment I think should be removed, but keep in mind that IMHO
95% of them could be removed.


> +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;
> +}
> +
> +/* Displayport compliance testing - Fast AUX transactions (optional) */
> +static uint8_t
> +intel_dp_autotest_faux_pattern(struct intel_dp *intel_dp)
> +{
> +       uint8_t test_result = DP_TEST_NAK;
> +       DRM_DEBUG_KMS("Displayport: Fast AUX (FAUX) not supported\n");

Why do we print this here and not for the other tests?


> +       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;

I would probably have named this "requested_test" or something else.


> +       int ret = 0;

Vairable "ret" is set but never read anywhere. If you edit
drivers/gpu/drm/i915/Makefile, you can get a compiler warning:

drivers/gpu/drm/i915/intel_dp.c: In function ‘intel_dp_handle_test_request’:
drivers/gpu/drm/i915/intel_dp.c:3467:6: warning: variable ‘ret’ set
but not used [-Wunused-but-set-variable]

You will also see that there are a few more points of our driver where
we make similar mistakes :)


> +
> +       DRM_DEBUG_KMS("Displayport: Received automated test request\n");
> +       /* Read DP_TEST_REQUEST register to identify the requested test */
> +       ret = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1);
> +
> +       /* Determine which test has been requested */
> +       switch (rxdata) {
> +       /* ACK/NAK response based on test function response
> +          Unimplemented/unsupported tests will NAK */
> +       case DP_TEST_LINK_TRAINING:
> +               DRM_DEBUG_KMS("Displayport: Executing LINK_TRAINING request\n");

We're kinda lying here, since we're still NAKing stuff, aren't we?

Thanks,
Paulo

> +               response = intel_dp_autotest_link_training(intel_dp);
> +               break;
> +       case DP_TEST_LINK_VIDEO_PATTERN:
> +               DRM_DEBUG_KMS("Displayport: Executing TEST_PATTERN request\n");
> +               response = intel_dp_autotest_video_pattern(intel_dp);
> +               break;
> +       case DP_TEST_LINK_EDID_READ:
> +               DRM_DEBUG_KMS("Displayport: Executing EDID request\n");
> +               response = intel_dp_autotest_edid(intel_dp);
> +               break;
> +       case DP_TEST_LINK_PHY_TEST_PATTERN:
> +               DRM_DEBUG_KMS("Displayport: Executing PHY_PATTERN request\n");
> +               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("Displayport: Executing FAUX_PATTERN request\n");
> +               response = intel_dp_autotest_faux_pattern(intel_dp);
> +               break;
> +       /* Unsupported test case or something went wrong */
> +       default:
> +               /* Log error here for unhandled test request */
> +               DRM_DEBUG_KMS("Displayport: Unhandled test request\n");
> +               break;
> +       }
> +       /* Send the response from the test result */
> +       ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TEST_RESPONSE, &response, 1);
>  }
>
>  /*
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/12] drm/i915: Add automated testing support for Displayport compliance testing

[Paulo Zanoni]

2014-07-14 16:10 GMT-03:00 Todd Previte <tprevite@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 responded to test automation
> requests from a sink device.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 84 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 82 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b5ec489..0d11145 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3328,11 +3328,91 @@ intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>                                        sink_irq_vector, 1) == 1;
>  }
>
> +/* 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;
> +}
> +
> +/* Displayport compliance testing - Fast AUX transactions (optional) */
> +static uint8_t
> +intel_dp_autotest_faux_pattern(struct intel_dp *intel_dp)
> +{
> +       uint8_t test_result = DP_TEST_NAK;
> +       DRM_DEBUG_KMS("Displayport: Fast AUX (FAUX) not supported\n");
> +       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 ret = 0;
> +
> +       DRM_DEBUG_KMS("Displayport: Received automated test request\n");
> +       /* Read DP_TEST_REQUEST register to identify the requested test */
> +       ret = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1);
> +
> +       /* Determine which test has been requested */
> +       switch (rxdata) {
> +       /* ACK/NAK response based on test function response
> +          Unimplemented/unsupported tests will NAK */
> +       case DP_TEST_LINK_TRAINING:
> +               DRM_DEBUG_KMS("Displayport: Executing LINK_TRAINING request\n");
> +               response = intel_dp_autotest_link_training(intel_dp);
> +               break;
> +       case DP_TEST_LINK_VIDEO_PATTERN:
> +               DRM_DEBUG_KMS("Displayport: Executing TEST_PATTERN request\n");
> +               response = intel_dp_autotest_video_pattern(intel_dp);
> +               break;
> +       case DP_TEST_LINK_EDID_READ:
> +               DRM_DEBUG_KMS("Displayport: Executing EDID request\n");
> +               response = intel_dp_autotest_edid(intel_dp);
> +               break;
> +       case DP_TEST_LINK_PHY_TEST_PATTERN:
> +               DRM_DEBUG_KMS("Displayport: Executing PHY_PATTERN request\n");
> +               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("Displayport: Executing FAUX_PATTERN request\n");
> +               response = intel_dp_autotest_faux_pattern(intel_dp);
> +               break;
> +       /* Unsupported test case or something went wrong */
> +       default:
> +               /* Log error here for unhandled test request */
> +               DRM_DEBUG_KMS("Displayport: Unhandled test request\n");
> +               break;
> +       }
> +       /* Send the response from the test result */
> +       ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TEST_RESPONSE, &response, 1);

Just one more idea: we could count amount of time passed during the
test execution, since the spec defines some timeouts.

>  }
>
>  /*
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[PATCH 02/12] drm/i915: Add a function to compute the EDID checksum for Displayport compliance

[Todd Previte]

This function computes the EDID checksum for a block of EDID data. This function
is necessary for Displayport compliance testing as it does not not require the
complete EDID checking functionality provided by the DRM layer functions.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 0d11145..f61502e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3328,6 +3328,29 @@ intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
 				       sink_irq_vector, 1) == 1;
 }
 
+static bool intel_dp_compute_edid_checksum(uint8_t *edid_data, uint8_t *edid_checksum)
+{
+	uint32_t byte_total = 0;
+	uint8_t i = 0;
+	bool edid_ok = true;
+
+	/* Compute byte total w/o the checksum value*/
+	for (i = 0; i < EDID_LENGTH - 2; i++)
+			byte_total += edid_data[i];
+
+	DRM_DEBUG_KMS("Displayport: EDID total = %d, EDID checksum =  %d\n", byte_total, edid_data[EDID_LENGTH - 1]);
+
+	/* Compute the checksum */
+	*edid_checksum = 256 - (byte_total % 256);
+
+	if (*edid_checksum != edid_data[EDID_LENGTH - 1]) {
+		DRM_DEBUG_KMS("Displayport: Invalid EDID checksum %d, should be %d\n", edid_data[EDID_LENGTH - 1], *edid_checksum);
+		edid_ok = false;
+	}
+
+	return edid_ok;
+}
+
 /* Displayport compliance testing - Link training */
 static uint8_t
 intel_dp_autotest_link_training(struct intel_dp *intel_dp)
-- 
1.9.1

Re: [PATCH 02/12] drm/i915: Add a function to compute the EDID checksum for Displayport compliance

[Paulo Zanoni]

2014-07-14 16:10 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> This function computes the EDID checksum for a block of EDID data. This function
> is necessary for Displayport compliance testing as it does not not require the
> complete EDID checking functionality provided by the DRM layer functions.

Can you give specific reasons why you can't reuse
drm_edid_block_valid(), or extract a piece of it to a new function
that will be called by both the i915 code and drm_edid_block_valid()?


>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 0d11145..f61502e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3328,6 +3328,29 @@ intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>                                        sink_irq_vector, 1) == 1;
>  }
>
> +static bool intel_dp_compute_edid_checksum(uint8_t *edid_data, uint8_t *edid_checksum)

A little note about patch-splitting: here you are adding a function
that is not called anywhere. This is not a common thing, we usually
only add a function on the patch where it gets called for the first
time. It is much easier to review a function when it has callers,
since you can know the context of why it is really needed, and see if
it is being called correctly more easily. You can also judge if it
needs to be static or exported. Also, adding functions without callers
can produce compiler warnings, like this:

drivers/gpu/drm/i915/intel_dp.c:3421:13: warning:
‘intel_dp_compute_edid_checksum’ defined but not used
[-Wunused-function]

You do the same thing with patch 4, 5 and 6. IMHO, you should squash
all these patches on patch 7. No problem that patch 7 is going to get
big: it is still one logical change.

Another example of why I think this should be squashed: when I see
patch 7, I don't see the need for the "edid_checksum" argument of this
function. Even if you're going to use the argument later, it's better
if you only patch the function to add the argument at the point you
really need it.


> +{
> +       uint32_t byte_total = 0;
> +       uint8_t i = 0;
> +       bool edid_ok = true;
> +
> +       /* Compute byte total w/o the checksum value*/
> +       for (i = 0; i < EDID_LENGTH - 2; i++)
> +                       byte_total += edid_data[i];

Too many tabs here.


> +
> +       DRM_DEBUG_KMS("Displayport: EDID total = %d, EDID checksum =  %d\n", byte_total, edid_data[EDID_LENGTH - 1]);

We allow a line to have more than 80 chars when the message being
print is too big and you would need to cut it in the middle, but on
this case you could have added a line break after the end of the
string, leaving "bytes_total, edid_data[...]);" on the line below.
This would have kept both lines smaller than the 80-column
restriction, without breaking a greppable debug message. Also, keep in
mind that DRM_DEBUG_KMS also prints the name of the function, so you
probably don't need to print, for example, the "Displayport"
characters. The same comment applies for the DRM_DEBUG_KMS call below.

Thanks,
Paulo

> +
> +       /* Compute the checksum */
> +       *edid_checksum = 256 - (byte_total % 256);
> +
> +       if (*edid_checksum != edid_data[EDID_LENGTH - 1]) {
> +               DRM_DEBUG_KMS("Displayport: Invalid EDID checksum %d, should be %d\n", edid_data[EDID_LENGTH - 1], *edid_checksum);
> +               edid_ok = false;
> +       }
> +
> +       return edid_ok;
> +}
> +
>  /* Displayport compliance testing - Link training */
>  static uint8_t
>  intel_dp_autotest_link_training(struct intel_dp *intel_dp)
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 03/12] drm/i915: Add counters in the drm_dp_aux struct for I2C NACKs and DEFERs

[Todd Previte]

These counters are used for Displayort complinace testing to detect error conditions
when executing certain compliance tests. Currently these are used in the EDID tests
to determine if the video mode needs to be set to the preferred mode or the failsafe
mode.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 2 ++
 include/drm/drm_dp_helper.h     | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 08e33b8..8353051 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -654,10 +654,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 
 		case DP_AUX_I2C_REPLY_NACK:
 			DRM_DEBUG_KMS("I2C nack\n");
+			aux->i2c_nack_count++;
 			return -EREMOTEIO;
 
 		case DP_AUX_I2C_REPLY_DEFER:
 			DRM_DEBUG_KMS("I2C defer\n");
+			aux->i2c_defer_count++;
 			usleep_range(400, 500);
 			continue;
 
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index a21568b..3749cb4 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -550,6 +550,7 @@ struct drm_dp_aux {
 	struct mutex hw_mutex;
 	ssize_t (*transfer)(struct drm_dp_aux *aux,
 			    struct drm_dp_aux_msg *msg);
+	uint8_t i2c_nack_count, i2c_defer_count;
 };
 
 ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
-- 
1.9.1

Re: [PATCH 03/12] drm/i915: Add counters in the drm_dp_aux struct for I2C NACKs and DEFERs

[Paulo Zanoni]

2014-07-14 16:10 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> These counters are used for Displayort complinace testing to detect error conditions
> when executing certain compliance tests. Currently these are used in the EDID tests
> to determine if the video mode needs to be set to the preferred mode or the failsafe
> mode.
>

It would be nice if you could cite on the commit message the name of
the specification and the name of the test(s) that use it.

Usually when I have patches that touch things outside
drivers/gpu/drm/i915, I add a Cc tag so I don't forget to email the
appropriate list:

Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 2 ++
>  include/drm/drm_dp_helper.h     | 1 +
>  2 files changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 08e33b8..8353051 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -654,10 +654,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>
>                 case DP_AUX_I2C_REPLY_NACK:
>                         DRM_DEBUG_KMS("I2C nack\n");
> +                       aux->i2c_nack_count++;
>                         return -EREMOTEIO;
>
>                 case DP_AUX_I2C_REPLY_DEFER:
>                         DRM_DEBUG_KMS("I2C defer\n");
> +                       aux->i2c_defer_count++;
>                         usleep_range(400, 500);
>                         continue;
>
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index a21568b..3749cb4 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -550,6 +550,7 @@ struct drm_dp_aux {
>         struct mutex hw_mutex;
>         ssize_t (*transfer)(struct drm_dp_aux *aux,
>                             struct drm_dp_aux_msg *msg);
> +       uint8_t i2c_nack_count, i2c_defer_count;

Does it really need to be uint8_t? I see on patch 7 that you don't
really write this value to a place that only accepts uint8_t-sized
arguments, so I fear that if we get 256 NACKs or DEFERs we may end up
doing the wrong thing.

Also, why don't we need to count the native NACKs and DEFERs?

>  };
>
>  ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[PATCH 02/10] drm/i915: Add counters in the drm_dp_aux struct for I2C NACKs and DEFERs

[Todd Previte]

These counters are used for Displayort compliance testing to detect error
conditions when executing tests 4.2.2.4 and 4.2.2.5 in the Displayport Link
CTS specificaiton. They determine whether to use the preferred/requested
mode or the failsafe mode during these tests.

V2:
- Addressed previous review feedback
- Updated commit message
- Changed from uint8_t to uint32_t

Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 2 ++
 include/drm/drm_dp_helper.h     | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 08e33b8..8353051 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -654,10 +654,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 
 		case DP_AUX_I2C_REPLY_NACK:
 			DRM_DEBUG_KMS("I2C nack\n");
+			aux->i2c_nack_count++;
 			return -EREMOTEIO;
 
 		case DP_AUX_I2C_REPLY_DEFER:
 			DRM_DEBUG_KMS("I2C defer\n");
+			aux->i2c_defer_count++;
 			usleep_range(400, 500);
 			continue;
 
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 8edeed0..23082ce 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -551,6 +551,7 @@ struct drm_dp_aux {
 	struct mutex hw_mutex;
 	ssize_t (*transfer)(struct drm_dp_aux *aux,
 			    struct drm_dp_aux_msg *msg);
+	uint32_t i2c_nack_count, i2c_defer_count;
 };
 
 ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 02/10] drm/i915: Add counters in the drm_dp_aux struct for I2C NACKs and DEFERs

[Daniel Vetter]

On Tue, Nov 04, 2014 at 03:17:35PM -0700, Todd Previte wrote:
> These counters are used for Displayort compliance testing to detect error
> conditions when executing tests 4.2.2.4 and 4.2.2.5 in the Displayport Link
> CTS specificaiton. They determine whether to use the preferred/requested
> mode or the failsafe mode during these tests.
> 
> V2:
> - Addressed previous review feedback
> - Updated commit message
> - Changed from uint8_t to uint32_t
> 
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 2 ++
>  include/drm/drm_dp_helper.h     | 1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 08e33b8..8353051 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -654,10 +654,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>  
>  		case DP_AUX_I2C_REPLY_NACK:
>  			DRM_DEBUG_KMS("I2C nack\n");
> +			aux->i2c_nack_count++;
>  			return -EREMOTEIO;
>  
>  		case DP_AUX_I2C_REPLY_DEFER:
>  			DRM_DEBUG_KMS("I2C defer\n");
> +			aux->i2c_defer_count++;
>  			usleep_range(400, 500);
>  			continue;
>  
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 8edeed0..23082ce 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -551,6 +551,7 @@ struct drm_dp_aux {
>  	struct mutex hw_mutex;
>  	ssize_t (*transfer)(struct drm_dp_aux *aux,
>  			    struct drm_dp_aux_msg *msg);
> +	uint32_t i2c_nack_count, i2c_defer_count;

My reply missed your resend. I think unsigned instead of uint32_t has
clearer intent - at least nearyb driver code uint32_t usually means a
32bit hw register value. Which this isn't.

I've bikeshedded this and picked this up into my drm-misc tree.
-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

[PATCH 04/12] drm/i915: Add wrapper function for intel_crtc_set_config()

[Todd Previte]

Add a wrapper around intel_crtc_set_config() to allow it to be called from the
DP compliance test functions. This is necessary to perform the internal mode
set operations required for compliance testing.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_display.c | 6 ++++++
 drivers/gpu/drm/i915/intel_drv.h     | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fa77557..77ab4f3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11249,6 +11249,12 @@ out_config:
 	return ret;
 }
 
+/* Wrapper function for setting a configuration for DP compliance testing */
+int intel_dp_set_config(struct drm_mode_set *set)
+{
+	return intel_crtc_set_config(set);
+}
+
 static const struct drm_crtc_funcs intel_crtc_funcs = {
 	.gamma_set = intel_crtc_gamma_set,
 	.set_config = intel_crtc_set_config,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5f7c7bd..d2ae54f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -836,6 +836,8 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
 				 struct intel_crtc_config *pipe_config);
 int intel_format_to_fourcc(int format);
 void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
+/* Wrapper function for setting a configuration for DP compliance testing */
+int intel_dp_set_config(struct drm_mode_set *set);
 
 
 /* intel_dp.c */
-- 
1.9.1

Re: [PATCH 04/12] drm/i915: Add wrapper function for intel_crtc_set_config()

[Paulo Zanoni]

2014-07-14 16:10 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> Add a wrapper around intel_crtc_set_config() to allow it to be called from the
> DP compliance test functions. This is necessary to perform the internal mode
> set operations required for compliance testing.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 6 ++++++
>  drivers/gpu/drm/i915/intel_drv.h     | 2 ++
>  2 files changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index fa77557..77ab4f3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11249,6 +11249,12 @@ out_config:
>         return ret;
>  }
>
> +/* Wrapper function for setting a configuration for DP compliance testing */
> +int intel_dp_set_config(struct drm_mode_set *set)
> +{
> +       return intel_crtc_set_config(set);

Why not just export intel_crtc_set_config(), since the wrapper does
nothing but call the wrapped thing? I see you don't change this
function on the other patches.


> +}
> +
>  static const struct drm_crtc_funcs intel_crtc_funcs = {
>         .gamma_set = intel_crtc_gamma_set,
>         .set_config = intel_crtc_set_config,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 5f7c7bd..d2ae54f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -836,6 +836,8 @@ void intel_mode_from_pipe_config(struct drm_display_mode *mode,
>                                  struct intel_crtc_config *pipe_config);
>  int intel_format_to_fourcc(int format);
>  void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
> +/* Wrapper function for setting a configuration for DP compliance testing */
> +int intel_dp_set_config(struct drm_mode_set *set);
>
>
>  /* intel_dp.c */
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[PATCH 05/12] drm/i915: Add a function to get the EDID preferred mode for Displayport compliance testing

[Todd Previte]

Add a simple function to pull the preferred mode out of an EDID block. This function
is designed for use during Displayport compliance testing.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f61502e..6c8f222 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3351,6 +3351,31 @@ static bool intel_dp_compute_edid_checksum(uint8_t *edid_data, uint8_t *edid_che
 	return edid_ok;
 }
 
+static struct drm_display_mode*
+intel_dp_get_edid_preferred_mode(struct intel_dp *intel_dp)
+{
+	struct drm_display_mode *found_mode = NULL;
+	struct drm_connector *connector = &intel_dp->attached_connector->base;
+	int mode_count = 0;
+
+	list_for_each_entry(found_mode, &connector->probed_modes, head) {
+		/* Check for a preferred mode */
+		if (found_mode->type & DRM_MODE_TYPE_PREFERRED) {
+			/* Found the preferred mode, return it */
+			DRM_DEBUG_KMS("Displayport: Found preferred mode '%s'\n",
+						  found_mode->name);
+			goto exit_with_mode;
+		}
+		mode_count++;
+	}
+	/* No mode found, report the error */
+	DRM_DEBUG_KMS("Displayport: Preferred mode not found in %d probed modes\n",
+				  mode_count);
+
+exit_with_mode:
+	return found_mode;
+}
+
 /* Displayport compliance testing - Link training */
 static uint8_t
 intel_dp_autotest_link_training(struct intel_dp *intel_dp)
-- 
1.9.1

Re: [PATCH 05/12] drm/i915: Add a function to get the EDID preferred mode for Displayport compliance testing

[Paulo Zanoni]

2014-07-14 16:10 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> Add a simple function to pull the preferred mode out of an EDID block. This function
> is designed for use during Displayport compliance testing.
>

The preferred mode can be changed by the quirks. Do you think that
could be a problem?

Also, as said earlier, this should be squashed in patch 7.

> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f61502e..6c8f222 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3351,6 +3351,31 @@ static bool intel_dp_compute_edid_checksum(uint8_t *edid_data, uint8_t *edid_che
>         return edid_ok;
>  }
>
> +static struct drm_display_mode*

One of the inconsistencies with our driver is that we have both
"return_type \n function_name" and "return_type function_name" (on the
same line). I wish we were consistent. Anyway, I'd add a space between
"_mode" and '*'

> +intel_dp_get_edid_preferred_mode(struct intel_dp *intel_dp)
> +{
> +       struct drm_display_mode *found_mode = NULL;
> +       struct drm_connector *connector = &intel_dp->attached_connector->base;
> +       int mode_count = 0;
> +
> +       list_for_each_entry(found_mode, &connector->probed_modes, head) {
> +               /* Check for a preferred mode */
> +               if (found_mode->type & DRM_MODE_TYPE_PREFERRED) {
> +                       /* Found the preferred mode, return it */
> +                       DRM_DEBUG_KMS("Displayport: Found preferred mode '%s'\n",
> +                                                 found_mode->name);
> +                       goto exit_with_mode;
> +               }
> +               mode_count++;
> +       }
> +       /* No mode found, report the error */
> +       DRM_DEBUG_KMS("Displayport: Preferred mode not found in %d probed modes\n",
> +                                 mode_count);
> +
> +exit_with_mode:
> +       return found_mode;
> +}
> +
>  /* Displayport compliance testing - Link training */
>  static uint8_t
>  intel_dp_autotest_link_training(struct intel_dp *intel_dp)
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[PATCH 06/12] drm/i915: Add a constant and function for getting the Displayport compliance failsafe video mode

[Todd Previte]

Adds the failsafe mode (640x480@60hz) as a constant and a function that retrieves it. These are
designed for use in Displayport compliance testing only and should not be used outside that
context.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 6c8f222..33b6dc9 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -82,6 +82,18 @@ static const struct dp_link_dpll chv_dpll[] = {
 		{ .p1 = 2, .p2 = 1, .n = 1, .m1 = 2, .m2 = 0x6c00000 } }
 };
 
+/* VESA 640x480@60Hz - DP compliance failsafe mode */
+static struct drm_display_mode dp_failsafe_mode = {
+	DRM_MODE("640x480", DRM_MODE_TYPE_DRIVER, 25200, 640, 664, 752,
+			 800, 0, 480, 490, 492, 525, 0,
+			 DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC)
+};
+
+struct drm_display_mode *intel_dp_get_failsafe_mode(void)
+{
+       return &dp_failsafe_mode;
+}
+
 /**
  * is_edp - is the given port attached to an eDP panel (either CPU or PCH)
  * @intel_dp: DP struct
-- 
1.9.1

Re: [PATCH 06/12] drm/i915: Add a constant and function for getting the Displayport compliance failsafe video mode

[Paulo Zanoni]

2014-07-14 16:10 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> Adds the failsafe mode (640x480@60hz) as a constant and a function that retrieves it. These are
> designed for use in Displayport compliance testing only and should not be used outside that
> context.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 6c8f222..33b6dc9 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -82,6 +82,18 @@ static const struct dp_link_dpll chv_dpll[] = {
>                 { .p1 = 2, .p2 = 1, .n = 1, .m1 = 2, .m2 = 0x6c00000 } }
>  };
>
> +/* VESA 640x480@60Hz - DP compliance failsafe mode */
> +static struct drm_display_mode dp_failsafe_mode = {
> +       DRM_MODE("640x480", DRM_MODE_TYPE_DRIVER, 25200, 640, 664, 752,
> +                        800, 0, 480, 490, 492, 525, 0,
> +                        DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC)
> +};
> +
> +struct drm_display_mode *intel_dp_get_failsafe_mode(void)

Since on patch 7 you call this from intel_dp.c, and you never add a
forward declaration to any .h file, I guess this function should be
static, right? Also, I'd squash it into patch 7.

> +{
> +       return &dp_failsafe_mode;
> +}
> +
>  /**
>   * is_edp - is the given port attached to an eDP panel (either CPU or PCH)
>   * @intel_dp: DP struct
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[PATCH 07/12] drm/i915: Update EDID automated test function for Displayport compliance

[Todd Previte]

Implements an updated version of the automated testing function that handles
Displayport compliance for EDID operations.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 77 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 76 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 33b6dc9..88f1bbe 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3408,7 +3408,82 @@ intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
 static uint8_t
 intel_dp_autotest_edid(struct intel_dp *intel_dp)
 {
-	uint8_t test_result = DP_TEST_NAK;
+	struct drm_connector *connector = &intel_dp->attached_connector->base;
+	struct i2c_adapter *adapter = &intel_dp->aux.ddc;
+	struct edid *edid_read = NULL;
+	uint8_t *edid_data = NULL;
+	uint8_t test_result = DP_TEST_NAK, checksum = 0;
+	uint32_t i = 0, ret = 0;
+	struct drm_display_mode *use_mode = NULL;
+	int mode_count = 0;
+	struct drm_mode_set modeset;
+
+	DRM_DEBUG_KMS("Displayport: EDID automated test\n");
+
+	/* Reset the NACK/DEFER counters */
+	intel_dp->aux.i2c_nack_count = 0;
+	intel_dp->aux.i2c_defer_count = 0;
+	/* Now read out the EDID */
+	edid_read = drm_get_edid(connector, adapter);
+
+	if (edid_read == NULL) {
+		/* Check for NACKs/DEFERs, goto failsafe if detected
+		   (DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5) */
+		if (intel_dp->aux.i2c_nack_count > 0 ||
+			intel_dp->aux.i2c_defer_count > 0)
+			DRM_DEBUG_KMS("Displayport: EDID read generated %d I2C NACKs, %d DEFERs\n",
+						  intel_dp->aux.i2c_nack_count,
+						  intel_dp->aux.i2c_defer_count);
+		goto failsafe;
+	}
+
+	/* FIXME: Need to determine how to detect E-DDC here (4.2.2.9) */
+	edid_data = (uint8_t *) edid_read;
+
+	if (intel_dp_compute_edid_checksum(edid_data, &checksum)) {
+		/* Write the checksum to EDID checksum register */
+		ret = drm_dp_dpcd_write(&intel_dp->aux,
+								DP_TEST_EDID_CHECKSUM,
+								&edid_read->checksum, 1);
+		/* Reponse is ACK and and checksum written */
+		test_result = DP_TEST_ACK | DP_TEST_EDID_CHECKSUM_WRITE;
+	} else {
+		/* Invalid checksum - EDID corruption detection test */
+		goto failsafe;
+	}
+
+	/* Update EDID modes */
+	mode_count = intel_connector_update_modes(connector, edid_read);
+	if (!mode_count) {
+		DRM_DEBUG_KMS("Displayport: Mode update failed\n");
+		goto failsafe;
+	}
+
+	/* Get the EDID preferred mode if available */
+	use_mode = intel_dp_get_edid_preferred_mode(intel_dp);
+	if (use_mode == NULL)
+		goto failsafe;
+	else
+		goto set_mode;
+
+failsafe:
+	DRM_DEBUG_KMS("Displayport: Setting failsafe display mode\n");
+	use_mode = intel_dp_get_failsafe_mode();
+	/* FIXME: <TAP> Need to set DP to 6bpc here as well */
+	intel_dp->attached_connector->encoder->new_crtc->config.pipe_bpp = 18;
+
+set_mode:
+	/* Configure the display mode necessary */
+	modeset.connectors = &connector;
+	modeset.num_connectors = 1;
+	modeset.crtc = connector->encoder->crtc;
+	modeset.fb = modeset.crtc->primary->fb;
+	modeset.x = 0;
+	modeset.y = 0;
+	modeset.mode = use_mode;
+	/* Set the config */
+	intel_dp_set_config(&modeset);
+
 	return test_result;
 }
 
-- 
1.9.1

Re: [PATCH 07/12] drm/i915: Update EDID automated test function for Displayport compliance

[Paulo Zanoni]

2014-07-14 16:10 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> Implements an updated version of the automated testing function that handles
> Displayport compliance for EDID operations.

Both the commit message and the code should mention the name of the
specification that defines these tests, and also mention which
specific tests are implemented by this patch/function. I see that
there are multiple tests being implemented here, but reading the 232
pages of the spec will require too much time, so knowing which ones
are implemented really helps the reviewers :)

Also, you should tell us what happens before and after this patch when
you run your own tests. How many tests were we previously passing? How
many tests are we passing now? I see there are some FIXME lines below,
which leads to the question: is the code you provided enough and
complete, or do we still need more adjustments to pass everything we
can with what you built? In other words: is this patch, alone, already
an improvement to the situation?

>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 77 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 76 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 33b6dc9..88f1bbe 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3408,7 +3408,82 @@ intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
>  static uint8_t
>  intel_dp_autotest_edid(struct intel_dp *intel_dp)
>  {
> -       uint8_t test_result = DP_TEST_NAK;
> +       struct drm_connector *connector = &intel_dp->attached_connector->base;
> +       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
> +       struct edid *edid_read = NULL;
> +       uint8_t *edid_data = NULL;
> +       uint8_t test_result = DP_TEST_NAK, checksum = 0;
> +       uint32_t i = 0, ret = 0;
> +       struct drm_display_mode *use_mode = NULL;
> +       int mode_count = 0;
> +       struct drm_mode_set modeset;

You have initialized every single variable you defined. This creates
the problem that unused variables are not pointed by the compiler
unless you enable the flags to warn set-but-not-used variables. For
example, "i" is unused, and "ret" is set but never used. I also don't
really see the point in, for example, NULL-initializing stuff like
edid_data and edid_read.


> +
> +       DRM_DEBUG_KMS("Displayport: EDID automated test\n");
> +
> +       /* Reset the NACK/DEFER counters */

As I said before, this is a great example of the "comment says what
the code already says" problem.


> +       intel_dp->aux.i2c_nack_count = 0;
> +       intel_dp->aux.i2c_defer_count = 0;
> +       /* Now read out the EDID */
> +       edid_read = drm_get_edid(connector, adapter);
> +
> +       if (edid_read == NULL) {
> +               /* Check for NACKs/DEFERs, goto failsafe if detected
> +                  (DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5) */

Our coding standard is to put aligned '*'s on each line of a
multi-line comment. So this should be "\t\t * (DP CTS 1.2 etc..."
instead of what is above. In theory, the "/*" and "*/" strings should
be on their own lines, alone, but we are inconsistent regarding this
(even though Daniel randomly complained about this to me a few times
in the past). As usual, check Documentation/CodingStyle for better
explanations.


> +               if (intel_dp->aux.i2c_nack_count > 0 ||
> +                       intel_dp->aux.i2c_defer_count > 0)

We tend to align the contents of the extra line with the '(' char on
the line above.


> +                       DRM_DEBUG_KMS("Displayport: EDID read generated %d I2C NACKs, %d DEFERs\n",
> +                                                 intel_dp->aux.i2c_nack_count,
> +                                                 intel_dp->aux.i2c_defer_count);

There are way too many tabs on the 2 lines above.


> +               goto failsafe;
> +       }
> +
> +       /* FIXME: Need to determine how to detect E-DDC here (4.2.2.9) */

But what is the problem with the current code? What are the
consequences of not implementing the FIXME?


> +       edid_data = (uint8_t *) edid_read;
> +
> +       if (intel_dp_compute_edid_checksum(edid_data, &checksum)) {
> +               /* Write the checksum to EDID checksum register */
> +               ret = drm_dp_dpcd_write(&intel_dp->aux,
> +                                                               DP_TEST_EDID_CHECKSUM,
> +                                                               &edid_read->checksum, 1);

Way too many tabs above too.


> +               /* Reponse is ACK and and checksum written */
> +               test_result = DP_TEST_ACK | DP_TEST_EDID_CHECKSUM_WRITE;
> +       } else {
> +               /* Invalid checksum - EDID corruption detection test */
> +               goto failsafe;
> +       }
> +
> +       /* Update EDID modes */
> +       mode_count = intel_connector_update_modes(connector, edid_read);
> +       if (!mode_count) {
> +               DRM_DEBUG_KMS("Displayport: Mode update failed\n");
> +               goto failsafe;
> +       }
> +
> +       /* Get the EDID preferred mode if available */
> +       use_mode = intel_dp_get_edid_preferred_mode(intel_dp);
> +       if (use_mode == NULL)
> +               goto failsafe;
> +       else
> +               goto set_mode;
> +
> +failsafe:
> +       DRM_DEBUG_KMS("Displayport: Setting failsafe display mode\n");
> +       use_mode = intel_dp_get_failsafe_mode();
> +       /* FIXME: <TAP> Need to set DP to 6bpc here as well */

What does <TAP> mean? What are the consequences of not setting DP to
6bpc here as well?


> +       intel_dp->attached_connector->encoder->new_crtc->config.pipe_bpp = 18;
> +
> +set_mode:
> +       /* Configure the display mode necessary */
> +       modeset.connectors = &connector;
> +       modeset.num_connectors = 1;
> +       modeset.crtc = connector->encoder->crtc;

I guess this could result in NULL pointer dereferences. I don't see a
guarantee that the connector will have a CRTC at this point.


> +       modeset.fb = modeset.crtc->primary->fb;

And I also don't see a guarantee that we'll have an FB, and that the
FB will be big enough for the current mode we're setting.


> +       modeset.x = 0;
> +       modeset.y = 0;
> +       modeset.mode = use_mode;
> +       /* Set the config */
> +       intel_dp_set_config(&modeset);
> +

I guess Daniel said this won't really work, right? This is why it will
probably be simpler to involve user space on the compliance testing
(in addition to the fact that we will test real-world production-used
code). At this point we could just write to the debugfs file something
like "set preferred mode" or "set failsafe mode", then wait for the
modeset to happen. We could either try to detect the modeset (by
setting some variable to zero before we send the message to
user-space, and then verifying that intel_set_mode changed it to true)
or we could make the user-space write a string to the debugfs file,
like "done" and assume user-space is correct and finish the function
(it's debugfs anyway, and root, and DRM master, right?).

Also, if we use the failsafe mode we'll return DP_TEST_NAK. Is that
intentional? Also, it is a little strange that the ACK is the last
thing we should do here, and the spec doesn't seem to be clear
reagarding when we should send it (or I just didn't find the piece of
text that says it...). I guess that by running the compliance tests we
will discover that by looking at the test result.

>         return test_result;
>  }
>
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 07/12] drm/i915: Update EDID automated test function for Displayport compliance

[Todd Previte]

[-- Attachment #1.1: Type: text/plain, Size: 15177 bytes --]



> Paulo Zanoni <mailto:przanoni@gmail.com>
> Tuesday, July 29, 2014 3:37 PM
> 2014-07-14 16:10 GMT-03:00 Todd Previte<tprevite@gmail.com>:
>> Implements an updated version of the automated testing function that handles
>> Displayport compliance for EDID operations.
>
> Both the commit message and the code should mention the name of the
> specification that defines these tests, and also mention which
> specific tests are implemented by this patch/function. I see that
> there are multiple tests being implemented here, but reading the 232
> pages of the spec will require too much time, so knowing which ones
> are implemented really helps the reviewers :)
Heh fair enough. I can put all the test information in the commit 
message. I think it's already in the code, but I'll double-check that as 
well.
>
> Also, you should tell us what happens before and after this patch when
> you run your own tests. How many tests were we previously passing? How
> many tests are we passing now? I see there are some FIXME lines below,
> which leads to the question: is the code you provided enough and
> complete, or do we still need more adjustments to pass everything we
> can with what you built? In other words: is this patch, alone, already
> an improvement to the situation?

Makes sense. I'll add the additional info in the commit messages.
>
>> Signed-off-by: Todd Previte<tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 77 ++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 76 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 33b6dc9..88f1bbe 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3408,7 +3408,82 @@ intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
>>   static uint8_t
>>   intel_dp_autotest_edid(struct intel_dp *intel_dp)
>>   {
>> -       uint8_t test_result = DP_TEST_NAK;
>> +       struct drm_connector *connector =&intel_dp->attached_connector->base;
>> +       struct i2c_adapter *adapter =&intel_dp->aux.ddc;
>> +       struct edid *edid_read = NULL;
>> +       uint8_t *edid_data = NULL;
>> +       uint8_t test_result = DP_TEST_NAK, checksum = 0;
>> +       uint32_t i = 0, ret = 0;
>> +       struct drm_display_mode *use_mode = NULL;
>> +       int mode_count = 0;
>> +       struct drm_mode_set modeset;
>
> You have initialized every single variable you defined. This creates
> the problem that unused variables are not pointed by the compiler
> unless you enable the flags to warn set-but-not-used variables. For
> example, "i" is unused, and "ret" is set but never used. I also don't
> really see the point in, for example, NULL-initializing stuff like
> edid_data and edid_read.
I always initialize all the variables I declare as a matter of course. 
I've always been of the opinion that I'd rather use up a little extra 
stack space than to have to track down the weird problems that can 
result from using uninitialized variables. Compilers complain about 
using uninitialized variables as well, so this is more of a difference 
of opinion than anything else.

However, since there are unused variables, I'll remove those in the next 
iteration of this patch. :)
>> +
>> +       DRM_DEBUG_KMS("Displayport: EDID automated test\n");
>> +
>> +       /* Reset the NACK/DEFER counters */
>
> As I said before, this is a great example of the "comment says what
> the code already says" problem.
Fair enough. It will be removed.
>
>
>> +       intel_dp->aux.i2c_nack_count = 0;
>> +       intel_dp->aux.i2c_defer_count = 0;
>> +       /* Now read out the EDID */
>> +       edid_read = drm_get_edid(connector, adapter);
>> +
>> +       if (edid_read == NULL) {
>> +               /* Check for NACKs/DEFERs, goto failsafe if detected
>> +                  (DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5) */
>
> Our coding standard is to put aligned '*'s on each line of a
> multi-line comment. So this should be "\t\t * (DP CTS 1.2 etc..."
> instead of what is above. In theory, the "/*" and "*/" strings should
> be on their own lines, alone, but we are inconsistent regarding this
> (even though Daniel randomly complained about this to me a few times
> in the past). As usual, check Documentation/CodingStyle for better
> explanations.
I'll adjust the code to conform to the coding standard, whatever it 
happens to be.
>
>> +               if (intel_dp->aux.i2c_nack_count>  0 ||
>> +                       intel_dp->aux.i2c_defer_count>  0)
>
> We tend to align the contents of the extra line with the '(' char on
> the line above.
I think this is a problem with tab sizes (as evidenced below). My editor 
is set for 4-space tabs whereas the kernel standard is 8-space tabs. 
I've resisted changing it because it spaces the code out way too much 
for my liking while I'm editing. But apparently this is going to be an 
issue moving forward, so I'm going to have to change it.
>
>> +                       DRM_DEBUG_KMS("Displayport: EDID read generated %d I2C NACKs, %d DEFERs\n",
>> +                                                 intel_dp->aux.i2c_nack_count,
>> +                                                 intel_dp->aux.i2c_defer_count);
>
> There are way too many tabs on the 2 lines above.
See above. This is the tab size problem.
>
>
>> +               goto failsafe;
>> +       }
>> +
>> +       /* FIXME: Need to determine how to detect E-DDC here (4.2.2.9) */
>
> But what is the problem with the current code? What are the
> consequences of not implementing the FIXME?
4.2.2.9 is a separate test. Until we address it in the code, the test 
will fail. If additional explanation is necessary, that can be added.
>
>> +       edid_data = (uint8_t *) edid_read;
>> +
>> +       if (intel_dp_compute_edid_checksum(edid_data,&checksum)) {
>> +               /* Write the checksum to EDID checksum register */
>> +               ret = drm_dp_dpcd_write(&intel_dp->aux,
>> +                                                               DP_TEST_EDID_CHECKSUM,
>> +&edid_read->checksum, 1);
>
> Way too many tabs above too.
Same tab problem.
>
>
>> +               /* Reponse is ACK and and checksum written */
>> +               test_result = DP_TEST_ACK | DP_TEST_EDID_CHECKSUM_WRITE;
>> +       } else {
>> +               /* Invalid checksum - EDID corruption detection test */
>> +               goto failsafe;
>> +       }
>> +
>> +       /* Update EDID modes */
>> +       mode_count = intel_connector_update_modes(connector, edid_read);
>> +       if (!mode_count) {
>> +               DRM_DEBUG_KMS("Displayport: Mode update failed\n");
>> +               goto failsafe;
>> +       }
>> +
>> +       /* Get the EDID preferred mode if available */
>> +       use_mode = intel_dp_get_edid_preferred_mode(intel_dp);
>> +       if (use_mode == NULL)
>> +               goto failsafe;
>> +       else
>> +               goto set_mode;
>> +
>> +failsafe:
>> +       DRM_DEBUG_KMS("Displayport: Setting failsafe display mode\n");
>> +       use_mode = intel_dp_get_failsafe_mode();
>> +       /* FIXME:<TAP>  Need to set DP to 6bpc here as well */
>
> What does<TAP>  mean? What are the consequences of not setting DP to
> 6bpc here as well?
My initials? :) That's an oversight on my part. Those are just notes 
that I'm writing to myself while working in the code. It's a tag I can 
easily search to make sure I haven't missed anything. That shouldn't 
have been in there and instead should have been a simple "FIXME" with 
the added explanation. To answer your question though, without 18bpp the 
test fails stating that the video format is not correct.

>> +       intel_dp->attached_connector->encoder->new_crtc->config.pipe_bpp = 18;
>> +
>> +set_mode:
>> +       /* Configure the display mode necessary */
>> +       modeset.connectors =&connector;
>> +       modeset.num_connectors = 1;
>> +       modeset.crtc = connector->encoder->crtc;
>
> I guess this could result in NULL pointer dereferences. I don't see a
> guarantee that the connector will have a CRTC at this point.
>
>
>> +       modeset.fb = modeset.crtc->primary->fb;
>
> And I also don't see a guarantee that we'll have an FB, and that the
> FB will be big enough for the current mode we're setting.
>
>
>> +       modeset.x = 0;
>> +       modeset.y = 0;
>> +       modeset.mode = use_mode;
>> +       /* Set the config */
>> +       intel_dp_set_config(&modeset);
>> +
>
> I guess Daniel said this won't really work, right? This is why it will
> probably be simpler to involve user space on the compliance testing
> (in addition to the fact that we will test real-world production-used
> code). At this point we could just write to the debugfs file something
> like "set preferred mode" or "set failsafe mode", then wait for the
> modeset to happen. We could either try to detect the modeset (by
> setting some variable to zero before we send the message to
> user-space, and then verifying that intel_set_mode changed it to true)
> or we could make the user-space write a string to the debugfs file,
> like "done" and assume user-space is correct and finish the function
> (it's debugfs anyway, and root, and DRM master, right?).
Right. I think this whole block is what Daniel was referring to. The two 
reasons to move it to userspace are that a) it's a more reliable 
solution to do this in userspace and b) it allows for testing the real 
code paths that are actively in use rather than just testing dedicated 
paths for a compliance badge.
>
> Also, if we use the failsafe mode we'll return DP_TEST_NAK. Is that
> intentional? Also, it is a little strange that the ACK is the last
> thing we should do here, and the spec doesn't seem to be clear
> reagarding when we should send it (or I just didn't find the piece of
> text that says it...). I guess that by running the compliance tests we
> will discover that by looking at the test result.
No it shouldn't NAK the test since it's been implemented. That's an 
error in the code that needs to be corrected. Page 24 of the Link CTS 
spec has some info on the ACK/NAK. Here's what it says:

     "If the test mode requested is supported, the Source DUT shall 
start transmitting the
     TEST_VIDEO_PATTERN in the test mode requested, and set the TEST_ACK 
bit in the TEST_RESPONSE
     register."

     "If the test mode requested is not supported, the Source DUT shall 
signal a negative acknowledgement by
     setting the TEST_NAK bit in the TEST_RESPONSE register."

     "The Source DUT must acknowledge the interrupt by writing to the 
TEST_RESPONSE register within 5
     seconds of IRQ HPD pulse detect."

In some cases, the ACK can only be set after the operation is complete 
(i.e. active video being transmitted or correct test pattern being 
transmitted). So for uniformity, the ACK with the appropriate other test 
bits set is done at the end. Also, with the 5 second timeout, there's no 
need to respond with an ACK immediately in some cases and wait til the 
end in others.

>>          return test_result;
>>   }
>>
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> Todd Previte <mailto:tprevite@gmail.com>
> Monday, July 14, 2014 12:10 PM
> Implements an updated version of the automated testing function that 
> handles
> Displayport compliance for EDID operations.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
> drivers/gpu/drm/i915/intel_dp.c | 77 
> ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 76 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> b/drivers/gpu/drm/i915/intel_dp.c
> index 33b6dc9..88f1bbe 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3408,7 +3408,82 @@ intel_dp_autotest_video_pattern(struct intel_dp 
> *intel_dp)
> static uint8_t
> intel_dp_autotest_edid(struct intel_dp *intel_dp)
> {
> - uint8_t test_result = DP_TEST_NAK;
> + struct drm_connector *connector = &intel_dp->attached_connector->base;
> + struct i2c_adapter *adapter = &intel_dp->aux.ddc;
> + struct edid *edid_read = NULL;
> + uint8_t *edid_data = NULL;
> + uint8_t test_result = DP_TEST_NAK, checksum = 0;
> + uint32_t i = 0, ret = 0;
> + struct drm_display_mode *use_mode = NULL;
> + int mode_count = 0;
> + struct drm_mode_set modeset;
> +
> + DRM_DEBUG_KMS("Displayport: EDID automated test\n");
> +
> + /* Reset the NACK/DEFER counters */
> + intel_dp->aux.i2c_nack_count = 0;
> + intel_dp->aux.i2c_defer_count = 0;
> + /* Now read out the EDID */
> + edid_read = drm_get_edid(connector, adapter);
> +
> + if (edid_read == NULL) {
> + /* Check for NACKs/DEFERs, goto failsafe if detected
> + (DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5) */
> + if (intel_dp->aux.i2c_nack_count > 0 ||
> + intel_dp->aux.i2c_defer_count > 0)
> + DRM_DEBUG_KMS("Displayport: EDID read generated %d I2C NACKs, %d 
> DEFERs\n",
> + intel_dp->aux.i2c_nack_count,
> + intel_dp->aux.i2c_defer_count);
> + goto failsafe;
> + }
> +
> + /* FIXME: Need to determine how to detect E-DDC here (4.2.2.9) */
> + edid_data = (uint8_t *) edid_read;
> +
> + if (intel_dp_compute_edid_checksum(edid_data, &checksum)) {
> + /* Write the checksum to EDID checksum register */
> + ret = drm_dp_dpcd_write(&intel_dp->aux,
> + DP_TEST_EDID_CHECKSUM,
> + &edid_read->checksum, 1);
> + /* Reponse is ACK and and checksum written */
> + test_result = DP_TEST_ACK | DP_TEST_EDID_CHECKSUM_WRITE;
> + } else {
> + /* Invalid checksum - EDID corruption detection test */
> + goto failsafe;
> + }
> +
> + /* Update EDID modes */
> + mode_count = intel_connector_update_modes(connector, edid_read);
> + if (!mode_count) {
> + DRM_DEBUG_KMS("Displayport: Mode update failed\n");
> + goto failsafe;
> + }
> +
> + /* Get the EDID preferred mode if available */
> + use_mode = intel_dp_get_edid_preferred_mode(intel_dp);
> + if (use_mode == NULL)
> + goto failsafe;
> + else
> + goto set_mode;
> +
> +failsafe:
> + DRM_DEBUG_KMS("Displayport: Setting failsafe display mode\n");
> + use_mode = intel_dp_get_failsafe_mode();
> + /* FIXME: <TAP> Need to set DP to 6bpc here as well */
> + intel_dp->attached_connector->encoder->new_crtc->config.pipe_bpp = 18;
> +
> +set_mode:
> + /* Configure the display mode necessary */
> + modeset.connectors = &connector;
> + modeset.num_connectors = 1;
> + modeset.crtc = connector->encoder->crtc;
> + modeset.fb = modeset.crtc->primary->fb;
> + modeset.x = 0;
> + modeset.y = 0;
> + modeset.mode = use_mode;
> + /* Set the config */
> + intel_dp_set_config(&modeset);
> +
> return test_result;
> }
>
> Todd Previte <mailto:tprevite@gmail.com>
> Monday, July 14, 2014 12:10 PM
>
> V2:
> - Addressed review feedback from the mailing list
> - Broke up patches into smaller, easily managed chunks
> - Reordered the patches such that they can be applied in order
> - Fixed checkpatch.pl errors across the patchset
> - Updated and enhanced functionality for the EDID test function
> - Completely revamped the mode set operations for compliance testing
>

-- 
Sent with Postbox <http://www.getpostbox.com>

[-- Attachment #1.2.1: Type: text/html, Size: 24134 bytes --]

[-- Attachment #1.2.2: postbox-contact.jpg --]
[-- Type: image/jpeg, Size: 1254 bytes --]

[-- Attachment #1.2.3: compose-unknown-contact.jpg --]
[-- Type: image/jpeg, Size: 770 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/12] drm/i915: Improve reliability for Displayport link training

[Todd Previte]

Link training for Displayport can fail in many ways and at multiple different points
during the training process. Previously, errors were logged but no additional action
was taken based on them. Consequently, training attempts could continue even after
errors have occured that would prevent successful link training. This patch updates
the link training functions and where/how they're used to be more intelligent about
failures and to stop trying to train the link when it's a lost cause.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 23 +++++++++--
 drivers/gpu/drm/i915/intel_dp.c  | 89 +++++++++++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_drv.h |  7 ++--
 3 files changed, 90 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index ded6013..c0727b8 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1246,6 +1246,7 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
 	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
 	enum port port = intel_ddi_get_encoder_port(intel_encoder);
 	int type = intel_encoder->type;
+	uint8_t fail_code = 0;
 
 	if (crtc->config.has_audio) {
 		DRM_DEBUG_DRIVER("Audio on pipe %c on DDI\n",
@@ -1274,10 +1275,19 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
 		intel_dp->DP |= DDI_PORT_WIDTH(intel_dp->lane_count);
 
 		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
-		intel_dp_start_link_train(intel_dp);
-		intel_dp_complete_link_train(intel_dp);
+		if (!intel_dp_start_link_train(intel_dp)) {
+			fail_code = 1;
+			goto failed;
+		}
+		if (!intel_dp_complete_link_train(intel_dp)) {
+			fail_code = 2;
+			goto failed;
+		}
 		if (port != PORT_A)
-			intel_dp_stop_link_train(intel_dp);
+			if (!intel_dp_stop_link_train(intel_dp)) {
+				fail_code = 3;
+				goto failed;
+			}
 	} else if (type == INTEL_OUTPUT_HDMI) {
 		struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
 
@@ -1285,6 +1295,13 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
 					   crtc->config.has_hdmi_sink,
 					   &crtc->config.adjusted_mode);
 	}
+
+	return;
+
+failed:
+	/* Clear link training here */
+	intel_dp_set_idle_link_train(enc_to_intel_dp(encoder));
+	DRM_DEBUG_KMS("Failed to pre-enable DP, fail code %d\n", fail_code);
 }
 
 static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 88f1bbe..1c6ee34 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2018,23 +2018,42 @@ static void chv_post_disable_dp(struct intel_encoder *encoder)
 	mutex_unlock(&dev_priv->dpio_lock);
 }
 
-static void intel_enable_dp(struct intel_encoder *encoder)
+static bool intel_enable_dp(struct intel_encoder *encoder)
 {
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
 	struct drm_device *dev = encoder->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t dp_reg = I915_READ(intel_dp->output_reg);
+	uint8_t fail_code = 0;
 
+	/* FIXME: Not sure this needs to be a WARN() */
 	if (WARN_ON(dp_reg & DP_PORT_EN))
-		return;
+		return false;
 
 	intel_edp_panel_vdd_on(intel_dp);
 	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
-	intel_dp_start_link_train(intel_dp);
+	if (!intel_dp_start_link_train(intel_dp)) {
+		fail_code = 1;
+		goto failed;
+	}
 	intel_edp_panel_on(intel_dp);
 	edp_panel_vdd_off(intel_dp, true);
-	intel_dp_complete_link_train(intel_dp);
-	intel_dp_stop_link_train(intel_dp);
+	if (!intel_dp_complete_link_train(intel_dp)) {
+		fail_code = 2;
+		goto failed;
+	}
+	if (!intel_dp_stop_link_train(intel_dp)) {
+		fail_code = 3;
+		goto failed;
+	}
+
+	return true;
+
+failed:
+	/* Clear link training here */
+	intel_dp_set_idle_link_train(intel_dp);
+	DRM_DEBUG_KMS("Failed to enable DP with code %d\n", fail_code);
+	return false;
 }
 
 static void g4x_enable_dp(struct intel_encoder *encoder)
@@ -2956,7 +2975,7 @@ intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP,
 	return ret == intel_dp->lane_count;
 }
 
-static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
+void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = intel_dig_port->base.base.dev;
@@ -2988,7 +3007,7 @@ static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
 }
 
 /* Enable corresponding port and start training pattern 1 */
-void
+bool
 intel_dp_start_link_train(struct intel_dp *intel_dp)
 {
 	struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)->base.base;
@@ -3007,11 +3026,17 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
 	link_config[1] = intel_dp->lane_count;
 	if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
 		link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
-	drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2);
+	if (drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2) != 2) {
+		DRM_DEBUG_KMS("Failed to write sink DPCD for link rate and lane count\n");
+		goto failed;
+	}
 
 	link_config[0] = 0;
 	link_config[1] = DP_SET_ANSI_8B10B;
-	drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
+	if (drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2) != 2) {
+		DRM_DEBUG_KMS("Failed to write sink DPCD for downspread control\n");
+		goto failed;
+	}
 
 	DP |= DP_PORT_EN;
 
@@ -3020,7 +3045,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
 				       DP_TRAINING_PATTERN_1 |
 				       DP_LINK_SCRAMBLING_DISABLE)) {
 		DRM_ERROR("failed to enable link training\n");
-		return;
+		goto failed;
 	}
 
 	voltage = 0xff;
@@ -3032,12 +3057,12 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
 		drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
 		if (!intel_dp_get_link_status(intel_dp, link_status)) {
 			DRM_ERROR("failed to get link status\n");
-			break;
+			goto failed;
 		}
 
 		if (drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) {
 			DRM_DEBUG_KMS("clock recovery OK\n");
-			break;
+			goto cr_done;
 		}
 
 		/* Check to see if we've tried the max voltage */
@@ -3048,7 +3073,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
 			++loop_tries;
 			if (loop_tries == 5) {
 				DRM_ERROR("too many full retries, give up\n");
-				break;
+				goto failed;
 			}
 			intel_dp_reset_link_train(intel_dp, &DP,
 						  DP_TRAINING_PATTERN_1 |
@@ -3062,7 +3087,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
 			++voltage_tries;
 			if (voltage_tries == 5) {
 				DRM_ERROR("too many voltage retries, give up\n");
-				break;
+				goto failed;
 			}
 		} else
 			voltage_tries = 0;
@@ -3071,14 +3096,20 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
 		/* Update training set as requested by target */
 		if (!intel_dp_update_link_train(intel_dp, &DP, link_status)) {
 			DRM_ERROR("failed to update link training\n");
-			break;
+			goto failed;
 		}
 	}
 
+cr_done:
 	intel_dp->DP = DP;
+	return true;
+
+failed:
+	DRM_DEBUG_KMS("Failed to initiate link training\n");
+	return false;
 }
 
-void
+bool
 intel_dp_complete_link_train(struct intel_dp *intel_dp)
 {
 	bool channel_eq = false;
@@ -3095,7 +3126,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
 				     training_pattern |
 				     DP_LINK_SCRAMBLING_DISABLE)) {
 		DRM_ERROR("failed to start channel equalization\n");
-		return;
+		return false;
 	}
 
 	tries = 0;
@@ -3154,14 +3185,17 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
 
 	intel_dp->DP = DP;
 
-	if (channel_eq)
+	if (channel_eq) {
 		DRM_DEBUG_KMS("Channel EQ done. DP Training successful\n");
+		return true;
+	}
 
+	return false;
 }
 
-void intel_dp_stop_link_train(struct intel_dp *intel_dp)
+bool intel_dp_stop_link_train(struct intel_dp *intel_dp)
 {
-	intel_dp_set_link_train(intel_dp, &intel_dp->DP,
+	return intel_dp_set_link_train(intel_dp, &intel_dp->DP,
 				DP_TRAINING_PATTERN_DISABLE);
 }
 
@@ -3600,9 +3634,18 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 	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);
-		intel_dp_start_link_train(intel_dp);
-		intel_dp_complete_link_train(intel_dp);
-		intel_dp_stop_link_train(intel_dp);
+		if (!intel_dp_start_link_train(intel_dp)) {
+			DRM_DEBUG_KMS("Start link training failed\n");
+			return;
+		}
+		if (!intel_dp_complete_link_train(intel_dp)) {
+			DRM_DEBUG_KMS("Complete link training failed\n");
+			return;
+		}
+		if (!intel_dp_stop_link_train(intel_dp)) {
+			DRM_DEBUG_KMS("Stop link training failed\n");
+			return;
+		}
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d2ae54f..79876df 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -844,9 +844,10 @@ int intel_dp_set_config(struct drm_mode_set *set);
 void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
 bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 			     struct intel_connector *intel_connector);
-void intel_dp_start_link_train(struct intel_dp *intel_dp);
-void intel_dp_complete_link_train(struct intel_dp *intel_dp);
-void intel_dp_stop_link_train(struct intel_dp *intel_dp);
+bool intel_dp_start_link_train(struct intel_dp *intel_dp);
+bool intel_dp_complete_link_train(struct intel_dp *intel_dp);
+bool intel_dp_stop_link_train(struct intel_dp *intel_dp);
+void intel_dp_set_idle_link_train(struct intel_dp *intel_dp);
 void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
 void intel_dp_encoder_destroy(struct drm_encoder *encoder);
 void intel_dp_check_link_status(struct intel_dp *intel_dp);
-- 
1.9.1

Re: [PATCH 08/12] drm/i915: Improve reliability for Displayport link training

[Paulo Zanoni]

2014-07-14 16:10 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> Link training for Displayport can fail in many ways and at multiple different points
> during the training process. Previously, errors were logged but no additional action
> was taken based on them. Consequently, training attempts could continue even after
> errors have occured that would prevent successful link training. This patch updates
> the link training functions and where/how they're used to be more intelligent about
> failures and to stop trying to train the link when it's a lost cause.

I agree we need to do something about this problem, but I'm not sure
how this patch improves the situation. Can you please describe more
how exactly the changes you did are getting us towards the solution of
the problem? Of course, the points where you start signaling
previously-unsignaled errors are obviously an improvement.

Anyway, this patch should probably be split in 3:
- A patch to add the boolean return values and change
intel_dp_check_link_status() + intel_enable_dp() +
intel_ddi_pre_enable().
- A patch to signal dpcd error cases we were previously ignoring.
- A patch to that changes how intel_dp_start_link_train() and
intel_dp_stop_link_train() currently behave (the "goto"s replacing
"break" statements).
Se below for better explanations.


The big problem here is that these encoder callbacks never fail, so
there's not really much to do after we detect a sink failure.

In the current code (without your patch), we already clearly signal
the link training failures with debug+error messages, so the new debug
messages at places linke intel_enable_dp() are not much of an
improvement. Also, we already run intel_dp_set_idle_link_train() at
the end of intel_dp_complete_link_train(), and we do additional things
such as calling intel_dp_stop_link_train(). And I guess we do the
non-DDI equivalent steps at some point too, so I'm not sure how
jumping straight to intel_dp_set_idle_link_train() helps, since we do
it anyway as part of the normal sequence. Also, our mode set sequence
is currently completely followed - even though the sink fails to
understand what we throw at it - and I'm always afraid of not
following the sequence exactly as described in the spec, since it
could lead to unpredicted bugs (we had this problem dozens of times in
the past).

I think the real cool solution would be to retry link training with
different parameters (different clock and number of lanes), but I
imagine this would require a lot of code refactoring since we probably
need to go back to the compute_config stages of the modeset sequence.
Or maybe just finding a way to tell the user-space modesetting app
that it has a black screen would already be helpful.

Other people may think that the real-real long-term solution would be
to fix our code so it never fails link training or gives black screens
:)

Some more below:

>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 23 +++++++++--
>  drivers/gpu/drm/i915/intel_dp.c  | 89 +++++++++++++++++++++++++++++-----------
>  drivers/gpu/drm/i915/intel_drv.h |  7 ++--
>  3 files changed, 90 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index ded6013..c0727b8 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1246,6 +1246,7 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
>         struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
>         enum port port = intel_ddi_get_encoder_port(intel_encoder);
>         int type = intel_encoder->type;
> +       uint8_t fail_code = 0;
>
>         if (crtc->config.has_audio) {
>                 DRM_DEBUG_DRIVER("Audio on pipe %c on DDI\n",
> @@ -1274,10 +1275,19 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
>                 intel_dp->DP |= DDI_PORT_WIDTH(intel_dp->lane_count);
>
>                 intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> -               intel_dp_start_link_train(intel_dp);
> -               intel_dp_complete_link_train(intel_dp);
> +               if (!intel_dp_start_link_train(intel_dp)) {
> +                       fail_code = 1;
> +                       goto failed;
> +               }
> +               if (!intel_dp_complete_link_train(intel_dp)) {
> +                       fail_code = 2;
> +                       goto failed;
> +               }
>                 if (port != PORT_A)
> -                       intel_dp_stop_link_train(intel_dp);
> +                       if (!intel_dp_stop_link_train(intel_dp)) {
> +                               fail_code = 3;
> +                               goto failed;
> +                       }
>         } else if (type == INTEL_OUTPUT_HDMI) {
>                 struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
>
> @@ -1285,6 +1295,13 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
>                                            crtc->config.has_hdmi_sink,
>                                            &crtc->config.adjusted_mode);
>         }
> +
> +       return;
> +
> +failed:
> +       /* Clear link training here */
> +       intel_dp_set_idle_link_train(enc_to_intel_dp(encoder));
> +       DRM_DEBUG_KMS("Failed to pre-enable DP, fail code %d\n", fail_code);
>  }
>
>  static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 88f1bbe..1c6ee34 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2018,23 +2018,42 @@ static void chv_post_disable_dp(struct intel_encoder *encoder)
>         mutex_unlock(&dev_priv->dpio_lock);
>  }
>
> -static void intel_enable_dp(struct intel_encoder *encoder)
> +static bool intel_enable_dp(struct intel_encoder *encoder)
>  {
>         struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
>         struct drm_device *dev = encoder->base.dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         uint32_t dp_reg = I915_READ(intel_dp->output_reg);
> +       uint8_t fail_code = 0;
>
> +       /* FIXME: Not sure this needs to be a WARN() */

You can git-blame that line and see its history :)

Anyway, the point is that if you reach this point of the code, DP
_must_ be disabled, so "dp_reg & DP_POR_EN" tells us there's a bug
somewhere else.


>         if (WARN_ON(dp_reg & DP_PORT_EN))
> -               return;
> +               return false;
>
>         intel_edp_panel_vdd_on(intel_dp);
>         intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
> -       intel_dp_start_link_train(intel_dp);
> +       if (!intel_dp_start_link_train(intel_dp)) {
> +               fail_code = 1;
> +               goto failed;
> +       }
>         intel_edp_panel_on(intel_dp);
>         edp_panel_vdd_off(intel_dp, true);
> -       intel_dp_complete_link_train(intel_dp);
> -       intel_dp_stop_link_train(intel_dp);
> +       if (!intel_dp_complete_link_train(intel_dp)) {
> +               fail_code = 2;
> +               goto failed;
> +       }
> +       if (!intel_dp_stop_link_train(intel_dp)) {
> +               fail_code = 3;
> +               goto failed;
> +       }
> +
> +       return true;
> +
> +failed:
> +       /* Clear link training here */
> +       intel_dp_set_idle_link_train(intel_dp);

Function intel_enable_dp() is called by all non-DDI gens, but
intel_dp_set_idle_link_train() has an early return for !HAS_DDI. So
this function call is basically doing nothing here. IMHO this is the
biggest problem with this patch.


> +       DRM_DEBUG_KMS("Failed to enable DP with code %d\n", fail_code);
> +       return false;
>  }
>
>  static void g4x_enable_dp(struct intel_encoder *encoder)
> @@ -2956,7 +2975,7 @@ intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP,
>         return ret == intel_dp->lane_count;
>  }
>
> -static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
> +void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
>  {
>         struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>         struct drm_device *dev = intel_dig_port->base.base.dev;
> @@ -2988,7 +3007,7 @@ static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
>  }
>
>  /* Enable corresponding port and start training pattern 1 */
> -void
> +bool
>  intel_dp_start_link_train(struct intel_dp *intel_dp)
>  {
>         struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)->base.base;
> @@ -3007,11 +3026,17 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>         link_config[1] = intel_dp->lane_count;
>         if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
>                 link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
> -       drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2);
> +       if (drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2) != 2) {
> +               DRM_DEBUG_KMS("Failed to write sink DPCD for link rate and lane count\n");
> +               goto failed;
> +       }
>
>         link_config[0] = 0;
>         link_config[1] = DP_SET_ANSI_8B10B;
> -       drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
> +       if (drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2) != 2) {
> +               DRM_DEBUG_KMS("Failed to write sink DPCD for downspread control\n");
> +               goto failed;
> +       }

These two above are useful additions and could probably go into a
separate patch. But maybe I'd make them be DRM_ERRORs since we
probably want to easily notice them - and get the bug reports.

Maybe we could also create a new dp_dpcd_write_safe() function/macro
that would be responsible for printing error messages in case we don't
transfer all the bits we want, then we could make the whole intel_dp.c
file use it. The nice thing of being a macro would be that the
DRM_ERROR would print the name of the caller function.


>
>         DP |= DP_PORT_EN;
>
> @@ -3020,7 +3045,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>                                        DP_TRAINING_PATTERN_1 |
>                                        DP_LINK_SCRAMBLING_DISABLE)) {
>                 DRM_ERROR("failed to enable link training\n");
> -               return;
> +               goto failed;
>         }
>
>         voltage = 0xff;
> @@ -3032,12 +3057,12 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>                 drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
>                 if (!intel_dp_get_link_status(intel_dp, link_status)) {
>                         DRM_ERROR("failed to get link status\n");
> -                       break;
> +                       goto failed;
>                 }
>
>                 if (drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) {
>                         DRM_DEBUG_KMS("clock recovery OK\n");
> -                       break;
> +                       goto cr_done;
>                 }
>
>                 /* Check to see if we've tried the max voltage */
> @@ -3048,7 +3073,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>                         ++loop_tries;
>                         if (loop_tries == 5) {
>                                 DRM_ERROR("too many full retries, give up\n");
> -                               break;
> +                               goto failed;
>                         }
>                         intel_dp_reset_link_train(intel_dp, &DP,
>                                                   DP_TRAINING_PATTERN_1 |
> @@ -3062,7 +3087,7 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>                         ++voltage_tries;
>                         if (voltage_tries == 5) {
>                                 DRM_ERROR("too many voltage retries, give up\n");
> -                               break;
> +                               goto failed;
>                         }
>                 } else
>                         voltage_tries = 0;
> @@ -3071,14 +3096,20 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>                 /* Update training set as requested by target */
>                 if (!intel_dp_update_link_train(intel_dp, &DP, link_status)) {
>                         DRM_ERROR("failed to update link training\n");
> -                       break;
> +                       goto failed;
>                 }
>         }
>
> +cr_done:
>         intel_dp->DP = DP;
> +       return true;
> +
> +failed:
> +       DRM_DEBUG_KMS("Failed to initiate link training\n");
> +       return false;

This set of changes where you replace "break" with "goto"s should be
on a separate patch, with a nice explanation of what are the
consequences of not doing "intel_dp->DP = DP" on the cases were we
just "goto failed". If the link training failed, we should probably
disable DP_PORT_EN.


>  }
>
> -void
> +bool
>  intel_dp_complete_link_train(struct intel_dp *intel_dp)
>  {
>         bool channel_eq = false;
> @@ -3095,7 +3126,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
>                                      training_pattern |
>                                      DP_LINK_SCRAMBLING_DISABLE)) {
>                 DRM_ERROR("failed to start channel equalization\n");
> -               return;
> +               return false;
>         }
>
>         tries = 0;
> @@ -3154,14 +3185,17 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
>
>         intel_dp->DP = DP;
>
> -       if (channel_eq)
> +       if (channel_eq) {
>                 DRM_DEBUG_KMS("Channel EQ done. DP Training successful\n");
> +               return true;
> +       }
>
> +       return false;
>  }

Same here.


>
> -void intel_dp_stop_link_train(struct intel_dp *intel_dp)
> +bool intel_dp_stop_link_train(struct intel_dp *intel_dp)
>  {
> -       intel_dp_set_link_train(intel_dp, &intel_dp->DP,
> +       return intel_dp_set_link_train(intel_dp, &intel_dp->DP,
>                                 DP_TRAINING_PATTERN_DISABLE);
>  }
>
> @@ -3600,9 +3634,18 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>         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);
> -               intel_dp_start_link_train(intel_dp);
> -               intel_dp_complete_link_train(intel_dp);
> -               intel_dp_stop_link_train(intel_dp);
> +               if (!intel_dp_start_link_train(intel_dp)) {
> +                       DRM_DEBUG_KMS("Start link training failed\n");
> +                       return;
> +               }
> +               if (!intel_dp_complete_link_train(intel_dp)) {
> +                       DRM_DEBUG_KMS("Complete link training failed\n");
> +                       return;
> +               }
> +               if (!intel_dp_stop_link_train(intel_dp)) {
> +                       DRM_DEBUG_KMS("Stop link training failed\n");
> +                       return;
> +               }
>         }
>  }
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d2ae54f..79876df 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -844,9 +844,10 @@ int intel_dp_set_config(struct drm_mode_set *set);
>  void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
>  bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>                              struct intel_connector *intel_connector);
> -void intel_dp_start_link_train(struct intel_dp *intel_dp);
> -void intel_dp_complete_link_train(struct intel_dp *intel_dp);
> -void intel_dp_stop_link_train(struct intel_dp *intel_dp);
> +bool intel_dp_start_link_train(struct intel_dp *intel_dp);
> +bool intel_dp_complete_link_train(struct intel_dp *intel_dp);
> +bool intel_dp_stop_link_train(struct intel_dp *intel_dp);
> +void intel_dp_set_idle_link_train(struct intel_dp *intel_dp);
>  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
>  void intel_dp_encoder_destroy(struct drm_encoder *encoder);
>  void intel_dp_check_link_status(struct intel_dp *intel_dp);
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 08/12] drm/i915: Improve reliability for Displayport link training

[Daniel Vetter]

On Wed, Jul 30, 2014 at 11:07:34AM -0300, Paulo Zanoni wrote:
> 2014-07-14 16:10 GMT-03:00 Todd Previte <tprevite@gmail.com>:
> > Link training for Displayport can fail in many ways and at multiple different points
> > during the training process. Previously, errors were logged but no additional action
> > was taken based on them. Consequently, training attempts could continue even after
> > errors have occured that would prevent successful link training. This patch updates
> > the link training functions and where/how they're used to be more intelligent about
> > failures and to stop trying to train the link when it's a lost cause.
> 
> I agree we need to do something about this problem, but I'm not sure
> how this patch improves the situation. Can you please describe more
> how exactly the changes you did are getting us towards the solution of
> the problem? Of course, the points where you start signaling
> previously-unsignaled errors are obviously an improvement.
> 
> Anyway, this patch should probably be split in 3:
> - A patch to add the boolean return values and change
> intel_dp_check_link_status() + intel_enable_dp() +
> intel_ddi_pre_enable().
> - A patch to signal dpcd error cases we were previously ignoring.
> - A patch to that changes how intel_dp_start_link_train() and
> intel_dp_stop_link_train() currently behave (the "goto"s replacing
> "break" statements).
> Se below for better explanations.
> 
> 
> The big problem here is that these encoder callbacks never fail, so
> there's not really much to do after we detect a sink failure.
> 
> In the current code (without your patch), we already clearly signal
> the link training failures with debug+error messages, so the new debug
> messages at places linke intel_enable_dp() are not much of an
> improvement. Also, we already run intel_dp_set_idle_link_train() at
> the end of intel_dp_complete_link_train(), and we do additional things
> such as calling intel_dp_stop_link_train(). And I guess we do the
> non-DDI equivalent steps at some point too, so I'm not sure how
> jumping straight to intel_dp_set_idle_link_train() helps, since we do
> it anyway as part of the normal sequence. Also, our mode set sequence
> is currently completely followed - even though the sink fails to
> understand what we throw at it - and I'm always afraid of not
> following the sequence exactly as described in the spec, since it
> could lead to unpredicted bugs (we had this problem dozens of times in
> the past).
> 
> I think the real cool solution would be to retry link training with
> different parameters (different clock and number of lanes), but I
> imagine this would require a lot of code refactoring since we probably
> need to go back to the compute_config stages of the modeset sequence.
> Or maybe just finding a way to tell the user-space modesetting app
> that it has a black screen would already be helpful.
> 
> Other people may think that the real-real long-term solution would be
> to fix our code so it never fails link training or gives black screens
> :)

DP spec says that we need to be able to retrain. My rough idea was that we
set some flags in pipe config (or wire up return values) which signals
that we need to rerun the modeset (including compute_config and all that).

To keep the link bw/count limits around we could add some flags to the
pipe config or something (and not completely clear it ofc).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 09/12] drm/i915: Update intel_dp_check_link_status() for Displayport compliance testing

[Todd Previte]

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, 18 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1c6ee34..65830e9 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3600,22 +3600,13 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 	u8 sink_irq_vector;
 	u8 link_status[DP_LINK_STATUS_SIZE];
 
-	/* FIXME: This access isn't protected by any locks. */
-	if (!intel_encoder->connectors_active)
-		return;
-
-	if (WARN_ON(!intel_encoder->base.crtc))
+	/* Bail if not connected */
+	if (intel_dp->attached_connector->base.status != connector_status_connected)
 		return;
 
-	/* 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)) {
+	/* Attempt to read the DPCD */
+	if (!intel_dp_get_dpcd(intel_dp))
 		return;
-	}
 
 	/* Try to read the source of the interrupt */
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
@@ -3625,12 +3616,25 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 				   DP_DEVICE_SERVICE_IRQ_VECTOR,
 				   sink_irq_vector);
 
-		if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
+		if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST) {
+			DRM_DEBUG_KMS("Displayport: Received automated test request\n");
 			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");
 	}
 
+	/* FIXME: This access isn't protected by any locks. */
+	if (!intel_encoder->connectors_active)
+		return;
+
+	if (WARN_ON(!intel_encoder->base.crtc))
+		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

Re: [PATCH 09/12] drm/i915: Update intel_dp_check_link_status() for Displayport compliance testing

[Paulo Zanoni]

2014-07-14 16:10 GMT-03: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.
>
> 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.
>

The problem here we that we just got an interrupt, so I am not sure if
we should believe what's in intel_dp->attached_connector->base.status:
doesn't it reflect the old state that we had before we got the
interrupt? I'm not sure if this information could be wrong. Can you
please elaborate on that?

The rest of the patch seems correct.

> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 32 ++++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 1c6ee34..65830e9 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3600,22 +3600,13 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>         u8 sink_irq_vector;
>         u8 link_status[DP_LINK_STATUS_SIZE];
>
> -       /* FIXME: This access isn't protected by any locks. */
> -       if (!intel_encoder->connectors_active)
> -               return;
> -
> -       if (WARN_ON(!intel_encoder->base.crtc))
> +       /* Bail if not connected */
> +       if (intel_dp->attached_connector->base.status != connector_status_connected)
>                 return;
>
> -       /* 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)) {
> +       /* Attempt to read the DPCD */
> +       if (!intel_dp_get_dpcd(intel_dp))
>                 return;
> -       }
>
>         /* Try to read the source of the interrupt */
>         if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> @@ -3625,12 +3616,25 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>                                    DP_DEVICE_SERVICE_IRQ_VECTOR,
>                                    sink_irq_vector);
>
> -               if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST)
> +               if (sink_irq_vector & DP_AUTOMATED_TEST_REQUEST) {
> +                       DRM_DEBUG_KMS("Displayport: Received automated test request\n");
>                         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");
>         }
>
> +       /* FIXME: This access isn't protected by any locks. */
> +       if (!intel_encoder->connectors_active)
> +               return;
> +
> +       if (WARN_ON(!intel_encoder->base.crtc))
> +               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



-- 
Paulo Zanoni

[PATCH 10/12] drm/i915: Update link training automated test function for Displayport compliance

[Todd Previte]

Implements basic link training functionality for Displayport automated compliance
testing.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 47 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 65830e9..e4b31ad 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3427,6 +3427,53 @@ static uint8_t
 intel_dp_autotest_link_training(struct intel_dp *intel_dp)
 {
 	uint8_t test_result = DP_TEST_NAK;
+	uint8_t rxdata[2];
+	uint8_t link_status[DP_LINK_STATUS_SIZE];
+	int bytes_ret = 0;
+	struct drm_connector *connector = &intel_dp->attached_connector->base;
+	struct intel_digital_port *intel_dig_port =
+		enc_to_dig_port(&intel_dp->attached_connector->encoder->base);
+
+	/* Read test parameters */
+	bytes_ret = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_LINK_RATE, rxdata, 2);
+
+	/* Set link rate directly */
+	intel_dp->link_bw = rxdata[0];
+	/* Preserve 7:5 when setting lane count */
+	intel_dp->lane_count &= 0xE0;
+	intel_dp->lane_count |= rxdata[1];
+
+	DRM_DEBUG_KMS("Displayport: Link training testing - %d lanes @ %02x link rate\n",
+			intel_dp->lane_count, intel_dp->link_bw);
+
+	/* Train the link */
+	intel_dp->DP = intel_dig_port->saved_port_bits |
+					DDI_BUF_CTL_ENABLE |
+					DDI_BUF_EMP_400MV_0DB_HSW;
+	intel_dp->DP |= DDI_PORT_WIDTH(intel_dp->lane_count);
+
+	if (!intel_dp_start_link_train(intel_dp)) {
+		DRM_DEBUG_KMS("Displayport: intel_dp_start_link_train() failed\n");
+		test_result = false;
+	}
+	if (!intel_dp_complete_link_train(intel_dp)) {
+		DRM_DEBUG_KMS("Displayport: intel_dp_complete_link_train() failed\n");
+		test_result = false;
+	}
+	if (!intel_dp_stop_link_train(intel_dp)) {
+		DRM_DEBUG_KMS("Displayport: intel_dp_stop_link_train() failed\n");
+		test_result = false;
+	}
+
+	/* Check link status for successful completion */
+	if (drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))
+		test_result = true;
+
+	if (test_result == false) {
+		/* Clear link training here */
+		intel_dp_set_idle_link_train(intel_dp);
+	}
+
 	return test_result;
 }
 
-- 
1.9.1

Re: [PATCH 10/12] drm/i915: Update link training automated test function for Displayport compliance

[Daniel Vetter]

On Mon, Jul 14, 2014 at 12:10:45PM -0700, Todd Previte wrote:
> Implements basic link training functionality for Displayport automated compliance
> testing.
> 
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 47 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 65830e9..e4b31ad 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3427,6 +3427,53 @@ static uint8_t
>  intel_dp_autotest_link_training(struct intel_dp *intel_dp)
>  {
>  	uint8_t test_result = DP_TEST_NAK;
> +	uint8_t rxdata[2];
> +	uint8_t link_status[DP_LINK_STATUS_SIZE];
> +	int bytes_ret = 0;
> +	struct drm_connector *connector = &intel_dp->attached_connector->base;
> +	struct intel_digital_port *intel_dig_port =
> +		enc_to_dig_port(&intel_dp->attached_connector->encoder->base);
> +
> +	/* Read test parameters */
> +	bytes_ret = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_LINK_RATE, rxdata, 2);
> +
> +	/* Set link rate directly */
> +	intel_dp->link_bw = rxdata[0];
> +	/* Preserve 7:5 when setting lane count */
> +	intel_dp->lane_count &= 0xE0;
> +	intel_dp->lane_count |= rxdata[1];

This won't work - if you change the link config you need to do a full
recomputation of the modeset config and a full modeset. No, we dont (yet)
have the infrastructure for this, which is why dp is such a pain since we
can change the link config once we've decided on something :(
-Daniel

> +
> +	DRM_DEBUG_KMS("Displayport: Link training testing - %d lanes @ %02x link rate\n",
> +			intel_dp->lane_count, intel_dp->link_bw);
> +
> +	/* Train the link */
> +	intel_dp->DP = intel_dig_port->saved_port_bits |
> +					DDI_BUF_CTL_ENABLE |
> +					DDI_BUF_EMP_400MV_0DB_HSW;
> +	intel_dp->DP |= DDI_PORT_WIDTH(intel_dp->lane_count);
> +
> +	if (!intel_dp_start_link_train(intel_dp)) {
> +		DRM_DEBUG_KMS("Displayport: intel_dp_start_link_train() failed\n");
> +		test_result = false;
> +	}
> +	if (!intel_dp_complete_link_train(intel_dp)) {
> +		DRM_DEBUG_KMS("Displayport: intel_dp_complete_link_train() failed\n");
> +		test_result = false;
> +	}
> +	if (!intel_dp_stop_link_train(intel_dp)) {
> +		DRM_DEBUG_KMS("Displayport: intel_dp_stop_link_train() failed\n");
> +		test_result = false;
> +	}
> +
> +	/* Check link status for successful completion */
> +	if (drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))
> +		test_result = true;
> +
> +	if (test_result == false) {
> +		/* Clear link training here */
> +		intel_dp_set_idle_link_train(intel_dp);
> +	}
> +
>  	return test_result;
>  }
>  
> -- 
> 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: [PATCH 10/12] drm/i915: Update link training automated test function for Displayport compliance

[Jesse Barnes]

On Tue, 22 Jul 2014 08:15:30 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Mon, Jul 14, 2014 at 12:10:45PM -0700, Todd Previte wrote:
> > Implements basic link training functionality for Displayport automated compliance
> > testing.
> > 
> > Signed-off-by: Todd Previte <tprevite@gmail.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 47 +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 47 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 65830e9..e4b31ad 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3427,6 +3427,53 @@ static uint8_t
> >  intel_dp_autotest_link_training(struct intel_dp *intel_dp)
> >  {
> >  	uint8_t test_result = DP_TEST_NAK;
> > +	uint8_t rxdata[2];
> > +	uint8_t link_status[DP_LINK_STATUS_SIZE];
> > +	int bytes_ret = 0;
> > +	struct drm_connector *connector = &intel_dp->attached_connector->base;
> > +	struct intel_digital_port *intel_dig_port =
> > +		enc_to_dig_port(&intel_dp->attached_connector->encoder->base);
> > +
> > +	/* Read test parameters */
> > +	bytes_ret = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_LINK_RATE, rxdata, 2);
> > +
> > +	/* Set link rate directly */
> > +	intel_dp->link_bw = rxdata[0];
> > +	/* Preserve 7:5 when setting lane count */
> > +	intel_dp->lane_count &= 0xE0;
> > +	intel_dp->lane_count |= rxdata[1];
> 
> This won't work - if you change the link config you need to do a full
> recomputation of the modeset config and a full modeset. No, we dont (yet)
> have the infrastructure for this, which is why dp is such a pain since we
> can change the link config once we've decided on something :(

I think that depends on how we're going to structure the code.  For
simply calling the link train functions, it looks like messing with the
intel_dp->foo fields will roughly do what Todd wants, which is to
simply try a re-train with a different set of params, ignoring the
actual fb and pipe configuration.

Or is that what you had in mind?  You'd like to see valid data and mode
timings to drive a given lane count and bw instead?  I'm not sure the
testing spec cares about that; Todd?

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 10/12] drm/i915: Update link training automated test function for Displayport compliance

[Daniel Vetter]

On Tue, Jul 22, 2014 at 10:40 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> > +   /* Set link rate directly */
>> > +   intel_dp->link_bw = rxdata[0];
>> > +   /* Preserve 7:5 when setting lane count */
>> > +   intel_dp->lane_count &= 0xE0;
>> > +   intel_dp->lane_count |= rxdata[1];
>>
>> This won't work - if you change the link config you need to do a full
>> recomputation of the modeset config and a full modeset. No, we dont (yet)
>> have the infrastructure for this, which is why dp is such a pain since we
>> can change the link config once we've decided on something :(
>
> I think that depends on how we're going to structure the code.  For
> simply calling the link train functions, it looks like messing with the
> intel_dp->foo fields will roughly do what Todd wants, which is to
> simply try a re-train with a different set of params, ignoring the
> actual fb and pipe configuration.
>
> Or is that what you had in mind?  You'd like to see valid data and mode
> timings to drive a given lane count and bw instead?  I'm not sure the
> testing spec cares about that; Todd?

If you change the link_bw then you need to do a full modeset sequence
since that's the only way to convince our hw to switch to the new
clock. The current code in this patch falls short and only retrains
the link.

But even if we'd do the full modeset the compute_config stage would
overwrite the link parameters again. So the correct way to do this is
to pimp the compute config code to have limits (we might as well
expose them as connector properties while at it for debugging) and
then do a full modeset sequence here. Then we could finally implement
(with a bit more code) the transparent fallback to a different config
if the first one doesn't work.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 10/12] drm/i915: Update link training automated test function for Displayport compliance

[Jesse Barnes]

On Tue, 22 Jul 2014 22:44:54 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Jul 22, 2014 at 10:40 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> >> > +   /* Set link rate directly */
> >> > +   intel_dp->link_bw = rxdata[0];
> >> > +   /* Preserve 7:5 when setting lane count */
> >> > +   intel_dp->lane_count &= 0xE0;
> >> > +   intel_dp->lane_count |= rxdata[1];
> >>
> >> This won't work - if you change the link config you need to do a full
> >> recomputation of the modeset config and a full modeset. No, we dont (yet)
> >> have the infrastructure for this, which is why dp is such a pain since we
> >> can change the link config once we've decided on something :(
> >
> > I think that depends on how we're going to structure the code.  For
> > simply calling the link train functions, it looks like messing with the
> > intel_dp->foo fields will roughly do what Todd wants, which is to
> > simply try a re-train with a different set of params, ignoring the
> > actual fb and pipe configuration.
> >
> > Or is that what you had in mind?  You'd like to see valid data and mode
> > timings to drive a given lane count and bw instead?  I'm not sure the
> > testing spec cares about that; Todd?
> 
> If you change the link_bw then you need to do a full modeset sequence
> since that's the only way to convince our hw to switch to the new
> clock. The current code in this patch falls short and only retrains
> the link.
> 
> But even if we'd do the full modeset the compute_config stage would
> overwrite the link parameters again. So the correct way to do this is
> to pimp the compute config code to have limits (we might as well
> expose them as connector properties while at it for debugging) and
> then do a full modeset sequence here. Then we could finally implement
> (with a bit more code) the transparent fallback to a different config
> if the first one doesn't work.

Right, yeah going through the full sequence sounds like it's required
for most of the tests based on what Todd tells me, since the test sink
will look at the active mode transmitted etc.

So the simple re-train and send misc isn't sufficient, even if the
training succeeds.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 11/12] drm/i915: Minor code clean up in intel_dp.c

[Todd Previte]

Cleans up a couple of unused variables and an extraneous debug log
message that was unintentionally left behind.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e4b31ad..0e207aaf 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3430,7 +3430,6 @@ intel_dp_autotest_link_training(struct intel_dp *intel_dp)
 	uint8_t rxdata[2];
 	uint8_t link_status[DP_LINK_STATUS_SIZE];
 	int bytes_ret = 0;
-	struct drm_connector *connector = &intel_dp->attached_connector->base;
 	struct intel_digital_port *intel_dig_port =
 		enc_to_dig_port(&intel_dp->attached_connector->encoder->base);
 
@@ -3494,7 +3493,7 @@ intel_dp_autotest_edid(struct intel_dp *intel_dp)
 	struct edid *edid_read = NULL;
 	uint8_t *edid_data = NULL;
 	uint8_t test_result = DP_TEST_NAK, checksum = 0;
-	uint32_t i = 0, ret = 0;
+	uint32_t ret = 0;
 	struct drm_display_mode *use_mode = NULL;
 	int mode_count = 0;
 	struct drm_mode_set modeset;
@@ -3592,7 +3591,6 @@ intel_dp_handle_test_request(struct intel_dp *intel_dp)
 	uint8_t rxdata = 0;
 	int ret = 0;
 
-	DRM_DEBUG_KMS("Displayport: Received automated test request\n");
 	/* Read DP_TEST_REQUEST register to identify the requested test */
 	ret = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1);
 
-- 
1.9.1

Re: [PATCH 11/12] drm/i915: Minor code clean up in intel_dp.c

[Daniel Vetter]

On Mon, Jul 14, 2014 at 12:10:46PM -0700, Todd Previte wrote:
> Cleans up a couple of unused variables and an extraneous debug log
> message that was unintentionally left behind.
> 
> Signed-off-by: Todd Previte <tprevite@gmail.com>

This should be squashed into the relevant earlier patches.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e4b31ad..0e207aaf 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3430,7 +3430,6 @@ intel_dp_autotest_link_training(struct intel_dp *intel_dp)
>  	uint8_t rxdata[2];
>  	uint8_t link_status[DP_LINK_STATUS_SIZE];
>  	int bytes_ret = 0;
> -	struct drm_connector *connector = &intel_dp->attached_connector->base;
>  	struct intel_digital_port *intel_dig_port =
>  		enc_to_dig_port(&intel_dp->attached_connector->encoder->base);
>  
> @@ -3494,7 +3493,7 @@ intel_dp_autotest_edid(struct intel_dp *intel_dp)
>  	struct edid *edid_read = NULL;
>  	uint8_t *edid_data = NULL;
>  	uint8_t test_result = DP_TEST_NAK, checksum = 0;
> -	uint32_t i = 0, ret = 0;
> +	uint32_t ret = 0;
>  	struct drm_display_mode *use_mode = NULL;
>  	int mode_count = 0;
>  	struct drm_mode_set modeset;
> @@ -3592,7 +3591,6 @@ intel_dp_handle_test_request(struct intel_dp *intel_dp)
>  	uint8_t rxdata = 0;
>  	int ret = 0;
>  
> -	DRM_DEBUG_KMS("Displayport: Received automated test request\n");
>  	/* Read DP_TEST_REQUEST register to identify the requested test */
>  	ret = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1);
>  
> -- 
> 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: [PATCH 11/12] drm/i915: Minor code clean up in intel_dp.c

[Todd Previte]

[-- Attachment #1.1: Type: text/plain, Size: 4128 bytes --]


> Daniel Vetter <mailto:daniel@ffwll.ch>
> Tuesday, July 15, 2014 12:47 AM
> On Mon, Jul 14, 2014 at 12:10:46PM -0700, Todd Previte wrote:
>> Cleans up a couple of unused variables and an extraneous debug log
>> message that was unintentionally left behind.
>>
>> Signed-off-by: Todd Previte<tprevite@gmail.com>
>
> This should be squashed into the relevant earlier patches.
> -Daniel
Sounds good. I'll squash them in and resend the patches as v3. Thanks 
Daniel.

-T
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index e4b31ad..0e207aaf 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3430,7 +3430,6 @@ intel_dp_autotest_link_training(struct intel_dp *intel_dp)
>>   	uint8_t rxdata[2];
>>   	uint8_t link_status[DP_LINK_STATUS_SIZE];
>>   	int bytes_ret = 0;
>> -	struct drm_connector *connector =&intel_dp->attached_connector->base;
>>   	struct intel_digital_port *intel_dig_port =
>>   		enc_to_dig_port(&intel_dp->attached_connector->encoder->base);
>>
>> @@ -3494,7 +3493,7 @@ intel_dp_autotest_edid(struct intel_dp *intel_dp)
>>   	struct edid *edid_read = NULL;
>>   	uint8_t *edid_data = NULL;
>>   	uint8_t test_result = DP_TEST_NAK, checksum = 0;
>> -	uint32_t i = 0, ret = 0;
>> +	uint32_t ret = 0;
>>   	struct drm_display_mode *use_mode = NULL;
>>   	int mode_count = 0;
>>   	struct drm_mode_set modeset;
>> @@ -3592,7 +3591,6 @@ intel_dp_handle_test_request(struct intel_dp *intel_dp)
>>   	uint8_t rxdata = 0;
>>   	int ret = 0;
>>
>> -	DRM_DEBUG_KMS("Displayport: Received automated test request\n");
>>   	/* Read DP_TEST_REQUEST register to identify the requested test */
>>   	ret = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST,&rxdata, 1);
>>
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> Todd Previte <mailto:tprevite@gmail.com>
> Monday, July 14, 2014 12:10 PM
> Cleans up a couple of unused variables and an extraneous debug log
> message that was unintentionally left behind.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
> drivers/gpu/drm/i915/intel_dp.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> b/drivers/gpu/drm/i915/intel_dp.c
> index e4b31ad..0e207aaf 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3430,7 +3430,6 @@ intel_dp_autotest_link_training(struct intel_dp 
> *intel_dp)
> uint8_t rxdata[2];
> uint8_t link_status[DP_LINK_STATUS_SIZE];
> int bytes_ret = 0;
> - struct drm_connector *connector = &intel_dp->attached_connector->base;
> struct intel_digital_port *intel_dig_port =
> enc_to_dig_port(&intel_dp->attached_connector->encoder->base);
>
> @@ -3494,7 +3493,7 @@ intel_dp_autotest_edid(struct intel_dp *intel_dp)
> struct edid *edid_read = NULL;
> uint8_t *edid_data = NULL;
> uint8_t test_result = DP_TEST_NAK, checksum = 0;
> - uint32_t i = 0, ret = 0;
> + uint32_t ret = 0;
> struct drm_display_mode *use_mode = NULL;
> int mode_count = 0;
> struct drm_mode_set modeset;
> @@ -3592,7 +3591,6 @@ intel_dp_handle_test_request(struct intel_dp 
> *intel_dp)
> uint8_t rxdata = 0;
> int ret = 0;
>
> - DRM_DEBUG_KMS("Displayport: Received automated test request\n");
> /* Read DP_TEST_REQUEST register to identify the requested test */
> ret = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_REQUEST, &rxdata, 1);
>
> Todd Previte <mailto:tprevite@gmail.com>
> Monday, July 14, 2014 12:10 PM
>
> V2:
> - Addressed review feedback from the mailing list
> - Broke up patches into smaller, easily managed chunks
> - Reordered the patches such that they can be applied in order
> - Fixed checkpatch.pl errors across the patchset
> - Updated and enhanced functionality for the EDID test function
> - Completely revamped the mode set operations for compliance testing
>

-- 
Sent with Postbox <http://www.getpostbox.com>

[-- Attachment #1.2.1: Type: text/html, Size: 8132 bytes --]

[-- Attachment #1.2.2: postbox-contact.jpg --]
[-- Type: image/jpeg, Size: 1192 bytes --]

[-- Attachment #1.2.3: compose-unknown-contact.jpg --]
[-- Type: image/jpeg, Size: 770 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 12/12] drm/i915: Add a delay in Displayport AUX transactions for compliance testing

[Todd Previte]

The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1 specifies that
repeated AUX transactions after a failure (NACK, DEFER or no response) must have
a minimum delay of 400us before the resend can occur. Tests 4.2.1.1 and 4.2.1.2 are two
tests that require this specifically.

Signed-off-by: Todd Previte <tprevite@gmail.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 0e207aaf..f0664cd 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -573,8 +573,12 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 				   DP_AUX_CH_CTL_RECEIVE_ERROR);
 
 			if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR |
-				      DP_AUX_CH_CTL_RECEIVE_ERROR))
+				      DP_AUX_CH_CTL_RECEIVE_ERROR)) {
+				/* DP compliance requires 400us delay for errors/timeouts
+				   (DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2) */
+				udelay(400);
 				continue;
+			}
 			if (status & DP_AUX_CH_CTL_DONE)
 				break;
 		}
-- 
1.9.1

Re: [PATCH 12/12] drm/i915: Add a delay in Displayport AUX transactions for compliance testing

[Daniel Vetter]

On Mon, Jul 14, 2014 at 12:10:47PM -0700, Todd Previte wrote:
> The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1 specifies that
> repeated AUX transactions after a failure (NACK, DEFER or no response) must have
> a minimum delay of 400us before the resend can occur. Tests 4.2.1.1 and 4.2.1.2 are two
> tests that require this specifically.
> 
> Signed-off-by: Todd Previte <tprevite@gmail.com>

Since this is a minimal timeout ... shouldn't we put it into the dp
helpers instead?
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 0e207aaf..f0664cd 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -573,8 +573,12 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  				   DP_AUX_CH_CTL_RECEIVE_ERROR);
>  
>  			if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR |
> -				      DP_AUX_CH_CTL_RECEIVE_ERROR))
> +				      DP_AUX_CH_CTL_RECEIVE_ERROR)) {
> +				/* DP compliance requires 400us delay for errors/timeouts
> +				   (DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2) */
> +				udelay(400);
>  				continue;
> +			}
>  			if (status & DP_AUX_CH_CTL_DONE)
>  				break;
>  		}
> -- 
> 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: [PATCH 12/12] drm/i915: Add a delay in Displayport AUX transactions for compliance testing

[Todd Previte]

[-- Attachment #1.1: Type: text/plain, Size: 3637 bytes --]


> Daniel Vetter <mailto:daniel@ffwll.ch>
> Tuesday, July 15, 2014 12:46 AM
> On Mon, Jul 14, 2014 at 12:10:47PM -0700, Todd Previte wrote:
>> The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1 specifies that
>> repeated AUX transactions after a failure (NACK, DEFER or no response) must have
>> a minimum delay of 400us before the resend can occur. Tests 4.2.1.1 and 4.2.1.2 are two
>> tests that require this specifically.
>>
>> Signed-off-by: Todd Previte<tprevite@gmail.com>
>
> Since this is a minimal timeout ... shouldn't we put it into the dp
> helpers instead?
> -Daniel
This delay catches the case where the sink device either does not 
respond at all or responds with an invalid AUX transaction. These are 
lower level errors than the DRM helper functions are supposed to handle. 
I mistakenly mentioned NACK/DEFER in the commit message - that's not an 
accurate description of what this patch does. I will fix the commit 
message for v3 of this patch.


-T
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 0e207aaf..f0664cd 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -573,8 +573,12 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>>   				   DP_AUX_CH_CTL_RECEIVE_ERROR);
>>
>>   			if (status&  (DP_AUX_CH_CTL_TIME_OUT_ERROR |
>> -				      DP_AUX_CH_CTL_RECEIVE_ERROR))
>> +				      DP_AUX_CH_CTL_RECEIVE_ERROR)) {
>> +				/* DP compliance requires 400us delay for errors/timeouts
>> +				   (DP CTS 1.2 Core Rev 1.1, 4.2.1.1&  4.2.1.2) */
>> +				udelay(400);
>>   				continue;
>> +			}
>>   			if (status&  DP_AUX_CH_CTL_DONE)
>>   				break;
>>   		}
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> Todd Previte <mailto:tprevite@gmail.com>
> Monday, July 14, 2014 12:10 PM
> The Displayport Link Layer Compliance Testing Specification 1.2 rev 
> 1.1 specifies that
> repeated AUX transactions after a failure (NACK, DEFER or no response) 
> must have
> a minimum delay of 400us before the resend can occur. Tests 4.2.1.1 
> and 4.2.1.2 are two
> tests that require this specifically.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com>
> ---
> drivers/gpu/drm/i915/intel_dp.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> b/drivers/gpu/drm/i915/intel_dp.c
> index 0e207aaf..f0664cd 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -573,8 +573,12 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> DP_AUX_CH_CTL_RECEIVE_ERROR);
>
> if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR |
> - DP_AUX_CH_CTL_RECEIVE_ERROR))
> + DP_AUX_CH_CTL_RECEIVE_ERROR)) {
> + /* DP compliance requires 400us delay for errors/timeouts
> + (DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2) */
> + udelay(400);
> continue;
> + }
> if (status & DP_AUX_CH_CTL_DONE)
> break;
> }
> Todd Previte <mailto:tprevite@gmail.com>
> Monday, July 14, 2014 12:10 PM
>
> V2:
> - Addressed review feedback from the mailing list
> - Broke up patches into smaller, easily managed chunks
> - Reordered the patches such that they can be applied in order
> - Fixed checkpatch.pl errors across the patchset
> - Updated and enhanced functionality for the EDID test function
> - Completely revamped the mode set operations for compliance testing
>

-- 
Sent with Postbox <http://www.getpostbox.com>

[-- Attachment #1.2.1: Type: text/html, Size: 7633 bytes --]

[-- Attachment #1.2.2: postbox-contact.jpg --]
[-- Type: image/jpeg, Size: 1192 bytes --]

[-- Attachment #1.2.3: compose-unknown-contact.jpg --]
[-- Type: image/jpeg, Size: 770 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 12/12] drm/i915: Add a delay in Displayport AUX transactions for compliance testing

[Paulo Zanoni]

[-- Attachment #1.1.1: Type: text/plain, Size: 4160 bytes --]

2014-07-15 12:34 GMT-03:00 Todd Previte <tprevite@gmail.com>:

>
>   Daniel Vetter <daniel@ffwll.ch>
>  Tuesday, July 15, 2014 12:46 AM
>
> On Mon, Jul 14, 2014 at 12:10:47PM -0700, Todd Previte wrote:
>
> The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1 specifies that
> repeated AUX transactions after a failure (NACK, DEFER or no response) must have
> a minimum delay of 400us before the resend can occur. Tests 4.2.1.1 and 4.2.1.2 are two
> tests that require this specifically.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com> <tprevite@gmail.com>
>
> Since this is a minimal timeout ... shouldn't we put it into the dp
> helpers instead?
> -Daniel
>
>  This delay catches the case where the sink device either does not respond
> at all or responds with an invalid AUX transaction. These are lower level
> errors than the DRM helper functions are supposed to handle. I mistakenly
> mentioned NACK/DEFER in the commit message - that's not an accurate
> description of what this patch does. I will fix the commit message for v3
> of this patch.
>
>
I agree your patch idea seems to be correct, but you should probably also
do a little review of drm_dp_dpcd_access() since it also retries everything
7 times and already has 400ms sleep.

Just one minor comment: why not use usleep_range() instead of udelay()?



>
> -T
>
>  ---
>  drivers/gpu/drm/i915/intel_dp.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 0e207aaf..f0664cd 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -573,8 +573,12 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>  				   DP_AUX_CH_CTL_RECEIVE_ERROR);
>
>  			if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR |
> -				      DP_AUX_CH_CTL_RECEIVE_ERROR))
> +				      DP_AUX_CH_CTL_RECEIVE_ERROR)) {
> +				/* DP compliance requires 400us delay for errors/timeouts
> +				   (DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2) */
> +				udelay(400);
>  				continue;
> +			}
>  			if (status & DP_AUX_CH_CTL_DONE)
>  				break;
>  		}
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing listIntel-gfx@lists.freedesktop.orghttp://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>   Todd Previte <tprevite@gmail.com>
>  Monday, July 14, 2014 12:10 PM
> The Displayport Link Layer Compliance Testing Specification 1.2 rev 1.1
> specifies that
> repeated AUX transactions after a failure (NACK, DEFER or no response)
> must have
> a minimum delay of 400us before the resend can occur. Tests 4.2.1.1 and
> 4.2.1.2 are two
> tests that require this specifically.
>
> Signed-off-by: Todd Previte <tprevite@gmail.com> <tprevite@gmail.com>
> ---
> drivers/gpu/drm/i915/intel_dp.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> index 0e207aaf..f0664cd 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -573,8 +573,12 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
> DP_AUX_CH_CTL_RECEIVE_ERROR);
>
> if (status & (DP_AUX_CH_CTL_TIME_OUT_ERROR |
> - DP_AUX_CH_CTL_RECEIVE_ERROR))
> + DP_AUX_CH_CTL_RECEIVE_ERROR)) {
> + /* DP compliance requires 400us delay for errors/timeouts
> + (DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2) */
> + udelay(400);
> continue;
> + }
> if (status & DP_AUX_CH_CTL_DONE)
> break;
> }
>   Todd Previte <tprevite@gmail.com>
>  Monday, July 14, 2014 12:10 PM
>
> V2:
> - Addressed review feedback from the mailing list
> - Broke up patches into smaller, easily managed chunks
> - Reordered the patches such that they can be applied in order
> - Fixed checkpatch.pl errors across the patchset
> - Updated and enhanced functionality for the EDID test function
> - Completely revamped the mode set operations for compliance testing
>
>
> --
> Sent with Postbox <http://www.getpostbox.com>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>


-- 
Paulo Zanoni

[-- Attachment #1.1.2: Type: text/html, Size: 8468 bytes --]

[-- Attachment #1.2: postbox-contact.jpg --]
[-- Type: image/jpeg, Size: 1192 bytes --]

[-- Attachment #1.3: compose-unknown-contact.jpg --]
[-- Type: image/jpeg, Size: 770 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] Displayport compliance testing

[Daniel Vetter]

On Mon, Jul 14, 2014 at 12:10:35PM -0700, Todd Previte wrote:
> >This patch set adds the foundational support for Displayport compliance testing in the
> >i915 driver. It implements the framework for automated test support that preclude the
> >need (most) for operator input during testing. Tests for AUX transactions, EDID reads
> >and basic link training have also been included, along with any support and utility
> >functions required. Some changes have also been made to existing code to accommodate
> >compliance testing.
> 
> V2:
> - Addressed review feedback from the mailing list
> - Broke up patches into smaller, easily managed chunks
> - Reordered the patches such that they can be applied in order
> - Fixed checkpatch.pl errors across the patchset  
> - Updated and enhanced functionality for the EDID test function
> - Completely revamped the mode set operations for compliance testing

Ok, high level review of the overall approach.

So your design is to implement special functions for each test procedure.
This has two issues:
- We have duplicated code we test, which has a really high chance to be
  different from what users actually run on their systems. And so dp
  validation won't actually validate the real code. Example would be the
  fallback edid reading tests using defer/nacks. The drm i2c helpers
  already have logic for this (including fallback modes), but very likely
  it doesn't quite match the dp requirements. So we need to adjust that
  (presuming the dp standard is sane).
- Even if we can fix this there's still lots of important code in the
  probe paths we can't test with this. Userspace updates edids through the
  get_connector ioctl, and there's lots of additional logic in-between
  until we reach the dp connector implementation in intel_dp.c.

So what I'd like to see is that the test procedures are implemented in
userspace, using nothing more than the standard kms interfaces.

Afaics we need three steps overall:
- Get a shot hpd pulse and notice that we should do a validation test
  sequence. We need to get this information to userspace, and the best
  approach would be a uevent on the connector in sysfs. We can supply any
  additional metadata (like the test mode) userspace needs with uevent
  attributes.

- Do the test from userspace. Depending upon what exactly needs to be done
  we might need to extend the properties exposed to userspace, e.g. dp
  link parameters.

- Give the result (edid checksum, ...) back to the dp sink/validator. A
  generic dp aux transaction interface for userspace in the dp helpers,
  maybe even as a real /dev node should fit the bill.

dp validation has the potential to automatically test a pile of code for
which we currently have 0 automated test coverage at all. I want to fully
seize this opportunity instead of doing the bare minimum to get a (rather
useless) certification.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2] Displayport compliance testing

[Jesse Barnes]

On Tue, 22 Jul 2014 08:41:11 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Mon, Jul 14, 2014 at 12:10:35PM -0700, Todd Previte wrote:
> > >This patch set adds the foundational support for Displayport compliance testing in the
> > >i915 driver. It implements the framework for automated test support that preclude the
> > >need (most) for operator input during testing. Tests for AUX transactions, EDID reads
> > >and basic link training have also been included, along with any support and utility
> > >functions required. Some changes have also been made to existing code to accommodate
> > >compliance testing.
> > 
> > V2:
> > - Addressed review feedback from the mailing list
> > - Broke up patches into smaller, easily managed chunks
> > - Reordered the patches such that they can be applied in order
> > - Fixed checkpatch.pl errors across the patchset  
> > - Updated and enhanced functionality for the EDID test function
> > - Completely revamped the mode set operations for compliance testing
> 
> Ok, high level review of the overall approach.
> 
> So your design is to implement special functions for each test procedure.
> This has two issues:
> - We have duplicated code we test, which has a really high chance to be
>   different from what users actually run on their systems. And so dp
>   validation won't actually validate the real code. Example would be the
>   fallback edid reading tests using defer/nacks. The drm i2c helpers
>   already have logic for this (including fallback modes), but very likely
>   it doesn't quite match the dp requirements. So we need to adjust that
>   (presuming the dp standard is sane).
> - Even if we can fix this there's still lots of important code in the
>   probe paths we can't test with this. Userspace updates edids through the
>   get_connector ioctl, and there's lots of additional logic in-between
>   until we reach the dp connector implementation in intel_dp.c.
> 
> So what I'd like to see is that the test procedures are implemented in
> userspace, using nothing more than the standard kms interfaces.
> 
> Afaics we need three steps overall:
> - Get a shot hpd pulse and notice that we should do a validation test
>   sequence. We need to get this information to userspace, and the best
>   approach would be a uevent on the connector in sysfs. We can supply any
>   additional metadata (like the test mode) userspace needs with uevent
>   attributes.
> 
> - Do the test from userspace. Depending upon what exactly needs to be done
>   we might need to extend the properties exposed to userspace, e.g. dp
>   link parameters.
> 
> - Give the result (edid checksum, ...) back to the dp sink/validator. A
>   generic dp aux transaction interface for userspace in the dp helpers,
>   maybe even as a real /dev node should fit the bill.
> 
> dp validation has the potential to automatically test a pile of code for
> which we currently have 0 automated test coverage at all. I want to fully
> seize this opportunity instead of doing the bare minimum to get a (rather
> useless) certification.

This amounts to a complete rewrite with a different goal.  In this
series, I only see two big new test functions added: one for EDID
testing and one for link training.  The EDID one could probably share
some more code with the core, but for link training it seems pretty
simple, unless we need a full mode & fb for some reason (I'm presuming
Todd has tested this with the equipment).  Paulo had some good
feedback, but overall this is a small addition to get the compliance
tests working and get some coverage on the link training paths which we
desperately need.

Having some additional user level tests would be great, but that's a
much bigger and different task than simply implementing what's required
for the DP compliance test.  Asking Todd to take on a huge new task
just because he posted this series is a big request.  Are you saying
you'll reject this approach entirely?

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH v2] Displayport compliance testing

[Daniel Vetter]

On Tue, Jul 22, 2014 at 10:48 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> Are you saying
> you'll reject this approach entirely?

I'm saying that I don't see terrible lot of value in adding a bunch of
code for a sticker, and that we should look into making it actually
useful by testing the paths that end-users end up using. And we have
to keep this working once it's merged.

But if it doesn't make sense to make this sticker useful while still
being able to get it then I'll reconsider.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2] Displayport compliance testing

[Jesse Barnes]

On Tue, 22 Jul 2014 22:53:44 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Jul 22, 2014 at 10:48 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > Are you saying
> > you'll reject this approach entirely?
> 
> I'm saying that I don't see terrible lot of value in adding a bunch of
> code for a sticker, and that we should look into making it actually
> useful by testing the paths that end-users end up using. And we have
> to keep this working once it's merged.
> 
> But if it doesn't make sense to make this sticker useful while still
> being able to get it then I'll reconsider.

Yeah I think it depends on the test.  We're supposed to go through
existing paths for testing e.g. link training with different params
(though with a fixed fb and mode), so getting coverage there is
something we want regardless.  But getting something like probing
covered as part of the compliance testing may be something else
entirely...

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH v2] Displayport compliance testing

[Paulo Zanoni]

2014-07-22 18:11 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
> On Tue, 22 Jul 2014 22:53:44 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
>
>> On Tue, Jul 22, 2014 at 10:48 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> > Are you saying
>> > you'll reject this approach entirely?
>>
>> I'm saying that I don't see terrible lot of value in adding a bunch of
>> code for a sticker, and that we should look into making it actually
>> useful by testing the paths that end-users end up using. And we have
>> to keep this working once it's merged.
>>
>> But if it doesn't make sense to make this sticker useful while still
>> being able to get it then I'll reconsider.
>
> Yeah I think it depends on the test.  We're supposed to go through
> existing paths for testing e.g. link training with different params
> (though with a fixed fb and mode), so getting coverage there is
> something we want regardless.  But getting something like probing
> covered as part of the compliance testing may be something else
> entirely...

I was finally able to take some time to read the spec, and I agree
that the hybrid approach looks like the way to go. Some tests require
specifically-crafted FBs, while some other tests cause real hotplug
events to be sent from the sink. If there's an unknown/unspecified
user-space running when the tests are happening, who knows how it is
going to react? Of course, for tests that can be implemented directly
inside the Kernel still using the "standard" code paths, we should do
it in the Kernel.

One possible approach that I thought would be the following:
- Each DP encoder provides its own debugfs file for DP test compiance
(e.g., /sys/kernel/debug/dri/0/i915_ddi_b_dp_test_compliance).
- If the file is not open, any requests for tests that require special
actions from our driver - outside of the normal behavior - will be
NACKed.
- If the file is open, we ACK test requests and print special strings
to the debugfs file telling the user-space app what it's supposed to
do. We could use simple strings like "set the preferred mode", "set
failsafe mode", "set mode using FB test pattern Y", etc. A stringly
typed protocol :)
- The user-space app needs to be the DRM master, open the debugfs
file, parse the operations it prints and act accordingly, and listen
to the hotplug events sent by the Kernel.
- If some special corner quirky case needs to be done (e.g., train
link with a specific number of lanes), the Kernel should store this
information at struct intel_dp, and then when a modeset is done on
this encoder, we check if the debugfs file is open (i.e., we're doing
compliance testing) and then we use the specified configuration. With
this, we can probably avoid special uevents or debug-only
connector/encoder properties.
- The user-space app could be part of intel-gpu-tools.

Anyway, this is just an alternate idea to Daniel's suggestion, and
many other possible implementation ideas would work for me. Todd, what
is your opinion?

I will continue the review of the patches we currently have, since it
seems we didn't decide what we're gonna merge.

>
> --
> Jesse Barnes, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH v2] Displayport compliance testing

[Daniel Vetter]

On Tue, Jul 29, 2014 at 06:53:57PM -0300, Paulo Zanoni wrote:
> 2014-07-22 18:11 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>:
> > On Tue, 22 Jul 2014 22:53:44 +0200
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> >> On Tue, Jul 22, 2014 at 10:48 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> >> > Are you saying
> >> > you'll reject this approach entirely?
> >>
> >> I'm saying that I don't see terrible lot of value in adding a bunch of
> >> code for a sticker, and that we should look into making it actually
> >> useful by testing the paths that end-users end up using. And we have
> >> to keep this working once it's merged.
> >>
> >> But if it doesn't make sense to make this sticker useful while still
> >> being able to get it then I'll reconsider.
> >
> > Yeah I think it depends on the test.  We're supposed to go through
> > existing paths for testing e.g. link training with different params
> > (though with a fixed fb and mode), so getting coverage there is
> > something we want regardless.  But getting something like probing
> > covered as part of the compliance testing may be something else
> > entirely...
> 
> I was finally able to take some time to read the spec, and I agree
> that the hybrid approach looks like the way to go. Some tests require
> specifically-crafted FBs, while some other tests cause real hotplug
> events to be sent from the sink. If there's an unknown/unspecified
> user-space running when the tests are happening, who knows how it is
> going to react? Of course, for tests that can be implemented directly
> inside the Kernel still using the "standard" code paths, we should do
> it in the Kernel.
> 
> One possible approach that I thought would be the following:
> - Each DP encoder provides its own debugfs file for DP test compiance
> (e.g., /sys/kernel/debug/dri/0/i915_ddi_b_dp_test_compliance).
> - If the file is not open, any requests for tests that require special
> actions from our driver - outside of the normal behavior - will be
> NACKed.

Yeah that sounds like a reasonable safety measure to make sure we don't do
stupid things (e.g. malicious DP connector trying to break into the
kernel).

> - If the file is open, we ACK test requests and print special strings
> to the debugfs file telling the user-space app what it's supposed to
> do. We could use simple strings like "set the preferred mode", "set
> failsafe mode", "set mode using FB test pattern Y", etc. A stringly
> typed protocol :)

We need to check how much work this is, since we'll probably need to
implement polling. Otoh we've just done that for the crc interfaces, so
shouldn't be too much fuzz.

If userspace needs to do special dp aux transactions I still think we
should simply expose a proper dp aux interface, similar to i2c.

> - The user-space app needs to be the DRM master, open the debugfs
> file, parse the operations it prints and act accordingly, and listen
> to the hotplug events sent by the Kernel.
> - If some special corner quirky case needs to be done (e.g., train
> link with a specific number of lanes), the Kernel should store this
> information at struct intel_dp, and then when a modeset is done on
> this encoder, we check if the debugfs file is open (i.e., we're doing
> compliance testing) and then we use the specified configuration. With
> this, we can probably avoid special uevents or debug-only
> connector/encoder properties.
> - The user-space app could be part of intel-gpu-tools.
> 
> Anyway, this is just an alternate idea to Daniel's suggestion, and
> many other possible implementation ideas would work for me. Todd, what
> is your opinion?

Well I've only tossed out a rough idea that we should try to use the same
paths for validation as userspace is using in general. Sounds like you
have some good ideas how to get there in practice.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2] Displayport compliance testing

[Todd Previte]

[-- Attachment #1.1: Type: text/plain, Size: 9628 bytes --]



> Paulo Zanoni <mailto:przanoni@gmail.com>
> Tuesday, July 29, 2014 2:53 PM
> 2014-07-22 18:11 GMT-03:00 Jesse Barnes<jbarnes@virtuousgeek.org>:
>> On Tue, 22 Jul 2014 22:53:44 +0200
>> Daniel Vetter<daniel@ffwll.ch>  wrote:
>>
>>> On Tue, Jul 22, 2014 at 10:48 PM, Jesse Barnes<jbarnes@virtuousgeek.org>  wrote:
>>>> Are you saying
>>>> you'll reject this approach entirely?
>>> I'm saying that I don't see terrible lot of value in adding a bunch of
>>> code for a sticker, and that we should look into making it actually
>>> useful by testing the paths that end-users end up using. And we have
>>> to keep this working once it's merged.
>>>
>>> But if it doesn't make sense to make this sticker useful while still
>>> being able to get it then I'll reconsider.
>> Yeah I think it depends on the test.  We're supposed to go through
>> existing paths for testing e.g. link training with different params
>> (though with a fixed fb and mode), so getting coverage there is
>> something we want regardless.  But getting something like probing
>> covered as part of the compliance testing may be something else
>> entirely...
>
> I was finally able to take some time to read the spec, and I agree
> that the hybrid approach looks like the way to go. Some tests require
> specifically-crafted FBs, while some other tests cause real hotplug
> events to be sent from the sink. If there's an unknown/unspecified
> user-space running when the tests are happening, who knows how it is
> going to react? Of course, for tests that can be implemented directly
> inside the Kernel still using the "standard" code paths, we should do
> it in the Kernel.
There's no question that this is going to require considerable support 
within the kernel. The tests that can be placed inside the kernel are 
mostly going to be ones that simply can't be accomplished from userspace 
(the 400us AUX transaction retry, for instance) or are elements that are 
necessary to enable the userspace tests to execute properly. The tests 
themselves though should definitely be handled out in userspace where 
possible.
> One possible approach that I thought would be the following:
> - Each DP encoder provides its own debugfs file for DP test compiance
> (e.g., /sys/kernel/debug/dri/0/i915_ddi_b_dp_test_compliance).
> - If the file is not open, any requests for tests that require special
> actions from our driver - outside of the normal behavior - will be
> NACKed.
There's a couple options here that I've considered. This is a perfectly 
valid option but it's also possible to just use one debugfs file for the 
compliance work. It's unlikely that multiple Displayport connectors will 
ever be testing simultaneously, so the need to adjust the parameters on 
a per-connector basis is arguable. Only the connector for the sink that 
issued the test request would be required to read the debugfs file and 
adhere to its parameters.

For general debugging of Displayport though, this is a better option as 
it allows for considerably more flexibility and utility. Personally I 
prefer the individual file approach, even though it might end up being 
somewhat more complicated to implement. The single-file approach does 
have merit though, so if anyone has a solid argument in favor of that, 
it's worth the discussion.
> - If the file is open, we ACK test requests and print special strings
> to the debugfs file telling the user-space app what it's supposed to
> do. We could use simple strings like "set the preferred mode", "set
> failsafe mode", "set mode using FB test pattern Y", etc. A stringly
> typed protocol :)
Probably better to use clearly-defined constants instead of strings, but 
yes, that's the idea. :)
> - The user-space app needs to be the DRM master, open the debugfs
> file, parse the operations it prints and act accordingly, and listen
> to the hotplug events sent by the Kernel.
Agreed.
> - If some special corner quirky case needs to be done (e.g., train
> link with a specific number of lanes), the Kernel should store this
> information at struct intel_dp, and then when a modeset is done on
> this encoder, we check if the debugfs file is open (i.e., we're doing
> compliance testing) and then we use the specified configuration. With
> this, we can probably avoid special uevents or debug-only
> connector/encoder properties.
I would argue here that it's better to leave as much of the active 
configuration undisturbed as possible and only update the fields 
necessary to complete the test(s). Additionally, any fields that are 
changed should be saved and subsequently restored upon completion of the 
test(s)
> - The user-space app could be part of intel-gpu-tools.
Cool, this is what I was planning on doing. One thing I was looking into 
was potentially launching the test app from a uevent in the kernel. But 
none of the solutions I could find were all that good and some were 
downright scary. So having the app as a separate entity that needs to be 
launched by the tester isn't such a bad idea.


Anyway, this is just an alternate idea to Daniel's suggestion, and
many other possible implementation ideas would work for me. Todd, what
is your opinion?

I will continue the review of the patches we currently have, since it
seems we didn't decide what we're gonna merge.

>
>> --
>> Jesse Barnes, Intel Open Source Technology Center
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> Jesse Barnes <mailto:jbarnes@virtuousgeek.org>
> Tuesday, July 22, 2014 2:11 PM
> On Tue, 22 Jul 2014 22:53:44 +0200
>
> Yeah I think it depends on the test. We're supposed to go through
> existing paths for testing e.g. link training with different params
> (though with a fixed fb and mode), so getting coverage there is
> something we want regardless. But getting something like probing
> covered as part of the compliance testing may be something else
> entirely...
>
> Daniel Vetter <mailto:daniel@ffwll.ch>
> Tuesday, July 22, 2014 1:53 PM
>
> I'm saying that I don't see terrible lot of value in adding a bunch of
> code for a sticker, and that we should look into making it actually
> useful by testing the paths that end-users end up using. And we have
> to keep this working once it's merged.
>
> But if it doesn't make sense to make this sticker useful while still
> being able to get it then I'll reconsider.
> -Daniel
> Jesse Barnes <mailto:jbarnes@virtuousgeek.org>
> Tuesday, July 22, 2014 1:48 PM
> On Tue, 22 Jul 2014 08:41:11 +0200
>
> This amounts to a complete rewrite with a different goal. In this
> series, I only see two big new test functions added: one for EDID
> testing and one for link training. The EDID one could probably share
> some more code with the core, but for link training it seems pretty
> simple, unless we need a full mode & fb for some reason (I'm presuming
> Todd has tested this with the equipment). Paulo had some good
> feedback, but overall this is a small addition to get the compliance
> tests working and get some coverage on the link training paths which we
> desperately need.
>
> Having some additional user level tests would be great, but that's a
> much bigger and different task than simply implementing what's required
> for the DP compliance test. Asking Todd to take on a huge new task
> just because he posted this series is a big request. Are you saying
> you'll reject this approach entirely?
>
> Daniel Vetter <mailto:daniel@ffwll.ch>
> Monday, July 21, 2014 11:41 PM
>
> Ok, high level review of the overall approach.
>
> So your design is to implement special functions for each test procedure.
> This has two issues:
> - We have duplicated code we test, which has a really high chance to be
> different from what users actually run on their systems. And so dp
> validation won't actually validate the real code. Example would be the
> fallback edid reading tests using defer/nacks. The drm i2c helpers
> already have logic for this (including fallback modes), but very likely
> it doesn't quite match the dp requirements. So we need to adjust that
> (presuming the dp standard is sane).
> - Even if we can fix this there's still lots of important code in the
> probe paths we can't test with this. Userspace updates edids through the
> get_connector ioctl, and there's lots of additional logic in-between
> until we reach the dp connector implementation in intel_dp.c.
>
> So what I'd like to see is that the test procedures are implemented in
> userspace, using nothing more than the standard kms interfaces.
>
> Afaics we need three steps overall:
> - Get a shot hpd pulse and notice that we should do a validation test
> sequence. We need to get this information to userspace, and the best
> approach would be a uevent on the connector in sysfs. We can supply any
> additional metadata (like the test mode) userspace needs with uevent
> attributes.
>
> - Do the test from userspace. Depending upon what exactly needs to be done
> we might need to extend the properties exposed to userspace, e.g. dp
> link parameters.
>
> - Give the result (edid checksum, ...) back to the dp sink/validator. A
> generic dp aux transaction interface for userspace in the dp helpers,
> maybe even as a real /dev node should fit the bill.
>
> dp validation has the potential to automatically test a pile of code for
> which we currently have 0 automated test coverage at all. I want to fully
> seize this opportunity instead of doing the bare minimum to get a (rather
> useless) certification.
> -Daniel

-- 
Sent with Postbox <http://www.getpostbox.com>

[-- Attachment #1.2.1: Type: text/html, Size: 17504 bytes --]

[-- Attachment #1.2.2: postbox-contact.jpg --]
[-- Type: image/jpeg, Size: 1254 bytes --]

[-- Attachment #1.2.3: compose-unknown-contact.jpg --]
[-- Type: image/jpeg, Size: 770 bytes --]

[-- Attachment #1.2.4: postbox-contact.jpg --]
[-- Type: image/jpeg, Size: 1203 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] Displayport compliance testing

[Jesse Barnes]

On Tue, 22 Jul 2014 13:48:45 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Tue, 22 Jul 2014 08:41:11 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Mon, Jul 14, 2014 at 12:10:35PM -0700, Todd Previte wrote:
> > > >This patch set adds the foundational support for Displayport compliance testing in the
> > > >i915 driver. It implements the framework for automated test support that preclude the
> > > >need (most) for operator input during testing. Tests for AUX transactions, EDID reads
> > > >and basic link training have also been included, along with any support and utility
> > > >functions required. Some changes have also been made to existing code to accommodate
> > > >compliance testing.
> > > 
> > > V2:
> > > - Addressed review feedback from the mailing list
> > > - Broke up patches into smaller, easily managed chunks
> > > - Reordered the patches such that they can be applied in order
> > > - Fixed checkpatch.pl errors across the patchset  
> > > - Updated and enhanced functionality for the EDID test function
> > > - Completely revamped the mode set operations for compliance testing
> > 
> > Ok, high level review of the overall approach.
> > 
> > So your design is to implement special functions for each test procedure.
> > This has two issues:
> > - We have duplicated code we test, which has a really high chance to be
> >   different from what users actually run on their systems. And so dp
> >   validation won't actually validate the real code. Example would be the
> >   fallback edid reading tests using defer/nacks. The drm i2c helpers
> >   already have logic for this (including fallback modes), but very likely
> >   it doesn't quite match the dp requirements. So we need to adjust that
> >   (presuming the dp standard is sane).
> > - Even if we can fix this there's still lots of important code in the
> >   probe paths we can't test with this. Userspace updates edids through the
> >   get_connector ioctl, and there's lots of additional logic in-between
> >   until we reach the dp connector implementation in intel_dp.c.
> > 
> > So what I'd like to see is that the test procedures are implemented in
> > userspace, using nothing more than the standard kms interfaces.
> > 
> > Afaics we need three steps overall:
> > - Get a shot hpd pulse and notice that we should do a validation test
> >   sequence. We need to get this information to userspace, and the best
> >   approach would be a uevent on the connector in sysfs. We can supply any
> >   additional metadata (like the test mode) userspace needs with uevent
> >   attributes.
> > 
> > - Do the test from userspace. Depending upon what exactly needs to be done
> >   we might need to extend the properties exposed to userspace, e.g. dp
> >   link parameters.
> > 
> > - Give the result (edid checksum, ...) back to the dp sink/validator. A
> >   generic dp aux transaction interface for userspace in the dp helpers,
> >   maybe even as a real /dev node should fit the bill.
> > 
> > dp validation has the potential to automatically test a pile of code for
> > which we currently have 0 automated test coverage at all. I want to fully
> > seize this opportunity instead of doing the bare minimum to get a (rather
> > useless) certification.
> 
> This amounts to a complete rewrite with a different goal.  In this
> series, I only see two big new test functions added: one for EDID
> testing and one for link training.  The EDID one could probably share
> some more code with the core, but for link training it seems pretty
> simple, unless we need a full mode & fb for some reason (I'm presuming
> Todd has tested this with the equipment).  Paulo had some good
> feedback, but overall this is a small addition to get the compliance
> tests working and get some coverage on the link training paths which we
> desperately need.
> 
> Having some additional user level tests would be great, but that's a
> much bigger and different task than simply implementing what's required
> for the DP compliance test.  Asking Todd to take on a huge new task
> just because he posted this series is a big request.  Are you saying
> you'll reject this approach entirely?

I'd have to check the spec, maybe the best approach is a hybrid; some
tests handled directly in the kernel to easily re-use our existing
bits, and some in userspace to push through a full mode set path more
easily.

-- 
Jesse Barnes, Intel Open Source Technology Center