Re: [PATCH 5/5] drm/i915: Implement intel_dp_autotest_video_pattern function for DP Video pattern compliance tests

Ander Conselvan De Oliveira
2016-05-23T12:00:19Z

On Fri, 2016-04-29 at 18:28 -0700, Manasi Navare wrote:
> This video pattern test function gets invoked through the
> compliance test handler on a HPD short pulse if the test type is
> set to DP_TEST_VIDEO_PATTERN. This performs the DPCD registers
> reads to read the requested test pattern, video pattern resolution,
> frame rate and bits per color value. The results of this analysis
> are handed off to userspace so that the userspace app can set the
> video pattern mode appropriately for the test result/response.
>
> The compliance_test_active flag is set 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.
>
> Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 14 ++++++-
>  drivers/gpu/drm/i915/intel_dp.c     | 76
> +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h    |  9 +++++
>  include/drm/drm_dp_helper.h         | 14 ++++++-
>  4 files changed, 111 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 6ee69b1..c8d0805 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4458,7 +4458,19 @@ static int i915_displayport_test_data_show(struct
> seq_file *m, void *data)
>   if (connector->status == connector_status_connected &&
>       connector->encoder != NULL) {
>   intel_dp = enc_to_intel_dp(connector->encoder);
> - seq_printf(m, "%lx", intel_dp->compliance_test_data);
> + if (intel_dp->compliance_test_type ==
> +     DP_TEST_LINK_EDID_READ)
> + seq_printf(m, "%lx",
> +    intel_dp->compliance_test_data);
> + else if (intel_dp->compliance_test_type ==
> +  DP_TEST_LINK_VIDEO_PATTERN) {
> + seq_printf(m, "hdisplay: %lu\n",
> +    intel_dp->test_data.hdisplay);
> + seq_printf(m, "vdisplay: %lu\n",
> +    intel_dp->test_data.vdisplay);
> + seq_printf(m, "bpc: %u\n",
> +    intel_dp->test_data.bpc);
> + }
>   } else
>   seq_puts(m, "0");
>   }
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 456fc17..134cff8 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4018,6 +4018,82 @@ static uint8_t intel_dp_autotest_link_training(struct
> intel_dp *intel_dp)
>  static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
>  {
>   uint8_t test_result = DP_TEST_NAK;
> + uint8_t test_pattern;
> + uint16_t  h_width, v_height, test_misc;
> + int status = 0;
> +
> + /* Automation support for Video Pattern Tests has a dependency
> +  * on Link training Automation support (CTS Test 4.3.1.11)
> +  * Hence it returns a TEST NAK until the Link Training automation
> +  * support is added to the kernel
> +  */
> + return test_result;

We shouldn't merge this patch until this is resolved. There's no point in adding
dead code.

> +
> + /* Read the TEST_PATTERN (DP CTS 3.1.5) */
> + status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_PATTERN,
> +   &test_pattern, 1);
> + if (status <= 0) {
> + DRM_DEBUG_KMS("Could not read test pattern from"
> +       "refernce sink\n");
> + return 0;
> + }
> + if (test_pattern != DP_COLOR_RAMP)
> + return test_result;
> + intel_dp->test_data.video_pattern = test_pattern;
> +
> + status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_H_WIDTH,
> +   &h_width, 2);
> + if (status <= 0) {
> + DRM_DEBUG_KMS("Could not read H Width from"
> +       "refernce sink\n");
> + return 0;
> + }
> + intel_dp->test_data.hdisplay = (h_width & DP_TEST_MSB_MASK) >> 8 |
> + (h_width << 8);

Just use the kernel endianness helpers, i.e.:

__le16 h_width;

drm_dp_dpcd_read(..., &h_width, 2)
hdisplay = le16_to_cpu(h_width);

> +
> + status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_V_HEIGHT,
> +   &v_height, 2);
> + if (status <= 0) {
> + DRM_DEBUG_KMS("Could not read V Height from"
> +       "refernce sink\n");

reference

> + return 0;
> + }
> + intel_dp->test_data.vdisplay = (v_height & DP_TEST_MSB_MASK) >> 8 |
> + (v_height << 8);

Same here.

> +
> + status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_MISC,
> +   &test_misc, 1);
> + if (status <= 0) {
> + DRM_DEBUG_KMS("Could not read TEST MISC from"
> +       "refernce sink\n");

reference

> + return 0;
> + }
> + if (((test_misc & DP_TEST_COLOR_FORMAT_MASK) >> 1) !=
> +     DP_VIDEO_PATTERN_RGB_FORMAT)
> + return test_result;
> + if (((test_misc & DP_TEST_DYNAMIC_RANGE_MASK) >> 3) !=
> +     DP_VIDEO_PATTERN_VESA)
> + return test_result;
> + switch ((test_misc & DP_TEST_BIT_DEPTH_MASK) >> 5) {

Maybe define *_SHIFT to avoid the hardcoded values. A couple of helpers to make
the code more readable woudn't be bad either.

> + case 0:
> + intel_dp->compliance_force_bpc = 6;
> + intel_dp->test_data.bpc = 6;
> + break;
> + case 1:
> + intel_dp->compliance_force_bpc = 8;
> + intel_dp->test_data.bpc = 8;

So here's where compliance_force_bpc from patch 3 is set. Would be better to
squash that patch with this one.

> + break;
> + default:
> + return test_result;
> + }
> + /* Set test active flag here so userspace doesn't interrupt things */
> + intel_dp->compliance_test_active = 1;
> +
> + test_result = DP_TEST_ACK;
> + DRM_DEBUG_KMS("Hdisplay = %lu\n Vdisplay = %lu\n BPC = %u\n ",
> +       intel_dp->test_data.hdisplay,
> +       intel_dp->test_data.vdisplay,
> +       intel_dp->test_data.bpc);
>   return test_result;
>  }
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 10eaff8..6ba8a75 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -794,6 +794,12 @@ enum link_m_n_set {
>   M2_N2
>  };
>
> +struct compliance_test_data {
> + uint8_t video_pattern;
> + uint16_t hdisplay, vdisplay;
> + uint8_t bpc;
> +};
> +
>  struct intel_dp {
>   i915_reg_t output_reg;
>   i915_reg_t aux_ch_ctl_reg;
> @@ -866,6 +872,9 @@ struct intel_dp {
>   unsigned long compliance_test_data;
>   bool compliance_test_active;
>   unsigned long compliance_force_bpc; /* 0 for default or bpc to use */
> + struct compliance_test_data test_data;   /* a structure to hold all
> dp
> +   * compliance test data
> +   */
>  };
>
>  struct intel_digital_port {
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index 92d9a52..7422dc5 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -412,7 +412,19 @@
>
>  #define DP_TEST_LANE_COUNT     0x220
>
> -#define DP_TEST_PATTERN     0x221
> +#define DP_TEST_PATTERN 0x221
> +#define DP_COLOR_RAMP (1 << 0)
> +#define DP_TEST_H_WIDTH 0x22E
> +#define DP_TEST_V_HEIGHT 0x230
> +#define DP_TEST_MISC 0x232
> +#define DP_TEST_MSB_MASK 0xFF00
> +#define DP_VIDEO_PATTERN_RGB_FORMAT 0
> +#define DP_TEST_COLOR_FORMAT_MASK 0x6
> +#define DP_TEST_DYNAMIC_RANGE_MASK (1 << 3)
> +#define DP_VIDEO_PATTERN_VESA 0
> +#define DP_TEST_BIT_DEPTH_MASK 0x00E0
> +#define DP_TEST_BIT_DEPTH_6 0
> +#define DP_TEST_BIT_DEPTH_8 1

This should be in a separate patch with

Cc: dri-devel@lists.freedesktop.org

>
>  #define DP_TEST_CRC_R_CR     0x240
>  #define DP_TEST_CRC_G_Y     0x242