Re: [Intel-gfx] [PATCH v7] drm/i915: implement EXTENDED_RECEIVER_CAPABILITY_FIELD_PRESENT
On Wed, Nov 28, 2018 at 11:09:46AM +0200, Jani Nikula wrote:
> On Tue, 27 Nov 2018, Manasi Navare wrote:
> > From: Matt Atwood
> >
> > 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 0h through Fh, 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 0h through Fh 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
> > Cc: Ville Syrjala
> > Signed-off-by: Matt Atwood
> > Tested-by: Manasi Navare
> > Acked-by: Manasi Navare
> > Reviewed-by: Rodrigo Vivi
> > ---
> > 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 h could be at a value less than
> > +* the true capability of the panel. The only way to check is to
> > +* then compare h 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(_dp->aux, DP_DP13_DPCD_REV,
> > +_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));
> > +}
> > +
> > +
>
>
Re: [Intel-gfx] [PATCH v7] drm/i915: implement EXTENDED_RECEIVER_CAPABILITY_FIELD_PRESENT
On Tue, 27 Nov 2018, Manasi Navare wrote:
> From: Matt Atwood
>
> 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 0h through Fh, 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 0h through Fh 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
> Cc: Ville Syrjala
> Signed-off-by: Matt Atwood
> Tested-by: Manasi Navare
> Acked-by: Manasi Navare
> Reviewed-by: Rodrigo Vivi
> ---
> 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 h could be at a value less than
> + * the true capability of the panel. The only way to check is to
> + * then compare h 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(_dp->aux, DP_DP13_DPCD_REV,
> + _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))
[Intel-gfx] [PATCH v7] drm/i915: implement EXTENDED_RECEIVER_CAPABILITY_FIELD_PRESENT
From: Matt Atwood
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 0h through Fh, 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 0h through Fh 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
Cc: Ville Syrjala
Signed-off-by: Matt Atwood
Tested-by: Manasi Navare
Acked-by: Manasi Navare
Reviewed-by: Rodrigo Vivi
---
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 h could be at a value less than
+* the true capability of the panel. The only way to check is to
+* then compare h 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(_dp->aux, DP_DP13_DPCD_REV,
+_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;
--
2.19.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
