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@xxxxxxxxx>
> > > > > > > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > > > > > > ---
> > > > > > > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
