On Thu, Jan 02, 2025 at 12:30:34PM +0200, Jani Nikula wrote:
> On Tue, 31 Dec 2024, Imre Deak <imre.deak@xxxxxxxxx> wrote:
> > On Thu, Dec 19, 2024 at 11:33:56PM +0200, Jani Nikula wrote:
> >> Handle 128b/132b SST in intel_dp_mtp_tu_compute_config(). The remote
> >> bandwidth overhead and time slot allocation are only relevant for MST;
> >> SST only needs the local bandwidth and a check that 64 slots isn't
> >> exceeded.
> >>
> >> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> >> ---
> >> drivers/gpu/drm/i915/display/intel_dp_mst.c | 109 +++++++++++---------
> >> 1 file changed, 61 insertions(+), 48 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> >> index d08824d2fefe..3fbf9163272b 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> >> @@ -257,10 +257,7 @@ int intel_dp_mtp_tu_compute_config(struct intel_dp *intel_dp,
> >>
> >> for (bpp = max_bpp; bpp >= min_bpp; bpp -= step) {
> >> int local_bw_overhead;
> >> - int remote_bw_overhead;
> >> int link_bpp_x16;
> >> - int remote_tu;
> >> - fixed20_12 pbn;
> >>
> >> drm_dbg_kms(display->drm, "Trying bpp %d\n", bpp);
> >>
> >> @@ -269,57 +266,73 @@ int intel_dp_mtp_tu_compute_config(struct intel_dp *intel_dp,
> >>
> >> local_bw_overhead = intel_dp_mst_bw_overhead(crtc_state,
> >> false, dsc_slice_count, link_bpp_x16);
> >> - remote_bw_overhead = intel_dp_mst_bw_overhead(crtc_state,
> >> - true, dsc_slice_count, link_bpp_x16);
> >> -
> >> intel_dp_mst_compute_m_n(crtc_state,
> >> local_bw_overhead,
> >> link_bpp_x16,
> >> &crtc_state->dp_m_n);
> >>
> >> - /*
> >> - * The TU size programmed to the HW determines which slots in
> >> - * an MTP frame are used for this stream, which needs to match
> >> - * the payload size programmed to the first downstream branch
> >> - * device's payload table.
> >> - *
> >> - * Note that atm the payload's PBN value DRM core sends via
> >> - * the ALLOCATE_PAYLOAD side-band message matches the payload
> >> - * size (which it calculates from the PBN value) it programs
> >> - * to the first branch device's payload table. The allocation
> >> - * in the payload table could be reduced though (to
> >> - * crtc_state->dp_m_n.tu), provided that the driver doesn't
> >> - * enable SSC on the corresponding link.
> >> - */
> >> - pbn.full = dfixed_const(intel_dp_mst_calc_pbn(adjusted_mode->crtc_clock,
> >> - link_bpp_x16,
> >> - remote_bw_overhead));
> >> - remote_tu = DIV_ROUND_UP(pbn.full, pbn_div.full);
> >> -
> >> - /*
> >> - * Aligning the TUs ensures that symbols consisting of multiple
> >> - * (4) symbol cycles don't get split between two consecutive
> >> - * MTPs, as required by Bspec.
> >> - * TODO: remove the alignment restriction for 128b/132b links
> >> - * on some platforms, where Bspec allows this.
> >> - */
> >> - remote_tu = ALIGN(remote_tu, 4 / crtc_state->lane_count);
> >> -
> >> - /*
> >> - * Also align PBNs accordingly, since MST core will derive its
> >> - * own copy of TU from the PBN in drm_dp_atomic_find_time_slots().
> >> - * The above comment about the difference between the PBN
> >> - * allocated for the whole path and the TUs allocated for the
> >> - * first branch device's link also applies here.
> >> - */
> >> - pbn.full = remote_tu * pbn_div.full;
> >> -
> >> - drm_WARN_ON(display->drm, remote_tu < crtc_state->dp_m_n.tu);
> >> - crtc_state->dp_m_n.tu = remote_tu;
> >> + if (intel_dp->is_mst) {
> >> + int remote_bw_overhead;
> >> + int remote_tu;
> >> + fixed20_12 pbn;
> >
> > Nit: pbn_div is only used for MST, so would (calculate/look it up from
> > mst_state) here. Also this MST block could be in a separate function,
> > perhaps for symmetry with the SST part in a function too, but this could
> > be a follow-up as well. Either way:
>
> I guess I'm just not sure yet which direction this should be taken. It
> would be easy enough to add the functions, but is that the right
> division? How to handle pbn_div in that case?
It's only needed to calculate PBN, hence only for MST. So the MST helper
could just use
drm_atomic_get_new_mst_topology_state()->pbn_div
> How is UHBR SST DSC going to impact all this?
Calculating PBN is only needed for MST, so it doesn't change the SST DSC
handling.
> I know I'm kind of weaseling out of fixing this up front, but I also try
> to avoid adding stuff that we may have to back out of later.
>
> I think I'd let this be for now.
Ok.
> > Reviewed-by: Imre Deak <imre.deak@xxxxxxxxx>
>
> Thanks,
> Jani.
>
> >
> >> +
> >> + remote_bw_overhead = intel_dp_mst_bw_overhead(crtc_state,
> >> + true, dsc_slice_count, link_bpp_x16);
> >> +
> >> + /*
> >> + * The TU size programmed to the HW determines which slots in
> >> + * an MTP frame are used for this stream, which needs to match
> >> + * the payload size programmed to the first downstream branch
> >> + * device's payload table.
> >> + *
> >> + * Note that atm the payload's PBN value DRM core sends via
> >> + * the ALLOCATE_PAYLOAD side-band message matches the payload
> >> + * size (which it calculates from the PBN value) it programs
> >> + * to the first branch device's payload table. The allocation
> >> + * in the payload table could be reduced though (to
> >> + * crtc_state->dp_m_n.tu), provided that the driver doesn't
> >> + * enable SSC on the corresponding link.
> >> + */
> >> + pbn.full = dfixed_const(intel_dp_mst_calc_pbn(adjusted_mode->crtc_clock,
> >> + link_bpp_x16,
> >> + remote_bw_overhead));
> >> + remote_tu = DIV_ROUND_UP(pbn.full, pbn_div.full);
> >> +
> >> + /*
> >> + * Aligning the TUs ensures that symbols consisting of multiple
> >> + * (4) symbol cycles don't get split between two consecutive
> >> + * MTPs, as required by Bspec.
> >> + * TODO: remove the alignment restriction for 128b/132b links
> >> + * on some platforms, where Bspec allows this.
> >> + */
> >> + remote_tu = ALIGN(remote_tu, 4 / crtc_state->lane_count);
> >> +
> >> + /*
> >> + * Also align PBNs accordingly, since MST core will derive its
> >> + * own copy of TU from the PBN in drm_dp_atomic_find_time_slots().
> >> + * The above comment about the difference between the PBN
> >> + * allocated for the whole path and the TUs allocated for the
> >> + * first branch device's link also applies here.
> >> + */
> >> + pbn.full = remote_tu * pbn_div.full;
> >> +
> >> + drm_WARN_ON(display->drm, remote_tu < crtc_state->dp_m_n.tu);
> >> + crtc_state->dp_m_n.tu = remote_tu;
> >> +
> >> + slots = drm_dp_atomic_find_time_slots(state, &intel_dp->mst_mgr,
> >> + connector->port,
> >> + dfixed_trunc(pbn));
> >> + } else {
> >> + /* Same as above for remote_tu */
> >> + crtc_state->dp_m_n.tu = ALIGN(crtc_state->dp_m_n.tu,
> >> + 4 / crtc_state->lane_count);
> >> +
> >> + if (crtc_state->dp_m_n.tu <= 64)
> >> + slots = crtc_state->dp_m_n.tu;
> >> + else
> >> + slots = -EINVAL;
> >> + }
> >>
> >> - slots = drm_dp_atomic_find_time_slots(state, &intel_dp->mst_mgr,
> >> - connector->port,
> >> - dfixed_trunc(pbn));
> >> if (slots == -EDEADLK)
> >> return slots;
> >>
> >> --
> >> 2.39.5
> >>
>
> --
> Jani Nikula, Intel
