Re: [PATCH 08/13] drm/i915: Support EDID compliance tests with the intel_dp_autotest_edid() function

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



2015-04-10 14:42 GMT-03:00 Todd Previte <tprevite@xxxxxxxxx>:
> 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
> V5:
> - Update test active flag variable name to match the change in the
>   first patch of the series.
> - Relocated the test active flag declaration and initialization
>   to this patch
> V6:
> - Updated to use the new flag for raw EDID header corruption
> - Removed the extra EDID read from the autotest function
> - Added the edid_checksum variable to struct intel_dp so that the
>   autotest function can write it to the sink device
> - Moved the update to the hpd_pulse function to another patch
> - Removed extraneous constants
> V7:
> - Fixed erroneous placement of the checksum assignment. In some cases
>   such as when the EDID read fails and is NULL, this causes a NULL ptr
>   dereference in the kernel. Bad news. Fixed now.
>
> Signed-off-by: Todd Previte <tprevite@xxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 46 ++++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  4 ++++
>  2 files changed, 50 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index ba2da44..3bfdc40 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -41,6 +41,12 @@
>
>  #define DP_LINK_CHECK_TIMEOUT  (10 * 1000)
>
> +/* Compliance test status bits  */
> +#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;
> @@ -3770,6 +3776,35 @@ 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)
>  {
>         uint8_t test_result = DP_TEST_NAK;
> +       uint32_t ret = 0;
> +
> +       if (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)
> +                */
> +               if (intel_dp->aux.i2c_nack_count > 0 ||
> +                       intel_dp->aux.i2c_defer_count > 0)
> +                       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 = INTEL_DP_RESOLUTION_FAILSAFE;

Since this case is expected to happen, shouldn't we return something
that's not DP_TEST_NAK here?

Also, we should try to write the checksum on this case too, shouldn't we?

I see on 4.2.2.6 that the only happy case involves setting the
failsafe resolution, and optionally sending the checksum.

(sorry for not catching this earlier... I just didn't spot the problem)


> +       } else {
> +               ret = drm_dp_dpcd_write(&intel_dp->aux,
> +                                       DP_TEST_EDID_CHECKSUM,
> +                                       &intel_dp->compliance_edid_checksum, 1);
> +               if (ret <= 0)
> +                       DRM_DEBUG_DRIVER("Failed to write EDID checksum\n");
> +               else
> +                       DRM_DEBUG_DRIVER("EDID checksum written to sink\n");
> +
> +               test_result = DP_TEST_ACK | DP_TEST_EDID_CHECKSUM_WRITE;
> +               intel_dp->compliance_test_data = INTEL_DP_RESOLUTION_STANDARD;
> +       }
> +
> +       /* Set test active flag here so userspace doesn't interrupt things */
> +       intel_dp->compliance_test_active = 1;
> +
>         return test_result;
>  }
>
> @@ -3785,7 +3820,10 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
>         uint8_t rxdata = 0;
>         int status = 0;
>
> +       intel_dp->compliance_test_active = 0;
>         intel_dp->compliance_test_type = 0;
> +       intel_dp->compliance_test_data = 0;
> +
>         intel_dp->aux.i2c_nack_count = 0;
>         intel_dp->aux.i2c_defer_count = 0;
>
> @@ -3920,11 +3958,19 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>
>         /* Displayport Link CTS Core 1.2 rev1.1 EDID testing
>          * 4.2.2.1 - EDID read required for all HPD events
> +        * Also used for 4.2.2.3, 4.2.2.4 4.2.2.5 and 4.2.2.6
> +        *   - compliance_edid_invalid flag used for 4.2.2.6
> +        *   - compliance_edid_checksum used for 4.2.2.3
>           */
> +       intel_dp->compliance_edid_invalid = 0;
>          edid_read = drm_get_edid(connector, adapter);
>          if (!edid_read || connector->edid_header_corrupt == 1) {
>                  DRM_DEBUG_DRIVER("Invalid EDID detected\n");
> +                intel_dp->compliance_edid_invalid = 1;
> +               intel_dp->compliance_edid_checksum = 0;
>          }
> +       else
> +               intel_dp->compliance_edid_checksum = edid_read->checksum;
>
>         /* Try to read the source of the interrupt */
>         if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index ab811e5..23030a1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -650,6 +650,10 @@ struct intel_dp {
>
>         /* Displayport compliance testing */
>         unsigned long compliance_test_type;
> +       unsigned long compliance_test_data;
> +       bool compliance_test_active;
> +       bool compliance_edid_invalid;
> +       uint8_t compliance_edid_checksum;
>  };
>
>  struct intel_digital_port {
> --
> 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





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux