| Message ID | 20181127205132.1001-1-manasi.d.navare@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v7] drm/i915: implement EXTENDED_RECEIVER_CAPABILITY_FIELD_PRESENT | expand |
On Tue, 27 Nov 2018, Manasi Navare <manasi.d.navare@intel.com> wrote: > From: Matt Atwood <matthew.s.atwood@intel.com> > > According to DP spec (2.9.3.1 of DP 1.4) if > EXTENDED_RECEIVER_CAPABILITY_FIELD_PRESENT is set the addresses in DPCD > 02200h through 0220Fh shall contain the DPRX's true capability. These > values will match 00000h through 0000Fh, except for DPCD_REV, > MAX_LINK_RATE, DOWN_STREAM_PORT_PRESENT. > > Read from DPCD once for all 3 values as this is an expensive operation. > Spec mentions that all of address space 02200h through 0220Fh should > contain the right information however currently only 3 values can > differ. > > There is no address space in the intel_dp->dpcd struct for addresses > 02200h through 0220Fh, and since so much of the data is a identical, > simply overwrite the values stored in 00000h through 0000Fh with the > values that can be overwritten from addresses 02200h through 0220Fh. > > This patch helps with backward compatibility for devices pre DP1.3. > > v2: read only dpcd values which can be affected, remove incorrect check, > split into drm include changes into separate patch, commit message, > verbose debugging statements during overwrite. > v3: white space fixes > v4: make path dependent on DPCD revision > 1.2 > v5: split into function, removed DPCD rev check > v6: add debugging prints for early exit conditions > v7 (From Manasi): > * Memcpy, memcmp and debig logging based on sizeof(dpcd_ext) (Jani N) > * Exit early (Jani N) > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com> > Tested-by: Manasi Navare <manasi.d.navare@intel.com> > Acked-by: Manasi Navare <manasi.d.navare@intel.com> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 41 +++++++++++++++++++++++++++++++++ > 1 file changed, 41 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 70ae3d57316b..a9eb14a4ab27 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3802,6 +3802,45 @@ intel_dp_link_down(struct intel_encoder *encoder, > } > } > > +static void > +intel_dp_extended_receiver_capabilities(struct intel_dp *intel_dp) > +{ > + u8 dpcd_ext[6]; > + > + /* > + * Prior to DP1.3 the bit represented by > + * DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT was reserved. > + * if it is set DP_DPCD_REV at 0000h could be at a value less than > + * the true capability of the panel. The only way to check is to > + * then compare 0000h and 2200h. > + */ > + if (!(intel_dp->dpcd[DP_TRAINING_AUX_RD_INTERVAL] & > + DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT)) > + return; > + > + DRM_DEBUG_KMS("DPCD: Reading extended receiver capabilities\n"); Superfluous debug logging. > + > + if (drm_dp_dpcd_read(&intel_dp->aux, DP_DP13_DPCD_REV, > + &dpcd_ext, sizeof(dpcd_ext)) != sizeof(dpcd_ext)) { > + DRM_ERROR("DPCD failed read at extended capabilities\n"); Most of our dpcd failures are logged using DRM_DEBUG_KMS. The ones that log DRM_ERROR seem to be very recent additions deviating from the debug loggin practice. There isn't much the user can do, really. > + return; > + } > + if (intel_dp->dpcd[DP_DPCD_REV] > dpcd_ext[DP_DPCD_REV]) { > + DRM_DEBUG_KMS("DPCD extended DPCD rev less than base DPCD rev\n"); Okay, seems like a rare event. > + return; > + } > + if (!memcmp(intel_dp->dpcd, dpcd_ext, sizeof(dpcd_ext))) { > + DRM_DEBUG_KMS("Extended Receiver Cap DPCD match the base DPCD\n"); I don't think this debug logging is needed. > + return; > + } > + > + DRM_DEBUG_KMS("Base DPCD: %*ph\n", (int)sizeof(dpcd_ext), intel_dp->dpcd); Using sizeof(dpcd_ext) when printing something else is a red flag. You could log the whole dpcd here. DRM_DEBUG_KMS("DPCD: %*ph (base)\n", (int) sizeof(intel_dp->dpcd), intel_dp->dpcd); > + DRM_DEBUG_KMS("Extended Receiver Cap DPCD: %*ph\n", > + (int)sizeof(dpcd_ext), dpcd_ext); The caller will log the *updated* DPCD right after this returns, you don't need to log dpcd_ext. > + memcpy(intel_dp->dpcd, dpcd_ext, sizeof(dpcd_ext)); > +} > + > + Superfluous newline. > bool > intel_dp_read_dpcd(struct intel_dp *intel_dp) > { > @@ -3809,6 +3848,8 @@ intel_dp_read_dpcd(struct intel_dp *intel_dp) > sizeof(intel_dp->dpcd)) < 0) > return false; /* aux transfer failed */ > > + intel_dp_extended_receiver_capabilities(intel_dp); > + > DRM_DEBUG_KMS("DPCD: %*ph\n", (int) sizeof(intel_dp->dpcd), intel_dp->dpcd); One other alternative is to have intel_dp_extended_receiver_capabilities() return true if the cap exists and is different from current DPCD, and *all* DPCD logging would be done here. Both the old and the new. Like so: DRM_DEBUG_KMS("DPCD: %*ph\n", (int) sizeof(intel_dp->dpcd), intel_dp->dpcd); if (intel_dp_extended_receiver_capabilities(intel_dp)) DRM_DEBUG_KMS("DPCD: %*ph\n (ext)", (int) sizeof(intel_dp->dpcd), intel_dp->dpcd); BR, Jani. > > return intel_dp->dpcd[DP_DPCD_REV] != 0;
On Wed, Nov 28, 2018 at 11:09:46AM +0200, Jani Nikula wrote: > On Tue, 27 Nov 2018, Manasi Navare <manasi.d.navare@intel.com> wrote: > > From: Matt Atwood <matthew.s.atwood@intel.com> > > > > According to DP spec (2.9.3.1 of DP 1.4) if > > EXTENDED_RECEIVER_CAPABILITY_FIELD_PRESENT is set the addresses in DPCD > > 02200h through 0220Fh shall contain the DPRX's true capability. These > > values will match 00000h through 0000Fh, except for DPCD_REV, > > MAX_LINK_RATE, DOWN_STREAM_PORT_PRESENT. > > > > Read from DPCD once for all 3 values as this is an expensive operation. > > Spec mentions that all of address space 02200h through 0220Fh should > > contain the right information however currently only 3 values can > > differ. > > > > There is no address space in the intel_dp->dpcd struct for addresses > > 02200h through 0220Fh, and since so much of the data is a identical, > > simply overwrite the values stored in 00000h through 0000Fh with the > > values that can be overwritten from addresses 02200h through 0220Fh. > > > > This patch helps with backward compatibility for devices pre DP1.3. > > > > v2: read only dpcd values which can be affected, remove incorrect check, > > split into drm include changes into separate patch, commit message, > > verbose debugging statements during overwrite. > > v3: white space fixes > > v4: make path dependent on DPCD revision > 1.2 > > v5: split into function, removed DPCD rev check > > v6: add debugging prints for early exit conditions > > v7 (From Manasi): > > * Memcpy, memcmp and debig logging based on sizeof(dpcd_ext) (Jani N) > > * Exit early (Jani N) > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com> > > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com> > > Tested-by: Manasi Navare <manasi.d.navare@intel.com> > > Acked-by: Manasi Navare <manasi.d.navare@intel.com> > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 41 +++++++++++++++++++++++++++++++++ > > 1 file changed, 41 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 70ae3d57316b..a9eb14a4ab27 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -3802,6 +3802,45 @@ intel_dp_link_down(struct intel_encoder *encoder, > > } > > } > > > > +static void > > +intel_dp_extended_receiver_capabilities(struct intel_dp *intel_dp) > > +{ > > + u8 dpcd_ext[6]; > > + > > + /* > > + * Prior to DP1.3 the bit represented by > > + * DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT was reserved. > > + * if it is set DP_DPCD_REV at 0000h could be at a value less than > > + * the true capability of the panel. The only way to check is to > > + * then compare 0000h and 2200h. > > + */ > > + if (!(intel_dp->dpcd[DP_TRAINING_AUX_RD_INTERVAL] & > > + DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT)) > > + return; > > + > > + DRM_DEBUG_KMS("DPCD: Reading extended receiver capabilities\n"); > > Superfluous debug logging. Will get rid of this > > > + > > + if (drm_dp_dpcd_read(&intel_dp->aux, DP_DP13_DPCD_REV, > > + &dpcd_ext, sizeof(dpcd_ext)) != sizeof(dpcd_ext)) { > > + DRM_ERROR("DPCD failed read at extended capabilities\n"); > > Most of our dpcd failures are logged using DRM_DEBUG_KMS. The ones that > log DRM_ERROR seem to be very recent additions deviating from the debug > loggin practice. There isn't much the user can do, really. Here this change from DEBUG_KMS to DRM_ERROR was as per Rodrigo's comment on the initial patch. (https://patchwork.freedesktop.org/patch/240452/) Also IMO it should be an error since it will give unexpected results as we were unable to get the true extended capabilities. > > > + return; > > + } > > + if (intel_dp->dpcd[DP_DPCD_REV] > dpcd_ext[DP_DPCD_REV]) { > > + DRM_DEBUG_KMS("DPCD extended DPCD rev less than base DPCD rev\n"); > > Okay, seems like a rare event. Again this check and logging comes from Rodrigo's review comments on the initial patch. https://patchwork.freedesktop.org/patch/240452/ > > > + return; > > + } > > + if (!memcmp(intel_dp->dpcd, dpcd_ext, sizeof(dpcd_ext))) { > > + DRM_DEBUG_KMS("Extended Receiver Cap DPCD match the base DPCD\n"); > > I don't think this debug logging is needed. Sure will get rid of this. > > > + return; > > + } > > + > > + DRM_DEBUG_KMS("Base DPCD: %*ph\n", (int)sizeof(dpcd_ext), intel_dp->dpcd); > > Using sizeof(dpcd_ext) when printing something else is a red flag. You > could log the whole dpcd here. > > DRM_DEBUG_KMS("DPCD: %*ph (base)\n", (int) sizeof(intel_dp->dpcd), intel_dp->dpcd); Yes will do this. > > > + DRM_DEBUG_KMS("Extended Receiver Cap DPCD: %*ph\n", > > + (int)sizeof(dpcd_ext), dpcd_ext); > > The caller will log the *updated* DPCD right after this returns, you > don't need to log dpcd_ext. Okay agreed, I will get rid of this debug print since the caller already prints it. > > > + memcpy(intel_dp->dpcd, dpcd_ext, sizeof(dpcd_ext)); > > +} > > + > > + > > Superfluous newline. Will remove. > > > bool > > intel_dp_read_dpcd(struct intel_dp *intel_dp) > > { > > @@ -3809,6 +3848,8 @@ intel_dp_read_dpcd(struct intel_dp *intel_dp) > > sizeof(intel_dp->dpcd)) < 0) > > return false; /* aux transfer failed */ > > > > + intel_dp_extended_receiver_capabilities(intel_dp); > > + > > DRM_DEBUG_KMS("DPCD: %*ph\n", (int) sizeof(intel_dp->dpcd), intel_dp->dpcd); > > One other alternative is to have > intel_dp_extended_receiver_capabilities() return true if the cap exists > and is different from current DPCD, and *all* DPCD logging would be done > here. Both the old and the new. Like so: > > DRM_DEBUG_KMS("DPCD: %*ph\n", (int) sizeof(intel_dp->dpcd), intel_dp->dpcd); > if (intel_dp_extended_receiver_capabilities(intel_dp)) > DRM_DEBUG_KMS("DPCD: %*ph\n (ext)", (int) sizeof(intel_dp->dpcd), intel_dp->dpcd); > I like the firts idea better where we print the old DPCD before memcpy and caller prints the DPCD (new/old) anyways after return so will stick to that. Manasi > BR, > Jani. > > > > > return intel_dp->dpcd[DP_DPCD_REV] != 0; > > -- > Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 70ae3d57316b..a9eb14a4ab27 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3802,6 +3802,45 @@ intel_dp_link_down(struct intel_encoder *encoder, } } +static void +intel_dp_extended_receiver_capabilities(struct intel_dp *intel_dp) +{ + u8 dpcd_ext[6]; + + /* + * Prior to DP1.3 the bit represented by + * DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT was reserved. + * if it is set DP_DPCD_REV at 0000h could be at a value less than + * the true capability of the panel. The only way to check is to + * then compare 0000h and 2200h. + */ + if (!(intel_dp->dpcd[DP_TRAINING_AUX_RD_INTERVAL] & + DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT)) + return; + + DRM_DEBUG_KMS("DPCD: Reading extended receiver capabilities\n"); + + if (drm_dp_dpcd_read(&intel_dp->aux, DP_DP13_DPCD_REV, + &dpcd_ext, sizeof(dpcd_ext)) != sizeof(dpcd_ext)) { + DRM_ERROR("DPCD failed read at extended capabilities\n"); + return; + } + if (intel_dp->dpcd[DP_DPCD_REV] > dpcd_ext[DP_DPCD_REV]) { + DRM_DEBUG_KMS("DPCD extended DPCD rev less than base DPCD rev\n"); + return; + } + if (!memcmp(intel_dp->dpcd, dpcd_ext, sizeof(dpcd_ext))) { + DRM_DEBUG_KMS("Extended Receiver Cap DPCD match the base DPCD\n"); + return; + } + + DRM_DEBUG_KMS("Base DPCD: %*ph\n", (int)sizeof(dpcd_ext), intel_dp->dpcd); + DRM_DEBUG_KMS("Extended Receiver Cap DPCD: %*ph\n", + (int)sizeof(dpcd_ext), dpcd_ext); + memcpy(intel_dp->dpcd, dpcd_ext, sizeof(dpcd_ext)); +} + + bool intel_dp_read_dpcd(struct intel_dp *intel_dp) { @@ -3809,6 +3848,8 @@ intel_dp_read_dpcd(struct intel_dp *intel_dp) sizeof(intel_dp->dpcd)) < 0) return false; /* aux transfer failed */ + intel_dp_extended_receiver_capabilities(intel_dp); + DRM_DEBUG_KMS("DPCD: %*ph\n", (int) sizeof(intel_dp->dpcd), intel_dp->dpcd); return intel_dp->dpcd[DP_DPCD_REV] != 0;