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

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

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@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@intel.com>
> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@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.

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.

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