| Message ID | 20180330222336.5262-7-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 30, 2018 at 03:23:32PM -0700, José Roberto de Souza wrote: > When PSR/PSR2 is enabled hardware can do aux ch transactions by it > self. > Spec requires that PSR is inactive before do any aux ch transaction > in HSW and BDW, for skl+ there is a aux ch mutex but the use is not > recommended. > So exiting PSR/PSR2 and waiting the transition to inactive to prevent > any concurrent access between hardware and software in aux ch > registers. > > VLV and CHV hardware don't do any aux as software is responsible to > do all the buffer tracking and it sends the wake up aux ch message > to sink. > ahh cool... I get back what I said on some previous patch. I like where it is going, but... > BSpec: 7530, 799 and 7532 > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 38 ++++++++++++++++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > drivers/gpu/drm/i915/intel_psr.c | 8 ++++++++ > 3 files changed, 48 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 62f82c4298ac..fedee4e7ed24 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1062,6 +1062,41 @@ static uint32_t skl_get_aux_send_ctl(struct intel_dp *intel_dp, > DP_AUX_CH_CTL_SYNC_PULSE_SKL(32); > } > > +/** > + * intel_dp_aux_ch_get - Get total control of aux ch registers > + * > + * By exiting or disabling any hardware feature that can also use the aux ch > + * registers at the same time as driver, this function will give total control > + * of aux ch registers to driver. > + */ > +static void intel_dp_aux_ch_get(struct intel_dp *intel_dp) > +{ > + if (!intel_dp->exit_psr_on_aux_ch_xfer) > + return; > + > + intel_psr_activate_block_get(intel_dp); > + intel_psr_exit(intel_dp, true); decision on exit and activate should be inside the block_get and block_put, based on current state of the counters. > +} > + > +/** > + * intel_dp_aux_ch_put - Release aux ch registers control > + * > + * It is the intel_dp_aux_ch_get() counterpart. > + */ > +static void intel_dp_aux_ch_put(struct intel_dp *intel_dp) > +{ > + if (!intel_dp->exit_psr_on_aux_ch_xfer) > + return; > + > + intel_psr_activate_block_put(intel_dp); > + /* Usually more than just one aux ch transaction is executed when > + * handling some event, activating PSR right way would cause several > + * msecs of delay waiting PSR to exit for each aux ch transaction, so > + * here asking it to be scheduled. > + */ > + intel_psr_activate(intel_dp, true); > +} > + > static int > intel_dp_aux_xfer(struct intel_dp *intel_dp, > const uint8_t *send, int send_bytes, > @@ -1101,6 +1136,8 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp, > > intel_dp_check_edp(intel_dp); > > + intel_dp_aux_ch_get(intel_dp); > + > /* Try to wait for any previous AUX channel activity */ > for (try = 0; try < 3; try++) { > status = I915_READ_NOTRACE(ch_ctl); > @@ -1223,6 +1260,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp, > > ret = recv_bytes; > out: > + intel_dp_aux_ch_put(intel_dp); > pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE); > > if (vdd) > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 020b96324135..177478f0b032 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1139,6 +1139,8 @@ struct intel_dp { > > /* Displayport compliance testing */ > struct intel_dp_compliance compliance; > + > + bool exit_psr_on_aux_ch_xfer; > }; > > struct intel_lspcon { > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index 8702dbafb42d..f88f12246a23 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -665,6 +665,13 @@ void intel_psr_enable(struct intel_dp *intel_dp, > msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5)); > } > > + /* From all platforms that supports PSR/PSR2 this 2 is the ones that > + * don't have restrictions about use of the aux ch while PSR/PSR2 is > + * enabled. > + */ > + if (!(IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))) > + intel_dp->exit_psr_on_aux_ch_xfer = true; > + > unlock: > mutex_unlock(&dev_priv->psr.lock); > } > @@ -732,6 +739,7 @@ void intel_psr_disable(struct intel_dp *intel_dp, > > dev_priv->psr.disable_source(intel_dp); > > + intel_dp->exit_psr_on_aux_ch_xfer = false; > /* Disable PSR on Sink */ > drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0); > > -- > 2.16.3 >
On Mon, 2018-04-02 at 11:23 -0700, Rodrigo Vivi wrote: > On Fri, Mar 30, 2018 at 03:23:32PM -0700, José Roberto de Souza wrote: > > When PSR/PSR2 is enabled hardware can do aux ch transactions by it > > self. > > Spec requires that PSR is inactive before do any aux ch transaction > > in HSW and BDW, for skl+ there is a aux ch mutex but the use is not > > recommended. > > So exiting PSR/PSR2 and waiting the transition to inactive to prevent > > any concurrent access between hardware and software in aux ch > > registers. > > > > VLV and CHV hardware don't do any aux as software is responsible to > > do all the buffer tracking and it sends the wake up aux ch message > > to sink. > > > > ahh cool... I get back what I said on some previous patch. > I like where it is going, but... > > > BSpec: 7530, 799 and 7532 > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 38 ++++++++++++++++++++++++++++++++++++++ > > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > > drivers/gpu/drm/i915/intel_psr.c | 8 ++++++++ > > 3 files changed, 48 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 62f82c4298ac..fedee4e7ed24 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1062,6 +1062,41 @@ static uint32_t skl_get_aux_send_ctl(struct intel_dp *intel_dp, > > DP_AUX_CH_CTL_SYNC_PULSE_SKL(32); > > } > > > > +/** > > + * intel_dp_aux_ch_get - Get total control of aux ch registers > > + * > > + * By exiting or disabling any hardware feature that can also use the aux ch > > + * registers at the same time as driver, this function will give total control > > + * of aux ch registers to driver. > > + */ > > +static void intel_dp_aux_ch_get(struct intel_dp *intel_dp) > > +{ > > + if (!intel_dp->exit_psr_on_aux_ch_xfer) > > + return; > > + > > + intel_psr_activate_block_get(intel_dp); > > + intel_psr_exit(intel_dp, true); > > decision on exit and activate should be inside the block_get and block_put, > based on current state of the counters. > I haven't read the patches yet, but you could use the kref infrastructure for reference counting. > > +} > > + > > +/** > > + * intel_dp_aux_ch_put - Release aux ch registers control > > + * > > + * It is the intel_dp_aux_ch_get() counterpart. > > + */ > > +static void intel_dp_aux_ch_put(struct intel_dp *intel_dp) > > +{ > > + if (!intel_dp->exit_psr_on_aux_ch_xfer) > > + return; > > + > > + intel_psr_activate_block_put(intel_dp); > > + /* Usually more than just one aux ch transaction is executed when > > + * handling some event, activating PSR right way would cause several > > + * msecs of delay waiting PSR to exit for each aux ch transaction, so > > + * here asking it to be scheduled. > > + */ > > + intel_psr_activate(intel_dp, true); > > +} > > + > > static int > > intel_dp_aux_xfer(struct intel_dp *intel_dp, > > const uint8_t *send, int send_bytes, > > @@ -1101,6 +1136,8 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp, > > > > intel_dp_check_edp(intel_dp); > > > > + intel_dp_aux_ch_get(intel_dp); > > + > > /* Try to wait for any previous AUX channel activity */ > > for (try = 0; try < 3; try++) { > > status = I915_READ_NOTRACE(ch_ctl); > > @@ -1223,6 +1260,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp, > > > > ret = recv_bytes; > > out: > > + intel_dp_aux_ch_put(intel_dp); > > pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE); > > > > if (vdd) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index 020b96324135..177478f0b032 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -1139,6 +1139,8 @@ struct intel_dp { > > > > /* Displayport compliance testing */ > > struct intel_dp_compliance compliance; > > + > > + bool exit_psr_on_aux_ch_xfer; > > }; > > > > struct intel_lspcon { > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > > index 8702dbafb42d..f88f12246a23 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -665,6 +665,13 @@ void intel_psr_enable(struct intel_dp *intel_dp, > > msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5)); > > } > > > > + /* From all platforms that supports PSR/PSR2 this 2 is the ones that > > + * don't have restrictions about use of the aux ch while PSR/PSR2 is > > + * enabled. > > + */ > > + if (!(IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))) > > + intel_dp->exit_psr_on_aux_ch_xfer = true; > > + > > unlock: > > mutex_unlock(&dev_priv->psr.lock); > > } > > @@ -732,6 +739,7 @@ void intel_psr_disable(struct intel_dp *intel_dp, > > > > dev_priv->psr.disable_source(intel_dp); > > > > + intel_dp->exit_psr_on_aux_ch_xfer = false; > > /* Disable PSR on Sink */ > > drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0); > > > > -- > > 2.16.3 > >
On Mon, 2018-04-02 at 11:23 -0700, Rodrigo Vivi wrote: > On Fri, Mar 30, 2018 at 03:23:32PM -0700, José Roberto de Souza wrote: > > When PSR/PSR2 is enabled hardware can do aux ch transactions by it > > self. > > Spec requires that PSR is inactive before do any aux ch transaction > > in HSW and BDW, for skl+ there is a aux ch mutex but the use is not > > recommended. > > So exiting PSR/PSR2 and waiting the transition to inactive to prevent > > any concurrent access between hardware and software in aux ch > > registers. Wouldn't the debugfs interface that you added to read sink PSR status always show PSR as disabled? Or do you intend to set intel_dp->exit_psr_on_aux_ch_xfer = true? > > > > VLV and CHV hardware don't do any aux as software is responsible to > > do all the buffer tracking and it sends the wake up aux ch message > > to sink. > > > > ahh cool... I get back what I said on some previous patch. > I like where it is going, but... > > > BSpec: 7530, 799 and 7532 > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 38 ++++++++++++++++++++++++++++++++++++++ > > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > > drivers/gpu/drm/i915/intel_psr.c | 8 ++++++++ > > 3 files changed, 48 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 62f82c4298ac..fedee4e7ed24 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -1062,6 +1062,41 @@ static uint32_t skl_get_aux_send_ctl(struct intel_dp *intel_dp, > > DP_AUX_CH_CTL_SYNC_PULSE_SKL(32); > > } > > > > +/** > > + * intel_dp_aux_ch_get - Get total control of aux ch registers > > + * > > + * By exiting or disabling any hardware feature that can also use the aux ch > > + * registers at the same time as driver, this function will give total control > > + * of aux ch registers to driver. > > + */ > > +static void intel_dp_aux_ch_get(struct intel_dp *intel_dp) > > +{ > > + if (!intel_dp->exit_psr_on_aux_ch_xfer) > > + return; > > + > > + intel_psr_activate_block_get(intel_dp); > > + intel_psr_exit(intel_dp, true); > > decision on exit and activate should be inside the block_get and block_put, > based on current state of the counters. > > > +} > > + > > +/** > > + * intel_dp_aux_ch_put - Release aux ch registers control > > + * > > + * It is the intel_dp_aux_ch_get() counterpart. > > + */ > > +static void intel_dp_aux_ch_put(struct intel_dp *intel_dp) > > +{ > > + if (!intel_dp->exit_psr_on_aux_ch_xfer) > > + return; > > + > > + intel_psr_activate_block_put(intel_dp); > > + /* Usually more than just one aux ch transaction is executed when > > + * handling some event, activating PSR right way would cause several > > + * msecs of delay waiting PSR to exit for each aux ch transaction, so > > + * here asking it to be scheduled. > > + */ > > + intel_psr_activate(intel_dp, true); > > +} > > + > > static int > > intel_dp_aux_xfer(struct intel_dp *intel_dp, > > const uint8_t *send, int send_bytes, > > @@ -1101,6 +1136,8 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp, > > > > intel_dp_check_edp(intel_dp); > > > > + intel_dp_aux_ch_get(intel_dp); > > + > > /* Try to wait for any previous AUX channel activity */ > > for (try = 0; try < 3; try++) { > > status = I915_READ_NOTRACE(ch_ctl); > > @@ -1223,6 +1260,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp, > > > > ret = recv_bytes; > > out: > > + intel_dp_aux_ch_put(intel_dp); > > pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE); > > > > if (vdd) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index 020b96324135..177478f0b032 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -1139,6 +1139,8 @@ struct intel_dp { > > > > /* Displayport compliance testing */ > > struct intel_dp_compliance compliance; > > + > > + bool exit_psr_on_aux_ch_xfer; > > }; > > > > struct intel_lspcon { > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > > index 8702dbafb42d..f88f12246a23 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -665,6 +665,13 @@ void intel_psr_enable(struct intel_dp *intel_dp, > > msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5)); > > } > > > > + /* From all platforms that supports PSR/PSR2 this 2 is the ones that > > + * don't have restrictions about use of the aux ch while PSR/PSR2 is > > + * enabled. > > + */ > > + if (!(IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))) > > + intel_dp->exit_psr_on_aux_ch_xfer = true; > > + > > unlock: > > mutex_unlock(&dev_priv->psr.lock); > > } > > @@ -732,6 +739,7 @@ void intel_psr_disable(struct intel_dp *intel_dp, > > > > dev_priv->psr.disable_source(intel_dp); > > > > + intel_dp->exit_psr_on_aux_ch_xfer = false; > > /* Disable PSR on Sink */ > > drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0); > > > > -- > > 2.16.3 > >
On Mon, 2018-04-02 at 12:00 -0700, Pandiyan, Dhinakaran wrote: > > > On Mon, 2018-04-02 at 11:23 -0700, Rodrigo Vivi wrote: > > On Fri, Mar 30, 2018 at 03:23:32PM -0700, José Roberto de Souza > > wrote: > > > When PSR/PSR2 is enabled hardware can do aux ch transactions by > > > it > > > self. > > > Spec requires that PSR is inactive before do any aux ch > > > transaction > > > in HSW and BDW, for skl+ there is a aux ch mutex but the use is > > > not > > > recommended. > > > So exiting PSR/PSR2 and waiting the transition to inactive to > > > prevent > > > any concurrent access between hardware and software in aux ch > > > registers. > > Wouldn't the debugfs interface that you added to read sink PSR status > always show PSR as disabled? Or do you intend to set > intel_dp->exit_psr_on_aux_ch_xfer = true? Yeah it will always be disabled, unless when the hack in 'drm/i915: Keep IGT PSR and frontbuffer tests functional' is set, Rodrigo asked to remove the debugfs file that was controlling the state of the hack and enable/disable it around CRC read so I could do that for the whole i915_edp_psr_status too. > > > > > > > > VLV and CHV hardware don't do any aux as software is responsible > > > to > > > do all the buffer tracking and it sends the wake up aux ch > > > message > > > to sink. > > > > > > > ahh cool... I get back what I said on some previous patch. > > I like where it is going, but... > > > > > BSpec: 7530, 799 and 7532 > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_dp.c | 38 > > > ++++++++++++++++++++++++++++++++++++++ > > > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > > > drivers/gpu/drm/i915/intel_psr.c | 8 ++++++++ > > > 3 files changed, 48 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > b/drivers/gpu/drm/i915/intel_dp.c > > > index 62f82c4298ac..fedee4e7ed24 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -1062,6 +1062,41 @@ static uint32_t > > > skl_get_aux_send_ctl(struct intel_dp *intel_dp, > > > DP_AUX_CH_CTL_SYNC_PULSE_SKL(32); > > > } > > > > > > +/** > > > + * intel_dp_aux_ch_get - Get total control of aux ch registers > > > + * > > > + * By exiting or disabling any hardware feature that can also > > > use the aux ch > > > + * registers at the same time as driver, this function will give > > > total control > > > + * of aux ch registers to driver. > > > + */ > > > +static void intel_dp_aux_ch_get(struct intel_dp *intel_dp) > > > +{ > > > + if (!intel_dp->exit_psr_on_aux_ch_xfer) > > > + return; > > > + > > > + intel_psr_activate_block_get(intel_dp); > > > + intel_psr_exit(intel_dp, true); > > > > decision on exit and activate should be inside the block_get and > > block_put, > > based on current state of the counters. > > > > > +} > > > + > > > +/** > > > + * intel_dp_aux_ch_put - Release aux ch registers control > > > + * > > > + * It is the intel_dp_aux_ch_get() counterpart. > > > + */ > > > +static void intel_dp_aux_ch_put(struct intel_dp *intel_dp) > > > +{ > > > + if (!intel_dp->exit_psr_on_aux_ch_xfer) > > > + return; > > > + > > > + intel_psr_activate_block_put(intel_dp); > > > + /* Usually more than just one aux ch transaction is > > > executed when > > > + * handling some event, activating PSR right way would > > > cause several > > > + * msecs of delay waiting PSR to exit for each aux ch > > > transaction, so > > > + * here asking it to be scheduled. > > > + */ > > > + intel_psr_activate(intel_dp, true); > > > +} > > > + > > > static int > > > intel_dp_aux_xfer(struct intel_dp *intel_dp, > > > const uint8_t *send, int send_bytes, > > > @@ -1101,6 +1136,8 @@ intel_dp_aux_xfer(struct intel_dp > > > *intel_dp, > > > > > > intel_dp_check_edp(intel_dp); > > > > > > + intel_dp_aux_ch_get(intel_dp); > > > + > > > /* Try to wait for any previous AUX channel activity */ > > > for (try = 0; try < 3; try++) { > > > status = I915_READ_NOTRACE(ch_ctl); > > > @@ -1223,6 +1260,7 @@ intel_dp_aux_xfer(struct intel_dp > > > *intel_dp, > > > > > > ret = recv_bytes; > > > out: > > > + intel_dp_aux_ch_put(intel_dp); > > > pm_qos_update_request(&dev_priv->pm_qos, > > > PM_QOS_DEFAULT_VALUE); > > > > > > if (vdd) > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > b/drivers/gpu/drm/i915/intel_drv.h > > > index 020b96324135..177478f0b032 100644 > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > @@ -1139,6 +1139,8 @@ struct intel_dp { > > > > > > /* Displayport compliance testing */ > > > struct intel_dp_compliance compliance; > > > + > > > + bool exit_psr_on_aux_ch_xfer; > > > }; > > > > > > struct intel_lspcon { > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > b/drivers/gpu/drm/i915/intel_psr.c > > > index 8702dbafb42d..f88f12246a23 100644 > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > @@ -665,6 +665,13 @@ void intel_psr_enable(struct intel_dp > > > *intel_dp, > > > msecs_to_jiffies(intel_dp- > > > >panel_power_cycle_delay * 5)); > > > } > > > > > > + /* From all platforms that supports PSR/PSR2 this 2 is > > > the ones that > > > + * don't have restrictions about use of the aux ch while > > > PSR/PSR2 is > > > + * enabled. > > > + */ > > > + if (!(IS_VALLEYVIEW(dev_priv) || > > > IS_CHERRYVIEW(dev_priv))) > > > + intel_dp->exit_psr_on_aux_ch_xfer = true; > > > + > > > unlock: > > > mutex_unlock(&dev_priv->psr.lock); > > > } > > > @@ -732,6 +739,7 @@ void intel_psr_disable(struct intel_dp > > > *intel_dp, > > > > > > dev_priv->psr.disable_source(intel_dp); > > > > > > + intel_dp->exit_psr_on_aux_ch_xfer = false; > > > /* Disable PSR on Sink */ > > > drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0); > > > > > > -- > > > 2.16.3 > > >
On Mon, 2018-04-02 at 11:42 -0700, Pandiyan, Dhinakaran wrote: > > > On Mon, 2018-04-02 at 11:23 -0700, Rodrigo Vivi wrote: > > On Fri, Mar 30, 2018 at 03:23:32PM -0700, José Roberto de Souza > > wrote: > > > When PSR/PSR2 is enabled hardware can do aux ch transactions by > > > it > > > self. > > > Spec requires that PSR is inactive before do any aux ch > > > transaction > > > in HSW and BDW, for skl+ there is a aux ch mutex but the use is > > > not > > > recommended. > > > So exiting PSR/PSR2 and waiting the transition to inactive to > > > prevent > > > any concurrent access between hardware and software in aux ch > > > registers. > > > > > > VLV and CHV hardware don't do any aux as software is responsible > > > to > > > do all the buffer tracking and it sends the wake up aux ch > > > message > > > to sink. > > > > > > > ahh cool... I get back what I said on some previous patch. > > I like where it is going, but... > > > > > BSpec: 7530, 799 and 7532 > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_dp.c | 38 > > > ++++++++++++++++++++++++++++++++++++++ > > > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > > > drivers/gpu/drm/i915/intel_psr.c | 8 ++++++++ > > > 3 files changed, 48 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > b/drivers/gpu/drm/i915/intel_dp.c > > > index 62f82c4298ac..fedee4e7ed24 100644 > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > @@ -1062,6 +1062,41 @@ static uint32_t > > > skl_get_aux_send_ctl(struct intel_dp *intel_dp, > > > DP_AUX_CH_CTL_SYNC_PULSE_SKL(32); > > > } > > > > > > +/** > > > + * intel_dp_aux_ch_get - Get total control of aux ch registers > > > + * > > > + * By exiting or disabling any hardware feature that can also > > > use the aux ch > > > + * registers at the same time as driver, this function will give > > > total control > > > + * of aux ch registers to driver. > > > + */ > > > +static void intel_dp_aux_ch_get(struct intel_dp *intel_dp) > > > +{ > > > + if (!intel_dp->exit_psr_on_aux_ch_xfer) > > > + return; > > > + > > > + intel_psr_activate_block_get(intel_dp); > > > + intel_psr_exit(intel_dp, true); > > > > decision on exit and activate should be inside the block_get and > > block_put, > > based on current state of the counters. > > > > I haven't read the patches yet, but you could use the kref > infrastructure for > reference counting. Cool I will use it, thanks. > > > > +} > > > + > > > +/** > > > + * intel_dp_aux_ch_put - Release aux ch registers control > > > + * > > > + * It is the intel_dp_aux_ch_get() counterpart. > > > + */ > > > +static void intel_dp_aux_ch_put(struct intel_dp *intel_dp) > > > +{ > > > + if (!intel_dp->exit_psr_on_aux_ch_xfer) > > > + return; > > > + > > > + intel_psr_activate_block_put(intel_dp); > > > + /* Usually more than just one aux ch transaction is > > > executed when > > > + * handling some event, activating PSR right way would > > > cause several > > > + * msecs of delay waiting PSR to exit for each aux ch > > > transaction, so > > > + * here asking it to be scheduled. > > > + */ > > > + intel_psr_activate(intel_dp, true); > > > +} > > > + > > > static int > > > intel_dp_aux_xfer(struct intel_dp *intel_dp, > > > const uint8_t *send, int send_bytes, > > > @@ -1101,6 +1136,8 @@ intel_dp_aux_xfer(struct intel_dp > > > *intel_dp, > > > > > > intel_dp_check_edp(intel_dp); > > > > > > + intel_dp_aux_ch_get(intel_dp); > > > + > > > /* Try to wait for any previous AUX channel activity */ > > > for (try = 0; try < 3; try++) { > > > status = I915_READ_NOTRACE(ch_ctl); > > > @@ -1223,6 +1260,7 @@ intel_dp_aux_xfer(struct intel_dp > > > *intel_dp, > > > > > > ret = recv_bytes; > > > out: > > > + intel_dp_aux_ch_put(intel_dp); > > > pm_qos_update_request(&dev_priv->pm_qos, > > > PM_QOS_DEFAULT_VALUE); > > > > > > if (vdd) > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > b/drivers/gpu/drm/i915/intel_drv.h > > > index 020b96324135..177478f0b032 100644 > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > @@ -1139,6 +1139,8 @@ struct intel_dp { > > > > > > /* Displayport compliance testing */ > > > struct intel_dp_compliance compliance; > > > + > > > + bool exit_psr_on_aux_ch_xfer; > > > }; > > > > > > struct intel_lspcon { > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > b/drivers/gpu/drm/i915/intel_psr.c > > > index 8702dbafb42d..f88f12246a23 100644 > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > @@ -665,6 +665,13 @@ void intel_psr_enable(struct intel_dp > > > *intel_dp, > > > msecs_to_jiffies(intel_dp- > > > >panel_power_cycle_delay * 5)); > > > } > > > > > > + /* From all platforms that supports PSR/PSR2 this 2 is > > > the ones that > > > + * don't have restrictions about use of the aux ch while > > > PSR/PSR2 is > > > + * enabled. > > > + */ > > > + if (!(IS_VALLEYVIEW(dev_priv) || > > > IS_CHERRYVIEW(dev_priv))) > > > + intel_dp->exit_psr_on_aux_ch_xfer = true; > > > + > > > unlock: > > > mutex_unlock(&dev_priv->psr.lock); > > > } > > > @@ -732,6 +739,7 @@ void intel_psr_disable(struct intel_dp > > > *intel_dp, > > > > > > dev_priv->psr.disable_source(intel_dp); > > > > > > + intel_dp->exit_psr_on_aux_ch_xfer = false; > > > /* Disable PSR on Sink */ > > > drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0); > > > > > > -- > > > 2.16.3 > > >
On Mon, 2018-04-02 at 22:16 +0000, Souza, Jose wrote: > On Mon, 2018-04-02 at 11:42 -0700, Pandiyan, Dhinakaran wrote: > > > > > > On Mon, 2018-04-02 at 11:23 -0700, Rodrigo Vivi wrote: > > > On Fri, Mar 30, 2018 at 03:23:32PM -0700, José Roberto de Souza > > > wrote: > > > > When PSR/PSR2 is enabled hardware can do aux ch transactions by > > > > it > > > > self. > > > > Spec requires that PSR is inactive before do any aux ch > > > > transaction > > > > in HSW and BDW, for skl+ there is a aux ch mutex but the use is > > > > not > > > > recommended. > > > > So exiting PSR/PSR2 and waiting the transition to inactive to > > > > prevent > > > > any concurrent access between hardware and software in aux ch > > > > registers. > > > > > > > > VLV and CHV hardware don't do any aux as software is > > > > responsible > > > > to > > > > do all the buffer tracking and it sends the wake up aux ch > > > > message > > > > to sink. > > > > > > > > > > ahh cool... I get back what I said on some previous patch. > > > I like where it is going, but... > > > > > > > BSpec: 7530, 799 and 7532 > > > > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_dp.c | 38 > > > > ++++++++++++++++++++++++++++++++++++++ > > > > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > > > > drivers/gpu/drm/i915/intel_psr.c | 8 ++++++++ > > > > 3 files changed, 48 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > > > b/drivers/gpu/drm/i915/intel_dp.c > > > > index 62f82c4298ac..fedee4e7ed24 100644 > > > > --- a/drivers/gpu/drm/i915/intel_dp.c > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > > > @@ -1062,6 +1062,41 @@ static uint32_t > > > > skl_get_aux_send_ctl(struct intel_dp *intel_dp, > > > > DP_AUX_CH_CTL_SYNC_PULSE_SKL(32); > > > > } > > > > > > > > +/** > > > > + * intel_dp_aux_ch_get - Get total control of aux ch registers > > > > + * > > > > + * By exiting or disabling any hardware feature that can also > > > > use the aux ch > > > > + * registers at the same time as driver, this function will > > > > give > > > > total control > > > > + * of aux ch registers to driver. > > > > + */ > > > > +static void intel_dp_aux_ch_get(struct intel_dp *intel_dp) > > > > +{ > > > > + if (!intel_dp->exit_psr_on_aux_ch_xfer) > > > > + return; > > > > + > > > > + intel_psr_activate_block_get(intel_dp); > > > > + intel_psr_exit(intel_dp, true); > > > > > > decision on exit and activate should be inside the block_get and > > > block_put, > > > based on current state of the counters. > > > > > > > I haven't read the patches yet, but you could use the kref > > infrastructure for > > reference counting. > > Cool I will use it, thanks. To use kref I would need to do something like this in intel_psr_init(): kref_init(&dev_priv->psr.activate_block_kref); /* kref_init() initialize the reference as 1, so calling kref_put() to * keep it as 0 but this will cause the release callback to be executed * but it will not be fully executed as PSR is not enabled yet. */ kref_put(&dev_priv->psr.activate_block_kref, intel_psr_activate_block_release); That is okay? keep it? > > > > > > > +} > > > > + > > > > +/** > > > > + * intel_dp_aux_ch_put - Release aux ch registers control > > > > + * > > > > + * It is the intel_dp_aux_ch_get() counterpart. > > > > + */ > > > > +static void intel_dp_aux_ch_put(struct intel_dp *intel_dp) > > > > +{ > > > > + if (!intel_dp->exit_psr_on_aux_ch_xfer) > > > > + return; > > > > + > > > > + intel_psr_activate_block_put(intel_dp); > > > > + /* Usually more than just one aux ch transaction is > > > > executed when > > > > + * handling some event, activating PSR right way would > > > > cause several > > > > + * msecs of delay waiting PSR to exit for each aux ch > > > > transaction, so > > > > + * here asking it to be scheduled. > > > > + */ > > > > + intel_psr_activate(intel_dp, true); > > > > +} > > > > + > > > > static int > > > > intel_dp_aux_xfer(struct intel_dp *intel_dp, > > > > const uint8_t *send, int send_bytes, > > > > @@ -1101,6 +1136,8 @@ intel_dp_aux_xfer(struct intel_dp > > > > *intel_dp, > > > > > > > > intel_dp_check_edp(intel_dp); > > > > > > > > + intel_dp_aux_ch_get(intel_dp); > > > > + > > > > /* Try to wait for any previous AUX channel activity > > > > */ > > > > for (try = 0; try < 3; try++) { > > > > status = I915_READ_NOTRACE(ch_ctl); > > > > @@ -1223,6 +1260,7 @@ intel_dp_aux_xfer(struct intel_dp > > > > *intel_dp, > > > > > > > > ret = recv_bytes; > > > > out: > > > > + intel_dp_aux_ch_put(intel_dp); > > > > pm_qos_update_request(&dev_priv->pm_qos, > > > > PM_QOS_DEFAULT_VALUE); > > > > > > > > if (vdd) > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > > b/drivers/gpu/drm/i915/intel_drv.h > > > > index 020b96324135..177478f0b032 100644 > > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > > @@ -1139,6 +1139,8 @@ struct intel_dp { > > > > > > > > /* Displayport compliance testing */ > > > > struct intel_dp_compliance compliance; > > > > + > > > > + bool exit_psr_on_aux_ch_xfer; > > > > }; > > > > > > > > struct intel_lspcon { > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > > > b/drivers/gpu/drm/i915/intel_psr.c > > > > index 8702dbafb42d..f88f12246a23 100644 > > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > > @@ -665,6 +665,13 @@ void intel_psr_enable(struct intel_dp > > > > *intel_dp, > > > > msecs_to_jiffies(intel_d > > > > p- > > > > > panel_power_cycle_delay * 5)); > > > > > > > > } > > > > > > > > + /* From all platforms that supports PSR/PSR2 this 2 is > > > > the ones that > > > > + * don't have restrictions about use of the aux ch > > > > while > > > > PSR/PSR2 is > > > > + * enabled. > > > > + */ > > > > + if (!(IS_VALLEYVIEW(dev_priv) || > > > > IS_CHERRYVIEW(dev_priv))) > > > > + intel_dp->exit_psr_on_aux_ch_xfer = true; > > > > + > > > > unlock: > > > > mutex_unlock(&dev_priv->psr.lock); > > > > } > > > > @@ -732,6 +739,7 @@ void intel_psr_disable(struct intel_dp > > > > *intel_dp, > > > > > > > > dev_priv->psr.disable_source(intel_dp); > > > > > > > > + intel_dp->exit_psr_on_aux_ch_xfer = false; > > > > /* Disable PSR on Sink */ > > > > drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0); > > > > > > > > -- > > > > 2.16.3 > > > > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 62f82c4298ac..fedee4e7ed24 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1062,6 +1062,41 @@ static uint32_t skl_get_aux_send_ctl(struct intel_dp *intel_dp, DP_AUX_CH_CTL_SYNC_PULSE_SKL(32); } +/** + * intel_dp_aux_ch_get - Get total control of aux ch registers + * + * By exiting or disabling any hardware feature that can also use the aux ch + * registers at the same time as driver, this function will give total control + * of aux ch registers to driver. + */ +static void intel_dp_aux_ch_get(struct intel_dp *intel_dp) +{ + if (!intel_dp->exit_psr_on_aux_ch_xfer) + return; + + intel_psr_activate_block_get(intel_dp); + intel_psr_exit(intel_dp, true); +} + +/** + * intel_dp_aux_ch_put - Release aux ch registers control + * + * It is the intel_dp_aux_ch_get() counterpart. + */ +static void intel_dp_aux_ch_put(struct intel_dp *intel_dp) +{ + if (!intel_dp->exit_psr_on_aux_ch_xfer) + return; + + intel_psr_activate_block_put(intel_dp); + /* Usually more than just one aux ch transaction is executed when + * handling some event, activating PSR right way would cause several + * msecs of delay waiting PSR to exit for each aux ch transaction, so + * here asking it to be scheduled. + */ + intel_psr_activate(intel_dp, true); +} + static int intel_dp_aux_xfer(struct intel_dp *intel_dp, const uint8_t *send, int send_bytes, @@ -1101,6 +1136,8 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp, intel_dp_check_edp(intel_dp); + intel_dp_aux_ch_get(intel_dp); + /* Try to wait for any previous AUX channel activity */ for (try = 0; try < 3; try++) { status = I915_READ_NOTRACE(ch_ctl); @@ -1223,6 +1260,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp, ret = recv_bytes; out: + intel_dp_aux_ch_put(intel_dp); pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE); if (vdd) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 020b96324135..177478f0b032 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1139,6 +1139,8 @@ struct intel_dp { /* Displayport compliance testing */ struct intel_dp_compliance compliance; + + bool exit_psr_on_aux_ch_xfer; }; struct intel_lspcon { diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index 8702dbafb42d..f88f12246a23 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -665,6 +665,13 @@ void intel_psr_enable(struct intel_dp *intel_dp, msecs_to_jiffies(intel_dp->panel_power_cycle_delay * 5)); } + /* From all platforms that supports PSR/PSR2 this 2 is the ones that + * don't have restrictions about use of the aux ch while PSR/PSR2 is + * enabled. + */ + if (!(IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))) + intel_dp->exit_psr_on_aux_ch_xfer = true; + unlock: mutex_unlock(&dev_priv->psr.lock); } @@ -732,6 +739,7 @@ void intel_psr_disable(struct intel_dp *intel_dp, dev_priv->psr.disable_source(intel_dp); + intel_dp->exit_psr_on_aux_ch_xfer = false; /* Disable PSR on Sink */ drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, 0);
When PSR/PSR2 is enabled hardware can do aux ch transactions by it self. Spec requires that PSR is inactive before do any aux ch transaction in HSW and BDW, for skl+ there is a aux ch mutex but the use is not recommended. So exiting PSR/PSR2 and waiting the transition to inactive to prevent any concurrent access between hardware and software in aux ch registers. VLV and CHV hardware don't do any aux as software is responsible to do all the buffer tracking and it sends the wake up aux ch message to sink. BSpec: 7530, 799 and 7532 Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 38 ++++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 2 ++ drivers/gpu/drm/i915/intel_psr.c | 8 ++++++++ 3 files changed, 48 insertions(+)