Re: [Intel-gfx] [PATCH 3/4] drm/i915: Cache sink_count for eDP
On Tue, 2018-10-09 at 16:27 +0300, Ville Syrjälä wrote:
> On Mon, Oct 08, 2018 at 05:54:17PM -0700, Dhinakaran Pandiyan wrote:
> > On Mon, 2018-10-08 at 17:35 -0700, Souza, Jose wrote:
> > > On Mon, 2018-10-08 at 17:19 -0700, Dhinakaran Pandiyan wrote:
> > > > On Fri, 2018-10-05 at 16:35 -0700, José Roberto de Souza wrote:
> > > > > For eDP panels all the DPCD and EDID data is cached when
> > > > > initializing
> > > > > the eDP connector so in futher detection it do not call
> > > > > intel_dp_detect_dpcd() for eDP.
> > > > > The problem is on the first short pulse interruption it calls
> > > > > intel_dp_get_dpcd() for eDP and DP and it will read and set
> > > > > the
> > > > > sink
> > > > > count, causing a mismatch between old sink count and the new
> > > > > one
> > > > > triggering a full detection without needed.
> > > > >
> > > > > Cc: Dhinakaran Pandiyan
> > > > > Signed-off-by: José Roberto de Souza
> > > > > ---
> > > > > drivers/gpu/drm/i915/intel_dp.c | 5 +
> > > > > 1 file changed, 5 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index 19f0c3f59cbe..4a1c31ec9065 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -3926,6 +3926,7 @@ intel_edp_init_dpcd(struct intel_dp
> > > > > *intel_dp)
> > > > > {
> > > > > struct drm_i915_private *dev_priv =
> > > > > to_i915(dp_to_dig_port(intel_dp)-
> > > > > >base.base.dev);
> > > > > + u8 val;
> > > > >
> > > > > /* this function is meant to be called only once */
> > > > > WARN_ON(intel_dp->dpcd[DP_DPCD_REV] != 0);
> > > > > @@ -3997,6 +3998,10 @@ intel_edp_init_dpcd(struct intel_dp
> > > > > *intel_dp)
> > > > >
> > > > > intel_dp_set_common_rates(intel_dp);
> > > > >
> > > > > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT,
> > > > > &val) <=
> > > > > 0)
> > > > > + return false;
> > > > > + intel_dp->sink_count = DP_GET_SINK_COUNT(val);
> > > >
> > > > Is this even relevant for eDPs? Seems unnecessary to read or
> > > > compare
> > > > sink count for eDP. I'd suggest skipping DP_SINK_COUNT checks
> > > > for
> > > > eDP.
> > >
> > > I'm not sure as DP specs for DP_SINK_COUNT says:
> > >
> > > The Sink device shall add one more if it has a local Rendering
> > > Function.
> > >
> > > and eDP spec do not redefine or alter this, so I guess is more
> > > safe
> > > also read for eDP too.
> > >
> >
> > We already special case eDP in several places, for example, don't
> > update link rates from the short pulse handler etc. And also don't
> > support hotplug, I don't see a point.
>
> IIRC some conformance test or something required that we read this.
> I guess what we could do is still read it but just not update
> intel_dp->sink_count. We already seem to have a special case which
> ignores a zero sink_count on eDP. Might as well extend that a bit
> I suppose.
Okay, I will skip the comparison for eDP.
>
> In general I think special cases are bad, so IMO we should try
> hard not add more unless really necessary. In this case it seems
> the special case is warranted. Unfortunately commit 1034ce707b57
> ("drm/i915: Fixing eDP detection on certain platforms") failed to add
> a comment explaining why. I'd appreciate if someone could add that
> comment now so that we don't forget this in the future.
Sure, I will add this comment.
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm/i915: Cache sink_count for eDP
On Mon, Oct 08, 2018 at 05:54:17PM -0700, Dhinakaran Pandiyan wrote:
> On Mon, 2018-10-08 at 17:35 -0700, Souza, Jose wrote:
> > On Mon, 2018-10-08 at 17:19 -0700, Dhinakaran Pandiyan wrote:
> > > On Fri, 2018-10-05 at 16:35 -0700, José Roberto de Souza wrote:
> > > > For eDP panels all the DPCD and EDID data is cached when
> > > > initializing
> > > > the eDP connector so in futher detection it do not call
> > > > intel_dp_detect_dpcd() for eDP.
> > > > The problem is on the first short pulse interruption it calls
> > > > intel_dp_get_dpcd() for eDP and DP and it will read and set the
> > > > sink
> > > > count, causing a mismatch between old sink count and the new one
> > > > triggering a full detection without needed.
> > > >
> > > > Cc: Dhinakaran Pandiyan
> > > > Signed-off-by: José Roberto de Souza
> > > > ---
> > > > drivers/gpu/drm/i915/intel_dp.c | 5 +
> > > > 1 file changed, 5 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 19f0c3f59cbe..4a1c31ec9065 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -3926,6 +3926,7 @@ intel_edp_init_dpcd(struct intel_dp
> > > > *intel_dp)
> > > > {
> > > > struct drm_i915_private *dev_priv =
> > > > to_i915(dp_to_dig_port(intel_dp)->base.base.dev);
> > > > + u8 val;
> > > >
> > > > /* this function is meant to be called only once */
> > > > WARN_ON(intel_dp->dpcd[DP_DPCD_REV] != 0);
> > > > @@ -3997,6 +3998,10 @@ intel_edp_init_dpcd(struct intel_dp
> > > > *intel_dp)
> > > >
> > > > intel_dp_set_common_rates(intel_dp);
> > > >
> > > > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &val) <=
> > > > 0)
> > > > + return false;
> > > > + intel_dp->sink_count = DP_GET_SINK_COUNT(val);
> > >
> > > Is this even relevant for eDPs? Seems unnecessary to read or
> > > compare
> > > sink count for eDP. I'd suggest skipping DP_SINK_COUNT checks for
> > > eDP.
> >
> > I'm not sure as DP specs for DP_SINK_COUNT says:
> >
> > The Sink device shall add one more if it has a local Rendering
> > Function.
> >
> > and eDP spec do not redefine or alter this, so I guess is more safe
> > also read for eDP too.
> >
>
> We already special case eDP in several places, for example, don't
> update link rates from the short pulse handler etc. And also don't
> support hotplug, I don't see a point.
IIRC some conformance test or something required that we read this.
I guess what we could do is still read it but just not update
intel_dp->sink_count. We already seem to have a special case which
ignores a zero sink_count on eDP. Might as well extend that a bit
I suppose.
In general I think special cases are bad, so IMO we should try
hard not add more unless really necessary. In this case it seems
the special case is warranted. Unfortunately commit 1034ce707b57
("drm/i915: Fixing eDP detection on certain platforms") failed to add
a comment explaining why. I'd appreciate if someone could add that
comment now so that we don't forget this in the future.
--
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm/i915: Cache sink_count for eDP
On Mon, 2018-10-08 at 17:35 -0700, Souza, Jose wrote:
> On Mon, 2018-10-08 at 17:19 -0700, Dhinakaran Pandiyan wrote:
> > On Fri, 2018-10-05 at 16:35 -0700, José Roberto de Souza wrote:
> > > For eDP panels all the DPCD and EDID data is cached when
> > > initializing
> > > the eDP connector so in futher detection it do not call
> > > intel_dp_detect_dpcd() for eDP.
> > > The problem is on the first short pulse interruption it calls
> > > intel_dp_get_dpcd() for eDP and DP and it will read and set the
> > > sink
> > > count, causing a mismatch between old sink count and the new one
> > > triggering a full detection without needed.
> > >
> > > Cc: Dhinakaran Pandiyan
> > > Signed-off-by: José Roberto de Souza
> > > ---
> > > drivers/gpu/drm/i915/intel_dp.c | 5 +
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index 19f0c3f59cbe..4a1c31ec9065 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -3926,6 +3926,7 @@ intel_edp_init_dpcd(struct intel_dp
> > > *intel_dp)
> > > {
> > > struct drm_i915_private *dev_priv =
> > > to_i915(dp_to_dig_port(intel_dp)->base.base.dev);
> > > + u8 val;
> > >
> > > /* this function is meant to be called only once */
> > > WARN_ON(intel_dp->dpcd[DP_DPCD_REV] != 0);
> > > @@ -3997,6 +3998,10 @@ intel_edp_init_dpcd(struct intel_dp
> > > *intel_dp)
> > >
> > > intel_dp_set_common_rates(intel_dp);
> > >
> > > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &val) <=
> > > 0)
> > > + return false;
> > > + intel_dp->sink_count = DP_GET_SINK_COUNT(val);
> >
> > Is this even relevant for eDPs? Seems unnecessary to read or
> > compare
> > sink count for eDP. I'd suggest skipping DP_SINK_COUNT checks for
> > eDP.
>
> I'm not sure as DP specs for DP_SINK_COUNT says:
>
> The Sink device shall add one more if it has a local Rendering
> Function.
>
> and eDP spec do not redefine or alter this, so I guess is more safe
> also read for eDP too.
>
We already special case eDP in several places, for example, don't
update link rates from the short pulse handler etc. And also don't
support hotplug, I don't see a point.
-DK
>
> >
> >
> > -DK
> >
> > > +
> > > return true;
> > > }
> > >
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm/i915: Cache sink_count for eDP
On Mon, 2018-10-08 at 17:19 -0700, Dhinakaran Pandiyan wrote:
> On Fri, 2018-10-05 at 16:35 -0700, José Roberto de Souza wrote:
> > For eDP panels all the DPCD and EDID data is cached when
> > initializing
> > the eDP connector so in futher detection it do not call
> > intel_dp_detect_dpcd() for eDP.
> > The problem is on the first short pulse interruption it calls
> > intel_dp_get_dpcd() for eDP and DP and it will read and set the
> > sink
> > count, causing a mismatch between old sink count and the new one
> > triggering a full detection without needed.
> >
> > Cc: Dhinakaran Pandiyan
> > Signed-off-by: José Roberto de Souza
> > ---
> > drivers/gpu/drm/i915/intel_dp.c | 5 +
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 19f0c3f59cbe..4a1c31ec9065 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3926,6 +3926,7 @@ intel_edp_init_dpcd(struct intel_dp
> > *intel_dp)
> > {
> > struct drm_i915_private *dev_priv =
> > to_i915(dp_to_dig_port(intel_dp)->base.base.dev);
> > + u8 val;
> >
> > /* this function is meant to be called only once */
> > WARN_ON(intel_dp->dpcd[DP_DPCD_REV] != 0);
> > @@ -3997,6 +3998,10 @@ intel_edp_init_dpcd(struct intel_dp
> > *intel_dp)
> >
> > intel_dp_set_common_rates(intel_dp);
> >
> > + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &val) <=
> > 0)
> > + return false;
> > + intel_dp->sink_count = DP_GET_SINK_COUNT(val);
> Is this even relevant for eDPs? Seems unnecessary to read or compare
> sink count for eDP. I'd suggest skipping DP_SINK_COUNT checks for
> eDP.
I'm not sure as DP specs for DP_SINK_COUNT says:
The Sink device shall add one more if it has a local Rendering
Function.
and eDP spec do not redefine or alter this, so I guess is more safe
also read for eDP too.
>
>
> -DK
>
> > +
> > return true;
> > }
> >
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm/i915: Cache sink_count for eDP
On Fri, 2018-10-05 at 16:35 -0700, José Roberto de Souza wrote:
> For eDP panels all the DPCD and EDID data is cached when initializing
> the eDP connector so in futher detection it do not call
> intel_dp_detect_dpcd() for eDP.
> The problem is on the first short pulse interruption it calls
> intel_dp_get_dpcd() for eDP and DP and it will read and set the sink
> count, causing a mismatch between old sink count and the new one
> triggering a full detection without needed.
>
> Cc: Dhinakaran Pandiyan
> Signed-off-by: José Roberto de Souza
> ---
> drivers/gpu/drm/i915/intel_dp.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> index 19f0c3f59cbe..4a1c31ec9065 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3926,6 +3926,7 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
> {
> struct drm_i915_private *dev_priv =
> to_i915(dp_to_dig_port(intel_dp)->base.base.dev);
> + u8 val;
>
> /* this function is meant to be called only once */
> WARN_ON(intel_dp->dpcd[DP_DPCD_REV] != 0);
> @@ -3997,6 +3998,10 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
>
> intel_dp_set_common_rates(intel_dp);
>
> + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &val) <=
> 0)
> + return false;
> + intel_dp->sink_count = DP_GET_SINK_COUNT(val);
Is this even relevant for eDPs? Seems unnecessary to read or compare
sink count for eDP. I'd suggest skipping DP_SINK_COUNT checks for eDP.
-DK
> +
> return true;
> }
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
