On Wed, 2016-05-25 at 15:46 -0700, Manasi Navare wrote:
> On Mon, May 23, 2016 at 03:00:20PM +0300, Ander Conselvan De Oliveira wrote:
> >
> > 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@xxxxxxxxx>
> > > ---
> > > 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.
> >
> >
> I agree. I would still respin it based on the review feedback and keep it
> ready.
> So as soon as the link training rework is done, this patch can be pulled in
> and
> this test can b enabled.
>
> >
> > >
> > > +
> > > + /* 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);
> >
> I am not changing the endiannness here. This logic is implemented to swap
> the LSB and MSB because dpcd_read helper function reads two bytes from two
> consecutive
> registers into a 16 bit variable. This is required because DPR writes 0:7 bits
> of Hwidth
> to register 0x22Fh and 8:15 bits into register 0x22Eh. So when dpcd_read reads
> these two registers,
> it stores LSB and MSB in the swapped manner in a 16 bit variable.
> Refer to Table 3-4 of VESA DP Link layer CTS spec version 1.2
Would the byte swap above still be necessary in a big-endian architecture?
While the snippet I wrote above is wrong, this is still an endianness issue. The
MSB is at the lowest address, which means the value is big-endian. And x86 is
little endian. So that should have been:
__be16 h_width;
drm_dp_dpcd_read(..., &h_width, sizeof h_width);
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"
> > > + "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.
> Will do
> >
> >
> > >
> > > + 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.
> I feel this should be a standalone patch that adds the test handler for
> video pattern generation and the force_bpc can be in a separate patch.
> However I will go ahead and change the commit message for the bpc patch
> to indicate that it gets set by video pattern CTS tests.
It's a better practice to include code where its used. It makes review easier
since things are in context and it also helps with bisectability. If a bisect
result would point to this patch, but the error would be in the code added by
the previous one, that wouldn't be immediately obvious. Since this patch is
fairly small, I don't think the split makes sense.
Ander
>
>
> >
> >
> > >
> > > + 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
> I agree, i will separate this out into a different patch.
> >
> >
> > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> >
> > >
> > >
> > > #define DP_TEST_CRC_R_CR 0x240
> > > #define DP_TEST_CRC_G_Y 0x242
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
