[06/10] drm/i915/icl: Mark TC port as safe when interruptions are disabled
diff mbox series

Message ID 20181002175054.15010-6-jose.souza@intel.com
State New
Headers show
Series
  • [01/10] drm: Do not call drm_dp_cec_set_edid() while registering DP connectors
Related show

Commit Message

José Roberto de Souza Oct. 2, 2018, 5:50 p.m. UTC
Before enter in power saving states or before unload the driver
spec states that display driver is required to to mark TC ports as
safe so hardware can do the disconnection flow without wait for
display driver handshake.
When driver is resumed or loaded again, if the TC live state is
still set as connected driver will mark again TC port as not safe as
required by spec.

BSpec: 2175

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Ville Syrjälä Oct. 2, 2018, 8:35 p.m. UTC | #1
On Tue, Oct 02, 2018 at 10:50:50AM -0700, José Roberto de Souza wrote:
> Before enter in power saving states or before unload the driver
> spec states that display driver is required to to mark TC ports as
> safe so hardware can do the disconnection flow without wait for
> display driver handshake.
> When driver is resumed or loaded again, if the TC live state is
> still set as connected driver will mark again TC port as not safe as
> required by spec.
> 
> BSpec: 2175
> 
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 2e242270e270..58616690f45f 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3640,6 +3640,7 @@ static void gen11_irq_reset(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int pipe;
> +	u32 val;
>  
>  	I915_WRITE(GEN11_GFX_MSTR_IRQ, 0);
>  	POSTING_READ(GEN11_GFX_MSTR_IRQ);
> @@ -3661,6 +3662,15 @@ static void gen11_irq_reset(struct drm_device *dev)
>  
>  	if (HAS_PCH_ICP(dev_priv))
>  		GEN3_IRQ_RESET(SDE);
> +
> +	/*
> +	 * Mark TC ports as safe so hardware can do the disconnect flow without
> +	 * wait for driver to do the handshake
> +	 */
> +	val = I915_READ(PORT_TX_DFLEXDPCSSS);
> +	for (pipe = 0; pipe < 4; pipe++)
> +		val &= ~(DP_PHY_MODE_STATUS_NOT_SAFE(pipe));
> +	I915_WRITE(PORT_TX_DFLEXDPCSSS, val);

Why would we have to do this here? The driver should relinquish control
if and when it has shut down the pipes etc. Sounds like a bug if we're
hanging on when we have no need for the port.

>  }
>  
>  void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv,
> -- 
> 2.19.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
José Roberto de Souza Oct. 10, 2018, 12:55 a.m. UTC | #2
On Tue, 2018-10-02 at 23:35 +0300, Ville Syrjälä wrote:
> On Tue, Oct 02, 2018 at 10:50:50AM -0700, José Roberto de Souza
> wrote:
> > Before enter in power saving states or before unload the driver
> > spec states that display driver is required to to mark TC ports as
> > safe so hardware can do the disconnection flow without wait for
> > display driver handshake.
> > When driver is resumed or loaded again, if the TC live state is
> > still set as connected driver will mark again TC port as not safe
> > as
> > required by spec.
> > 
> > BSpec: 2175
> > 
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > b/drivers/gpu/drm/i915/i915_irq.c
> > index 2e242270e270..58616690f45f 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -3640,6 +3640,7 @@ static void gen11_irq_reset(struct drm_device
> > *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	int pipe;
> > +	u32 val;
> >  
> >  	I915_WRITE(GEN11_GFX_MSTR_IRQ, 0);
> >  	POSTING_READ(GEN11_GFX_MSTR_IRQ);
> > @@ -3661,6 +3662,15 @@ static void gen11_irq_reset(struct
> > drm_device *dev)
> >  
> >  	if (HAS_PCH_ICP(dev_priv))
> >  		GEN3_IRQ_RESET(SDE);
> > +
> > +	/*
> > +	 * Mark TC ports as safe so hardware can do the disconnect flow
> > without
> > +	 * wait for driver to do the handshake
> > +	 */
> > +	val = I915_READ(PORT_TX_DFLEXDPCSSS);
> > +	for (pipe = 0; pipe < 4; pipe++)
> > +		val &= ~(DP_PHY_MODE_STATUS_NOT_SAFE(pipe));
> > +	I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> 
> Why would we have to do this here? The driver should relinquish
> control
> if and when it has shut down the pipes etc. Sounds like a bug if
> we're
> hanging on when we have no need for the port.

Right now we take control and only release it when port is
disconnected.

You mean release it in intel_dp_link_down()?
I guess it could be moved to that function but we would need:
- make icl_tc_port_connected() not grab control in any further calls to
intel_digital_port_connected() when driver is not using it
- to add special case to handle MST
- release control when a error happen when doing the modeset

In my opinion that would make it way more complicated with no benefit.

Also keeping it in gen11_irq_reset() would give back to HW the control
when loading the module, fixing possible problems with a BIOS that do
not release control of it before handing out system to bootloader.

> 
> >  }
> >  
> >  void gen8_irq_power_well_post_enable(struct drm_i915_private
> > *dev_priv,
> > -- 
> > 2.19.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Oct. 10, 2018, 5:15 p.m. UTC | #3
On Wed, Oct 10, 2018 at 12:55:07AM +0000, Souza, Jose wrote:
> On Tue, 2018-10-02 at 23:35 +0300, Ville Syrjälä wrote:
> > On Tue, Oct 02, 2018 at 10:50:50AM -0700, José Roberto de Souza
> > wrote:
> > > Before enter in power saving states or before unload the driver
> > > spec states that display driver is required to to mark TC ports as
> > > safe so hardware can do the disconnection flow without wait for
> > > display driver handshake.
> > > When driver is resumed or loaded again, if the TC live state is
> > > still set as connected driver will mark again TC port as not safe
> > > as
> > > required by spec.
> > > 
> > > BSpec: 2175
> > > 
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_irq.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > > b/drivers/gpu/drm/i915/i915_irq.c
> > > index 2e242270e270..58616690f45f 100644
> > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > @@ -3640,6 +3640,7 @@ static void gen11_irq_reset(struct drm_device
> > > *dev)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > >  	int pipe;
> > > +	u32 val;
> > >  
> > >  	I915_WRITE(GEN11_GFX_MSTR_IRQ, 0);
> > >  	POSTING_READ(GEN11_GFX_MSTR_IRQ);
> > > @@ -3661,6 +3662,15 @@ static void gen11_irq_reset(struct
> > > drm_device *dev)
> > >  
> > >  	if (HAS_PCH_ICP(dev_priv))
> > >  		GEN3_IRQ_RESET(SDE);
> > > +
> > > +	/*
> > > +	 * Mark TC ports as safe so hardware can do the disconnect flow
> > > without
> > > +	 * wait for driver to do the handshake
> > > +	 */
> > > +	val = I915_READ(PORT_TX_DFLEXDPCSSS);
> > > +	for (pipe = 0; pipe < 4; pipe++)
> > > +		val &= ~(DP_PHY_MODE_STATUS_NOT_SAFE(pipe));
> > > +	I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> > 
> > Why would we have to do this here? The driver should relinquish
> > control
> > if and when it has shut down the pipes etc. Sounds like a bug if
> > we're
> > hanging on when we have no need for the port.
> 
> Right now we take control and only release it when port is
> disconnected.

Disconnection is totally asynchronous. Someone could be using the
port/aux for anything when the disconnect irq happens. Presumably
bad things will happen if we just go and yank the control away
when someone is doing stuff. I believe even the spec tells us
to properly shut things down during the disconnect flow and make
sure eg. the aux power wells have been fully shut down before
relinquishing control.
José Roberto de Souza Oct. 10, 2018, 5:23 p.m. UTC | #4
On Wed, 2018-10-10 at 20:15 +0300, Ville Syrjälä wrote:
> On Wed, Oct 10, 2018 at 12:55:07AM +0000, Souza, Jose wrote:
> > On Tue, 2018-10-02 at 23:35 +0300, Ville Syrjälä wrote:
> > > On Tue, Oct 02, 2018 at 10:50:50AM -0700, José Roberto de Souza
> > > wrote:
> > > > Before enter in power saving states or before unload the driver
> > > > spec states that display driver is required to to mark TC ports
> > > > as
> > > > safe so hardware can do the disconnection flow without wait for
> > > > display driver handshake.
> > > > When driver is resumed or loaded again, if the TC live state is
> > > > still set as connected driver will mark again TC port as not
> > > > safe
> > > > as
> > > > required by spec.
> > > > 
> > > > BSpec: 2175
> > > > 
> > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_irq.c | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > > > b/drivers/gpu/drm/i915/i915_irq.c
> > > > index 2e242270e270..58616690f45f 100644
> > > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > > @@ -3640,6 +3640,7 @@ static void gen11_irq_reset(struct
> > > > drm_device
> > > > *dev)
> > > >  {
> > > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > >  	int pipe;
> > > > +	u32 val;
> > > >  
> > > >  	I915_WRITE(GEN11_GFX_MSTR_IRQ, 0);
> > > >  	POSTING_READ(GEN11_GFX_MSTR_IRQ);
> > > > @@ -3661,6 +3662,15 @@ static void gen11_irq_reset(struct
> > > > drm_device *dev)
> > > >  
> > > >  	if (HAS_PCH_ICP(dev_priv))
> > > >  		GEN3_IRQ_RESET(SDE);
> > > > +
> > > > +	/*
> > > > +	 * Mark TC ports as safe so hardware can do the
> > > > disconnect flow
> > > > without
> > > > +	 * wait for driver to do the handshake
> > > > +	 */
> > > > +	val = I915_READ(PORT_TX_DFLEXDPCSSS);
> > > > +	for (pipe = 0; pipe < 4; pipe++)
> > > > +		val &= ~(DP_PHY_MODE_STATUS_NOT_SAFE(pipe));
> > > > +	I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> > > 
> > > Why would we have to do this here? The driver should relinquish
> > > control
> > > if and when it has shut down the pipes etc. Sounds like a bug if
> > > we're
> > > hanging on when we have no need for the port.
> > 
> > Right now we take control and only release it when port is
> > disconnected.
> 
> Disconnection is totally asynchronous. Someone could be using the
> port/aux for anything when the disconnect irq happens. Presumably
> bad things will happen if we just go and yank the control away
> when someone is doing stuff. I believe even the spec tells us
> to properly shut things down during the disconnect flow and make
> sure eg. the aux power wells have been fully shut down before
> relinquishing control.

In my understanding at the point of the gen11_irq_reset() call all DDI
DDI ports and aux channels are already off.

>
Ville Syrjälä Oct. 10, 2018, 5:52 p.m. UTC | #5
On Wed, Oct 10, 2018 at 05:23:30PM +0000, Souza, Jose wrote:
> On Wed, 2018-10-10 at 20:15 +0300, Ville Syrjälä wrote:
> > On Wed, Oct 10, 2018 at 12:55:07AM +0000, Souza, Jose wrote:
> > > On Tue, 2018-10-02 at 23:35 +0300, Ville Syrjälä wrote:
> > > > On Tue, Oct 02, 2018 at 10:50:50AM -0700, José Roberto de Souza
> > > > wrote:
> > > > > Before enter in power saving states or before unload the driver
> > > > > spec states that display driver is required to to mark TC ports
> > > > > as
> > > > > safe so hardware can do the disconnection flow without wait for
> > > > > display driver handshake.
> > > > > When driver is resumed or loaded again, if the TC live state is
> > > > > still set as connected driver will mark again TC port as not
> > > > > safe
> > > > > as
> > > > > required by spec.
> > > > > 
> > > > > BSpec: 2175
> > > > > 
> > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_irq.c | 10 ++++++++++
> > > > >  1 file changed, 10 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > > > > b/drivers/gpu/drm/i915/i915_irq.c
> > > > > index 2e242270e270..58616690f45f 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > > > @@ -3640,6 +3640,7 @@ static void gen11_irq_reset(struct
> > > > > drm_device
> > > > > *dev)
> > > > >  {
> > > > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > >  	int pipe;
> > > > > +	u32 val;
> > > > >  
> > > > >  	I915_WRITE(GEN11_GFX_MSTR_IRQ, 0);
> > > > >  	POSTING_READ(GEN11_GFX_MSTR_IRQ);
> > > > > @@ -3661,6 +3662,15 @@ static void gen11_irq_reset(struct
> > > > > drm_device *dev)
> > > > >  
> > > > >  	if (HAS_PCH_ICP(dev_priv))
> > > > >  		GEN3_IRQ_RESET(SDE);
> > > > > +
> > > > > +	/*
> > > > > +	 * Mark TC ports as safe so hardware can do the
> > > > > disconnect flow
> > > > > without
> > > > > +	 * wait for driver to do the handshake
> > > > > +	 */
> > > > > +	val = I915_READ(PORT_TX_DFLEXDPCSSS);
> > > > > +	for (pipe = 0; pipe < 4; pipe++)
> > > > > +		val &= ~(DP_PHY_MODE_STATUS_NOT_SAFE(pipe));
> > > > > +	I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> > > > 
> > > > Why would we have to do this here? The driver should relinquish
> > > > control
> > > > if and when it has shut down the pipes etc. Sounds like a bug if
> > > > we're
> > > > hanging on when we have no need for the port.
> > > 
> > > Right now we take control and only release it when port is
> > > disconnected.
> > 
> > Disconnection is totally asynchronous. Someone could be using the
> > port/aux for anything when the disconnect irq happens. Presumably
> > bad things will happen if we just go and yank the control away
> > when someone is doing stuff. I believe even the spec tells us
> > to properly shut things down during the disconnect flow and make
> > sure eg. the aux power wells have been fully shut down before
> > relinquishing control.
> 
> In my understanding at the point of the gen11_irq_reset() call all DDI
> DDI ports and aux channels are already off.

I'm not talking about gen11_irq_reset(). But if we are then that gets
called during driver load too and everything could be up and running.
Though I'm not sure what the BIOS/GOP will do w.r.t. the safe mode
knob.

Anyways, I don't think poking at some display stuff from irq_reset()
is a particularly clean apporoach. The irq code should only concern
itself with irqs. If we properly track which mode the port is in then
I presume we'd just put it back into the tbt mode when the last
typec mode user goes away. Or if we try to keep it in typec mode even
when there are no users (as some kind of optimimization perhaps?) then
we should probably switch it back to tbt mode during some display
suspend/shutdown sequence.
José Roberto de Souza Oct. 10, 2018, 6:27 p.m. UTC | #6
On Wed, 2018-10-10 at 20:52 +0300, Ville Syrjälä wrote:
> On Wed, Oct 10, 2018 at 05:23:30PM +0000, Souza, Jose wrote:
> > On Wed, 2018-10-10 at 20:15 +0300, Ville Syrjälä wrote:
> > > On Wed, Oct 10, 2018 at 12:55:07AM +0000, Souza, Jose wrote:
> > > > On Tue, 2018-10-02 at 23:35 +0300, Ville Syrjälä wrote:
> > > > > On Tue, Oct 02, 2018 at 10:50:50AM -0700, José Roberto de
> > > > > Souza
> > > > > wrote:
> > > > > > Before enter in power saving states or before unload the
> > > > > > driver
> > > > > > spec states that display driver is required to to mark TC
> > > > > > ports
> > > > > > as
> > > > > > safe so hardware can do the disconnection flow without wait
> > > > > > for
> > > > > > display driver handshake.
> > > > > > When driver is resumed or loaded again, if the TC live
> > > > > > state is
> > > > > > still set as connected driver will mark again TC port as
> > > > > > not
> > > > > > safe
> > > > > > as
> > > > > > required by spec.
> > > > > > 
> > > > > > BSpec: 2175
> > > > > > 
> > > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/i915_irq.c | 10 ++++++++++
> > > > > >  1 file changed, 10 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > > > > > b/drivers/gpu/drm/i915/i915_irq.c
> > > > > > index 2e242270e270..58616690f45f 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > > > > @@ -3640,6 +3640,7 @@ static void gen11_irq_reset(struct
> > > > > > drm_device
> > > > > > *dev)
> > > > > >  {
> > > > > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > > >  	int pipe;
> > > > > > +	u32 val;
> > > > > >  
> > > > > >  	I915_WRITE(GEN11_GFX_MSTR_IRQ, 0);
> > > > > >  	POSTING_READ(GEN11_GFX_MSTR_IRQ);
> > > > > > @@ -3661,6 +3662,15 @@ static void gen11_irq_reset(struct
> > > > > > drm_device *dev)
> > > > > >  
> > > > > >  	if (HAS_PCH_ICP(dev_priv))
> > > > > >  		GEN3_IRQ_RESET(SDE);
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * Mark TC ports as safe so hardware can do the
> > > > > > disconnect flow
> > > > > > without
> > > > > > +	 * wait for driver to do the handshake
> > > > > > +	 */
> > > > > > +	val = I915_READ(PORT_TX_DFLEXDPCSSS);
> > > > > > +	for (pipe = 0; pipe < 4; pipe++)
> > > > > > +		val &= ~(DP_PHY_MODE_STATUS_NOT_SAFE(pipe));
> > > > > > +	I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> > > > > 
> > > > > Why would we have to do this here? The driver should
> > > > > relinquish
> > > > > control
> > > > > if and when it has shut down the pipes etc. Sounds like a bug
> > > > > if
> > > > > we're
> > > > > hanging on when we have no need for the port.
> > > > 
> > > > Right now we take control and only release it when port is
> > > > disconnected.
> > > 
> > > Disconnection is totally asynchronous. Someone could be using the
> > > port/aux for anything when the disconnect irq happens. Presumably
> > > bad things will happen if we just go and yank the control away
> > > when someone is doing stuff. I believe even the spec tells us
> > > to properly shut things down during the disconnect flow and make
> > > sure eg. the aux power wells have been fully shut down before
> > > relinquishing control.
> > 
> > In my understanding at the point of the gen11_irq_reset() call all
> > DDI
> > DDI ports and aux channels are already off.
> 
> I'm not talking about gen11_irq_reset(). But if we are then that gets
> called during driver load too and everything could be up and running.
> Though I'm not sure what the BIOS/GOP will do w.r.t. the safe mode
> knob.

BIOS should do the same connection flow to use sinks in tc ports.

> 
> Anyways, I don't think poking at some display stuff from irq_reset()
> is a particularly clean apporoach. The irq code should only concern

Move it to a function? Or move everything reseting display irq to a
function like is done for gen11_gt_irq_reset().

> itself with irqs. If we properly track which mode the port is in then
> I presume we'd just put it back into the tbt mode when the last
> typec mode user goes away. Or if we try to keep it in typec mode even
> when there are no users (as some kind of optimimization perhaps?)
> then
> we should probably switch it back to tbt mode during some display
> suspend/shutdown sequence.

We don't control what mode the connector is in, HW is the one that tell
us if it is in: type-c, TBT, legacy or disconnected state our only job
is grab and release this knob.

>
Ville Syrjälä Oct. 10, 2018, 6:38 p.m. UTC | #7
On Wed, Oct 10, 2018 at 06:27:07PM +0000, Souza, Jose wrote:
> On Wed, 2018-10-10 at 20:52 +0300, Ville Syrjälä wrote:
> > On Wed, Oct 10, 2018 at 05:23:30PM +0000, Souza, Jose wrote:
> > > On Wed, 2018-10-10 at 20:15 +0300, Ville Syrjälä wrote:
> > > > On Wed, Oct 10, 2018 at 12:55:07AM +0000, Souza, Jose wrote:
> > > > > On Tue, 2018-10-02 at 23:35 +0300, Ville Syrjälä wrote:
> > > > > > On Tue, Oct 02, 2018 at 10:50:50AM -0700, José Roberto de
> > > > > > Souza
> > > > > > wrote:
> > > > > > > Before enter in power saving states or before unload the
> > > > > > > driver
> > > > > > > spec states that display driver is required to to mark TC
> > > > > > > ports
> > > > > > > as
> > > > > > > safe so hardware can do the disconnection flow without wait
> > > > > > > for
> > > > > > > display driver handshake.
> > > > > > > When driver is resumed or loaded again, if the TC live
> > > > > > > state is
> > > > > > > still set as connected driver will mark again TC port as
> > > > > > > not
> > > > > > > safe
> > > > > > > as
> > > > > > > required by spec.
> > > > > > > 
> > > > > > > BSpec: 2175
> > > > > > > 
> > > > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > > > > ---
> > > > > > >  drivers/gpu/drm/i915/i915_irq.c | 10 ++++++++++
> > > > > > >  1 file changed, 10 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > > > > > > b/drivers/gpu/drm/i915/i915_irq.c
> > > > > > > index 2e242270e270..58616690f45f 100644
> > > > > > > --- a/drivers/gpu/drm/i915/i915_irq.c
> > > > > > > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > > > > > > @@ -3640,6 +3640,7 @@ static void gen11_irq_reset(struct
> > > > > > > drm_device
> > > > > > > *dev)
> > > > > > >  {
> > > > > > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > > > >  	int pipe;
> > > > > > > +	u32 val;
> > > > > > >  
> > > > > > >  	I915_WRITE(GEN11_GFX_MSTR_IRQ, 0);
> > > > > > >  	POSTING_READ(GEN11_GFX_MSTR_IRQ);
> > > > > > > @@ -3661,6 +3662,15 @@ static void gen11_irq_reset(struct
> > > > > > > drm_device *dev)
> > > > > > >  
> > > > > > >  	if (HAS_PCH_ICP(dev_priv))
> > > > > > >  		GEN3_IRQ_RESET(SDE);
> > > > > > > +
> > > > > > > +	/*
> > > > > > > +	 * Mark TC ports as safe so hardware can do the
> > > > > > > disconnect flow
> > > > > > > without
> > > > > > > +	 * wait for driver to do the handshake
> > > > > > > +	 */
> > > > > > > +	val = I915_READ(PORT_TX_DFLEXDPCSSS);
> > > > > > > +	for (pipe = 0; pipe < 4; pipe++)
> > > > > > > +		val &= ~(DP_PHY_MODE_STATUS_NOT_SAFE(pipe));
> > > > > > > +	I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> > > > > > 
> > > > > > Why would we have to do this here? The driver should
> > > > > > relinquish
> > > > > > control
> > > > > > if and when it has shut down the pipes etc. Sounds like a bug
> > > > > > if
> > > > > > we're
> > > > > > hanging on when we have no need for the port.
> > > > > 
> > > > > Right now we take control and only release it when port is
> > > > > disconnected.
> > > > 
> > > > Disconnection is totally asynchronous. Someone could be using the
> > > > port/aux for anything when the disconnect irq happens. Presumably
> > > > bad things will happen if we just go and yank the control away
> > > > when someone is doing stuff. I believe even the spec tells us
> > > > to properly shut things down during the disconnect flow and make
> > > > sure eg. the aux power wells have been fully shut down before
> > > > relinquishing control.
> > > 
> > > In my understanding at the point of the gen11_irq_reset() call all
> > > DDI
> > > DDI ports and aux channels are already off.
> > 
> > I'm not talking about gen11_irq_reset(). But if we are then that gets
> > called during driver load too and everything could be up and running.
> > Though I'm not sure what the BIOS/GOP will do w.r.t. the safe mode
> > knob.
> 
> BIOS should do the same connection flow to use sinks in tc ports.
> 
> > 
> > Anyways, I don't think poking at some display stuff from irq_reset()
> > is a particularly clean apporoach. The irq code should only concern
> 
> Move it to a function? Or move everything reseting display irq to a
> function like is done for gen11_gt_irq_reset().
> 
> > itself with irqs. If we properly track which mode the port is in then
> > I presume we'd just put it back into the tbt mode when the last
> > typec mode user goes away. Or if we try to keep it in typec mode even
> > when there are no users (as some kind of optimimization perhaps?)
> > then
> > we should probably switch it back to tbt mode during some display
> > suspend/shutdown sequence.
> 
> We don't control what mode the connector is in, HW is the one that tell
> us if it is in: type-c, TBT, legacy or disconnected state our only job
> is grab and release this knob.

So s/tbt mode/safe mode/ or whatever. Semantics.

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 2e242270e270..58616690f45f 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3640,6 +3640,7 @@  static void gen11_irq_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int pipe;
+	u32 val;
 
 	I915_WRITE(GEN11_GFX_MSTR_IRQ, 0);
 	POSTING_READ(GEN11_GFX_MSTR_IRQ);
@@ -3661,6 +3662,15 @@  static void gen11_irq_reset(struct drm_device *dev)
 
 	if (HAS_PCH_ICP(dev_priv))
 		GEN3_IRQ_RESET(SDE);
+
+	/*
+	 * Mark TC ports as safe so hardware can do the disconnect flow without
+	 * wait for driver to do the handshake
+	 */
+	val = I915_READ(PORT_TX_DFLEXDPCSSS);
+	for (pipe = 0; pipe < 4; pipe++)
+		val &= ~(DP_PHY_MODE_STATUS_NOT_SAFE(pipe));
+	I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
 }
 
 void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv,