| Message ID | 1451998226-21433-6-git-send-email-shubhangi.shrivastava@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote: > This patch reads sink_count dpcd always and removes its > read operation based on values in downstream port dpcd. > > SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT dpcd. > SINK_COUNT denotes if a display is attached, while > DOWNSTREAM_PORT_PRESET indicates how many ports are available > in the dongle where display can be attached. so it is possible > for sink count to change irrespective of value in downstream > port dpcd. > > Here is a table of possible values and scenarios > > sink_count downstream_port > present > 0 0 no display is attached > 0 1 dongle is connected without display > 1 0 display connected directly > 1 1 display connected through dongle > > Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com> > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> > Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index c2e8516..0d58bfd 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3865,6 +3865,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) > if (intel_dp->dpcd[DP_DPCD_REV] == 0) > return false; /* DPCD not present */ > > + if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT, > + &intel_dp->sink_count, 1) < 0) > + return false; > + > + if (!DP_GET_SINK_COUNT(intel_dp->sink_count)) > + return false; > + My understanding is that this function should only read the DPCD data while detection based on that data is done in intel_dp_detect_dpcd(). With the return on sink_count == 0 here, we skip the end of the function, which updates the cached downstream port information. Is there a reason why we need this early return here? Also, I think this could be squashed with the previous patch. Ander > /* Check if the panel supports PSR */ > memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd)); > if (is_edp(intel_dp)) { > @@ -4386,10 +4393,6 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp) > if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 && > intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) { > > - if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT, > - &intel_dp->sink_count, 1) < 0) > - return connector_status_unknown; > - > return DP_GET_SINK_COUNT(intel_dp->sink_count) ? > connector_status_connected : connector_status_disconnected; > }
On Thursday 14 January 2016 06:34 PM, Ander Conselvan De Oliveira wrote: > On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote: >> This patch reads sink_count dpcd always and removes its >> read operation based on values in downstream port dpcd. >> >> SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT dpcd. >> SINK_COUNT denotes if a display is attached, while >> DOWNSTREAM_PORT_PRESET indicates how many ports are available >> in the dongle where display can be attached. so it is possible >> for sink count to change irrespective of value in downstream >> port dpcd. >> >> Here is a table of possible values and scenarios >> >> sink_count downstream_port >> present >> 0 0 no display is attached >> 0 1 dongle is connected without display >> 1 0 display connected directly >> 1 1 display connected through dongle >> >> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com> >> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> >> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com> >> --- >> drivers/gpu/drm/i915/intel_dp.c | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index c2e8516..0d58bfd 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -3865,6 +3865,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) >> if (intel_dp->dpcd[DP_DPCD_REV] == 0) >> return false; /* DPCD not present */ >> >> + if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT, >> + &intel_dp->sink_count, 1) < 0) >> + return false; >> + >> + if (!DP_GET_SINK_COUNT(intel_dp->sink_count)) >> + return false; >> + > My understanding is that this function should only read the DPCD data while > detection based on that data is done in intel_dp_detect_dpcd(). With the return > on sink_count == 0 here, we skip the end of the function, which updates the > cached downstream port information. Is there a reason why we need this early > return here? > > Also, I think this could be squashed with the previous patch. > > Ander As described in the commit message, if sink_count is 0, then there is no display present. So, irrespective of value of downstream port, we should terminate the function and thus, an early return is present here. >> /* Check if the panel supports PSR */ >> memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd)); >> if (is_edp(intel_dp)) { >> @@ -4386,10 +4393,6 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp) >> if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 && >> intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) { >> >> - if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT, >> - &intel_dp->sink_count, 1) < 0) >> - return connector_status_unknown; >> - >> return DP_GET_SINK_COUNT(intel_dp->sink_count) ? >> connector_status_connected : connector_status_disconnected; >> }
On Monday 18 January 2016 06:14 PM, Shubhangi Shrivastava wrote: > > > On Thursday 14 January 2016 06:34 PM, Ander Conselvan De Oliveira wrote: >> On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote: >>> This patch reads sink_count dpcd always and removes its >>> read operation based on values in downstream port dpcd. >>> >>> SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT dpcd. >>> SINK_COUNT denotes if a display is attached, while >>> DOWNSTREAM_PORT_PRESET indicates how many ports are available >>> in the dongle where display can be attached. so it is possible >>> for sink count to change irrespective of value in downstream >>> port dpcd. >>> >>> Here is a table of possible values and scenarios >>> >>> sink_count downstream_port >>> present >>> 0 0 no display is attached >>> 0 1 dongle is connected without display >>> 1 0 display connected directly >>> 1 1 display connected through dongle >>> >>> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com> >>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> >>> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_dp.c | 11 +++++++---- >>> 1 file changed, 7 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_dp.c >>> b/drivers/gpu/drm/i915/intel_dp.c >>> index c2e8516..0d58bfd 100644 >>> --- a/drivers/gpu/drm/i915/intel_dp.c >>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>> @@ -3865,6 +3865,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) >>> if (intel_dp->dpcd[DP_DPCD_REV] == 0) >>> return false; /* DPCD not present */ >>> + if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT, >>> + &intel_dp->sink_count, 1) < 0) >>> + return false; >>> + >>> + if (!DP_GET_SINK_COUNT(intel_dp->sink_count)) >>> + return false; >>> + >> My understanding is that this function should only read the DPCD data >> while >> detection based on that data is done in intel_dp_detect_dpcd(). With >> the return >> on sink_count == 0 here, we skip the end of the function, which >> updates the >> cached downstream port information. Is there a reason why we need >> this early >> return here? >> >> Also, I think this could be squashed with the previous patch. >> >> Ander > As described in the commit message, if sink_count is 0, then there is > no display present. So, irrespective of value of downstream port, we > should terminate the function and thus, an early return is present here. > Squashing this patch with the previous one. >>> /* Check if the panel supports PSR */ >>> memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd)); >>> if (is_edp(intel_dp)) { >>> @@ -4386,10 +4393,6 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp) >>> if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 && >>> intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) { >>> - if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT, >>> - &intel_dp->sink_count, 1) < 0) >>> - return connector_status_unknown; >>> - >>> return DP_GET_SINK_COUNT(intel_dp->sink_count) ? >>> connector_status_connected : connector_status_disconnected; >>> } >
On Mon, 2016-01-18 at 18:14 +0530, Shubhangi Shrivastava wrote: > > On Thursday 14 January 2016 06:34 PM, Ander Conselvan De Oliveira wrote: > > On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote: > > > This patch reads sink_count dpcd always and removes its > > > read operation based on values in downstream port dpcd. > > > > > > SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT dpcd. > > > SINK_COUNT denotes if a display is attached, while > > > DOWNSTREAM_PORT_PRESET indicates how many ports are available > > > in the dongle where display can be attached. so it is possible > > > for sink count to change irrespective of value in downstream > > > port dpcd. > > > > > > Here is a table of possible values and scenarios > > > > > > sink_count downstream_port > > > present > > > 0 0 no display is attached > > > 0 1 dongle is connected without display > > > 1 0 display connected directly > > > 1 1 display connected through dongle > > > > > > Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com> > > > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> > > > Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_dp.c | 11 +++++++---- > > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > b/drivers/gpu/drm/i915/intel_dp.c > > > index c2e8516..0d58bfd 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -3865,6 +3865,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) > > > if (intel_dp->dpcd[DP_DPCD_REV] == 0) > > > return false; /* DPCD not present */ > > > > > > + if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT, > > > + &intel_dp->sink_count, 1) < 0) > > > + return false; > > > + > > > + if (!DP_GET_SINK_COUNT(intel_dp->sink_count)) > > > + return false; > > > + > > My understanding is that this function should only read the DPCD data while > > detection based on that data is done in intel_dp_detect_dpcd(). With the > > return > > on sink_count == 0 here, we skip the end of the function, which updates the > > cached downstream port information. Is there a reason why we need this early > > return here? > > > > Also, I think this could be squashed with the previous patch. > > > > Ander > As described in the commit message, if sink_count is 0, then there is no > display present. So, irrespective of value of downstream port, we should > terminate the function and thus, an early return is present here. You wrote that "SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT dpcd". Now, the get_dpcd() function is called from different places with the purpose of retrieving information stored in dpcd. By adding the early return, the downstream port information, which you claimed is independent from sink count, is not updated. The way I see it, you should terminate detection when sink count is 0, not the reading of DPCD. That way the logical split between intel_dp_get_dpcd() and intel_dp_detect_dpcd() is maintained. The former reads DPCD and the latter reasons about it. Ander > > > > /* Check if the panel supports PSR */ > > > memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd)); > > > if (is_edp(intel_dp)) { > > > @@ -4386,10 +4393,6 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp) > > > if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 && > > > intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) { > > > > > > - if (intel_dp_dpcd_read_wake(&intel_dp->aux, > > > DP_SINK_COUNT, > > > - &intel_dp->sink_count, 1) < > > > 0) > > > - return connector_status_unknown; > > > - > > > return DP_GET_SINK_COUNT(intel_dp->sink_count) ? > > > connector_status_connected : > > > connector_status_disconnected; > > > } >
On Monday 18 January 2016 06:30 PM, Ander Conselvan De Oliveira wrote: > On Mon, 2016-01-18 at 18:14 +0530, Shubhangi Shrivastava wrote: >> On Thursday 14 January 2016 06:34 PM, Ander Conselvan De Oliveira wrote: >>> On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote: >>>> This patch reads sink_count dpcd always and removes its >>>> read operation based on values in downstream port dpcd. >>>> >>>> SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT dpcd. >>>> SINK_COUNT denotes if a display is attached, while >>>> DOWNSTREAM_PORT_PRESET indicates how many ports are available >>>> in the dongle where display can be attached. so it is possible >>>> for sink count to change irrespective of value in downstream >>>> port dpcd. >>>> >>>> Here is a table of possible values and scenarios >>>> >>>> sink_count downstream_port >>>> present >>>> 0 0 no display is attached >>>> 0 1 dongle is connected without display >>>> 1 0 display connected directly >>>> 1 1 display connected through dongle >>>> >>>> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com> >>>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> >>>> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_dp.c | 11 +++++++---- >>>> 1 file changed, 7 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c >>>> b/drivers/gpu/drm/i915/intel_dp.c >>>> index c2e8516..0d58bfd 100644 >>>> --- a/drivers/gpu/drm/i915/intel_dp.c >>>> +++ b/drivers/gpu/drm/i915/intel_dp.c >>>> @@ -3865,6 +3865,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) >>>> if (intel_dp->dpcd[DP_DPCD_REV] == 0) >>>> return false; /* DPCD not present */ >>>> >>>> + if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT, >>>> + &intel_dp->sink_count, 1) < 0) >>>> + return false; >>>> + >>>> + if (!DP_GET_SINK_COUNT(intel_dp->sink_count)) >>>> + return false; >>>> + >>> My understanding is that this function should only read the DPCD data while >>> detection based on that data is done in intel_dp_detect_dpcd(). With the >>> return >>> on sink_count == 0 here, we skip the end of the function, which updates the >>> cached downstream port information. Is there a reason why we need this early >>> return here? >>> >>> Also, I think this could be squashed with the previous patch. >>> >>> Ander >> As described in the commit message, if sink_count is 0, then there is no >> display present. So, irrespective of value of downstream port, we should >> terminate the function and thus, an early return is present here. > You wrote that "SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT > dpcd". Now, the get_dpcd() function is called from different places with the > purpose of retrieving information stored in dpcd. By adding the early return, > the downstream port information, which you claimed is independent from sink > count, is not updated. > > The way I see it, you should terminate detection when sink count is 0, not the > reading of DPCD. That way the logical split between intel_dp_get_dpcd() and > intel_dp_detect_dpcd() is maintained. The former reads DPCD and the latter > reasons about it. > > Ander Yes, that's how it is.. But, 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. >>>> /* Check if the panel supports PSR */ >>>> memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd)); >>>> if (is_edp(intel_dp)) { >>>> @@ -4386,10 +4393,6 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp) >>>> if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 && >>>> intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) { >>>> >>>> - if (intel_dp_dpcd_read_wake(&intel_dp->aux, >>>> DP_SINK_COUNT, >>>> - &intel_dp->sink_count, 1) < >>>> 0) >>>> - return connector_status_unknown; >>>> - >>>> return DP_GET_SINK_COUNT(intel_dp->sink_count) ? >>>> connector_status_connected : >>>> connector_status_disconnected; >>>> }
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index c2e8516..0d58bfd 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3865,6 +3865,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) if (intel_dp->dpcd[DP_DPCD_REV] == 0) return false; /* DPCD not present */ + if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT, + &intel_dp->sink_count, 1) < 0) + return false; + + if (!DP_GET_SINK_COUNT(intel_dp->sink_count)) + return false; + /* Check if the panel supports PSR */ memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd)); if (is_edp(intel_dp)) { @@ -4386,10 +4393,6 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp) if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 && intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) { - if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT, - &intel_dp->sink_count, 1) < 0) - return connector_status_unknown; - return DP_GET_SINK_COUNT(intel_dp->sink_count) ? connector_status_connected : connector_status_disconnected; }
This patch reads sink_count dpcd always and removes its read operation based on values in downstream port dpcd. SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT dpcd. SINK_COUNT denotes if a display is attached, while DOWNSTREAM_PORT_PRESET indicates how many ports are available in the dongle where display can be attached. so it is possible for sink count to change irrespective of value in downstream port dpcd. Here is a table of possible values and scenarios sink_count downstream_port present 0 0 no display is attached 0 1 dongle is connected without display 1 0 display connected directly 1 1 display connected through dongle Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)