| Message ID | 20180717214939.32260-2-matthew.s.atwood@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2018-07-17 at 14:49 -0700, matthew.s.atwood@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, I still see 6 bytes read and 3 copied. > remove incorrect check, > split into drm include changes into separate patch, commit message, > verbose debugging statements during overwrite. > > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 37 > +++++++++++++++++++++++++++++++++ > 1 file changed, 37 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > b/drivers/gpu/drm/i915/intel_dp.c > index dde92e4af5d3..364cf41a8b89 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3738,6 +3738,43 @@ intel_dp_read_dpcd(struct intel_dp *intel_dp) > sizeof(intel_dp->dpcd)) < 0) > return false; /* aux transfer failed */ > > + if (intel_dp->dpcd[DP_TRAINING_AUX_RD_INTERVAL] & > + DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT) { > + uint8_t dpcd_ext[6]; > + > + DRM_DEBUG_KMS("DPCD: Extended Receiver Capability > Field Present, accessing 02200h through 022FFh\n"); > + > + if (drm_dp_dpcd_read(&intel_dp->aux, > DP_DP13_DPCD_REV, > + &dpcd_ext, sizeof(dpcd_ext)) < 0) > + return false; /* aux transfer failed */ > + > + if (memcmp(&intel_dp->dpcd[DP_DPCD_REV], > &dpcd_ext[DP_DPCD_REV], > + sizeof(u8))) Why use memcmp and memcmpy if it's just one byte? You could just use "==" > { > + DRM_DEBUG_KMS("DPCD: new value for DPCD > Revision previous value %2x new value %2x\n", > + intel_dp->dpcd[DP_DPCD_REV], > + dpcd_ext[DP_DPCD_REV]); > + memcpy(&intel_dp->dpcd[DP_DPCD_REV], > + &dpcd_ext[DP_DPCD_REV], > + sizeof(u8)); > + } > + if (memcmp(&intel_dp->dpcd[DP_MAX_LINK_RATE], > + &dpcd_ext[DP_MAX_LINK_RATE], sizeof(u8))) > { > + DRM_DEBUG_KMS("DPCD: new value for DPCD Max > Link Rate previous value %2x new value %2x\n", > + intel_dp- > >dpcd[DP_MAX_LINK_RATE], > + dpcd_ext[DP_MAX_LINK_RATE]); > + memcpy(&intel_dp->dpcd[DP_MAX_LINK_RATE], > + &dpcd_ext[DP_MAX_LINK_RATE], > sizeof(u8)); > + } > + if (memcmp(&intel_dp- > >dpcd[DP_DOWNSTREAMPORT_PRESENT], > + &dpcd_ext[DP_DOWNSTREAMPORT_PRESENT], > sizeof(u8))) { > + DRM_DEBUG_KMS("DPCD: new value for DPCD > Downstream Port Present previous value %2x new value %2x\n", > + intel_dp- > >dpcd[DP_DOWNSTREAMPORT_PRESENT], > + dpcd_ext[DP_DOWNSTREAMPORT_PRE > SENT]); > + memcpy(&intel_dp- > >dpcd[DP_DOWNSTREAMPORT_PRESENT], > + &dpcd_ext[DP_DOWNSTREAMPORT_PRESENT], > + sizeof(u8)); > + } > + } > DRM_DEBUG_KMS("DPCD: %*ph\n", (int) sizeof(intel_dp->dpcd), > intel_dp->dpcd); > > return intel_dp->dpcd[DP_DPCD_REV] != 0;
On Tue, Jul 17, 2018 at 03:34:35PM -0700, Dhinakaran Pandiyan wrote: > On Tue, 2018-07-17 at 14:49 -0700, matthew.s.atwood@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, > > I still see 6 bytes read and 3 copied. +1 > > > remove incorrect check, > > split into drm include changes into separate patch, commit message, > > verbose debugging statements during overwrite. > > > > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 37 > > +++++++++++++++++++++++++++++++++ > > 1 file changed, 37 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > b/drivers/gpu/drm/i915/intel_dp.c > > index dde92e4af5d3..364cf41a8b89 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -3738,6 +3738,43 @@ intel_dp_read_dpcd(struct intel_dp *intel_dp) > > sizeof(intel_dp->dpcd)) < 0) > > return false; /* aux transfer failed */ > > > > + if (intel_dp->dpcd[DP_TRAINING_AUX_RD_INTERVAL] & > > + DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT) { > > + uint8_t dpcd_ext[6]; > > + > > + DRM_DEBUG_KMS("DPCD: Extended Receiver Capability > > Field Present, accessing 02200h through 022FFh\n"); > > + > > + if (drm_dp_dpcd_read(&intel_dp->aux, > > DP_DP13_DPCD_REV, > > + &dpcd_ext, sizeof(dpcd_ext)) < 0) > > + return false; /* aux transfer failed */ > > + > > + if (memcmp(&intel_dp->dpcd[DP_DPCD_REV], > > &dpcd_ext[DP_DPCD_REV], > > + sizeof(u8))) > > Why use memcmp and memcmpy if it's just one byte? You could just use > "==" I think == should work here, but why not memcmp anyways?! > > > { > > + DRM_DEBUG_KMS("DPCD: new value for DPCD > > Revision previous value %2x new value %2x\n", > > + intel_dp->dpcd[DP_DPCD_REV], > > + dpcd_ext[DP_DPCD_REV]); > > + memcpy(&intel_dp->dpcd[DP_DPCD_REV], > > + &dpcd_ext[DP_DPCD_REV], > > + sizeof(u8)); > > + } > > + if (memcmp(&intel_dp->dpcd[DP_MAX_LINK_RATE], > > + &dpcd_ext[DP_MAX_LINK_RATE], sizeof(u8))) > > { > > + DRM_DEBUG_KMS("DPCD: new value for DPCD Max > > Link Rate previous value %2x new value %2x\n", > > + intel_dp- > > >dpcd[DP_MAX_LINK_RATE], > > + dpcd_ext[DP_MAX_LINK_RATE]); > > + memcpy(&intel_dp->dpcd[DP_MAX_LINK_RATE], > > + &dpcd_ext[DP_MAX_LINK_RATE], > > sizeof(u8)); > > + } > > + if (memcmp(&intel_dp- > > >dpcd[DP_DOWNSTREAMPORT_PRESENT], > > + &dpcd_ext[DP_DOWNSTREAMPORT_PRESENT], > > sizeof(u8))) { > > + DRM_DEBUG_KMS("DPCD: new value for DPCD > > Downstream Port Present previous value %2x new value %2x\n", > > + intel_dp- > > >dpcd[DP_DOWNSTREAMPORT_PRESENT], > > + dpcd_ext[DP_DOWNSTREAMPORT_PRE > > SENT]); > > + memcpy(&intel_dp- > > >dpcd[DP_DOWNSTREAMPORT_PRESENT], > > + &dpcd_ext[DP_DOWNSTREAMPORT_PRESENT], > > + sizeof(u8)); > > + } > > + } > > DRM_DEBUG_KMS("DPCD: %*ph\n", (int) sizeof(intel_dp->dpcd), > > intel_dp->dpcd); > > > > return intel_dp->dpcd[DP_DPCD_REV] != 0; > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, 2018-07-17 at 15:34 -0700, Dhinakaran Pandiyan wrote: > On Tue, 2018-07-17 at 14:49 -0700, matthew.s.atwood@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, > I still see 6 bytes read and 3 copied. Ignore this, the original patch was reading 16B. Thanks for clarifying Matt. > > > > > remove incorrect check, > > split into drm include changes into separate patch, commit message, > > verbose debugging statements during overwrite. > > > > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 37 > > +++++++++++++++++++++++++++++++++ > > 1 file changed, 37 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > b/drivers/gpu/drm/i915/intel_dp.c > > index dde92e4af5d3..364cf41a8b89 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -3738,6 +3738,43 @@ intel_dp_read_dpcd(struct intel_dp > > *intel_dp) > > sizeof(intel_dp->dpcd)) < 0) > > return false; /* aux transfer failed */ > > > > + if (intel_dp->dpcd[DP_TRAINING_AUX_RD_INTERVAL] & > > + DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT) { > > + uint8_t dpcd_ext[6]; > > + > > + DRM_DEBUG_KMS("DPCD: Extended Receiver Capability > > Field Present, accessing 02200h through 022FFh\n"); > > + > > + if (drm_dp_dpcd_read(&intel_dp->aux, > > DP_DP13_DPCD_REV, > > + &dpcd_ext, sizeof(dpcd_ext)) < 0) > > + return false; /* aux transfer failed */ > > + > > + if (memcmp(&intel_dp->dpcd[DP_DPCD_REV], > > &dpcd_ext[DP_DPCD_REV], > > + sizeof(u8))) > Why use memcmp and memcmpy if it's just one byte? You could just use > "==" > I believe this is what Jani suggested. if (memcmp(old_dpcd, new_dpcd, sizeof(new_dpcd)) { DRM_DEBUG_KMS(); memcpy(old_dpcd, new_dpcd, sizeof(new_dpcd); } We lose the information about which specific fields in the 6 bytes changed, but that's okay IMO. > > > > { > > + DRM_DEBUG_KMS("DPCD: new value for DPCD > > Revision previous value %2x new value %2x\n", > > + intel_dp->dpcd[DP_DPCD_REV], > > + dpcd_ext[DP_DPCD_REV]); > > + memcpy(&intel_dp->dpcd[DP_DPCD_REV], > > + &dpcd_ext[DP_DPCD_REV], > > + sizeof(u8)); > > + } > > + if (memcmp(&intel_dp->dpcd[DP_MAX_LINK_RATE], > > + &dpcd_ext[DP_MAX_LINK_RATE], > > sizeof(u8))) > > { > > + DRM_DEBUG_KMS("DPCD: new value for DPCD > > Max > > Link Rate previous value %2x new value %2x\n", > > + intel_dp- > > > > > > dpcd[DP_MAX_LINK_RATE], > > + dpcd_ext[DP_MAX_LINK_RATE]); > > + memcpy(&intel_dp->dpcd[DP_MAX_LINK_RATE], > > + &dpcd_ext[DP_MAX_LINK_RATE], > > sizeof(u8)); > > + } > > + if (memcmp(&intel_dp- > > > > > > dpcd[DP_DOWNSTREAMPORT_PRESENT], > > + &dpcd_ext[DP_DOWNSTREAMPORT_PRESENT], > > sizeof(u8))) { > > + DRM_DEBUG_KMS("DPCD: new value for DPCD > > Downstream Port Present previous value %2x new value %2x\n", > > + intel_dp- > > > > > > dpcd[DP_DOWNSTREAMPORT_PRESENT], > > + dpcd_ext[DP_DOWNSTREAMPORT_P > > RE > > SENT]); > > + memcpy(&intel_dp- > > > > > > dpcd[DP_DOWNSTREAMPORT_PRESENT], > > + &dpcd_ext[DP_DOWNSTREAMPORT_PRESENT > > ], > > + sizeof(u8)); > > + } > > + } > > DRM_DEBUG_KMS("DPCD: %*ph\n", (int) sizeof(intel_dp- > > >dpcd), > > intel_dp->dpcd); > > > > return intel_dp->dpcd[DP_DPCD_REV] != 0;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index dde92e4af5d3..364cf41a8b89 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3738,6 +3738,43 @@ intel_dp_read_dpcd(struct intel_dp *intel_dp) sizeof(intel_dp->dpcd)) < 0) return false; /* aux transfer failed */ + if (intel_dp->dpcd[DP_TRAINING_AUX_RD_INTERVAL] & + DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT) { + uint8_t dpcd_ext[6]; + + DRM_DEBUG_KMS("DPCD: Extended Receiver Capability Field Present, accessing 02200h through 022FFh\n"); + + if (drm_dp_dpcd_read(&intel_dp->aux, DP_DP13_DPCD_REV, + &dpcd_ext, sizeof(dpcd_ext)) < 0) + return false; /* aux transfer failed */ + + if (memcmp(&intel_dp->dpcd[DP_DPCD_REV], &dpcd_ext[DP_DPCD_REV], + sizeof(u8))) { + DRM_DEBUG_KMS("DPCD: new value for DPCD Revision previous value %2x new value %2x\n", + intel_dp->dpcd[DP_DPCD_REV], + dpcd_ext[DP_DPCD_REV]); + memcpy(&intel_dp->dpcd[DP_DPCD_REV], + &dpcd_ext[DP_DPCD_REV], + sizeof(u8)); + } + if (memcmp(&intel_dp->dpcd[DP_MAX_LINK_RATE], + &dpcd_ext[DP_MAX_LINK_RATE], sizeof(u8))) { + DRM_DEBUG_KMS("DPCD: new value for DPCD Max Link Rate previous value %2x new value %2x\n", + intel_dp->dpcd[DP_MAX_LINK_RATE], + dpcd_ext[DP_MAX_LINK_RATE]); + memcpy(&intel_dp->dpcd[DP_MAX_LINK_RATE], + &dpcd_ext[DP_MAX_LINK_RATE], sizeof(u8)); + } + if (memcmp(&intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT], + &dpcd_ext[DP_DOWNSTREAMPORT_PRESENT], sizeof(u8))) { + DRM_DEBUG_KMS("DPCD: new value for DPCD Downstream Port Present previous value %2x new value %2x\n", + intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT], + dpcd_ext[DP_DOWNSTREAMPORT_PRESENT]); + memcpy(&intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT], + &dpcd_ext[DP_DOWNSTREAMPORT_PRESENT], + sizeof(u8)); + } + } DRM_DEBUG_KMS("DPCD: %*ph\n", (int) sizeof(intel_dp->dpcd), intel_dp->dpcd); return intel_dp->dpcd[DP_DPCD_REV] != 0;