[Intel-gfx] [PATCH 06/10] drm/i915/icl: Mark TC port as safe when interruptions are disabled

[Ville Syrjälä]

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 at intel.com>
> > > > > Signed-off-by: José Roberto de Souza <jose.souza at 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.

-- 
Ville Syrjälä
Intel