Pushed the first one, thanks for the review Matt.
On Thu, 2020-04-16 at 16:10 +0000, Souza, Jose wrote:
> On Wed, 2020-04-15 at 20:56 -0700, Matt Roper wrote:
> > On Tue, Apr 14, 2020 at 04:04:40PM -0700, José Roberto de Souza
> > wrote:
> > > Right now dp.regs.dp_tp_ctl/status are only set during the
> > > encoder
> > > pre_enable() hook, what is causing all reads and writes to those
> > > registers to go to offset 0x0 before pre_enable() is executed.
> > >
> > > So if i915 takes the BIOS state and don't do a modeset any
> > > following
> > > link retraing will fail.
> > >
> > > In the case that i915 needs to do a modeset, the DDI disable
> > > sequence
> > > will write to a wrong register not disabling DP 'Transport
> > > Enable'
> > > in
> > > DP_TP_CTL, making a HDMI modeset in the same port/transcoder to
> > > not light up the monitor.
> >
> > So to clarify I understand the problematic sequence properly:
> > * i915 inherits already-enabled display from BIOS; pre_enable is
> > never
> > called
> > * we do a modeset, so we have to disable the display and then try
> > to
> > re-enable
> > - intel_disable_ddi_buf() writes to offset 0 rather than the
> > proper
> > register offset when attempting to reprogram DP_TP_CTL and
> > disable DP
> > - when we re-enable, the old, still-active DP settings in
> > hardware
> > cause problems and the display doesn't light up
> >
> > A couple clarifying questions:
> > - It seems like we should have seen unclaimed register warnings
> > from
> > the bogus writes to offset 0 during disable. Any idea why those
> > didn't show up?
>
> Offset 0x0 is valid BSpec: 29316
>
> > - In the commit message you mention that it's the DP Transport
> > Enable
> > that's the culprit here, which breaks future attempts to light
> > up
> > HDMI. I assume this means that we're also switching which pipe
> > we're driving the port with, not just doing any
> > modeset? Otherwise
> > DP would stay DP, HDMI would stay HDMI, and we wouldn't see this
> > problem with DP being active on an HDMI monitor (although we'd
> > still
> > be writing to an invalid register offset during our first DP
> > disable
> > which might cause other problems). Might be worth adding a
> > mention
> > of the pipe change to the commit message to clarify.
>
> Got this state in a system were BIOS is lighing up DP in pipe B when
> HDMI is also connected and leaving pipe A off, no idea why BIOS
> decided
> this.
> But when i915 takes over we do the modeset to light up HDMI and
> reorder the pipes.
>
> > The changes here look correct to me; we'll ensure the DP registers
> > have
> > proper offsets before we do our first modeset, and then the rest of
> > the
> > runtime behavior thereafter should be unchanged. So
> >
> > Reviewed-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> >
> >
> > > So here for GENs older than 12, that have those registers fixed
> > > at
> > > port offset range it is loading at encoder/port init while for
> > > GEN12
> > > it will keep setting it at encoder pre_enable() and during HW
> > > state
> > > readout.
> > >
> > > Fixes: 4444df6e205b ("drm/i915/tgl: move DP_TP_* to transcoder")
> > > Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > > Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/display/intel_ddi.c | 14 +++++++++++---
> > > drivers/gpu/drm/i915/display/intel_dp.c | 5 ++---
> > > 2 files changed, 13 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > index be6c61bcbc9c..1aab93a94f40 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > @@ -3252,9 +3252,6 @@ static void hsw_ddi_pre_enable_dp(struct
> > > intel_atomic_state *state,
> > > intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
> > > crtc_state->lane_count, is_mst);
> > >
> > > - intel_dp->regs.dp_tp_ctl = DP_TP_CTL(port);
> > > - intel_dp->regs.dp_tp_status = DP_TP_STATUS(port);
> > > -
> > > intel_edp_panel_on(intel_dp);
> > >
> > > intel_ddi_clk_select(encoder, crtc_state);
> > > @@ -4061,12 +4058,18 @@ void intel_ddi_get_config(struct
> > > intel_encoder *encoder,
> > > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config-
> > > > uapi.crtc);
> > > enum transcoder cpu_transcoder = pipe_config->cpu_transcoder;
> > > + struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > u32 temp, flags = 0;
> > >
> > > /* XXX: DSI transcoder paranoia */
> > > if (drm_WARN_ON(&dev_priv->drm,
> > > transcoder_is_dsi(cpu_transcoder)))
> > > return;
> > >
> > > + if (INTEL_GEN(dev_priv) >= 12) {
> > > + intel_dp->regs.dp_tp_ctl =
> > > TGL_DP_TP_CTL(cpu_transcoder);
> > > + intel_dp->regs.dp_tp_status =
> > > TGL_DP_TP_STATUS(cpu_transcoder);
> > > + }
> > > +
> > > intel_dsc_get_config(encoder, pipe_config);
> > >
> > > temp = intel_de_read(dev_priv,
> > > TRANS_DDI_FUNC_CTL(cpu_transcoder));
> > > @@ -4396,6 +4399,7 @@ static const struct drm_encoder_funcs
> > > intel_ddi_funcs = {
> > > static struct intel_connector *
> > > intel_ddi_init_dp_connector(struct intel_digital_port
> > > *intel_dig_port)
> > > {
> > > + struct drm_i915_private *dev_priv = to_i915(intel_dig_port-
> > > > base.base.dev);
> > > struct intel_connector *connector;
> > > enum port port = intel_dig_port->base.port;
> > >
> > > @@ -4406,6 +4410,10 @@ intel_ddi_init_dp_connector(struct
> > > intel_digital_port *intel_dig_port)
> > > intel_dig_port->dp.output_reg = DDI_BUF_CTL(port);
> > > intel_dig_port->dp.prepare_link_retrain =
> > > intel_ddi_prepare_link_retrain;
> > > + if (INTEL_GEN(dev_priv) < 12) {
> > > + intel_dig_port->dp.regs.dp_tp_ctl = DP_TP_CTL(port);
> > > + intel_dig_port->dp.regs.dp_tp_status =
> > > DP_TP_STATUS(port);
> > > + }
> > >
> > > if (!intel_dp_init_connector(intel_dig_port, connector)) {
> > > kfree(connector);
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > > b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index d4fcc9583869..03591ab76b0d 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -2671,9 +2671,6 @@ static void intel_dp_prepare(struct
> > > intel_encoder *encoder,
> > > intel_crtc_has_type(pipe_config,
> > > INTEL_OUTPUT_DP_MS
> > > T));
> > >
> > > - intel_dp->regs.dp_tp_ctl = DP_TP_CTL(port);
> > > - intel_dp->regs.dp_tp_status = DP_TP_STATUS(port);
> > > -
> > > /*
> > > * There are four kinds of DP registers:
> > > *
> > > @@ -8470,6 +8467,8 @@ bool intel_dp_init(struct drm_i915_private
> > > *dev_priv,
> > >
> > > intel_dig_port->dp.output_reg = output_reg;
> > > intel_dig_port->max_lanes = 4;
> > > + intel_dig_port->dp.regs.dp_tp_ctl = DP_TP_CTL(port);
> > > + intel_dig_port->dp.regs.dp_tp_status = DP_TP_STATUS(port);
> > >
> > > intel_encoder->type = INTEL_OUTPUT_DP;
> > > intel_encoder->power_domain = intel_port_to_power_domain(port);
> > > --
> > > 2.26.0
> > >
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
