[Intel-gfx] [PATCH 3/7] drm/i915/tgl: Select master trasconder for MST stream

[Ville Syrjälä]

On Mon, Dec 02, 2019 at 10:03:38PM +0000, Souza, Jose wrote:
> On Thu, 2019-11-28 at 14:06 +0200, Ville Syrjälä wrote:
> > On Thu, Nov 28, 2019 at 01:14:37AM +0000, Souza, Jose wrote:
> > > On Wed, 2019-11-27 at 21:59 +0200, Ville Syrjälä wrote:
> > > > On Tue, Nov 26, 2019 at 08:30:31PM +0000, Souza, Jose wrote:
> > > > > On Tue, 2019-11-26 at 22:05 +0200, Ville Syrjälä wrote:
> > > > > > On Fri, Nov 22, 2019 at 04:54:55PM -0800, José Roberto de
> > > > > > Souza
> > > > > > wrote:
> > > > > > > On TGL the blending of all the streams have moved from DDI
> > > > > > > to
> > > > > > > transcoder, so now every transcoder working over the same
> > > > > > > MST
> > > > > > > port
> > > > > > > must
> > > > > > > send its stream to a master transcoder and master will send
> > > > > > > to
> > > > > > > DDI
> > > > > > > respecting the time slots.
> > > > > > > 
> > > > > > > A previous approach was using the lowest pipe/transcoder as
> > > > > > > master
> > > > > > > transcoder but as the comment in
> > > > > > > skl_commit_modeset_enables()
> > > > > > > states,
> > > > > > > that is not always true.
> > > > > > > 
> > > > > > > So here promoting the first pipe/transcoder of the stream
> > > > > > > as
> > > > > > > master.
> > > > > > > That caused several other problems as during the commit
> > > > > > > phase
> > > > > > > the
> > > > > > > state computed should not be changed.
> > > > > > > 
> > > > > > > So the master transcoder is store into intel_dp and the
> > > > > > > modeset
> > > > > > > in
> > > > > > > slave pipes/transcoders is forced using
> > > > > > > mst_master_trans_pending.
> > > > > > > 
> > > > > > > v2:
> > > > > > > - added missing config compute to trigger fullmodeset in
> > > > > > > slave
> > > > > > > transcoders
> > > > > > > 
> > > > > > > BSpec: 50493
> > > > > > > BSpec: 49190
> > > > > > > Cc: Ville Syrjälä <ville.syrjala at linux.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      |  10 +-
> > > > > > >  drivers/gpu/drm/i915/display/intel_display.c  |  58
> > > > > > > ++++++-
> > > > > > >  .../drm/i915/display/intel_display_types.h    |   3 +
> > > > > > >  drivers/gpu/drm/i915/display/intel_dp.c       |   1 +
> > > > > > >  drivers/gpu/drm/i915/display/intel_dp_mst.c   | 149
> > > > > > > +++++++++++++++++-
> > > > > > >  drivers/gpu/drm/i915/display/intel_dp_mst.h   |   2 +
> > > > > > >  6 files changed, 216 insertions(+), 7 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > > > > b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > > > > index a976606d21c7..d2f0d393d3ee 100644
> > > > > > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > > > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > > > > > @@ -35,6 +35,7 @@
> > > > > > >  #include "intel_display_types.h"
> > > > > > >  #include "intel_dp.h"
> > > > > > >  #include "intel_dp_link_training.h"
> > > > > > > +#include "intel_dp_mst.h"
> > > > > > >  #include "intel_dpio_phy.h"
> > > > > > >  #include "intel_dsi.h"
> > > > > > >  #include "intel_fifo_underrun.h"
> > > > > > > @@ -1903,8 +1904,13 @@
> > > > > > > intel_ddi_transcoder_func_reg_val_get(const
> > > > > > > struct intel_crtc_state *crtc_state)
> > > > > > >  		temp |= TRANS_DDI_MODE_SELECT_DP_MST;
> > > > > > >  		temp |= DDI_PORT_WIDTH(crtc_state->lane_count);
> > > > > > >  
> > > > > > > -		if (INTEL_GEN(dev_priv) >= 12)
> > > > > > > -			temp |=
> > > > > > > TRANS_DDI_MST_TRANSPORT_SELECT(crtc_state->cpu_transcoder);
> > > > > > > +		if (INTEL_GEN(dev_priv) >= 12) {
> > > > > > > +			enum transcoder master;
> > > > > > > +
> > > > > > > +			master =
> > > > > > > intel_dp_mst_master_trans_get(encoder);
> > > > > > 
> > > > > > Why isn't that just stored in the crtc state like everything
> > > > > > else?
> > > > > > 
> > > > > > I'm thinking we should maybe do it just like port sync and
> > > > > > have
> > > > > > both
> > > > > > master + slave_mask. That way it should be pretty trivial to
> > > > > > add
> > > > > > all
> > > > > > the relevant crtcs to the state when needed.
> > > > > 
> > > > > I guess port sync is not doing the right thing and it could
> > > > > cause
> > > > > underruns.
> > > > > When it is going to enable the master CRTC of the port sync it
> > > > > forcibly
> > > > > enables the slave first, what could cause underruns because of
> > > > > overlap
> > > > > in ddb allocations(that is what I understood from the comment
> > > > > in
> > > > > skl_commit_modeset_enables()).
> > > > 
> > > > Not necessarily just underruns but even a system hang. The fix
> > > > should
> > > > be
> > > > a trivial "check the slave for ddb overlap as well", but
> > > > apparently I
> > > > failed at convicing people to do that.
> > > > 
> > > > I've actually been pondering about decoupling the plane updates
> > > > from
> > > > the crtc enable stuff entirely. At least theoretically crtc
> > > > enable
> > > > should be able to excute in any order as long we don't enable any
> > > > new planes.
> > > > 
> > > > But none of that really matters for the discussion at hand.
> > > > Though
> > > > there are other problems with the port sync stuff that would need
> > > > to be handled better. Eg. I think it now adds both crtcs to the
> > > > state
> > > > always which is going to cut the fps in half. Also the place
> > > > where
> > > > it does that stuff is rather suspicious. All that stuff should be
> > > > somewhere a bit higher up IMO.
> > > > 
> > > > > So for MST we only know who is the master in the commit phase
> > > > > and
> > > > > at
> > > > > this point we should not modify the computed state.
> > > > 
> > > > I'm not suggesting modifying anything during commit phase. I
> > > > think
> > > > you are effectiely doing that right now by stuffing that mst
> > > > master
> > > > transcoder into intel_dp.
> > > 
> > > Sorry, I still don't get what approach are you suggesting here.
> > > 
> > > If we can't modify the state in the commit phase, adding
> > > mst_master_transcoder in the CRTC state will not be possible while
> > > respecting the order imposed by ddb allocations.
> > 
> > The ddb allocation ordering only comes into play when there are
> > already active pipes. It should always be possible to not enable
> > the slaves until the master has been shuffled into place in the 
> > ddb and enabled.
> 
> This sounds contradictory to what you answered here: 
> https://lists.freedesktop.org/archives/intel-gfx/2019-November/221608.html
> 
> Will need to some testing to get the steps but I was able consistent to
> get to state were doing a full modeset in pipe A(mst master) caused the
> pipe B(mst slave) to enabled first because of the ddb allocations.
> 
> So can I or not do something like port sync does? And force the enable
> of master before the slaves? If possible, the comment in
> skl_commit_modeset_enables() will need some changes.

I suspect for the mst stuff we should do:

while_dirty_mst_masters() {
	if (!ddb_overlap)
		enable();
}
while_dirty_mst_slaves() {
	if (!ddb_overlap)
		enable();
}

-- 
Ville Syrjälä
Intel