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

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

On Tue, Dec 03, 2019 at 10:12:47PM +0000, Souza, Jose wrote:
> On Tue, 2019-12-03 at 14:47 +0200, Ville Syrjälä wrote:
> > 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@linux.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      |  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();
> > }
> 
> What about this case?
> 
> Pipe/transcoder A and B in the same MST stream
> 
> # old state - DDB allocation: AABBB
> mst master = transcoder A(computed in atomic check phase)
> entries[0].start = 0
> entries[0].end = 1
> entries[1].start = 2
> entries[1].end = 4
> 
> # new state - DDB allocation: AAABBB
> mst master = transcoder A(computed in atomic check phase)
> entries[0].start = 0
> entries[0].end = 2
> entries[1].start = 3
> entries[1].end = 5
> 
> while_dirty_mst_masters()
> 	first iteration: pipe A will overlap with old pipe B DDB

There won't be an old DDB allocation for a pipe if it's going
trough a modeset.

> allocation
> 	second iteration: pipe B is slave of A
> 	third iteration: ?
> 
> 
> > 

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