On Tue, Nov 22, 2016 at 01:39:26PM -0800, Manasi Navare wrote:
> The intel_dp_autotest_video_pattern() 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 at intel.com>
> Cc: Jani Nikula <jani.nikula at linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter at intel.com>
> Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 14 +++++++-
> drivers/gpu/drm/i915/intel_dp.c | 68
> +++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_drv.h | 9 +++++
> 3 files changed, 90 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 3799a12..b048a0e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -4545,7 +4545,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: %d\n",
> + intel_dp->test_data.hdisplay);
> + seq_printf(m, "vdisplay: %d\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 f93e130..784f86e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -28,8 +28,10 @@
> #include <linux/i2c.h>
> #include <linux/slab.h>
> #include <linux/export.h>
> +#include <linux/types.h>
> #include <linux/notifier.h>
> #include <linux/reboot.h>
> +#include <asm/byteorder.h>
> #include <drm/drmP.h>
> #include <drm/drm_atomic_helper.h>
> #include <drm/drm_crtc.h>
> @@ -3868,7 +3870,73 @@ 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 test_misc;
> + __be16 h_width, v_height;
> + int status = 0;
> +
> + /* 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 "
> + "reference 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 "
> + "reference sink\n");
> + return 0;
> + }
> + intel_dp->test_data.hdisplay = be16_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 "
> + "reference sink\n");
> + return 0;
> + }
> + intel_dp->test_data.vdisplay = be16_to_cpu(v_height);
> +
> + 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 "
> + "reference sink\n");
> + return 0;
> + }
> + if (((test_misc & DP_TEST_COLOR_FORMAT_MASK) >> DP_TEST_MISC_BIT_1) !=
> + DP_VIDEO_PATTERN_RGB_FORMAT)
> + return test_result;
> + if (((test_misc & DP_TEST_DYNAMIC_RANGE_MASK) >> DP_TEST_MISC_BIT_3) !=
> + DP_VIDEO_PATTERN_VESA)
> + return test_result;
> + switch ((test_misc & DP_TEST_BIT_DEPTH_MASK) >> DP_TEST_MISC_BIT_5) {
> + 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;
> + 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;
> +
> return test_result;
> +
> }
>
> static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 3eb428e..ff4431e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -880,6 +880,12 @@ struct intel_dp_desc {
> u8 sw_minor_rev;
> } __packed;
>
> +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;
> @@ -961,6 +967,9 @@ struct intel_dp {
> u8 compliance_test_lane_count;
> u8 compliance_test_link_rate;
> 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
> + */
I like this idea of tying all the compliance test data into a sub-struct,
but please refactor the existing code (and the force_bpc you're adding in
this series) to also use that. Consistency wins over pretty imo.
Otherwise seems all reasonable, but this isn't a detailed review.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
