Re: [PATCH 11/17] drm/i915: Update the EDID automated compliance test function

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Dec 17, 2014 at 02:25:42PM -0200, Paulo Zanoni wrote:
> 2014-12-10 21:53 GMT-02:00 Todd Previte <tprevite@gmail.com>:
> > 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 are handed
> > off to userspace so that the remainder of the test can be conducted there.
> >
> > 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
> >
> > Signed-off-by: Todd Previte <tprevite@gmail.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 72 ++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 71 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index b6f5a72..2a13124 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -40,6 +40,13 @@
> >
> >  #define DP_LINK_CHECK_TIMEOUT  (10 * 1000)
> >
> > +/* Compliance test status bits  */
> > +#define  INTEL_DP_EDID_OK              (0<<0)
> > +#define  INTEL_DP_EDID_CORRUPT         (1<<0)
> > +#define  INTEL_DP_RESOLUTION_PREFERRED (1<<2)
> > +#define  INTEL_DP_RESOLUTION_STANDARD  (1<<3)
> > +#define  INTEL_DP_RESOLUTION_FAILSAFE  (1<<4)
> > +
> >  struct dp_link_dpll {
> >         int link_bw;
> >         struct dpll dpll;
> > @@ -3761,9 +3768,72 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
> >         return test_result;
> >  }
> >
> > +static bool intel_dp_compute_edid_checksum(uint8_t *edid_data,
> > +                                          uint8_t *edid_checksum)
> > +{
> > +       uint32_t byte_total = 0;
> > +       uint8_t i = 0;
> > +       bool edid_ok = true;
> > +
> > +       /* Don't include last byte (the checksum) in the computation */
> > +       for (i = 0; i < EDID_LENGTH - 2; i++)
> 
> Shouldn't this be "i < EDID_LENGHT - 1"?
> 
> 
> > +               byte_total += edid_data[i];
> > +
> > +       *edid_checksum = 256 - (byte_total % 256);
> > +
> > +       if (*edid_checksum != edid_data[EDID_LENGTH - 1]) {
> > +               DRM_DEBUG_KMS("Invalid EDID checksum %02x, should be %02x\n",
> > +                             edid_data[EDID_LENGTH - 40 - 1], *edid_checksum);
> > +               edid_ok = false;
> > +       }
> > +
> > +       return edid_ok;
> > +}
> > +
> >  static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
> >  {
> > -       uint8_t test_result = DP_TEST_NAK;
> > +       struct drm_connector *connector = &intel_dp->attached_connector->base;
> > +       struct i2c_adapter *adapter = &intel_dp->aux.ddc;
> > +       struct edid *edid_read = NULL;
> > +       uint8_t *edid_data = NULL;
> > +       uint8_t test_result = DP_TEST_NAK, checksum = 0;
> > +       uint32_t ret = 0;
> > +
> > +       intel_dp->aux.i2c_nack_count = 0;
> > +       intel_dp->aux.i2c_defer_count = 0;
> > +
> > +       edid_read = drm_get_edid(connector, adapter);
> > +
> > +       if (edid_read == NULL) {
> > +               /* 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);
> 
> Don't we need to use these _count values somehow, instead of just
> printing them in the logs?
> 
> Everything else looks fine.
> 
> > +               intel_dp->compliance_test_data = INTEL_DP_EDID_CORRUPT |
> > +                                                INTEL_DP_RESOLUTION_FAILSAFE;
> > +       } else {
> > +               edid_data = (uint8_t *) edid_read;
> > +
> > +               if (intel_dp_compute_edid_checksum(edid_data, &checksum)) {
> > +                       ret = drm_dp_dpcd_write(&intel_dp->aux,
> > +                                               DP_TEST_EDID_CHECKSUM,
> > +                                               &edid_read->checksum, 1);
> > +                       test_result = DP_TEST_ACK |
> > +                                     DP_TEST_EDID_CHECKSUM_WRITE;
> > +                       intel_dp->compliance_test_data =
> > +                               INTEL_DP_EDID_OK |
> > +                               INTEL_DP_RESOLUTION_PREFERRED;
> > +               } else {
> > +                       /* Invalid checksum - EDID corruption detection */
> > +                       intel_dp->compliance_test_data =
> > +                               INTEL_DP_EDID_CORRUPT |
> > +                               INTEL_DP_RESOLUTION_FAILSAFE;

Just something random I've spotted while driving by: drm_get_edid does all
the checksum stuff for you already (it retries up to 4 times if the
checkusm is off and also checks a few other things). We should never reach
this case and the checksum function is essentially dead code.

Or do I miss something?
-Daniel

> > +               }
> > +       }
> > +
> >         return test_result;
> >  }
> >
> > --
> > 1.9.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx