| Message ID | 20181011004124.4110-5-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/6] drm/i915/psr: Use intel_psr_exit() in intel_psr_disable_source() | expand |
On Wed, Oct 10, 2018 at 05:41:23PM -0700, José Roberto de Souza wrote: > Some eDP panels do not set a valid sink count value and even for the > ones that sets is should always be one for eDP, that is why it is not > cached in intel_edp_init_dpcd(). > > But intel_dp_short_pulse() compares the old count with the read one > if there is a mistmatch a full port detection will be executed, what > was happening in the first short pulse interruption of eDP panels > that sets sink count. > > Instead of just skip the compasison for eDP panels, lets not read > the sink count at all for eDP. > > v2: the previous version of this patch was caching the sink count > in intel_edp_init_dpcd() but I was pointed out by Ville a patch that > handled a case of a eDP panel that do not set sink count > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 44 +++++++++++++++++++-------------- > 1 file changed, 26 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 13ff89be6ad6..1826d491efd7 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4034,8 +4034,6 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp) > static bool > intel_dp_get_dpcd(struct intel_dp *intel_dp) > { > - u8 sink_count; > - > if (!intel_dp_read_dpcd(intel_dp)) > return false; > > @@ -4045,25 +4043,35 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) > intel_dp_set_common_rates(intel_dp); > } > > - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &sink_count) <= 0) > - return false; > - > /* > - * Sink count can change between short pulse hpd hence > - * a member variable in intel_dp will track any changes > - * between short pulse interrupts. > + * Some eDP panels do not set a valid value for sink count, that is why > + * it don't care about read it here and in intel_edp_init_dpcd(). Can't quite parse that. "Some eDP panels do not set a valid value for sink count, so we ignore it." or something like that perhaps. > */ > - intel_dp->sink_count = DP_GET_SINK_COUNT(sink_count); > + if (!intel_dp_is_edp(intel_dp)) { > + u8 count; > + ssize_t r; > > - /* > - * SINK_COUNT == 0 and DOWNSTREAM_PORT_PRESENT == 1 implies that > - * a dongle is present but no display. Unless we require to know > - * if a dongle is present or not, we don't need to update > - * downstream port information. So, an early return here saves > - * time from performing other operations which are not required. > - */ > - if (!intel_dp_is_edp(intel_dp) && !intel_dp->sink_count) > - return false; > + r = drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &count); > + if (r < 1) > + return false; My earlier suggestion was that we should keep reading this anyway because some cts maybe required it. Would at least avoid mixing in two functional changes into once patch. > + > + /* > + * Sink count can change between short pulse hpd hence > + * a member variable in intel_dp will track any changes > + * between short pulse interrupts. > + */ > + intel_dp->sink_count = DP_GET_SINK_COUNT(count); > + > + /* > + * SINK_COUNT == 0 and DOWNSTREAM_PORT_PRESENT == 1 implies that > + * a dongle is present but no display. Unless we require to know > + * if a dongle is present or not, we don't need to update > + * downstream port information. So, an early return here saves > + * time from performing other operations which are not required. > + */ > + if (!intel_dp->sink_count) > + return false; > + } > > if (!drm_dp_is_branch(intel_dp->dpcd)) > return true; /* native DP sink */ > -- > 2.19.1
On Thu, 2018-10-11 at 16:21 +0300, Ville Syrjälä wrote: > On Wed, Oct 10, 2018 at 05:41:23PM -0700, José Roberto de Souza > wrote: > > Some eDP panels do not set a valid sink count value and even for > > the > > ones that sets is should always be one for eDP, that is why it is > > not > > cached in intel_edp_init_dpcd(). > > > > But intel_dp_short_pulse() compares the old count with the read one > > if there is a mistmatch a full port detection will be executed, > > what > > was happening in the first short pulse interruption of eDP panels > > that sets sink count. > > > > Instead of just skip the compasison for eDP panels, lets not read > > the sink count at all for eDP. > > > > v2: the previous version of this patch was caching the sink count > > in intel_edp_init_dpcd() but I was pointed out by Ville a patch > > that > > handled a case of a eDP panel that do not set sink count > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 44 +++++++++++++++++++-------- > > ------ > > 1 file changed, 26 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > b/drivers/gpu/drm/i915/intel_dp.c > > index 13ff89be6ad6..1826d491efd7 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -4034,8 +4034,6 @@ intel_edp_init_dpcd(struct intel_dp > > *intel_dp) > > static bool > > intel_dp_get_dpcd(struct intel_dp *intel_dp) > > { > > - u8 sink_count; > > - > > if (!intel_dp_read_dpcd(intel_dp)) > > return false; > > > > @@ -4045,25 +4043,35 @@ intel_dp_get_dpcd(struct intel_dp > > *intel_dp) > > intel_dp_set_common_rates(intel_dp); > > } > > > > - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, > > &sink_count) <= 0) > > - return false; > > - > > /* > > - * Sink count can change between short pulse hpd hence > > - * a member variable in intel_dp will track any changes > > - * between short pulse interrupts. > > + * Some eDP panels do not set a valid value for sink count, > > that is why > > + * it don't care about read it here and in > > intel_edp_init_dpcd(). > > Can't quite parse that. > > "Some eDP panels do not set a valid value > for sink count, so we ignore it." > > or something like that perhaps. > > > */ > > - intel_dp->sink_count = DP_GET_SINK_COUNT(sink_count); > > + if (!intel_dp_is_edp(intel_dp)) { > > + u8 count; > > + ssize_t r; > > > > - /* > > - * SINK_COUNT == 0 and DOWNSTREAM_PORT_PRESENT == 1 implies > > that > > - * a dongle is present but no display. Unless we require to > > know > > - * if a dongle is present or not, we don't need to update > > - * downstream port information. So, an early return here saves > > - * time from performing other operations which are not > > required. > > - */ > > - if (!intel_dp_is_edp(intel_dp) && !intel_dp->sink_count) > > - return false; > > + r = drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, > > &count); > > + if (r < 1) > > + return false; > > My earlier suggestion was that we should keep reading this > anyway because some cts maybe required it. Would at least > avoid mixing in two functional changes into once patch. Documenting the outcome of IRC discussion so that we are on the same page - CTS does not test eDP, hence it is okay to skip reading sink count for eDP panels. > > > + > > + /* > > + * Sink count can change between short pulse hpd hence > > + * a member variable in intel_dp will track any changes > > + * between short pulse interrupts. > > + */ > > + intel_dp->sink_count = DP_GET_SINK_COUNT(count); > > + > > + /* > > + * SINK_COUNT == 0 and DOWNSTREAM_PORT_PRESENT == 1 > > implies that > > + * a dongle is present but no display. Unless we > > require to know > > + * if a dongle is present or not, we don't need to > > update > > + * downstream port information. So, an early return > > here saves > > + * time from performing other operations which are not > > required. > > + */ > > + if (!intel_dp->sink_count) > > + return false; > > + } > > > > if (!drm_dp_is_branch(intel_dp->dpcd)) > > return true; /* native DP sink */ > > -- > > 2.19.1 > >
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 13ff89be6ad6..1826d491efd7 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4034,8 +4034,6 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp) static bool intel_dp_get_dpcd(struct intel_dp *intel_dp) { - u8 sink_count; - if (!intel_dp_read_dpcd(intel_dp)) return false; @@ -4045,25 +4043,35 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) intel_dp_set_common_rates(intel_dp); } - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &sink_count) <= 0) - return false; - /* - * Sink count can change between short pulse hpd hence - * a member variable in intel_dp will track any changes - * between short pulse interrupts. + * Some eDP panels do not set a valid value for sink count, that is why + * it don't care about read it here and in intel_edp_init_dpcd(). */ - intel_dp->sink_count = DP_GET_SINK_COUNT(sink_count); + if (!intel_dp_is_edp(intel_dp)) { + u8 count; + ssize_t r; - /* - * SINK_COUNT == 0 and DOWNSTREAM_PORT_PRESENT == 1 implies that - * a dongle is present but no display. Unless we require to know - * if a dongle is present or not, we don't need to update - * downstream port information. So, an early return here saves - * time from performing other operations which are not required. - */ - if (!intel_dp_is_edp(intel_dp) && !intel_dp->sink_count) - return false; + r = drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &count); + if (r < 1) + return false; + + /* + * Sink count can change between short pulse hpd hence + * a member variable in intel_dp will track any changes + * between short pulse interrupts. + */ + intel_dp->sink_count = DP_GET_SINK_COUNT(count); + + /* + * SINK_COUNT == 0 and DOWNSTREAM_PORT_PRESENT == 1 implies that + * a dongle is present but no display. Unless we require to know + * if a dongle is present or not, we don't need to update + * downstream port information. So, an early return here saves + * time from performing other operations which are not required. + */ + if (!intel_dp->sink_count) + return false; + } if (!drm_dp_is_branch(intel_dp->dpcd)) return true; /* native DP sink */
Some eDP panels do not set a valid sink count value and even for the ones that sets is should always be one for eDP, that is why it is not cached in intel_edp_init_dpcd(). But intel_dp_short_pulse() compares the old count with the read one if there is a mistmatch a full port detection will be executed, what was happening in the first short pulse interruption of eDP panels that sets sink count. Instead of just skip the compasison for eDP panels, lets not read the sink count at all for eDP. v2: the previous version of this patch was caching the sink count in intel_edp_init_dpcd() but I was pointed out by Ville a patch that handled a case of a eDP panel that do not set sink count Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 44 +++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 18 deletions(-)