[Intel-gfx] [PATCH 01/12] drm/i915: Add automated testing support for Displayport compliance testing
Paulo Zanoni
przanoni at gmail.com
Tue Jul 22 00:09:22 CEST 2014
Hi
2014-07-14 16:10 GMT-03:00 Todd Previte <tprevite at 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 at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
More information about the Intel-gfx
mailing list