2014-07-14 16:10 GMT-03:00 Todd Previte <tprevite@xxxxxxxxx>:
> 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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
