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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx