Re: [Intel-gfx] [PATCH v8] drm/i915: implement EXTENDED_RECEIVER_CAPABILITY_FIELD_PRESENT
On Thu, Nov 29, 2018 at 11:43:18AM -0800, Manasi Navare wrote:
> On Thu, Nov 29, 2018 at 11:28:34AM -0800, Rodrigo Vivi wrote:
> > On Thu, Nov 29, 2018 at 10:34:05AM -0800, 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)
> > > v8 (From Manasi):
> >
> > it seems you should have signed-off this commit.
>
> Oh yes will do
>
> >
> > > * Get rid of superfluous debug prints (Jani N)
> > > * Print entire base DPCD before memcpy (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
> >
> > This changed a lot that I'm not sure my rv-b should remain ;)
>
> Yes agree, i will remove that
>
> >
> > > ---
> > > drivers/gpu/drm/i915/intel_dp.c | 35 +
> > > 1 file changed, 35 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index 70ae3d57316b..5698441abe47 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -3802,6 +3802,39 @@ 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;
> > > +
> > > + 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;
> > > + }
> >
> > no strong feeling, just yet-another bikesheding:
> > but the mix with blank lines and no blank lines is at least strange to my
> > OCD.
>
> Guess the newlines are just residues from spins, will make it uniform
>
> >
> > > + if (!memcmp(intel_dp->dpcd, dpcd_ext, sizeof(dpcd_ext)))
> > > + return;
> > > +
> >
> > > + DRM_DEBUG_KMS("Base DPCD: %*ph\n",
> > > + (int)sizeof(intel_dp->dpcd), intel_dp->dpcd);
> > > + memcpy(intel_dp->dpcd, dpcd_ext, sizeof(dpcd_ext));
> >
> > hm... I'm afraid the full copy might be dangerous.
>
> We are not doing full copy here since the size of dpcd_ext is only 6 bytes.
> We are only restricting it from 0x2200 to 0x2205
>
> And the reason we are doing all 6 is because memcmp returned that something
> changed.
>
> Manasi
>
> >
> > 0x2215 to 0x22FF is
> > "RESERVED for Extended Receiver V Capability Read all 0s"
> >
> > On this range of the original one we have
> > 0x0021 which contains DP_MST_CAP bit that we use in our code.
> >
> > This full copy seems to have the potential to break MST if not
> > more cases.
> >
> > I prefer the individual copy of the bits that we know that we need.
> >
> > but if you prefer the full copy it seems that it would be better
> > to limit
Re: [Intel-gfx] [PATCH v8] drm/i915: implement EXTENDED_RECEIVER_CAPABILITY_FIELD_PRESENT
On Thu, Nov 29, 2018 at 11:28:34AM -0800, Rodrigo Vivi wrote:
> On Thu, Nov 29, 2018 at 10:34:05AM -0800, 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)
> > v8 (From Manasi):
>
> it seems you should have signed-off this commit.
Oh yes will do
>
> > * Get rid of superfluous debug prints (Jani N)
> > * Print entire base DPCD before memcpy (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
>
> This changed a lot that I'm not sure my rv-b should remain ;)
Yes agree, i will remove that
>
> > ---
> > drivers/gpu/drm/i915/intel_dp.c | 35 +
> > 1 file changed, 35 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 70ae3d57316b..5698441abe47 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3802,6 +3802,39 @@ 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;
> > +
> > + 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;
> > + }
>
> no strong feeling, just yet-another bikesheding:
> but the mix with blank lines and no blank lines is at least strange to my OCD.
Guess the newlines are just residues from spins, will make it uniform
>
> > + if (!memcmp(intel_dp->dpcd, dpcd_ext, sizeof(dpcd_ext)))
> > + return;
> > +
>
> > + DRM_DEBUG_KMS("Base DPCD: %*ph\n",
> > + (int)sizeof(intel_dp->dpcd), intel_dp->dpcd);
> > + memcpy(intel_dp->dpcd, dpcd_ext, sizeof(dpcd_ext));
>
> hm... I'm afraid the full copy might be dangerous.
We are not doing full copy here since the size of dpcd_ext is only 6 bytes.
We are only restricting it from 0x2200 to 0x2205
And the reason we are doing all 6 is because memcmp returned that something
changed.
Manasi
>
> 0x2215 to 0x22FF is
> "RESERVED for Extended Receiver V Capability Read all 0s"
>
> On this range of the original one we have
> 0x0021 which contains DP_MST_CAP bit that we use in our code.
>
> This full copy seems to have the potential to break MST if not
> more cases.
>
> I prefer the individual copy of the bits that we know that we need.
>
> but if you prefer the full copy it seems that it would be better
> to limit memcopy to 0x0015
>
> Thanks,
> Rodrigo.
>
> > +}
> > +
> > bool
> > intel_dp_read_dpcd(struct intel_dp *intel_dp)
> > {
> > @@ -3809,6 +3842,8 @@ intel_dp_read_dpcd(struct intel_dp *intel_dp)
> > sizeof(intel_dp->dpcd)) < 0)
> > return
Re: [Intel-gfx] [PATCH v8] drm/i915: implement EXTENDED_RECEIVER_CAPABILITY_FIELD_PRESENT
On Thu, Nov 29, 2018 at 10:34:05AM -0800, 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)
> v8 (From Manasi):
it seems you should have signed-off this commit.
> * Get rid of superfluous debug prints (Jani N)
> * Print entire base DPCD before memcpy (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
This changed a lot that I'm not sure my rv-b should remain ;)
> ---
> drivers/gpu/drm/i915/intel_dp.c | 35 +
> 1 file changed, 35 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 70ae3d57316b..5698441abe47 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3802,6 +3802,39 @@ 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;
> +
> + 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;
> + }
no strong feeling, just yet-another bikesheding:
but the mix with blank lines and no blank lines is at least strange to my OCD.
> + if (!memcmp(intel_dp->dpcd, dpcd_ext, sizeof(dpcd_ext)))
> + return;
> +
> + DRM_DEBUG_KMS("Base DPCD: %*ph\n",
> + (int)sizeof(intel_dp->dpcd), intel_dp->dpcd);
> + memcpy(intel_dp->dpcd, dpcd_ext, sizeof(dpcd_ext));
hm... I'm afraid the full copy might be dangerous.
0x2215 to 0x22FF is
"RESERVED for Extended Receiver V Capability Read all 0s"
On this range of the original one we have
0x0021 which contains DP_MST_CAP bit that we use in our code.
This full copy seems to have the potential to break MST if not
more cases.
I prefer the individual copy of the bits that we know that we need.
but if you prefer the full copy it seems that it would be better
to limit memcopy to 0x0015
Thanks,
Rodrigo.
> +}
> +
> bool
> intel_dp_read_dpcd(struct intel_dp *intel_dp)
> {
> @@ -3809,6 +3842,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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v8] 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)
v8 (From Manasi):
* Get rid of superfluous debug prints (Jani N)
* Print entire base DPCD before memcpy (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 | 35 +
1 file changed, 35 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 70ae3d57316b..5698441abe47 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3802,6 +3802,39 @@ 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;
+
+ 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)))
+ return;
+
+ DRM_DEBUG_KMS("Base DPCD: %*ph\n",
+ (int)sizeof(intel_dp->dpcd), intel_dp->dpcd);
+ memcpy(intel_dp->dpcd, dpcd_ext, sizeof(dpcd_ext));
+}
+
bool
intel_dp_read_dpcd(struct intel_dp *intel_dp)
{
@@ -3809,6 +3842,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
