On Mon, 04 May 2015, Daniel Vetter <daniel@xxxxxxxx> wrote:
> On Thu, Apr 30, 2015 at 03:30:10PM -0300, Paulo Zanoni wrote:
>> 2015-04-18 4:04 GMT-03:00 Todd Previte <tprevite@xxxxxxxxx>:
>> > Updates the EDID compliance test function to perform the analyze and react to
>> > the EDID data read as a result of a hot plug event. The results of this
>> > analysis are handed off to userspace so that the userspace app can set the
>> > display mode appropriately for the test result/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.
>> > V8:
>> > - Updated to support the kfree() on the EDID data added previously
>> > V9:
>> > - Updated for the long_hpd flag propagation
>> > V10:
>> > - Updated to use actual checksum from the EDID read that occurs during
>> > normal hot plug path execution
>> > - Removed variables from intel_dp struct that are no longer needed
>> > - Updated the patch subject to more closely match the nature and contents
>> > of the patch
>> >
>> > Signed-off-by: Todd Previte <tprevite@xxxxxxxxx>
>> > ---
>> > drivers/gpu/drm/i915/intel_dp.c | 41 ++++++++++++++++++++++++++++++++++++++++
>> > drivers/gpu/drm/i915/intel_drv.h | 2 ++
>> > 2 files changed, 43 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> > index 55d1f5f..4c1f6a9 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
>>
>> Now that you cleaned all the other stuff, can't we start at 0 instead
>> of 4? (of course, this would require a change to the user space tool)
>>
>>
>> > +#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;
>> > @@ -3994,6 +4000,38 @@ 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;
>> > + struct intel_connector *intel_connector = intel_dp->attached_connector;
>> > + struct drm_connector *connector = &intel_connector->base;
>> > +
>> > + if (intel_connector->detect_edid == NULL ||
>> > + connector->edid_corrupt == 1 ||
>> > + 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;
>> > + } else {
>> > + ret = drm_dp_dpcd_write(&intel_dp->aux,
>> > + DP_TEST_EDID_CHECKSUM,
>> > + &intel_connector->detect_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");
>>
>> You're mixing DRM_DEBUG_DRIVER and DRM_DEBUG_KMS. I'd stick with only
>> one, and I'd probably pick _KMS.
>>
>> With or without the above bikesheds: Reviewed-by: Paulo Zanoni
>> <paulo.r.zanoni@xxxxxxxxx>
>
> Yeah I second the two polish requests from Paulo. Merged the previous
> patch meanwhile.
I reverted this to unblock people because it breaks the build due to the
dependency on patch 2/5 which was apparently not merged yet. Sorry
Daniel, didn't get hold of you fast enough...
BR,
Jani.
> -Daniel
>
>>
>> > +
>> > + 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;
>> > }
>> >
>> > @@ -4009,7 +4047,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;
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> > index a4675fa..6c71be9 100644
>> > --- a/drivers/gpu/drm/i915/intel_drv.h
>> > +++ b/drivers/gpu/drm/i915/intel_drv.h
>> > @@ -742,6 +742,8 @@ struct intel_dp {
>> >
>> > /* Displayport compliance testing */
>> > unsigned long compliance_test_type;
>> > + unsigned long compliance_test_data;
>> > + bool compliance_test_active;
>> > };
>> >
>> > 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
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
