Re: [PATCH 5/9] drm/i915: Update the EDID automated compliance test function

[Todd Previte <tprevite@gmail.com>]

On 4/8/2015 10:02 AM, Paulo Zanoni wrote:
> 2015-03-31 14:15 GMT-03:00 Todd Previte <tprevite@gmail.com>:
>> Updates the EDID compliance test function to perform the EDID read as
>> required by the tests. This read needs to take place in the kernel for
>> reasons of speed and efficiency. The results of the EDID read operations
>> are handed off to userspace so that the userspace app can set the display
>> mode appropriately for the test response.
>>
>> The compliance_test_active flag now appears at the end of the individual
>> test handling functions. This is so that the kernel-side operations can
>> be completed without the risk of interruption from the userspace app
>> that is polling on that flag.
>>
>> V2:
>> - Addressed mailing list feedback
>> - Removed excess debug messages
>> - Removed extraneous comments
>> - Fixed formatting issues (line length > 80)
>> - Updated the debug message in compute_edid_checksum to output hex values
>>    instead of decimal
>> V3:
>> - Addressed more list feedback
>> - Added the test_active flag to the autotest function
>> - Removed test_active flag from handler
>> - Added failsafe check on the compliance test active flag
>>    at the end of the test handler
>> - Fixed checkpatch.pl issues
>> V4:
>> - Removed the checksum computation function and its use as it has been
>>    rendered superfluous by changes to the core DRM EDID functions
>> - Updated to use the raw header corruption detection mechanism
>> - Moved the declaration of the test_data variable here
>>
>> Signed-off-by: Todd Previte <tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c  | 53 ++++++++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/intel_drv.h |  3 ++-
>>   2 files changed, 55 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 57f8e43..16d35903 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -41,6 +41,16 @@
>>
>>   #define DP_LINK_CHECK_TIMEOUT  (10 * 1000)
>>
>> +/* Compliance test status bits  */
>> +#define INTEL_DP_EDID_SHIFT_MASK       0
>> +#define INTEL_DP_EDID_OK               (0 << INTEL_DP_EDID_SHIFT_MASK)
>> +#define INTEL_DP_EDID_CORRUPT          (1 << INTEL_DP_EDID_SHIFT_MASK)
>> +
>> +#define INTEL_DP_RESOLUTION_SHIFT_MASK 4
>> +#define INTEL_DP_RESOLUTION_PREFERRED  (1 << INTEL_DP_RESOLUTION_SHIFT_MASK)
>> +#define INTEL_DP_RESOLUTION_STANDARD   (2 << INTEL_DP_RESOLUTION_SHIFT_MASK)
>> +#define INTEL_DP_RESOLUTION_FAILSAFE   (3 << INTEL_DP_RESOLUTION_SHIFT_MASK)
>> +
>>   struct dp_link_dpll {
>>          int link_bw;
>>          struct dpll dpll;
>> @@ -3766,7 +3776,44 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
>>
>>   static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
>>   {
>> +       struct drm_connector *connector = &intel_dp->attached_connector->base;
>> +       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
>> +       struct edid *edid_read = NULL;
>>          uint8_t test_result = DP_TEST_NAK;
>> +       uint32_t ret = 0;
> Variable "ret" is set but not used.

Well it was "used" but the value wasn't really checked. The next version 
of this patch checks the value and reports status based on that.

>
>> +
>> +       edid_read = drm_get_edid(connector, adapter);
> This is the additional edid read mentioned in the review of the previous patch.

This one has been removed for the next patch revision.

>
>> +
>> +       if (drm_raw_edid_header_corrupt() == 1) {
>> +               DRM_DEBUG_DRIVER("EDID Header corrupted\n");
>> +               intel_dp->compliance_edid_invalid = 1;
>> +       }
>> +
>> +       if (edid_read == NULL || intel_dp->compliance_edid_invalid ||
>> +           intel_dp->aux.i2c_defer_count > 6) {
>> +               /* Check for NACKs/DEFERs, use failsafe if detected
>> +                  (DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5) */
> Missing '*' char on the comment.
Fixed.
>
>
>> +               if (intel_dp->aux.i2c_nack_count > 0 ||
>> +                       intel_dp->aux.i2c_defer_count > 0)
> Since we're potentially reading edid more than once after the test
> starts - as explained in the review of p4 -, do those nack/defer
> counts make sense? Shouldn't we zero them before each edid read
> attempt?

That's a good point. That had potential to adversely affect the test 
results for the NACKs and DEFERs. Removing the extra EDID read should 
eliminate that problem though, so I think the next version should be ok.

>
>> +                       DRM_DEBUG_KMS("EDID read had %d NACKs, %d DEFERs\n",
>> +                                     intel_dp->aux.i2c_nack_count,
>> +                                     intel_dp->aux.i2c_defer_count);
>> +               intel_dp->compliance_test_data = DP_TEST_LINK_EDID_READ |
>> +                                                INTEL_DP_EDID_CORRUPT  |
>> +                                                INTEL_DP_RESOLUTION_FAILSAFE;
> The compliance_test_data variable certainly needs a very good
> documentation on what its bits means - possibly on top of its
> definition, which is on patch 1 right now. Maybe it should be split
> into more than one variable, since the bits that go inside it come
> from different places. At least in the definition we should notice
> "bits2-4 are from dp_helper.h". Another problem is that bits 0 and 1
> from dp_helper.h can't be used there, which is even more confusing.

Actually this already *has* been split into different variables. :) The 
test type (the bits from the dp_helper header) are already in a variable 
of the same name (compliance_test_type). The EDID corrupt flags can be 
removed too, since those are no longer necessary. The test data simply 
indicates to user space what resolution to set. I'll put a quick 
description that effect in the header where those variables are declared 
and clean up the code above for the next patch version.

>
>> +       } else {
>> +               ret = drm_dp_dpcd_write(&intel_dp->aux,
>> +                                       DP_TEST_EDID_CHECKSUM,
>> +                                       &edid_read->checksum, 1);
>> +               test_result = DP_TEST_ACK | DP_TEST_EDID_CHECKSUM_WRITE;
>> +               intel_dp->compliance_test_data = DP_TEST_LINK_EDID_READ |
>> +                                                INTEL_DP_EDID_OK       |
>> +                                                INTEL_DP_RESOLUTION_STANDARD;
>> +       }
>> +
>> +       /* Set test active flag here so userspace doesn't interrupt things */
>> +       intel_dp->compliance_testing_active = 1;
>> +
>>          return test_result;
>>   }
>>
>> @@ -4596,6 +4643,12 @@ mst_fail:
>>                  DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
>>                  intel_dp->is_mst = false;
>>                  drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
>> +       } else {
>> +               /* SST mode - handle short/long pulses here */
>> +               drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> +               intel_dp_check_link_status(intel_dp);
>> +               drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> +               ret = IRQ_HANDLED;
> I guess this chunk belongs to patch 6, especially since you rewrite
> part of this new code there.
Yep. This has been moved already. As I mentioned earlier on IRC some of 
the patch chunks have been moved around and squished here and there. So 
I've regenerated the whole set for V5.
>
>>          }
>>   put_power:
>>          intel_display_power_put(dev_priv, power_domain);
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 42e4251..fb6134f 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -649,7 +649,8 @@ struct intel_dp {
>>                                       uint32_t aux_clock_divider);
>>
>>          /* Displayport compliance testing */
>> -       unsigned long compliance_test_type;
>> +       unsigned char compliance_test_type;
> This is the chunk I was talking about in my review of p1.
Yeah, this should all be fixed now.
>> +       unsigned long compliance_test_data;
>>          bool compliance_testing_active;
>>          bool compliance_edid_invalid;
>>   };
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>

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