[Intel-gfx] [PATCH 2/2] drm/i915: Fix the MST PBN divider calculation

[Imre Deak]

On Mon, Jan 25, 2021 at 05:55:03PM -0500, Lyude Paul wrote:
> On Mon, 2021-01-25 at 23:04 +0200, Imre Deak wrote:
> > On Mon, Jan 25, 2021 at 02:24:58PM -0500, Lyude Paul wrote:
> > > On Mon, 2021-01-25 at 19:36 +0200, Imre Deak wrote:
> > > > Atm the driver will calculate a wrong MST timeslots/MTP (aka time unit)
> > > > value for MST streams if the link parameters (link rate or lane count)
> > > > are limited in a way independent of the sink capabilities (reported by
> > > > DPCD).
> > > > 
> > > > One example of such a limitation is when a MUX between the sink and
> > > > source connects only a limited number of lanes to the display and
> > > > connects the rest of the lanes to other peripherals (USB).
> > > > 
> > > > Another issue is that atm MST core calculates the divider based on the
> > > > backwards compatible DPCD (at address 0x0000) vs. the extended
> > > > capability info (at address 0x2200). This can result in leaving some
> > > > part of the MST BW unused (For instance in case of the WD19TB dock).
> > > > 
> > > > Fix the above two issues by calculating the PBN divider value based on
> > > > the rate and lane count link parameters that the driver uses for all
> > > > other computation.
> > > > 
> > > > Bugzilla: https://gitlab.freedesktop.org/drm/intel/-/issues/2977
> > > > Cc: Lyude Paul <lyude at redhat.com>
> > > > Cc: Ville Syrjala <ville.syrjala at intel.com>
> > > > Cc: <stable at vger.kernel.org>
> > > > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_dp_mst.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > index d6a1b961a0e8..b4621ed0127e 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > > > @@ -68,7 +68,9 @@ static int intel_dp_mst_compute_link_config(struct
> > > > intel_encoder *encoder,
> > > >  
> > > >                 slots = drm_dp_atomic_find_vcpi_slots(state, &intel_dp-
> > > > > mst_mgr,
> > > >                                                       connector->port,
> > > > -                                                     crtc_state->pbn, 0);
> > > > +                                                     crtc_state->pbn,
> > > > +                                                    
> > > > drm_dp_get_vc_payload_bw(crtc_state->port_clock,
> > > > +                                                                      
> > > 
> > > This patch looks fine, however you should take care to also update the
> > > documentation for drm_dp_atomic_find_vcpi_slots() so that it mentiones that
> > > pbn_div should be DSC aware but also is not exclusive to systems supporting
> > > DSC
> > > over MST (see the docs for the @pbn_div parameter)
> > 
> > I thought (as a follow-up work) that drm_dp_atomic_find_vcpi_slots() and
> > drm_dp_mst_allocate_vcpi() could be made more generic, requiring the
> > drivers to always pass in pbn_div. By that we could remove
> > mst_mgr::pbn_div, keeping only one copy of this value (the one passed to
> > the above functions).
> 
> I'm fine with that! The only thing I ask is (even though it's taken forever) we
> are eventually planning on making it so that we'll have MST helpers that can
> suggest changing the PBN divisor in order to implement link fallback retraining.
> As long as we're still able to make that work in the future, I'm totally fine
> with this.

I don't see a problem wrt. link retraining, pbn_div passed to the above
functions should just reflect the actual rate and lane count the link
was trained with.

> > > Thank you for doing this! I've been meaning to fix the WD19 issues for a
> > > while
> > > now but have been too bogged down by other stuff to spend any time on MST
> > > recently.
> > > 
> > > >         crtc_state->lane_count));
> > > >                 if (slots == -EDEADLK)
> > > >                         return slots;
> > > >                 if (slots >= 0)
> > > 
> > > -- 
> > > Sincerely,
> > >    Lyude Paul (she/her)
> > >    Software Engineer at Red Hat
> > >    
> > > Note: I deal with a lot of emails and have a lot of bugs on my plate. If
> > > you've
> > > asked me a question, are waiting for a review/merge on a patch, etc. and I
> > > haven't responded in a while, please feel free to send me another email to
> > > check
> > > on my status. I don't bite!
> > > 
> > 
> 
> -- 
> Sincerely,
>    Lyude Paul (she/her)
>    Software Engineer at Red Hat
>    
> Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
> asked me a question, are waiting for a review/merge on a patch, etc. and I
> haven't responded in a while, please feel free to send me another email to check
> on my status. I don't bite!
>