[Intel-gfx] [PATCH 1/3] drm/i915/display: Load DP_TP_CTL/STATUS offset before use it

[Souza, Jose]

On Fri, 2020-04-17 at 21:01 +0300, Ville Syrjälä wrote:
> On Wed, Apr 15, 2020 at 04:53:19PM +0000, Souza, Jose wrote:
> > On Tue, 2020-04-14 at 22:33 -0700, Lucas De Marchi wrote:
> > > On Tue, Apr 14, 2020 at 4:03 PM José Roberto de Souza
> > > <jose.souza at intel.com> 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 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 at intel.com>
> > > > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > > > ---
> > > >  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);
> > > 
> > > reason to be where it was is because of MST. I think what you are
> > > doing here will break it since now this is set for the port and
> > > not
> > > transcoder.
> > > intel_mst_pre_enable_dp() would call here only for the first
> > > stream,
> > > so all the others would use this same transcoder.
> > 
> > For TGL+ it moved to transcoder but for other it is still on port
> > and
> > it is kept in this patch. The fix here for TGL+ is load those 2
> > during
> > HW state readout.
> > Inside MST code it will continue to get from
> > intel_mst->primary.dp.
> 
> FYI looks like I have a reasonable way to get rid of this by finally 
> plumbing the crtc_state all the way down into link training code
> (has been on the TODO list for years). This should also allow us to
> elminate the encoder->type mess in the ddi_buf_trans code. And we
> get rid of some crtc->config stuff as well.

Cool and Chris was faster and already reviewed it.

> 
> MST retraining was the main obstacle left. I think I mostly solved
> it with the patch I just sent today. The one remaining issue I
> recall is the lane_mask for drm_dp_channel_eq_ok(). So far I don't
> have a better plan than to keep intel_dp->lane_count. But most of
> the other ad-hoc state under intel_dp can hopefully be nuked.

Okay but will merge this to fix issues that we have right now, please
CC me when you send those.

>