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

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

On Wed, Dec 04, 2019 at 06:48:42PM +0000, Souza, Jose wrote:
> On Wed, 2019-12-04 at 12:55 +0200, Ville Syrjälä wrote:
> > 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      |  1
> > > > > > > > > > > 0 +-
> > > > > > > > > > >  drivers/gpu/drm/i915/display/intel_display.c  |  5
> > > > > > > > > > > 8
> > > > > > > > > > > ++++++-
> > > > > > > > > > >  .../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.
> 
> Just added some debug messages to skl_commit_modeset_enables(), changed
> the resolution of the 2 pipes enabled in the same MST stream and
> old_crtc_state->wm.skl.ddb is set kept set with the old values.
> 
> So should we fix that first loop in skl_commit_modeset_enables()?
> 
> for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> new_crtc_state, i)
> 	/* ignore allocations for crtc's that have been turned off. */
> 	if (!needs_modeset(new_crtc_state) && new_crtc_state-
> >hw.active)
> 		entries[i] = old_crtc_state->wm.skl.ddb;

I thought this is what it already did, but I guess I was wrong. Yes,
I think this is what it should do.

> In this case there is not difference between old_crtc_state->wm.skl.ddb 
> or new_crtc_state->wm.skl.ddb, so maybe also change to new_crtc_state?

I think it's better to keep using the old crtc state here. Just in case
someone comes up with a good reason to do ddb reallocation across pipes
when there isn't a modeset.

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